-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactoring the createValidateSelectedSitesField middleware function #3658
Conversation
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the validation logic in the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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 (
|
Auth-service changes in this PR available for preview here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #3658 +/- ##
========================================
Coverage 27.04% 27.04%
========================================
Files 146 146
Lines 21339 21339
Branches 274 274
========================================
Hits 5772 5772
Misses 15567 15567 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/auth-service/routes/v2/preferences.js (1)
Line range hint
119-214
: Add validation for 'selectedSites' in 'validateUniqueFieldsInSelectedSites'To avoid potential runtime errors when
selectedSites
is undefined or not an array, ensure that a validation check is in place. This will enhance the robustness of the middleware.Consider adding the following check at the start of the function:
const validateUniqueFieldsInSelectedSites = (req, res, next) => { const selectedSites = req.body.selected_sites; + if (!Array.isArray(selectedSites) || selectedSites.length === 0) { + return res.status(400).json({ + success: false, + message: "selected_sites must be a non-empty array", + }); + } // Create Sets to track unique values for each field const uniqueSiteIds = new Set(); // ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/auth-service/routes/v2/preferences.js (8 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/auth-service/routes/v2/preferences.js (3)
382-382
: Verify the use of 'allowId' parameter set to trueIn the calls to
createValidateSelectedSitesField
, theallowId
parameter is set totrue
, allowing the_id
field. Please confirm that this behavior is intended and consistent with the application requirements.Also applies to: 584-584, 787-787, 957-957
1188-1191
: Validation logic for 'selected_sites' conforms to requirementsThe use of
createValidateSelectedSitesField
with required fields["site_id", "search_name", "name"]
andallowId
set tofalse
appropriately restricts the_id
field. The implementation aligns with the intended validation rules.
1227-1227
: Clarify the use of empty 'requiredFields' arrayAn empty array is passed as
requiredFields
tocreateValidateSelectedSitesField
. Ensure that no required fields are necessary in this context and that the function handles this scenario correctly.
function createValidateSelectedSitesField(requiredFields, allowId = false) { | ||
return function (value) { | ||
// Edge case: Check if _id field is present | ||
if (!allowId && "_id" in value) { | ||
throw new Error("_id field is not allowed"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure 'value' is not null or undefined before checking '_id' field
To prevent potential runtime errors, consider adding a check to ensure that value
is not null or undefined before accessing its properties. This will safeguard against situations where value
might be missing.
Apply this diff to include the null check:
function createValidateSelectedSitesField(requiredFields, allowId = false) {
return function (value) {
+ if (!value) {
+ throw new Error("Value must not be null or undefined");
+ }
// Edge case: Check if _id field is present
if (!allowId && "_id" in value) {
throw new Error("_id field is not allowed");
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
function createValidateSelectedSitesField(requiredFields, allowId = false) { | |
return function (value) { | |
// Edge case: Check if _id field is present | |
if (!allowId && "_id" in value) { | |
throw new Error("_id field is not allowed"); | |
} | |
function createValidateSelectedSitesField(requiredFields, allowId = false) { | |
return function (value) { | |
if (!value) { | |
throw new Error("Value must not be null or undefined"); | |
} | |
// Edge case: Check if _id field is present | |
if (!allowId && "_id" in value) { | |
throw new Error("_id field is not allowed"); | |
} |
…any fields that are not present in the input objects
Auth-service changes in this PR available for preview here |
…lue is not null or undefined before accessing its properties
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/auth-service/routes/v2/preferences.js (10 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/auth-service/routes/v2/preferences.js (1)
1195-1198
: Review theallowId
parameter usageIn this instance,
allowId
is set tofalse
, but_id
is not included in the required fields. Ensure this is intentional and that_id
should indeed be disallowed in this context.If
_id
should be allowed, consider settingallowId
totrue
. If it should be required, include it in therequiredFields
array.
@@ -1214,7 +1231,7 @@ router.put( | |||
return ObjectId(value); | |||
}), | |||
body("selected_site") | |||
.custom(createValidateSelectedSitesField([])) | |||
.custom(createValidateSelectedSitesField([], false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify required fields when using createValidateSelectedSitesField
Currently, createValidateSelectedSitesField
is called with an empty array for requiredFields
. This may lead to unintended behavior since no fields are being validated as required. Please specify the required fields to ensure proper validation.
For example:
.custom(createValidateSelectedSitesField(
- [],
+ ['search_name', 'name'],
false
))
Committable suggestion was skipped due to low confidence.
// Check for duplicate site_id if it exists | ||
if (item.site_id !== undefined) { | ||
if (uniqueSiteIds.has(item.site_id)) { | ||
duplicateSiteIds.push(item.site_id); | ||
} else { | ||
uniqueSiteIds.add(item.site_id); | ||
} | ||
} | ||
|
||
// Check for duplicate search_name | ||
if (uniqueSearchNames.has(item.search_name)) { | ||
duplicateSearchNames.push(item.search_name); | ||
} else { | ||
uniqueSearchNames.add(item.search_name); | ||
// Check for duplicate search_name if it exists | ||
if (item.search_name !== undefined) { | ||
if (uniqueSearchNames.has(item.search_name)) { | ||
duplicateSearchNames.push(item.search_name); | ||
} else { | ||
uniqueSearchNames.add(item.search_name); | ||
} | ||
} | ||
|
||
// Check for duplicate name | ||
if (uniqueNames.has(item.name)) { | ||
duplicateNames.push(item.name); | ||
} else { | ||
uniqueNames.add(item.name); | ||
// Check for duplicate name if it exists | ||
if (item.name !== undefined) { | ||
if (uniqueNames.has(item.name)) { | ||
duplicateNames.push(item.name); | ||
} else { | ||
uniqueNames.add(item.name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor duplicate code in validateUniqueFieldsInSelectedSites
The function contains repetitive code blocks for checking duplicates of site_id
, search_name
, and name
. Consider refactoring this logic to reduce code duplication and enhance maintainability.
Here's a suggested refactor:
const validateUniqueFieldsInSelectedSites = (req, res, next) => {
const selectedSites = req.body.selected_sites;
// Fields to check for duplicates
+ const fieldsToCheck = ['site_id', 'search_name', 'name'];
+ const uniqueFields = {};
+ const duplicateFields = {};
+ // Initialize Sets for each field
+ fieldsToCheck.forEach((field) => {
+ uniqueFields[field] = new Set();
+ duplicateFields[field] = new Set();
+ });
selectedSites.forEach((item) => {
+ fieldsToCheck.forEach((field) => {
+ if (item[field] !== undefined) {
+ if (uniqueFields[field].has(item[field])) {
+ duplicateFields[field].add(item[field]);
+ } else {
+ uniqueFields[field].add(item[field]);
+ }
+ }
+ });
});
// Prepare error messages based on duplicates found
let errorMessage = "";
+ fieldsToCheck.forEach((field) => {
+ if (duplicateFields[field].size > 0) {
+ errorMessage +=
+ `Duplicate ${field}s found: ${[...duplicateFields[field]].join(", ")}. `;
+ }
+ });
// If any duplicates were found, respond with an error
if (errorMessage) {
return res.status(400).json({
success: false,
message: errorMessage.trim(),
});
}
next();
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// Check for duplicate site_id if it exists | |
if (item.site_id !== undefined) { | |
if (uniqueSiteIds.has(item.site_id)) { | |
duplicateSiteIds.push(item.site_id); | |
} else { | |
uniqueSiteIds.add(item.site_id); | |
} | |
} | |
// Check for duplicate search_name | |
if (uniqueSearchNames.has(item.search_name)) { | |
duplicateSearchNames.push(item.search_name); | |
} else { | |
uniqueSearchNames.add(item.search_name); | |
// Check for duplicate search_name if it exists | |
if (item.search_name !== undefined) { | |
if (uniqueSearchNames.has(item.search_name)) { | |
duplicateSearchNames.push(item.search_name); | |
} else { | |
uniqueSearchNames.add(item.search_name); | |
} | |
} | |
// Check for duplicate name | |
if (uniqueNames.has(item.name)) { | |
duplicateNames.push(item.name); | |
} else { | |
uniqueNames.add(item.name); | |
// Check for duplicate name if it exists | |
if (item.name !== undefined) { | |
if (uniqueNames.has(item.name)) { | |
duplicateNames.push(item.name); | |
} else { | |
uniqueNames.add(item.name); | |
} | |
const validateUniqueFieldsInSelectedSites = (req, res, next) => { | |
const selectedSites = req.body.selected_sites; | |
// Fields to check for duplicates | |
const fieldsToCheck = ['site_id', 'search_name', 'name']; | |
const uniqueFields = {}; | |
const duplicateFields = {}; | |
// Initialize Sets for each field | |
fieldsToCheck.forEach((field) => { | |
uniqueFields[field] = new Set(); | |
duplicateFields[field] = new Set(); | |
}); | |
selectedSites.forEach((item) => { | |
fieldsToCheck.forEach((field) => { | |
if (item[field] !== undefined) { | |
if (uniqueFields[field].has(item[field])) { | |
duplicateFields[field].add(item[field]); | |
} else { | |
uniqueFields[field].add(item[field]); | |
} | |
} | |
}); | |
}); | |
// Prepare error messages based on duplicates found | |
let errorMessage = ""; | |
fieldsToCheck.forEach((field) => { | |
if (duplicateFields[field].size > 0) { | |
errorMessage += | |
`Duplicate ${field}s found: ${[...duplicateFields[field]].join(", ")}. `; | |
} | |
}); | |
// If any duplicates were found, respond with an error | |
if (errorMessage) { | |
return res.status(400).json({ | |
success: false, | |
message: errorMessage.trim(), | |
}); | |
} | |
next(); | |
}; |
Description
Refactoring the
createValidateSelectedSitesField
middleware function to add one edge case and also refactornig thevalidateUniqueFieldsInSelectedSites
middleware function to only consider fields that are present in the input objects.Changes Made
allowId
to the middleware function. This boolean parameter determines whether the_id
field is permitted._id
field now depends on the allowId parameter. IfallowId
isfalse
, the middleware will throw an error when_id
is present.!== undefined
) before checking for duplicates. This ensures that only provided fields are considered.Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
When using this middleware in your route definitions, you can specify whether to allow the
_id
field:Summary by CodeRabbit
Summary by CodeRabbit
New Features
selected_sites
in various routes to conditionally handle the presence of_id
fields.selected_sites
array, ensuring unique entries.Bug Fixes
_id
presence based on the new validation logic.