chore: Adding migration to make user email unique in an organization#39567
chore: Adding migration to make user email unique in an organization#39567trishaanand merged 1 commit intoreleasefrom
Conversation
WalkthroughA new migration class, Changes
Sequence Diagram(s)sequenceDiagram
participant M as Migration067_UpdateUserEmailIndex
participant DB as MongoTemplate
participant L as Logger
M->>L: Log start of migration
M->>DB: dropIndexIfExists (index on email)
DB-->>M: Index dropped/confirmation
M->>DB: ensureIndexes (create unique compound index on email & organizationId)
DB-->>M: Index created/confirmation
alt Exception occurs
M->>L: Log exception
M->>M: Rethrow exception
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration067_UpdateUserEmailIndex.java (1)
39-42: Consider more specific error handling.The current error handling simply logs and rethrows any exception. Consider handling specific MongoDB exceptions separately to provide more helpful error messages and recovery options.
} catch (Exception e) { log.error("Error updating user email index", e); + if (e instanceof org.springframework.dao.DuplicateKeyException) { + log.error("Duplicate keys found. This might indicate existing users with the same email in the same organization."); + } else if (e instanceof org.springframework.dao.DataAccessResourceFailureException) { + log.error("Database connection issue during migration. Please verify MongoDB connection."); + } throw e; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration067_UpdateUserEmailIndex.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration067_UpdateUserEmailIndex.java (3)
30-36: LGTM on compound index creation.The compound index creation with uniqueness constraint on both email and organizationId is properly implemented, which will ensure emails are unique within an organization rather than globally.
1-45:Details
❓ Verification inconclusive
Verify data integrity after migration.
Since this migration changes from a global email uniqueness to org-scoped uniqueness, verify that there are no unexpected data integrity issues that could arise during the migration if duplicate emails already exist across organizations.
🏁 Script executed:
#!/bin/bash # Script to check for potential duplicate emails across organizations echo "Checking for potential user email conflicts across organizations..." # Sample MongoDB query to find any users with same email but different organizations # In a real environment, you would use appropriate MongoDB connection details cat <<'EOF' > check_duplicates.js db.getCollection("users").aggregate([ { $group: { _id: "$email", count: { $sum: 1 }, organizations: { $addToSet: "$organizationId" } }}, { $match: { count: { $gt: 1 } } } ]) EOF echo "In a production environment, you would execute the above MongoDB query to identify potential conflicts." echo "If conflicts exist, they should be resolved before running this migration."Length of output: 526
Data Integrity Verification Reminder
Please ensure that the migration's shift from a global email uniqueness constraint to an organization-scoped uniqueness constraint does not lead to data integrity issues. Specifically, before deploying this change, verify that there are no duplicate emails across different organizations. The sample MongoDB query provided below illustrates how one might identify potential conflicts:
- Location:
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration067_UpdateUserEmailIndex.java- Action Items:
- Run the provided aggregation query in your production environment to check for any email conflicts.
- Resolve any detected duplicates prior to executing the migration.
If the verification script finds potential conflicts, address them to prevent runtime issues post-migration.
26-27:Details
❓ Verification inconclusive
Verify existing index before dropping.
Consider adding a check to verify the existing index structure before dropping it, to ensure data consistency.
🏁 Script executed:
#!/bin/bash # Check for current index structure in the codebase or related migrations rg -A 5 "new Index\(\).+on\(\"email\"" --type javaLength of output: 50
Action on Email Index Verification:
The current migration code drops the "email" index without first verifying its existing structure. Our initial search did not reveal any sign of a pre-drop check (e.g., no evidence of a correspondingnew Index(...)configuration), which raises a potential risk for inconsistent state if the index's structure has changed unexpectedly.Recommendations:
- Review the
dropIndexIfExistsMethod: Check if it already performs any implicit validation of the existing index structure.- Consider Index Structure Verification: If no internal checks are present, add a manual verification step (or MongoDB query) to confirm that the existing index matches the expected configuration before dropping it.
Please verify these points manually as the automated search was inconclusive.
| @RollbackExecution | ||
| public void rollbackExecution() {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty rollback method needs implementation.
The rollback method is currently empty. Implement a proper rollback strategy that would restore the original email index if the migration needs to be reversed.
@RollbackExecution
public void rollbackExecution() {}
+public void rollbackExecution(MongoTemplate mongoTemplate) {
+ log.info("Rolling back migration for user email index");
+ try {
+ // Drop the compound index
+ dropIndexIfExists(mongoTemplate, User.class, "email_organizationId_compound_index");
+
+ // Recreate the original email index
+ Index emailIndex = new Index()
+ .on("email", ASC)
+ .unique()
+ .named("email");
+
+ ensureIndexes(mongoTemplate, User.class, emailIndex);
+
+ log.info("Completed rollback of user email index migration");
+ } catch (Exception e) {
+ log.error("Error rolling back user email index migration", e);
+ throw e;
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
…ppsmithorg#39567) ## Description Chore migration to make emails unique only in an organization instead of entire instance. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test all ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13673657485> > Commit: a9c44d6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13673657485&attempt=1&selectiontype=test&testsstatus=failed&specsstatus=fail" target="_blank">Cypress dashboard</a>. > Tags: @tag.All > Spec: > The following are new failures, please fix them before merging the PR: <ol> > <li>cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts</ol> > <a href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master" target="_blank">List of identified flaky tests</a>. > <hr>Wed, 05 Mar 2025 11:24:33 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Enhanced backend processing of user email data associated with organizational information, leading to improved query performance and data consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Chore migration to make emails unique only in an organization instead of entire instance.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/test all
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13673657485
Commit: a9c44d6
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts
List of identified flaky tests.Wed, 05 Mar 2025 11:24:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit