This is an automated email from the ASF dual-hosted git repository. ntimofeev pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/cayenne.git
commit b487051586e5f7859a92e735c5f5b0b6d3a11f74 Author: stariy95 <[email protected]> AuthorDate: Thu Nov 9 18:03:25 2023 +0400 CAY-2817 Pagination flow refactoring --- RELEASE-NOTES.txt | 1 + .../cayenne/access/DataContextQueryAction.java | 58 ++++++---- .../cayenne/access/DataDomainQueryAction.java | 12 +- .../cayenne/access/IncrementalFaultList.java | 126 +++++++++++++++------ .../access/MixedResultIncrementalFaultList.java | 20 +--- .../access/SimpleIdIncrementalFaultList.java | 5 +- .../cayenne/access/IncrementalFaultListIT.java | 4 +- .../SimpleIdIncrementalFaultListDataRowsIT.java | 7 +- .../access/SimpleIdIncrementalFaultListIT.java | 44 ++++--- 9 files changed, 178 insertions(+), 99 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 23868ae11..e1ef21051 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -40,6 +40,7 @@ CAY-2795 Add unit tests for the Json type CAY-2802 Upgrade Gradle to 7.6.1 CAY-2803 Test infrastructure: declarative custom DI modules in ServerCase CAY-2805 Stop calling exp parser internally +CAY-2817 Pagination flow refactoring Bug Fixes: diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/DataContextQueryAction.java b/cayenne-server/src/main/java/org/apache/cayenne/access/DataContextQueryAction.java index a1d9263a5..bf276f22e 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/DataContextQueryAction.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/DataContextQueryAction.java @@ -23,6 +23,7 @@ import java.util.Collection; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import org.apache.cayenne.ObjectContext; import org.apache.cayenne.PersistenceState; @@ -104,30 +105,14 @@ class DataContextQueryAction extends ObjectContextQueryAction { @Override protected boolean interceptPaginatedQuery() { if (metadata.getPageSize() > 0) { - Integer maxIdQualifierSize = actingDataContext.getParentDataDomain().getMaxIdQualifierSize(); - List<?> paginatedList; - List<Object> rsMapping = metadata.getResultSetMapping(); - boolean mixedResults = false; - if(rsMapping != null) { - if(rsMapping.size() > 1) { - mixedResults = true; - } else if(rsMapping.size() == 1) { - mixedResults = !(rsMapping.get(0) instanceof EntityResultSegment) - || !metadata.isSingleResultSetMapping(); - } - } + // this will select raw ids + runQuery(); - if(mixedResults) { - paginatedList = new MixedResultIncrementalFaultList<>(actingDataContext, query, maxIdQualifierSize); - } else { - DbEntity dbEntity = metadata.getDbEntity(); - if (dbEntity != null && dbEntity.getPrimaryKeys().size() == 1) { - paginatedList = new SimpleIdIncrementalFaultList<>(actingDataContext, query, maxIdQualifierSize); - } else { - paginatedList = new IncrementalFaultList<>(actingDataContext, query, maxIdQualifierSize); - } - } + List<?> rawIds = response.firstList(); + int maxIdQualifierSize = actingDataContext.getParentDataDomain().getMaxIdQualifierSize(); + IncrementalFaultList<?> paginatedList = createIncrementalFaultList(rawIds, maxIdQualifierSize); + // replace result with a paginated list that will deal with id-to-object resolution response = new ListResponse(paginatedList); return DONE; } @@ -135,6 +120,35 @@ class DataContextQueryAction extends ObjectContextQueryAction { return !DONE; } + private IncrementalFaultList<?> createIncrementalFaultList(List<?> rawIds, int maxIdQualifierSize) { + // just a sanity check + Objects.requireNonNull(rawIds, "Trying to execute paginated query that is not a select query"); + if(isMixedResultsForPaginatedQuery()) { + return new MixedResultIncrementalFaultList<>(actingDataContext, query, maxIdQualifierSize, rawIds); + } else { + DbEntity dbEntity = metadata.getDbEntity(); + if (dbEntity != null && dbEntity.getPrimaryKeys().size() == 1) { + return new SimpleIdIncrementalFaultList<>(actingDataContext, query, maxIdQualifierSize, rawIds); + } else { + return new IncrementalFaultList<>(actingDataContext, query, maxIdQualifierSize, rawIds); + } + } + } + + private boolean isMixedResultsForPaginatedQuery() { + boolean mixedResults = false; + List<Object> rsMapping = metadata.getResultSetMapping(); + if(rsMapping != null) { + if(rsMapping.size() > 1) { + mixedResults = true; + } else if(rsMapping.size() == 1) { + mixedResults = !(rsMapping.get(0) instanceof EntityResultSegment) + || !metadata.isSingleResultSetMapping(); + } + } + return mixedResults; + } + @Override protected boolean interceptRefreshQuery() { if (query instanceof RefreshQuery) { diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java b/cayenne-server/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java index 0b01cb620..e6778c442 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/DataDomainQueryAction.java @@ -139,9 +139,7 @@ class DataDomainQueryAction implements QueryRouter, OperationObserver { } } - if (!noObjectConversion) { - interceptObjectConversion(); - } + interceptObjectConversion(); return response; } @@ -528,7 +526,7 @@ class DataDomainQueryAction implements QueryRouter, OperationObserver { @SuppressWarnings({"unchecked", "rawtypes"}) private void interceptObjectConversion() { - if (context == null) { + if (noObjectConversion()) { return; } @@ -550,6 +548,12 @@ class DataDomainQueryAction implements QueryRouter, OperationObserver { response.reset(); } + private boolean noObjectConversion() { + return context == null + || noObjectConversion + || metadata.getPageSize() > 0; + } + private ObjectConversionStrategy<?, ?> getConverter() { ObjectConversionStrategy<?, ?> converter; diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/IncrementalFaultList.java b/cayenne-server/src/main/java/org/apache/cayenne/access/IncrementalFaultList.java index c4648d3ea..e07dd4372 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/IncrementalFaultList.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/IncrementalFaultList.java @@ -21,7 +21,6 @@ package org.apache.cayenne.access; import org.apache.cayenne.CayenneRuntimeException; import org.apache.cayenne.Persistent; -import org.apache.cayenne.ResultIterator; import org.apache.cayenne.exp.Expression; import org.apache.cayenne.exp.ExpressionFactory; import org.apache.cayenne.map.ObjEntity; @@ -58,20 +57,18 @@ import java.util.NoSuchElementException; */ public class IncrementalFaultList<E> implements List<E>, Serializable { - protected int pageSize; + protected final int pageSize; protected final List elements; - protected DataContext dataContext; - protected ObjEntity rootEntity; - protected int unfetchedObjects; + protected final DataContext dataContext; + protected final ObjEntity rootEntity; + protected volatile int unfetchedObjects; /** * Stores a hint allowing to distinguish data rows from unfetched ids when * the query fetches data rows. */ - protected int idWidth; - - IncrementalListHelper helper; - protected QueryMetadata metadata; + protected final int idWidth; + protected final QueryMetadata metadata; /** * Defines the upper limit on the size of fetches. This is needed to avoid @@ -79,15 +76,13 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { */ protected int maxFetchSize; - // Don't confuse this with the JDBC ResultSet fetch size setting - this - // controls - // the where clause generation that is necessary to fetch specific records a - // page - // at a time. Some JDBC Drivers/Databases may have limits on statement - // length - // or complexity of the where clause - e.g., PostgreSQL having a default - // limit of - // 10,000 nested expressions. + IncrementalListHelper helper; + + // Don't confuse this with the JDBC ResultSet fetch size setting - + // this controls the where clause generation that is necessary to fetch specific records a + // page at a time. Some JDBC Drivers/Databases may have limits on statement + // length or complexity of the where clause - e.g., PostgreSQL having a default + // limit of 10,000 nested expressions. /** * Creates a new IncrementalFaultList using a given DataContext and query. @@ -101,7 +96,7 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @param maxFetchSize * maximum number of fetches in one query */ - public IncrementalFaultList(DataContext dataContext, Query query, int maxFetchSize) { + IncrementalFaultList(DataContext dataContext, Query query, int maxFetchSize, List<?> data) { this.metadata = query.getMetaData(dataContext.getEntityResolver()); if (metadata.getPageSize() <= 0) { throw new CayenneRuntimeException("Not a paginated query; page size: " + metadata.getPageSize()); @@ -116,12 +111,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { } this.idWidth = metadata.getDbEntity().getPrimaryKeys().size(); - - List<Object> elementsUnsynced = new ArrayList<>(); - fillIn(query, elementsUnsynced); - this.elements = Collections.synchronizedList(elementsUnsynced); - this.maxFetchSize = maxFetchSize; + // make a copy of data, as we need to modify content of this list later + this.elements = Collections.synchronizedList(new ArrayList<>(data)); + this.unfetchedObjects = elements.size(); } /** @@ -147,18 +140,20 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * fully resolved. For the rest of the list, only ObjectIds are read. * * @since 3.0 + * @deprecated since 5.0, does nothing */ + @Deprecated(forRemoval = true) protected void fillIn(final Query query, List<Object> elementsList) { + } - elementsList.clear(); - - try (ResultIterator<?> it = dataContext.performIteratedQuery(query)) { - while (it.hasNextRow()) { - elementsList.add(it.nextRow()); - } - } + /** + * Sets initial data (i.e. list of ObjectIds) for this FaultList + * + * @param elementsList initial data to fill this list with + * @since 5.0 + */ + public void fillIn(List<?> elementsList) { - unfetchedObjects = elementsList.size(); } /** @@ -173,6 +168,9 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * (DataObject or DataRows depending on the query type). */ private void validateListObject(Object object) throws IllegalArgumentException { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } // I am not sure if such a check makes sense??? @@ -197,6 +195,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { return; } + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { if (elements.size() == 0) { return; @@ -295,7 +297,7 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { throw new CayenneRuntimeException("Expected %d objects, retrieved %d", ids.size(), objects.size()); } - // We have less objects then ids + // We have fewer objects than ids // check that we are really missing some ids and throw an exception in that case StringBuilder buffer = null; boolean first = true; @@ -409,9 +411,13 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * next(). */ public Iterator<E> iterator() { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + // by virtue of get(index)'s implementation, resolution of ids into // objects will occur on pageSize boundaries as necessary. - return new Iterator<E>() { + return new Iterator<>() { int listIndex = 0; @@ -459,6 +465,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.Collection#addAll(Collection) */ public boolean addAll(Collection<? extends E> c) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.addAll(c); } @@ -468,6 +478,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.List#addAll(int, Collection) */ public boolean addAll(int index, Collection<? extends E> c) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.addAll(index, c); } @@ -477,6 +491,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.Collection#clear() */ public void clear() { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { elements.clear(); } @@ -486,6 +504,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.Collection#contains(Object) */ public boolean contains(Object o) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.contains(o); } @@ -495,12 +517,20 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.Collection#containsAll(Collection) */ public boolean containsAll(Collection<?> c) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.containsAll(c); } } public E get(int index) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { Object o = elements.get(index); @@ -527,6 +557,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.Collection#isEmpty() */ public boolean isEmpty() { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.isEmpty(); } @@ -537,6 +571,10 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { } public E remove(int index) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { // have to resolve the page to return correct object E object = get(index); @@ -546,18 +584,30 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { } public boolean remove(Object o) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.remove(o); } } public boolean removeAll(Collection<?> c) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.removeAll(c); } } public boolean retainAll(Collection<?> c) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.retainAll(c); } @@ -578,12 +628,20 @@ public class IncrementalFaultList<E> implements List<E>, Serializable { * @see java.util.Collection#size() */ public int size() { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { return elements.size(); } } public List<E> subList(int fromIndex, int toIndex) { + if(elements == null) { + throw new IllegalStateException("IncrementalFaultList is not initialized"); + } + synchronized (elements) { resolveInterval(fromIndex, toIndex); return elements.subList(fromIndex, toIndex); diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/MixedResultIncrementalFaultList.java b/cayenne-server/src/main/java/org/apache/cayenne/access/MixedResultIncrementalFaultList.java index 33c5da5b1..471259ca3 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/MixedResultIncrementalFaultList.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/MixedResultIncrementalFaultList.java @@ -70,8 +70,8 @@ class MixedResultIncrementalFaultList<E> extends IncrementalFaultList<E> { * @param query Main query used to retrieve data. Must have "pageSize" * property set to a value greater than zero. */ - MixedResultIncrementalFaultList(DataContext dataContext, Query query, int maxFetchSize) { - super(dataContext, query, maxFetchSize); + MixedResultIncrementalFaultList(DataContext dataContext, Query query, int maxFetchSize, List<?> data) { + super(dataContext, query, maxFetchSize, data); } @Override @@ -89,8 +89,8 @@ class MixedResultIncrementalFaultList<E> extends IncrementalFaultList<E> { } } - // if there is no entities in this results, - // than all data is already there and we don't need to resolve any objects + // if there is no entities in these results, + // then all data is already there, and we don't need to resolve any objects if(indexToEntity.isEmpty()) { return new ScalarArrayListHelper(); } else { @@ -98,18 +98,6 @@ class MixedResultIncrementalFaultList<E> extends IncrementalFaultList<E> { } } - @Override - protected void fillIn(final Query query, List<Object> elementsList) { - elementsList.clear(); - try (ResultIterator it = dataContext.performIteratedQuery(query)) { - while (it.hasNextRow()) { - elementsList.add(it.nextRow()); - } - } - - unfetchedObjects = elementsList.size(); - } - @Override protected void resolveInterval(int fromIndex, int toIndex) { if (fromIndex >= toIndex || scalarResult) { diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/SimpleIdIncrementalFaultList.java b/cayenne-server/src/main/java/org/apache/cayenne/access/SimpleIdIncrementalFaultList.java index d52e8cafa..c9c586048 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/SimpleIdIncrementalFaultList.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/SimpleIdIncrementalFaultList.java @@ -19,6 +19,7 @@ package org.apache.cayenne.access; import java.util.Collection; +import java.util.List; import java.util.Map; import org.apache.cayenne.Persistent; @@ -38,8 +39,8 @@ class SimpleIdIncrementalFaultList<E> extends IncrementalFaultList<E> { protected DbAttribute pk; - SimpleIdIncrementalFaultList(DataContext dataContext, Query query, int maxFetchSize) { - super(dataContext, query, maxFetchSize); + SimpleIdIncrementalFaultList(DataContext dataContext, Query query, int maxFetchSize, List<?> data) { + super(dataContext, query, maxFetchSize, data); Collection<DbAttribute> pks = rootEntity.getDbEntity().getPrimaryKeys(); if (pks.size() != 1) { diff --git a/cayenne-server/src/test/java/org/apache/cayenne/access/IncrementalFaultListIT.java b/cayenne-server/src/test/java/org/apache/cayenne/access/IncrementalFaultListIT.java index 51822def5..388cce6ac 100644 --- a/cayenne-server/src/test/java/org/apache/cayenne/access/IncrementalFaultListIT.java +++ b/cayenne-server/src/test/java/org/apache/cayenne/access/IncrementalFaultListIT.java @@ -27,6 +27,8 @@ import org.apache.cayenne.unit.di.server.UseServerRuntime; import org.apache.cayenne.util.Util; import org.junit.Test; +import java.util.List; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -41,7 +43,7 @@ public class IncrementalFaultListIT extends ServerCase { ObjectSelect<Artist> query = ObjectSelect.query(Artist.class) .pageSize(10); - IncrementalFaultList<Artist> i1 = new IncrementalFaultList<Artist>(context, query, 10); + IncrementalFaultList<Artist> i1 = new IncrementalFaultList<Artist>(context, query, 10, List.of()); IncrementalFaultList<Artist> i2 = Util.cloneViaSerialization(i1); assertNotNull(i2); diff --git a/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListDataRowsIT.java b/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListDataRowsIT.java index 3808fe113..fa9110de8 100644 --- a/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListDataRowsIT.java +++ b/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListDataRowsIT.java @@ -64,8 +64,11 @@ public class SimpleIdIncrementalFaultListDataRowsIT extends ServerCase { createArtistsDataSet(); ObjectSelect<DataRow> q = ObjectSelect.dataRowQuery(Artist.class) - .pageSize(6).orderBy("db:ARTIST_ID", SortOrder.ASCENDING); - list = new SimpleIdIncrementalFaultList<>(context, q, 10000); + .pageSize(6).orderBy(Artist.ARTIST_ID_PK_PROPERTY.asc()); + List<Long> select = Artist.SELF.columnQuery(Artist.ARTIST_ID_PK_PROPERTY) + .orderBy(Artist.ARTIST_ID_PK_PROPERTY.asc()) + .select(context); + list = new SimpleIdIncrementalFaultList<>(context, q, 10000, select); } protected void createArtistsDataSet() throws Exception { diff --git a/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListIT.java b/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListIT.java index 972712826..a8fc5c408 100644 --- a/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListIT.java +++ b/cayenne-server/src/test/java/org/apache/cayenne/access/SimpleIdIncrementalFaultListIT.java @@ -83,17 +83,32 @@ public class SimpleIdIncrementalFaultListIT extends ServerCase { tArtist.insert(33025, "artist25"); } + private SimpleIdIncrementalFaultList<?> prepareList(int pageSize) throws Exception { + createArtistsDataSet(); + + ObjectSelect<Artist> query = Artist.SELF.query() + // make sure total number of objects is not dividable + // by the page size, to test the last smaller page + .pageSize(pageSize) + .orderBy(Artist.ARTIST_ID_PK_PROPERTY.asc()); + // we need manually fill data here + List<Long> ids = Artist.SELF.columnQuery(Artist.ARTIST_ID_PK_PROPERTY) + .orderBy(Artist.ARTIST_ID_PK_PROPERTY.asc()) + .select(context); + + return new SimpleIdIncrementalFaultList<>(context, query, 10000, ids); + } + @Test public void testRemoveDeleted() throws Exception { createArtistsDataSet(); - // DataContext context = createDataContext(); - - ObjectSelect<Artist> query = ObjectSelect.query(Artist.class) - .pageSize(10); - SimpleIdIncrementalFaultList<Artist> list = new SimpleIdIncrementalFaultList<Artist>( + ObjectSelect<Artist> query = Artist.SELF.query().pageSize(10); + SimpleIdIncrementalFaultList<Artist> list = new SimpleIdIncrementalFaultList<>( context, - query, 10000); + query, + 10000, + Artist.SELF.columnQuery(Artist.ARTIST_ID_PK_PROPERTY).select(context)); assertEquals(25, list.size()); @@ -105,17 +120,6 @@ public class SimpleIdIncrementalFaultListIT extends ServerCase { assertEquals(24, list.size()); } - private SimpleIdIncrementalFaultList<?> prepareList(int pageSize) throws Exception { - createArtistsDataSet(); - - ObjectSelect<Artist> query = ObjectSelect.query(Artist.class) - // make sure total number of objects is not divisable - // by the page size, to test the last smaller page - .pageSize(pageSize) - .orderBy("db:ARTIST_ID", SortOrder.ASCENDING); - return new SimpleIdIncrementalFaultList<>(context, query, 10000); - } - @Test public void testSize() throws Exception { SimpleIdIncrementalFaultList<?> list = prepareList(6); @@ -173,7 +177,11 @@ public class SimpleIdIncrementalFaultListIT extends ServerCase { SimpleIdIncrementalFaultList<Artist> list = new SimpleIdIncrementalFaultList<>( context, - q, 10000); + q, + 10000, + Artist.SELF.columnQuery(Artist.ARTIST_ID_PK_PROPERTY) + .orderBy(Artist.ARTIST_NAME.asc()) + .select(context)); assertSame(newArtist, list.get(25)); }
