From d091dba65a44fdab79d69d1a252d31d37e5b6195 Mon Sep 17 00:00:00 2001 From: Bauke Scholtz Date: Tue, 3 Nov 2026 09:11:05 -0400 Subject: [PATCH 1/2] Order by a collection field via an ID query on DB2 DB2 does not accept a correlated subquery as order expression (SQLSTATE 42703), so ordering on a field of a @OneToMany or @ElementCollection via the COALESCE subquery fails there. On DB2 the page is now loaded in two queries. The first selects the IDs of the page, grouped by ID, ordered by MIN or MAX of a CASE over a LEFT JOIN of the collection, which applies the required criteria on the collection per element. The restrictions are those of the entity query, plus the EXISTS subquery of the count query when a QueryBuilder is used. The second query loads the entities having those IDs, which are then put in the order of the IDs. EclipseLink and OpenJPA do not register parameters which occur only in the ORDER BY clause, so the element criteria also appear in an always true WHERE predicate. OpenJPA cannot render a NULL literal in a CASE, so NULLIF(field, field) is used instead. Follow-up to #61 Co-Authored-By: Claude Opus 5.5 --- .../service/BaseEntityService.java | 227 +++++++++++++++--- .../persistence/service/PageBuilder.java | 29 +++ .../persistence/test/OmniPersistenceIT.java | 59 +++++ 3 files changed, 278 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java b/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java index 08ca42c8..a1443abf 100644 --- a/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java +++ b/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java @@ -18,6 +18,7 @@ import static java.lang.Integer.MAX_VALUE; import static java.lang.String.format; import static java.util.Collections.emptyList; +import static java.util.Comparator.comparing; import static java.util.Optional.ofNullable; import static java.util.logging.Level.FINE; import static java.util.logging.Level.FINER; @@ -27,6 +28,7 @@ import static java.util.stream.Collectors.toMap; import static java.util.stream.Collectors.toSet; import static java.util.stream.IntStream.range; +import static org.omnifaces.persistence.Database.DB2; import static org.omnifaces.persistence.Database.POSTGRESQL; import static org.omnifaces.persistence.Database.SQLSERVER; import static org.omnifaces.persistence.JPA.QUERY_HINT_CACHE_RETRIEVE_MODE; @@ -100,6 +102,7 @@ import jakarta.persistence.criteria.FetchParent; import jakarta.persistence.criteria.From; import jakarta.persistence.criteria.Join; +import jakarta.persistence.criteria.JoinType; import jakarta.persistence.criteria.Order; import jakarta.persistence.criteria.ParameterExpression; import jakarta.persistence.criteria.Path; @@ -1893,7 +1896,7 @@ protected PartialResultList getPage(Page page, boolean count, b var criteriaBuilder = getEntityManager().getCriteriaBuilder(); TypedQuery entityQuery = buildEntityQuery(pageBuilder, criteriaBuilder); var countQuery = count ? buildCountQuery(pageBuilder, criteriaBuilder) : null; - PartialResultList resultList = executeQuery(pageBuilder, entityQuery, countQuery); + PartialResultList resultList = executeQuery(pageBuilder, entityQuery, countQuery, criteriaBuilder); logger.log(FINER, () -> format(LOG_FINER_QUERY_RESULT, resultList, resultList.getEstimatedTotalNumberOfResults())); return resultList; } @@ -1910,7 +1913,7 @@ private TypedQuery buildEntityQuery(PageBuilder pageBuilder, var entityQueryRoot = buildRoot(entityQuery, pageBuilder.getPage()); pageBuilder.setEntityQueryRoot(entityQueryRoot); var pathResolver = buildSelection(pageBuilder, entityQuery, entityQueryRoot, criteriaBuilder); - var parameters = buildRestrictions(pageBuilder, entityQuery, criteriaBuilder, pathResolver); + var parameters = buildRestrictions(pageBuilder, entityQuery, criteriaBuilder, pathResolver, new HashMap<>()); buildOrderBy(pageBuilder, entityQuery, criteriaBuilder, pathResolver, parameters); return buildTypedQuery(pageBuilder, entityQuery, entityQueryRoot, parameters); } @@ -1920,19 +1923,19 @@ private TypedQuery buildCountQuery(PageBuilder pageBuilde var countQueryRoot = countQuery.from(entityType); countQuery.select(criteriaBuilder.count(countQueryRoot)); var parameters = pageBuilder.shouldBuildCountSubquery() - ? buildCountSubquery(pageBuilder, countQuery, countQueryRoot, criteriaBuilder) + ? buildCountSubquery(pageBuilder, countQuery, countQueryRoot, criteriaBuilder, new HashMap<>()) : Collections.emptyMap(); return buildTypedQuery(pageBuilder, countQuery, null, parameters); } private Map buildCountSubquery( - PageBuilder pageBuilder, CriteriaQuery countQuery, Root countRoot, CriteriaBuilder criteriaBuilder + PageBuilder pageBuilder, CriteriaQuery countQuery, Root countRoot, CriteriaBuilder criteriaBuilder, Map parameters ) { var countSubquery = countQuery.subquery(pageBuilder.getResultType()); var countSubqueryRoot = buildRoot(countSubquery, null); var subqueryPathResolver = buildSelection(pageBuilder, countSubquery, countSubqueryRoot, criteriaBuilder); - var parameters = buildRestrictions(pageBuilder, countSubquery, criteriaBuilder, subqueryPathResolver); + buildRestrictions(pageBuilder, countSubquery, criteriaBuilder, subqueryPathResolver, parameters); // SELECT COUNT(e) FROM E e WHERE EXISTS (SELECT t.id FROM T t WHERE [restrictions] AND t.id = e.id) countQuery.where( @@ -1970,25 +1973,115 @@ private static void setMappedParameters(Query query, Map mappedP mappedParameters.forEach(query::setParameter); } - private PartialResultList executeQuery(PageBuilder pageBuilder, TypedQuery entityQuery, TypedQuery countQuery) { + private PartialResultList executeQuery( + PageBuilder pageBuilder, TypedQuery entityQuery, TypedQuery countQuery, CriteriaBuilder criteriaBuilder + ) + { var page = pageBuilder.getPage(); + var entities = pageBuilder.shouldBuildIdQuery() ? getResultListViaIdQuery(pageBuilder, criteriaBuilder) : getResultList(pageBuilder, entityQuery); + + if (pageBuilder.canBuildValueBasedPagingPredicate() && page.isReversed()) { + var reversed = new ArrayList<>(entities); + Collections.reverse(reversed); + entities = reversed; + } + + var estimatedTotalNumberOfResults = countQuery != null ? countQuery.getSingleResult().intValue() : -1; + return new PartialResultList<>(entities, page.getOffset(), estimatedTotalNumberOfResults); + } + + private List getResultList(PageBuilder pageBuilder, TypedQuery entityQuery) { var entities = entityQuery.getResultList(); if ( !entities.isEmpty() && pageBuilder.getEntityQueryRoot() instanceof PostponedFetchRoot postponedFetchRoot && postponedFetchRoot.hasPostponedFetches() ) { - entities = postponedFetchRoot.runPostponedFetches(page, getEntityManager(), entityType, idAttribute.getName(), this::getId, entities); + entities = postponedFetchRoot.runPostponedFetches( + pageBuilder.getPage(), getEntityManager(), entityType, idAttribute.getName(), this::getId, entities + ); } - if (pageBuilder.canBuildValueBasedPagingPredicate() && page.isReversed()) { - var reversed = new ArrayList<>(entities); - Collections.reverse(reversed); - entities = reversed; + return entities; + } + + /** + * Returns the entities of the page in two queries: first the IDs of the page in the desired order, then the entities having those IDs, which are put in the + * order of the IDs. This is used when ordering on a field of a @OneToMany or @ElementCollection on DB2, because DB2 does not + * accept a correlated subquery as order expression (SQLSTATE 42703), see {@link #buildIdQuery(PageBuilder, CriteriaBuilder)}. + */ + private List getResultListViaIdQuery(PageBuilder pageBuilder, CriteriaBuilder criteriaBuilder) { + var ids = buildIdQuery(pageBuilder, criteriaBuilder).getResultList(); + + if (ids.isEmpty()) { + return emptyList(); } - var estimatedTotalNumberOfResults = countQuery != null ? countQuery.getSingleResult().intValue() : -1; - return new PartialResultList<>(entities, page.getOffset(), estimatedTotalNumberOfResults); + var page = pageBuilder.getPage(); + var idsPage = Page.with().range(0, ids.size()).allMatch(page.getRequiredCriteria()).anyMatch(page.getOptionalCriteria()); + page.getOrdering().forEach(idsPage::orderBy); // The ordering is still needed to sort the elements of fetched collections. + var idsPageBuilder = new PageBuilder<>(idsPage.build(), pageBuilder.isCacheable(), pageBuilder.getResultType(), pageBuilder.getQueryBuilder()); + idsPageBuilder.restrictToIds(ids); + + var positions = new HashMap(ids.size()); + range(0, ids.size()).forEach(i -> positions.put(ids.get(i), i)); + var entities = new ArrayList<>(getResultList(idsPageBuilder, buildEntityQuery(idsPageBuilder, criteriaBuilder))); + entities.sort(comparing(entity -> positions.get(this.getId(entity)))); + return entities; + } + + /** + * Builds the query returning the IDs of the page, ordered by the smallest or largest value of a field of a @OneToMany or + * @ElementCollection via an aggregate over a LEFT JOIN of the collection, grouped by the ID. The restrictions are applied the + * same as in the entity query, because some of them are weaker in a subquery on OpenJPA. Those of a {@link QueryBuilder} can only be applied via a + * subquery, so these are additionally applied via an EXISTS subquery, the same as in the count query. All parameters share one map, so that + * their names are unique. The query is not distinct, because it is grouped by the ID. + */ + @SuppressWarnings("unchecked") + private TypedQuery buildIdQuery(PageBuilder pageBuilder, CriteriaBuilder criteriaBuilder) { + var page = pageBuilder.getPage(); + var idQuery = criteriaBuilder.createQuery(identifierType); + var idQueryRoot = idQuery.from(entityType); + var parameters = new HashMap(); + + if (pageBuilder.hasQueryBuilder()) { + buildCountSubquery(pageBuilder, idQuery, idQueryRoot, criteriaBuilder, parameters); + } + + var pathResolver = new RootPathResolver(idQueryRoot, idAttribute.getName(), elementCollections, manyOrOneToOnes); + buildRestrictions(pageBuilder, idQuery, criteriaBuilder, pathResolver, parameters); + var id = (Expression) pathResolver.get(idAttribute.getName()); + var elementPredicates = new ArrayList(); + var groupBy = new ArrayList>(List.of(id)); + var orders = new ArrayList(page.getOrdering().size() + 1); + Entry lastCollectionOrder = null; + + for (var order : page.getOrdering().entrySet()) { + var collectionPath = findCollectionPath(order.getKey()); + + if (collectionPath != null) { + lastCollectionOrder = order; + var aggregate = buildCollectionAggregate(order, collectionPath, page, idQueryRoot, criteriaBuilder, elementPredicates, parameters); + orders.add(order.getValue() ? criteriaBuilder.asc(aggregate) : criteriaBuilder.desc(aggregate)); + continue; + } + + if (!ID.equals(order.getKey()) && !idAttribute.getName().equals(order.getKey())) { + groupBy.add(pathResolver.get(order.getKey())); + } + + orders.add(buildOrder(order, criteriaBuilder, pathResolver, false)); + } + + if (!elementPredicates.isEmpty()) { + // EclipseLink and OpenJPA do not register parameters which occur only in the ORDER BY clause. + var registration = criteriaBuilder.or(criteriaBuilder.and(toArray(elementPredicates)), criteriaBuilder.isNotNull(id)); + idQuery.where(conjunctRestrictionsIfNecessary(criteriaBuilder, idQuery.getRestriction(), registration)); + } + + addIdTiebreakerIfNecessary(orders, page.getOrdering(), lastCollectionOrder, criteriaBuilder, pathResolver); + idQuery.select(id).distinct(false).groupBy(groupBy).orderBy(orders); + return buildTypedQuery(pageBuilder, idQuery, idQueryRoot, parameters); } // Selection actions ---------------------------------------------------------------------------------------------- @@ -2037,6 +2130,7 @@ private PathResolver buildSelection(PageBuilder pageBuilder, Ab } else if (pageBuilder.getResultType() == entityType) { pageBuilder.shouldBuildCountSubquery(mapping != null); // mapping is empty but not null when getPage(..., QueryBuilder) is used. + pageBuilder.hasQueryBuilder(mapping != null); // A collection ordering is a correlated subquery, and a non-null mapping may aggregate, so value based paging cannot be reliably used then. pageBuilder.canBuildValueBasedPagingPredicate(mapping == null && !containsCollectionOrdering(pageBuilder.getPage())); return new RootPathResolver(root, idAttribute.getName(), elementCollections, manyOrOneToOnes); @@ -2074,15 +2168,14 @@ private void buildOrderBy( var page = pageBuilder.getPage(); var ordering = page.getOrdering(); - if (ordering.isEmpty()) { + if (ordering.isEmpty() || pageBuilder.getIds() != null) { // The entities restricted to IDs are put in the order of the IDs afterwards. return; } var reversed = pageBuilder.canBuildValueBasedPagingPredicate() && page.isReversed(); var root = pathResolver.get(null); var orders = new ArrayList(ordering.size() + 1); - var collectionOrdered = false; - var tiebreakerAscending = false; + Entry lastCollectionOrder = null; for (var order : ordering.entrySet()) { var collectionPath = pathResolver instanceof MappedPathResolver ? null : findCollectionPath(order.getKey()); @@ -2092,8 +2185,7 @@ private void buildOrderBy( continue; } - collectionOrdered = true; - tiebreakerAscending = order.getValue(); + lastCollectionOrder = order; if (!(root instanceof PostponedFetchRoot) && isFetched((From) root, collectionPath)) { // The fetched elements are selected, also in a distinct query. @@ -2102,18 +2194,31 @@ private void buildOrderBy( else if (criteriaQuery.isDistinct()) { logger.log(WARNING, () -> format(LOG_WARNING_UNSUPPORTED_DISTINCT_COLLECTION_ORDER, order.getKey())); } + else if (getDatabase() == DB2) { + pageBuilder.shouldBuildIdQuery(true); // The order is then built by buildIdQuery. + } else { orders.add(buildCollectionOrder(order, collectionPath, page, criteriaQuery, criteriaBuilder, pathResolver, parameters)); } } - if (collectionOrdered && !ordering.containsKey(ID) && !ordering.containsKey(idAttribute.getName())) { - // The ID keeps the page order stable when collection elements tie. Its direction is that of the last collection order. + addIdTiebreakerIfNecessary(orders, ordering, lastCollectionOrder, criteriaBuilder, pathResolver); + criteriaQuery.orderBy(orders); + } + + /** + * Adds the ID as last order when there is a collection order, so that the page order stays stable when collection elements tie. Its direction is that of + * the last collection order. + */ + private void addIdTiebreakerIfNecessary( + List orders, Map ordering, Entry lastCollectionOrder, CriteriaBuilder criteriaBuilder, + PathResolver pathResolver + ) + { + if (lastCollectionOrder != null && !ordering.containsKey(ID) && !ordering.containsKey(idAttribute.getName())) { var id = pathResolver.get(idAttribute.getName()); - orders.add(tiebreakerAscending ? criteriaBuilder.asc(id) : criteriaBuilder.desc(id)); + orders.add(lastCollectionOrder.getValue() ? criteriaBuilder.asc(id) : criteriaBuilder.desc(id)); } - - criteriaQuery.orderBy(orders); } private boolean containsCollectionOrdering(Page page) { @@ -2161,7 +2266,8 @@ private static boolean isFetched(From root, String collectionPath) { * would JOIN the collection and multiply the rows. Where entities without elements are placed, depends on how the database orders null. *

* The order is a correlated subquery wrapped in COALESCE with a NULL fallback, because EclipseLink does not accept a subquery as - * order expression, and SQL Server does not accept COALESCE with only one argument. + * order expression, and SQL Server does not accept COALESCE with only one argument. DB2 accepts neither, so there the order is built by + * {@link #buildIdQuery(PageBuilder, CriteriaBuilder)} instead. */ @SuppressWarnings({ "unchecked", "rawtypes" }) private Order buildCollectionOrder( @@ -2180,12 +2286,59 @@ private Order buildCollectionOrder( var joins = new HashMap>(); var predicates = new ArrayList(); predicates.add(criteriaBuilder.equal(subqueryRoot.get(idAttribute.getName()), pathResolver.get(idAttribute.getName()))); + predicates.addAll(buildElementPredicates(collection, collectionPath, JoinType.INNER, joins, page, criteriaBuilder, parameters)); + Expression field = getElementPath(collection, collectionPath, order.getKey(), JoinType.INNER, joins); + subquery.select(order.getValue() ? criteriaBuilder.least(field) : criteriaBuilder.greatest(field)).where(toArray(predicates)); + var expression = criteriaBuilder.function("COALESCE", field.getJavaType(), subquery, criteriaBuilder.nullLiteral(field.getJavaType())); + return order.getValue() ? criteriaBuilder.asc(expression) : criteriaBuilder.desc(expression); + } + + /** + * Builds the smallest value of a field of a @OneToMany or @ElementCollection among the elements of the collection when given + * ordering is ascending, or else the largest one, as an aggregate over a LEFT JOIN of the collection, for use in a query grouped by the ID. It + * considers the same elements as {@link #buildCollectionOrder(Entry, String, Page, CriteriaQuery, CriteriaBuilder, PathResolver, Map)}: the required + * criteria on the collection are applied per element via a CASE, so that an entity without matching elements gets NULL. These + * criteria are added to given element predicates. + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + private Expression buildCollectionAggregate( + Entry order, String collectionPath, Page page, Root root, CriteriaBuilder criteriaBuilder, List elementPredicates, + Map parameters + ) + { + From collection = root; + + for (var attribute : collectionPath.split("\\.")) { + collection = collection.join(attribute, JoinType.LEFT); + } + + var joins = new HashMap>(); + var predicates = buildElementPredicates(collection, collectionPath, JoinType.LEFT, joins, page, criteriaBuilder, parameters); + var field = (Expression) getElementPath(collection, collectionPath, order.getKey(), JoinType.LEFT, joins); + elementPredicates.addAll(predicates); + // NULLIF(field, field) is NULL; OpenJPA cannot render a NULL literal in a CASE. + Expression value = predicates.isEmpty() + ? field + : criteriaBuilder.selectCase().when(criteriaBuilder.and(toArray(predicates)), field) + .otherwise(criteriaBuilder.nullif(field, (Expression) field)); + return order.getValue() ? criteriaBuilder.least(value) : criteriaBuilder.greatest(value); + } + + /** + * Returns the predicates of the required criteria on given collection, applied per element, with multiple values and a negation applied per element. + */ + private List buildElementPredicates( + From collection, String collectionPath, JoinType joinType, Map> joins, Page page, CriteriaBuilder criteriaBuilder, + Map parameters + ) + { + var predicates = new ArrayList(); for (var criteria : page.getRequiredCriteria().entrySet()) { var field = criteria.getKey(); if (field.equals(collectionPath) || field.startsWith(collectionPath + ".")) { - var path = getElementPath(collection, collectionPath, field, joins); + var path = getElementPath(collection, collectionPath, field, joinType, joins); var predicate = buildElementPredicate( path, path.getJavaType(), criteria.getValue(), criteriaBuilder, new UncheckedParameterBuilder(field, criteriaBuilder, parameters) ); @@ -2196,17 +2349,14 @@ private Order buildCollectionOrder( } } - Expression field = getElementPath(collection, collectionPath, order.getKey(), joins); - subquery.select(order.getValue() ? criteriaBuilder.least(field) : criteriaBuilder.greatest(field)).where(toArray(predicates)); - var expression = criteriaBuilder.function("COALESCE", field.getJavaType(), subquery, criteriaBuilder.nullLiteral(field.getJavaType())); - return order.getValue() ? criteriaBuilder.asc(expression) : criteriaBuilder.desc(expression); + return predicates; } /** * Returns the path of given field of an element of given collection. JOINs are shared via given map, so that the criteria and the order on the elements * apply to the same elements. */ - private Expression getElementPath(From collection, String collectionPath, String field, Map> joins) { + private Expression getElementPath(From collection, String collectionPath, String field, JoinType joinType, Map> joins) { if (field.equals(collectionPath)) { return collection; } @@ -2224,7 +2374,7 @@ private Expression getElementPath(From collection, String collectionPat var parent = from; var attribute = attributes[i]; path += "." + attribute; - from = joins.computeIfAbsent(path, k -> parent.join(attribute)); + from = joins.computeIfAbsent(path, k -> parent.join(attribute, joinType)); } return from; @@ -2238,11 +2388,10 @@ private static Order buildOrder(Entry order, CriteriaBuilder cr // Searching actions ----------------------------------------------------------------------------------------------- private Map buildRestrictions( - PageBuilder pageBuilder, AbstractQuery query, CriteriaBuilder criteriaBuilder, PathResolver pathResolver + PageBuilder pageBuilder, AbstractQuery query, CriteriaBuilder criteriaBuilder, PathResolver pathResolver, Map parameters ) { var page = pageBuilder.getPage(); - var parameters = new HashMap(page.getRequiredCriteria().size() + page.getOptionalCriteria().size()); var requiredPredicates = buildPredicates(page.getRequiredCriteria(), query, criteriaBuilder, pathResolver, parameters); var optionalPredicates = buildPredicates(page.getOptionalCriteria(), query, criteriaBuilder, pathResolver, parameters); Predicate restriction = null; @@ -2284,6 +2433,10 @@ criteriaBuilder, restriction, buildValueBasedPagingPredicate(page, criteriaBuild ); } + if (!(query instanceof Subquery) && pageBuilder.getIds() != null) { + restriction = conjunctRestrictionsIfNecessary(criteriaBuilder, restriction, pathResolver.get(idAttribute.getName()).in(pageBuilder.getIds())); + } + if (restriction != null) { var distinct = !(pathResolver instanceof MappedPathResolver) // DTO queries have GROUP BY from aggregations; DISTINCT is redundant and causes // OpenJPA to wrap the query losing projected columns. @@ -2328,8 +2481,8 @@ private > Predicate buildValueBasedPagingPredicate( return criteriaBuilder.or(toArray(predicates)); } - private List buildPredicates( - Map criteria, AbstractQuery query, CriteriaBuilder criteriaBuilder, PathResolver pathResolver, Map parameters + private List buildPredicates( + Map criteria, AbstractQuery query, CriteriaBuilder criteriaBuilder, PathResolver pathResolver, Map parameters ) { return stream(criteria) @@ -2338,8 +2491,8 @@ private List buildPredicates( .collect(toList()); } - private Predicate buildPredicate( - Entry parameter, AbstractQuery query, CriteriaBuilder criteriaBuilder, PathResolver pathResolver, Map parameters + private Predicate buildPredicate( + Entry parameter, AbstractQuery query, CriteriaBuilder criteriaBuilder, PathResolver pathResolver, Map parameters ) { var field = parameter.getKey(); diff --git a/src/main/java/org/omnifaces/persistence/service/PageBuilder.java b/src/main/java/org/omnifaces/persistence/service/PageBuilder.java index 42359c1e..a5733fc5 100644 --- a/src/main/java/org/omnifaces/persistence/service/PageBuilder.java +++ b/src/main/java/org/omnifaces/persistence/service/PageBuilder.java @@ -12,6 +12,8 @@ */ package org.omnifaces.persistence.service; +import java.util.List; + import jakarta.persistence.criteria.Root; import org.omnifaces.persistence.model.dto.Page; @@ -33,6 +35,9 @@ class PageBuilder { private Root entityQueryRoot; private boolean shouldBuildCountSubquery; private boolean canBuildValueBasedPagingPredicate; + private boolean shouldBuildIdQuery; + private boolean hasQueryBuilder; + private List ids; public PageBuilder(Page page, boolean cacheable, Class resultType, MappedQueryBuilder queryBuilder) { this.page = page; @@ -66,6 +71,30 @@ public boolean canBuildValueBasedPagingPredicate() { return canBuildValueBasedPagingPredicate; } + public void shouldBuildIdQuery(boolean yes) { + shouldBuildIdQuery |= yes; + } + + public boolean shouldBuildIdQuery() { + return shouldBuildIdQuery; + } + + public void hasQueryBuilder(boolean yes) { + hasQueryBuilder |= yes; + } + + public boolean hasQueryBuilder() { + return hasQueryBuilder; + } + + public void restrictToIds(List ids) { + this.ids = ids; + } + + public List getIds() { + return ids; + } + public Page getPage() { return page; } diff --git a/src/test/java/org/omnifaces/persistence/test/OmniPersistenceIT.java b/src/test/java/org/omnifaces/persistence/test/OmniPersistenceIT.java index 4de8713f..eb42d102 100644 --- a/src/test/java/org/omnifaces/persistence/test/OmniPersistenceIT.java +++ b/src/test/java/org/omnifaces/persistence/test/OmniPersistenceIT.java @@ -784,6 +784,65 @@ void testPageOrderedByOneToManyWithListAndNegatedCriteriaOnIt() { assertOrderedAcrossPages(withoutOne, key, true, (offset, limit) -> personService().getPage(negatedPage.apply(offset, limit), false)); } + /** + * Ordering on a collection field must not drop entities which match the criteria on the collection but have no single element matching all of them, such as + * an entity with only a MOBILE phone for "not both MOBILE and HOME", or an entity whose MOBILE phone and whose number with a 1 are on different phones. + */ + @Test + void testPageOrderedByOneToManyKeepsEntitiesWithoutMatchingElement() { + var persons = personService().getAllWithPhones(); + var notBothMobileAndHome = Map.of("phones.type", Not.value(List.of(Phone.Type.MOBILE, Phone.Type.HOME))); + assertTrue( + persons.stream().map(OmniPersistenceIT::phoneTypes) + .anyMatch(types -> types.equals(Set.of(Phone.Type.MOBILE)) || types.equals(Set.of(Phone.Type.HOME))), + "Some person has only MOBILE or only HOME phones" + ); + assertKeepsAllMatchingEntities("phones.number", notBothMobileAndHome); + + var mobileAndNumberWith1 = Map.of("phones.type", Phone.Type.MOBILE, "phones.number", Like.contains("1")); + assertTrue( + persons.stream().map(Person::getPhones).anyMatch( + phones -> phones.stream().anyMatch(phone -> phone.getType() == Phone.Type.MOBILE) + && phones.stream().anyMatch(phone -> phone.getNumber().contains("1")) + && phones.stream().noneMatch(phone -> phone.getType() == Phone.Type.MOBILE && phone.getNumber().contains("1")) + ), + "Some person has a MOBILE phone and a number with a 1 on different phones" + ); + assertKeepsAllMatchingEntities("phones.number", mobileAndNumberWith1); + } + + /** + * Ordering on an enum field of a @OneToMany and on an @ElementCollection, with criteria on the same collection, must return each + * matching entity once. + */ + @Test + void testPageOrderedByEnumOfCollectionWithCriteriaOnIt() { + assertKeepsAllMatchingEntities("phones.type", Map.of("phones.number", Like.contains("1"))); + assertKeepsAllMatchingEntities("groups", Map.of("groups", Not.value(Group.USER))); + } + + private static Set phoneTypes(Person person) { + return person.getPhones().stream().map(Phone::getType).collect(toSet()); + } + + private void assertKeepsAllMatchingEntities(String orderBy, Map criteria) { + var expected = personService().getPage(Page.with().allMatch(criteria).build(), false).stream().map(Person::getId).sorted().toList(); + assertFalse(expected.isEmpty(), "Some entities match " + criteria); + var ids = new ArrayList(); + + for (var offset = 0;; offset += 10) { + var persons = personService().getPage(Page.with().range(offset, 10).allMatch(criteria).orderBy(orderBy, true).build(), false); + + if (persons.isEmpty()) { + break; + } + + persons.forEach(person -> ids.add(person.getId())); + } + + assertEquals(expected, ids.stream().sorted().toList(), "Each entity matching " + criteria + " is on exactly one page when ordered by " + orderBy); + } + @Test void testPageOrderedByOneToManyWithOptionalCriteria() { var matching = personService().getAllWithPhones().stream() From d9d9d7ca5540a24c107873b8673d6ee40a89773d Mon Sep 17 00:00:00 2001 From: Bauke Scholtz Date: Tue, 3 Nov 2026 09:18:28 -0400 Subject: [PATCH 2/2] Derive the count subquery flag from hasQueryBuilder Co-Authored-By: Claude Opus 5.5 --- .../org/omnifaces/persistence/service/BaseEntityService.java | 3 +-- .../java/org/omnifaces/persistence/service/PageBuilder.java | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java b/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java index a1443abf..542e7c7e 100644 --- a/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java +++ b/src/main/java/org/omnifaces/persistence/service/BaseEntityService.java @@ -2129,8 +2129,7 @@ private PathResolver buildSelection(PageBuilder pageBuilder, Ab return new MappedPathResolver(root, idAttribute.getName(), paths, elementCollections, manyOrOneToOnes); } else if (pageBuilder.getResultType() == entityType) { - pageBuilder.shouldBuildCountSubquery(mapping != null); // mapping is empty but not null when getPage(..., QueryBuilder) is used. - pageBuilder.hasQueryBuilder(mapping != null); + pageBuilder.hasQueryBuilder(mapping != null); // mapping is empty but not null when getPage(..., QueryBuilder) is used. // A collection ordering is a correlated subquery, and a non-null mapping may aggregate, so value based paging cannot be reliably used then. pageBuilder.canBuildValueBasedPagingPredicate(mapping == null && !containsCollectionOrdering(pageBuilder.getPage())); return new RootPathResolver(root, idAttribute.getName(), elementCollections, manyOrOneToOnes); diff --git a/src/main/java/org/omnifaces/persistence/service/PageBuilder.java b/src/main/java/org/omnifaces/persistence/service/PageBuilder.java index a5733fc5..71ace457 100644 --- a/src/main/java/org/omnifaces/persistence/service/PageBuilder.java +++ b/src/main/java/org/omnifaces/persistence/service/PageBuilder.java @@ -60,7 +60,7 @@ public void shouldBuildCountSubquery(boolean yes) { } public boolean shouldBuildCountSubquery() { - return shouldBuildCountSubquery; + return shouldBuildCountSubquery || hasQueryBuilder; } public void canBuildValueBasedPagingPredicate(boolean yes) {