From 0388f634a0aa3da3e5dab000fb64ec0b7949faec Mon Sep 17 00:00:00 2001 From: Abhijeet <41686026+abhvsn@users.noreply.github.com> Date: Wed, 18 Sep 2024 07:25:59 +0530 Subject: [PATCH] chore: Add empty policyMap when policies are empty or null to fix NPE (#36374) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description We have quite a lot of objects in the DB for which we expect the policies should be present but that's not the case. This is causing issues when we migrated to using `policyMap` where we didn't create empty map if the policies object is empty expecting all such entries are anyway not accessible to the user. We can query mongodb to find such entries: ``` db .collectionName .find({ policies: [], deletedAt: {$exists: false} }) ``` We expect collections like plugins, customJSlibs etc to have empty values for policies and hence policyMap was not migrated for this, but we are seeing the same pattern for other collections as well like newAction, workspace etc. which is why we are seeing NPEs. With this PR we intend to remove this NPE by adding a empty map to policyMap field. With prod dump this is taking 340sec so we need to plan accordingly. /test Sanity ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: c1573a68be0da4ee78ae2cb1ece8deb9ce6b8cb4 > Cypress dashboard. > Tags: `@tag.Sanity` > Spec: >
Tue, 17 Sep 2024 13:41:53 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Implemented a database migration to ensure all relevant documents have a defined policy map, improving data consistency. - **Bug Fixes** - Addressed issues where documents had null or missing policy maps by updating them to an empty policy map. --- ...tion062AddEmptyPolicyMapForNullValues.java | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration062AddEmptyPolicyMapForNullValues.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration062AddEmptyPolicyMapForNullValues.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration062AddEmptyPolicyMapForNullValues.java new file mode 100644 index 00000000000..51b33b244fe --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration062AddEmptyPolicyMapForNullValues.java @@ -0,0 +1,113 @@ +package com.appsmith.server.migrations.db.ce; + +import com.appsmith.external.helpers.Stopwatch; +import com.mongodb.client.result.UpdateResult; +import io.mongock.api.annotations.ChangeUnit; +import io.mongock.api.annotations.Execution; +import io.mongock.api.annotations.RollbackExecution; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.data.mongodb.core.ReactiveMongoTemplate; +import org.springframework.data.mongodb.core.query.Criteria; +import org.springframework.data.mongodb.core.query.Query; +import org.springframework.data.mongodb.core.query.Update; +import org.springframework.data.mongodb.core.query.UpdateDefinition; +import reactor.core.publisher.Mono; + +import java.util.HashMap; +import java.util.Set; + +import static com.appsmith.server.migrations.constants.FieldName.POLICY_MAP; +import static org.springframework.data.mongodb.core.query.Criteria.where; + +@RequiredArgsConstructor +@Slf4j +@ChangeUnit(order = "062", id = "add_empty_policyMap_for_null_entries") +public class Migration062AddEmptyPolicyMapForNullValues { + + private final ReactiveMongoTemplate mongoTemplate; + + private static final Set COLLECTION_NAMES = Set.of( + "actionCollection", + "application", + "applicationSnapshot", + "asset", + "collection", + "config", + "customJSLib", + "datasource", + "datasourceStorage", + "datasourceStorageStructure", + "emailVerificationToken", + "gitDeployKeys", + "newAction", + "newPage", + "passwordResetToken", + "permissionGroup", + "plugin", + "theme", + "user", + "userData", + "workspace", + "approvalRequest", + "auditLog", + "datasourceConfigurationStructure", + "environment", + "inviteUser", + "knowledgeStore", + "module", + "moduleInstance", + "package", + "role", + "userApiKey", + "userGroup", + "workflow"); + + @RollbackExecution + public void rollbackExecution() {} + + @Execution + public void execute() { + Stopwatch stopwatch = new Stopwatch("Migration062AddEmptyPolicyMapForNullValues"); + Mono.whenDelayError(COLLECTION_NAMES.stream() + .map(c -> addEmptyPolicyMapForNullEntries(mongoTemplate, c)) + .toList()) + .onErrorResume(error -> { + String errorPrefix = "Error while adding empty policyMap for null values"; + // As we are using Mono.whenDelayError, we expect multiple errors to be suppressed in a single error + if (error.getSuppressed().length > 0) { + for (Throwable suppressed : error.getSuppressed()) { + log.error(errorPrefix, suppressed); + } + } else { + log.error(errorPrefix, error); + } + return Mono.error(error); + }) + .block(); + stopwatch.stopAndLogTimeInMillis(); + } + + private static Mono addEmptyPolicyMapForNullEntries( + ReactiveMongoTemplate mongoTemplate, String collectionName) { + log.info("Adding empty policyMap for empty policies {}", collectionName); + + // Update policyMap for documents where policies is empty or null + Criteria policyMapNotExists = + new Criteria().orOperator(where(POLICY_MAP).exists(false)); + Criteria notDeletedCriteria = where("deletedAt").isNull(); + + final Query query = new Query().addCriteria(policyMapNotExists).addCriteria(notDeletedCriteria); + UpdateDefinition update = new Update().set(POLICY_MAP, new HashMap<>()); + final Mono convertToMap = mongoTemplate.updateMulti(query, update, collectionName); + + return convertToMap + .elapsed() + .doOnSuccess(it -> log.info( + "Migrated {} documents in {} in {}ms", + it.getT2().getModifiedCount(), + collectionName, + it.getT1())) + .then(); + } +}