Rendering empty list by default

[DanielLiu1123]

mybatis-dynamic-sql has the following default behavior:

delete(c -> c.where(id, isIn(emptyList())).and(deletedAt, isNull()));

will render:

delete from t where deleted_at is null;

instead of:

delete from t where id in () and deleted_at is null;

This can lead to accidental data deletion, and it's often hard to address this problem. It's the developers' mistake when they write such code, often because they don't check if the list is empty (which, in my opinion, is a common issue). If such a mistake leads to data being wrongly deleted, it can be disastrous.

If rendering in () would indeed cause bad sql at runtime, it would help developers quickly find the problem and realize they did not check if the list was empty.
If in () is not rendered, our code would just magically work, until this problem explodes. Then, you would get a lot of feedback from users, and then you would have to find and fix the problem (believe me, you wouldn't want to do this). Then you would check if the list was empty.

So, whether in () is rendered or not, developers need to do the same thing: check if the list is empty first. But the outcomes of these two methods are very different.

As for the framework, it should not change my SQL, even if it is wrong. MyBatis QBE (query by example) treats empty lists by rendering in (), not by ignoring them, The default behavior of mybatis-dynamic-sql and MyBatis QBE should align.

Many issues have mentioned this problem: #228, #509, #752.

In version 1.5.1, a configuration option was introduced to address this issue, emptyListConditionRenderingAllowed, whose default value is false. This means that the risk of this problem occurring still exists, especially for developers new to mybatis-dynamic-sql. So, I suggest changing the default value of emptyListConditionRenderingAllowed to true, which would be a more reasonable default behavior for the framework.

Finally, I want to say: mybatis-dynamic-sql is really great. I have used JOOQ and JPA, but this project is the most comfortable to use. I love the selective mode (ignores null values)!

[jeffgbutler]

I am open to this idea - it would be the safe choice. There could be some impact on existing code, but my sense is that is a relatively minor problem.

If anyone thinks differently, please comment on this issue. Unless there is a strong counter argument, I will implement this change.

[jeffgbutler]

@DanielLiu1123 Here's what I'm thinking I will do...

1. Set the "in" conditions so they always render. So isIn(emptyList) will always render as in()
2. Set the "in when present" conditions so they don't render if empty. So isInWhenPresent(emptyList) will not render.
3. Remove the new configuration setting about rendering empty lists

This is consistent with the other "when present" conditions, and it removes the need for the configuration setting - which is awkward at best. This also allows a user to clearly specify their intent in the code.

WDYT?

[DanielLiu1123]

LGTM.

But there's one thing to note: "when present" has different behaviors for different types. For non-collection types (e.g. String), it means rendering when not null. For collection types, it means rendering when not null and not empty. For the isInWhenPresent method, handling it this way is reasonable, but it might need additional comments to explain this behavior.

[jeffgbutler]

Thanks - you are correct. The "when present" methods will accept a null collection. The others like "isIn" will throw NPE for a null collection.

I'm planning on adding some documentation about this.