Refactor: Optimize user list and update functions for performance #5268
Conversation
📝 WalkthroughWalkthroughRewrote User.list from aggregation to find().lean() plus parallel lookups and manual in-memory population; changed error propagation to thrown HttpError. Updated user utilities to always invoke mailer.update (awaited or fire-and-forget), sanitize interests, make password-reset emails async, add initiatePasswordReset and generateResetToken, and add transition-based token expiry in login. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Caller
participant S as UserService.list
participant DBU as Users DB
participant DBP as Permissions DB
participant DBR as Roles DB
participant DBG as Groups DB
participant DBN as Networks DB
participant DBC as Clients DB
C->>S: list(request)
rect rgba(230,240,255,0.5)
S->>DBU: find().lean() base users
S->>S: collect related IDs (perm/role/group/network/client)
par Parallel lookups
S->>DBP: find by permission IDs
S->>DBR: find by role IDs
S->>DBG: find by group IDs
S->>DBN: find by network IDs
S->>DBC: find by client IDs
end
S->>S: build maps, populate users, remove legacy fields, omit password
end
S-->>C: { success, message, data, totalCount, status }
note over S: Errors thrown as HttpError
sequenceDiagram
autonumber
actor U as Updater
participant M as Mailer
participant DB as User DB
U->>DB: apply user modifications
alt Awaited mailer path
U->>M: mailer.update(updatedUserDetails, ids)
M-->>U: success / failure / no response
alt success
U-->>Caller: return modification result
else failure or no response
U-->>Caller: throw 500 HttpError
end
else Fire-and-forget path
U-)M: mailer.update(...).catch(log)
U-->>Caller: return modification result
end
sequenceDiagram
autonumber
actor C as Caller
participant U as UserUtil
participant Auth as AuthService
participant M as Mailer
C->>U: initiatePasswordReset({email})
U->>Auth: generateResetToken(next)
Auth-->>U: token
U-)M: send reset email (async).catch(log)
U-->>C: success (immediate)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #5268 +/- ##
===========================================
- Coverage 11.99% 11.99% -0.01%
===========================================
Files 198 199 +1
Lines 25634 25813 +179
Branches 721 756 +35
===========================================
+ Hits 3076 3095 +19
- Misses 22535 22695 +160
Partials 23 23
🚀 New features to boost your workflow:
|
|
Auth-service changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/auth-service/utils/user.util.js (2)
762-897: Duplicate ‘update’ definitions — later one overrides the earlier (logic lost).Only the second
update(Lines 4029–4135) is exported; the first (Lines 762–897) is dead code. That silently drops the “interests” sanitization and the awaited mailer path.
- Keep a single
updateand merge the missing pieces:
- Port the “interests” sanitization into the surviving function.
- Gate “await vs fire-and-forget” on a config flag instead of duplicating functions.
Suggested consolidation:
@@ 4034,4051 @@ - const sanitizedUpdate = { ...body }; + const sanitizedUpdate = { ...body }; + + // Interests sanitization (ported from the earlier update) + if ("interests" in sanitizedUpdate) { + const v = sanitizedUpdate.interests; + if (typeof v === "string") { + sanitizedUpdate.interests = v.trim() ? [v.trim()] : []; + } else if (Array.isArray(v)) { + sanitizedUpdate.interests = v.map((i) => (i ? String(i).trim() : "")).filter(Boolean); + } else { + delete sanitizedUpdate.interests; + } + } @@ 4096,4115 @@ - // Asynchronously send email without blocking the response. - // The mailer utility should handle environment-specific logic (e.g., using a mock mailer in dev). - mailer - .update( - { - email, - firstName, - lastName, - updatedUserDetails: emailUpdatePayload, - }, - next - ) - .catch((error) => { - logger.error( - `Failed to send profile update email to ${email}: ${error.message}` - ); - }); + // Configurable: await in non-prod, fire-and-forget in prod + const shouldAwaitMailer = + (constants && constants.ENVIRONMENT !== "production") || + process.env.NODE_ENV !== "production"; + const send = () => + mailer.update( + { email, firstName, lastName, updatedUserDetails: emailUpdatePayload }, + next + ); + if (shouldAwaitMailer) { + try { + const mailerRes = await send(); + if (mailerRes && mailerRes.success === false) { + logger.error(`Profile update email failed for ${email}: ${mailerRes.message}`); + } + } catch (e) { + logger.error(`Profile update email threw for ${email}: ${e.message}`); + } + } else { + send().catch((e) => + logger.error(`Profile update email failed for ${email}: ${e.message}`) + ); + }Also remove the earlier duplicate
updateto avoid future drift.Also applies to: 4029-4135
710-721: Action required: refactor removed legacy fields — update callers or restore backward compatibilityUserModel.list now returns populated "groups/networks/permissions" and deletes "group_roles"/"network_roles" (see src/auth-service/models/User.js:1007-1008). Multiple consumers still read the legacy fields and will break:
- src/auth-service/services/rbac.service.js (uses group_roles/network_roles in many places — e.g. ~68-76, 136-159, 245-309, 512-531).
- src/device-registry/utils/organization.util.js (user.group_roles at 161,192,289-297).
- src/auth-service/routes/v2/users.routes.js and v3/users.routes.js (manual populate/mapping of network_roles — see ~236-266, 291-304).
- src/auth-service/utils/group.util.js (aggregation formatting of group_roles.createdAt via $dateToString "%Y-%m-%d" — ~3459-3462).
- src/auth-service/utils/role-permissions.util.js (summaries and lookups expect group_roles/network_roles).
- Unit tests and migrations under src/auth-service/utils/test/** and src/auth-service/migrations/** reference legacy fields.
Either (A) reintroduce group_roles/network_roles in User.list as a backward-compatible shim, or (B) update all listed consumers (and tests/migrations) to use the new populated fields ("groups", "networks", "permissions").
🧹 Nitpick comments (6)
src/auth-service/models/User.js (5)
804-810: Don’t fetch secrets; project out password/reset fields up front.Instead of deleting later, exclude sensitive fields in the base query to reduce payload and risk.
- const users = await this.find(filter) + const users = await this.find(filter) + .select("-password -resetPasswordToken -resetPasswordExpires") .sort({ createdAt: -1 }) .skip(skip ? parseInt(skip) : 0) .limit(limit ? parseInt(limit) : parseInt(constants.DEFAULT_LIMIT)) .lean(); // Use lean for performance
858-869: Redundant re-import of getModelByTenant.It’s already imported at the top (Line 13). Drop this inner require to avoid shadowing and confusion.
- const { getModelByTenant } = require("@config/database"); const GroupModel = require("@models/Group"); const NetworkModel = require("@models/Network"); const RoleModel = require("@models/Role"); const ClientSchema = new mongoose.Schema({}, { strict: false }); const ClientModel = getModelByTenant( this.db.name, "client", ClientSchema );
869-901: Large related queries lack projections; trim fields to control response size.Consider adding .select() to clients, my_networks, and my_groups (and possibly roles) to avoid returning heavy/unused fields.
953-1030: Membership entries are dropped if role is missing.The current mapping returns null when either group/network or role is missing, potentially hiding memberships with a null role. Prefer keeping the membership and setting role to null for visibility.
- if (!group || !role) return null; + if (!group) return null; // require group return { _id: group._id, ... - role: { - _id: role._id, - role_name: role.role_name, - role_permissions: role.role_permissions, - }, + role: role + ? { + _id: role._id, + role_name: role.role_name, + role_permissions: role.role_permissions, + } + : null, userType: gr.userType || "user", };Apply the same pattern for networks.
1011-1026: createdAt formatting: prefer ISO/UTC for consistency (or verify compatibility).Manual local-time formatting may diverge from prior API expectations/timezone. If backward compatibility doesn’t require this exact format, return ISO (UTC) for both createdAt and updatedAt.
Would switching to
createdAt = ts.toISOString()break any known consumers?src/auth-service/utils/user.util.js (1)
4096-4115: Non-blocking mailer path: solid pattern, but add minimal backoff.Add a tiny retry/backoff (e.g., 1 retry with jitter) to improve resilience on transient SMTP/API hiccups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/auth-service/models/User.js(2 hunks)src/auth-service/utils/user.util.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/auth-service/utils/user.util.js (2)
src/auth-service/utils/request.util.js (9)
user(22-25)responseFromSendEmail(81-90)responseFromSendEmail(295-309)responseFromSendEmail(647-656)responseFromSendEmail(759-768)responseFromSendEmail(859-867)responseFromSendEmail(894-897)logger(12-14)httpStatus(7-7)src/auth-service/utils/common/mailer.util.js (8)
mailer(573-1519)logger(20-20)httpStatus(7-7)error(895-898)error(920-923)error(941-944)error(1291-1298)error(1318-1325)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
src/auth-service/utils/user.util.js (1)
221-221: Good fix: add cache TTL.Setting 10‑minute expiration prevents stale/infinite cache entries.
src/auth-service/models/User.js (1)
1051-1057: Behavior change — list now throws; verified callers handle itAll occurrences of UserModel(...).list are awaited inside try/catch blocks that call next(...) on error (checked: src/auth-service/utils/user.util.js — lines ~624, 711, 739, 1148, 4282; src/auth-service/utils/network.util.js — lines ~1267, 1314). Thrown errors will be routed to the existing catch → next(...) paths — no change required.
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/auth-service/utils/user.util.js (4)
849-881: Duplicate update() + awaited mailer can turn a successful DB update into a 500
- This file defines createUserModule.update twice; the later definition (Line 4017+) overrides this one. Keep a single implementation to avoid dead code and confusion.
- Awaiting mailer.update here couples DB success to email delivery and can regress UX into a 500 path.
Action:
- Remove the earlier duplicate update (Lines 762–897) and keep the later fire‑and‑forget version.
- If you must keep an awaited path, never fail the request on email failure; surface as a non‑blocking warning.
4104-4122: Background email in update(): remove next to avoid late middleware executionFire‑and‑forget is good. However, passing next into a background promise can trigger middleware after the HTTP response has been sent.
Apply:
- mailer - .update( - { - email, - firstName, - lastName, - updatedUserDetails: emailUpdatePayload, - }, - next - ) + void mailer + .update({ + email, + firstName, + lastName, + updatedUserDetails: emailUpdatePayload, + }) .catch((error) => { logger.error( `Failed to send profile update email to ${email}: ${error.message}` ); });
4208-4231: Same here: don’t pass next into async mailer in resetPassword()Avoid invoking route middleware from a detached promise.
- mailer - .updateForgottenPassword( - { - email, - firstName, - lastName, - }, - next - ) + void mailer + .updateForgottenPassword({ + email, + firstName, + lastName, + }) .catch((error) => { logger.error( `Failed to send password reset confirmation email to ${email}: ${error.message}` ); });
3890-3911: Don’t pass next into fire‑and‑forget mailer callsPassing next to an async/background task risks “headers already sent” or double error-handling after the response returns. Drop next and just log.
Apply:
- mailer - .updateForgottenPassword( - { - email, - firstName, - lastName, - }, - next - ) + void mailer + .updateForgottenPassword({ + email, + firstName, + lastName, + }) .catch((error) => { logger.error( `Failed to send password reset confirmation email to ${email}: ${error.message}` ); });
🧹 Nitpick comments (1)
src/auth-service/utils/user.util.js (1)
4026-4043: Interests sanitization: dedupe entriesNice normalization. Consider de‑duplicating to avoid repeated values.
- sanitizedUpdate.interests = interestsValue - .map((item) => (item ? String(item).trim() : "")) - .filter(Boolean); + sanitizedUpdate.interests = Array.from( + new Set( + interestsValue + .map((item) => (item ? String(item).trim() : "")) + .filter(Boolean) + ) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/auth-service/utils/user.util.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/auth-service/utils/user.util.js (2)
src/auth-service/utils/request.util.js (9)
user(22-25)responseFromSendEmail(81-90)responseFromSendEmail(295-309)responseFromSendEmail(647-656)responseFromSendEmail(759-768)responseFromSendEmail(859-867)responseFromSendEmail(894-897)logger(12-14)httpStatus(7-7)src/auth-service/utils/common/mailer.util.js (8)
mailer(573-1519)logger(20-20)httpStatus(7-7)error(895-898)error(920-923)error(941-944)error(1291-1298)error(1318-1325)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-auth-service
🔇 Additional comments (1)
src/auth-service/utils/user.util.js (1)
4256-4274: LGTM: generateResetToken helperSmall, focused, and returns a consistent shape; good reuse across flows.
…nant-aware manner,
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/auth-service/models/Client.js (3)
4-4: Use Schema.Types.ObjectId;mongoose.ObjectIdis non-standard.This can cause type resolution issues across Mongoose versions.
Apply:
-const ObjectId = mongoose.ObjectId; +const ObjectId = Schema.Types.ObjectId;
243-252: Do not expose client_secret in API serialization — make it non-selectable and remove it from toJSONtoJSON currently returns client_secret (src/auth-service/models/Client.js) and must be removed; make the schema field non-selectable and opt-in where required.
Apply:
@@ - client_secret: { type: String, trim: true }, + client_secret: { type: String, trim: true, select: false }, @@ toJSON() { return { _id: this._id, - client_secret: this.client_secret, redirect_uri: this.redirect_uri, name: this.name, isActive: this.isActive, description: this.description, rateLimit: this.rateLimit, ip_address: this.ip_address, ip_addresses: this.ip_addresses, }; },Action items (brief):
- Update call sites that intentionally return the secret to explicitly opt-in with .select('+client_secret') and gate access with proper authorization: notably src/auth-service/utils/client.util.js (create/regenerate/update flows).
- Review and adjust DB projection configs that reference client_secret (src/auth-service/config/global/db-projections.js and blog-content-manager equivalents) so they don’t re-introduce accidental exposure.
- Update tests and any code that currently assumes the secret is in default serialization (src/auth-service/models/test/ut_client.model.js, src/auth-service/routes/v2/test/ut_clients.routes.js, and parallel blog-content-manager tests).
- Re-run a repository-wide search for "client_secret" and fix remaining callers and projections to opt in or stop expecting the secret.
256-264: Fix tenant-model registration to avoid OverwriteModelError; add tenant-context logging.getModelByTenant currently registers models unconditionally on tenant connections — calling db.model(modelName, schema) in getQueryTenantDB/getCommandTenantDB will throw Mongoose's OverwriteModelError on repeated calls. Fix model registration and add contextual logging.
- Critical (must fix): src/auth-service/config/database.js — change getQueryTenantDB/getCommandTenantDB to avoid re-defining a compiled model. Example options (pick one):
- Check and return existing model: if (db.models && db.models[modelName]) return db.model(modelName); db.model(modelName, schema); return db.model(modelName);
- Or wrap registration in try/catch and return the existing model when OverwriteModelError is thrown.
- Minor (apply): src/auth-service/models/Client.js (lines ~256-264) — add tenant-context logging in the catch before rethrowing; the original diff below is still relevant.
Apply this diff to Client.js:
const ClientModel = (tenant) => { const defaultTenant = constants.DEFAULT_TENANT || "airqo"; const dbTenant = isEmpty(tenant) ? defaultTenant : tenant; try { - return getModelByTenant(dbTenant, "client", ClientSchema); + return getModelByTenant(dbTenant, "client", ClientSchema); } catch (error) { - throw error; + logger.error(`getModelByTenant failed for tenant=${dbTenant}`, error); + throw error; } };Files to update: src/auth-service/config/database.js (getQueryTenantDB, getCommandTenantDB), src/auth-service/models/Client.js (lines 256-264).
🧹 Nitpick comments (3)
src/auth-service/models/Client.js (3)
219-224: Prefer findOneAndDelete and explicit select over projection with findOneAndRemove.
findOneAndRemoveis legacy and projection semantics can be surprising, especially withselect: falsefields. UsefindOneAndDelete()and an explicit.select().Apply:
- const options = { - projection: { _id: 1, client_secret: 1 }, // Preserve client_secret projection - }; - - const removedClient = await this.findOneAndRemove(filter, options).exec(); + const removedClient = await this.findOneAndDelete(filter) + .select('+client_secret _id') + .exec();
174-176: Avoid relying on Mongoose’s internal _doc; use toObject().This is more stable across versions.
Apply:
- return createSuccessResponse("update", updatedClient._doc, "client"); + return createSuccessResponse("update", updatedClient.toObject(), "client");
48-50: Remove no-op pre("update") hook.Dead hook adds noise and can confuse maintainers.
Apply:
-ClientSchema.pre("update", function (next) { - return next(); -});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/auth-service/models/Client.js(1 hunks)src/auth-service/models/User.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/auth-service/models/User.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-auth-service
1 participant
🚀 Pull Request
📋 Description
What does this PR do?
This PR refactors the list static method in User.model.js and the update function in user.util.js. The list method's complex aggregation pipeline is replaced with a more robust manual population pattern. The update function is streamlined to remove environment-specific logic for sending emails, delegating this responsibility to the mailer utility.
Why is this change needed?
The previous aggregation in the list method was complex and could be brittle across different deployment environments, leading to potential performance issues or unexpected failures. The update function's logic for sending emails was inconsistent across environments. These changes improve performance, reliability, and maintainability, ensuring the API behavior remains consistent while making the code more explicit and stable.
🔗 Related Issues
🔄 Type of Change
🏗️ Affected Services
Microservices changed: auth-service
🧪 Testing
Test summary:
Manual testing was performed on the /users (list) and user profile update endpoints to ensure the API responses remain identical to the previous implementation, guaranteeing backward compatibility with any consuming frontend applications. Verified that user data is correctly populated and profile updates trigger the expected side effects.
💥 Breaking Changes
📝 Additional Notes
The refactoring of the list method away from a large aggregation pipeline to a "manual population" pattern is a deliberate choice to increase code stability and avoid environment-specific issues with complex Mongoose/MongoDB features. This makes the data retrieval process more explicit and resilient.
✅ Checklist
Summary by CodeRabbit
Performance
Refactor
New Features
Bug Fixes
Style