From 632c8a7b69c8dfceec741108ba1afc7a55e6502b Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Mon, 12 Feb 2024 12:58:42 +0530 Subject: [PATCH] chore: use common function for building query (#31018) The query execution methods, `queryAllExecute`, `queryOneExecute` etc., aren't using the same function to build the `Query` object. This PR fixes it by using the common function. But the common function has a problem. Check this out, this is the current implementation: ```java Query query = new Query(); criterias.stream().forEach(criteria -> query.addCriteria(criteria)); if (aclPermission == null) { query.addCriteria(new Criteria().andOperator(notDeleted())); } else { query.addCriteria(new Criteria().andOperator(notDeleted(), userAcl(permissionGroups, aclPermission))); } if (!isEmpty(projectionFieldNames)) { projectionFieldNames.stream().forEach(fieldName -> query.fields().include(fieldName)); } return query; ``` Here, we use `addCriteria` to add each of the criteria items given to us, into the `query`. After that, we use `.andOperator` to add the not-deleted and permission checks. Looks good on the surface. Let's take an example. If the given criteria list has `fieldName = "abc"` as the condition, this will end up in the final query as (pseudo-code representation): ```javascript { fieldName: "abc", $and: { $and: { deleted: false or missing, deletedAt: null }, policies: { $elemMatch: permission check here, } } } ``` Perfectly working query. Now, what if the incoming criteria list is a little more complex, and has an `or` condition in it. This is what we end up with: ```javascript { $or: { field1: "val", field2: "val" }, $and: { $and: { deleted: false or missing, deletedAt: null }, policies: { $elemMatch: permission check here, } } } ``` We end up with a `$or` and `$and` next to each other. This doesn't make sense to MongoDB. The way the query is built in the `queryAllExecute` method previously, actually doesn't fall into this. That's the version we're changing the common method into now. This is what it looks like: ```java final ArrayList criteriaList = new ArrayList<>(criterias); criteriaList.add(notDeleted()); final Criteria permissionCriteria = userAcl(permissionGroups, aclPermission); if (permissionCriteria != null) { criteriaList.add(permissionCriteria); } final Query query = new Query(new Criteria().andOperator(criteriaList.toArray(new Criteria[0]))); if (!isEmpty(projectionFieldNames)) { query.fields().include(projectionFieldNames.toArray(new String[0])); } return query; ``` With this, the resulting query looks something like this: ```javascript { $and: { $or: { field1: "val", field2: "val" }, $and: { deleted: false or missing, deletedAt: null }, policies: { $elemMatch: permission check here, } } } ``` This isn't new code. This is how we've been building the query for `queryAll` today. By moving this to the common method, we have this resilient query building for `queryOne` and `queryFirst` as well. --- .../repositories/AppsmithRepository.java | 11 ---- .../ce/BaseAppsmithRepositoryCEImpl.java | 53 ++++--------------- 2 files changed, 11 insertions(+), 53 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java index ddfacd4506..63014c3895 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java @@ -5,10 +5,7 @@ import com.appsmith.server.acl.AclPermission; import com.appsmith.server.repositories.ce.params.QueryAllParams; import com.mongodb.bulk.BulkWriteResult; import com.mongodb.client.result.InsertManyResult; -import org.springframework.data.domain.Sort; -import org.springframework.data.mongodb.core.query.Criteria; import org.springframework.data.mongodb.core.query.Update; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import java.util.List; @@ -25,14 +22,6 @@ public interface AppsmithRepository { QueryAllParams queryBuilder(); - Flux queryAllWithStrictPermissionGroups( - List criterias, - Optional> includeFields, - Optional permission, - Sort sort, - int limit, - int skip); - Mono setUserPermissionsInObject(T obj, Set permissionGroups); Mono setUserPermissionsInObject(T obj); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java index 71cc5ac8bd..650f6dea29 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java @@ -313,62 +313,31 @@ public abstract class BaseAppsmithRepositoryCEImpl { List projectionFieldNames, Set permissionGroups, AclPermission aclPermission) { - Query query = new Query(); - criterias.stream().forEach(criteria -> query.addCriteria(criteria)); - if (aclPermission == null) { - query.addCriteria(new Criteria().andOperator(notDeleted())); - } else { - query.addCriteria(new Criteria().andOperator(notDeleted(), userAcl(permissionGroups, aclPermission))); + final ArrayList criteriaList = new ArrayList<>(criterias); + criteriaList.add(notDeleted()); + + final Criteria permissionCriteria = userAcl(permissionGroups, aclPermission); + if (permissionCriteria != null) { + criteriaList.add(permissionCriteria); } + final Query query = new Query(new Criteria().andOperator(criteriaList.toArray(new Criteria[0]))); + if (!isEmpty(projectionFieldNames)) { - projectionFieldNames.stream().forEach(fieldName -> query.fields().include(fieldName)); + query.fields().include(projectionFieldNames.toArray(new String[0])); } return query; } - public Flux queryAllWithStrictPermissionGroups( - List criterias, - Optional> includeFields, - Optional permission, - Sort sort, - int limit, - int skip) { - Mono> permissionGroupsMono = ReactiveSecurityContextHolder.getContext() - .map(ctx -> ctx.getAuthentication()) - .map(auth -> auth.getPrincipal()) - .flatMap(principal -> getStrictPermissionGroupsForUser((User) principal)); - return permissionGroupsMono.flatMapMany(permissionGroups -> queryBuilder() - .criteria(criterias) - .fields(includeFields.orElse(null)) - .permission(permission.orElse(null)) - .permissionGroups(permissionGroups) - .sort(sort) - .limit(limit) - .skip(skip) - .all()); - } - public QueryAllParams queryBuilder() { return new QueryAllParams<>(this); } public Flux queryAllExecute(QueryAllParams params) { return tryGetPermissionGroups(params).flatMapMany(permissionGroups -> { - final ArrayList criteriaList = new ArrayList<>(params.getCriteria()); - criteriaList.add(notDeleted()); - - final Criteria permissionCriteria = userAcl(permissionGroups, params.getPermission()); - if (permissionCriteria != null) { - criteriaList.add(permissionCriteria); - } - - final Query query = new Query(new Criteria().andOperator(criteriaList.toArray(new Criteria[0]))); - - if (!CollectionUtils.isEmpty(params.getFields())) { - query.fields().include(params.getFields().toArray(new String[0])); - } + final Query query = createQueryWithPermission( + params.getCriteria(), params.getFields(), permissionGroups, params.getPermission()); if (params.getSkip() > NO_SKIP) { query.skip(params.getSkip());