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

ensuring that each preference is unique for each GROUP #4060

Merged
merged 4 commits into from
Dec 13, 2024
Merged

ensuring that each preference is unique for each GROUP #4060

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
70fe61e
ensuring that each preference is unique for each GROUP
Baalmart Dec 13, 2024
faa861b
removing the unique from user_id
Baalmart Dec 13, 2024
73108c1
make the code more maintainable
Baalmart Dec 13, 2024
00a93ae
processObjectId function could be more robust by adding type checking…
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
261 changes: 220 additions & 41 deletions src/auth-service/models/Preference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ const PreferenceSchema = new mongoose.Schema(
type: ObjectId,
required: [true, "user_id is required"],
ref: "user",
unique: true,
},
site_ids: [
{
Expand Down Expand Up @@ -213,48 +212,228 @@ PreferenceSchema.plugin(uniqueValidator, {
message: `{VALUE} should be unique!`,
});

PreferenceSchema.pre("save", function (next) {
const fieldsToUpdate = [
"selected_sites",
"selected_grids",
"selected_cohorts",
"selected_devices",
"selected_airqlouds",
];

const currentDate = new Date();

fieldsToUpdate.forEach((field) => {
if (this[field]) {
this[field] = Array.from(
new Set(
this[field].map((item) => ({
...item,
createdAt: item.createdAt || currentDate,
}))
)
PreferenceSchema.index({ user_id: 1, group_id: 1 }, { unique: true });

Comment on lines +215 to +216
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

Adjust Unique Index to Handle Optional 'group_id'

An index is added to enforce uniqueness on { user_id: 1, group_id: 1 }. If group_id can be null or optional, multiple documents with the same user_id and a null group_id will violate this uniqueness constraint because MongoDB considers all null values as equal in unique indexes.

Consider making group_id required if uniqueness is essential, or modify the index to be a partial index that excludes documents where group_id is null.

PreferenceSchema.pre(
[
"save",
"create",
"update",
"findByIdAndUpdate",
"updateMany",
"updateOne",
"findOneAndUpdate",
],
async function (next) {
try {
// Determine if this is a new document or an update
const isNew = this.isNew;
const updateData = this.getUpdate ? this.getUpdate() : this;

// Utility function to validate and process ObjectIds
const processObjectId = (id) => {
return id instanceof mongoose.Types.ObjectId
? id
: mongoose.Types.ObjectId(id);
};
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

Enhance ObjectId validation

The processObjectId function could be more robust by adding type checking and error handling.

       const processObjectId = (id) => {
+        if (!id) return null;
+        if (id instanceof mongoose.Types.ObjectId) return id;
+        try {
+          return mongoose.Types.ObjectId(id);
+        } catch (error) {
+          logger.error(`Invalid ObjectId: ${id}`);
+          throw new Error(`Invalid ObjectId: ${id}`);
+        }
-        return id instanceof mongoose.Types.ObjectId
-          ? id
-          : mongoose.Types.ObjectId(id);
       };
📝 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 processObjectId = (id) => {
return id instanceof mongoose.Types.ObjectId
? id
: mongoose.Types.ObjectId(id);
};
const processObjectId = (id) => {
if (!id) return null;
if (id instanceof mongoose.Types.ObjectId) return id;
try {
return mongoose.Types.ObjectId(id);
} catch (error) {
logger.error(`Invalid ObjectId: ${id}`);
throw new Error(`Invalid ObjectId: ${id}`);
}
};


// Comprehensive ID fields processing
const idFieldsToProcess = [
"airqloud_id",
"airqloud_ids",
"grid_id",
"grid_ids",
"cohort_id",
"cohort_ids",
"network_id",
"network_ids",
"group_id",
"group_ids",
"site_ids",
"device_ids",
];

idFieldsToProcess.forEach((field) => {
if (updateData[field]) {
if (Array.isArray(updateData[field])) {
updateData[field] = updateData[field].map(processObjectId);
} else {
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 = [
{ field: "pollutant", default: "pm2_5" },
{ field: "frequency", default: "hourly" },
{ field: "chartType", default: "line" },
{ field: "chartTitle", default: "Chart Title" },
{ field: "chartSubTitle", default: "Chart SubTitle" },
];

defaultFields.forEach(({ field, default: defaultValue }) => {
if (isNew && !updateData[field]) {
updateData[field] = defaultValue;
}
});

// Handle date fields
if (isNew) {
const currentDate = new Date();
updateData.startDate =
updateData.startDate || addWeeksToProvideDateTime(currentDate, -2);
updateData.endDate = updateData.endDate || currentDate;
}

// Validate and process period schema
if (updateData.period) {
const validPeriodFields = ["value", "label", "unitValue", "unit"];
const periodUpdate = {};

validPeriodFields.forEach((field) => {
if (updateData.period[field] !== undefined) {
periodUpdate[field] = updateData.period[field];
}
});

// Additional period validation
if (
periodUpdate.unitValue !== undefined &&
typeof periodUpdate.unitValue !== "number"
) {
periodUpdate.unitValue = Number(periodUpdate.unitValue);
}

updateData.period = periodUpdate;
}

// Process and validate selected arrays with their specific schemas
const selectedArrayProcessors = {
selected_sites: (site) => {
const processedSite = { ...site };

// Validate and process ObjectIds
if (site._id) processedSite._id = processObjectId(site._id);
if (site.grid_id)
processedSite.grid_id = processObjectId(site.grid_id);

// Validate numeric fields
const numericFields = [
"latitude",
"longitude",
"approximate_latitude",
"approximate_longitude",
];
numericFields.forEach((field) => {
if (processedSite[field] !== undefined) {
processedSite[field] = Number(processedSite[field]);
}
});

// Ensure createdAt is a valid date
processedSite.createdAt = site.createdAt || new Date();

// Validate string fields
const stringFields = [
"country",
"district",
"sub_county",
"parish",
"county",
"generated_name",
"name",
"city",
"formatted_name",
"region",
"search_name",
];
stringFields.forEach((field) => {
if (processedSite[field]) {
processedSite[field] = String(processedSite[field]).trim();
}
});

// Ensure boolean fields
processedSite.isFeatured = !!site.isFeatured;

return processedSite;
},
selected_grids: (grid) => ({
_id: processObjectId(grid._id),
name: String(grid.name).trim(),
createdAt: grid.createdAt || new Date(),
}),
selected_cohorts: (cohort) => ({
_id: processObjectId(cohort._id),
name: String(cohort.name).trim(),
createdAt: cohort.createdAt || new Date(),
}),
selected_devices: (device) => ({
_id: processObjectId(device._id),
name: String(device.name).trim(),
createdAt: device.createdAt || new Date(),
}),
selected_airqlouds: (airqloud) => ({
_id: processObjectId(airqloud._id),
name: String(airqloud.name).trim(),
createdAt: airqloud.createdAt || new Date(),
}),
};

// Process selected arrays
Object.keys(selectedArrayProcessors).forEach((field) => {
if (updateData[field]) {
updateData[field] = updateData[field].map(
selectedArrayProcessors[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) => {
if (updateData[field]) {
updateData.$addToSet = updateData.$addToSet || {};
updateData.$addToSet[field] = {
$each: updateData[field],
};
delete updateData[field];
}
});

// Optional: Add comprehensive logging
console.log(
`Preprocessing preference document: ${isNew ? "New" : "Update"}`,
{
user_id: updateData.user_id,
pollutant: updateData.pollutant,
startDate: updateData.startDate,
endDate: updateData.endDate,
}
);
}
});

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

fieldsToAddToSet.forEach((field) => {
if (this[field]) {
this[field] = Array.from(new Set(this[field].map((id) => id.toString())));
}
});

return next();
});
next();
} catch (error) {
console.error("Error in Preference pre-hook:", error);
return next(error);
}
}
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

Separate Document and Query Middleware in Pre-Hook

The pre-hook (lines 218~ to 436~) is attempting to handle both document middleware (like save) and query middleware (like updateOne) within the same function. In Mongoose, this refers to the document in document middleware and the query in query middleware. Using this.isNew and modifying updateData directly may not behave as intended in query middleware.

It's advisable to separate the pre-hooks for document and query operations to handle their contexts appropriately. This ensures that validations and transformations are correctly applied in each scenario.

);

PreferenceSchema.methods = {
toJSON() {
Expand Down
Loading
Loading