Merge pull request #794 from jeffgbutler/new-in-rendering

Change "in" Condition Rendering - Render With Empty Lists

Author: jeffgbutler

CHANGELOG.md

@@ -4,9 +4,17 @@ This log will detail notable changes to MyBatis Dynamic SQL. Full details are av
 
 ## Release 1.5.2 - Unreleased
 
-This is a small maintenance release with improvements to the Kotlin DSL for CASE expressions. We worked on this soon
-after the 1.5.1 release, so wanted to get it out quickly.
-See this PR for details: ([#785](https://github.com/mybatis/mybatis-dynamic-sql/pull/785))
+This is a small maintenance release with the following changes:
+
+1. Improvements to the Kotlin DSL for CASE expressions (infix methods for "else" and "then"). See this PR for 
+   details: ([#785](https://github.com/mybatis/mybatis-dynamic-sql/pull/785))
+2. **Potentially Breaking Change**: the "in" conditions ("isIn", "isNotIn", "isInCaseInsensitive",
+   "isNotInCaseInsensitive") will now render if the input list of values is empty. This will lead
+   to a runtime exception. This change was made out of an abundance of caution and is the safest choice.
+   If you wish to allow "in" conditions to be removed from where clauses when the list is empty,
+   then use the "when present" versions of those conditions. If you are unsure how this works, please
+   read the documentation here: https://mybatis.org/mybatis-dynamic-sql/docs/conditions.html#optionality-with-the-%E2%80%9Cin%E2%80%9D-conditions
+   For background on the reason for the change, see the discussion here: https://github.com/mybatis/mybatis-dynamic-sql/issues/788
 
 GitHub milestone: [https://github.com/mybatis/mybatis-dynamic-sql/milestone/14?closed=1](https://github.com/mybatis/mybatis-dynamic-sql/milestone/14?closed=1)
 
@@ -34,8 +42,8 @@ We've tested this extensively and the code is, of course, 100% covered by test c
 covered every scenario. Please let us know if you find issues.
 
 Full documentation is available here:
-- [Java Case Expression DSL Documentation](caseExpressions.md)
-- [Kotlin Case Expression DSL Documentation](kotlinCaseExpressions.md)
+- [Java Case Expression DSL Documentation](https://mybatis.org/mybatis-dynamic-sql/docs/caseExpressions.html)
+- [Kotlin Case Expression DSL Documentation](https://mybatis.org/mybatis-dynamic-sql/docs/kotlinCaseExpressions.html)
 
 The pull request for this change is ([#761](https://github.com/mybatis/mybatis-dynamic-sql/pull/761))
 

src/main/java/org/mybatis/dynamic/sql/AbstractListValueCondition.java

@@ -23,8 +23,6 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import org.mybatis.dynamic.sql.render.RenderingContext;
-
 public abstract class AbstractListValueCondition<T> implements VisitableCondition<T> {
     protected final Collection<T> values;
 
@@ -41,15 +39,6 @@ public boolean isEmpty() {
         return values.isEmpty();
     }
 
-    @Override
-    public boolean shouldRender(RenderingContext renderingContext) {
-        if (isEmpty()) {
-            return renderingContext.isEmptyListConditionRenderingAllowed();
-        } else {
-            return true;
-        }
-    }
-
     @Override
     public <R> R accept(ConditionVisitor<T, R> visitor) {
         return visitor.visit(this);
@@ -85,14 +74,12 @@ protected <R, S extends AbstractListValueCondition<R>> S mapSupport(Function<? s
     }
 
     /**
-     * If renderable, apply the predicate to each value in the list and return a new condition with the filtered values.
-     *     Else returns a condition that will not render (this). If all values are filtered out of the value
-     *     list, then the condition will not render.
+     * If not empty, apply the predicate to each value in the list and return a new condition with the filtered values.
+     *     Else returns an empty condition (this).
      *
-     * @param predicate
-     *            predicate applied to the values, if renderable
+     * @param predicate predicate applied to the values, if not empty
      *
-     * @return a new condition with filtered values if renderable, otherwise a condition that will not render.
+     * @return a new condition with filtered values if renderable, otherwise an empty condition
      */
     public abstract AbstractListValueCondition<T> filter(Predicate<? super T> predicate);
 

src/main/java/org/mybatis/dynamic/sql/SqlBuilder.java

@@ -76,6 +76,8 @@
 import org.mybatis.dynamic.sql.where.condition.IsGreaterThanWithSubselect;
 import org.mybatis.dynamic.sql.where.condition.IsIn;
 import org.mybatis.dynamic.sql.where.condition.IsInCaseInsensitive;
+import org.mybatis.dynamic.sql.where.condition.IsInCaseInsensitiveWhenPresent;
+import org.mybatis.dynamic.sql.where.condition.IsInWhenPresent;
 import org.mybatis.dynamic.sql.where.condition.IsInWithSubselect;
 import org.mybatis.dynamic.sql.where.condition.IsLessThan;
 import org.mybatis.dynamic.sql.where.condition.IsLessThanColumn;
@@ -91,6 +93,8 @@
 import org.mybatis.dynamic.sql.where.condition.IsNotEqualToWithSubselect;
 import org.mybatis.dynamic.sql.where.condition.IsNotIn;
 import org.mybatis.dynamic.sql.where.condition.IsNotInCaseInsensitive;
+import org.mybatis.dynamic.sql.where.condition.IsNotInCaseInsensitiveWhenPresent;
+import org.mybatis.dynamic.sql.where.condition.IsNotInWhenPresent;
 import org.mybatis.dynamic.sql.where.condition.IsNotInWithSubselect;
 import org.mybatis.dynamic.sql.where.condition.IsNotLike;
 import org.mybatis.dynamic.sql.where.condition.IsNotLikeCaseInsensitive;
@@ -764,12 +768,12 @@ static <T> IsInWithSubselect<T> isIn(Buildable<SelectModel> selectModelBuilder)
     }
 
     @SafeVarargs
-    static <T> IsIn<T> isInWhenPresent(T... values) {
-        return IsIn.of(values).filter(Objects::nonNull);
+    static <T> IsInWhenPresent<T> isInWhenPresent(T... values) {
+        return IsInWhenPresent.of(values);
     }
 
-    static <T> IsIn<T> isInWhenPresent(Collection<T> values) {
-        return values == null ? IsIn.empty() : IsIn.of(values).filter(Objects::nonNull);
+    static <T> IsInWhenPresent<T> isInWhenPresent(Collection<T> values) {
+        return values == null ? IsInWhenPresent.empty() : IsInWhenPresent.of(values);
     }
 
     @SafeVarargs
@@ -786,12 +790,12 @@ static <T> IsNotInWithSubselect<T> isNotIn(Buildable<SelectModel> selectModelBui
     }
 
     @SafeVarargs
-    static <T> IsNotIn<T> isNotInWhenPresent(T... values) {
-        return IsNotIn.of(values).filter(Objects::nonNull);
+    static <T> IsNotInWhenPresent<T> isNotInWhenPresent(T... values) {
+        return IsNotInWhenPresent.of(values);
     }
 
-    static <T> IsNotIn<T> isNotInWhenPresent(Collection<T> values) {
-        return values == null ? IsNotIn.empty() : IsNotIn.of(values).filter(Objects::nonNull);
+    static <T> IsNotInWhenPresent<T> isNotInWhenPresent(Collection<T> values) {
+        return values == null ? IsNotInWhenPresent.empty() : IsNotInWhenPresent.of(values);
     }
 
     static <T> IsBetween.Builder<T> isBetween(T value1) {
@@ -909,12 +913,12 @@ static IsInCaseInsensitive isInCaseInsensitive(Collection<String> values) {
         return IsInCaseInsensitive.of(values);
     }
 
-    static IsInCaseInsensitive isInCaseInsensitiveWhenPresent(String... values) {
-        return IsInCaseInsensitive.of(values).filter(Objects::nonNull);
+    static IsInCaseInsensitiveWhenPresent isInCaseInsensitiveWhenPresent(String... values) {
+        return IsInCaseInsensitiveWhenPresent.of(values);
     }
 
-    static IsInCaseInsensitive isInCaseInsensitiveWhenPresent(Collection<String> values) {
-        return values == null ? IsInCaseInsensitive.empty() : IsInCaseInsensitive.of(values).filter(Objects::nonNull);
+    static IsInCaseInsensitiveWhenPresent isInCaseInsensitiveWhenPresent(Collection<String> values) {
+        return values == null ? IsInCaseInsensitiveWhenPresent.empty() : IsInCaseInsensitiveWhenPresent.of(values);
     }
 
     static IsNotInCaseInsensitive isNotInCaseInsensitive(String... values) {
@@ -925,13 +929,13 @@ static IsNotInCaseInsensitive isNotInCaseInsensitive(Collection<String> values)
         return IsNotInCaseInsensitive.of(values);
     }
 
-    static IsNotInCaseInsensitive isNotInCaseInsensitiveWhenPresent(String... values) {
-        return IsNotInCaseInsensitive.of(values).filter(Objects::nonNull);
+    static IsNotInCaseInsensitiveWhenPresent isNotInCaseInsensitiveWhenPresent(String... values) {
+        return IsNotInCaseInsensitiveWhenPresent.of(values);
     }
 
-    static IsNotInCaseInsensitive isNotInCaseInsensitiveWhenPresent(Collection<String> values) {
-        return values == null ? IsNotInCaseInsensitive.empty() :
-                IsNotInCaseInsensitive.of(values).filter(Objects::nonNull);
+    static IsNotInCaseInsensitiveWhenPresent isNotInCaseInsensitiveWhenPresent(Collection<String> values) {
+        return values == null ? IsNotInCaseInsensitiveWhenPresent.empty() :
+                IsNotInCaseInsensitiveWhenPresent.of(values);
     }
 
     // order by support

src/main/java/org/mybatis/dynamic/sql/configuration/GlobalConfiguration.java

@@ -26,7 +26,6 @@ public class GlobalConfiguration {
     public static final String CONFIGURATION_FILE_PROPERTY = "mybatis-dynamic-sql.configurationFile"; //$NON-NLS-1$
     private static final String DEFAULT_PROPERTY_FILE = "mybatis-dynamic-sql.properties"; //$NON-NLS-1$
     private boolean isNonRenderingWhereClauseAllowed = false;
-    private boolean isEmptyListConditionRenderingAllowed = false;
     private final Properties properties = new Properties();
 
     public GlobalConfiguration() {
@@ -66,16 +65,9 @@ void loadProperties(InputStream inputStream, String propertyFile) {
     private void initializeKnownProperties() {
         String value = properties.getProperty("nonRenderingWhereClauseAllowed", "false"); //$NON-NLS-1$ //$NON-NLS-2$
         isNonRenderingWhereClauseAllowed = Boolean.parseBoolean(value);
-
-        value = properties.getProperty("emptyListConditionRenderingAllowed", "false"); //$NON-NLS-1$ //$NON-NLS-2$
-        isEmptyListConditionRenderingAllowed = Boolean.parseBoolean(value);
     }
 
     public boolean isIsNonRenderingWhereClauseAllowed() {
         return isNonRenderingWhereClauseAllowed;
     }
-
-    public boolean isEmptyListConditionRenderingAllowed() {
-        return isEmptyListConditionRenderingAllowed;
-    }
 }

src/main/java/org/mybatis/dynamic/sql/configuration/StatementConfiguration.java

@@ -25,13 +25,6 @@
  * Configurable behaviors are detailed below:
  *
  * <dl>
- *     <dt>emptyListConditionRenderingAllowed</dt>
- *     <dd>If false (default), the framework will not render list conditions that are empty in a where clause.
- *         This is beneficial in that it will not allow the library to generate invalid SQL, but it has a
- *         potentially dangerous side effect where a statement could be generated that impacts more rows
- *         then expected. If true, an empty list will be rendered as "in ()", "not in ()", etc. which will likely
- *         cause an SQLException at runtime.
- *     </dd>
  *     <dt>nonRenderingWhereClauseAllowed</dt>
  *     <dd>If false (default), the framework will throw a {@link NonRenderingWhereClauseException}
  *         if a where clause is specified in the statement, but it fails to render because all
@@ -51,9 +44,6 @@ public class StatementConfiguration {
     private boolean isNonRenderingWhereClauseAllowed =
             GlobalContext.getConfiguration().isIsNonRenderingWhereClauseAllowed();
 
-    private boolean isEmptyListConditionRenderingAllowed =
-            GlobalContext.getConfiguration().isEmptyListConditionRenderingAllowed();
-
     public boolean isNonRenderingWhereClauseAllowed() {
         return isNonRenderingWhereClauseAllowed;
     }
@@ -62,13 +52,4 @@ public StatementConfiguration setNonRenderingWhereClauseAllowed(boolean nonRende
         isNonRenderingWhereClauseAllowed = nonRenderingWhereClauseAllowed;
         return this;
     }
-
-    public boolean isEmptyListConditionRenderingAllowed() {
-        return isEmptyListConditionRenderingAllowed;
-    }
-
-    public StatementConfiguration setEmptyListConditionRenderingAllowed(boolean emptyListConditionRenderingAllowed) {
-        isEmptyListConditionRenderingAllowed = emptyListConditionRenderingAllowed;
-        return this;
-    }
 }

src/main/java/org/mybatis/dynamic/sql/render/RenderingContext.java

@@ -101,10 +101,6 @@ public boolean isNonRenderingClauseAllowed() {
         return statementConfiguration.isNonRenderingWhereClauseAllowed();
     }
 
-    public boolean isEmptyListConditionRenderingAllowed() {
-        return statementConfiguration.isEmptyListConditionRenderingAllowed();
-    }
-
     /**
      * Create a new rendering context based on this, with the table alias calculator modified to include the
      * specified child table alias calculator. This is used by the query expression renderer when the alias calculator

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

@@ -22,6 +22,7 @@
 import java.util.function.Predicate;
 
 import org.mybatis.dynamic.sql.AbstractListValueCondition;
+import org.mybatis.dynamic.sql.render.RenderingContext;
 
 public class IsIn<T> extends AbstractListValueCondition<T> {
     private static final IsIn<?> EMPTY = new IsIn<>(Collections.emptyList());
@@ -36,6 +37,11 @@ protected IsIn(Collection<T> values) {
         super(values);
     }
 
+    @Override
+    public boolean shouldRender(RenderingContext renderingContext) {
+        return true;
+    }
+
     @Override
     public String operator() {
         return "in"; //$NON-NLS-1$
@@ -47,13 +53,12 @@ public IsIn<T> filter(Predicate<? super T> predicate) {
     }
 
     /**
-     * If renderable, apply the mapping to each value in the list return a new condition with the mapped values.
-     *     Else return a condition that will not render (this).
+     * If not empty, apply the mapping to each value in the list return a new condition with the mapped values.
+     *     Else return an empty condition (this).
      *
-     * @param mapper a mapping function to apply to the values, if renderable
+     * @param mapper a mapping function to apply to the values, if not empty
      * @param <R> type of the new condition
-     * @return a new condition with mapped values if renderable, otherwise a condition
-     *     that will not render.
+     * @return a new condition with mapped values if renderable, otherwise an empty condition
      */
     public <R> IsIn<R> map(Function<? super T, ? extends R> mapper) {
         Function<Collection<R>, IsIn<R>> constructor = IsIn::new;

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

@@ -22,6 +22,7 @@
 import java.util.function.UnaryOperator;
 
 import org.mybatis.dynamic.sql.AbstractListValueCondition;
+import org.mybatis.dynamic.sql.render.RenderingContext;
 import org.mybatis.dynamic.sql.util.StringUtilities;
 
 public class IsInCaseInsensitive extends AbstractListValueCondition<String>
@@ -36,6 +37,11 @@ protected IsInCaseInsensitive(Collection<String> values) {
         super(values);
     }
 
+    @Override
+    public boolean shouldRender(RenderingContext renderingContext) {
+        return true;
+    }
+
     @Override
     public String operator() {
         return "in"; //$NON-NLS-1$
@@ -47,12 +53,11 @@ public IsInCaseInsensitive filter(Predicate<? super String> predicate) {
     }
 
     /**
-     * If renderable, apply the mapping to each value in the list return a new condition with the mapped values.
-     *     Else return a condition that will not render (this).
+     * If not empty, apply the mapping to each value in the list return a new condition with the mapped values.
+     *     Else return an empty condition (this).
      *
-     * @param mapper a mapping function to apply to the values, if renderable
-     * @return a new condition with mapped values if renderable, otherwise a condition
-     *     that will not render.
+     * @param mapper a mapping function to apply to the values, if not empty
+     * @return a new condition with mapped values if renderable, otherwise an empty condition
      */
     public IsInCaseInsensitive map(UnaryOperator<String> mapper) {
         return mapSupport(mapper, IsInCaseInsensitive::new, IsInCaseInsensitive::empty);

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

@@ -0,0 +1,69 @@
+/*
+ *    Copyright 2016-2024 the original author or authors.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *       https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+package org.mybatis.dynamic.sql.where.condition;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Objects;
+import java.util.function.Predicate;
+import java.util.function.UnaryOperator;
+import java.util.stream.Collectors;
+
+import org.mybatis.dynamic.sql.AbstractListValueCondition;
+import org.mybatis.dynamic.sql.util.StringUtilities;
+
+public class IsInCaseInsensitiveWhenPresent extends AbstractListValueCondition<String>
+        implements CaseInsensitiveVisitableCondition {
+    private static final IsInCaseInsensitiveWhenPresent EMPTY = new IsInCaseInsensitiveWhenPresent(Collections.emptyList());
+
+    public static IsInCaseInsensitiveWhenPresent empty() {
+        return EMPTY;
+    }
+
+    protected IsInCaseInsensitiveWhenPresent(Collection<String> values) {
+        super(values.stream().filter(Objects::nonNull).collect(Collectors.toList()));
+    }
+
+    @Override
+    public String operator() {
+        return "in"; //$NON-NLS-1$
+    }
+
+    @Override
+    public IsInCaseInsensitiveWhenPresent filter(Predicate<? super String> predicate) {
+        return filterSupport(predicate, IsInCaseInsensitiveWhenPresent::new, this, IsInCaseInsensitiveWhenPresent::empty);
+    }
+
+    /**
+     * If not empty, apply the mapping to each value in the list return a new condition with the mapped values.
+     *     Else return an empty condition (this).
+     *
+     * @param mapper a mapping function to apply to the values, if not empty
+     * @return a new condition with mapped values if renderable, otherwise an empty condition
+     */
+    public IsInCaseInsensitiveWhenPresent map(UnaryOperator<String> mapper) {
+        return mapSupport(mapper, IsInCaseInsensitiveWhenPresent::new, IsInCaseInsensitiveWhenPresent::empty);
+    }
+
+    public static IsInCaseInsensitiveWhenPresent of(String... values) {
+        return of(Arrays.asList(values));
+    }
+
+    public static IsInCaseInsensitiveWhenPresent of(Collection<String> values) {
+        return new IsInCaseInsensitiveWhenPresent(values).map(StringUtilities::safelyUpperCase);
+    }
+}