Skip to content
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.

Sign up for GitHub

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

fixing runtime when upserting prefereces #4069

Merged
merged 2 commits into from
Dec 13, 2024
Merged

fixing runtime when upserting prefereces #4069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb7574d
fixing runtime when upserting prefereces
Baalmart Dec 13, 2024
666c098
addressing QA feedback
Baalmart Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 34 additions & 27 deletions src/auth-service/models/Preference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ PreferenceSchema.pre(
const processObjectId = (id) => {
if (!id) return null;
if (id instanceof mongoose.Types.ObjectId) return id;
if (typeof id === "string" && id.trim() === "") return null;
try {
return mongoose.Types.ObjectId(id);
} catch (error) {
Expand All @@ -242,37 +243,43 @@ PreferenceSchema.pre(
}
};

// Comprehensive ID fields processing
const idFieldsToProcess = [
// Define field categories
const singleIdFields = [
"user_id",
"group_id",
"airqloud_id",
"airqloud_ids",
"grid_id",
"grid_ids",
"cohort_id",
"cohort_ids",
"network_id",
];
const arrayIdFields = [
"airqloud_ids",
"grid_ids",
"cohort_ids",
"network_ids",
"group_id",
"group_ids",
"site_ids",
"device_ids",
"group_ids",
];
const selectedArrayFields = [
"selected_sites",
"selected_grids",
"selected_devices",
"selected_cohorts",
"selected_airqlouds",
];

idFieldsToProcess.forEach((field) => {
// Process single ID fields
singleIdFields.forEach((field) => {
if (updateData[field]) {
if (Array.isArray(updateData[field])) {
updateData[field] = updateData[field].map(processObjectId);
} else {
updateData[field] = processObjectId(updateData[field]);
}
updateData[field] = processObjectId(updateData[field]);
}
});

// Validate user_id
if (!updateData.user_id) {
return next(new Error("user_id is required"));
}
updateData.user_id = processObjectId(updateData.user_id);

// Set default values if not provided
const defaultFields = [
Expand Down Expand Up @@ -393,29 +400,29 @@ PreferenceSchema.pre(
};

// Process selected arrays
Object.keys(selectedArrayProcessors).forEach((field) => {
selectedArrayFields.forEach((field) => {
if (updateData[field]) {
updateData[field] = updateData[field].map(
selectedArrayProcessors[field]
);

// Use $addToSet for selected arrays
updateData.$addToSet = updateData.$addToSet || {};
updateData.$addToSet[field] = {
$each: updateData[field],
};
delete updateData[field];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent overwriting of $addToSet in selected arrays processing.

When processing selectedArrayFields, directly assigning to updateData.$addToSet may overwrite existing entries. To avoid this, merge new fields into $addToSet without replacing existing ones.

Modify the code to merge fields:

           // Use $addToSet for selected arrays
-          updateData.$addToSet = updateData.$addToSet || {};
-          updateData.$addToSet[field] = {
+          updateData.$addToSet = {
+            ...updateData.$addToSet,
+            [field]: {
               $each: updateData[field],
             },
           };
           delete updateData[field];
📝 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.

Suggested change
selectedArrayFields.forEach((field) => {
if (updateData[field]) {
updateData[field] = updateData[field].map(
selectedArrayProcessors[field]
);
// Use $addToSet for selected arrays
updateData.$addToSet = updateData.$addToSet || {};
updateData.$addToSet[field] = {
$each: updateData[field],
};
delete updateData[field];
selectedArrayFields.forEach((field) => {
if (updateData[field]) {
updateData[field] = updateData[field].map(
selectedArrayProcessors[field]
);
// Use $addToSet for selected arrays
updateData.$addToSet = {
...updateData.$addToSet,
[field]: {
$each: updateData[field],
},
};
delete updateData[field];

}
});

// Prepare $addToSet for array fields to prevent duplicates
const arrayFieldsToAddToSet = [
...idFieldsToProcess,
"selected_sites",
"selected_grids",
"selected_devices",
"selected_cohorts",
"selected_airqlouds",
];

arrayFieldsToAddToSet.forEach((field) => {
// Process array ID fields
arrayIdFields.forEach((field) => {
if (updateData[field]) {
updateData.$addToSet = updateData.$addToSet || {};
updateData.$addToSet[field] = {
$each: updateData[field],
$each: Array.isArray(updateData[field])
? updateData[field].map(processObjectId)
: [processObjectId(updateData[field])],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure $addToSet merges existing array ID fields.

Similar to the previous issue, when adding array ID fields to $addToSet, ensure you merge with existing fields to prevent overwriting.

Update the code as follows:

           if (updateData[field]) {
-            updateData.$addToSet = updateData.$addToSet || {};
-            updateData.$addToSet[field] = {
+            updateData.$addToSet = {
+              ...updateData.$addToSet,
+              [field]: {
                 $each: Array.isArray(updateData[field])
                   ? updateData[field].map(processObjectId)
                   : [processObjectId(updateData[field])],
               },
             };
             delete updateData[field];
           }
📝 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.

Suggested change
// Process array ID fields
arrayIdFields.forEach((field) => {
if (updateData[field]) {
updateData.$addToSet = updateData.$addToSet || {};
updateData.$addToSet[field] = {
$each: updateData[field],
$each: Array.isArray(updateData[field])
? updateData[field].map(processObjectId)
: [processObjectId(updateData[field])],
// Process array ID fields
arrayIdFields.forEach((field) => {
if (updateData[field]) {
updateData.$addToSet = {
...updateData.$addToSet,
[field]: {
$each: Array.isArray(updateData[field])
? updateData[field].map(processObjectId)
: [processObjectId(updateData[field])],
},
};

};
delete updateData[field];
}
Expand Down
93 changes: 74 additions & 19 deletions src/auth-service/utils/create-preference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,25 +54,40 @@ const validateUserAndGroup = async (tenant, userId, groupId, next) => {
const prepareUpdate = (body, fieldsToUpdate, fieldsToAddToSet) => {
const update = { ...body };

// Handle fields that should be added to set (array fields)
fieldsToAddToSet.forEach((field) => {
if (update[field]) {
update["$addToSet"] = { [field]: { $each: update[field] } };
update["$addToSet"] = update["$addToSet"] || {};
update["$addToSet"][field] = {
$each: Array.isArray(update[field]) ? update[field] : [update[field]],
};
delete update[field];
}
});

// Handle fields that need special processing (with createdAt)
fieldsToUpdate.forEach((field) => {
if (update[field]) {
update[field] = update[field].map((item) => ({
...item,
createdAt: item.createdAt || new Date(),
}));

update["$addToSet"] = { [field]: { $each: update[field] } };
update["$addToSet"] = update["$addToSet"] || {};
update["$addToSet"][field] = { $each: update[field] };
delete update[field];
}
});

// Remove simple ObjectId fields from being processed by $addToSet
const singleObjectIdFields = ["user_id", "group_id"];
singleObjectIdFields.forEach((field) => {
if (update[field]) {
// Keep the original value for simple ObjectId fields
update[field] = update[field];
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unnecessary self-assignment of update[field].

Assigning update[field] to itself on line 87 is redundant and has no effect. Removing this line will clean up the code without altering functionality.

Apply this diff to eliminate the redundant assignment:

       if (update[field]) {
-        // Keep the original value for simple ObjectId fields
-        update[field] = update[field];
       }
📝 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.

Suggested change
const singleObjectIdFields = ["user_id", "group_id"];
singleObjectIdFields.forEach((field) => {
if (update[field]) {
// Keep the original value for simple ObjectId fields
update[field] = update[field];
}
});
const singleObjectIdFields = ["user_id", "group_id"];
singleObjectIdFields.forEach((field) => {
if (update[field]) {
}
});
🧰 Tools
🪛 Biome (1.9.4)

[error] 87-87: field is assigned to itself.

This is where is assigned.

(lint/correctness/noSelfAssign)


return update;
};

Expand Down Expand Up @@ -169,7 +184,34 @@ const preferences = {
body,
} = request;

const filterResponse = generateFilter.preferences(request, next);
// Validate user and group
const validationError = await validateUserAndGroup(
tenant,
body.user_id,
body.group_id,
next
);
if (validationError) return;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify error handling after validateUserAndGroup call.

The validateUserAndGroup function handles errors internally and does not return a value. Therefore, checking validationError is unnecessary since it will always be undefined.

Consider removing the unnecessary variable and condition:

-      const validationError = await validateUserAndGroup(
+      await validateUserAndGroup(
         tenant,
         body.user_id,
         body.group_id,
         next
       );
-      if (validationError) return;
📝 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.

Suggested change
// Validate user and group
const validationError = await validateUserAndGroup(
tenant,
body.user_id,
body.group_id,
next
);
if (validationError) return;
// Validate user and group
await validateUserAndGroup(
tenant,
body.user_id,
body.group_id,
next
);

const fieldsToUpdate = [
"selected_sites",
"selected_grids",
"selected_cohorts",
"selected_devices",
"selected_airqlouds",
];

const fieldsToAddToSet = [
"airqloud_ids",
"device_ids",
"cohort_ids",
"grid_ids",
"site_ids",
"network_ids",
"group_ids",
];

const filter = generateFilter.preferences(request, next);
if (isEmpty(filterResponse) || isEmpty(filterResponse.user_id)) {
next(
new HttpError(
Expand All @@ -183,21 +225,7 @@ const preferences = {
);
}

const PreferenceDetails = await PreferenceModel(tenant)
.findOne(filterResponse)
.select("_id")
.lean();

if (isEmpty(PreferenceDetails)) {
next(
new HttpError("Bad Request Errors", httpStatus.BAD_REQUEST, {
message: `No existing preferences for the provided User ID: ${filterResponse.user_id.toString()}`,
})
);
}

const filter = PreferenceDetails;
const update = body;
const update = prepareUpdate(body, fieldsToUpdate, fieldsToAddToSet);

const modifyResponse = await PreferenceModel(tenant).modify(
{
Expand Down Expand Up @@ -305,6 +333,33 @@ const preferences = {
body,
} = request;

// Validate user and group
const validationError = await validateUserAndGroup(
tenant,
body.user_id,
body.group_id,
next
);
if (validationError) return;

const fieldsToUpdate = [
"selected_sites",
"selected_grids",
"selected_cohorts",
"selected_devices",
"selected_airqlouds",
];

const fieldsToAddToSet = [
"airqloud_ids",
"device_ids",
"cohort_ids",
"grid_ids",
"site_ids",
"network_ids",
"group_ids",
];

logText("Replace the existing selected_ids....");

const filterResponse = generateFilter.preferences(request, next);
Expand All @@ -320,7 +375,7 @@ const preferences = {
};
}

const update = body;
const update = prepareUpdate(body, fieldsToUpdate, fieldsToAddToSet);
const options = { upsert: true, new: true };

const modifyResponse = await PreferenceModel(tenant).findOneAndUpdate(
Expand Down
Loading