Skip to content

Commit

Permalink
Merge pull request #596 from stariy95/PR_594_fix
Browse files Browse the repository at this point in the history 
CAY-2814 Select query iterator() and batchIterator() methods return incorrect results
  • Loading branch information
@stariy95
stariy95 committed Nov 7, 2023
2 parents 5f5b304 + 78883fc commit 3438ed9
Show file tree
Hide file tree
Showing 15 changed files with 1,377 additions and 179 deletions.
23 changes: 23 additions & 0 deletions cayenne-server/src/main/java/org/apache/cayenne/QueryResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
113 changes: 18 additions & 95 deletions cayenne-server/src/main/java/org/apache/cayenne/access/DataContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand All @@ -841,8 +816,7 @@ private boolean isObjectArrayResult(QueryMetadata md) {
* {@link #iterate(Select, org.apache.cayenne.ResultIteratorCallback)} to
* get access to objects.
*/
// TODO: deprecate once all selecting queries start implementing Select<T>
// interface
// TODO: deprecate once all selecting queries start implementing Select<T> interface
@SuppressWarnings({ "rawtypes" })
public ResultIterator performIteratedQuery(Query query) {

Expand Down Expand Up @@ -879,7 +853,7 @@ public ResultIterator performIteratedQuery(Query query) {
if (tx.isRollbackOnly()) {
try {
tx.rollback();
} catch (Exception rollbackEx) {
} catch (Exception ignored) {
}
}
}
Expand All @@ -891,7 +865,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();
Expand Down Expand Up @@ -937,16 +911,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);
}

Expand Down Expand Up @@ -1027,8 +1001,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));
}
Expand Down Expand Up @@ -1176,7 +1150,7 @@ protected void fireDataChannelChanged(Object postedBy, GraphDiff changes) {
super.fireDataChannelChanged(postedBy, changes);
}

private TransactionFactory getTransactionFactory() {
TransactionFactory getTransactionFactory() {
attachToRuntimeIfNeeded();
return transactionFactory;
}
Expand All @@ -1191,55 +1165,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();
}
}

}
Loading

0 comments on commit 3438ed9

Please sign in to comment.