From c28afd0ba250b41105ea0a710e24a8a8f00b7690 Mon Sep 17 00:00:00 2001 From: Elia Mezzano Date: Fri, 31 Jul 2026 11:23:31 +0200 Subject: [PATCH 1/2] Fixed common search returning multiple values in case of join --- .../services/content/AbstractContentSearcherDAO.java | 4 +++- .../jacms/aps/system/services/resource/ResourceDAO.java | 2 ++ .../aps/system/services/content/ContentSearcherDAO.java | 4 +++- .../agiletec/aps/system/common/AbstractSearcherDAO.java | 9 ++++++++- .../system/common/entity/AbstractEntitySearcherDAO.java | 2 ++ 5 files changed, 18 insertions(+), 3 deletions(-) diff --git a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java index 575e5c2b91..94c628fc97 100644 --- a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java +++ b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java @@ -220,7 +220,9 @@ protected String createQueryString(EntitySearchFilter[] filters, String[] groups if (!isCount) { boolean ordered = this.appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); - } + } else { + this.closeMasterCountQueryBlock(query); + } //System.out.println("********** " + query.toString()); return query.toString(); } diff --git a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java index 00e6d41760..4094dd1699 100644 --- a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java +++ b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java @@ -382,6 +382,8 @@ private String createQueryString(FieldSearchFilter[] filters, List categ if (!isCount) { super.appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); + } else { + this.closeMasterCountQueryBlock(query); } return query.toString(); } diff --git a/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java b/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java index 077e2f3bab..ef0177585e 100644 --- a/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java +++ b/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java @@ -174,7 +174,9 @@ private String createQueryString(List workflowFilters, if (!isCount) { appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); - } + } else { + this.closeMasterCountQueryBlock(query); + } return query.toString(); } diff --git a/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java b/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java index 8a192e97b7..ec25051ac7 100644 --- a/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java @@ -231,6 +231,8 @@ protected String createQueryString(FieldSearchFilter[] filters, boolean isCount, if (!isCount) { boolean ordered = appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); + } else { + this.closeMasterCountQueryBlock(query); } return query.toString(); } @@ -247,11 +249,16 @@ protected StringBuffer createBaseQueryBlock(FieldSearchFilter[] filters, boolean protected StringBuffer createMasterCountQueryBlock() { String masterTableName = this.getMasterTableName(); - StringBuffer query = new StringBuffer("SELECT COUNT(*)"); + StringBuffer query = new StringBuffer("SELECT COUNT(*) FROM ( SELECT DISTINCT "); + query.append(masterTableName).append(".").append(this.getMasterTableIdFieldName()); query.append(" FROM ").append(masterTableName).append(" "); return query; } + protected void closeMasterCountQueryBlock(StringBuffer query) { + query.append(") counter"); + } + private StringBuffer createMasterSelectQueryBlock(FieldSearchFilter[] filters, boolean selectAll) { String masterTableName = this.getMasterTableName(); StringBuffer query = new StringBuffer("SELECT ").append(masterTableName).append("."); diff --git a/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java b/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java index cbb25efb0c..c698f299aa 100644 --- a/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java @@ -219,6 +219,8 @@ protected String createQueryString(EntitySearchFilter[] filters, boolean isCount if (!isCount) { boolean ordered = this.appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); + } else { + this.closeMasterCountQueryBlock(query); } return query.toString(); } From ca0d3c6a850123c9b9f6b3d496bb0ec1b4f653df Mon Sep 17 00:00:00 2001 From: "Matteo E. Minnai" Date: Fri, 28 Aug 2026 17:01:07 +0200 Subject: [PATCH 2/2] ESB-1223 The count and the list were two independently maintained queries, so making the count DISTINCT left pagination walking a row set the total no longer described. The count is now the list query's body wrapped - DISTINCT only where a join can multiply a row, and GROUP BY with an aggregate where ordering by a multi-valued attribute makes DISTINCT powerless. Covered by behavioural and SQL-shape tests, and verified on Derby, PostgreSQL, MySQL and Oracle --- .gitignore | 3 + .../content/AbstractContentSearcherDAO.java | 10 +- .../content/PublicContentSearcherDAO.java | 18 +- .../system/services/resource/ResourceDAO.java | 29 +- .../ContentSearchJoinCountRegressionTest.java | 313 +++++++++++++ .../ContentSearcherDaoQueryShapeTest.java | 229 ++++++++++ .../resource/ResourceDaoQueryShapeTest.java | 116 +++++ .../services/resource/TestResourceDAO.java | 4 + .../resource/TestMultipleResourceAction.java | 1 + .../ContentControllerIntegrationTest.java | 85 +++- .../services/content/ContentSearcherDAO.java | 9 +- ...tentWorkflowSearcherDaoQueryShapeTest.java | 80 ++++ .../system/common/AbstractSearcherDAO.java | 144 +++++- .../entity/AbstractEntitySearcherDAO.java | 270 ++++++++++-- .../services/actionlog/ActionLogDAO.java | 2 +- .../java/com/agiletec/ConfigTestUtils.java | 18 + .../aps/system/common/QueryCapture.java | 97 +++++ .../system/common/QueryLimitResolverTest.java | 18 + .../common/SearcherDaoQueryShapeTest.java | 411 ++++++++++++++++++ .../agiletec/aps/system/common/SqlShape.java | 195 +++++++++ pom.xml | 88 ++++ .../system/services/message/MessageDAO.java | 17 +- .../services/message/MessageSearcherDAO.java | 3 +- 23 files changed, 2076 insertions(+), 84 deletions(-) create mode 100644 cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearchJoinCountRegressionTest.java create mode 100644 cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearcherDaoQueryShapeTest.java create mode 100644 cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDaoQueryShapeTest.java create mode 100644 contentworkflow-plugin/src/test/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentWorkflowSearcherDaoQueryShapeTest.java create mode 100644 engine/src/test/java/com/agiletec/aps/system/common/QueryCapture.java create mode 100644 engine/src/test/java/com/agiletec/aps/system/common/SearcherDaoQueryShapeTest.java create mode 100644 engine/src/test/java/com/agiletec/aps/system/common/SqlShape.java diff --git a/.gitignore b/.gitignore index a774f6d738..d8be0908a1 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,6 @@ target work *.tgz derby.log +/.REVIEW/ +/.TESTING/ +/.PLAN/ diff --git a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java index 94c628fc97..8527a86ced 100644 --- a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java +++ b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/AbstractContentSearcherDAO.java @@ -156,7 +156,7 @@ protected PreparedStatement buildStatement(EntitySearchFilter[] filters, //System.out.println("QUERY : " + query); PreparedStatement stat = null; try { - stat = conn.prepareStatement(query); + stat = this.prepareStatement(conn, query); int index = 0; index = super.addAttributeFilterStatementBlock(filters, index, stat); index = this.addMetadataFieldFilterStatementBlock(filters, index, stat); @@ -217,14 +217,12 @@ protected String createQueryString(EntitySearchFilter[] filters, String[] groups hasAppendWhereClause = this.verifyWhereClauseAppend(query, hasAppendWhereClause); this.addGroupsQueryBlock(query, groups); } + boolean grouped = this.appendGroupByQueryBlock(filters, query, selectAll); if (!isCount) { - boolean ordered = this.appendOrderQueryBlocks(filters, query, false); + this.appendOrderQueryBlocks(filters, query, false, grouped); this.appendLimitQueryBlock(filters, query); - } else { - this.closeMasterCountQueryBlock(query); } - //System.out.println("********** " + query.toString()); - return query.toString(); + return this.toQueryString(query, isCount); } protected void addGroupsQueryBlock(StringBuffer query, Collection userGroupCodes) { diff --git a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/PublicContentSearcherDAO.java b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/PublicContentSearcherDAO.java index 28d20951bc..8543162145 100644 --- a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/PublicContentSearcherDAO.java +++ b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/content/PublicContentSearcherDAO.java @@ -41,8 +41,7 @@ public List loadContentsId(String[] categories, } else { groupCodes.addAll(userGroupCodes); } - EntitySearchFilter onLineFilter = new EntitySearchFilter(IContentManager.CONTENT_ONLINE_FILTER_KEY, false); - filters = this.addFilter(filters, onLineFilter); + // the online filter is added by buildStatement, which the count goes through too - see there List contentsId = new ArrayList(); Connection conn = null; PreparedStatement stat = null; @@ -66,10 +65,23 @@ public List loadContentsId(String[] categories, return contentsId; } + /** + * Restrict the search to published contents. + * + *

Applied here because this is the one method both sides pass through: countContents + * and loadContentsId of the base class each call it, so the count and the list are built + * from the same filter set and cannot report different row sets. Adding the filter in the list method + * alone - which is what this class did - made the total count drafts the list would never return.

+ * + *

The filter carries no value, so it emits contents.onlinexml IS NOT NULL with no + * placeholder and binds nothing: the parameter positions below are unaffected by it.

+ */ @Override protected PreparedStatement buildStatement(EntitySearchFilter[] filters, String[] categories, boolean orClauseCategoryFilter, Collection userGroupCodes, boolean isCount, boolean selectAll, Connection conn) { + filters = this.addFilter(filters, + new EntitySearchFilter(IContentManager.CONTENT_ONLINE_FILTER_KEY, false)); ArrayList groups = new ArrayList<>(); ArrayList remainingFilters = new ArrayList<>(); for (EntitySearchFilter filter : filters) { @@ -85,7 +97,7 @@ protected PreparedStatement buildStatement(EntitySearchFilter[] filters, String query = this.createQueryString(filters, groupsArr, categories, orClauseCategoryFilter, groupsForSelect, isCount, selectAll); PreparedStatement stat = null; try { - stat = conn.prepareStatement(query); + stat = this.prepareStatement(conn, query); int index = 0; index = super.addAttributeFilterStatementBlock(filters, index, stat); index = this.addMetadataFieldFilterStatementBlock(filters, index, stat); diff --git a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java index 4094dd1699..2582e8cecc 100644 --- a/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java +++ b/cms-plugin/src/main/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDAO.java @@ -361,7 +361,7 @@ private PreparedStatement buildStatement(FieldSearchFilter[] filters, List 0) { for (String category : categories) { @@ -382,14 +382,14 @@ private String createQueryString(FieldSearchFilter[] filters, List categ if (!isCount) { super.appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); - } else { - this.closeMasterCountQueryBlock(query); } - return query.toString(); + return this.toQueryString(query, isCount); } private StringBuffer createBaseQueryBlock(FieldSearchFilter[] filters, boolean selectAll, boolean isCount, List categories) { - StringBuffer query = super.createBaseQueryBlock(filters, isCount, selectAll); + // count and list share one body: the category joins are the only thing that can return several + // rows per resource, and both sides have to see the same set + StringBuffer query = this.createMasterSelectQueryBlock(filters, selectAll); if (categories != null) { for (int i = 0; i < categories.size(); i++) { query.append(String.format( @@ -563,6 +563,25 @@ public void updateResourceRelations(ResourceInterface resource) { } } + /** + * A resource holds one resourcerelations row per category, and nothing in the schema + * forbids the same pair twice, so the joined query can return the resource more than once. Both the + * list and the count select distinct ids; the columns the ORDER BY references have to be projected + * as well, since Derby and PostgreSQL reject an ORDER BY outside the select list under DISTINCT. + */ + @Override + protected StringBuffer createMasterSelectQueryBlock(FieldSearchFilter[] filters, boolean selectAll) { + if (selectAll) { + return super.createMasterSelectQueryBlock(filters, selectAll); + } + String masterTableName = this.getMasterTableName(); + StringBuffer query = new StringBuffer("SELECT DISTINCT ").append(masterTableName).append(".") + .append(this.getMasterTableIdFieldName()); + this.appendOrderFieldsSelectBlock(filters, query); + query.append(" FROM ").append(masterTableName).append(" "); + return query; + } + @Override protected String getMasterTableName() { return "resources"; diff --git a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearchJoinCountRegressionTest.java b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearchJoinCountRegressionTest.java new file mode 100644 index 0000000000..ba5078aaed --- /dev/null +++ b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearchJoinCountRegressionTest.java @@ -0,0 +1,313 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.plugins.jacms.aps.system.services.content; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.agiletec.aps.BaseTestCase; +import com.agiletec.aps.system.SystemConstants; +import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.entity.model.EntitySearchFilter; +import com.agiletec.aps.system.common.model.dao.SearcherDaoPaginatedResult; +import com.agiletec.aps.system.services.group.IGroupManager; +import com.agiletec.plugins.jacms.aps.system.JacmsSystemConstants; +import com.agiletec.plugins.jacms.aps.system.services.resource.IResourceManager; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Regression coverage for commit c28afd0ba ("Fixed common search returning multiple values in case of + * join"), which made the count query distinct while leaving the list query - and therefore SQL + * pagination - working on the multiplied row set. + * + *

The fixture attribute "Titolo" is stored in workcontentsearch once per language (it, en) for 11 + * contents, so an attribute filter on it joins 22 rows for 11 distinct contents.

+ */ +class ContentSearchJoinCountRegressionTest extends BaseTestCase { + + /** Contents holding the multi-language "Titolo" attribute, in creation-date order. */ + private static final String[] EXPECTED_CONTENTS = {"EVN191", "EVN192", "EVN193", "EVN194", "EVN103", + "EVN20", "EVN23", "EVN24", "EVN25", "EVN41", "EVN21"}; + + private static final String JOINING_ATTRIBUTE = "Titolo"; + + private IContentManager contentManager; + private IResourceManager resourceManager; + private IGroupManager groupManager; + + @BeforeEach + private void init() throws Exception { + this.contentManager = (IContentManager) this.getService(JacmsSystemConstants.CONTENT_MANAGER); + this.resourceManager = (IResourceManager) this.getService(JacmsSystemConstants.RESOURCE_MANAGER); + this.groupManager = (IGroupManager) this.getService(SystemConstants.GROUP_MANAGER); + } + + /** + * The behaviour c28afd0ba intends to deliver: the count must collapse the rows multiplied by the + * join on workcontentsearch. Green after the commit, red before it. + */ + @Test + void countContents_withJoiningAttributeFilter_countsDistinctContents() throws Throwable { + EntitySearchFilter[] filters = {creationDateOrder(), joiningAttributeFilter()}; + List allMatching = this.contentManager.loadWorkContentsId(null, false, filters, allGroups()); + assertEquals(EXPECTED_CONTENTS.length, allMatching.size()); + + Integer count = this.contentManager.countWorkContents(null, false, filters, allGroups()); + assertEquals(EXPECTED_CONTENTS.length, count.intValue()); + } + + /** + * The regression. The count is distinct but the list query is not, and LIMIT/OFFSET is applied to + * the multiplied row set, so a caller that derives the number of pages from the count - as + * PagedMetadata and the admin content finder both do - stops paging before it has seen every + * content. + */ + @Test + void paginatedSearch_withJoiningAttributeFilter_returnsEveryCountedContent() throws Throwable { + int pageSize = EXPECTED_CONTENTS.length; + int declaredCount = paginatedWorkContents(pageSize, 0).getCount(); + int lastPage = lastPage(declaredCount, pageSize); + + // page exactly the way PagedMetadata and the admin content finder do: the declared count is + // the only thing that tells the caller when to stop asking for pages. + Set reachable = new HashSet<>(); + for (int page = 0; page < lastPage; page++) { + reachable.addAll(paginatedWorkContents(pageSize, page * pageSize).getList()); + } + + List unreachable = Arrays.stream(EXPECTED_CONTENTS) + .filter(id -> !reachable.contains(id)) + .collect(Collectors.toList()); + assertTrue(unreachable.isEmpty(), "contents matching the filter but not reachable within the " + + lastPage + " page(s) implied by the declared count of " + declaredCount + ": " + unreachable); + } + + /** + * Same root cause, pre-existing rather than introduced by c28afd0ba: LIMIT slices duplicated rows + * and de-duplication happens per page in Java, so a content whose rows straddle the page boundary + * is served twice. + */ + @Test + void paginatedSearch_withJoiningAttributeFilter_neverRepeatsAnIdAcrossPages() throws Throwable { + int pageSize = EXPECTED_CONTENTS.length; + List seen = new ArrayList<>(); + List repeated = new ArrayList<>(); + // the join yields two rows per content, so walk the whole multiplied row set + for (int offset = 0; offset < EXPECTED_CONTENTS.length * 2; offset += pageSize) { + for (String id : paginatedWorkContents(pageSize, offset).getList()) { + if (seen.contains(id)) { + repeated.add(id); + } else { + seen.add(id); + } + } + } + assertTrue(repeated.isEmpty(), "ids served on more than one page: " + repeated); + } + + /** + * Ordering by the multi-valued attribute - the case DISTINCT cannot collapse, because the + * ORDER BY forces the attribute column into the projection. The body groups on the content id and + * reaches the attribute through an aggregate instead, so the total is exact rather than generous. + * + *

Before the GROUP BY change this reported 22 for 11 contents: consistent and lossless, but it + * drew a second page for data that fits on one.

+ */ + @Test + void countContents_orderedByTheJoiningAttribute_isExact() throws Throwable { + EntitySearchFilter[] filters = {orderedJoiningAttributeFilter()}; + + List ids = this.contentManager.loadWorkContentsId(null, false, filters, allGroups()); + Integer count = this.contentManager.countWorkContents(null, false, filters, allGroups()); + + assertEquals(EXPECTED_CONTENTS.length, count.intValue()); + assertEquals(ids.size(), count.intValue()); + } + + /** + * The invariant of the whole fix, on the ordered-attribute path: every counted content is + * reachable within the pages the count implies, and none is served twice. + */ + @Test + void paginatedSearch_orderedByTheJoiningAttribute_pagesOverContentsNotJoinedRows() throws Throwable { + int pageSize = 4; + int declaredCount = paginatedByAttribute(pageSize, 0).getCount(); + assertEquals(EXPECTED_CONTENTS.length, declaredCount); + + List seen = new ArrayList<>(); + List repeated = new ArrayList<>(); + for (int page = 0; page < lastPage(declaredCount, pageSize); page++) { + List ids = paginatedByAttribute(pageSize, page * pageSize).getList(); + assertTrue(ids.size() <= pageSize, "page " + page + " returned " + ids.size() + " ids"); + for (String id : ids) { + if (seen.contains(id)) { + repeated.add(id); + } else { + seen.add(id); + } + } + } + + assertTrue(repeated.isEmpty(), "ids served on more than one page: " + repeated); + List unreachable = Arrays.stream(EXPECTED_CONTENTS) + .filter(id -> !seen.contains(id)) + .collect(Collectors.toList()); + assertTrue(unreachable.isEmpty(), "contents counted but not reachable by paging: " + unreachable); + } + + /** + * Both directions return every content exactly once and do so deterministically, which is what + * paging needs from an ORDER BY. + * + *

Note what is deliberately not asserted: that DESC is the reverse of ASC. A content + * holding several values sorts on the one the direction asks for - the lowest ascending, the + * highest descending - so on a multi-valued attribute the two orders are not reverses of each + * other. That is the point of the aggregate, not a defect: the alternative is sorting on whichever + * row the database happened to pick. The ASC-to-MIN and DESC-to-MAX mapping is pinned on the + * generated SQL by ContentSearcherDaoQueryShapeTest.

+ */ + @Test + void orderingByTheJoiningAttribute_isCompleteAndStableInBothDirections() throws Throwable { + EntitySearchFilter descending = orderedJoiningAttributeFilter(); + descending.setOrder(EntitySearchFilter.DESC_ORDER); + + List ascending = this.contentManager.loadWorkContentsId(null, false, + new EntitySearchFilter[]{orderedJoiningAttributeFilter()}, allGroups()); + List descendingIds = this.contentManager.loadWorkContentsId(null, false, + new EntitySearchFilter[]{descending}, allGroups()); + + for (List ids : List.of(ascending, descendingIds)) { + assertEquals(EXPECTED_CONTENTS.length, ids.size()); + assertEquals(new HashSet<>(Arrays.asList(EXPECTED_CONTENTS)), new HashSet<>(ids)); + } + assertNotEquals(ascending, descendingIds, "the direction made no difference to the order"); + // a repeated request must slice the same order, or paging can serve a row twice + assertEquals(ascending, this.contentManager.loadWorkContentsId(null, false, + new EntitySearchFilter[]{orderedJoiningAttributeFilter()}, allGroups())); + } + + /** + * Guard: a metadata-only filter builds no join, so the count keeps the value it had before + * c28afd0ba. This is the "unrelated searchers are unaffected" case, expressed on the searcher the + * commit was aimed at. + */ + @Test + void countContents_withoutJoiningFilter_isUnchanged() throws Throwable { + EntitySearchFilter descr = new EntitySearchFilter<>(IContentManager.CONTENT_DESCR_FILTER_KEY, + false, "Cont", true); + EntitySearchFilter[] filters = {creationDateOrder(), descr}; + + List ids = this.contentManager.loadWorkContentsId(null, false, filters, allGroups()); + Integer count = this.contentManager.countWorkContents(null, false, filters, allGroups()); + assertEquals(9, count.intValue()); + assertEquals(ids.size(), count.intValue()); + } + + /** + * The published-content search restricts itself to online contents, and its count must apply the + * same restriction as its list - otherwise the total includes drafts the list will never return and + * the caller is offered pages that come back empty. + * + *

The fixture is a mixed one on purpose: the draft corpus is larger than the published one, so a + * count that ignored the online filter would be visibly larger than the list it describes.

+ */ + @Test + void countPublicContents_appliesTheOnlineFilterItsListApplies() throws Throwable { + EntitySearchFilter[] filters = {creationDateOrder()}; + + List published = this.contentManager.loadPublicContentsId(null, false, filters, allGroups()); + SearcherDaoPaginatedResult paged = + this.contentManager.getPaginatedPublicContentsId(null, false, filters, allGroups()); + + assertFalse(published.isEmpty(), "empty published fixture"); + assertEquals(published.size(), paged.getCount().intValue(), + "the reported total must describe the rows the list can return"); + // and it must genuinely be a subset of the drafts, or the fixture proves nothing + List drafts = this.contentManager.loadWorkContentsId(null, false, filters, allGroups()); + assertTrue(drafts.size() > published.size(), + "fixture no longer has more draft than published contents: " + drafts.size() + + " vs " + published.size()); + } + + /** + * Guard: resourcerelations rows are written from a Set, so the category joins are 1:1 and the + * distinct count cannot change the value resources report - with one category and with two. + */ + @Test + void countResources_matchesResourceListSize() throws Throwable { + assertResourceCountMatchesList(List.of("resCat1")); + assertResourceCountMatchesList(List.of("resCat1", "resCat3")); + } + + private void assertResourceCountMatchesList(List categories) throws Throwable { + SearcherDaoPaginatedResult result = this.resourceManager + .getPaginatedResourcesId(new FieldSearchFilter[0], categories, allGroups()); + assertFalse(result.getList().isEmpty(), "empty fixture for categories " + categories); + assertEquals(result.getList().size(), result.getCount().intValue(), + "count and list disagree for categories " + categories); + } + + private SearcherDaoPaginatedResult paginatedWorkContents(int pageSize, int offset) throws Throwable { + EntitySearchFilter[] filters = {creationDateOrder(), joiningAttributeFilter(), + new EntitySearchFilter<>(pageSize, offset)}; + return this.contentManager.getPaginatedWorkContentsId(null, false, filters, allGroups()); + } + + /** + * Attribute filter with no value and no language code: it matches every workcontentsearch row for + * the attribute, in every language, which is what multiplies the joined rows. + */ + private EntitySearchFilter joiningAttributeFilter() { + return new EntitySearchFilter<>(JOINING_ATTRIBUTE, true); + } + + private SearcherDaoPaginatedResult paginatedByAttribute(int pageSize, int offset) throws Throwable { + EntitySearchFilter[] filters = {orderedJoiningAttributeFilter(), + new EntitySearchFilter<>(pageSize, offset)}; + return this.contentManager.getPaginatedWorkContentsId(null, false, filters, allGroups()); + } + + /** The same multiplying filter, now also carrying the order - which is what forces the grouping. */ + private EntitySearchFilter orderedJoiningAttributeFilter() { + EntitySearchFilter filter = joiningAttributeFilter(); + filter.setOrder(EntitySearchFilter.ASC_ORDER); + return filter; + } + + private EntitySearchFilter creationDateOrder() { + EntitySearchFilter order = new EntitySearchFilter<>( + IContentManager.CONTENT_CREATION_DATE_FILTER_KEY, false); + order.setOrder(EntitySearchFilter.ASC_ORDER); + return order; + } + + private static int lastPage(int count, int pageSize) { + return (int) Math.ceil((double) count / (double) pageSize); + } + + private List allGroups() { + return this.groupManager.getGroups().stream().map(group -> group.getName()) + .collect(Collectors.toList()); + } + +} diff --git a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearcherDaoQueryShapeTest.java b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearcherDaoQueryShapeTest.java new file mode 100644 index 0000000000..d399f01c8b --- /dev/null +++ b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearcherDaoQueryShapeTest.java @@ -0,0 +1,229 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.plugins.jacms.aps.system.services.content; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.QueryCapture; +import com.agiletec.aps.system.common.SqlShape; +import com.agiletec.aps.system.common.entity.model.EntitySearchFilter; +import com.agiletec.aps.system.services.group.Group; +import java.util.Collection; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * The shape of the SQL the content searchers generate, asserted without a database. + * + *

An attribute filter joins the search table, which holds one row per content per attribute per + * language, so the joined query can return a content several times. The count and the list are one + * body precisely so that the multiplication is seen by both.

+ * + * @see QueryCapture + */ +class ContentSearcherDaoQueryShapeTest { + + private static final String DERBY = "org.apache.derby.jdbc.EmbeddedDriver"; + private static final String ATTRIBUTE = "Titolo"; + /** An admin sees every group, so no group block is added and the shape stays readable. */ + private static final Collection ADMIN = List.of(Group.ADMINS_GROUP_NAME); + + private QueryCapture capture; + + @BeforeEach + void setUp() { + this.capture = new QueryCapture(); + } + + @Test + void contentCount_isTheListBodyWrapped() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + EntitySearchFilter[] filters = {attributeLike(), creationDateOrder()}; + String[] categories = {"cat1"}; + Collection groups = List.of("customers"); + + dao.countContents(categories, false, filters, groups); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.loadContentsId(categories, false, filters, groups); + String listQuery = this.capture.single(); + + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + assertEquals(1, SqlShape.occurrences(countQuery, SqlShape.COUNT_PREFIX)); + assertEquals(1, SqlShape.occurrences(countQuery, SqlShape.COUNT_SUFFIX)); + assertTrue(SqlShape.normalize(countQuery).contains("contents.maingroup = ?"), countQuery); + } + + @Test + void contentSelectBlock_isDistinctAndProjectsOnlyTheOrderedColumns() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + + dao.loadContentsId(null, false, new EntitySearchFilter[]{attributeLike(), creationDateOrder()}, ADMIN); + + String query = this.capture.single(); + assertTrue(SqlShape.isDistinct(query), query); + assertEquals(List.of("contents.contentid", "contents.created"), SqlShape.selectedColumns(query)); + assertEquals(List.of("contents.created", "contents.contentid"), SqlShape.orderedColumns(query)); + assertEquals(List.of("workcontentsearch"), SqlShape.joinedTables(query)); + } + + /** + * The LIKE filter's value column used to be projected and aliased, and nothing ever read it back. + * Under DISTINCT it would make the content distinct once per language, which is the defect. + */ + @Test + void contentListQuery_dropsTheColumnsProjectedOnlyForALikeFilter() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + + dao.loadContentsId(null, false, new EntitySearchFilter[]{attributeLike()}, ADMIN); + + String query = this.capture.single(); + assertEquals(List.of("contents.contentid"), SqlShape.selectedColumns(query)); + assertFalse(SqlShape.normalize(query).contains("AS textvalue"), query); + } + + /** + * Ordering by an attribute is the case DISTINCT cannot collapse: the ORDER BY names a + * column of the joined search table, and projecting it - which DISTINCT would require - makes the + * content distinct once per language. The body groups on the content id instead and reaches the + * attribute through an aggregate, which needs no projection. + * + *

The count wraps that same grouped body, so it counts contents and its total is exact.

+ */ + @Test + void orderingByAMultiValuedAttribute_groupsInsteadOfProjectingTheAttribute() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + EntitySearchFilter[] filters = {ordered(attributeLike())}; + + dao.countContents(null, false, filters, ADMIN); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.loadContentsId(null, false, filters, ADMIN); + String listQuery = this.capture.single(); + + assertEquals(List.of("contents.contentid"), SqlShape.selectedColumns(listQuery)); + assertEquals(List.of("contents.contentid"), SqlShape.groupedColumns(listQuery)); + assertFalse(SqlShape.isDistinct(listQuery), listQuery); + assertEquals(List.of("MIN(workcontentsearch0.textvalue)", "contents.contentid"), + SqlShape.orderedColumns(listQuery)); + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + assertTrue(SqlShape.isGrouped(countQuery), countQuery); + } + + /** + * ASC takes the lowest value a content holds, DESC the highest - so a content sorts on the value + * the requested direction actually asks for, rather than on whichever joined row the database + * happened to pick. + */ + @Test + void theAggregateFollowsTheRequestedDirection() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + EntitySearchFilter descending = attributeLike(); + descending.setOrder(FieldSearchFilter.DESC_ORDER); + + dao.loadContentsId(null, false, new EntitySearchFilter[]{ordered(attributeLike())}, ADMIN); + String ascending = this.capture.single(); + this.capture.clear(); + dao.loadContentsId(null, false, new EntitySearchFilter[]{descending}, ADMIN); + String descendingQuery = this.capture.single(); + + assertEquals(List.of("MIN(workcontentsearch0.textvalue)", "contents.contentid"), + SqlShape.orderedColumns(ascending)); + assertEquals(List.of("MAX(workcontentsearch0.textvalue)", "contents.contentid"), + SqlShape.orderedColumns(descendingQuery)); + // the tie-breaker keeps following the direction it breaks + assertTrue(SqlShape.normalize(descendingQuery).endsWith("contents.contentid DESC"), descendingQuery); + } + + /** + * The paging block sits after an aggregate ORDER BY over a grouped body - a shape none of the + * engines had ever been handed before this change. + */ + @Test + void paginationOfAGroupedQueryKeepsTheVendorPagingBlock() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + + dao.loadContentsId(null, false, + new EntitySearchFilter[]{ordered(attributeLike()), new EntitySearchFilter(10, 5)}, ADMIN); + + String query = this.capture.single(); + assertTrue(SqlShape.isGrouped(query), query); + assertEquals("OFFSET 5 ROWS FETCH NEXT 10 ROWS ONLY", SqlShape.pagingBlock(query)); + } + + /** + * Scoping: a metadata order alongside the attribute filter cannot multiply a row, so the query + * keeps the DISTINCT shape it had - same plan, same total, same row order as before. + */ + @Test + void filteringOnAnAttributeButOrderingOnMetadata_doesNotGroup() { + WorkContentSearcherDAO dao = this.capture.wire(new WorkContentSearcherDAO(), DERBY); + + dao.loadContentsId(null, false, new EntitySearchFilter[]{attributeLike(), creationDateOrder()}, ADMIN); + + String query = this.capture.single(); + assertFalse(SqlShape.isGrouped(query), query); + assertTrue(SqlShape.isDistinct(query), query); + } + + /** + * The public searcher restricts its search to published contents, and both of its queries are built + * from that same filter set - the filter is applied in buildStatement, which the count and the list + * both pass through. + * + *

It used to be applied in loadContentsId alone, so the count included drafts the list would + * never return and the API reported pages that came back empty. Same class of defect as the join + * one, a level up: there the two queries disagreed on the body, here on the filters.

+ */ + @Test + void publicContentSearcher_appliesTheOnlineFilterToTheCountAsWell() { + PublicContentSearcherDAO dao = this.capture.wire(new PublicContentSearcherDAO(), DERBY); + EntitySearchFilter[] filters = {attributeLike()}; + + dao.countContents(null, false, filters, ADMIN); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.loadContentsId(null, false, filters, ADMIN); + String listQuery = this.capture.single(); + + assertTrue(SqlShape.normalize(listQuery).contains("contents.onlinexml IS NOT NULL"), listQuery); + assertTrue(SqlShape.normalize(countQuery).contains("contents.onlinexml IS NOT NULL"), countQuery); + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + assertEquals(List.of("contentsearch"), SqlShape.joinedTables(listQuery)); + // the filter carries no value, so it must not have introduced a placeholder + assertEquals(SqlShape.occurrences(listQuery, "?"), SqlShape.occurrences(countQuery, "?")); + } + + // ---------------------------------------------------------------- fixtures + + private static EntitySearchFilter attributeLike() { + return new EntitySearchFilter<>(ATTRIBUTE, true, "abc", true); + } + + private static EntitySearchFilter creationDateOrder() { + EntitySearchFilter filter = new EntitySearchFilter(IContentManager.CONTENT_CREATION_DATE_FILTER_KEY, false); + filter.setOrder(FieldSearchFilter.ASC_ORDER); + return filter; + } + + private static EntitySearchFilter ordered(EntitySearchFilter filter) { + filter.setOrder(FieldSearchFilter.ASC_ORDER); + return filter; + } + +} diff --git a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDaoQueryShapeTest.java b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDaoQueryShapeTest.java new file mode 100644 index 0000000000..3dbab31267 --- /dev/null +++ b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/ResourceDaoQueryShapeTest.java @@ -0,0 +1,116 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.plugins.jacms.aps.system.services.resource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.QueryCapture; +import com.agiletec.aps.system.common.SqlShape; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * The shape of the SQL {@link ResourceDAO} generates, asserted without a database. + * + *

A resource holds one resourcerelations row per category and the schema carries no + * uniqueness on the pair, so the joined query can return a resource more than once. The DAO is the + * one searcher outside the entity family whose body joins, and it has to de-duplicate on both + * sides: a distinct count beside a plain list is the original defect in miniature.

+ * + * @see QueryCapture + */ +class ResourceDaoQueryShapeTest { + + private static final String DERBY = "org.apache.derby.jdbc.EmbeddedDriver"; + private static final List TWO_CATEGORIES = List.of("cat1", "cat2"); + + private QueryCapture capture; + + @BeforeEach + void setUp() { + this.capture = new QueryCapture(); + } + + @Test + void resourceCount_isTheListBodyWrapped() { + ResourceDAO dao = this.capture.wire(new ResourceDAO(), DERBY); + FieldSearchFilter[] filters = {descriptionOrder()}; + + dao.countResources(filters, TWO_CATEGORIES, null); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.searchResourcesId(filters, TWO_CATEGORIES); + String listQuery = this.capture.single(); + + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + assertEquals(1, SqlShape.occurrences(countQuery, SqlShape.COUNT_PREFIX)); + assertEquals(1, SqlShape.occurrences(countQuery, SqlShape.COUNT_SUFFIX)); + } + + @Test + void bothSidesAreDistinctAndJoinOncePerCategory() { + ResourceDAO dao = this.capture.wire(new ResourceDAO(), DERBY); + FieldSearchFilter[] filters = {descriptionOrder()}; + + dao.countResources(filters, TWO_CATEGORIES, null); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.searchResourcesId(filters, TWO_CATEGORIES); + String listQuery = this.capture.single(); + + assertTrue(SqlShape.isDistinct(countQuery), countQuery); + assertTrue(SqlShape.isDistinct(listQuery), listQuery); + assertEquals(List.of("resourcerelations", "resourcerelations"), SqlShape.joinedTables(countQuery)); + assertEquals(SqlShape.joinedTables(countQuery), SqlShape.joinedTables(listQuery)); + } + + /** + * Derby and PostgreSQL reject an ORDER BY on a column outside the select list under DISTINCT, so + * the ordered column is projected - and only that one, or the extra column would make the + * resource distinct again, row by row. + */ + @Test + void distinctListQuery_projectsTheOrderedColumnAndNothingElse() { + ResourceDAO dao = this.capture.wire(new ResourceDAO(), DERBY); + + dao.searchResourcesId(new FieldSearchFilter[]{descriptionOrder()}, TWO_CATEGORIES); + + String query = this.capture.single(); + assertEquals(List.of("resources.resid", "resources.descr"), SqlShape.selectedColumns(query)); + assertEquals(List.of("resources.descr", "resources.resid"), SqlShape.orderedColumns(query)); + } + + @Test + void withoutCategories_theBodyDoesNotJoin() { + ResourceDAO dao = this.capture.wire(new ResourceDAO(), DERBY); + + dao.countResources(new FieldSearchFilter[]{descriptionOrder()}, null, null); + + String query = this.capture.single(); + assertEquals(List.of(), SqlShape.joinedTables(query)); + assertEquals(List.of("resources.resid", "resources.descr"), SqlShape.selectedColumns(query)); + } + + // ---------------------------------------------------------------- fixtures + + private static FieldSearchFilter descriptionOrder() { + FieldSearchFilter filter = new FieldSearchFilter("descr"); + filter.setOrder(FieldSearchFilter.ASC_ORDER); + return filter; + } + +} diff --git a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/TestResourceDAO.java b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/TestResourceDAO.java index be846d1623..6187d4b200 100644 --- a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/TestResourceDAO.java +++ b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/resource/TestResourceDAO.java @@ -45,6 +45,9 @@ void testAddDeleteResource() throws Throwable { resource.setMainGroup(Group.FREE_GROUP_NAME); resource.setType("Image"); resource.setFolder("/temp"); + // masterfilename is NOT NULL: AbstractResource defaults it to "", which Oracle stores as NULL, + // and the manager would never hand the DAO a resource without the uploaded file's name + resource.setMasterFileName("temp.jpg"); //resource.setBaseURL("temp"); ResourceRecordVO resourceRecordVO = null; try { @@ -55,6 +58,7 @@ void testAddDeleteResource() throws Throwable { _resourceDao.addResource(resource); resourceRecordVO = _resourceDao.loadResourceVo(resource.getId()); assertEquals(resourceRecordVO.getDescr().equals("temp"), true); + assertEquals("temp.jpg", resourceRecordVO.getMasterFileName()); _resourceDao.deleteResource(resource.getId(), null); resourceRecordVO = _resourceDao.loadResourceVo(resource.getId()); assertNull(resourceRecordVO); diff --git a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/apsadmin/resource/TestMultipleResourceAction.java b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/apsadmin/resource/TestMultipleResourceAction.java index 434739cdca..42b7678097 100644 --- a/cms-plugin/src/test/java/com/agiletec/plugins/jacms/apsadmin/resource/TestMultipleResourceAction.java +++ b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/apsadmin/resource/TestMultipleResourceAction.java @@ -242,6 +242,7 @@ void testDelete() throws Throwable { resource.setMainGroup(Group.FREE_GROUP_NAME); resource.setDescr("Levò la bocca dal fero pasto quel peccator"); resource.setCategories(new ArrayList()); + resource.setMasterFileName("levo_la_bocca.jpg"); this.resourceManager.addResource(resource); resourceId = resource.getId(); diff --git a/cms-plugin/src/test/java/org/entando/entando/plugins/jacms/web/content/ContentControllerIntegrationTest.java b/cms-plugin/src/test/java/org/entando/entando/plugins/jacms/web/content/ContentControllerIntegrationTest.java index e4400fdb98..5d202bba8e 100644 --- a/cms-plugin/src/test/java/org/entando/entando/plugins/jacms/web/content/ContentControllerIntegrationTest.java +++ b/cms-plugin/src/test/java/org/entando/entando/plugins/jacms/web/content/ContentControllerIntegrationTest.java @@ -3082,15 +3082,17 @@ void testLoadOrderedPublicEvents_8() throws Throwable { result.andDo(resultPrint()) .andExpect(status().isOk()) .andExpect(jsonPath("$.payload.size()", is(9))) - .andExpect(jsonPath("$.payload[0].id", is("EVN20"))) + // every row in this result has typecode EVN, so the sort key is fully tied and the + // order comes from the contentid tie-breaker + .andExpect(jsonPath("$.payload[0].id", is("EVN191"))) .andExpect(jsonPath("$.payload[1].id", is("EVN192"))) - .andExpect(jsonPath("$.payload[2].id", is("EVN23"))) - .andExpect(jsonPath("$.payload[3].id", is("EVN24"))) - .andExpect(jsonPath("$.payload[4].id", is("EVN21"))) - .andExpect(jsonPath("$.payload[5].id", is("EVN25"))) - .andExpect(jsonPath("$.payload[6].id", is("EVN191"))) - .andExpect(jsonPath("$.payload[7].id", is("EVN194"))) - .andExpect(jsonPath("$.payload[8].id", is("EVN193"))); + .andExpect(jsonPath("$.payload[2].id", is("EVN193"))) + .andExpect(jsonPath("$.payload[3].id", is("EVN194"))) + .andExpect(jsonPath("$.payload[4].id", is("EVN20"))) + .andExpect(jsonPath("$.payload[5].id", is("EVN21"))) + .andExpect(jsonPath("$.payload[6].id", is("EVN23"))) + .andExpect(jsonPath("$.payload[7].id", is("EVN24"))) + .andExpect(jsonPath("$.payload[8].id", is("EVN25"))); } @Test @@ -3110,15 +3112,17 @@ void testLoadOrderedPublicEvents_9() throws Throwable { result.andDo(resultPrint()) .andExpect(status().isOk()) .andExpect(jsonPath("$.payload.size()", is(9))) - .andExpect(jsonPath("$.payload[0].id", is("EVN193"))) - .andExpect(jsonPath("$.payload[1].id", is("EVN194"))) - .andExpect(jsonPath("$.payload[2].id", is("EVN191"))) - .andExpect(jsonPath("$.payload[3].id", is("EVN25"))) - .andExpect(jsonPath("$.payload[4].id", is("EVN21"))) - .andExpect(jsonPath("$.payload[5].id", is("EVN24"))) + // every row in this result has the same status, so the sort key is fully tied and the + // order comes from the contentid tie-breaker + .andExpect(jsonPath("$.payload[0].id", is("EVN191"))) + .andExpect(jsonPath("$.payload[1].id", is("EVN192"))) + .andExpect(jsonPath("$.payload[2].id", is("EVN193"))) + .andExpect(jsonPath("$.payload[3].id", is("EVN194"))) + .andExpect(jsonPath("$.payload[4].id", is("EVN20"))) + .andExpect(jsonPath("$.payload[5].id", is("EVN21"))) .andExpect(jsonPath("$.payload[6].id", is("EVN23"))) - .andExpect(jsonPath("$.payload[7].id", is("EVN192"))) - .andExpect(jsonPath("$.payload[8].id", is("EVN20"))); + .andExpect(jsonPath("$.payload[7].id", is("EVN24"))) + .andExpect(jsonPath("$.payload[8].id", is("EVN25"))); } @Test @@ -4357,6 +4361,55 @@ void testContentWithReferenceBatch() throws Exception { } } + /** + * The reported total must describe the rows the endpoint can actually return. + * + *

This guards the count/list pairing at the API layer: ContentService pairs + * countContents with loadContentsId through + * getPaginatedPublicContentsId, and if the two ever describe different row sets the + * client is handed pages that come back empty.

+ * + *

It does not reproduce the historical defect, and was measured not to: when + * PublicContentSearcherDAO applied the online filter to its list alone, this endpoint + * still reported 24 for 24. The reason is that a published search resolves a narrower group set than + * a draft one (ContentService.getAllowedGroups(user, true)), and in the standard fixture + * that narrower corpus happens to be fully published. The defect is demonstrated one layer down, in + * ContentSearchJoinCountRegressionTest, where the manager reported 25 for a list of 24.

+ * + *

Asserted against the payload rather than a literal, so the test survives fixture changes.

+ */ + @Test + void testGetPublishedContents_totalItemsDescribesThePayload() throws Exception { + UserDetails user = new OAuth2TestUtils.UserBuilder("jack_bauer", "0x24").grantedToRoleAdmin().build(); + String accessToken = mockOAuthInterceptor(user); + + String published = mockMvc + .perform(get("/plugins/cms/contents") + .param("status", "published") + .param("pageSize", "100") + .header("Authorization", "Bearer " + accessToken)) + .andExpect(status().isOk()) + .andReturn().getResponse().getContentAsString(); + String draft = mockMvc + .perform(get("/plugins/cms/contents") + .param("pageSize", "100") + .header("Authorization", "Bearer " + accessToken)) + .andExpect(status().isOk()) + .andReturn().getResponse().getContentAsString(); + + int publishedTotal = JsonPath.read(published, "$.metaData.totalItems"); + int publishedReturned = ((List) JsonPath.read(published, "$.payload")).size(); + int draftTotal = JsonPath.read(draft, "$.metaData.totalItems"); + + Assertions.assertEquals(publishedReturned, publishedTotal, + "totalItems must describe the payload the endpoint returns for a published search"); + // the two searches resolve different group sets, so this is a sanity check on the fixture + // being mixed at all - not evidence that the published corpus contains a draft + Assertions.assertTrue(draftTotal >= publishedTotal, + "a draft search must never return fewer contents than the published one: " + + draftTotal + " vs " + publishedTotal); + } + @Test void testGetContentsWithLinkability() throws Exception { UserDetails user = new OAuth2TestUtils.UserBuilder("jack_bauer", "0x24").grantedToRoleAdmin().build(); diff --git a/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java b/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java index ef0177585e..31776d0e80 100644 --- a/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java +++ b/contentworkflow-plugin/src/main/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentSearcherDAO.java @@ -106,7 +106,7 @@ private PreparedStatement buildStatement(List workflowFilt String query = this.createQueryString(workflowFilters, filters, categories, orClauseCategoryFilter, groupsForSelect, isCount, selectAll); PreparedStatement stat = null; try { - stat = conn.prepareStatement(query); + stat = this.prepareStatement(conn, query); int index = 0; index = super.addAttributeFilterStatementBlock(filters, index, stat); index = this.addMetadataFieldFilterStatementBlock(filters, index, stat); @@ -171,13 +171,12 @@ private String createQueryString(List workflowFilters, query.append(" )) "); } query.append(") "); + boolean grouped = this.appendGroupByQueryBlock(filters, query, selectAll); if (!isCount) { - appendOrderQueryBlocks(filters, query, false); + this.appendOrderQueryBlocks(filters, query, false, grouped); this.appendLimitQueryBlock(filters, query); - } else { - this.closeMasterCountQueryBlock(query); } - return query.toString(); + return this.toQueryString(query, isCount); } } diff --git a/contentworkflow-plugin/src/test/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentWorkflowSearcherDaoQueryShapeTest.java b/contentworkflow-plugin/src/test/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentWorkflowSearcherDaoQueryShapeTest.java new file mode 100644 index 0000000000..95d03e3126 --- /dev/null +++ b/contentworkflow-plugin/src/test/java/com/agiletec/plugins/jpcontentworkflow/aps/system/services/content/ContentWorkflowSearcherDaoQueryShapeTest.java @@ -0,0 +1,80 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.plugins.jpcontentworkflow.aps.system.services.content; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.agiletec.aps.system.common.QueryCapture; +import com.agiletec.aps.system.common.SqlShape; +import com.agiletec.aps.system.common.entity.model.EntitySearchFilter; +import com.agiletec.aps.system.services.group.Group; +import com.agiletec.plugins.jpcontentworkflow.aps.system.services.workflow.model.WorkflowSearchFilter; +import java.util.Collection; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * The fifth and last createQueryString variant. It builds its own workflow-step block + * and closes through the same composition point as the other four, so the count it produces is the + * body its list pages over - workflow block included. + * + * @see QueryCapture + */ +class ContentWorkflowSearcherDaoQueryShapeTest { + + private static final String DERBY = "org.apache.derby.jdbc.EmbeddedDriver"; + private static final Collection ADMIN = List.of(Group.ADMINS_GROUP_NAME); + + private QueryCapture capture; + + @BeforeEach + void setUp() { + this.capture = new QueryCapture(); + } + + @Test + void workflowCount_isTheListBodyWrapped() { + ContentSearcherDAO dao = this.capture.wire(new ContentSearcherDAO(), DERBY); + List workflowFilters = List.of(workflowFilter()); + EntitySearchFilter[] filters = {attributeLike()}; + + dao.countContents(workflowFilters, null, false, filters, ADMIN); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.loadContentsId(workflowFilters, null, false, filters, ADMIN); + String listQuery = this.capture.single(); + + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + assertEquals(1, SqlShape.occurrences(countQuery, SqlShape.COUNT_PREFIX)); + assertEquals(1, SqlShape.occurrences(countQuery, SqlShape.COUNT_SUFFIX)); + assertTrue(SqlShape.isDistinct(countQuery), countQuery); + assertTrue(SqlShape.normalize(countQuery).contains("contents.status IN ("), countQuery); + assertEquals(List.of("workcontentsearch"), SqlShape.joinedTables(countQuery)); + assertEquals(List.of("contents.contentid"), SqlShape.selectedColumns(listQuery)); + } + + private static EntitySearchFilter attributeLike() { + return new EntitySearchFilter<>("Titolo", true, "abc", true); + } + + private static WorkflowSearchFilter workflowFilter() { + WorkflowSearchFilter filter = new WorkflowSearchFilter(); + filter.setTypeCode("EVN"); + filter.addAllowedStep("step1"); + return filter; + } + +} diff --git a/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java b/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java index ec25051ac7..a187e71919 100644 --- a/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/common/AbstractSearcherDAO.java @@ -23,10 +23,13 @@ import java.util.ArrayList; import java.util.Calendar; import java.util.Date; +import java.util.HashSet; import java.util.List; +import java.util.Set; import com.agiletec.aps.util.ApsTenantApplicationUtils; import org.apache.commons.lang3.ArrayUtils; +import org.apache.commons.lang3.StringUtils; import org.entando.entando.ent.util.EntLogging.EntLogger; import org.entando.entando.ent.util.EntLogging.EntLogFactory; @@ -43,6 +46,15 @@ public abstract class AbstractSearcherDAO extends AbstractDAO { private static final EntLogger logger = EntLogFactory.getSanitizedLogger(AbstractSearcherDAO.class); private static final String DEFAULT_LIKE_CLAUSE = "LIKE ? "; + /** + * The two halves wrapping the body of a count query, so that the count always matches the number of + * rows the corresponding list query can return. Applied together by {@link #toQueryString}, which is + * the only place allowed to use them: a count block that cannot be opened on its own cannot be left + * open either. + */ + protected static final String COUNT_QUERY_PREFIX = "SELECT COUNT(*) FROM ( "; + protected static final String COUNT_QUERY_SUFFIX = ") counter"; + private String likeClause; private String dataSourceClassName; @@ -97,10 +109,9 @@ protected FieldSearchFilter[] addFilter(FieldSearchFilter[] filters, FieldSearch protected PreparedStatement buildStatement(FieldSearchFilter[] filters, boolean isCount, boolean selectAll, Connection conn) { String query = this.createQueryString(filters, isCount, selectAll); - logger.trace("{}", query); PreparedStatement stat = null; try { - stat = conn.prepareStatement(query); + stat = this.prepareStatement(conn, query); int index = 0; index = this.addMetadataFieldFilterStatementBlock(filters, index, stat); } catch (Throwable t) { @@ -110,6 +121,37 @@ protected PreparedStatement buildStatement(FieldSearchFilter[] filters, boolean return stat; } + /** + * The single point where a searcher hands a query to the driver. Every buildStatement + * goes through it, so the balance check below runs without anyone having to remember it. + * + * @param conn The connection. + * @param query The query to prepare. + * @return The prepared statement. + * @throws SQLException In case of error. + */ + protected final PreparedStatement prepareStatement(Connection conn, String query) throws SQLException { + logger.trace("{}", query); + this.checkCountQueryBlockBalance(query); + return conn.prepareStatement(query); + } + + /** + * Report a query whose count block is left open, or closed twice. {@link #toQueryString} cannot build + * one, but a subclass writing the markers by hand can, and the database would reject it with an opaque + * syntax error naming no source. The query is left untouched: this names the DAO that built it. + * + * @param query The query about to be prepared. + */ + private void checkCountQueryBlockBalance(String query) { + int opened = StringUtils.countMatches(query, COUNT_QUERY_PREFIX); + int closed = StringUtils.countMatches(query, COUNT_QUERY_SUFFIX); + if (opened != closed) { + logger.error("Unbalanced count query block built by {}: {} opening and {} closing markers - query: {}", + this.getClass().getName(), opened, closed, query); + } + } + /** * Add to the statement the filters on the entity metadata. * @@ -227,14 +269,28 @@ protected void addObjectSearchStatementBlock(PreparedStatement stat, protected String createQueryString(FieldSearchFilter[] filters, boolean isCount, boolean selectAll) { StringBuffer query = this.createBaseQueryBlock(filters, isCount, selectAll); - boolean hasAppendWhereClause = this.appendMetadataFieldFilterQueryBlocks(filters, query, false); + this.appendMetadataFieldFilterQueryBlocks(filters, query, false); if (!isCount) { - boolean ordered = appendOrderQueryBlocks(filters, query, false); + this.appendOrderQueryBlocks(filters, query, false); this.appendLimitQueryBlock(filters, query); - } else { - this.closeMasterCountQueryBlock(query); } - return query.toString(); + return this.toQueryString(query, isCount); + } + + /** + * Close a query built by any of the createQueryString variants. A count wraps the body + * the matching list query pages over - both halves of the wrapper are applied here, in one + * expression, so the two can neither disagree nor be left unbalanced. + * + * @param query The body of the query: select block, joins and where clauses, without order or limit. + * @param isCount True when the query counts the rows of that body. + * @return The query to prepare. + */ + protected final String toQueryString(StringBuffer query, boolean isCount) { + if (!isCount) { + return query.toString(); + } + return new StringBuffer(COUNT_QUERY_PREFIX).append(query).append(COUNT_QUERY_SUFFIX).toString(); } protected StringBuffer createBaseQueryBlock(FieldSearchFilter[] filters, boolean isCount, boolean selectAll) { @@ -247,19 +303,24 @@ protected StringBuffer createBaseQueryBlock(FieldSearchFilter[] filters, boolean return query; } + /** + * The body counted by the count query of a searcher that queries the master table alone. No join can + * multiply a row here, so it is not distinct - a derived table without DISTINCT, aggregate or LIMIT is + * merged by the planner, leaving a plain count. Subclasses that do join must de-duplicate instead, and + * must do it on their list query too, or the two stop agreeing. Returns the body alone: + * {@link #toQueryString} wraps it. + * + * @return The body of the count query. + */ protected StringBuffer createMasterCountQueryBlock() { String masterTableName = this.getMasterTableName(); - StringBuffer query = new StringBuffer("SELECT COUNT(*) FROM ( SELECT DISTINCT "); + StringBuffer query = new StringBuffer("SELECT "); query.append(masterTableName).append(".").append(this.getMasterTableIdFieldName()); query.append(" FROM ").append(masterTableName).append(" "); return query; } - protected void closeMasterCountQueryBlock(StringBuffer query) { - query.append(") counter"); - } - - private StringBuffer createMasterSelectQueryBlock(FieldSearchFilter[] filters, boolean selectAll) { + protected StringBuffer createMasterSelectQueryBlock(FieldSearchFilter[] filters, boolean selectAll) { String masterTableName = this.getMasterTableName(); StringBuffer query = new StringBuffer("SELECT ").append(masterTableName).append("."); if (selectAll) { @@ -368,6 +429,8 @@ protected boolean appendOrderQueryBlocks(FieldSearchFilter[] filters, StringBuff if (filters == null) { return ordered; } + Set orderedFields = new HashSet<>(); + Object lastOrder = null; for (FieldSearchFilter filter : filters) { if (null != filter.getKey() && null != filter.getOrder() && !filter.isNullOption()) { if (!ordered) { @@ -378,11 +441,66 @@ protected boolean appendOrderQueryBlocks(FieldSearchFilter[] filters, StringBuff } String fieldName = this.getTableFieldName(filter.getKey()); query.append(this.getMasterTableName()).append(".").append(fieldName).append(" ").append(filter.getOrder()); + orderedFields.add(fieldName); + lastOrder = filter.getOrder(); } } + this.appendOrderTieBreaker(query, ordered, orderedFields, this.getMasterTableIdFieldName(), lastOrder); return ordered; } + /** + * Break ties on the master id so the ordering is total. A sort whose key repeats leaves the order of + * the tied rows to the database, and LIMIT/OFFSET then slices an order that can differ between the + * request for one page and the request for the next - so a row can be served twice, or never. + * + *

The tie-breaker follows the direction of the sort it breaks. When the requested key is the same + * for every row it becomes the only visible ordering, and a DESC request has to keep reading as + * descending.

+ * + * @param query The query under construction. + * @param ordered Whether an ORDER BY block was opened. + * @param orderedFields The master-table fields already ordered on. + * @param idFieldName The master id field. + * @param lastOrder The direction of the last order term, or null to default to ascending. + */ + protected void appendOrderTieBreaker(StringBuffer query, boolean ordered, Set orderedFields, + String idFieldName, Object lastOrder) { + if (!ordered || orderedFields.contains(idFieldName)) { + return; + } + String direction = (null == lastOrder) ? FieldSearchFilter.ASC_ORDER : lastOrder.toString(); + query.append(", ").append(this.getMasterTableName()).append(".").append(idFieldName) + .append(" ").append(direction); + } + + /** + * Project the columns the ORDER BY block will reference. Only needed by a subclass whose select block + * is distinct: Derby and PostgreSQL reject an ORDER BY on a column outside the select list under + * DISTINCT. Mirrors the filter predicate of {@link #appendOrderQueryBlocks} and must keep mirroring it. + * + * @param filters The filters of the query. + * @param query The query under construction. + */ + protected void appendOrderFieldsSelectBlock(FieldSearchFilter[] filters, StringBuffer query) { + if (null == filters) { + return; + } + Set projected = new HashSet<>(); + projected.add(this.getMasterTableIdFieldName()); + for (FieldSearchFilter filter : filters) { + if (null == filter.getKey() || null == filter.getOrder() || filter.isNullOption()) { + continue; + } + String fieldName = this.getTableFieldName(filter.getKey()); + // two filters can order on the same column; a count wraps this block in a derived table, and + // a derived table may not repeat a column name + if (projected.add(fieldName)) { + query.append(", ").append(this.getMasterTableName()).append(".").append(fieldName); + } + } + } + protected boolean verifyWhereClauseAppend(StringBuffer query, boolean hasAppendWhereClause) { if (hasAppendWhereClause) { query.append("AND "); diff --git a/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java b/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java index c698f299aa..5380a7cf60 100644 --- a/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/common/entity/AbstractEntitySearcherDAO.java @@ -20,9 +20,12 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Date; +import java.util.HashSet; import java.util.List; +import java.util.Set; import com.agiletec.aps.system.common.AbstractSearcherDAO; +import com.agiletec.aps.system.common.FieldSearchFilter; import com.agiletec.aps.system.common.entity.model.ApsEntityRecord; import com.agiletec.aps.system.common.entity.model.EntitySearchFilter; import org.entando.entando.ent.util.EntLogging.EntLogger; @@ -138,7 +141,7 @@ private PreparedStatement buildStatement(EntitySearchFilter[] filters, boolean i String query = this.createQueryString(filters, isCount, selectAll); PreparedStatement stat = null; try { - stat = conn.prepareStatement(query); + stat = this.prepareStatement(conn, query); int index = 0; index = this.addAttributeFilterStatementBlock(filters, index, stat); index = this.addMetadataFieldFilterStatementBlock(filters, index, stat); @@ -216,13 +219,12 @@ protected String createQueryString(EntitySearchFilter[] filters, boolean isCount StringBuffer query = this.createBaseQueryBlock(filters, isCount, selectAll); boolean hasAppendWhereClause = this.appendFullAttributeFilterQueryBlocks(filters, query, false); this.appendMetadataFieldFilterQueryBlocks(filters, query, hasAppendWhereClause); + boolean grouped = this.appendGroupByQueryBlock(filters, query, selectAll); if (!isCount) { - boolean ordered = this.appendOrderQueryBlocks(filters, query, false); + this.appendOrderQueryBlocks(filters, query, false, grouped); this.appendLimitQueryBlock(filters, query); - } else { - this.closeMasterCountQueryBlock(query); } - return query.toString(); + return this.toQueryString(query, isCount); } /** @@ -238,7 +240,10 @@ protected String createQueryString(EntitySearchFilter[] filters, boolean isCount protected StringBuffer createBaseQueryBlock(EntitySearchFilter[] filters, boolean isCount, boolean selectAll) { StringBuffer query = null; if (isCount) { - query = this.createMasterCountQueryBlock(); + // count the rows of the very select block the list query pages over, so that the two can + // never disagree: an attribute filter joins the search table and can match several rows + // per entity, and LIMIT/OFFSET is applied to whatever that block returns + query = this.createMasterSelectQueryBlock(filters, false); } else { query = this.createMasterSelectQueryBlock(filters, selectAll); } @@ -248,33 +253,179 @@ protected StringBuffer createBaseQueryBlock(EntitySearchFilter[] filters, boolea protected StringBuffer createMasterSelectQueryBlock(EntitySearchFilter[] filters, boolean selectAll) { String masterTableName = this.getEntityMasterTableName(); - StringBuffer query = new StringBuffer("SELECT ").append(masterTableName).append("."); + boolean grouped = this.isGroupedByMasterId(filters, selectAll); + StringBuffer query = new StringBuffer("SELECT "); + if (!selectAll && !grouped) { + // GROUP BY on the master id already returns one row per entity + query.append("DISTINCT "); + } + query.append(masterTableName).append("."); if (selectAll) { query.append("* "); } else { query.append(this.getEntityMasterTableIdFieldName()); } if (filters != null) { - String searchTableName = this.getEntitySearchTableName(); - for (int i = 0; i < filters.length; i++) { - EntitySearchFilter filter = filters[i]; - if (!filter.isAttributeFilter() && filter.isLikeOption()) { - String tableFieldName = this.getTableFieldName(filter.getKey()); - //check for id column already present - if (!tableFieldName.equals(this.getMasterTableIdFieldName())) { - query.append(", ").append(masterTableName).append(".").append(tableFieldName); - } - } else if (filter.isAttributeFilter() && filter.isLikeOption()) { - String columnName = this.getAttributeFieldColunm(filter); - query.append(", ").append(searchTableName).append(i).append(".").append(columnName); - query.append(" AS ").append(columnName).append(i).append(" "); - } + if (selectAll) { + this.appendLikeFieldsSelectBlock(filters, query); + } else { + this.appendOrderFieldsSelectBlock(filters, query, grouped); } } query.append(" FROM ").append(masterTableName).append(" "); return query; } + /** + * Whether the body collapses the entity in SQL rather than with DISTINCT. + * + *

DISTINCT cannot collapse an entity that is ordered by an attribute: the ORDER BY + * names a column of the joined search table, that column has to be projected, and an entity + * holding one value per language then produces rows that are genuinely distinct. Grouping on the + * master id collapses it, and the attribute is reached through an aggregate instead.

+ * + *

Deliberately scoped to that case. Ordering on metadata alone cannot multiply a row, so those + * searches - the large majority - keep the plan, the totals and the row order they have.

+ * + * @param filters The filters of the query. + * @param selectAll True when the query loads whole records; that path has no count paired with it + * and projects the master table's CLOB columns, so it is never grouped. + * @return True when the body must group by the master id. + */ + protected boolean isGroupedByMasterId(EntitySearchFilter[] filters, boolean selectAll) { + if (selectAll || null == filters) { + return false; + } + for (EntitySearchFilter filter : filters) { + if (this.isOrderFilter(filter) && filter.isAttributeFilter()) { + return true; + } + } + return false; + } + + /** + * The filters the ORDER BY block will emit a term for. Every method that has to stay aligned with + * that block - the projection, the GROUP BY - asks this rather than repeating the condition. + * + * @param filter The filter to test. + * @return True when the filter carries an order the query builder honours. + */ + private boolean isOrderFilter(EntitySearchFilter filter) { + return (null != filter.getKey() || null != filter.getRoleName()) + && null != filter.getOrder() && !filter.isNullOption(); + } + + /** + * The master-table columns the ORDER BY block references, de-duplicated and in the order the + * filters declare them. They are projected, and when the body groups they are also grouped on: + * Derby and Oracle both reject an un-aggregated column that is not in the GROUP BY, even one + * functionally dependent on the grouping key that MySQL and PostgreSQL accept. + * + * @param filters The filters of the query. + * @return The column names, without the table prefix. + */ + private List metadataOrderColumns(EntitySearchFilter[] filters) { + List columns = new ArrayList<>(); + if (null == filters) { + return columns; + } + for (EntitySearchFilter filter : filters) { + if (!this.isOrderFilter(filter) || filter.isAttributeFilter()) { + continue; + } + String fieldName = this.getTableFieldName(filter.getKey()); + // two filters can order on the same column; a count wraps this block in a derived table, + // and a derived table may not repeat a column name + if (!columns.contains(fieldName) && !fieldName.equals(this.getEntityMasterTableIdFieldName())) { + columns.add(fieldName); + } + } + return columns; + } + + /** + * Group the body on the master id so that an entity holding several values for the ordered + * attribute collapses to one row. Part of the body, not of the order block: the count wraps the + * same block, so it counts groups and its total becomes exact. + * + * @param filters The filters of the query. + * @param query The query under construction. + * @param selectAll True when the query loads whole records. + * @return True when the clause was appended, to be handed to + * {@link #appendOrderQueryBlocks(EntitySearchFilter[], StringBuffer, boolean, boolean)} - the two + * cannot disagree because one produces what the other consumes. + */ + protected boolean appendGroupByQueryBlock(EntitySearchFilter[] filters, StringBuffer query, boolean selectAll) { + if (!this.isGroupedByMasterId(filters, selectAll)) { + return false; + } + String masterTableName = this.getEntityMasterTableName(); + query.append("GROUP BY ").append(masterTableName).append(".") + .append(this.getEntityMasterTableIdFieldName()); + for (String column : this.metadataOrderColumns(filters)) { + query.append(", ").append(masterTableName).append(".").append(column); + } + query.append(" "); + return true; + } + + private void appendLikeFieldsSelectBlock(EntitySearchFilter[] filters, StringBuffer query) { + String masterTableName = this.getEntityMasterTableName(); + String searchTableName = this.getEntitySearchTableName(); + for (int i = 0; i < filters.length; i++) { + EntitySearchFilter filter = filters[i]; + if (!filter.isAttributeFilter() && filter.isLikeOption()) { + String tableFieldName = this.getTableFieldName(filter.getKey()); + //check for id column already present + if (!tableFieldName.equals(this.getMasterTableIdFieldName())) { + query.append(", ").append(masterTableName).append(".").append(tableFieldName); + } + } else if (filter.isAttributeFilter() && filter.isLikeOption()) { + String columnName = this.getAttributeFieldColunm(filter); + query.append(", ").append(searchTableName).append(i).append(".").append(columnName); + query.append(" AS ").append(columnName).append(i).append(" "); + } + } + } + + /** + * Project the columns the ORDER BY block will reference. Under DISTINCT they have to appear in the + * select list, and they are the only extra columns allowed to: any other column of the joined + * search table would make the entity distinct again, row by row. + * + *

When the body groups, the attribute column is deliberately not projected. Projecting + * it is what stops DISTINCT collapsing the entity, and the ORDER BY reaches it through an + * aggregate instead, which needs no projection.

+ * + * @param filters The filters of the query. + * @param query The query under construction. + * @param grouped True when the body groups by the master id. + */ + private void appendOrderFieldsSelectBlock(EntitySearchFilter[] filters, StringBuffer query, boolean grouped) { + for (String column : this.metadataOrderColumns(filters)) { + query.append(", ").append(this.getEntityMasterTableName()).append(".").append(column); + } + if (grouped) { + return; + } + for (int i = 0; i < filters.length; i++) { + EntitySearchFilter filter = filters[i]; + if (!this.isOrderFilter(filter) || !filter.isAttributeFilter()) { + continue; + } + String searchTableNameAlias = this.getEntitySearchTableName() + i; + String columnName = this.getAttributeFieldColunm(this.getOrderReferenceValue(filter)); + if (null == columnName) { + query.append(", ").append(searchTableNameAlias).append(".textvalue"); + query.append(", ").append(searchTableNameAlias).append(".datevalue"); + query.append(", ").append(searchTableNameAlias).append(".numvalue"); + } else { + query.append(", ").append(searchTableNameAlias).append(".").append(columnName); + } + } + } + protected void appendJoinSearchTableQueryBlock(EntitySearchFilter[] filters, StringBuffer query) { if (filters == null) { return; @@ -415,13 +566,33 @@ protected boolean appendMetadataFieldFilterQueryBlocks(EntitySearchFilter[] filt return hasAppendWhereClause; } + /** + * Order a body that does not group. Kept for callers that build an un-grouped query; a caller that + * appended a GROUP BY must use the four-argument form, or the attribute term would name a column + * that is neither grouped nor aggregated. + */ protected boolean appendOrderQueryBlocks(EntitySearchFilter[] filters, StringBuffer query, boolean ordered) { + return this.appendOrderQueryBlocks(filters, query, ordered, false); + } + + /** + * @param filters The filters of the query. + * @param query The query under construction. + * @param ordered Whether an ORDER BY block was already opened. + * @param grouped True when the body groups by the master id, as reported by + * {@link #appendGroupByQueryBlock}. + * @return Whether an ORDER BY block is open. + */ + protected boolean appendOrderQueryBlocks(EntitySearchFilter[] filters, StringBuffer query, boolean ordered, + boolean grouped) { if (filters == null) { return ordered; } + Set orderedFields = new HashSet<>(); + Object lastOrder = null; for (int i = 0; i < filters.length; i++) { EntitySearchFilter filter = filters[i]; - if ((null != filter.getKey() || null != filter.getRoleName()) && null != filter.getOrder() && !filter.isNullOption()) { + if (this.isOrderFilter(filter)) { if (!ordered) { query.append("ORDER BY "); ordered = true; @@ -430,13 +601,16 @@ protected boolean appendOrderQueryBlocks(EntitySearchFilter[] filters, StringBuf } if (filter.isAttributeFilter()) { String tableName = this.getEntitySearchTableName() + i; - this.addAttributeOrderQueryBlock(tableName, query, filter, filter.getOrder().toString()); + this.addAttributeOrderQueryBlock(tableName, query, filter, filter.getOrder().toString(), grouped); } else { String fieldName = this.getTableFieldName(filter.getKey()); query.append(this.getEntityMasterTableName()).append(".").append(fieldName).append(" ").append(filter.getOrder()); + orderedFields.add(fieldName); } + lastOrder = filter.getOrder(); } } + this.appendOrderTieBreaker(query, ordered, orderedFields, this.getEntityMasterTableIdFieldName(), lastOrder); return ordered; } @@ -472,10 +646,49 @@ protected boolean verifyWhereClauseAppend(StringBuffer query, boolean hasAppendW return hasAppendWhereClause; } - private void addAttributeOrderQueryBlock(String searchTableNameAlias, StringBuffer query, EntitySearchFilter filter, String order) { + private void addAttributeOrderQueryBlock(String searchTableNameAlias, StringBuffer query, + EntitySearchFilter filter, String order, boolean grouped) { if (order == null) { order = ""; } + Object object = this.getOrderReferenceValue(filter); + if (null == object) { + query.append(this.orderTerm(searchTableNameAlias, "textvalue", order, grouped)).append(", ") + .append(this.orderTerm(searchTableNameAlias, "datevalue", order, grouped)).append(", ") + .append(this.orderTerm(searchTableNameAlias, "numvalue", order, grouped)); + return; + } + query.append(this.orderTerm(searchTableNameAlias, this.getAttributeFieldColunm(object), order, grouped)); + } + + /** + * One ORDER BY term over an attribute column. When the body groups, the column belongs to the + * joined search table and is not part of the grouping key, so it is reached through an aggregate: + * an entity holding several values sorts on the one the requested direction asks for - the lowest + * ascending, the highest descending. + * + * @param searchTableNameAlias The alias of the joined search table. + * @param columnName The column holding the attribute value. + * @param order The requested direction. + * @param grouped True when the body groups by the master id. + * @return The term, direction included. + */ + private String orderTerm(String searchTableNameAlias, String columnName, String order, boolean grouped) { + StringBuilder term = new StringBuilder(); + if (grouped) { + String aggregate = FieldSearchFilter.DESC_ORDER.equalsIgnoreCase(order) ? "MAX" : "MIN"; + term.append(aggregate).append("(").append(searchTableNameAlias).append(".").append(columnName).append(")"); + } else { + term.append(searchTableNameAlias).append(".").append(columnName); + } + return term.append(" ").append(order).toString(); + } + + /** + * The value an ORDER BY on an attribute filter is resolved against. Shared with the select block so + * that the projected column and the ordered column are always the same one. + */ + private Object getOrderReferenceValue(EntitySearchFilter filter) { Object object = filter.getValue(); if (object == null) { object = filter.getStart(); @@ -483,14 +696,7 @@ private void addAttributeOrderQueryBlock(String searchTableNameAlias, StringBuff if (object == null) { object = filter.getEnd(); } - if (null == object) { - query.append(searchTableNameAlias).append(".textvalue ").append(order).append(", ") - .append(searchTableNameAlias).append(".datevalue ").append(order).append(", ") - .append(searchTableNameAlias).append(".numvalue ").append(order); - return; - } - query.append(searchTableNameAlias).append(".").append(this.getAttributeFieldColunm(object)).append(" "); - query.append(order); + return object; } private String getAttributeFieldColunm(EntitySearchFilter filter) { diff --git a/engine/src/main/java/org/entando/entando/aps/system/services/actionlog/ActionLogDAO.java b/engine/src/main/java/org/entando/entando/aps/system/services/actionlog/ActionLogDAO.java index 637a21b393..e87487ccab 100644 --- a/engine/src/main/java/org/entando/entando/aps/system/services/actionlog/ActionLogDAO.java +++ b/engine/src/main/java/org/entando/entando/aps/system/services/actionlog/ActionLogDAO.java @@ -241,7 +241,7 @@ private PreparedStatement buildStatement(FieldSearchFilter[] filters, Collection String query = (isSelectMax) ? this.createQueryStringForSelectMax(filters, groupCodes): this.createQueryString(filters, groupCodes); PreparedStatement stat = null; try { - stat = conn.prepareStatement(query); + stat = this.prepareStatement(conn, query); int index = 0; index = this.addMetadataFieldFilterStatementBlock(filters, index, stat); index = this.addGroupStatementBlock(groupCodes, index, stat); diff --git a/engine/src/test/java/com/agiletec/ConfigTestUtils.java b/engine/src/test/java/com/agiletec/ConfigTestUtils.java index c5584df7f6..b83cfab250 100644 --- a/engine/src/test/java/com/agiletec/ConfigTestUtils.java +++ b/engine/src/test/java/com/agiletec/ConfigTestUtils.java @@ -164,6 +164,7 @@ private void createDatasource(String dsNameControlKey, InitialContext builder, P ds.setMaxTotal(12); ds.setMaxIdle(4); ds.setDriverClassName(className); + this.applySessionInitSql(ds, className); bindOrRebind(builder, "java:comp/env/jdbc/" + beanName, ds); logger.debug("created datasource " + beanName); } catch (Throwable t) { @@ -171,6 +172,23 @@ private void createDatasource(String dsNameControlKey, InitialContext builder, P } } + /** + * Oracle parses a date literal against the session NLS_DATE_FORMAT, which defaults to DD-MON-RR, while + * the Liquibase fixtures render dates as ISO literals. Without this every suite fails at fixture load + * with ORA-01843. The statements run on each physical connection the pool opens, which is the scope + * the fixtures and the DAOs both need. + * + * @param ds The datasource being built. + * @param driverClassName The driver of that datasource. + */ + private void applySessionInitSql(BasicDataSource ds, String driverClassName) { + if (null != driverClassName && driverClassName.toLowerCase().contains("oracle")) { + ds.setConnectionInitSqls(Arrays.asList( + "ALTER SESSION SET NLS_DATE_FORMAT='YYYY-MM-DD HH24:MI:SS'", + "ALTER SESSION SET NLS_TIMESTAMP_FORMAT='YYYY-MM-DD HH24:MI:SS.FF'")); + } + } + /** * Restituisce l'insieme dei file di configurazione dei bean definiti nel * sistema. Il metodo và esteso nel caso si inseriscano file di diff --git a/engine/src/test/java/com/agiletec/aps/system/common/QueryCapture.java b/engine/src/test/java/com/agiletec/aps/system/common/QueryCapture.java new file mode 100644 index 0000000000..dd3845dafa --- /dev/null +++ b/engine/src/test/java/com/agiletec/aps/system/common/QueryCapture.java @@ -0,0 +1,97 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.aps.system.common; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import javax.sql.DataSource; + +/** + * The SQL a searcher DAO hands to the driver, captured without a database. + * + *

Every searcher reaches the driver through {@link AbstractSearcherDAO#prepareStatement}, so + * calling a DAO's own search or count method against this datasource yields the generated query and + * nothing else: the statement records its parameters into a stub and the result set is always + * empty, which leaves counts at zero and id lists empty.

+ */ +public final class QueryCapture { + + private final List queries = new ArrayList<>(); + private final DataSource dataSource; + + public QueryCapture() { + try { + ResultSet emptyResult = mock(ResultSet.class); + when(emptyResult.next()).thenReturn(false); + PreparedStatement statement = mock(PreparedStatement.class); + when(statement.executeQuery()).thenReturn(emptyResult); + Connection connection = mock(Connection.class); + when(connection.prepareStatement(anyString())).thenAnswer(invocation -> { + this.queries.add(invocation.getArgument(0)); + return statement; + }); + this.dataSource = mock(DataSource.class); + when(this.dataSource.getConnection()).thenReturn(connection); + } catch (SQLException e) { + throw new IllegalStateException("the stubs above cannot throw", e); + } + } + + /** + * Wire a DAO to this capture. The driver class name decides the paging syntax, and there is no + * real datasource to read it from. + * + * @param dao The DAO under test. + * @param driverClassName The JDBC driver the DAO should generate for. + * @param The DAO type. + * @return The same DAO, wired. + */ + public T wire(T dao, String driverClassName) { + dao.setDataSource(this.dataSource); + dao.setDataSourceClassName(driverClassName); + return dao; + } + + public DataSource getDataSource() { + return this.dataSource; + } + + public List getQueries() { + return Collections.unmodifiableList(this.queries); + } + + /** + * @return The only query captured so far. + */ + public String single() { + if (this.queries.size() != 1) { + throw new IllegalStateException("expected exactly one query, captured " + this.queries); + } + return this.queries.get(0); + } + + public void clear() { + this.queries.clear(); + } + +} diff --git a/engine/src/test/java/com/agiletec/aps/system/common/QueryLimitResolverTest.java b/engine/src/test/java/com/agiletec/aps/system/common/QueryLimitResolverTest.java index 9d640357e4..e83807c289 100644 --- a/engine/src/test/java/com/agiletec/aps/system/common/QueryLimitResolverTest.java +++ b/engine/src/test/java/com/agiletec/aps/system/common/QueryLimitResolverTest.java @@ -1,6 +1,7 @@ package com.agiletec.aps.system.common; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import org.apache.commons.dbcp2.BasicDataSource; import org.junit.jupiter.api.Test; @@ -51,6 +52,23 @@ void testOracleDriver() throws Exception { " OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY "); } + /** + * A vendor the resolver does not know fails on the first paginated request, at runtime, with no + * compile-time signal. Pinned as current behaviour, not as desired behaviour. + */ + @Test + void testUnknownDriver() { + String driverClassName = "com.example.UnknownDriver"; + Mockito.when(dataSource.getDriverClassName()).thenReturn(driverClassName); + + FieldSearchFilter filter = new FieldSearchFilter(); + filter.setLimit(1); + filter.setOffset(0); + + assertThrows(UnsupportedOperationException.class, + () -> QueryLimitResolver.createLimitBlock(filter, dataSource, driverClassName)); + } + private void testCreateLimitBlock(String driverClassName, String expected) throws Exception { Mockito.when(dataSource.getDriverClassName()).thenReturn(driverClassName); diff --git a/engine/src/test/java/com/agiletec/aps/system/common/SearcherDaoQueryShapeTest.java b/engine/src/test/java/com/agiletec/aps/system/common/SearcherDaoQueryShapeTest.java new file mode 100644 index 0000000000..c529cbcb88 --- /dev/null +++ b/engine/src/test/java/com/agiletec/aps/system/common/SearcherDaoQueryShapeTest.java @@ -0,0 +1,411 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.aps.system.common; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import com.agiletec.aps.system.common.entity.IEntityManager; +import com.agiletec.aps.system.common.entity.model.EntitySearchFilter; +import com.agiletec.aps.system.services.group.GroupDAO; +import java.math.BigDecimal; +import java.util.Date; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.entando.entando.aps.system.services.actionlog.ActionLogDAO; +import org.entando.entando.aps.system.services.actionlog.model.ActionLogRecordSearchBean; +import org.entando.entando.aps.system.services.userprofile.UserProfileSearcherDAO; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.slf4j.LoggerFactory; + +/** + * The shape of the SQL the searcher DAOs generate, asserted without a database. + * + *

The count and the list are one body: the count query is the list query's body wrapped, so the + * two cannot report different row sets. Execution tests cannot see that - a wrong projection yields + * a valid query returning a wrong number - which is what these assertions are for.

+ * + * @see QueryCapture + */ +class SearcherDaoQueryShapeTest { + + private static final String DERBY = "org.apache.derby.jdbc.EmbeddedDriver"; + + private QueryCapture capture; + + @BeforeEach + void setUp() { + this.capture = new QueryCapture(); + } + + // ---------------------------------------------------------------- the wrapper + + @Test + void countQuery_opensAndClosesTheCountBlockExactlyOnce() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + + dao.countGroups(new FieldSearchFilter[]{descriptionLike()}); + + String query = this.capture.single(); + assertEquals(1, SqlShape.occurrences(query, SqlShape.COUNT_PREFIX)); + assertEquals(1, SqlShape.occurrences(query, SqlShape.COUNT_SUFFIX)); + } + + @Test + void listQuery_carriesNoCountBlock() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + + dao.searchGroups(new FieldSearchFilter[]{descriptionLike()}); + + String query = this.capture.single(); + assertEquals(0, SqlShape.occurrences(query, SqlShape.COUNT_PREFIX)); + assertEquals(0, SqlShape.occurrences(query, SqlShape.COUNT_SUFFIX)); + } + + // ------------------------------------------------- join-free searchers: no DISTINCT + + /** + * A searcher whose count queries the master table alone cannot multiply a row, so its count body + * is a plain select: a derived table with no DISTINCT, aggregate or LIMIT is merged by the + * planner, leaving a count that can be answered from an index. + */ + @Test + void joinFreeCount_selectsTheMasterIdWithoutDistinct() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + + dao.countGroups(new FieldSearchFilter[]{descriptionLike()}); + + assertEquals("SELECT COUNT(*) FROM ( SELECT authgroups.groupname FROM authgroups" + + " WHERE UPPER(authgroups.descr) LIKE ? ) counter", + SqlShape.normalize(this.capture.single())); + } + + @Test + void actionLogCount_selectsTheMasterIdWithoutDistinct() { + ActionLogDAO dao = this.capture.wire(new ActionLogDAO(), DERBY); + ActionLogRecordSearchBean searchBean = new ActionLogRecordSearchBean(); + searchBean.setUsername("admin"); + + dao.countActionLogRecords(searchBean); + + String query = this.capture.single(); + assertFalse(SqlShape.isDistinct(query), query); + assertEquals(List.of("actionlogrecords.id"), SqlShape.selectedColumns(query)); + assertEquals(List.of(), SqlShape.joinedTables(query)); + } + + /** + * The base list query is not distinct, so it is free to order on a column it does not project. + * The rule that every ordered column has to be in the select list belongs to the distinct + * searchers below. + */ + @Test + void joinFreeList_ordersOnAColumnItDoesNotProject() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + + dao.searchGroups(new FieldSearchFilter[]{ordered(descriptionLike(), FieldSearchFilter.ASC_ORDER)}); + + String query = this.capture.single(); + assertFalse(SqlShape.isDistinct(query), query); + assertEquals(List.of("authgroups.groupname"), SqlShape.selectedColumns(query)); + assertEquals(List.of("authgroups.descr", "authgroups.groupname"), SqlShape.orderedColumns(query)); + } + + // ----------------------------------------------- entity searchers: one body, distinct + + @Test + void entityCount_isTheListBodyWrapped() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + EntitySearchFilter[] filters = {attributeLike(), orderedMetadata()}; + + dao.count(filters); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.searchId(filters); + String listQuery = this.capture.single(); + + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + } + + @Test + void entitySelectBlock_isDistinct() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchId(new EntitySearchFilter[]{attributeLike()}); + + String query = this.capture.single(); + assertTrue(SqlShape.isDistinct(query), query); + assertEquals(List.of("authuserprofilesearch"), SqlShape.joinedTables(query)); + } + + /** + * A LIKE filter used to add the search table's value column to the select list, aliased and never + * read back. Under DISTINCT that column makes the entity distinct row by row, which is the defect + * this whole shape exists to prevent. + */ + @Test + void entityListQuery_dropsTheColumnsProjectedOnlyForALikeFilter() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchId(new EntitySearchFilter[]{attributeLike()}); + + String query = this.capture.single(); + assertEquals(List.of("authuserprofiles.username"), SqlShape.selectedColumns(query)); + assertFalse(SqlShape.normalize(query).contains("AS textvalue"), query); + } + + /** + * Every ordered column has to be reachable by the engine: a plain column reference must be in the + * select list - Derby and PostgreSQL reject an ORDER BY outside it under DISTINCT - while an + * aggregate must not be, because projecting it is exactly what would stop the entity collapsing. + * + *

Covers all five resolutions of the order block, including the two that resolve to three + * columns at once (an allowed-values filter and a filter carrying no value).

+ */ + @ParameterizedTest(name = "ordered by {0}") + @MethodSource("orderedFilters") + void listQuery_makesEveryOrderedColumnReachable(String description, EntitySearchFilter orderFilter) { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchId(new EntitySearchFilter[]{orderFilter}); + + String query = this.capture.single(); + List projected = SqlShape.selectedColumns(query); + List grouped = SqlShape.groupedColumns(query); + for (String term : SqlShape.orderedColumns(query)) { + if (SqlShape.isAggregate(term)) { + String column = SqlShape.aggregatedColumn(term); + assertFalse(projected.contains(column), + () -> "aggregated " + column + " but also projected it - " + query); + assertFalse(grouped.isEmpty(), () -> "aggregate without a GROUP BY - " + query); + } else { + assertTrue(projected.contains(term), + () -> "ordered by " + term + " but projected " + projected + " - " + query); + } + } + // a grouped body collapses the entity itself; DISTINCT on top would be redundant + assertEquals(SqlShape.isGrouped(query), !SqlShape.isDistinct(query), query); + } + + /** + * The grouping is scoped to the case that needs it. Ordering on metadata alone cannot multiply a + * row, so those searches keep the plan, the totals and the row order they had. + */ + @Test + void orderingOnMetadataAlone_doesNotGroup() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchId(new EntitySearchFilter[]{attributeLike(), orderedMetadata()}); + + String query = this.capture.single(); + assertFalse(SqlShape.isGrouped(query), query); + assertTrue(SqlShape.isDistinct(query), query); + } + + /** + * Ordering by an attribute groups instead, and the count wraps the grouped body - so it counts + * entities rather than joined rows, and its total is exact. + */ + @Test + void orderingByAnAttribute_groupsOnTheMasterIdOnBothSides() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + EntitySearchFilter[] filters = {ordered(attributeLike(), FieldSearchFilter.ASC_ORDER)}; + + dao.count(filters); + String countQuery = this.capture.single(); + this.capture.clear(); + dao.searchId(filters); + String listQuery = this.capture.single(); + + assertEquals(List.of("authuserprofiles.username"), SqlShape.groupedColumns(countQuery)); + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + assertFalse(SqlShape.isDistinct(listQuery), listQuery); + assertEquals(List.of("authuserprofiles.username"), SqlShape.selectedColumns(listQuery)); + } + + /** + * A metadata column ordered alongside the attribute is grouped on as well. Derby and Oracle both + * reject an un-aggregated column outside the GROUP BY, even one functionally dependent on the + * grouping key that MySQL and PostgreSQL accept - so the portable form is the explicit one. + */ + @Test + void aMetadataOrderAlongsideAnAttribute_joinsTheGroupingKey() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchId(new EntitySearchFilter[]{ordered(attributeLike(), FieldSearchFilter.ASC_ORDER), + orderedMetadata()}); + + String query = this.capture.single(); + assertEquals(List.of("authuserprofiles.username", "authuserprofiles.profiletype"), + SqlShape.groupedColumns(query)); + assertEquals(List.of("authuserprofiles.username", "authuserprofiles.profiletype"), + SqlShape.selectedColumns(query)); + } + + /** + * The select-all path loads whole records, has no count paired with it and projects the master + * table's CLOB columns. It is never grouped - an aggregate there would have to be matched by a + * grouping key for every projected column. + */ + @Test + void selectAllPath_isNeverGrouped() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchRecords(new EntitySearchFilter[]{ordered(attributeLike(), FieldSearchFilter.ASC_ORDER)}); + + String query = this.capture.single(); + assertFalse(SqlShape.isGrouped(query), query); + assertFalse(SqlShape.normalize(query).contains("MIN("), query); + } + + static Stream orderedFilters() { + return Stream.of( + Arguments.of("a metadata field", orderedMetadata()), + Arguments.of("an attribute carrying a value", + ordered(new EntitySearchFilter<>("Nome", true, "abc", false), FieldSearchFilter.ASC_ORDER)), + Arguments.of("an attribute carrying a range", + ordered(new EntitySearchFilter<>("Data", true, new Date(0), new Date()), FieldSearchFilter.ASC_ORDER)), + Arguments.of("an attribute carrying allowed values", + ordered(new EntitySearchFilter<>("Numero", true, + List.of(BigDecimal.ONE, BigDecimal.TEN), false), FieldSearchFilter.DESC_ORDER)), + Arguments.of("an attribute carrying no value at all", + ordered(new EntitySearchFilter("Nome", true), FieldSearchFilter.ASC_ORDER))); + } + + /** + * The select-all path loads whole records and has no count paired with it. It must keep its old + * shape: contents.workxml and authuserprofiles.profilexml are CLOB, and Derby rejects DISTINCT + * over a CLOB. + */ + @Test + void selectAllPath_isNotDistinct() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchRecords(new EntitySearchFilter[]{attributeLike()}); + + String query = this.capture.single(); + assertFalse(SqlShape.isDistinct(query), query); + assertTrue(SqlShape.normalize(query).startsWith("SELECT authuserprofiles.*"), query); + } + + // ---------------------------------------------------------------- the balance guard + + /** + * The only remaining route to an unbalanced count block is a subclass writing the markers by + * hand. The guard names the DAO that built the query; it does not repair it, and it does not + * throw - the database rejects such a query on its own, and hiding that would be worse. + */ + @Test + void unbalancedCountBlock_isReportedByNameAndNotRepaired() { + UnbalancedGroupDAO dao = this.capture.wire(new UnbalancedGroupDAO(), DERBY); + Logger logger = (Logger) LoggerFactory.getLogger(AbstractSearcherDAO.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + dao.countGroups(new FieldSearchFilter[]{descriptionLike()}); + } finally { + logger.detachAppender(appender); + } + + String query = this.capture.single(); + assertEquals(2, SqlShape.occurrences(query, SqlShape.COUNT_PREFIX)); + assertEquals(1, SqlShape.occurrences(query, SqlShape.COUNT_SUFFIX)); + List errors = appender.list.stream() + .filter(event -> Level.ERROR.equals(event.getLevel())) + .map(ILoggingEvent::getFormattedMessage) + .collect(Collectors.toList()); + assertEquals(1, errors.size(), () -> "expected one report, got " + errors); + assertTrue(errors.get(0).contains(UnbalancedGroupDAO.class.getName()), errors.get(0)); + } + + // ---------------------------------------------------------------- order and paging + + /** + * ORDER BY and the paging block are appended on the list side only. They are what the count body + * must not contain: a count over a paged body would count one page, and a count that sorted would + * pay for a sort nobody reads. The per-vendor syntax of the block itself is covered by + * {@link QueryLimitResolverTest}. + */ + @Test + void orderAndPagingBelongToTheListQueryOnly() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + FieldSearchFilter[] filters = {ordered(descriptionLike(), FieldSearchFilter.ASC_ORDER), + new FieldSearchFilter(10, 5)}; + + dao.searchGroups(filters); + String listQuery = this.capture.single(); + this.capture.clear(); + dao.countGroups(filters); + String countQuery = this.capture.single(); + + assertEquals("OFFSET 5 ROWS FETCH NEXT 10 ROWS ONLY", SqlShape.pagingBlock(listQuery)); + assertEquals("", SqlShape.pagingBlock(countQuery)); + assertFalse(SqlShape.normalize(countQuery).contains("ORDER BY"), countQuery); + assertEquals(SqlShape.listBody(listQuery), SqlShape.countBody(countQuery)); + } + + // ---------------------------------------------------------------- fixtures + + private static FieldSearchFilter descriptionLike() { + return new FieldSearchFilter<>("descr", "test", true); + } + + private static FieldSearchFilter ordered(FieldSearchFilter filter, String order) { + filter.setOrder(order); + return filter; + } + + private static EntitySearchFilter ordered(EntitySearchFilter filter, String order) { + filter.setOrder(order); + return filter; + } + + private static EntitySearchFilter attributeLike() { + return new EntitySearchFilter<>("Nome", true, "abc", true); + } + + private static EntitySearchFilter orderedMetadata() { + return ordered(new EntitySearchFilter<>(IEntityManager.ENTITY_TYPE_CODE_FILTER_KEY, false, "PFL", false), + FieldSearchFilter.ASC_ORDER); + } + + /** Exposes the count entry point: no manager in the engine reaches it for profiles. */ + private static class ProbeProfileSearcherDAO extends UserProfileSearcherDAO { + + Integer count(EntitySearchFilter[] filters) { + return this.countId(filters); + } + } + + /** A subclass that writes an opening marker of its own: the wrapper then opens twice, closes once. */ + private static class UnbalancedGroupDAO extends GroupDAO { + + @Override + protected StringBuffer createMasterCountQueryBlock() { + return new StringBuffer(COUNT_QUERY_PREFIX).append(super.createMasterCountQueryBlock()); + } + } + +} diff --git a/engine/src/test/java/com/agiletec/aps/system/common/SqlShape.java b/engine/src/test/java/com/agiletec/aps/system/common/SqlShape.java new file mode 100644 index 0000000000..f0a389ac86 --- /dev/null +++ b/engine/src/test/java/com/agiletec/aps/system/common/SqlShape.java @@ -0,0 +1,195 @@ +/* + * Copyright 2015-Present Entando Inc. (http://www.entando.com) All rights reserved. + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + */ +package com.agiletec.aps.system.common; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.apache.commons.lang3.StringUtils; + +/** + * Reads the parts of a generated query that carry the contract, so that a shape assertion does not + * also assert whitespace or formatting. The count markers are the DAO's own constants: a test that + * pins the composition has to move when they move. + */ +public final class SqlShape { + + public static final String COUNT_PREFIX = AbstractSearcherDAO.COUNT_QUERY_PREFIX; + public static final String COUNT_SUFFIX = AbstractSearcherDAO.COUNT_QUERY_SUFFIX; + + private static final String ORDER_BY = " ORDER BY "; + private static final String GROUP_BY = "GROUP BY "; + /** The paging block, in either of the two syntaxes {@link QueryLimitResolver} emits. */ + private static final String[] PAGING = {" OFFSET ", " LIMIT "}; + + private SqlShape() { + // utility + } + + /** + * @param sql Any generated query. + * @return The same query with every run of whitespace reduced to one space. + */ + public static String normalize(String sql) { + return (null == sql) ? null : sql.replaceAll("\\s+", " ").trim(); + } + + public static int occurrences(String sql, String marker) { + return StringUtils.countMatches(normalize(sql), normalize(marker)); + } + + public static boolean isCountQuery(String sql) { + return normalize(sql).startsWith(normalize(COUNT_PREFIX)); + } + + /** + * @param countQuery A count query. + * @return The block the count counts: everything the wrapper encloses. + */ + public static String countBody(String countQuery) { + String query = normalize(countQuery); + String prefix = normalize(COUNT_PREFIX); + String suffix = normalize(COUNT_SUFFIX); + if (!query.startsWith(prefix) || !query.endsWith(suffix)) { + throw new IllegalArgumentException("not a count query: " + query); + } + return query.substring(prefix.length(), query.length() - suffix.length()).trim(); + } + + /** + * @param listQuery A list query. + * @return The block the list query pages over: everything before ORDER BY and the paging block. + */ + public static String listBody(String listQuery) { + String query = normalize(listQuery); + int cut = query.length(); + for (String marker : new String[]{ORDER_BY, PAGING[0], PAGING[1]}) { + int index = query.indexOf(marker); + if (index >= 0 && index < cut) { + cut = index; + } + } + return query.substring(0, cut).trim(); + } + + /** + * @param query A count or a list query. + * @return The projected columns, in order, without the DISTINCT keyword. + */ + public static List selectedColumns(String query) { + String body = isCountQuery(query) ? countBody(query) : normalize(query); + String selectList = StringUtils.substringBefore(StringUtils.substringAfter(body, "SELECT "), " FROM "); + selectList = StringUtils.removeStart(selectList.trim(), "DISTINCT ").trim(); + List columns = new ArrayList<>(); + for (String column : selectList.split(",")) { + columns.add(column.trim()); + } + return Collections.unmodifiableList(columns); + } + + public static boolean isDistinct(String query) { + String body = isCountQuery(query) ? countBody(query) : normalize(query); + return body.startsWith("SELECT DISTINCT "); + } + + /** + * @param query A list query. + * @return The columns the ORDER BY references, without their direction. + */ + public static List orderedColumns(String query) { + String normalized = normalize(query); + int index = normalized.indexOf(ORDER_BY); + if (index < 0) { + return Collections.emptyList(); + } + String block = normalized.substring(index + ORDER_BY.length()); + for (String marker : PAGING) { + block = StringUtils.substringBefore(block, marker); + } + List columns = new ArrayList<>(); + for (String term : block.split(",")) { + columns.add(term.trim().replaceAll("(?i)\\s+(ASC|DESC)$", "")); + } + return Collections.unmodifiableList(columns); + } + + /** + * @param query A count or a list query. + * @return The columns the body groups on, in order, or empty when the body does not group. + */ + public static List groupedColumns(String query) { + String body = isCountQuery(query) ? countBody(query) : normalize(query); + int index = body.indexOf(GROUP_BY); + if (index < 0) { + return Collections.emptyList(); + } + String block = body.substring(index + GROUP_BY.length()); + block = StringUtils.substringBefore(block, ORDER_BY.trim()); + List columns = new ArrayList<>(); + for (String term : block.split(",")) { + columns.add(term.trim()); + } + return Collections.unmodifiableList(columns); + } + + public static boolean isGrouped(String query) { + return !groupedColumns(query).isEmpty(); + } + + /** + * @param term One term of an ORDER BY block. + * @return True when the term is an aggregate rather than a plain column reference. + */ + public static boolean isAggregate(String term) { + return term.startsWith("MIN(") || term.startsWith("MAX("); + } + + /** + * @param term An aggregate term. + * @return The column the aggregate is taken over. + */ + public static String aggregatedColumn(String term) { + return StringUtils.substringBefore(StringUtils.substringAfter(term, "("), ")").trim(); + } + + /** + * @param query A list query. + * @return The paging block, or an empty string when the query is not paged. + */ + public static String pagingBlock(String query) { + String normalized = normalize(query); + for (String marker : PAGING) { + int index = normalized.indexOf(marker); + if (index >= 0) { + return normalized.substring(index).trim(); + } + } + return ""; + } + + /** + * @param query Any generated query. + * @return The joined tables, in the order they are joined. + */ + public static List joinedTables(String query) { + List tables = new ArrayList<>(); + String[] parts = normalize(query).split("(?i)INNER JOIN "); + for (int i = 1; i < parts.length; i++) { + tables.add(Arrays.stream(parts[i].split(" ")).findFirst().orElse("")); + } + return Collections.unmodifiableList(tables); + } + +} diff --git a/pom.xml b/pom.xml index 009335364b..4a4cd262e0 100644 --- a/pom.xml +++ b/pom.xml @@ -1629,6 +1629,94 @@ + + + test-postgresql + + entando + localhost + 55432 + org.postgresql.Driver + jdbc:postgresql://${test.database.hostname}:${test.database.port}/${test.database.name}port + jdbc:postgresql://${test.database.hostname}:${test.database.port}/${test.database.name}serv + + + + org.postgresql + postgresql + 42.7.8 + test + + + + + + test-mysql + + entando + localhost + 53306 + com.mysql.cj.jdbc.Driver + jdbc:mysql://${test.database.hostname}:${test.database.port}/${test.database.name}port?useSSL=false&allowPublicKeyRetrieval=true + jdbc:mysql://${test.database.hostname}:${test.database.port}/${test.database.name}serv?useSSL=false&allowPublicKeyRetrieval=true + + + + com.mysql + mysql-connector-j + 9.4.0 + test + + + + + test-oracle + + entando + localhost + 1521 + TESTENV + oracle.jdbc.OracleDriver + + jdbc:oracle:thin:@//${test.database.hostname}:${test.database.port}/${test.database.service} + jdbc:oracle:thin:@//${test.database.hostname}:${test.database.port}/${test.database.service} + + -Djava.security.egd=file:/dev/./urandom -Doracle.jdbc.disableOob=true -Doracle.net.disableOob=true + + + + com.oracle.ojdbc + ojdbc8 + 19.3.0.0 + test + + + local-dev diff --git a/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageDAO.java b/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageDAO.java index 9dadbf7601..5d70a2843d 100644 --- a/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageDAO.java +++ b/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageDAO.java @@ -67,6 +67,19 @@ protected String getAddEntityRecordQuery() { return ADD_MESSAGE; } + /** + * Drop the sub-second part of a date before it is written. Both columns are declared as a plain + * timestamp, so the fraction cannot be stored in any case, but the engines disagree on how they + * discard it: MySQL rounds to the nearest second while Derby, PostgreSQL and Oracle truncate. Left + * to the engine, a value written at .5 or later comes back a second ahead of the one held in memory. + * + * @param date The date to store. + * @return The same instant, floored to the second. + */ + private static Timestamp toWholeSeconds(java.util.Date date) { + return new Timestamp(Math.floorDiv(date.getTime(), 1000L) * 1000L); + } + @Override protected void buildAddEntityStatement(IApsEntity entity, PreparedStatement stat) throws Throwable { Message message = (Message) entity; @@ -74,7 +87,7 @@ protected void buildAddEntityStatement(IApsEntity entity, PreparedStatement stat stat.setString(2, message.getUsername()); stat.setString(3, message.getLangCode()); stat.setString(4, message.getTypeCode()); - stat.setTimestamp(5, new Timestamp(message.getCreationDate().getTime())); + stat.setTimestamp(5, toWholeSeconds(message.getCreationDate())); stat.setString(6, message.getXML()); } @@ -135,7 +148,7 @@ public void addAnswer(Answer answer) throws EntException { stat.setString(1, answer.getAnswerId()); stat.setString(2, answer.getMessageId()); stat.setString(3, answer.getOperator()); - stat.setTimestamp(4, new Timestamp(answer.getSendDate().getTime())); + stat.setTimestamp(4, toWholeSeconds(answer.getSendDate())); stat.setString(5, answer.getText()); stat.executeUpdate(); conn.commit(); diff --git a/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageSearcherDAO.java b/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageSearcherDAO.java index 6a01ca63b3..9f963c208f 100644 --- a/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageSearcherDAO.java +++ b/webdynamicform-plugin/src/main/java/com/agiletec/plugins/jpwebdynamicform/aps/system/services/message/MessageSearcherDAO.java @@ -95,7 +95,8 @@ protected String createMessageQueryString(EntitySearchFilter[] filters, boolean boolean hasAppendWhereClause = this.appendFullAttributeFilterQueryBlocks(filters, query, false); hasAppendWhereClause = this.appendMetadataFieldFilterQueryBlocks(filters, query, hasAppendWhereClause); this.appendAnsweredFilterQueryBlocks(answered, query, hasAppendWhereClause); - appendOrderQueryBlocks(filters, query, false); + boolean grouped = this.appendGroupByQueryBlock(filters, query, selectAll); + appendOrderQueryBlocks(filters, query, false, grouped); return query.toString(); }