mybatis/mybatis-dynamic-sql

Render in(emptyList) and notIn(emptyList) more reasonably

pine-cones opened this issue · 4 comments

pine-cones commented

I have read through the previously published issues: #228 and #509, and I strongly agree with jesusslim's idea.

I can understand that you never want to build invalid sql, but in my opinion, once we use isIn, whatever param we pass, you should never change our sql, we don't want fixing, if you think our sql is invalid just throw exception, not change it to another wrong sql.

In my understanding, the configuration nonRenderingWhereClauseAllowed only takes effect when there are no where conditions at all. When I use logical deletion, there will definitely be a where condition present. However, using in(emptyList) will modify all data, which is a very dangerous operation.

// will render to sql: update tableName set enabled = 0 where enabled = 1
update(dsl -> dsl.set(enabled).equalTo(false).where(enabled, isEqualTo(true)).and(id, isIn(Collections.emptyList())));

I'm thinking, if the condition is isIn(emptyList), can it be render to false? Similarly, isNotIn(emptyList) can be render to true. This ensures both the syntax and semantics of SQL.

jeffgbutler commented

Rendering an empty list to true or false is just another kind of "changing your SQL" and IMHO it is also a very non-intuitive way of doing so.

What I would prefer is adding a configuration setting that would render empty lists as () which seems more intuitive to me, doesn't change your SQL, and relies on the database throwing an exception in this case.

So your example above would render to this:

update tableName set enabled = 0 where enabled = 1 and id in ()

What do you think?

pine-cones commented

I think it is good.

It is indeed a good thing that the library won't generate invalid SQL, but sometimes we have to make some compromises...

DanielLiu1123 commented

Consider setting the default value of emptyListConditionRenderingAllowed to true. This behavior is really dangerous. I believe developers would rather see a runtime exception than have data accidentally modified or deleted.

I believe that when encountering an empty list, Mybatis example also throws an exception. Throwing an exception allows us to find errors in the code before going live, rather than finding the bug after data has been wrongly modified or deleted.

BTW, this issue caused us to accidentally delete thousands of records in the production environment (now fixed). It took us half a day to identify this problem, and it's really easy to overlook.

jeffgbutler commented

I'm open to this idea, but it would probably break some existing code. I'd like to get some community feedback before proceeding.

Please make a new issue with this request and let's see if there are any comments from other users.

  • 👍1