Improved: Refactoring ‘EntityCondition’ - Turn EntityCondition into a…

…n interface

(OFBIZ-10691)

This is done in the spirit of favouring abstraction over
implementation.  To achieve that the following change has been made:

* The useless ‘EntityCondition#eval’ methods has been removed

* The ‘PriceServices#calculatePurchasePrice’ method has been fixed to
  refer to ‘EntityCondition#makeCondition’ instead of
  ‘EntityExpr#makeCondition’.

Additionally, the genericity of the ‘makeCondition’ methods has been
improved to allow having a hierarchy of class between the
‘EntityCondition’ type and the subtypes.

Exceptionally a ‘serialVersionUID’ has been used instead of
suppressing the 'serial' warning for the ‘EntityCondition’ interface
to satisfy both Eclipse and OpenJDK linting features which disagree on
the validity of using the ‘@SuppressWarnings’ annotation in the case
of interfaces extending ‘Serializable’.


Thanks Mathieu for the contribution

git-svn-id: https://svn.apache.org/repos/asf/ofbiz/ofbiz-framework/trunk@1850373 13f79535-47bb-0310-9956-ffa450edef68

applications/product/src/main/java/org/apache/ofbiz/product/price/PriceServices.java

@@ -40,7 +40,6 @@
 import org.apache.ofbiz.entity.GenericEntityException;
 import org.apache.ofbiz.entity.GenericValue;
 import org.apache.ofbiz.entity.condition.EntityCondition;
-import org.apache.ofbiz.entity.condition.EntityExpr;
 import org.apache.ofbiz.entity.condition.EntityOperator;
 import org.apache.ofbiz.entity.util.EntityQuery;
 import org.apache.ofbiz.entity.util.EntityUtil;
@@ -1225,12 +1224,11 @@ public static Map<String, Object> calculatePurchasePrice(DispatchContext dctx, M
         if (UtilValidate.isNotEmpty(agreementId)) {
             //TODO Search before if agreement is associate to SupplierProduct.
             //confirm that agreement is price application on purchase type and contains a value for the product
-            EntityCondition cond = EntityCondition.makeCondition(
-                    UtilMisc.toList(
-                            EntityExpr.makeCondition("agreementId", agreementId),
-                            EntityExpr.makeCondition("agreementItemTypeId", "AGREEMENT_PRICING_PR"),
-                            EntityExpr.makeCondition("agreementTypeId", "PURCHASE_AGREEMENT"),
-                            EntityExpr.makeCondition("productId", productId)));
+            EntityCondition cond = EntityCondition.makeConditionMap(
+                    "agreementId", agreementId,
+                    "agreementItemTypeId", "AGREEMENT_PRICING_PR",
+                    "agreementTypeId", "PURCHASE_AGREEMENT",
+                    "productId", productId);
             try {
                 List<GenericValue> agreementPrices = delegator.findList("AgreementItemAndProductAppl", cond, UtilMisc.toSet("price", "currencyUomId"), null, null, true);
                 if (UtilValidate.isNotEmpty(agreementPrices)) {

framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityCondition.java

@@ -32,121 +32,253 @@
 import org.apache.ofbiz.entity.model.ModelEntity;
 
 /**
- * <p>Represents the conditions to be used to constrain a query.</p>
- * <p>An EntityCondition can represent various type of constraints, including:</p>
+ * Represents a condition expression that can be used in a SQL 'WHERE' clause
+ * which is used to constrain a SQL query.
+ * <p>
+ * An EntityCondition can represent various type of constraints, including:
  * <ul>
  *  <li>EntityConditionList: a list of EntityConditions, combined with the operator specified
  *  <li>EntityExpr: for simple expressions or expressions that combine EntityConditions
  *  <li>EntityFieldMap: a map of fields where the field (key) equals the value, combined with the operator specified
  * </ul>
  * These can be used in various combinations using the EntityConditionList and EntityExpr objects.
- *
  */
-@SuppressWarnings("serial")
-public abstract class EntityCondition implements IsEmpty, Serializable {
+public interface EntityCondition extends IsEmpty, Serializable {
+
+    static final long serialVersionUID = -7601205800717391212L;
 
-    public static <L,R,LL,RR> EntityExpr makeCondition(L lhs, EntityComparisonOperator<LL,RR> operator, R rhs) {
+    /**
+     * Constructs a condition expression to represent a comparison between two elements.
+     *
+     * @param lhs the left hand side element
+     * @param operator the binary infix operator
+     * @param rhs the right hand side element
+     * @return a condition expression representing a comparison between two elements
+     */
+    static <L,R,LL,RR> EntityExpr makeCondition(L lhs, EntityComparisonOperator<LL,RR> operator, R rhs) {
         return new EntityExpr(lhs, operator, rhs);
     }
 
-    public static <R> EntityExpr makeCondition(String fieldName, R value) {
+    /**
+     * Constructs a condition expression to represent a equality comparison between a field and a value.
+     *
+     * @param fieldName the name of the field to compare
+     * @param value the value to find in field
+     * @return a condition expression representing a equality comparison
+     */
+    static <R> EntityExpr makeCondition(String fieldName, R value) {
         return new EntityExpr(fieldName, EntityOperator.EQUALS, value);
     }
 
-    public static EntityExpr makeCondition(EntityCondition lhs, EntityJoinOperator operator, EntityCondition rhs) {
+    /**
+     * Constructs a condition expression to represent a combination of two condition expressions.
+     *
+     * @param lhs the left hand side condition
+     * @param operator the binary infix operator used to combine {@code lhs} and {@code rhs} conditions
+     * @param rhs the right hand side condition
+     * @return a condition expression representing a combination of condition expression
+     */
+    static EntityExpr makeCondition(EntityCondition lhs, EntityJoinOperator operator, EntityCondition rhs) {
         return new EntityExpr(lhs, operator, rhs);
     }
 
+    /**
+     * Constructs a condition expression to represent a combination of condition expressions.
+     *
+     * @param operator the binary infix operator used to join every elements of {@code conditionList}
+     * @param conditionList the list of condition expressions to join
+     * @return a condition expression representing a combination of condition expressions.
+     */
     @SafeVarargs
-    public static <T extends EntityCondition> EntityConditionList<T> makeCondition(EntityJoinOperator operator, T... conditionList) {
-        return new EntityConditionList<>(Arrays.<T>asList(conditionList), operator);
+    static <R extends EntityCondition, T extends R>
+    EntityConditionList<R> makeCondition(EntityJoinOperator operator, T... conditionList) {
+        return new EntityConditionList<>(Arrays.asList(conditionList), operator);
     }
 
+    /**
+     * Constructs a condition expression to represent a conjunction of condition expressions
+     *
+     * @param conditionList the condition expressions to join with {@link EntityOperator#AND}
+     * @return a condition expression representing a conjunction of condition expressions
+     */
     @SafeVarargs
-    public static <T extends EntityCondition> EntityConditionList<T> makeCondition(T... conditionList) {
-        return new EntityConditionList<>(Arrays.<T>asList(conditionList), EntityOperator.AND);
+    static <R extends EntityCondition, T extends R>
+    EntityConditionList<R> makeCondition(T... conditionList) {
+        return new EntityConditionList<>(Arrays.asList(conditionList), EntityOperator.AND);
     }
 
-    public static <T extends EntityCondition> EntityConditionList<T> makeCondition(List<T> conditionList, EntityJoinOperator operator) {
+    /**
+     * Constructs a condition expression to represent a combination of condition expressions.
+     *
+     * @param conditionList the list of condition expressions to join
+     * @param operator the binary infix operator used to join every elements of {@code conditionList}
+     * @return a condition expression representing a combination of condition expressions.
+     */
+    static <T extends EntityCondition>
+    EntityConditionList<T> makeCondition(List<? extends T> conditionList, EntityJoinOperator operator) {
         return new EntityConditionList<>(conditionList, operator);
     }
 
-    public static <T extends EntityCondition> EntityConditionList<T> makeCondition(List<T> conditionList) {
+    /**
+     * Constructs a condition expression to represent a conjunction of condition expressions
+     *
+     * @param conditionList the condition expressions to join with {@link EntityOperator#AND}
+     * @return a condition expression representing a conjunction of condition expressions
+     */
+    static <T extends EntityCondition> EntityConditionList<T> makeCondition(List<? extends T> conditionList) {
         return new EntityConditionList<>(conditionList, EntityOperator.AND);
     }
 
-    public static <L,R> EntityFieldMap makeCondition(Map<String, ? extends Object> fieldMap, EntityComparisonOperator<L,R> compOp, EntityJoinOperator joinOp) {
+    /**
+     * Constructs a condition expression to represent a combination of field/value comparisons.
+     *
+     * @param fieldMap the map associating a field to the value to match
+     * @param compOp the binary infix operator used to compare the field and the value
+     * @param joinOp the binary infix operator used to join the field/value comparisons
+     * @return a condition expression representing a combination of field/value comparisons
+     */
+    static <L,R> EntityFieldMap makeCondition(Map<String, ? extends Object> fieldMap,
+            EntityComparisonOperator<L,R> compOp, EntityJoinOperator joinOp) {
         return new EntityFieldMap(fieldMap, compOp, joinOp);
     }
 
-    public static EntityFieldMap makeCondition(Map<String, ? extends Object> fieldMap, EntityJoinOperator joinOp) {
+    /**
+     * Constructs a condition expression to represent a combination of field/value equality comparisons.
+     *
+     * @param fieldMap the map associating a field to the value to match with {@link EntityOperator#EQUALS}
+     * @param joinOp the binary infix operator used to join the field/value equality comparisons
+     * @return a condition expression representing a combination of field/value equality comparisons
+     */
+    static EntityFieldMap makeCondition(Map<String, ? extends Object> fieldMap, EntityJoinOperator joinOp) {
         return new EntityFieldMap(fieldMap, EntityOperator.EQUALS, joinOp);
     }
 
-    public static EntityFieldMap makeCondition(Map<String, ? extends Object> fieldMap) {
+    /**
+     * Constructs a condition expression to represent a conjunction of field/value equality comparisons.
+     *
+     * @param fieldMap the map associating a field to the value to match with {@link EntityOperator#EQUALS}
+     * @return a condition expression representing a conjunction of field/value equality comparisons
+     */
+    static EntityFieldMap makeCondition(Map<String, ? extends Object> fieldMap) {
         return new EntityFieldMap(fieldMap, EntityOperator.EQUALS, EntityOperator.AND);
     }
 
-    public static <L,R> EntityFieldMap makeCondition(EntityComparisonOperator<L,R> compOp, EntityJoinOperator joinOp, Object... keysValues) {
+    /**
+     * Constructs a condition expression to represent a combination of field/value comparisons.
+     *
+     * @param compOp the binary infix operator used to compare the field and the value
+     * @param joinOp the binary infix operator used to join the field/value comparisons
+     * @param keysValues the field/values pairs to match
+     * @return a condition expression representing a combination of field/value comparisons
+     */
+    static <L,R> EntityFieldMap makeCondition(EntityComparisonOperator<L,R> compOp, EntityJoinOperator joinOp,
+            Object... keysValues) {
         return new EntityFieldMap(compOp, joinOp, keysValues);
     }
 
-    public static EntityFieldMap makeCondition(EntityJoinOperator joinOp, Object... keysValues) {
+    /**
+     * Constructs a condition expression to represent a combination of field/value equality comparisons.
+     *
+     * @param joinOp the binary infix operator used to join the field/value equality comparisons
+     * @param keysValues the field/values pairs to match with {@link EntityOperator#EQUALS}
+     * @return a condition expression representing a combination of field/value equality comparisons
+     */
+    static EntityFieldMap makeCondition(EntityJoinOperator joinOp, Object... keysValues) {
         return new EntityFieldMap(EntityOperator.EQUALS, joinOp, keysValues);
     }
 
-    public static EntityFieldMap makeConditionMap(Object... keysValues) {
+    /**
+     * Constructs a condition expression to represent a conjunction of field/value equality comparisons.
+     *
+     * @param keysValues the field/values pairs to match with {@link EntityOperator#EQUALS}
+     * @return a condition expression representing a conjunction of field/value equality comparisons
+     */
+    static EntityFieldMap makeConditionMap(Object... keysValues) {
         return new EntityFieldMap(EntityOperator.EQUALS, EntityOperator.AND, keysValues);
     }
 
-    public static EntityDateFilterCondition makeConditionDate(String fromDateName, String thruDateName) {
+    /**
+     * Constructs a condition expression to filter rows that are currently valid.
+     *
+     * This means that we remove rows whose from/thru date range does not match the current date.
+     * The <i>current date</i> is the one computed when the SQL query is generated.
+     *
+     * @param fromDateName the name of the field corresponding to the from date
+     * @param thruDateName the name of the field corresponding to the thru date
+     * @return a condition expression filtering rows that are currently valid
+     */
+    static EntityDateFilterCondition makeConditionDate(String fromDateName, String thruDateName) {
         return new EntityDateFilterCondition(fromDateName, thruDateName);
     }
 
-    public static EntityWhereString makeConditionWhere(String sqlString) {
+    /**
+     * Constructs a condition expression backed by a raw SQL string
+     *
+     * @param sqlString the SQL string
+     * @return a raw SQL string condition expression
+     */
+    static EntityWhereString makeConditionWhere(String sqlString) {
         return new EntityWhereString(sqlString);
     }
 
-    @Override
-    public String toString() {
-        return makeWhereString(null, new ArrayList<EntityConditionParam>(), null);
-    }
-
     /**
      * Applies a visitor to this condition.
      *
      * @param visitor the visitor to be applied
      */
-    abstract public void accept(EntityConditionVisitor visitor);
+    void accept(EntityConditionVisitor visitor);
 
-    abstract public String makeWhereString(ModelEntity modelEntity, List<EntityConditionParam> entityConditionParams, Datasource datasourceInfo);
+    /**
+     * Dumps the corresponding SQL string.
+     *
+     * @param modelEntity the model of the entity
+     * @param entityConditionParams the effective parameters used to substitute '?' parameters
+     * @param datasourceInfo the model of the data source interpreting the SQL
+     * @return the corresponding SQL string
+     */
+    String makeWhereString(ModelEntity modelEntity, List<EntityConditionParam> entityConditionParams,
+            Datasource datasourceInfo);
 
-    abstract public void checkCondition(ModelEntity modelEntity) throws GenericModelException;
+    /**
+     * Verifies that this condition expression is valid.
+     *
+     * @param modelEntity the model of the entity
+     * @throws GenericModelException when this condition expression is not valid
+     */
+    void checkCondition(ModelEntity modelEntity) throws GenericModelException;
 
-    public boolean entityMatches(GenericEntity entity) {
+    /**
+     * Checks that this condition expression matches a particular entity.
+     *
+     * @param entity the entity to match
+     * @return {@code true} if this condition expression matches {@code entity}
+     */
+    default boolean entityMatches(GenericEntity entity) {
         return mapMatches(entity.getDelegator(), entity);
     }
 
-    public Boolean eval(GenericEntity entity) {
-        return eval(entity.getDelegator(), entity);
-    }
-
-    public Boolean eval(Delegator delegator, Map<String, ? extends Object> map) {
-        return mapMatches(delegator, map) ? Boolean.TRUE : Boolean.FALSE;
-    }
-
-    abstract public boolean mapMatches(Delegator delegator, Map<String, ? extends Object> map);
-
-    abstract public EntityCondition freeze();
+    /**
+     * Checks that this condition expression matches a particular entity.
+     *
+     * @param delegator the delegator used to match
+     * @param map the entity definition to match
+     * @return {@code true} if this condition expression matches {@code map} when using {@code delegator}
+     */
+    boolean mapMatches(Delegator delegator, Map<String, ? extends Object> map);
 
-    @Override
-    public boolean equals(Object obj) {
-        throw new UnsupportedOperationException("equals:" + getClass().getName());
-    }
+    /**
+     * Create a Frozen condition expression corresponding to this condition expression.
+     *
+     * @return the frozen condition expression
+     */
+    EntityCondition freeze();
 
-    @Override
-    public int hashCode() {
-        throw new UnsupportedOperationException("hashCode: " + getClass().getName());
+    /**
+     * Dumps the corresponding SQL string without substituting '?' parameters.
+     *
+     * @return the corresponding SQL string
+     */
+    default String makeWhereString() {
+        return makeWhereString(null, new ArrayList<EntityConditionParam>(), null);
     }
-
 }

framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityConditionBuilder.java

@@ -37,7 +37,7 @@ public class EntityConditionBuilder extends BuilderSupport {
     public static final String module = EntityConditionBuilder.class.getName();
 
     @SuppressWarnings("serial")
-    private static class ConditionHolder extends EntityCondition {
+    private static class ConditionHolder implements EntityCondition {
         protected EntityCondition condition;
 
         protected ConditionHolder(EntityCondition condition) {
@@ -75,6 +75,11 @@ public boolean equals(Object obj) {
         public void accept(EntityConditionVisitor visitor) {
             throw new IllegalArgumentException(getClass().getName() + ".accept not implemented");
         }
+
+        @Override
+        public String toString() {
+            return makeWhereString();
+        }
     }
 
     @Override

framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityConditionFunction.java

@@ -32,7 +32,7 @@
  *
  */
 @SuppressWarnings("serial")
-public abstract class EntityConditionFunction extends EntityCondition {
+public abstract class EntityConditionFunction implements EntityCondition {
 
     public static final int ID_NOT = 1;
 
@@ -116,4 +116,9 @@ public String makeWhereString(ModelEntity modelEntity, List<EntityConditionParam
     public void checkCondition(ModelEntity modelEntity) throws GenericModelException {
         condition.checkCondition(modelEntity);
     }
+
+    @Override
+    public String toString() {
+        return makeWhereString();
+    }
 }

framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityConditionList.java

@@ -27,7 +27,7 @@
  */
 @SuppressWarnings("serial")
 public class EntityConditionList<T extends EntityCondition> extends EntityConditionListBase<T> {
-    public EntityConditionList(List<T> conditionList, EntityJoinOperator operator) {
+    public EntityConditionList(List<? extends T> conditionList, EntityJoinOperator operator) {
         super(conditionList, operator);
     }