From 97e1db17cce875146408648935519b9e91394094 Mon Sep 17 00:00:00 2001 From: subratadeypappu Date: Mon, 11 Dec 2023 19:27:31 +0600 Subject: [PATCH] chore: Add code split for query criteria (#29517) (#29520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Pull Request Template > > Use this template to quickly create a well written pull request. Delete all quotes before creating the pull request. > ## Description > Add a TL;DR when description is extra long (helps content team) > > Please include a summary of the changes and which issue has been fixed. Please also include relevant motivation > and context. List any dependencies that are required for this change > > Links to Notion, Figma or any other documents that might be relevant to the PR > > #### PR fixes following issue(s) Fixes # (issue number) > if no issue exists, please create an issue and ask the maintainers about this first > > #### Media > A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video > > #### Type of change - Chore (housekeeping or task changes that don't impact user perception) ## Testing > #### How Has This Been Tested? > Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration. > Delete anything that is not relevant - [ ] Manual - [ ] JUnit - [ ] Jest - [ ] Cypress > > #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed ## Summary by CodeRabbit - **Refactor** - Improved the efficiency and maintainability of the codebase by refactoring the criteria building logic for database queries related to actions and action collections. --- ...ustomActionCollectionRepositoryCEImpl.java | 36 ++++++---- .../ce/CustomNewActionRepositoryCEImpl.java | 69 +++++++++++++------ 2 files changed, 71 insertions(+), 34 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java index 13c40b4c45..6be508dcbd 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java @@ -61,10 +61,7 @@ public class CustomActionCollectionRepositoryCEImpl extends BaseAppsmithReposito return queryAll(List.of(applicationCriteria), aclPermission, sort); } - @Override - public Flux findByApplicationIdAndViewMode( - String applicationId, boolean viewMode, AclPermission aclPermission) { - + protected List getCriteriaForFindByApplicationIdAndViewMode(String applicationId, boolean viewMode) { List criteria = new ArrayList<>(); Criteria applicationCriterion = where(fieldName(QActionCollection.actionCollection.applicationId)) @@ -79,18 +76,20 @@ public class CustomActionCollectionRepositoryCEImpl extends BaseAppsmithReposito .is(null); criteria.add(deletedCriterion); } + return criteria; + } + + @Override + public Flux findByApplicationIdAndViewMode( + String applicationId, boolean viewMode, AclPermission aclPermission) { + + List criteria = this.getCriteriaForFindByApplicationIdAndViewMode(applicationId, viewMode); return queryAll(criteria, aclPermission); } - @Override - public Flux findAllActionCollectionsByNameDefaultPageIdsViewModeAndBranch( - String name, - List pageIds, - boolean viewMode, - String branchName, - AclPermission aclPermission, - Sort sort) { + protected List getCriteriaForFindAllActionCollectionsByNameDefaultPageIdsViewModeAndBranch( + String branchName, boolean viewMode, String name, List pageIds) { /** * TODO : This function is called by get(params) to get all actions by params and hence * only covers criteria of few fields like page id, name, etc. Make this generic to cover @@ -150,6 +149,19 @@ public class CustomActionCollectionRepositoryCEImpl extends BaseAppsmithReposito .is(null); criteriaList.add(deletedCriteria); } + return criteriaList; + } + + @Override + public Flux findAllActionCollectionsByNameDefaultPageIdsViewModeAndBranch( + String name, + List pageIds, + boolean viewMode, + String branchName, + AclPermission aclPermission, + Sort sort) { + List criteriaList = this.getCriteriaForFindAllActionCollectionsByNameDefaultPageIdsViewModeAndBranch( + branchName, viewMode, name, pageIds); return queryAll(criteriaList, aclPermission, sort); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java index ad9fba1887..edcdbdcb77 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java @@ -68,16 +68,14 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< @Override public Flux findByApplicationId(String applicationId, AclPermission aclPermission) { - Criteria applicationIdCriteria = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationIdCriteria = this.getCriterionForFindByApplicationId(applicationId); return queryAll(List.of(applicationIdCriteria), aclPermission); } @Override public Flux findByApplicationId( String applicationId, Optional aclPermission, Optional sort) { - Criteria applicationIdCriteria = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationIdCriteria = this.getCriterionForFindByApplicationId(applicationId); return queryAll(List.of(applicationIdCriteria), aclPermission, sort); } @@ -195,6 +193,14 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< @Override public Flux findAllActionsByNameAndPageIdsAndViewMode( String name, List pageIds, Boolean viewMode, AclPermission aclPermission, Sort sort) { + List criteriaList = + this.getCriteriaForFindAllActionsByNameAndPageIdsAndViewMode(name, pageIds, viewMode); + + return queryAll(criteriaList, aclPermission, sort); + } + + protected List getCriteriaForFindAllActionsByNameAndPageIdsAndViewMode( + String name, List pageIds, Boolean viewMode) { /** * TODO : This function is called by get(params) to get all actions by params and hence * only covers criteria of few fields like page id, name, etc. Make this generic to cover @@ -243,8 +249,7 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< .is(null); criteriaList.add(deletedCriteria); } - - return queryAll(criteriaList, aclPermission, sort); + return criteriaList; } @Override @@ -339,20 +344,30 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< @Override public Flux findByApplicationId(String applicationId, AclPermission aclPermission, Sort sort) { - Criteria applicationCriteria = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationCriteria = this.getCriterionForFindByApplicationId(applicationId); return queryAll(List.of(applicationCriteria), aclPermission, sort); } + protected Criteria getCriterionForFindByApplicationId(String applicationId) { + Criteria applicationCriteria = + where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + return applicationCriteria; + } + @Override public Flux findByApplicationIdAndViewMode( String applicationId, Boolean viewMode, AclPermission aclPermission) { + List criteria = this.getCriteriaForFindByApplicationIdAndViewMode(applicationId, viewMode); + + return queryAll(criteria, aclPermission); + } + + protected List getCriteriaForFindByApplicationIdAndViewMode(String applicationId, Boolean viewMode) { List criteria = new ArrayList<>(); - Criteria applicationCriterion = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationCriterion = this.getCriterionForFindByApplicationId(applicationId); criteria.add(applicationCriterion); if (Boolean.FALSE.equals(viewMode)) { @@ -363,8 +378,7 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< .is(null); criteria.add(deletedCriterion); } - - return queryAll(criteria, aclPermission); + return criteria; } @Override @@ -434,10 +448,17 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< @Override public Flux findNonJsActionsByApplicationIdAndViewMode( String applicationId, Boolean viewMode, AclPermission aclPermission) { + List criteria = + this.getCriteriaForFindNonJsActionsByApplicationIdAndViewMode(applicationId, viewMode); + + return queryAll(criteria, aclPermission); + } + + protected List getCriteriaForFindNonJsActionsByApplicationIdAndViewMode( + String applicationId, Boolean viewMode) { List criteria = new ArrayList<>(); - Criteria applicationCriterion = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationCriterion = this.getCriterionForFindByApplicationId(applicationId); criteria.add(applicationCriterion); Criteria nonJsTypeCriteria = @@ -452,13 +473,20 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< .is(null); criteria.add(deletedCriterion); } - - return queryAll(criteria, aclPermission); + return criteria; } @Override public Flux findAllNonJsActionsByNameAndPageIdsAndViewMode( String name, List pageIds, Boolean viewMode, AclPermission aclPermission, Sort sort) { + List criteriaList = + this.getCriteriaForFindAllNonJsActionsByNameAndPageIdsAndViewMode(name, pageIds, viewMode); + + return queryAll(criteriaList, aclPermission, sort); + } + + protected List getCriteriaForFindAllNonJsActionsByNameAndPageIdsAndViewMode( + String name, List pageIds, Boolean viewMode) { List criteriaList = new ArrayList<>(); Criteria nonJsTypeCriteria = @@ -507,8 +535,7 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< .is(null); criteriaList.add(deletedCriteria); } - - return queryAll(criteriaList, aclPermission, sort); + return criteriaList; } /** @@ -578,8 +605,7 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< @Override public Mono> publishActions(String applicationId, AclPermission permission) { - Criteria applicationIdCriteria = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationIdCriteria = this.getCriterionForFindByApplicationId(applicationId); Mono> permissionGroupsMono = getCurrentUserPermissionGroupsIfRequired(Optional.ofNullable(permission)); @@ -615,8 +641,7 @@ public class CustomNewActionRepositoryCEImpl extends BaseAppsmithRepositoryImpl< @Override public Mono archiveDeletedUnpublishedActions(String applicationId, AclPermission permission) { - Criteria applicationIdCriteria = - where(fieldName(QNewAction.newAction.applicationId)).is(applicationId); + Criteria applicationIdCriteria = this.getCriterionForFindByApplicationId(applicationId); String unpublishedDeletedAtFieldName = String.format( "%s.%s", fieldName(QNewAction.newAction.unpublishedAction),