Add generics to ExpressionIterator,

make sure ordering in from is guaranteed and fixed

Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>

foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/queries/repreparation/AddJoinedAttributeTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -25,7 +25,7 @@
 public class AddJoinedAttributeTest extends AutoVerifyTestCase {
     private ReadAllQuery query;
     private Vector employees;
-    private static String EXPECTED_SQL = "SELECT t1.EMP_ID, t2.EMP_ID, t1.F_NAME, t1.GENDER, t1.L_NAME, t2.SALARY, t1.START_TIME, t1.END_TIME, t1.END_DATE, t1.START_DATE, t1.ADDR_ID, t1.MANAGER_ID, t1.VERSION, t0.ADDRESS_ID, t0.CITY, t0.COUNTRY, t0.P_CODE, t0.PROVINCE, t0.STREET FROM ADDRESS t0, SALARY t2, EMPLOYEE t1 WHERE ((t2.EMP_ID = t1.EMP_ID) AND (t0.ADDRESS_ID = t1.ADDR_ID))";
+    private static String EXPECTED_SQL = "SELECT t1.EMP_ID, t2.EMP_ID, t1.F_NAME, t1.GENDER, t1.L_NAME, t2.SALARY, t1.START_TIME, t1.END_TIME, t1.END_DATE, t1.START_DATE, t1.ADDR_ID, t1.MANAGER_ID, t1.VERSION, t0.ADDRESS_ID, t0.CITY, t0.COUNTRY, t0.P_CODE, t0.PROVINCE, t0.STREET FROM EMPLOYEE t1, SALARY t2, ADDRESS t0 WHERE ((t2.EMP_ID = t1.EMP_ID) AND (t0.ADDRESS_ID = t1.ADDR_ID))";
 
     public AddJoinedAttributeTest() {
         setDescription("Test if SQL is reprepared the second time");

foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/queries/repreparation/AddNonFetchedJoinedAttributeTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -28,8 +28,8 @@
 public class AddNonFetchedJoinedAttributeTest extends AutoVerifyTestCase {
     private ReadAllQuery query1, query2;
     private Vector employees1, employees2;
-    private static String EXPECTED_SQL1 = "SELECT t1.EMP_ID, t2.EMP_ID, t1.F_NAME, t1.GENDER, t1.L_NAME, t2.SALARY, t1.START_TIME, t1.END_TIME, t1.END_DATE, t1.START_DATE, t1.ADDR_ID, t1.MANAGER_ID, t1.VERSION FROM ADDRESS t0, SALARY t2, EMPLOYEE t1 WHERE ((t2.EMP_ID = t1.EMP_ID) AND (t0.ADDRESS_ID = t1.ADDR_ID))";
-    private static String EXPECTED_SQL2 = "SELECT t3.EMP_ID, t4.EMP_ID, t3.F_NAME, t3.GENDER, t3.L_NAME, t4.SALARY, t3.START_TIME, t3.END_TIME, t3.END_DATE, t3.START_DATE, t3.ADDR_ID, t3.MANAGER_ID, t3.VERSION FROM SALARY t4, EMPLOYEE t3, SALARY t2, EMPLOYEE t1, ADDRESS t0 WHERE ((t4.EMP_ID = t3.EMP_ID) AND (((t1.EMP_ID = t3.MANAGER_ID) AND (t2.EMP_ID = t1.EMP_ID)) AND (t0.ADDRESS_ID = t1.ADDR_ID)))";
+    private static String EXPECTED_SQL1 = "SELECT t1.EMP_ID, t2.EMP_ID, t1.F_NAME, t1.GENDER, t1.L_NAME, t2.SALARY, t1.START_TIME, t1.END_TIME, t1.END_DATE, t1.START_DATE, t1.ADDR_ID, t1.MANAGER_ID, t1.VERSION FROM EMPLOYEE t1, SALARY t2, ADDRESS t0 WHERE ((t2.EMP_ID = t1.EMP_ID) AND (t0.ADDRESS_ID = t1.ADDR_ID))";
+    private static String EXPECTED_SQL2 = "SELECT t3.EMP_ID, t4.EMP_ID, t3.F_NAME, t3.GENDER, t3.L_NAME, t4.SALARY, t3.START_TIME, t3.END_TIME, t3.END_DATE, t3.START_DATE, t3.ADDR_ID, t3.MANAGER_ID, t3.VERSION FROM EMPLOYEE t3, SALARY t4, EMPLOYEE t1, SALARY t2, ADDRESS t0 WHERE ((t4.EMP_ID = t3.EMP_ID) AND (((t1.EMP_ID = t3.MANAGER_ID) AND (t2.EMP_ID = t1.EMP_ID)) AND (t0.ADDRESS_ID = t1.ADDR_ID)))";
 
 
     public AddNonFetchedJoinedAttributeTest() {

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/expressions/Expression.java

@@ -1095,7 +1095,7 @@ protected void convertNodeToUseOuterJoin() {
      * equality operations between two field nodes.
      */
     public Expression convertToUseOuterJoin() {
-        ExpressionIterator iterator = new ExpressionIterator() {
+        ExpressionIterator<Void> iterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 each.convertNodeToUseOuterJoin();

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/CompoundExpression.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2022 IBM Corporation. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -104,7 +104,7 @@ public DatabaseTable aliasForTable(DatabaseTable table) {
     @Override
     public Expression asOf(AsOfClause clause) {
         final AsOfClause finalClause = clause;
-        ExpressionIterator iterator = new ExpressionIterator() {
+        ExpressionIterator<Void> iterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 if (each.isDataExpression()) {

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/ExpressionIterator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -16,16 +16,16 @@
 
 import org.eclipse.persistence.expressions.Expression;
 
-import java.util.Enumeration;
-import java.util.Vector;
+import java.util.List;
 
 /**
- * Used to itterate an expression tree, through inner subclasses.
+ * Used to iterate an expression tree, through inner subclasses.
+ * @param <T> result type
  */
-public abstract class ExpressionIterator {
+public abstract class ExpressionIterator<T> {
 
     /** Allow the iteration to build a result. */
-    protected Object result;
+    protected T result;
 
     /** Some iterations require a statement. */
     protected SQLSelectStatement statement;
@@ -40,7 +40,7 @@ protected ExpressionIterator() {
         super();
     }
 
-    public Object getResult() {
+    public T getResult() {
         return result;
     }
 
@@ -59,16 +59,16 @@ public boolean hasAlreadyVisited(Expression expression) {
 
     /**
      * INTERNAL:
-     * This method must be defined by subclasses to implement the logic of the iteratation.
+     * This method must be defined by subclasses to implement the logic of the iteration.
      */
     public abstract void iterate(Expression expression);
 
     /**
      * INTERNAL:
      */
-    public void iterateOn(Vector expressions) {
-        for (Enumeration expressionEnum = expressions.elements(); expressionEnum.hasMoreElements();) {
-            iterate((Expression)expressionEnum.nextElement());
+    public void iterateOn(List<Expression> expressions) {
+        for (Expression expression : expressions) {
+            iterate(expression);
         }
     }
 
@@ -80,7 +80,7 @@ public void iterateOn(Expression expression) {
         expression.iterateOn(this);
     }
 
-    public void setResult(Object result) {
+    public void setResult(T result) {
         this.result = result;
     }
 

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/FunctionExpression.java

@@ -1,6 +1,6 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2019, 2023 IBM Corporation. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2024 IBM Corporation. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -129,7 +129,7 @@ public DatabaseTable aliasForTable(DatabaseTable table) {
     @Override
     public Expression asOf(AsOfClause clause) {
         final AsOfClause finalClause = clause;
-        ExpressionIterator iterator = new ExpressionIterator() {
+        ExpressionIterator<Void> iterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 if (each.isDataExpression()) {

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/SQLSelectStatement.java

@@ -60,9 +60,9 @@
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Hashtable;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -826,7 +826,7 @@ public void assignAliases(Vector allExpressions) {
         // For  CR#2627019
         currentAliasNumber = getCurrentAliasNumber();
 
-        ExpressionIterator iterator = new ExpressionIterator() {
+        ExpressionIterator<Void> iterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 currentAliasNumber = each.assignTableAliasesStartingAt(currentAliasNumber);
@@ -883,7 +883,7 @@ public DatabaseCall buildCall(AbstractSession session) {
      * This is used by cursored stream to determine if an expression used distinct as the size must account for this.
      */
     public void computeDistinct() {
-        ExpressionIterator iterator = new ExpressionIterator() {
+        ExpressionIterator<Void> iterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression expression) {
                 if (expression.isQueryKeyExpression() && ((QueryKeyExpression)expression).shouldQueryToManyRelationship()) {
@@ -938,23 +938,24 @@ public void computeTables() {
         // Compute tables should never defer to computeTablesFromTables
         // This iterator will pull all the table aliases out of an expression, and
         // put them in a map.
-        ExpressionIterator iterator = new ExpressionIterator() {
+        ExpressionIterator<Map<DatabaseTable, DatabaseTable>> iterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 TableAliasLookup aliases = each.getTableAliases();
 
                 if (aliases != null) {
-                    // Insure that an aliased table is only added to a single
+                    // Ensure that an aliased table is only added to a single
                     // FROM clause.
                     if (!aliases.haveBeenAddedToStatement()) {
-                        aliases.addToMap((Map<DatabaseTable, DatabaseTable>)getResult());
+                        aliases.addToMap(getResult());
                         aliases.setHaveBeenAddedToStatement(true);
                     }
                 }
             }
         };
 
-        iterator.setResult(new Hashtable(5));
+        //we want consistent order in the from and Hashtable nor HashMap guarantee ordering
+        iterator.setResult(new LinkedHashMap<>(5));
 
         if (getWhereClause() != null) {
             iterator.iterateOn(getWhereClause());
@@ -967,24 +968,24 @@ public void iterate(Expression each) {
 
         //Iterate on fields as well in that rare case where the select is not in the where clause
         for (Object field : getFields()) {
-            if (field instanceof Expression) {
-                iterator.iterateOn((Expression)field);
+            if (field instanceof Expression e) {
+                iterator.iterateOn(e);
             }
         }
 
         //Iterate on non-selected fields as well in that rare case where the from is not in the where clause
         if (hasNonSelectFields()) {
             for (Object field : getNonSelectFields()) {
-                if (field instanceof Expression) {
-                    iterator.iterateOn((Expression)field);
+                if (field instanceof Expression e) {
+                    iterator.iterateOn(e);
                 }
             }
         }
 
         // Always iterator on the builder, as the where clause may not contain the builder, i.e. value=value.
         iterator.iterateOn(getBuilder());
 
-        Map<DatabaseTable, DatabaseTable> allTables = (Map<DatabaseTable, DatabaseTable>)iterator.getResult();
+        Map<DatabaseTable, DatabaseTable> allTables = iterator.getResult();
         setTableAliases(allTables);
 
         for (DatabaseTable table : allTables.values()) {
@@ -997,7 +998,8 @@ public void iterate(Expression each) {
      * no ambiguity
      */
     public void computeTablesFromTables() {
-        Map<DatabaseTable, DatabaseTable> allTables = new Hashtable<>();
+        //we want consistent order in the from and Hashtable nor HashMap guarantee ordering
+        Map<DatabaseTable, DatabaseTable> allTables = new LinkedHashMap<>();
         AsOfClause asOfClause = null;
 
         if (getBuilder().hasAsOfClause() && !getBuilder().getSession().getProject().hasGenericHistorySupport()) {

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/queries/ExpressionQueryMechanism.java

@@ -471,7 +471,7 @@ protected static String getAliasTableName(SQLSelectStatement selectStatement, Da
         if(!selectStatement.requiresAliases()) {
             return null;
         }
-        HashSet aliasTables = new HashSet();
+        Set<DatabaseTable> aliasTables = new HashSet<>();
         Iterator<Map.Entry<DatabaseTable, DatabaseTable>> itEntries = selectStatement.getTableAliases().entrySet().iterator();
         DatabaseTable aliasTable = null;
         while(itEntries.hasNext()) {
@@ -489,14 +489,14 @@ protected static String getAliasTableName(SQLSelectStatement selectStatement, Da
         // The table has several aliases,
         // remove the aliases that used by DataExpressions
         // with baseExpression NOT the expressionBuilder used by the statement
-        ExpressionIterator expIterator = new ExpressionIterator() {
+        ExpressionIterator<Collection<DatabaseTable>> expIterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 if(each instanceof DataExpression dataExpression) {
                     DatabaseField field = dataExpression.getField();
                     if(field != null) {
                         if(dataExpression.getBaseExpression() != getStatement().getBuilder()) {
-                            ((Collection)getResult()).remove(dataExpression.getAliasedField().getTable());
+                            getResult().remove(dataExpression.getAliasedField().getTable());
                         }
                     }
                 }
@@ -512,14 +512,14 @@ public boolean shouldIterateOverSubSelects() {
         expIterator.iterateOn(selectStatement.getWhereClause());
 
         if(aliasTables.size() == 1) {
-            aliasTable = (DatabaseTable)aliasTables.iterator().next();
+            aliasTable = aliasTables.iterator().next();
             return aliasTable.getQualifiedName();
         } else if(aliasTables.isEmpty()) {
             // should never happen
             return aliasTable.getQualifiedName();
         } else {
             // should never happen
-            aliasTable = (DatabaseTable)aliasTables.iterator().next();
+            aliasTable = aliasTables.iterator().next();
             return aliasTable.getQualifiedName();
         }
     }
@@ -2097,7 +2097,7 @@ public void prepareUpdateAll() {
         //     Employee-base example: valueExp = builder.get("manager").get("firstName");
         // Before iterating the table is set into result,
         // if expression requiring select is found, then resul set to null.
-        ExpressionIterator expRequiresSelectIterator = new ExpressionIterator() {
+        ExpressionIterator<DatabaseTable> expRequiresSelectIterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 if(getResult() == null) {
@@ -2124,7 +2124,7 @@ public void iterate(Expression each) {
                     }
                     DatabaseField field = dataExpression.getField();
                     if(field != null) {
-                        if(!field.getTable().equals((DatabaseTable)getResult())) {
+                        if(!field.getTable().equals(getResult())) {
                             setResult(null);
                             return;
                         }
@@ -2209,13 +2209,13 @@ public boolean shouldIterateOverSubSelects() {
             //
             // This ExpressionIterator will be used for collecting fields from
             // selection criteria and assigned expressions.
-            ExpressionIterator expIterator = new ExpressionIterator() {
+            ExpressionIterator<Collection<DatabaseField>> expIterator = new ExpressionIterator<>() {
                 @Override
                 public void iterate(Expression each) {
                     if(each instanceof DataExpression dataExpression) {
                         DatabaseField field = dataExpression.getField();
                         if(field != null) {
-                            ((Collection)getResult()).add(field);
+                            getResult().add(field);
                         }
                     }
                 }

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/mappings/querykeys/ForeignReferenceQueryKey.java

@@ -168,29 +168,29 @@ public DatabaseTable getReferenceTable(ClassDescriptor desc) {
      * The returned relationTable still could be null.
      */
     public DatabaseTable getRelationTable(ClassDescriptor referenceDescriptor) {
-        ExpressionIterator expIterator = new ExpressionIterator() {
+        ExpressionIterator<Collection<DatabaseTable>> expIterator = new ExpressionIterator<>() {
             @Override
             public void iterate(Expression each) {
                 if(each.isTableExpression()) {
-                    ((Collection)this.getResult()).add(((TableExpression)each).getTable());
+                    this.getResult().add(((TableExpression)each).getTable());
                 }
                 else if(each.isDataExpression()) {
                     DatabaseField field = ((DataExpression)each).getField();
                     if(field != null && field.hasTableName()) {
-                        ((Collection)this.getResult()).add(field.getTable());
+                        this.getResult().add(field.getTable());
                     }
                 } else if(each.isParameterExpression()) {
                     DatabaseField field = ((ParameterExpression)each).getField();
                     if(field != null && field.hasTableName()) {
-                        ((Collection)this.getResult()).add(field.getTable());
+                        this.getResult().add(field.getTable());
                     }
                 }
             }
         };
 
-        expIterator.setResult(new HashSet());
+        expIterator.setResult(new HashSet<>());
         expIterator.iterateOn(this.joinCriteria);
-        HashSet<DatabaseTable> tables = (HashSet)expIterator.getResult();
+        Collection<DatabaseTable> tables = expIterator.getResult();
 
         DatabaseTable relationTable = null;
         Iterator<DatabaseTable> it = tables.iterator();

foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/mappings/querykeys/QueryKey.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -44,22 +44,18 @@ public QueryKey() {
      */
     @Override
     public Object clone() {
-        Object object = null;
-
         try {
-            object = super.clone();
+            return super.clone();
         } catch (Exception exception) {
             throw new InternalError(exception.toString());
         }
-
-        return object;
     }
 
     /**
      * INTERNAL:
      * Convert all the class-name-based settings in this QueryKey to actual class-based
      * settings
-     * Will be overridded by subclasses
+     * Will be overridden by subclasses
      */
     public void convertClassNamesToClasses(ClassLoader classLoader){}