Skip to content

Commit f036631

Browse files
jeffgbutlerjeffgbutler
committed
Throw InvalidSqlException during rendering if an In condition is empty
This changes the prior behavior where the condition would render as invalid SQL and a runtime exception from the database was expected.
1 parent 06a52b9 commit f036631

File tree

11 files changed

+83
-176
lines changed

11 files changed

+83
-176
lines changed

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ Other important changes:
2626
- The library now requires Java 17
2727
- Deprecated code from prior releases is removed
2828
- We now allow CASE expressions in ORDER BY Clauses
29+
- The "In" conditions will now throw `InvalidSqlException` during rendering if the list of values is empty. Previously
30+
an empty In condition would render as invalid SQL and would usually cause a runtime exception from the database.
31+
With this change, the exception thrown is more predictable and the error is caught before sending the SQL to the
32+
database.
2933

3034
## Release 1.5.2 - June 3, 2024
3135

‎src/main/java/org/mybatis/dynamic/sql/util/Validator.java‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,20 @@ public static void assertNotEmpty(Collection<?> collection, String messageNumber
2626
assertFalse(collection.isEmpty(), messageNumber);
2727
}
2828

29+
public static void assertNotEmpty(Collection<?> collection, String messageNumber, String p1) {
30+
assertFalse(collection.isEmpty(), messageNumber, p1);
31+
}
32+
2933
public static void assertFalse(boolean condition, String messageNumber) {
30-
internalAssertFalse(condition, Messages.getString(messageNumber));
34+
if (condition) {
35+
throw new InvalidSqlException(Messages.getString(messageNumber));
36+
}
3137
}
3238

3339
public static void assertFalse(boolean condition, String messageNumber, String p1) {
34-
internalAssertFalse(condition, Messages.getString(messageNumber, p1));
40+
if (condition) {
41+
throw new InvalidSqlException(Messages.getString(messageNumber, p1));
42+
}
3543
}
3644

3745
public static void assertTrue(boolean condition, String messageNumber) {
@@ -41,10 +49,4 @@ public static void assertTrue(boolean condition, String messageNumber) {
4149
public static void assertTrue(boolean condition, String messageNumber, String p1) {
4250
assertFalse(!condition, messageNumber, p1);
4351
}
44-
45-
private static void internalAssertFalse(boolean condition, String message) {
46-
if (condition) {
47-
throw new InvalidSqlException(message);
48-
}
49-
}
5052
}

‎src/main/java/org/mybatis/dynamic/sql/where/condition/IsIn.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import org.mybatis.dynamic.sql.AbstractListValueCondition;
2525
import org.mybatis.dynamic.sql.render.RenderingContext;
26+
import org.mybatis.dynamic.sql.util.Validator;
2627

2728
public class IsIn<T> extends AbstractListValueCondition<T> {
2829
private static final IsIn<?> EMPTY = new IsIn<>(Collections.emptyList());
@@ -39,6 +40,7 @@ protected IsIn(Collection<T> values) {
3940

4041
@Override
4142
public boolean shouldRender(RenderingContext renderingContext) {
43+
Validator.assertNotEmpty(values, "ERROR.44", "IsIn"); //$NON-NLS-1$ //$NON-NLS-2$
4244
return true;
4345
}
4446

‎src/main/java/org/mybatis/dynamic/sql/where/condition/IsInCaseInsensitive.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.mybatis.dynamic.sql.AbstractListValueCondition;
2525
import org.mybatis.dynamic.sql.render.RenderingContext;
2626
import org.mybatis.dynamic.sql.util.StringUtilities;
27+
import org.mybatis.dynamic.sql.util.Validator;
2728

2829
public class IsInCaseInsensitive extends AbstractListValueCondition<String>
2930
implements CaseInsensitiveVisitableCondition {
@@ -39,6 +40,7 @@ protected IsInCaseInsensitive(Collection<String> values) {
3940

4041
@Override
4142
public boolean shouldRender(RenderingContext renderingContext) {
43+
Validator.assertNotEmpty(values, "ERROR.44", "IsInCaseInsensitive"); //$NON-NLS-1$ //$NON-NLS-2$
4244
return true;
4345
}
4446

‎src/main/java/org/mybatis/dynamic/sql/where/condition/IsNotIn.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import org.mybatis.dynamic.sql.AbstractListValueCondition;
2525
import org.mybatis.dynamic.sql.render.RenderingContext;
26+
import org.mybatis.dynamic.sql.util.Validator;
2627

2728
public class IsNotIn<T> extends AbstractListValueCondition<T> {
2829
private static final IsNotIn<?> EMPTY = new IsNotIn<>(Collections.emptyList());
@@ -39,6 +40,7 @@ protected IsNotIn(Collection<T> values) {
3940

4041
@Override
4142
public boolean shouldRender(RenderingContext renderingContext) {
43+
Validator.assertNotEmpty(values, "ERROR.44", "IsNotIn"); //$NON-NLS-1$ //$NON-NLS-2$
4244
return true;
4345
}
4446

‎src/main/java/org/mybatis/dynamic/sql/where/condition/IsNotInCaseInsensitive.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.mybatis.dynamic.sql.AbstractListValueCondition;
2525
import org.mybatis.dynamic.sql.render.RenderingContext;
2626
import org.mybatis.dynamic.sql.util.StringUtilities;
27+
import org.mybatis.dynamic.sql.util.Validator;
2728

2829
public class IsNotInCaseInsensitive extends AbstractListValueCondition<String>
2930
implements CaseInsensitiveVisitableCondition {
@@ -39,6 +40,7 @@ protected IsNotInCaseInsensitive(Collection<String> values) {
3940

4041
@Override
4142
public boolean shouldRender(RenderingContext renderingContext) {
43+
Validator.assertNotEmpty(values, "ERROR.44", "IsNotInCaseInsensitive"); //$NON-NLS-1$ //$NON-NLS-2$
4244
return true;
4345
}
4446

‎src/main/resources/org/mybatis/dynamic/sql/util/messages.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,5 @@ ERROR.40=Case expressions must have at least one "when" clause
6060
ERROR.41=You cannot call "then" in a Kotlin case expression more than once
6161
ERROR.42=You cannot call `else` in a Kotlin case expression more than once
6262
ERROR.43=A Kotlin cast expression must have one, and only one, `as` element
63+
ERROR.44={0} conditions must contain at least one value
6364
INTERNAL.ERROR=Internal Error {0}

‎src/site/markdown/docs/conditions.md‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,8 @@ mapping if you so desire.
204204

205205
Starting with version 1.5.2, we made a change to the rendering rules for the "in" conditions. This was done to limit the
206206
danger of conditions failing to render and thus affecting more rows than expected. For the base conditions ("isIn",
207-
"isNotIn", etc.), if the list of values is empty, then the condition will still render, but the resulting SQL will
208-
be invalid and will cause a runtime exception. We believe this is the safest outcome. For example, suppose
207+
"isNotIn", etc.), if the list of values is empty, then the library will throw
208+
`org.mybatis.dynamic.sql.exception.InvalidSqlException`. We believe this is the safest outcome. For example, suppose
209209
a DELETE statement was coded as follows:
210210

211211
```java
@@ -214,12 +214,6 @@ a DELETE statement was coded as follows:
214214
.and(id, isIn(Collections.emptyList()));
215215
```
216216

217-
This statement will be rendered as follows:
218-
219-
```sql
220-
delete from foo where status = ? and id in ()
221-
```
222-
223217
This will cause a runtime error due to invalid SQL, but it eliminates the possibility of deleting ALL rows with
224218
active status. If you want to allow the "in" condition to drop from the SQL if the list is empty, then use the
225219
"inWhenPresent" condition.
@@ -229,8 +223,8 @@ and the case-insensitive versions of these conditions:
229223

230224
| Input | Effect |
231225
|------------------------------------------|-----------------------------------------------------------------------------------|
232-
| isIn(null) | NullPointerException |
233-
| isIn(Collections.emptyList()) | Rendered as "in ()" (Invalid SQL) |
226+
| isIn(null) | NullPointerException thrown |
227+
| isIn(Collections.emptyList()) | InvalidSqlException thrown |
234228
| isIn(2, 3, null) | Rendered as "in (?, ?, ?)" (Parameter values are 2, 3, and null) |
235229
| isInWhenPresent(null) | Condition Not Rendered |
236230
| isInWhenPresent(Collections.emptyList()) | Condition Not Rendered |

‎src/test/java/examples/animal/data/VariousListConditionsTest.java‎

Lines changed: 56 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,12 @@
3434
import java.io.InputStreamReader;
3535
import java.sql.Connection;
3636
import java.sql.DriverManager;
37-
import java.sql.SQLSyntaxErrorException;
3837
import java.util.Collection;
3938
import java.util.Collections;
4039
import java.util.List;
4140
import java.util.Map;
4241

4342
import org.apache.ibatis.datasource.unpooled.UnpooledDataSource;
44-
import org.apache.ibatis.exceptions.PersistenceException;
4543
import org.apache.ibatis.jdbc.ScriptRunner;
4644
import org.apache.ibatis.mapping.Environment;
4745
import org.apache.ibatis.session.Configuration;
@@ -51,8 +49,10 @@
5149
import org.apache.ibatis.transaction.jdbc.JdbcTransactionFactory;
5250
import org.junit.jupiter.api.BeforeEach;
5351
import org.junit.jupiter.api.Test;
52+
import org.mybatis.dynamic.sql.exception.InvalidSqlException;
5453
import org.mybatis.dynamic.sql.render.RenderingStrategies;
5554
import org.mybatis.dynamic.sql.select.render.SelectStatementProvider;
55+
import org.mybatis.dynamic.sql.util.Messages;
5656
import org.mybatis.dynamic.sql.util.mybatis3.CommonSelectMapper;
5757

5858
class VariousListConditionsTest {
@@ -136,26 +136,15 @@ void testInWhenPresentWithNull() {
136136

137137
@Test
138138
void testInWithEmptyList() {
139-
try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
140-
CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
141-
142-
SelectStatementProvider selectStatement = select(id, animalName)
143-
.from(animalData)
144-
.where(id, isIn(Collections.emptyList()))
145-
.orderBy(id)
146-
.build()
147-
.render(RenderingStrategies.MYBATIS3);
148-
149-
assertThat(selectStatement.getSelectStatement()).isEqualTo(
150-
"select id, animal_name from AnimalData " +
151-
"where id in () " +
152-
"order by id"
153-
);
154-
155-
assertThatExceptionOfType(PersistenceException.class).isThrownBy(() ->
156-
mapper.selectManyMappedRows(selectStatement)
157-
).withCauseInstanceOf(SQLSyntaxErrorException.class);
158-
}
139+
var selectModel = select(id, animalName)
140+
.from(animalData)
141+
.where(id, isIn(Collections.emptyList()))
142+
.orderBy(id)
143+
.build();
144+
145+
assertThatExceptionOfType(InvalidSqlException.class)
146+
.isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
147+
.withMessage(Messages.getString("ERROR.44", "IsIn"));
159148
}
160149

161150
@Test
@@ -319,121 +308,66 @@ void testNotInCaseInsensitiveWhenPresentMap() {
319308

320309
@Test
321310
void testInEventuallyEmpty() {
322-
try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
323-
CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
324-
325-
SelectStatementProvider selectStatement = select(id, animalName)
326-
.from(animalData)
327-
.where(id, isIn(1, 2).filter(s -> false))
328-
.orderBy(id)
329-
.build()
330-
.render(RenderingStrategies.MYBATIS3);
331-
332-
assertThat(selectStatement.getSelectStatement()).isEqualTo(
333-
"select id, animal_name from AnimalData " +
334-
"where id in () " +
335-
"order by id"
336-
);
337-
338-
assertThatExceptionOfType(PersistenceException.class).isThrownBy(
339-
() -> mapper.selectManyMappedRows(selectStatement))
340-
.withCauseInstanceOf(SQLSyntaxErrorException.class);
341-
}
311+
var selectModel = select(id, animalName)
312+
.from(animalData)
313+
.where(id, isIn(1, 2).filter(s -> false))
314+
.orderBy(id)
315+
.build();
316+
317+
assertThatExceptionOfType(InvalidSqlException.class)
318+
.isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
319+
.withMessage(Messages.getString("ERROR.44", "IsIn"));
342320
}
343321

344322
@Test
345323
void testInCaseInsensitiveEventuallyEmpty() {
346-
try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
347-
CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
348-
349-
SelectStatementProvider selectStatement = select(id, animalName)
350-
.from(animalData)
351-
.where(animalName, isInCaseInsensitive("Fred", "Betty").filter(s -> false))
352-
.orderBy(id)
353-
.build()
354-
.render(RenderingStrategies.MYBATIS3);
355-
356-
assertThat(selectStatement.getSelectStatement()).isEqualTo(
357-
"select id, animal_name from AnimalData " +
358-
"where upper(animal_name) in () " +
359-
"order by id"
360-
);
361-
362-
assertThatExceptionOfType(PersistenceException.class).isThrownBy(
363-
() -> mapper.selectManyMappedRows(selectStatement))
364-
.withCauseInstanceOf(SQLSyntaxErrorException.class);
365-
}
324+
var selectModel = select(id, animalName)
325+
.from(animalData)
326+
.where(animalName, isInCaseInsensitive("Fred", "Betty").filter(s -> false))
327+
.orderBy(id)
328+
.build();
329+
330+
assertThatExceptionOfType(InvalidSqlException.class)
331+
.isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
332+
.withMessage(Messages.getString("ERROR.44", "IsInCaseInsensitive"));
366333
}
367334

368335
@Test
369336
void testNotInEventuallyEmpty() {
370-
try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
371-
CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
372-
373-
SelectStatementProvider selectStatement = select(id, animalName)
374-
.from(animalData)
375-
.where(id, isNotIn(1, 2).filter(s -> false))
376-
.orderBy(id)
377-
.build()
378-
.render(RenderingStrategies.MYBATIS3);
379-
380-
assertThat(selectStatement.getSelectStatement()).isEqualTo(
381-
"select id, animal_name from AnimalData " +
382-
"where id not in () " +
383-
"order by id"
384-
);
385-
386-
assertThatExceptionOfType(PersistenceException.class).isThrownBy(
387-
() -> mapper.selectManyMappedRows(selectStatement))
388-
.withCauseInstanceOf(SQLSyntaxErrorException.class);
389-
}
337+
var selectModel = select(id, animalName)
338+
.from(animalData)
339+
.where(id, isNotIn(1, 2).filter(s -> false))
340+
.orderBy(id)
341+
.build();
342+
343+
assertThatExceptionOfType(InvalidSqlException.class)
344+
.isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
345+
.withMessage(Messages.getString("ERROR.44", "IsNotIn"));
390346
}
391347

392348
@Test
393349
void testNotInCaseInsensitiveEventuallyEmpty() {
394-
try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
395-
CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
396-
397-
SelectStatementProvider selectStatement = select(id, animalName)
398-
.from(animalData)
399-
.where(animalName, isNotInCaseInsensitive("Fred", "Betty").filter(s -> false))
400-
.orderBy(id)
401-
.build()
402-
.render(RenderingStrategies.MYBATIS3);
403-
404-
assertThat(selectStatement.getSelectStatement()).isEqualTo(
405-
"select id, animal_name from AnimalData " +
406-
"where upper(animal_name) not in () " +
407-
"order by id"
408-
);
409-
410-
assertThatExceptionOfType(PersistenceException.class).isThrownBy(
411-
() -> mapper.selectManyMappedRows(selectStatement))
412-
.withCauseInstanceOf(SQLSyntaxErrorException.class);
413-
}
350+
var selectModel = select(id, animalName)
351+
.from(animalData)
352+
.where(animalName, isNotInCaseInsensitive("Fred", "Betty").filter(s -> false))
353+
.orderBy(id)
354+
.build();
355+
356+
assertThatExceptionOfType(InvalidSqlException.class)
357+
.isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
358+
.withMessage(Messages.getString("ERROR.44", "IsNotInCaseInsensitive"));
414359
}
415360

416361
@Test
417362
void testInEventuallyEmptyDoubleFilter() {
418-
try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
419-
CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
420-
421-
SelectStatementProvider selectStatement = select(id, animalName)
422-
.from(animalData)
423-
.where(id, isIn(1, 2).filter(s -> false).filter(s -> false))
424-
.orderBy(id)
425-
.build()
426-
.render(RenderingStrategies.MYBATIS3);
427-
428-
assertThat(selectStatement.getSelectStatement()).isEqualTo(
429-
"select id, animal_name from AnimalData " +
430-
"where id in () " +
431-
"order by id"
432-
);
433-
434-
assertThatExceptionOfType(PersistenceException.class).isThrownBy(
435-
() -> mapper.selectManyMappedRows(selectStatement))
436-
.withCauseInstanceOf(SQLSyntaxErrorException.class);
437-
}
363+
var selectModel = select(id, animalName)
364+
.from(animalData)
365+
.where(id, isIn(1, 2).filter(s -> false).filter(s -> false))
366+
.orderBy(id)
367+
.build();
368+
369+
assertThatExceptionOfType(InvalidSqlException.class)
370+
.isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
371+
.withMessage(Messages.getString("ERROR.44", "IsIn"));
438372
}
439373
}

0 commit comments

Comments
 (0)