Merge pull request #833 from jeffgbutler/empty-list-exception

jeffgbutler · web-flow · commit 6e6e225ff0b5 · 2024-08-09T10:01:03.000-04:00
Throw InvalidSqlException during rendering if an In condition is empty
diff --git a/CHANGELOG.md b/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
 
diff --git a/src/main/java/org/mybatis/dynamic/sql/exception/DuplicateTableAliasException.java b/src/main/java/org/mybatis/dynamic/sql/exception/DuplicateTableAliasException.java
@@ -15,6 +15,7 @@
  */
 package org.mybatis.dynamic.sql.exception;
 
+import java.io.Serial;
 import java.util.Objects;
 
 import org.mybatis.dynamic.sql.SqlTable;
@@ -34,6 +35,7 @@
  */
 public class DuplicateTableAliasException extends DynamicSqlException {
 
+    @Serial
     private static final long serialVersionUID = -2631664872557787391L;
 
     public DuplicateTableAliasException(SqlTable table, String newAlias, String existingAlias) {
diff --git a/src/main/java/org/mybatis/dynamic/sql/exception/DynamicSqlException.java b/src/main/java/org/mybatis/dynamic/sql/exception/DynamicSqlException.java
@@ -15,7 +15,10 @@
  */
 package org.mybatis.dynamic.sql.exception;
 
+import java.io.Serial;
+
 public class DynamicSqlException extends RuntimeException {
+    @Serial
     private static final long serialVersionUID = 349021672061361244L;
 
     public DynamicSqlException(String message) {
diff --git a/src/main/java/org/mybatis/dynamic/sql/exception/InvalidSqlException.java b/src/main/java/org/mybatis/dynamic/sql/exception/InvalidSqlException.java
@@ -15,7 +15,10 @@
  */
 package org.mybatis.dynamic.sql.exception;
 
+import java.io.Serial;
+
 public class InvalidSqlException extends DynamicSqlException {
+    @Serial
     private static final long serialVersionUID = 1666851020951347843L;
 
     public InvalidSqlException(String message) {
diff --git a/src/main/java/org/mybatis/dynamic/sql/exception/NonRenderingWhereClauseException.java b/src/main/java/org/mybatis/dynamic/sql/exception/NonRenderingWhereClauseException.java
@@ -19,6 +19,8 @@
 import org.mybatis.dynamic.sql.configuration.StatementConfiguration;
 import org.mybatis.dynamic.sql.util.Messages;
 
+import java.io.Serial;
+
 /**
  * This exception is thrown when the where clause in a statement will not render.
  * This can happen if all the optional conditions in a where clause fail to
@@ -40,6 +42,7 @@
  * @author Jeff Butler
  */
 public class NonRenderingWhereClauseException extends DynamicSqlException {
+    @Serial
     private static final long serialVersionUID = 6619119078542625135L;
 
     public NonRenderingWhereClauseException() {
diff --git a/src/main/java/org/mybatis/dynamic/sql/util/Validator.java b/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);
-        }
-    }
 }
diff --git a/src/main/java/org/mybatis/dynamic/sql/where/condition/IsIn.java b/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;
     }
 
diff --git a/src/main/java/org/mybatis/dynamic/sql/where/condition/IsInCaseInsensitive.java b/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;
     }
 
diff --git a/src/main/java/org/mybatis/dynamic/sql/where/condition/IsNotIn.java b/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;
     }
 
diff --git a/src/main/java/org/mybatis/dynamic/sql/where/condition/IsNotInCaseInsensitive.java b/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;
     }
 
diff --git a/src/main/resources/org/mybatis/dynamic/sql/util/messages.properties b/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}
diff --git a/src/site/markdown/docs/conditions.md b/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                                                            |
diff --git a/src/test/java/examples/animal/data/VariousListConditionsTest.java b/src/test/java/examples/animal/data/VariousListConditionsTest.java
diff --git a/src/test/java/org/mybatis/dynamic/sql/select/SelectStatementTest.java b/src/test/java/org/mybatis/dynamic/sql/select/SelectStatementTest.java
diff --git a/src/test/kotlin/examples/kotlin/mybatis3/canonical/PersonMapperTest.kt b/src/test/kotlin/examples/kotlin/mybatis3/canonical/PersonMapperTest.kt
