fix: update preference job to handle compound unique key with group_id #4078
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4078 +/- ##
========================================
Coverage 11.81% 11.81%
========================================
Files 115 115
Lines 15482 15482
Branches 319 319
========================================
Hits 1829 1829
Misses 13653 13653 |
|
Auth-service changes in this PR available for preview here |
|
Warning Rate limit exceeded@Baalmart has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/auth-service/bin/jobs/preferences-update-job.js (1)
124-124: Handle errors during preference creation more robustlyCurrently, errors during the creation of preferences are logged but not handled further. Consider implementing additional error handling, such as retries or notifications, to ensure that critical operations are not silently failing.
You might refactor the error handling like this:
await PreferenceModel("airqo") .create(defaultPreferenceWithGroupId) .catch((error) => { logger.error( `🐛🐛 Failed to create preference for user ${userIdStr}: ${stringify( error )}` ); + // Additional error handling logic here });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/bin/jobs/preferences-update-job.js(5 hunks)
🔇 Additional comments (3)
src/auth-service/bin/jobs/preferences-update-job.js (3)
145-146: Confirm that defaults are set in your Mongoose schema
Using setDefaultsOnInsert: true with upsert: true ensures defaults are applied when a new document is inserted. Verify that your PreferenceModel schema has default values defined for the necessary fields.
3-3: Ensure mongoose is included in your dependencies
The import statement for mongoose has been added. Please verify that mongoose is listed as a dependency in your package.json to prevent any runtime issues.
Run the following script to check if mongoose is declared in your package.json dependencies:
✅ Verification successful
mongoose dependency is properly configured
The mongoose package is already included in the auth-service dependencies with version ^5.12.14, so no additional action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that mongoose is listed as a dependency in package.json
cat package.json | jq '.dependencies.mongoose'
Length of output: 91
Script:
#!/bin/bash
# Let's first find the package.json files in the repository
fd package.json
# Then let's check for mongoose in all package.json files
fd package.json --exec cat {} \; | grep -i mongoose
Length of output: 816
Script:
#!/bin/bash
# Let's specifically check the auth-service package.json since that's where our file is located
cat src/auth-service/package.json | jq '.dependencies.mongoose'
Length of output: 74
97-99: Confirm the data type consistency of group_id in the query
When querying the PreferenceModel, ensure that the group_id field's data type in the query matches the type stored in the database. If group_id is stored as a string in the database, you might need to adjust the query accordingly.
Run the following script to verify the data type of group_id in your database:
✅ Verification successful
Let me analyze the schema definitions to determine the data type of group_id. Since we don't have direct database access, examining the model definitions will help us understand the expected data type.
I need to check the specific schema definition for the Preference model to verify the data type of group_id field.
Data type consistency for group_id is correctly implemented
The schema definition shows that group_id is properly defined as ObjectId type and references the "group" collection:
group_id: {
type: ObjectId,
ref: "group",
default: mongoose.Types.ObjectId(constants.DEFAULT_GROUP),
}The query in the code is using defaultGroupId which would be an ObjectId, making it consistent with the schema definition. The data type matches the model's expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the data type of 'group_id' in the 'preferences' collection
# Replace 'yourDatabase' and 'preferences' with the actual database and collection names
mongo yourDatabase --eval 'db.preferences.findOne({}, {group_id: 1})'
# Expected outcome: The output shows the data type of 'group_id'
Length of output: 167
Script:
#!/bin/bash
# Let's check the schema definition and model files to understand the data type
# First, let's find relevant model files
fd -t f "preference" -X grep -l "group_id"
# Then, let's examine the schema definition
ast-grep --pattern 'group_id: {
$$$
}'
# Also search for mongoose schema definitions
rg -A 5 "new (mongoose\.Schema|Schema)\("
Length of output: 65688
Script:
#!/bin/bash
# Let's examine the Preference model definition specifically
cat ./src/auth-service/models/Preference.js
Length of output: 16233
| const defaultPreferenceWithGroupId = { | ||
| ...defaultPreference, | ||
| user_id: user._id, | ||
| group_id: defaultGroupId, | ||
| selected_sites: selectedSites, | ||
| }; |
There was a problem hiding this comment.
Ensure all required fields are included in defaultPreferenceWithGroupId
While constructing defaultPreferenceWithGroupId, make sure that all required fields defined in the PreferenceModel schema are included. This prevents validation errors during creation or update operations.
Review the PreferenceModel schema and update defaultPreferenceWithGroupId if any required fields are missing.
| const defaultGroupId = mongoose.Types.ObjectId("64f54e4621d9b90013925a08"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the defaultGroupId
Hardcoding the group_id can lead to maintenance challenges and reduced flexibility across different environments. Consider retrieving the defaultGroupId from a configuration file or environment variable to enhance configurability.
Apply this diff to refactor:
-const defaultGroupId = mongoose.Types.ObjectId("64f54e4621d9b90013925a08");
+const defaultGroupId = mongoose.Types.ObjectId(process.env.DEFAULT_GROUP_ID);Ensure that DEFAULT_GROUP_ID is defined in your environment variables for all deployment environments.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/auth-service/bin/jobs/preferences-update-job.js (1)
114-119: Avoid potential duplication of 'group_id' in preference objectsSince
defaultPreferencealready includes agroup_id, adding it again may cause confusion. Consider removinggroup_idfromdefaultPreferenceor ensure that it is intentionally overwritten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/bin/jobs/preferences-update-job.js(5 hunks)
🔇 Additional comments (5)
src/auth-service/bin/jobs/preferences-update-job.js (5)
3-3: Importing 'mongoose' for ObjectId usage
Including mongoose here is appropriate for utilizing Types.ObjectId in your code.
96-99: Including 'group_id' in the query ensures accurate preference retrieval
Adding group_id to the query correctly handles the compound unique key, ensuring that preferences are fetched accurately.
145-146: Verify the necessity of 'setDefaultsOnInsert' option
When using $set with explicit values, setDefaultsOnInsert may not apply schema defaults as intended. Verify if this option is needed, as the provided values might override the defaults.
Consider reviewing Mongoose's documentation on the behavior of setDefaultsOnInsert when using $set.
124-124: Creating new preferences with comprehensive data
Using .create with defaultPreferenceWithGroupId ensures that new preferences are initialized with all required fields, including group_id.
137-138: Including 'group_id' in the update criteria ensures correct targeting
Adding group_id to the findOneAndUpdate query criteria ensures that the update targets the correct preference document, which is essential with the compound unique key.
| $set: defaultPreferenceWithGroupId, | ||
| }, |
There was a problem hiding this comment.
Prevent overwriting existing preference data
Using $set: defaultPreferenceWithGroupId might unintentionally overwrite existing preference fields. To avoid data loss, update only the necessary fields.
Modify the update operation to set specific fields:
-$set: defaultPreferenceWithGroupId,
+$set: {
+ selected_sites: selectedSites,
+ group_id: defaultGroupId,
+},This approach updates only the selected_sites and group_id fields, preserving other preference data.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -74,6 +75,9 @@ | |||
| return; | |||
| } | |||
|
|
|||
| // Specify the group_id you want to use | |||
| const defaultGroupId = mongoose.Types.ObjectId("64f54e4621d9b90013925a08"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the 'defaultGroupId' value
Consider moving the defaultGroupId to a configuration file or an environment variable. Hardcoding ObjectIds can make future maintenance challenging if the ID changes.
Apply this change to use a constant or environment variable:
-const defaultGroupId = mongoose.Types.ObjectId("64f54e4621d9b90013925a08");
+const defaultGroupId = mongoose.Types.ObjectId(process.env.DEFAULT_GROUP_ID || constants.DEFAULT_GROUP_ID);Ensure that process.env.DEFAULT_GROUP_ID or constants.DEFAULT_GROUP_ID provides the correct ObjectId value.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/auth-service/bin/jobs/preferences-update-job.js (2)
61-70: Consider adding type checking for defaultGroupId parameter.While the function is well-implemented, it might be good to add a type check for the defaultGroupId parameter to ensure it's a valid ObjectId.
const validateUserGroupMembership = (user, defaultGroupId) => { + if (!mongoose.Types.ObjectId.isValid(defaultGroupId)) { + return false; + } if (!user.group_roles || user.group_roles.length === 0) { return false; } return user.group_roles.some( (role) => role.group.toString() === defaultGroupId.toString() ); };
119-120: Consider adding error handling for ObjectId conversion.While the code works, it might be good to add try-catch for the ObjectId conversion as it could throw an error if the constant value is invalid.
- const defaultGroupId = mongoose.Types.ObjectId(constants.DEFAULT_GROUP); + let defaultGroupId; + try { + defaultGroupId = mongoose.Types.ObjectId(constants.DEFAULT_GROUP); + } catch (error) { + logger.error(`Invalid DEFAULT_GROUP value: ${constants.DEFAULT_GROUP}`); + return; + }Also applies to: 135-138
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/bin/jobs/preferences-update-job.js(7 hunks)
🔇 Additional comments (6)
src/auth-service/bin/jobs/preferences-update-job.js (6)
3-3: LGTM! Good practice using constants for default values.
The mongoose import and direct use of constants in defaultPreference improve code maintainability and consistency.
Also applies to: 55-58
16-38: Well-structured validation function with clear error handling.
The validateDefaultValues function provides a robust check for critical configuration values. The error message clearly identifies missing values, making troubleshooting easier.
102-105: Good practice implementing early validation.
Early validation check prevents unnecessary processing and database operations if critical values are missing.
143-148: Well-implemented compound query for preferences.
The query correctly uses both user_id and group_id, aligning with the compound unique key requirement.
161-167: Well-structured update logic with proper MongoDB options.
The update logic correctly handles the compound unique key and uses appropriate MongoDB options for atomic operations. The use of setDefaultsOnInsert is particularly good for maintaining data consistency.
Also applies to: 185-195
220-221: Clean module exports.
Good practice exporting the updatePreferences function, making it available for testing and reuse.
|
Auth-service changes in this PR available for preview here |
|
Auth-service changes in this PR available for preview here |
Description
Recent changes to the user preference schema introduced a compound unique index using
user_idandgroup_id. The existing preference update job was failing for new users due to duplicate key errors when attempting to create or update preferences.The original implementation did not account for the new unique key constraint, which requires both
user_idandgroup_idto be unique together. This led to MongoDB duplicate key errors when trying to create or update preferences for users.Refactored the preference update job to explicitly handle the new unique key constraint
Added explicit
group_idfiltering and assignment during preference updatesImplemented robust upsert logic to handle preference creation and updates
Maintained existing business logic for default preference generation
Improved error handling and logging for preference update processes
Modified
updatePreferencesfunction to use a specificgroup_idAdded
upsertandsetDefaultsOnInsertoptions to handle document creationEnsured consistency in preference creation across different user batches
Preserved existing default preference generation logic
Changes Made
updatePreferencesfunction to explicitly handlegroup_idin preference queriesdefaultGroupIdto ensure consistent group assignment during preference updatesuser_idandgroup_idunique keyupsertandsetDefaultsOnInsertoptions to handle edge cases in preference document creationuser_idandgroup_idgroup_idTesting
Affected Services
API Documentation Updated?
Additional Notes
This modification should resolve the duplicate key error by explicitly specifying the
group_idin both the query and the document creation/update process. The job will now create or update preferences with the specific group ID, avoiding duplicate key conflicts.Summary by CodeRabbit
New Features
Bug Fixes