Fix Mongoose 9 pre-save hook incompatibility causing all integration tests to fail#11
Conversation
Agent-Logs-Url: https://github.com/amritghimire/express-user-model/sessions/a1a16211-80c9-478f-9545-e48ffc69d3b6 Co-authored-by: amritghimire <16842655+amritghimire@users.noreply.github.com>
…space bug Agent-Logs-Url: https://github.com/amritghimire/express-user-model/sessions/ad372813-3a9d-429e-bf26-c1251bee9ae0 Co-authored-by: amritghimire <16842655+amritghimire@users.noreply.github.com>
…ests Agent-Logs-Url: https://github.com/amritghimire/express-user-model/sessions/2494ff2a-0695-409d-b8e1-cb6024605cda Co-authored-by: amritghimire <16842655+amritghimire@users.noreply.github.com>
….x compatibility Agent-Logs-Url: https://github.com/amritghimire/express-user-model/sessions/e34e6ddb-823c-4f23-bb16-55241c0df922 Co-authored-by: amritghimire <16842655+amritghimire@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the library and its test suite for compatibility with Mongoose 9 (kareem v3), addressing failing save() calls due to deprecated callback-style middleware and modernizing several model/middleware methods to promise/async usage.
Changes:
- Update the Basic plugin
pre('save')hook andfull_name()behavior. - Modernize Email plugin and Verify middleware behavior (async/await) and add/adjust unit + integration tests accordingly.
- Upgrade dependencies (Mongoose/Mocha/uuid), update lockfile format, and add GitHub Actions CI.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/basic.js | Adjusts pre-save hook style for Mongoose 9 and trims full_name() output. |
| lib/email.js | Updates uuid import and converts several methods to async behavior. |
| lib/verify.js | Converts verification middleware to async/await and persists verification changes reliably. |
| test/test.js | Converts integration tests to async/await and adds coverage for new behaviors. |
| test/unit.test.js | Adds offline unit tests for Basic/Email/Verify behaviors. |
| package.json | Upgrades mocha/mongoose/uuid and adds npm overrides. |
| package-lock.json | Updates dependency tree and lockfileVersion to v3. |
| Makefile | Removes jshint from make test. |
| .github/workflows/test.yml | Adds CI matrix for Node + MongoDB with npm ci + npm test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| schema.pre("save", function () { | ||
| if (this.isNew) { | ||
| this.username= this.username.toLowerCase(); | ||
| this.username = this.username.toLowerCase(); |
There was a problem hiding this comment.
The pre-save hook always reads/writes this.username, which breaks when options.usernameField is customized (the plugin may not add a username path at all). In that case this.username is undefined and toLowerCase() will throw on save(). Use options.usernameField (and guard when the field is missing) so custom username fields are normalized too.
| this.username = this.username.toLowerCase(); | |
| var usernameField = options.usernameField; | |
| if (usernameField && typeof this[usernameField] === 'string') { | |
| this[usernameField] = this[usernameField].toLowerCase(); | |
| } |
|
|
||
| if (!newPrimaryEmail) throw new Error('Email does not exist'); | ||
|
|
||
| if (currentPrimaryEmail !== newPrimaryEmail) { |
There was a problem hiding this comment.
setPrimaryEmail() compares currentPrimaryEmail (a string) to newPrimaryEmail (a subdocument), so the inequality check is effectively always true and will always call save() even if the requested email is already the current primary. Compare against the address string (or newPrimaryEmail.address) to avoid unnecessary writes.
| if (currentPrimaryEmail !== newPrimaryEmail) { | |
| if (currentPrimaryEmail !== email) { |
| schema.methods.sendVerificationToken = async function (email) { | ||
| var email_doc = (typeof email === 'string' ? this.getEmail(email) : email); | ||
| if (!email_doc) { | ||
| return callback(email, null); | ||
| throw new Error('Email not found'); | ||
| } | ||
| if(email_doc.verified === true){ | ||
| return callback(null, null); | ||
| if (email_doc.verified === true) { | ||
| return null; | ||
| } | ||
| if (email_doc.token === undefined) { | ||
| return callback(email, null); | ||
| throw new Error('Token not available'); | ||
| } | ||
|
|
||
|
|
||
| return callback(email_doc.address, email_doc.token); | ||
| return {address: email_doc.address, token: email_doc.token}; | ||
| }; |
There was a problem hiding this comment.
sendVerificationToken was changed from a callback-style API to an async method that throws/returns values. This is a breaking API change for library consumers and also diverges from the documented usage. Consider supporting both forms (e.g., accept an optional callback) or bumping the major version and updating documentation accordingly.
| await account.save(); | ||
| console.log('no error'); | ||
| } catch (error) { | ||
| console.log('error' + error.message); |
There was a problem hiding this comment.
The before hook catches account.save() errors but only logs them and continues. If the save fails, the suite will proceed with an empty DB and produce misleading downstream failures. Re-throw the error (or otherwise fail the hook) so test setup failures stop the run.
| console.log('error' + error.message); | |
| console.log('error' + error.message); | |
| throw error; |
|
|
||
| await account.setPrimaryEmail('mail@ranjitghimire.com.np'); | ||
| var email3 = account.getPrimaryEmail(); | ||
| email3.should.eql('mail@ranjitghimire.com.np'); |
There was a problem hiding this comment.
Several tests rely on state created by earlier tests (e.g., sendVerificationToken('iamritghimire@gmail.com') assumes that email was added and persisted by a previous test). This makes the suite order-dependent and brittle (fails under reordering/parallelization). Prefer arranging the required account/email state inside each test (or in a shared beforeEach) and explicitly saving after mutations like addEmail().
| email3.should.eql('mail@ranjitghimire.com.np'); | |
| email3.should.eql('mail@ranjitghimire.com.np'); | |
| await account.save(); |
|
|
||
| strategy: | ||
| matrix: | ||
| node-version: [18.x, 20.x, 22.x] |
There was a problem hiding this comment.
The workflow tests Node 18, but mongoose@9 (and its mongodb dependency) declares engines: { node: ">=20.19.0" } in the updated lockfile. The Node 18 jobs are expected to fail installs or runtime. Remove Node 18 from the matrix (or downgrade mongoose), and consider pinning to explicit supported Node versions.
| node-version: [18.x, 20.x, 22.x] | |
| node-version: [20.x, 22.x] |
| "diff": "^8.0.3", | ||
| "serialize-javascript": "^7.0.5" |
There was a problem hiding this comment.
overrides uses caret ranges (e.g., diff: ^8.0.3, serialize-javascript: ^7.0.5), which can float to new minor/major releases and make installs less deterministic. If these overrides are for security fixes, consider pinning exact versions and adding a brief note (e.g., in a comment or changelog) about why they’re needed.
| "diff": "^8.0.3", | |
| "serialize-javascript": "^7.0.5" | |
| "diff": "8.0.3", | |
| "serialize-javascript": "7.0.5" |
|
@copilot fix the comments in this pull request itself |
…comparison, test before hook, workflow matrix, package overrides Agent-Logs-Url: https://github.com/amritghimire/express-user-model/sessions/3edb0836-f424-46f5-b377-6c49b3a2eb3b Co-authored-by: amritghimire <16842655+amritghimire@users.noreply.github.com>
All reviewer comments addressed in commit
|
lib/basic.js: pre-save hook usesoptions.usernameFielddynamically with type guard instead of hardcodedthis.usernamelib/email.js:setPrimaryEmailnow compares two strings (currentPrimaryEmail !== email) to avoid always-triggering savetest/test.js:beforehook re-throwssave()errors to surface setup failures clearlytest/test.js: "Add an email to user" addsawait account.save()after mutations for deterministic test state.github/workflows/test.yml: remove Node 18.x (mongoose@9 requires Node >= 20)package.json: pin exact versions fordiffandserialize-javascriptoverrides