diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ef040d6c..010c421c2 100644 --- 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 index ee8c1e2d1..89ad51fdb 100644 --- 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 index 3a8bc5791..df7d61ce0 100644 --- 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 index 51ce3f787..ee1a13a9d 100644 --- 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 index 3dfa0ed36..23210e5a4 100644 --- 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 index 1d4b3f7b8..47814c7a2 100644 --- 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 index ce00047b5..ef6979b67 100644 --- 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 extends AbstractListValueCondition { private static final IsIn EMPTY = new IsIn<>(Collections.emptyList()); @@ -39,6 +40,7 @@ protected IsIn(Collection 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 index 3fe30c1ef..274226158 100644 --- 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 implements CaseInsensitiveVisitableCondition { @@ -39,6 +40,7 @@ protected IsInCaseInsensitive(Collection 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 index dc7358b2a..9c621f00b 100644 --- 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 extends AbstractListValueCondition { private static final IsNotIn EMPTY = new IsNotIn<>(Collections.emptyList()); @@ -39,6 +40,7 @@ protected IsNotIn(Collection 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 index fa43af0b6..e91e893c6 100644 --- 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 implements CaseInsensitiveVisitableCondition { @@ -39,6 +40,7 @@ protected IsNotInCaseInsensitive(Collection 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 index c25eb02c5..ef092e651 100644 --- 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 index 55a2e6617..2e266e22b 100644 --- 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 index 469b64718..4e71ef668 100644 --- a/src/test/java/examples/animal/data/VariousListConditionsTest.java +++ b/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")); } } diff --git a/src/test/java/org/mybatis/dynamic/sql/select/SelectStatementTest.java b/src/test/java/org/mybatis/dynamic/sql/select/SelectStatementTest.java index f548a4d71..6311403d8 100644 --- a/src/test/java/org/mybatis/dynamic/sql/select/SelectStatementTest.java +++ b/src/test/java/org/mybatis/dynamic/sql/select/SelectStatementTest.java @@ -21,7 +21,6 @@ import static org.mybatis.dynamic.sql.SqlBuilder.*; import java.sql.JDBCType; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -283,18 +282,6 @@ void testGroupBySingleColumn() { ); } - @Test - void testInEmptyList() { - List emptyList = Collections.emptyList(); - SelectStatementProvider selectStatement = select(column1, column3) - .from(table, "a") - .where(column3, isIn(emptyList)) - .build() - .render(RenderingStrategies.MYBATIS3); - - assertThat(selectStatement.getSelectStatement()).isEqualTo("select a.column1, a.column3 from foo a where a.column3 in ()"); - } - @Test void testNotInEmptyList() { List emptyList = Collections.emptyList(); @@ -358,17 +345,6 @@ void testNotInWhenPresentEmptyList() { ); } - @Test - void testNotInCaseInsensitiveEmptyList() { - SelectStatementProvider selectStatement = select(column1, column3) - .from(table, "a") - .where(column3, isNotInCaseInsensitive(Collections.emptyList())) - .build() - .render(RenderingStrategies.MYBATIS3); - - assertThat(selectStatement.getSelectStatement()).isEqualTo("select a.column1, a.column3 from foo a where upper(a.column3) not in ()"); - } - @Test void testNotInCaseInsensitiveWhenPresentEmptyList() { SelectModel selectModel = select(column1, column3) diff --git a/src/test/kotlin/examples/kotlin/mybatis3/canonical/PersonMapperTest.kt b/src/test/kotlin/examples/kotlin/mybatis3/canonical/PersonMapperTest.kt index 75a703c09..6178e0326 100644 --- a/src/test/kotlin/examples/kotlin/mybatis3/canonical/PersonMapperTest.kt +++ b/src/test/kotlin/examples/kotlin/mybatis3/canonical/PersonMapperTest.kt @@ -912,18 +912,6 @@ class PersonMapperTest { assertThat(insertStatement.insertStatement).isEqualTo(expected) } - @Test - fun testRenderingEmptyList() { - val selectStatement = select(id, firstName, lastName, birthDate, employed, occupation, addressId) { - from(person) - where { id isIn emptyList() } - } - - val expected = "select id, first_name, last_name, birth_date, employed, occupation, address_id from Person " + - "where id in ()" - assertThat(selectStatement.selectStatement).isEqualTo(expected) - } - @Test fun testSumWithCase() { sqlSessionFactory.openSession().use { session ->