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.

CHANGELOG.md

@@ -26,6 +26,10 @@ Other important changes:
 - The library now requires Java 17
 - Deprecated code from prior releases is removed
 - We now allow CASE expressions in ORDER BY Clauses
+- The "In" conditions will now throw `InvalidSqlException` during rendering if the list of values is empty. Previously
+  an empty In condition would render as invalid SQL and would usually cause a runtime exception from the database.
+  With this change, the exception thrown is more predictable and the error is caught before sending the SQL to the
+  database.
 
 ## Release 1.5.2 - June 3, 2024
 

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

@@ -26,12 +26,20 @@ public static void assertNotEmpty(Collection<?> collection, String messageNumber
         assertFalse(collection.isEmpty(), messageNumber);
     }
 
+    public static void assertNotEmpty(Collection<?> collection, String messageNumber, String p1) {
+        assertFalse(collection.isEmpty(), messageNumber, p1);
+    }
+
     public static void assertFalse(boolean condition, String messageNumber) {
-        internalAssertFalse(condition, Messages.getString(messageNumber));
+        if (condition) {
+            throw new InvalidSqlException(Messages.getString(messageNumber));
+        }
     }
 
     public static void assertFalse(boolean condition, String messageNumber, String p1) {
-        internalAssertFalse(condition, Messages.getString(messageNumber, p1));
+        if (condition) {
+            throw new InvalidSqlException(Messages.getString(messageNumber, p1));
+        }
     }
 
     public static void assertTrue(boolean condition, String messageNumber) {
@@ -41,10 +49,4 @@ public static void assertTrue(boolean condition, String messageNumber) {
     public static void assertTrue(boolean condition, String messageNumber, String p1) {
         assertFalse(!condition, messageNumber, p1);
     }
-
-    private static void internalAssertFalse(boolean condition, String message) {
-        if (condition) {
-            throw new InvalidSqlException(message);
-        }
-    }
 }

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

@@ -23,6 +23,7 @@
 
 import org.mybatis.dynamic.sql.AbstractListValueCondition;
 import org.mybatis.dynamic.sql.render.RenderingContext;
+import org.mybatis.dynamic.sql.util.Validator;
 
 public class IsIn<T> extends AbstractListValueCondition<T> {
     private static final IsIn<?> EMPTY = new IsIn<>(Collections.emptyList());
@@ -39,6 +40,7 @@ protected IsIn(Collection<T> values) {
 
     @Override
     public boolean shouldRender(RenderingContext renderingContext) {
+        Validator.assertNotEmpty(values, "ERROR.44", "IsIn"); //$NON-NLS-1$ //$NON-NLS-2$
         return true;
     }
 

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

@@ -24,6 +24,7 @@
 import org.mybatis.dynamic.sql.AbstractListValueCondition;
 import org.mybatis.dynamic.sql.render.RenderingContext;
 import org.mybatis.dynamic.sql.util.StringUtilities;
+import org.mybatis.dynamic.sql.util.Validator;
 
 public class IsInCaseInsensitive extends AbstractListValueCondition<String>
         implements CaseInsensitiveVisitableCondition {
@@ -39,6 +40,7 @@ protected IsInCaseInsensitive(Collection<String> values) {
 
     @Override
     public boolean shouldRender(RenderingContext renderingContext) {
+        Validator.assertNotEmpty(values, "ERROR.44", "IsInCaseInsensitive"); //$NON-NLS-1$ //$NON-NLS-2$
         return true;
     }
 

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

@@ -23,6 +23,7 @@
 
 import org.mybatis.dynamic.sql.AbstractListValueCondition;
 import org.mybatis.dynamic.sql.render.RenderingContext;
+import org.mybatis.dynamic.sql.util.Validator;
 
 public class IsNotIn<T> extends AbstractListValueCondition<T> {
     private static final IsNotIn<?> EMPTY = new IsNotIn<>(Collections.emptyList());
@@ -39,6 +40,7 @@ protected IsNotIn(Collection<T> values) {
 
     @Override
     public boolean shouldRender(RenderingContext renderingContext) {
+        Validator.assertNotEmpty(values, "ERROR.44", "IsNotIn"); //$NON-NLS-1$ //$NON-NLS-2$
         return true;
     }
 

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

@@ -24,6 +24,7 @@
 import org.mybatis.dynamic.sql.AbstractListValueCondition;
 import org.mybatis.dynamic.sql.render.RenderingContext;
 import org.mybatis.dynamic.sql.util.StringUtilities;
+import org.mybatis.dynamic.sql.util.Validator;
 
 public class IsNotInCaseInsensitive extends AbstractListValueCondition<String>
         implements CaseInsensitiveVisitableCondition {
@@ -39,6 +40,7 @@ protected IsNotInCaseInsensitive(Collection<String> values) {
 
     @Override
     public boolean shouldRender(RenderingContext renderingContext) {
+        Validator.assertNotEmpty(values, "ERROR.44", "IsNotInCaseInsensitive"); //$NON-NLS-1$ //$NON-NLS-2$
         return true;
     }
 

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

@@ -60,4 +60,5 @@ ERROR.40=Case expressions must have at least one "when" clause
 ERROR.41=You cannot call "then" in a Kotlin case expression more than once
 ERROR.42=You cannot call `else` in a Kotlin case expression more than once
 ERROR.43=A Kotlin cast expression must have one, and only one, `as` element
+ERROR.44={0} conditions must contain at least one value
 INTERNAL.ERROR=Internal Error {0}

src/site/markdown/docs/conditions.md

@@ -204,8 +204,8 @@ mapping if you so desire.
 
 Starting with version 1.5.2, we made a change to the rendering rules for the "in" conditions. This was done to limit the
 danger of conditions failing to render and thus affecting more rows than expected. For the base conditions ("isIn",
-"isNotIn", etc.), if the list of values is empty, then the condition will still render, but the resulting SQL will
-be invalid and will cause a runtime exception. We believe this is the safest outcome. For example, suppose
+"isNotIn", etc.), if the list of values is empty, then the library will throw
+`org.mybatis.dynamic.sql.exception.InvalidSqlException`. We believe this is the safest outcome. For example, suppose
 a DELETE statement was coded as follows:
 
 ```java
@@ -214,12 +214,6 @@ a DELETE statement was coded as follows:
      .and(id, isIn(Collections.emptyList()));
 ```
 
-This statement will be rendered as follows:
-
-```sql
-   delete from foo where status = ? and id in ()
-```
-
 This will cause a runtime error due to invalid SQL, but it eliminates the possibility of deleting ALL rows with
 active status. If you want to allow the "in" condition to drop from the SQL if the list is empty, then use the
 "inWhenPresent" condition.
@@ -229,8 +223,8 @@ and the case-insensitive versions of these conditions:
 
 | Input                                    | Effect                                                                            |
 |------------------------------------------|-----------------------------------------------------------------------------------|
-| isIn(null)                               | NullPointerException                                                              |
-| isIn(Collections.emptyList())            | Rendered as "in ()" (Invalid SQL)                                                 |
+| isIn(null)                               | NullPointerException thrown                                                       |
+| isIn(Collections.emptyList())            | InvalidSqlException thrown                                                        |
 | isIn(2, 3, null)                         | Rendered as "in (?, ?, ?)" (Parameter values are 2, 3, and null)                  |
 | isInWhenPresent(null)                    | Condition Not Rendered                                                            |
 | isInWhenPresent(Collections.emptyList()) | Condition Not Rendered                                                            |

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

@@ -34,14 +34,12 @@
 import java.io.InputStreamReader;
 import java.sql.Connection;
 import java.sql.DriverManager;
-import java.sql.SQLSyntaxErrorException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
 import org.apache.ibatis.datasource.unpooled.UnpooledDataSource;
-import org.apache.ibatis.exceptions.PersistenceException;
 import org.apache.ibatis.jdbc.ScriptRunner;
 import org.apache.ibatis.mapping.Environment;
 import org.apache.ibatis.session.Configuration;
@@ -51,8 +49,10 @@
 import org.apache.ibatis.transaction.jdbc.JdbcTransactionFactory;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.mybatis.dynamic.sql.exception.InvalidSqlException;
 import org.mybatis.dynamic.sql.render.RenderingStrategies;
 import org.mybatis.dynamic.sql.select.render.SelectStatementProvider;
+import org.mybatis.dynamic.sql.util.Messages;
 import org.mybatis.dynamic.sql.util.mybatis3.CommonSelectMapper;
 
 class VariousListConditionsTest {
@@ -136,26 +136,15 @@ void testInWhenPresentWithNull() {
 
     @Test
     void testInWithEmptyList() {
-        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
-            CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
-
-            SelectStatementProvider selectStatement = select(id, animalName)
-                    .from(animalData)
-                    .where(id, isIn(Collections.emptyList()))
-                    .orderBy(id)
-                    .build()
-                    .render(RenderingStrategies.MYBATIS3);
-
-            assertThat(selectStatement.getSelectStatement()).isEqualTo(
-                    "select id, animal_name from AnimalData " +
-                            "where id in () " +
-                            "order by id"
-            );
-
-            assertThatExceptionOfType(PersistenceException.class).isThrownBy(() ->
-                mapper.selectManyMappedRows(selectStatement)
-            ).withCauseInstanceOf(SQLSyntaxErrorException.class);
-        }
+        var selectModel = select(id, animalName)
+                .from(animalData)
+                .where(id, isIn(Collections.emptyList()))
+                .orderBy(id)
+                .build();
+
+        assertThatExceptionOfType(InvalidSqlException.class)
+                .isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
+                .withMessage(Messages.getString("ERROR.44", "IsIn"));
     }
 
     @Test
@@ -319,121 +308,66 @@ void testNotInCaseInsensitiveWhenPresentMap() {
 
     @Test
     void testInEventuallyEmpty() {
-        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
-            CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
-
-            SelectStatementProvider selectStatement = select(id, animalName)
-                    .from(animalData)
-                    .where(id, isIn(1, 2).filter(s -> false))
-                    .orderBy(id)
-                    .build()
-                    .render(RenderingStrategies.MYBATIS3);
-
-            assertThat(selectStatement.getSelectStatement()).isEqualTo(
-                    "select id, animal_name from AnimalData " +
-                            "where id in () " +
-                            "order by id"
-            );
-
-            assertThatExceptionOfType(PersistenceException.class).isThrownBy(
-                    () -> mapper.selectManyMappedRows(selectStatement))
-                    .withCauseInstanceOf(SQLSyntaxErrorException.class);
-        }
+        var selectModel = select(id, animalName)
+                .from(animalData)
+                .where(id, isIn(1, 2).filter(s -> false))
+                .orderBy(id)
+                .build();
+
+        assertThatExceptionOfType(InvalidSqlException.class)
+                .isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
+                .withMessage(Messages.getString("ERROR.44", "IsIn"));
     }
 
     @Test
     void testInCaseInsensitiveEventuallyEmpty() {
-        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
-            CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
-
-            SelectStatementProvider selectStatement = select(id, animalName)
-                    .from(animalData)
-                    .where(animalName, isInCaseInsensitive("Fred", "Betty").filter(s -> false))
-                    .orderBy(id)
-                    .build()
-                    .render(RenderingStrategies.MYBATIS3);
-
-            assertThat(selectStatement.getSelectStatement()).isEqualTo(
-                    "select id, animal_name from AnimalData " +
-                            "where upper(animal_name) in () " +
-                            "order by id"
-            );
-
-            assertThatExceptionOfType(PersistenceException.class).isThrownBy(
-                            () -> mapper.selectManyMappedRows(selectStatement))
-                    .withCauseInstanceOf(SQLSyntaxErrorException.class);
-        }
+        var selectModel = select(id, animalName)
+                .from(animalData)
+                .where(animalName, isInCaseInsensitive("Fred", "Betty").filter(s -> false))
+                .orderBy(id)
+                .build();
+
+        assertThatExceptionOfType(InvalidSqlException.class)
+                .isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
+                .withMessage(Messages.getString("ERROR.44", "IsInCaseInsensitive"));
     }
 
     @Test
     void testNotInEventuallyEmpty() {
-        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
-            CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
-
-            SelectStatementProvider selectStatement = select(id, animalName)
-                    .from(animalData)
-                    .where(id, isNotIn(1, 2).filter(s -> false))
-                    .orderBy(id)
-                    .build()
-                    .render(RenderingStrategies.MYBATIS3);
-
-            assertThat(selectStatement.getSelectStatement()).isEqualTo(
-                    "select id, animal_name from AnimalData " +
-                            "where id not in () " +
-                            "order by id"
-            );
-
-            assertThatExceptionOfType(PersistenceException.class).isThrownBy(
-                            () -> mapper.selectManyMappedRows(selectStatement))
-                    .withCauseInstanceOf(SQLSyntaxErrorException.class);
-        }
+        var selectModel = select(id, animalName)
+                .from(animalData)
+                .where(id, isNotIn(1, 2).filter(s -> false))
+                .orderBy(id)
+                .build();
+
+        assertThatExceptionOfType(InvalidSqlException.class)
+                .isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
+                .withMessage(Messages.getString("ERROR.44", "IsNotIn"));
     }
 
     @Test
     void testNotInCaseInsensitiveEventuallyEmpty() {
-        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
-            CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
-
-            SelectStatementProvider selectStatement = select(id, animalName)
-                    .from(animalData)
-                    .where(animalName, isNotInCaseInsensitive("Fred", "Betty").filter(s -> false))
-                    .orderBy(id)
-                    .build()
-                    .render(RenderingStrategies.MYBATIS3);
-
-            assertThat(selectStatement.getSelectStatement()).isEqualTo(
-                    "select id, animal_name from AnimalData " +
-                            "where upper(animal_name) not in () " +
-                            "order by id"
-            );
-
-            assertThatExceptionOfType(PersistenceException.class).isThrownBy(
-                            () -> mapper.selectManyMappedRows(selectStatement))
-                    .withCauseInstanceOf(SQLSyntaxErrorException.class);
-        }
+        var selectModel = select(id, animalName)
+                .from(animalData)
+                .where(animalName, isNotInCaseInsensitive("Fred", "Betty").filter(s -> false))
+                .orderBy(id)
+                .build();
+
+        assertThatExceptionOfType(InvalidSqlException.class)
+                .isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
+                .withMessage(Messages.getString("ERROR.44", "IsNotInCaseInsensitive"));
     }
 
     @Test
     void testInEventuallyEmptyDoubleFilter() {
-        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
-            CommonSelectMapper mapper = sqlSession.getMapper(CommonSelectMapper.class);
-
-            SelectStatementProvider selectStatement = select(id, animalName)
-                    .from(animalData)
-                    .where(id, isIn(1, 2).filter(s -> false).filter(s -> false))
-                    .orderBy(id)
-                    .build()
-                    .render(RenderingStrategies.MYBATIS3);
-
-            assertThat(selectStatement.getSelectStatement()).isEqualTo(
-                    "select id, animal_name from AnimalData " +
-                            "where id in () " +
-                            "order by id"
-            );
-
-            assertThatExceptionOfType(PersistenceException.class).isThrownBy(
-                            () -> mapper.selectManyMappedRows(selectStatement))
-                    .withCauseInstanceOf(SQLSyntaxErrorException.class);
-        }
+        var selectModel = select(id, animalName)
+                .from(animalData)
+                .where(id, isIn(1, 2).filter(s -> false).filter(s -> false))
+                .orderBy(id)
+                .build();
+
+        assertThatExceptionOfType(InvalidSqlException.class)
+                .isThrownBy(() -> selectModel.render(RenderingStrategies.MYBATIS3))
+                .withMessage(Messages.getString("ERROR.44", "IsIn"));
     }
 }