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 4049dc9b6afc6597763ec4ed0668fb2f3388a8e8 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 | 76 ++- .../content/PublicContentSearcherDAO.java | 18 +- .../system/services/resource/ResourceDAO.java | 52 +- .../ContentSearchJoinCountRegressionTest.java | 312 +++++++++++ .../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 | 184 +++++- .../aps/system/common/SearchableFields.java | 86 +++ .../entity/AbstractEntitySearcherDAO.java | 278 +++++++-- .../authorization/AuthorizationDAO.java | 20 +- .../aps/system/services/group/GroupDAO.java | 10 +- .../services/pagemodel/PageModelDAO.java | 13 +- .../services/actionlog/ActionLogDAO.java | 58 +- .../services/guifragment/GuiFragmentDAO.java | 20 +- .../services/oauth2/OAuth2TokenDAO.java | 44 +- .../services/oauth2/OAuthConsumerDAO.java | 23 +- .../userprofile/UserProfileSearcherDAO.java | 37 +- .../java/com/agiletec/ConfigTestUtils.java | 18 + .../aps/system/common/QueryCapture.java | 97 ++++ .../system/common/QueryLimitResolverTest.java | 22 +- .../common/SearcherDaoQueryShapeTest.java | 530 ++++++++++++++++++ .../agiletec/aps/system/common/SqlShape.java | 198 +++++++ .../ApiConsumerControllerIntegrationTest.java | 14 + .../GuiFragmentControllerIntegrationTest.java | 17 + pom.xml | 88 +++ .../services/mapping/SeoMappingDAO.java | 12 +- .../system/services/message/MessageDAO.java | 17 +- .../services/message/MessageSearcherDAO.java | 38 +- 34 files changed, 2594 insertions(+), 215 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/main/java/com/agiletec/aps/system/common/SearchableFields.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..4aede8e6f0 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 @@ -14,6 +14,7 @@ package com.agiletec.plugins.jacms.aps.system.services.content; import com.agiletec.aps.system.SystemConstants; +import com.agiletec.aps.system.common.SearchableFields; import com.agiletec.aps.system.common.entity.AbstractEntitySearcherDAO; import com.agiletec.aps.system.common.entity.model.ApsEntityRecord; import com.agiletec.aps.system.common.entity.model.EntitySearchFilter; @@ -40,6 +41,31 @@ public abstract class AbstractContentSearcherDAO extends AbstractEntitySearcherDAO implements IContentSearcherDAO { private static final EntLogger _logger = EntLogFactory.getSanitizedLogger(AbstractContentSearcherDAO.class); + + private static final String CONTENTID = "contentid"; + private static final String CONTENTTYPE = "contenttype"; + + /** + * The search keys this searcher accepts. Most name their column directly; the entity keys and the + * two whose column is spelled differently are declared as aliases. group is accepted + * because the chain this replaced accepted it - it is a code literal, not caller input, and keeping + * it makes this an exact translation of that chain rather than a silent correction to it. + */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "descr", + "status", + "created", + "published", + "maingroup", + "currentversion", + "firsteditor", + "lasteditor", + "restriction", + "group") + .alias(IContentManager.ENTITY_ID_FILTER_KEY, CONTENTID) + .alias(IContentManager.ENTITY_TYPE_CODE_FILTER_KEY, CONTENTTYPE) + .alias(IContentManager.CONTENT_MODIFY_DATE_FILTER_KEY, "lastmodified") + .alias(IContentManager.CONTENT_ONLINE_FILTER_KEY, "onlinexml"); @Override public int countContents(String[] categories, boolean orClauseCategoryFilter, @@ -105,36 +131,8 @@ public List loadContentsId(String[] categories, } @Override - protected String getTableFieldName(String metadataFieldKey) { - if (metadataFieldKey.equals(IContentManager.ENTITY_ID_FILTER_KEY)) { - return this.getEntityMasterTableIdFieldName(); - } else if (metadataFieldKey.equals(IContentManager.ENTITY_TYPE_CODE_FILTER_KEY)) { - return this.getEntityMasterTableIdTypeFieldName(); - } else if (metadataFieldKey.equals(IContentManager.CONTENT_DESCR_FILTER_KEY)) { - return "descr"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_STATUS_FILTER_KEY)) { - return "status"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_CREATION_DATE_FILTER_KEY)) { - return "created"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_MODIFY_DATE_FILTER_KEY)) { - return "lastmodified"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_PUBLISH_DATE_FILTER_KEY)) { - return "published"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_ONLINE_FILTER_KEY)) { - return "onlinexml"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_MAIN_GROUP_FILTER_KEY)) { - return "maingroup"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_CURRENT_VERSION_FILTER_KEY)) { - return "currentversion"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_FIRST_EDITOR_FILTER_KEY)) { - return "firsteditor"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_LAST_EDITOR_FILTER_KEY)) { - return "lasteditor"; - } else if (metadataFieldKey.equals(IContentManager.CONTENT_RESTRICTION_FILTER_KEY)) { - return "restriction"; - }else if (metadataFieldKey.equals(IContentManager.CONTENT_GROUP_FILTER_KEY)) { - return "group"; - } else throw new RuntimeException("Chiave di ricerca '" + metadataFieldKey + "' non riconosciuta"); + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } protected PreparedStatement buildStatement(EntitySearchFilter[] filters, @@ -156,7 +154,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 +215,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) { @@ -284,8 +280,8 @@ protected Collection getGroupsForSelect(Collection userGroupCode @Override protected ApsEntityRecord createRecord(ResultSet result) throws Throwable { ContentRecordVO contentVo = new ContentRecordVO(); - contentVo.setId(result.getString("contentid")); - contentVo.setTypeCode(result.getString("contenttype")); + contentVo.setId(result.getString(CONTENTID)); + contentVo.setTypeCode(result.getString(CONTENTTYPE)); contentVo.setDescription(result.getString("descr")); contentVo.setStatus(result.getString("status")); String xmlWork = result.getString("workxml"); @@ -311,11 +307,11 @@ protected String getEntityMasterTableName() { } @Override protected String getEntityMasterTableIdFieldName() { - return "contentid"; + return CONTENTID; } @Override protected String getEntityMasterTableIdTypeFieldName() { - return "contenttype"; + return CONTENTTYPE; } protected abstract String getContentRelationsTableName(); 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..e73abc6900 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 @@ -15,6 +15,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import com.agiletec.aps.system.services.category.Category; import com.agiletec.aps.system.services.category.ICategoryManager; import com.agiletec.plugins.jacms.aps.system.services.resource.model.ResourceInterface; @@ -48,6 +49,20 @@ public class ResourceDAO extends AbstractSearcherDAO implements IResourceDAO { private static final EntLogger logger = EntLogFactory.getSanitizedLogger(ResourceDAO.class); + + /** The columns of resources a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "resid", + "restype", + "descr", + "maingroup", + "resourcexml", + "masterfilename", + "creationdate", + "lastmodified", + "owner", + "folderpath", + "correlationcode"); private ICategoryManager categoryManager; @@ -361,7 +376,7 @@ private PreparedStatement buildStatement(FieldSearchFilter[] filters, List 0) { for (String category : categories) { @@ -377,19 +392,19 @@ private PreparedStatement buildStatement(FieldSearchFilter[] filters, List categories, boolean isCount) { - StringBuffer query = this.createBaseQueryBlock(filters, false, isCount, categories); + StringBuffer query = this.createBaseQueryBlock(filters, false, categories); this.appendMetadataFieldFilterQueryBlocks(filters, query, false); 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); + private StringBuffer createBaseQueryBlock(FieldSearchFilter[] filters, boolean selectAll, List categories) { + // 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 +578,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"; @@ -574,8 +608,8 @@ protected String getMasterTableIdFieldName() { } @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } } 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..bbe2f12a34 --- /dev/null +++ b/cms-plugin/src/test/java/com/agiletec/plugins/jacms/aps/system/services/content/ContentSearchJoinCountRegressionTest.java @@ -0,0 +1,312 @@ +/* + * 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 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 + 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)) + .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)) + .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()) + .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..e32f974fc0 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,11 +23,15 @@ 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.exception.EntRuntimeException; import org.entando.entando.ent.util.EntLogging.EntLogger; import org.entando.entando.ent.util.EntLogging.EntLogFactory; @@ -43,6 +47,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 +110,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 +122,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 +270,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 +304,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) { @@ -309,7 +371,7 @@ protected boolean addFilters(FieldSearchFilter filter, StringBuffer query, boole return hasAppendWhereClause; } hasAppendWhereClause = this.verifyWhereClauseAppend(query, hasAppendWhereClause); - String tableFieldName = this.getTableFieldName(filter.getKey()); + String tableFieldName = this.resolveTableFieldName(filter.getKey()); if (filter.getAllowedValues() != null && filter.getAllowedValues().size() > 0) { List allowedValues = filter.getAllowedValues(); for (int j = 0; j < allowedValues.size(); j++) { @@ -368,6 +430,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) { @@ -376,13 +440,68 @@ protected boolean appendOrderQueryBlocks(FieldSearchFilter[] filters, StringBuff } else { query.append(", "); } - String fieldName = this.getTableFieldName(filter.getKey()); + String fieldName = this.resolveTableFieldName(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.resolveTableFieldName(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 "); @@ -393,7 +512,40 @@ protected boolean verifyWhereClauseAppend(StringBuffer query, boolean hasAppendW return hasAppendWhereClause; } - protected abstract String getTableFieldName(String metadataFieldKey); + /** + * The column a caller-provided metadata key names, looked up in the fields this searcher accepts. + * Every query block that concatenates a column name goes through here. + * + *

Filter values are bound as parameters, but a filter key becomes a column name + * by concatenation - in the WHERE block, in the ORDER BY, in the projection and in the GROUP BY - + * so an unchecked key is a SQL injection vector. Searchers used to map the key themselves, either + * straight through - relying on the REST layer validating it against a DTO's fields, a guarantee + * made far from here, invisible to static analysis, and absent for every non-REST caller - or + * through a chain of comparisons that each had to remember to reject the unknown key.

+ * + *

The lookup happens here instead, once, so no subclass can be written that skips it, and what + * comes back is the column as {@link #getSearchableFields()} declares it, never as the caller + * spelled it.

+ * + * @param metadataFieldKey The key supplied by the caller. + * @return The column it names, in the spelling this searcher declares. + */ + protected final String resolveTableFieldName(String metadataFieldKey) { + String column = this.getSearchableFields().columnFor(metadataFieldKey); + if (null == column) { + logger.error("Unrecognized search key '{}' for {}", metadataFieldKey, this.getClass().getName()); + throw new EntRuntimeException("Unrecognized search key for " + this.getClass().getName()); + } + return column; + } + + /** + * The search keys this searcher accepts and the column each one names, as literals - never caller + * input. A key outside them is refused. + * + * @return The accepted fields. + */ + protected abstract SearchableFields getSearchableFields(); /** * Return the name of the entities master table. diff --git a/engine/src/main/java/com/agiletec/aps/system/common/SearchableFields.java b/engine/src/main/java/com/agiletec/aps/system/common/SearchableFields.java new file mode 100644 index 0000000000..01d840edd1 --- /dev/null +++ b/engine/src/main/java/com/agiletec/aps/system/common/SearchableFields.java @@ -0,0 +1,86 @@ +/* + * 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.HashMap; +import java.util.Locale; +import java.util.Map; + +/** + * The search keys a searcher DAO accepts, and the column each one names. + * + *

A filter key becomes a column name by concatenation, so the set of keys a searcher accepts is + * also the bound on what can reach the SQL. Declaring it as data - rather than as a chain of + * comparisons inside each DAO - is what lets {@link AbstractSearcherDAO} apply the same check to + * every searcher, and lets a searcher whose keys differ from its columns say so instead of relying + * on the two happening to coincide.

+ * + *

Keys are matched without regard to case, because the keys callers send are DTO field names + * (pluginCode) while columns are spelled as the schema declares them + * (plugincode), and every database the engine supports folds unquoted identifiers. What + * a lookup returns is always the column as declared here, never as the caller spelled it.

+ * + * @author E.Santoboni + */ +public final class SearchableFields { + + private final Map columnsByKey; + + private SearchableFields(Map columnsByKey) { + this.columnsByKey = columnsByKey; + } + + /** + * Fields whose key is the column name. + * + * @param columns The columns, as literals - never caller input. + * @return The fields those columns make up. + */ + public static SearchableFields columns(String... columns) { + Map columnsByKey = new HashMap<>(); + for (String column : columns) { + columnsByKey.put(normalize(column), column); + } + return new SearchableFields(columnsByKey); + } + + /** + * These fields, plus a key naming a column spelled differently - a logical key such as + * entityId, or one the REST layer exposes under another name. + * + * @param searchKey The key callers use. + * @param column The column it names, as a literal - never caller input. + * @return The fields, with that key added. + */ + public SearchableFields alias(String searchKey, String column) { + Map widened = new HashMap<>(this.columnsByKey); + widened.put(normalize(searchKey), column); + return new SearchableFields(widened); + } + + /** + * The column a key names. + * + * @param searchKey The key supplied by the caller. + * @return The column, as declared here, or null when the key is not one of these fields. + */ + String columnFor(String searchKey) { + return (null == searchKey) ? null : this.columnsByKey.get(normalize(searchKey)); + } + + private static String normalize(String searchKey) { + return searchKey.toLowerCase(Locale.ROOT); + } + +} 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..bf33d2a92f 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; @@ -36,6 +39,8 @@ public abstract class AbstractEntitySearcherDAO extends AbstractSearcherDAO implements IEntitySearcherDAO { private static final EntLogger _logger = EntLogFactory.getSanitizedLogger(AbstractEntitySearcherDAO.class); + private static final String TEXTVALUE = "textvalue"; + @Override public List searchRecords(EntitySearchFilter[] filters) { @@ -138,7 +143,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 +221,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 +242,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 +255,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.resolveTableFieldName(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.resolveTableFieldName(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 +568,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 +603,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()); + String fieldName = this.resolveTableFieldName(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 +648,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 +698,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) { @@ -514,13 +722,13 @@ private String getAttributeFieldColunm(Object attributeValue) { if (null == attributeValue) { columnName = null; } else if (attributeValue instanceof String) { - columnName = "textvalue"; + columnName = TEXTVALUE; } else if (attributeValue instanceof Date) { columnName = "datevalue"; } else if (attributeValue instanceof BigDecimal) { columnName = "numvalue"; } else if (attributeValue instanceof Boolean) { - columnName = "textvalue"; + columnName = TEXTVALUE; } return columnName; } diff --git a/engine/src/main/java/com/agiletec/aps/system/services/authorization/AuthorizationDAO.java b/engine/src/main/java/com/agiletec/aps/system/services/authorization/AuthorizationDAO.java index bc60d8ebc9..c90772d96b 100644 --- a/engine/src/main/java/com/agiletec/aps/system/services/authorization/AuthorizationDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/services/authorization/AuthorizationDAO.java @@ -15,6 +15,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import com.agiletec.aps.system.services.group.Group; import com.agiletec.aps.system.services.role.Role; @@ -42,6 +43,15 @@ public class AuthorizationDAO extends AbstractSearcherDAO implements IAuthorizat public static final int BATCH_SIZE_FLUSH = 50; private static final EntLogger _logger = EntLogFactory.getSanitizedLogger(AuthorizationDAO.class); + + private static final String USERNAME = "username"; + + /** The columns of authusergrouprole a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "id", + USERNAME, + "groupname", + "rolename"); @Override public void addUserAuthorization(String username, Authorization authorization) { @@ -234,7 +244,7 @@ public boolean externalAuthSyncCheck(final String username, final Long iat) { try (ResultSet rs = selectStmt.executeQuery()) { if (rs.next()) { - userId = rs.getString("username"); + userId = rs.getString(USERNAME); lastSyncedIat = rs.getLong("iat"); } } @@ -274,7 +284,7 @@ public void externalAuthSync(final String username, final Long iat, try (ResultSet rs = selectStmt.executeQuery()) { if (rs.next()) { - usernameTracked = rs.getString("username"); + usernameTracked = rs.getString(USERNAME); oldIat = rs.getLong("iat"); } } @@ -494,8 +504,8 @@ public int doBatchDeletion(Connection conn, long epochSeconds, int batchSize) th @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override @@ -505,7 +515,7 @@ protected String getMasterTableName() { @Override protected String getMasterTableIdFieldName() { - return "username"; + return USERNAME; } private final String ADD_AUTHORIZATION = diff --git a/engine/src/main/java/com/agiletec/aps/system/services/group/GroupDAO.java b/engine/src/main/java/com/agiletec/aps/system/services/group/GroupDAO.java index df4edafed9..7e0c63832b 100644 --- a/engine/src/main/java/com/agiletec/aps/system/services/group/GroupDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/services/group/GroupDAO.java @@ -23,6 +23,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import org.entando.entando.ent.util.EntLogging.EntLogger; import org.entando.entando.ent.util.EntLogging.EntLogFactory; @@ -33,6 +34,11 @@ public class GroupDAO extends AbstractSearcherDAO implements IGroupDAO { private static final EntLogger logger = EntLogFactory.getSanitizedLogger(GroupDAO.class); + + /** The columns of authgroups a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "groupname", + "descr"); @Override public int countGroups(FieldSearchFilter[] filters) { @@ -160,8 +166,8 @@ public void deleteGroup(String groupName) { } @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override diff --git a/engine/src/main/java/com/agiletec/aps/system/services/pagemodel/PageModelDAO.java b/engine/src/main/java/com/agiletec/aps/system/services/pagemodel/PageModelDAO.java index 13474df09c..872b0eac3f 100644 --- a/engine/src/main/java/com/agiletec/aps/system/services/pagemodel/PageModelDAO.java +++ b/engine/src/main/java/com/agiletec/aps/system/services/pagemodel/PageModelDAO.java @@ -23,6 +23,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import org.entando.entando.ent.exception.EntException; import org.apache.commons.lang3.StringUtils; import org.entando.entando.aps.system.services.widgettype.IWidgetTypeManager; @@ -36,6 +37,14 @@ public class PageModelDAO extends AbstractSearcherDAO implements IPageModelDAO { private static final EntLogger logger = EntLogFactory.getSanitizedLogger(PageModelDAO.class); + + /** The columns of pagemodels a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "code", + "descr", + "frames", + "plugincode", + "templategui"); @Override public int count(FieldSearchFilter[] filters) { @@ -191,8 +200,8 @@ public void deleteModel(String code) { } @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override 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..5bbe0bd1e8 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 @@ -33,6 +33,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import com.agiletec.aps.system.services.group.Group; import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; @@ -51,6 +52,25 @@ public class ActionLogDAO extends AbstractSearcherDAO implements IActionLogDAO { private final EntLogger logger = EntLogFactory.getSanitizedLogger(getClass()); + private static final String USERNAME = "username"; + private static final String ACTIONDATE = "actiondate"; + private static final String NAMESPACE = "namespace"; + private static final String ACTIONNAME = "actionname"; + private static final String PARAMETERS = "parameters"; + private static final String ACTIVITYSTREAMINFO = "activitystreaminfo"; + private static final String UPDATEDATE = "updatedate"; + + /** The columns of actionlogrecords a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "id", + USERNAME, + ACTIONDATE, + NAMESPACE, + ACTIONNAME, + PARAMETERS, + ACTIVITYSTREAMINFO, + UPDATEDATE); + private static final String ADD_ACTION_RECORD = "INSERT INTO actionlogrecords ( id, username, actiondate, namespace, actionname, parameters, activitystreaminfo, updatedate) " + "VALUES ( ? , ? , ? , ? , ? , ? , ? , ? )"; @@ -241,7 +261,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); @@ -328,22 +348,22 @@ protected FieldSearchFilter[] createFilters(IActionLogRecordSearchBean searchBea } String username = searchBean.getUsername(); if (null != username && username.trim().length() > 0) { - FieldSearchFilter filter = new FieldSearchFilter("username", this.extractSearchValues(username), true); + FieldSearchFilter filter = new FieldSearchFilter(USERNAME, this.extractSearchValues(username), true); filters = super.addFilter(filters, filter); } String namespace = searchBean.getNamespace(); if (null != namespace && namespace.trim().length() > 0) { - FieldSearchFilter filter = new FieldSearchFilter("namespace", this.extractSearchValues(namespace), true); + FieldSearchFilter filter = new FieldSearchFilter(NAMESPACE, this.extractSearchValues(namespace), true); filters = super.addFilter(filters, filter); } String actionName = searchBean.getActionName(); if (null != actionName && actionName.trim().length() > 0) { - FieldSearchFilter filter = new FieldSearchFilter("actionname", this.extractSearchValues(actionName), true); + FieldSearchFilter filter = new FieldSearchFilter(ACTIONNAME, this.extractSearchValues(actionName), true); filters = super.addFilter(filters, filter); } String parameters = searchBean.getParams(); if (null != parameters && parameters.trim().length() > 0) { - FieldSearchFilter filter = new FieldSearchFilter("parameters", this.extractSearchValues(parameters), true); + FieldSearchFilter filter = new FieldSearchFilter(PARAMETERS, this.extractSearchValues(parameters), true); filters = super.addFilter(filters, filter); } Date startCreation = searchBean.getStartCreation(); @@ -351,7 +371,7 @@ protected FieldSearchFilter[] createFilters(IActionLogRecordSearchBean searchBea if (null != startCreation || null != endCreation) { Timestamp tsStart = (null != startCreation) ? new Timestamp(startCreation.getTime()) : null; Timestamp tsEnd = (null != endCreation) ? new Timestamp(endCreation.getTime()) : null; - FieldSearchFilter filter = new FieldSearchFilter("actiondate", tsStart, tsEnd); + FieldSearchFilter filter = new FieldSearchFilter(ACTIONDATE, tsStart, tsEnd); filter.setOrder(FieldSearchFilter.Order.DESC); filters = super.addFilter(filters, filter); } @@ -360,12 +380,12 @@ protected FieldSearchFilter[] createFilters(IActionLogRecordSearchBean searchBea if (null != startUpdate || null != endUpdate) { Timestamp tsStart = (null != startUpdate) ? new Timestamp(startUpdate.getTime()) : null; Timestamp tsEnd = (null != endUpdate) ? new Timestamp(endUpdate.getTime()) : null; - FieldSearchFilter filter = new FieldSearchFilter("updatedate", tsStart, tsEnd); + FieldSearchFilter filter = new FieldSearchFilter(UPDATEDATE, tsStart, tsEnd); filter.setOrder(FieldSearchFilter.Order.DESC); filters = super.addFilter(filters, filter); } if (searchBean instanceof IActivityStreamSearchBean) { - FieldSearchFilter filter = new FieldSearchFilter("activitystreaminfo"); + FieldSearchFilter filter = new FieldSearchFilter(ACTIVITYSTREAMINFO); filters = super.addFilter(filters, filter); } } @@ -392,15 +412,15 @@ public ActionLogRecord getActionRecord(int id) { if (res.next()) { actionRecord = new ActionLogRecord(); actionRecord.setId(id); - Timestamp actionDate = res.getTimestamp("actiondate"); + Timestamp actionDate = res.getTimestamp(ACTIONDATE); actionRecord.setActionDate(new Date(actionDate.getTime())); - Timestamp updateDate = res.getTimestamp("updatedate"); + Timestamp updateDate = res.getTimestamp(UPDATEDATE); actionRecord.setUpdateDate(new Date(updateDate.getTime())); - actionRecord.setActionName(res.getString("actionname")); - actionRecord.setNamespace(res.getString("namespace")); - actionRecord.setParameters(res.getString("parameters")); - actionRecord.setUsername(res.getString("username")); - String asiXml = res.getString("activitystreaminfo"); + actionRecord.setActionName(res.getString(ACTIONNAME)); + actionRecord.setNamespace(res.getString(NAMESPACE)); + actionRecord.setParameters(res.getString(PARAMETERS)); + actionRecord.setUsername(res.getString(USERNAME)); + String asiXml = res.getString(ACTIVITYSTREAMINFO); if (null != asiXml && asiXml.trim().length() > 0) { ActivityStreamInfo asi = ActivityStreamInfoDOM.unmarshalInfo(asiXml); actionRecord.setActivityStreamInfo(asi); @@ -466,8 +486,8 @@ protected String getMasterTableIdFieldName() { } @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override @@ -520,9 +540,9 @@ private void extractRecordToDelete(String groupName, ResultSet result = null; try { List idList = new ArrayList<>(); - FieldSearchFilter filter1 = new FieldSearchFilter("actiondate"); + FieldSearchFilter filter1 = new FieldSearchFilter(ACTIONDATE); filter1.setOrder(FieldSearchFilter.Order.DESC); - FieldSearchFilter filter2 = new FieldSearchFilter("activitystreaminfo"); + FieldSearchFilter filter2 = new FieldSearchFilter(ACTIVITYSTREAMINFO); FieldSearchFilter[] filters = {filter1, filter2}; List groupCodes = new ArrayList<>(); groupCodes.add(groupName); diff --git a/engine/src/main/java/org/entando/entando/aps/system/services/guifragment/GuiFragmentDAO.java b/engine/src/main/java/org/entando/entando/aps/system/services/guifragment/GuiFragmentDAO.java index 7d840be09b..75eaaa08a9 100644 --- a/engine/src/main/java/org/entando/entando/aps/system/services/guifragment/GuiFragmentDAO.java +++ b/engine/src/main/java/org/entando/entando/aps/system/services/guifragment/GuiFragmentDAO.java @@ -22,6 +22,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import org.apache.commons.lang3.StringUtils; import org.entando.entando.ent.util.EntLogging.EntLogger; import org.entando.entando.ent.util.EntLogging.EntLogFactory; @@ -33,6 +34,17 @@ public class GuiFragmentDAO extends AbstractSearcherDAO implements IGuiFragmentD private static final EntLogger logger = EntLogFactory.getSanitizedLogger(GuiFragmentDAO.class); + private static final String PLUGINCODE = "plugincode"; + + /** The columns of guifragment a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "code", + "widgettypecode", + PLUGINCODE, + "gui", + "defaultgui", + "locked"); + private static final String ADD_GUIFRAGMENT = "INSERT INTO guifragment (code, widgettypecode, plugincode, gui, locked ) VALUES (? , ? , ? , ? , ?)"; private static final String UPDATE_GUIFRAGMENT = "UPDATE guifragment SET widgettypecode = ?, plugincode = ? , gui = ? WHERE code = ? "; @@ -44,8 +56,8 @@ public class GuiFragmentDAO extends AbstractSearcherDAO implements IGuiFragmentD private static final String LOAD_GUIFRAGMENT_PLUGIN_CODES = "SELECT plugincode FROM guifragment"; @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override @@ -230,7 +242,7 @@ protected GuiFragment buildGuiFragmentFromRes(ResultSet res) { guiFragment = new GuiFragment(); guiFragment.setCode(res.getString("code")); guiFragment.setWidgetTypeCode(res.getString("widgettypecode")); - guiFragment.setPluginCode(res.getString("plugincode")); + guiFragment.setPluginCode(res.getString(PLUGINCODE)); guiFragment.setGui(res.getString("gui")); guiFragment.setDefaultGui(res.getString("defaultgui")); Integer locked = res.getInt("locked"); @@ -252,7 +264,7 @@ public List loadGuiFragmentPluginCodes() { stat = conn.prepareStatement(LOAD_GUIFRAGMENT_PLUGIN_CODES); res = stat.executeQuery(); while (res.next()) { - String code = res.getString("plugincode"); + String code = res.getString(PLUGINCODE); if (StringUtils.isNotEmpty(code) && !codes.contains(code)) { codes.add(code); } diff --git a/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuth2TokenDAO.java b/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuth2TokenDAO.java index bf03304443..a4c72c3000 100644 --- a/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuth2TokenDAO.java +++ b/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuth2TokenDAO.java @@ -15,6 +15,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import org.entando.entando.ent.util.EntLogging.EntLogger; import org.entando.entando.ent.util.EntLogging.EntLogFactory; @@ -41,6 +42,21 @@ public class OAuth2TokenDAO extends AbstractSearcherDAO implements IOAuth2TokenD private static final EntLogger logger = EntLogFactory.getSanitizedLogger(OAuth2TokenDAO.class); + private static final String CLIENTID = "clientid"; + private static final String EXPIRESIN = "expiresin"; + private static final String REFRESHTOKEN = "refreshtoken"; + private static final String GRANTTYPE = "granttype"; + private static final String LOCALUSER = "localuser"; + + /** The columns of api_oauth_tokens a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "accesstoken", + CLIENTID, + EXPIRESIN, + REFRESHTOKEN, + GRANTTYPE, + LOCALUSER); + private static final String ERROR_REMOVE_ACCESS_TOKEN = "Error while remove access token"; private static final String INSERT_TOKEN = "INSERT INTO api_oauth_tokens (accesstoken, clientid, expiresin, refreshtoken, granttype, localuser) VALUES (? , ? , ? , ? , ?, ?)"; @@ -60,8 +76,8 @@ public class OAuth2TokenDAO extends AbstractSearcherDAO implements IOAuth2TokenD private static final String DELETE_TOKEN_BY_REFRESH = DELETE_TOKEN_PREFIX + "WHERE refreshtoken = ? "; @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override @@ -79,15 +95,15 @@ public List findTokensByClientIdAndUserName(String clientId, if (StringUtils.isBlank(clientId) && StringUtils.isBlank(username)) { throw new RuntimeException("clientId and username cannot both be null"); } - FieldSearchFilter expirationFilter = new FieldSearchFilter("expiresin"); + FieldSearchFilter expirationFilter = new FieldSearchFilter(EXPIRESIN); expirationFilter.setOrder(FieldSearchFilter.Order.ASC); FieldSearchFilter[] filters = {expirationFilter}; if (!StringUtils.isBlank(clientId)) { - FieldSearchFilter clientIdFilter = new FieldSearchFilter("clientid", clientId, true); + FieldSearchFilter clientIdFilter = new FieldSearchFilter(CLIENTID, clientId, true); filters = ArrayUtils.add(filters, clientIdFilter); } if (!StringUtils.isBlank(username)) { - FieldSearchFilter usernameFilter = new FieldSearchFilter("localuser", username, true); + FieldSearchFilter usernameFilter = new FieldSearchFilter(LOCALUSER, username, true); filters = ArrayUtils.add(filters, usernameFilter); } List accessTokens = new ArrayList<>(); @@ -122,20 +138,20 @@ protected OAuth2AccessToken getAccessToken(final String token, Connection conn) stat.setString(1, token); res = stat.executeQuery(); if (res.next()) { - String refreshTokenValue = res.getString("refreshtoken"); + String refreshTokenValue = res.getString(REFRESHTOKEN); OAuth2RefreshToken refreshToken = refreshTokenValue != null ? new OAuth2RefreshToken(refreshTokenValue, java.time.Instant.now()) : null; - Timestamp timestamp = res.getTimestamp("expiresin"); + Timestamp timestamp = res.getTimestamp(EXPIRESIN); Date expiration = new Date(timestamp.getTime()); // Use the immutable constructor pattern accessToken = new OAuth2AccessTokenImpl( token, expiration.toInstant(), - res.getString("clientid"), - res.getString("granttype"), - res.getString("localuser"), + res.getString(CLIENTID), + res.getString(GRANTTYPE), + res.getString(LOCALUSER), refreshToken ); } @@ -283,7 +299,7 @@ public void deleteExpiredToken(int expirationTime) { @Override public OAuth2RefreshToken readRefreshToken(String tokenValue) { - FieldSearchFilter filter = new FieldSearchFilter("refreshtoken", tokenValue, true); + FieldSearchFilter filter = new FieldSearchFilter(REFRESHTOKEN, tokenValue, true); FieldSearchFilter[] filters = {filter}; List accessTokens = super.searchId(filters); if (null != accessTokens && accessTokens.size() > 0) { @@ -304,9 +320,9 @@ public OAuth2Authorization readAuthenticationForRefreshToken(OAuth2RefreshToken stat.setString(1, refreshToken.getTokenValue()); res = stat.executeQuery(); if (res.next()) { - String username = res.getString("localuser"); - String clientId = res.getString("clientid"); - String grantType = res.getString("granttype"); + String username = res.getString(LOCALUSER); + String clientId = res.getString(CLIENTID); + String grantType = res.getString(GRANTTYPE); // In Spring Security 6.x OAuth2Authorization, we need to build a more complete structure // For now, we'll return null and log a warning since this method should be handled diff --git a/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuthConsumerDAO.java b/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuthConsumerDAO.java index fb5caf2695..a2652ffd1b 100644 --- a/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuthConsumerDAO.java +++ b/engine/src/main/java/org/entando/entando/aps/system/services/oauth2/OAuthConsumerDAO.java @@ -22,6 +22,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import org.entando.entando.ent.exception.EntException; import java.sql.Types; import java.util.ArrayList; @@ -40,6 +41,20 @@ public class OAuthConsumerDAO extends AbstractSearcherDAO implements IOAuthConsu private static final EntLogger logger = EntLogFactory.getSanitizedLogger(OAuthConsumerDAO.class); + private static final String CONSUMERKEY = "consumerkey"; + + /** The columns of api_oauth_consumers a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + CONSUMERKEY, + "consumersecret", + "name", + "description", + "callbackurl", + "scope", + "authorizedgranttypes", + "expirationdate", + "issueddate"); + private static final String SELECT_CONSUMER = "SELECT consumerkey, consumersecret, name, description, callbackurl, scope, authorizedgranttypes, expirationdate, issueddate " + "FROM api_oauth_consumers WHERE consumerkey = ? "; @@ -134,7 +149,7 @@ public List getConsumers(FieldSearchFilter[] filters) { private ConsumerRecordVO consumerFromResultSet(ResultSet res) throws SQLException { ConsumerRecordVO consumer = new ConsumerRecordVO(); - consumer.setKey(res.getString("consumerkey")); + consumer.setKey(res.getString(CONSUMERKEY)); consumer.setSecret(res.getString("consumersecret")); consumer.setCallbackUrl(res.getString("callbackurl")); consumer.setName(res.getString("name")); @@ -237,7 +252,7 @@ public void deleteConsumer(String clientId) { @Override protected String getMasterTableIdFieldName() { - return "consumerkey"; + return CONSUMERKEY; } @Override @@ -246,8 +261,8 @@ protected String getMasterTableName() { } @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } } diff --git a/engine/src/main/java/org/entando/entando/aps/system/services/userprofile/UserProfileSearcherDAO.java b/engine/src/main/java/org/entando/entando/aps/system/services/userprofile/UserProfileSearcherDAO.java index 43ab6d449a..900fa4d1b8 100644 --- a/engine/src/main/java/org/entando/entando/aps/system/services/userprofile/UserProfileSearcherDAO.java +++ b/engine/src/main/java/org/entando/entando/aps/system/services/userprofile/UserProfileSearcherDAO.java @@ -17,6 +17,7 @@ import org.entando.entando.aps.system.services.userprofile.model.UserProfileRecord; +import com.agiletec.aps.system.common.SearchableFields; import com.agiletec.aps.system.common.entity.AbstractEntitySearcherDAO; import com.agiletec.aps.system.common.entity.IEntityManager; import com.agiletec.aps.system.common.entity.model.ApsEntityRecord; @@ -27,12 +28,22 @@ */ public class UserProfileSearcherDAO extends AbstractEntitySearcherDAO { + private static final String USERNAME = "username"; + private static final String PROFILETYPE = "profiletype"; + + /** The search keys this searcher accepts. username is the master table's id column. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + USERNAME, + "publicprofile") + .alias(IEntityManager.ENTITY_ID_FILTER_KEY, USERNAME) + .alias(IEntityManager.ENTITY_TYPE_CODE_FILTER_KEY, PROFILETYPE); + @Override protected ApsEntityRecord createRecord(ResultSet result) throws Throwable { UserProfileRecord record = new UserProfileRecord(); - record.setId(result.getString("username")); + record.setId(result.getString(USERNAME)); record.setXml(result.getString("profilexml")); - record.setTypeCode(result.getString("profiletype")); + record.setTypeCode(result.getString(PROFILETYPE)); record.setPublicProfile(result.getInt("publicprofile") == 1); return record; } @@ -44,12 +55,12 @@ protected String getEntityMasterTableName() { @Override protected String getEntityMasterTableIdFieldName() { - return "username"; + return USERNAME; } @Override protected String getEntityMasterTableIdTypeFieldName() { - return "profiletype"; + return PROFILETYPE; } @Override @@ -59,7 +70,7 @@ protected String getEntitySearchTableName() { @Override protected String getEntitySearchTableIdFieldName() { - return "username"; + return USERNAME; } @Override @@ -69,22 +80,12 @@ protected String getEntityAttributeRoleTableName() { @Override protected String getEntityAttributeRoleTableIdFieldName() { - return "username"; + return USERNAME; } @Override - protected String getTableFieldName(String metadataFieldKey) { - if (metadataFieldKey.equalsIgnoreCase("username")) { - return this.getEntityMasterTableIdFieldName(); - } else if (metadataFieldKey.equals(IEntityManager.ENTITY_ID_FILTER_KEY)) { - return this.getEntityMasterTableIdFieldName(); - } else if (metadataFieldKey.equals(IEntityManager.ENTITY_TYPE_CODE_FILTER_KEY)) { - return this.getEntityMasterTableIdTypeFieldName(); - } else if (metadataFieldKey.equals(IUserProfileManager.PUBLIC_PROFILE_FILTER_KEY)) { - return "publicprofile"; - } else { - throw new RuntimeException("Key '" + metadataFieldKey + "' not recognized"); - } + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } } 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..a2130d5b70 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,12 +1,13 @@ package com.agiletec.aps.system.common; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.when; import org.apache.commons.dbcp2.BasicDataSource; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -51,8 +52,25 @@ 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"; + 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); + when(dataSource.getDriverClassName()).thenReturn(driverClassName); FieldSearchFilter filter = new FieldSearchFilter(); filter.setLimit(1); 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..907e09f430 --- /dev/null +++ b/engine/src/test/java/com/agiletec/aps/system/common/SearcherDaoQueryShapeTest.java @@ -0,0 +1,530 @@ +/* + * 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.assertThrows; +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.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.guifragment.GuiFragmentDAO; +import org.entando.entando.aps.system.services.oauth2.OAuthConsumerDAO; +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) + .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)); + } + + // ---------------------------------------------------------------- the field whitelist + + /** + * A filter key becomes a column name by concatenation, so it is checked against the columns the + * searcher accepts. Values are bound as parameters and were never the exposure; keys are. + * + *

The REST layer validates keys against a DTO's fields, but that is a guarantee made far from + * here and absent for every non-REST caller - so the searcher does not rely on it.

+ */ + @Test + void aFilterKeyThatIsNotAColumn_neverReachesTheDriver() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + FieldSearchFilter[] filters = {new FieldSearchFilter<>("descr) OR 1=1 --", "x", true)}; + + assertThrows(RuntimeException.class, () -> dao.searchGroups(filters)); + assertTrue(this.capture.getQueries().isEmpty(), + () -> "a query was still handed to the driver: " + this.capture.getQueries()); + } + + /** The mirror image: a key that does name a column is untouched. */ + @Test + void aFilterKeyThatIsAColumn_isAccepted() { + GroupDAO dao = this.capture.wire(new GroupDAO(), DERBY); + + dao.searchGroups(new FieldSearchFilter[]{descriptionLike()}); + + assertTrue(SqlShape.normalize(this.capture.single()).contains("UPPER(authgroups.descr)"), + this.capture.single()); + } + + /** + * The allowlist is matched without regard to case, because SQL identifiers are folded by every + * database the engine supports - ORDER BY guifragment.pluginCode and + * ...plugincode are the same column on Derby, PostgreSQL, MySQL and Oracle. + * + *

The keys reaching the DAO are DTO field names, which are camelCase: pluginCode is + * an inherited GuiFragmentDtoSmall field, it passes the REST validator, and + * GuiFragmentService forwards it without remapping. Matching it exactly would refuse a + * query that works.

+ */ + @Test + void aFilterKeyDifferingOnlyByCase_isAccepted() { + GuiFragmentDAO dao = this.capture.wire(new GuiFragmentDAO(), DERBY); + + dao.searchGuiFragments(new FieldSearchFilter[]{sortOnly("pluginCode")}); + + assertTrue(SqlShape.normalize(this.capture.single()).contains("ORDER BY guifragment.plugincode"), + this.capture.single()); + } + + /** + * And what is concatenated is the column as the searcher declares it, not as the caller spelled it, + * so caller-supplied text does not reach the query even on the accepting path. + */ + @Test + void anAcceptedKey_isEmittedInTheSearchersOwnSpelling() { + OAuthConsumerDAO dao = this.capture.wire(new OAuthConsumerDAO(), DERBY); + + dao.getConsumerKeys(new FieldSearchFilter[]{sortOnly("issuedDate")}); + + String query = SqlShape.normalize(this.capture.single()); + assertTrue(query.contains("issueddate"), query); + assertFalse(query.contains("issuedDate"), query); + } + + /** Case folding is not a way past the allowlist: an unknown key is still refused. */ + @Test + void aFilterKeyThatIsNoColumnInAnyCase_isStillRefused() { + GuiFragmentDAO dao = this.capture.wire(new GuiFragmentDAO(), DERBY); + FieldSearchFilter[] filters = {sortOnly("PLUGINCODE) OR 1=1 --")}; + + assertThrows(RuntimeException.class, () -> dao.searchGuiFragments(filters)); + assertTrue(this.capture.getQueries().isEmpty(), + () -> "a query was still handed to the driver: " + this.capture.getQueries()); + } + + /** + * A searcher whose keys are not its column names declares that as an alias, and the alias resolves + * to the column. typeCode is the key every entity manager uses; on profiles the column + * behind it is profiletype. + * + *

This is the path the three entity searchers used to walk through an if/else chain of their own, + * each responsible for rejecting the unknown key. Declaring the mapping as data is what let that + * check move into {@link AbstractSearcherDAO} for every searcher at once.

+ */ + @Test + void anAliasedKey_resolvesToItsColumn() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + + dao.searchId(new EntitySearchFilter[]{ + ordered(new EntitySearchFilter<>(IEntityManager.ENTITY_TYPE_CODE_FILTER_KEY, false), + FieldSearchFilter.ASC_ORDER)}); + + String query = SqlShape.normalize(this.capture.single()); + assertTrue(query.contains("authuserprofiles.profiletype"), query); + assertFalse(query.contains("typeCode"), query); + } + + /** An alias is the only way in: the column it hides is not itself a key. */ + @Test + void theColumnBehindAnAlias_isNotAKeyOfItsOwn() { + ProbeProfileSearcherDAO dao = this.capture.wire(new ProbeProfileSearcherDAO(), DERBY); + EntitySearchFilter[] filters = {ordered(new EntitySearchFilter<>("profiletype", false), + FieldSearchFilter.ASC_ORDER)}; + + assertThrows(RuntimeException.class, () -> dao.searchId(filters)); + assertTrue(this.capture.getQueries().isEmpty(), + () -> "a query was still handed to the driver: " + this.capture.getQueries()); + } + + // ---------------------------------------------------------------- fixtures + + private static FieldSearchFilter sortOnly(String key) { + FieldSearchFilter filter = new FieldSearchFilter<>(key); + filter.setSortOnly(true); + filter.setOrder(FieldSearchFilter.ASC_ORDER); + return filter; + } + + 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..fe9243e397 --- /dev/null +++ b/engine/src/test/java/com/agiletec/aps/system/common/SqlShape.java @@ -0,0 +1,198 @@ +/* + * 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 java.util.regex.Pattern; +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 "}; + /** A trailing sort direction. Possessive so a run of spaces cannot backtrack. */ + private static final Pattern SORT_DIRECTION = Pattern.compile("(?i)\\s++(ASC|DESC)$"); + + 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(SORT_DIRECTION.matcher(term.trim()).replaceAll("")); + } + 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/engine/src/test/java/org/entando/entando/web/api/oauth2/ApiConsumerControllerIntegrationTest.java b/engine/src/test/java/org/entando/entando/web/api/oauth2/ApiConsumerControllerIntegrationTest.java index 38b3847a5a..e4b6712277 100644 --- a/engine/src/test/java/org/entando/entando/web/api/oauth2/ApiConsumerControllerIntegrationTest.java +++ b/engine/src/test/java/org/entando/entando/web/api/oauth2/ApiConsumerControllerIntegrationTest.java @@ -257,6 +257,20 @@ private Date getDate(String date) { return DateConverter.parseDate(date, SystemConstants.API_DATE_FORMAT); } + /** + * issuedDate is an ApiConsumer field, so the REST validator accepts it as + * a sort key, and reMapFilterKeys forwards it unchanged - only key is + * remapped. The searcher's allowlist holds the column, issueddate, so it has to match + * the key without regard to case or this endpoint refuses a sort it advertises. + */ + @Test + void shouldSortOnAFieldWhoseColumnDiffersOnlyByCase() throws Exception { + authRequest(get(BASE_URL).param("sort", "issuedDate")) + .andExpect(status().isOk()); + authRequest(get(BASE_URL).param("sort", "expirationDate").param("direction", "DESC")) + .andExpect(status().isOk()); + } + private ResultActions authRequest(MockHttpServletRequestBuilder requestBuilder) throws Exception { return mockMvc.perform(requestBuilder .header("Authorization", "Bearer " + accessToken) diff --git a/engine/src/test/java/org/entando/entando/web/guifragment/GuiFragmentControllerIntegrationTest.java b/engine/src/test/java/org/entando/entando/web/guifragment/GuiFragmentControllerIntegrationTest.java index c5f93ce8fe..52b51e1be7 100644 --- a/engine/src/test/java/org/entando/entando/web/guifragment/GuiFragmentControllerIntegrationTest.java +++ b/engine/src/test/java/org/entando/entando/web/guifragment/GuiFragmentControllerIntegrationTest.java @@ -82,6 +82,23 @@ void testGetFragments_1() throws Exception { testCors("/fragments"); } + /** + * pluginCode is an inherited GuiFragmentDtoSmall field, so the REST + * validator accepts it as a sort key, and GuiFragmentService passes the filters to the + * searcher without remapping. The searcher's allowlist holds the column, plugincode, + * so it has to match the key without regard to case or this endpoint refuses a sort it advertises. + */ + @Test + void shouldSortOnAFieldWhoseColumnDiffersOnlyByCase() throws Exception { + String accessToken = getAccessToken(); + mockMvc.perform(get("/fragments").param("sort", "pluginCode") + .header("Authorization", "Bearer " + accessToken)) + .andExpect(status().isOk()); + mockMvc.perform(get("/fragments").param("sort", "pluginCode").param("direction", "DESC") + .header("Authorization", "Bearer " + accessToken)) + .andExpect(status().isOk()); + } + @Test void testGetFragments_2() throws Exception { String accessToken = getAccessToken(); 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/seo-plugin/src/main/java/org/entando/entando/plugins/jpseo/aps/system/services/mapping/SeoMappingDAO.java b/seo-plugin/src/main/java/org/entando/entando/plugins/jpseo/aps/system/services/mapping/SeoMappingDAO.java index 0cf13c2edd..64c01fb5cc 100644 --- a/seo-plugin/src/main/java/org/entando/entando/plugins/jpseo/aps/system/services/mapping/SeoMappingDAO.java +++ b/seo-plugin/src/main/java/org/entando/entando/plugins/jpseo/aps/system/services/mapping/SeoMappingDAO.java @@ -36,6 +36,7 @@ import com.agiletec.aps.system.common.AbstractSearcherDAO; import com.agiletec.aps.system.common.FieldSearchFilter; +import com.agiletec.aps.system.common.SearchableFields; import org.entando.entando.ent.exception.EntException; /** @@ -44,6 +45,13 @@ public class SeoMappingDAO extends AbstractSearcherDAO implements ISeoMappingDAO { private static final EntLogger _logger = EntLogFactory.getSanitizedLogger(SeoMappingDAO.class); + + /** The columns of jpseo_friendlycode a search key may name. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "friendlycode", + "pagecode", + "contentid", + "langcode"); private static final String TABLE_NAME = "jpseo_friendlycode"; @@ -169,8 +177,8 @@ public List searchFriendlyCode(FieldSearchFilter[] filters) { } @Override - protected String getTableFieldName(String metadataFieldKey) { - return metadataFieldKey; + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } @Override 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..6325ec3eb9 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 @@ -21,6 +21,7 @@ */ package com.agiletec.plugins.jpwebdynamicform.aps.system.services.message; +import com.agiletec.aps.system.common.SearchableFields; import com.agiletec.aps.system.common.entity.AbstractEntitySearcherDAO; import com.agiletec.aps.system.common.entity.IEntityManager; import com.agiletec.aps.system.common.entity.model.ApsEntityRecord; @@ -40,12 +41,22 @@ */ public class MessageSearcherDAO extends AbstractEntitySearcherDAO implements IMessageSearcherDAO { + private static final String MESSAGEID = "messageid"; + private static final String MESSAGETYPE = "messagetype"; + + /** The search keys this searcher accepts. */ + private static final SearchableFields SEARCHABLE_FIELDS = SearchableFields.columns( + "username") + .alias(IEntityManager.ENTITY_ID_FILTER_KEY, MESSAGEID) + .alias(IEntityManager.ENTITY_TYPE_CODE_FILTER_KEY, MESSAGETYPE) + .alias(IMessageManager.CREATION_DATE_FILTER_KEY, "creationdate"); + @Override protected ApsEntityRecord createRecord(ResultSet result) throws Throwable { MessageRecordVO record = new MessageRecordVO(); - record.setId(result.getString("messageid")); + record.setId(result.getString(MESSAGEID)); record.setXml(result.getString("messagexml")); - record.setTypeCode(result.getString("messagetype")); + record.setTypeCode(result.getString(MESSAGETYPE)); record.setUsername(result.getString("username")); record.setLangCode(result.getString("langcode")); record.setCreationDate(result.getTimestamp("creationdate")); @@ -95,7 +106,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(); } @@ -120,12 +132,12 @@ protected String getEntityMasterTableName() { @Override protected String getEntityMasterTableIdFieldName() { - return "messageid"; + return MESSAGEID; } @Override protected String getEntityMasterTableIdTypeFieldName() { - return "messagetype"; + return MESSAGETYPE; } @Override @@ -135,7 +147,7 @@ protected String getEntitySearchTableName() { @Override protected String getEntitySearchTableIdFieldName() { - return "messageid"; + return MESSAGEID; } @Override @@ -145,20 +157,12 @@ protected String getEntityAttributeRoleTableName() { @Override protected String getEntityAttributeRoleTableIdFieldName() { - return "messageid"; + return MESSAGEID; } @Override - protected String getTableFieldName(String metadataFieldKey) { - if (metadataFieldKey.equals(IEntityManager.ENTITY_ID_FILTER_KEY)) { - return this.getEntityMasterTableIdFieldName(); - } else if (metadataFieldKey.equals(IEntityManager.ENTITY_TYPE_CODE_FILTER_KEY)) { - return this.getEntityMasterTableIdTypeFieldName(); - } else if (metadataFieldKey.equals(IMessageManager.USERNAME_FILTER_KEY)) { - return "username"; - } else if (metadataFieldKey.equals(IMessageManager.CREATION_DATE_FILTER_KEY)) { - return "creationdate"; - } else throw new RuntimeException("Chiave di ricerca '" + metadataFieldKey + "' non riconosciuta"); + protected SearchableFields getSearchableFields() { + return SEARCHABLE_FIELDS; } }