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 2 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
94 changes: 87 additions & 7 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 @@ -44,12 +44,57 @@ const preferences = {
const { tenant } = query;
logObject("the body", body);
const user_id = body.user_id;
const user = await UserModel(tenant).findById(user_id).lean();
if (isEmpty(user_id) || isEmpty(user)) {
next(
new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
message: "The provided User does not exist",
value: user_id,
const group_id = body.group_id;

if (!isEmpty(user_id)) {
const user = await UserModel(tenant).findById(user_id).lean();
if (isEmpty(user_id) || isEmpty(user)) {
next(
new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
message: "The provided User does not exist",
value: user_id,
})
);
}
}

// Check if user belongs to the specified group
if (!isEmpty(group_id)) {
const userBelongsToGroup = user.group_roles.some(
(role) => role.group.toString() === group_id
);

if (!userBelongsToGroup) {
return next(
new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
message: "User does not belong to the specified group",
})
);
}
}
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

Handle Potential 'user' Undefined Scenario When Checking Group Membership

In both the create and upsert methods, when checking if the user belongs to a specified group, the code assumes that user is defined and has a property group_roles. If user is undefined (e.g., if the user does not exist), accessing user.group_roles will result in a runtime error.

To ensure robustness, add a check to confirm that user exists before accessing its properties.

// Before accessing user.group_roles
if (!isEmpty(group_id)) {
+ if (user && user.group_roles) {
    const userBelongsToGroup = user.group_roles.some(
      (role) => role.group.toString() === group_id
    );
    if (!userBelongsToGroup) {
      return next(
        new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
          message: "User does not belong to the specified group",
        })
      );
    }
+ } else {
+   return next(
+     new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
+       message: "User not found or invalid user data",
+     })
+   );
  }
}

Also applies to: 205-219

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

Refactor to Eliminate Duplicate Validation Logic

The validation logic for user_id and group_id in the create and upsert methods is almost identical. Duplicated code can make maintenance more challenging and increase the risk of inconsistencies.

Extract the common validation steps into a separate helper function that both methods can call. This will improve code readability and maintainability.

Also applies to: 186-219


const filterResponse = generateFilter.preferences(request, next);
if (isEmpty(filterResponse) || isEmpty(filterResponse.user_id)) {
return {
success: false,
message: "Internal Server Error",
errors: {
message:
"Unable to obtain the corresponding identifier associated with this preference --- please reach out to [email protected]",
},
status: httpStatus.INTERNAL_SERVER_ERROR,
};
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

Ensure Consistent Error Handling Using 'next()'

In the create and upsert methods, the error handling is inconsistent. Some errors are passed to the next middleware using next(), while others are returned directly as objects. For example, in lines 77~ to 86~ and 193~ to 201~, errors are returned instead of using next().

To maintain a uniform error-handling mechanism and ensure that all errors are processed by the middleware chain, it's advisable to consistently use next() to pass errors.

Apply the following changes:

// In the 'create' method
-if (isEmpty(filterResponse) || isEmpty(filterResponse.user_id)) {
-  return {
-    success: false,
-    message: "Internal Server Error",
-    errors: {
-      message:
-        "Unable to obtain the corresponding identifier associated with this preference --- please reach out to [email protected]",
-    },
-    status: httpStatus.INTERNAL_SERVER_ERROR,
-  };
+if (isEmpty(filterResponse) || isEmpty(filterResponse.user_id)) {
+  next(
+    new HttpError(
+      "Internal Server Error",
+      httpStatus.INTERNAL_SERVER_ERROR,
+      {
+        message:
+          "Unable to obtain the corresponding identifier associated with this preference --- please reach out to [email protected]",
+      }
+    )
+  );
+  return;
}

// In the 'upsert' method
-if (isEmpty(user_id) || isEmpty(user)) {
-  return {
-    success: false,
-    message: "Internal Server Error",
-    errors: {
-      message: "The provided User does not exist",
-    },
-    status: httpStatus.INTERNAL_SERVER_ERROR,
-  };
+if (isEmpty(user_id) || isEmpty(user)) {
+  next(
+    new HttpError(
+      "Internal Server Error",
+      httpStatus.INTERNAL_SERVER_ERROR,
+      { message: "The provided User does not exist" }
+    )
+  );
+  return;
}

Also applies to: 189-201

}

// Check if a preference already exists for this user and group
const existingPreference = await PreferenceModel(tenant).findOne(
filterResponse
);

if (existingPreference) {
return next(
new HttpError("Conflict", httpStatus.CONFLICT, {
message: "Preferences for this user and group already exist",
})
);
}
Expand Down Expand Up @@ -138,6 +183,41 @@ const preferences = {
body,
} = request;

const user_id = body.user_id;
const group_id = body.group_id;

// Validate user exists and belongs to the group
if (!isEmpty(user_id)) {
const user = await UserModel(tenant).findById(user_id).lean();
if (isEmpty(user_id) || isEmpty(user)) {
return {
success: false,
message: "Internal Server Error",
errors: {
message: "The provided User does not exist",
},
status: httpStatus.INTERNAL_SERVER_ERROR,
};
}
}

if (!isEmpty(group_id)) {
const userBelongsToGroup = user.group_roles.some(
(role) => role.group.toString() === group_id
);

if (!userBelongsToGroup) {
return {
success: false,
message: "Bad Request",
errors: {
message: "User does not belong to the specified group",
},
status: httpStatus.BAD_REQUEST,
};
}
}

const fieldsToUpdate = [
"selected_sites",
"selected_grids",
Expand Down Expand Up @@ -170,7 +250,7 @@ const preferences = {
};
}

const update = body;
const update = { ...body };

fieldsToAddToSet.forEach((field) => {
if (update[field]) {
Expand Down
Loading
Loading