CAY-2814 select query iterator() and batch iterator() methods return incorrect results

cayenne-server/src/main/java/org/apache/cayenne/QueryResponse.java

@@ -68,12 +68,26 @@ public interface QueryResponse {
      */
     boolean isList();
 
+    /**
+     * Returns whether current response  is an iterator
+     *
+     * @since 5.0
+     */
+    boolean isIterator();
+
     /**
      * Returns a List under the current iterator position. Use {@link #isList()} to check
      * the result type before calling this method.
      */
     List<?> currentList();
 
+    /**
+     * Returns a current iterator.
+     *
+     * @since 5.0
+     */
+    ResultIterator<?> currentIterator();
+
     /**
      * Returns an update count under the current iterator position. Returned value is an
      * int[] to accommodate batch queries. For a regular update result, the value will be
@@ -100,6 +114,15 @@ public interface QueryResponse {
     @SuppressWarnings("rawtypes")
     List firstList();
 
+    /**
+     * A utility method for quickly retrieving the Iterator in the response. Returns
+     * null if the query has no iterator.
+     *
+     * @since 5.0
+     */
+    @SuppressWarnings("rawtypes")
+    ResultIterator firstIterator();
+
     /**
      * A utility method for quickly retrieving the first update count from the response.
      * Note that this method resets current iterator to an undefined state.

cayenne-server/src/main/java/org/apache/cayenne/access/DataContext.java

@@ -65,7 +65,6 @@
 import org.apache.cayenne.tx.TransactionFactory;
 import org.apache.cayenne.util.EventUtil;
 import org.apache.cayenne.util.GenericResponse;
-import org.apache.cayenne.util.ResultIteratorIterator;
 import org.apache.cayenne.util.Util;
 
 /**
@@ -180,7 +179,7 @@ public DataDomain getParentDataDomain() {
             return (DataDomain) channel;
         }
 
-        List response = channel.onQuery(this, new DataDomainQuery()).firstList();
+        List<?> response = channel.onQuery(this, new DataDomainQuery()).firstList();
 
         if (response != null && response.size() > 0 && response.get(0) instanceof DataDomain) {
             return (DataDomain) response.get(0);
@@ -277,7 +276,7 @@ public Collection<?> uncommittedObjects() {
         // guess target collection size
         Collection<Object> objects = new ArrayList<>(len > 100 ? len / 2 : len);
 
-        Iterator it = getObjectStore().getObjectIterator();
+        Iterator<?> it = getObjectStore().getObjectIterator();
         while (it.hasNext()) {
             Persistent object = (Persistent) it.next();
             int state = object.getPersistenceState();
@@ -420,7 +419,7 @@ public List objectsFromDataRows(ClassDescriptor descriptor, List<? extends DataR
         return new ObjectResolver(this, descriptor, true).synchronizedObjectsFromDataRows(dataRows);
     }
 
-    private List objectsFromDataRowsFromParentContext(ClassDescriptor descriptor, List<? extends DataRow> dataRows) {
+    private List <?> objectsFromDataRowsFromParentContext(ClassDescriptor descriptor, List<? extends DataRow> dataRows) {
         return getChannel().onQuery(this, new ObjectsFromDataRowsQuery(descriptor, dataRows)).firstList();
     }
 
@@ -568,9 +567,9 @@ public boolean visitToMany(ToManyProperty property) {
 
                     Object value = property.readProperty(persistent);
                     @SuppressWarnings("unchecked")
-                    Collection<Map.Entry> collection = (value instanceof Map)
+                    Collection<Map.Entry<?,?>> collection = (value instanceof Map)
                             ? ((Map) value).entrySet()
-                            : (Collection) value;
+                            : (Collection<Map.Entry<?, ?>>) value;
 
                     for (Object target : collection) {
                         if (target instanceof Persistent) {
@@ -612,7 +611,7 @@ public boolean visitAttribute(AttributeProperty property) {
      * 
      * @see #invalidateObjects(Collection)
      */
-    public void unregisterObjects(Collection dataObjects) {
+    public void unregisterObjects(Collection<?> dataObjects) {
         getObjectStore().objectsUnregistered(dataObjects);
     }
 
@@ -796,36 +795,12 @@ GraphDiff flushToParent(boolean cascade) {
     @SuppressWarnings("unchecked")
     @Override
     public <T> ResultIterator<T> iterator(final Select<T> query) {
-        final ResultIterator<?> rows = performIteratedQuery(query);
-        final QueryMetadata md = query.getMetaData(getEntityResolver());
-
-        if (md.isFetchingDataRows() || isObjectArrayResult(md)) {
-            // no need to convert result
-            return (ResultIterator<T>) rows;
-        } else {
-            // this is a bit optimized version of 'objectFromDataRow' with
-            // resolver cached for reuse... still the rest is pretty suboptimal
-            final ObjectResolver resolver = new ObjectResolver(this, md.getClassDescriptor(), true);
-            return new DataRowResultIterator(rows, resolver);
-        }
+        IteratedQueryDecorator queryDecorator = new IteratedQueryDecorator(query);
+        Query queryToRun = nonNullDelegate().willPerformQuery(this, queryDecorator);
+        QueryResponse queryResponse = (onQuery(this, queryToRun));
+        return (ResultIterator<T>) queryResponse.currentIterator();
     }
 
-    /**
-     * This method repeats logic of DataDomainQueryAction.interceptObjectConversion() method.
-     * The difference is that iterator(or batchIterator) doesn't support "mixed" results.
-     */
-    private boolean isObjectArrayResult(QueryMetadata md) {
-        List<Object> resultMapping = md.getResultSetMapping();
-        if(resultMapping == null) {
-            return false;
-        }
-
-        if (md.isSingleResultSetMapping()) {
-            return !(resultMapping.get(0) instanceof EntityResultSegment);
-        } else {
-            return true;
-        }
-    }
 
     /**
      * Performs a single database select query returning result as a
@@ -879,7 +854,7 @@ public ResultIterator performIteratedQuery(Query query) {
                 if (tx.isRollbackOnly()) {
                     try {
                         tx.rollback();
-                    } catch (Exception rollbackEx) {
+                    } catch (Exception ignored) {
                     }
                 }
             }
@@ -891,7 +866,7 @@ public ResultIterator performIteratedQuery(Query query) {
     /**
      * Runs an iterated query in a transactional context provided by the caller.
      */
-    ResultIterator internalPerformIteratedQuery(Query query) {
+    ResultIterator <?> internalPerformIteratedQuery(Query query) {
         // note that for now DataChannel API does not support cursors (aka
         // ResultIterator), so we have to go directly to the DataDomain.
         IteratedSelectObserver observer = new IteratedSelectObserver();
@@ -937,16 +912,16 @@ public QueryResponse performGenericQuery(Query query) {
      * 
      * @return A list of DataObjects or a DataRows, depending on the value
      *         returned by {@link QueryMetadata#isFetchingDataRows()}.
+     *         Сan also return an iterator if the query is an instance of iteratedQuery.
      */
     @Override
-    @SuppressWarnings("unchecked")
     public List performQuery(Query query) {
         query = nonNullDelegate().willPerformQuery(this, query);
         if (query == null) {
             return new ArrayList<>(1);
         }
 
-        List result = onQuery(this, query).firstList();
+        List<?> result = onQuery(this, query).firstList();
         return result != null ? result : new ArrayList<>(1);
     }
 
@@ -1027,8 +1002,8 @@ public List<?> performQuery(String queryName, boolean expireCachedLists) {
      *            is required in case a query uses caching.
      * @since 1.1
      */
-    public List<?> performQuery(String queryName, Map parameters, boolean expireCachedLists) {
-        return performQuery(expireCachedLists ?
+    public List<?> performQuery(String queryName, Map <String,?>parameters, boolean expireCachedLists) {
+        return (List<?>) performQuery(expireCachedLists ?
                 MappedSelect.query(queryName).params(parameters).forceNoCache() :
                 MappedSelect.query(queryName).params(parameters));
     }
@@ -1176,7 +1151,7 @@ protected void fireDataChannelChanged(Object postedBy, GraphDiff changes) {
         super.fireDataChannelChanged(postedBy, changes);
     }
 
-    private TransactionFactory getTransactionFactory() {
+    TransactionFactory getTransactionFactory() {
         attachToRuntimeIfNeeded();
         return transactionFactory;
     }
@@ -1191,55 +1166,4 @@ public void setTransactionFactory(TransactionFactory transactionFactory) {
         this.transactionFactory = transactionFactory;
     }
 
-    /**
-     * ResultIterator that can convert DataRow to Persistent object on the fly.
-     */
-    static class DataRowResultIterator<T> implements ResultIterator<T> {
-
-        final ResultIterator<?> rows;
-        ObjectResolver resolver;
-
-        DataRowResultIterator(ResultIterator<?> rows, ObjectResolver resolver) {
-            this.rows = rows;
-            this.resolver = resolver;
-        }
-
-        @Override
-        public Iterator<T> iterator() {
-            return new ResultIteratorIterator<>(this);
-        }
-
-        @Override
-        public List<T> allRows() {
-            List<T> list = new ArrayList<>();
-            while (hasNextRow()) {
-                list.add(nextRow());
-            }
-            return list;
-        }
-
-        @Override
-        public boolean hasNextRow() {
-            return rows.hasNextRow();
-        }
-
-        @Override
-        @SuppressWarnings("unchecked")
-        public T nextRow() {
-            DataRow row = (DataRow) rows.nextRow();
-            List<T> objects = (List<T>) resolver.synchronizedObjectsFromDataRows(Collections.singletonList(row));
-            return objects.get(0);
-        }
-
-        @Override
-        public void skipRow() {
-            rows.skipRow();
-        }
-
-        @Override
-        public void close() {
-            rows.close();
-        }
-    }
-
 }