fix: Accounts.config({ collection }) now properly updates Meteor.users and creates indexes#14272
Open
dupontbertrand wants to merge 6 commits intometeor:develfrom
Open
fix: Accounts.config({ collection }) now properly updates Meteor.users and creates indexes#14272dupontbertrand wants to merge 6 commits intometeor:develfrom
dupontbertrand wants to merge 6 commits intometeor:develfrom
Conversation
…s and creates indexes Fixes meteor#12610. When calling Accounts.config({ collection }) after startup, three issues prevented it from working correctly: 1. The comparison logic in config() used name equality, which failed when passing a different Mongo.Collection instance with the same name 2. Meteor.users was assigned once at module load and never updated 3. Indexes and allow rules were not created on the replacement collection The fix adds a _onUsersCollectionChanged() hook called by config() when the collection changes, overridden in AccountsServer (updates Meteor.users + runs setupUsersCollection) and AccountsClient (updates Meteor.users).
Expand JSDoc for the collection parameter with behavior details and examples. Add a "Using a custom users collection" section to the Accounts guide explaining use cases, code examples, and the Meteor settings alternative.
✅ Deploy Preview for v3-migration-docs canceled.
|
✅ Deploy Preview for v3-meteor-api-docs canceled.
|
Add onUsersCollectionChanged() registration API so that sub-packages
can re-run their index setup when Accounts.config({ collection }) swaps
the users collection. Update accounts-password, accounts-oauth, and
accounts-passwordless to register their index creation callbacks.
Also fix _onUsersCollectionChanged overrides to call super.
Clean up test names and add assertions for accounts-password indexes (password reset token, email verification token) on custom collections.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds support for configuring a custom users collection via Accounts.config({ collection }); keeps Meteor.users and account subsystems synchronized when the collection changes; runs/refreshes required indexes for accounts, oauth, password, and passwordless packages on collection change; documents usage and adds tests for behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/accounts-base/accounts_tests.js (1)
781-833: Make these collection-swap tests self-cleaning and deterministic.These tests mutate the global
Accounts.userssingleton, so if an assertion throws before the final reset, later tests will run against the wrong collection. Also, the fixed 2-second sleep on Line 819 will stay flaky under slower CI. Wrapping each swap intry/finallyand polling for the expected indexes would make this block much more reliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/accounts-base/accounts_tests.js` around lines 781 - 833, These tests mutate the global Accounts.users and can leave state dirty or be flaky due to the fixed sleep; wrap each Tinytest.addAsync body that calls Accounts.config(...) in a try/finally that always restores the original collection (saved as origCollection) so cleanup runs even on assertion failure, and replace the fixed Meteor.setTimeout(2000) wait in the "indexes created" test with a polling loop that repeatedly calls Accounts.users.rawCollection().indexes() (or rawCollection = Accounts.users.rawCollection(); await rawCollection.indexes()) until the expected index keys ('username' and 'emails.address') appear or a short overall timeout elapses, failing the test if timeout is reached; ensure references to Accounts.config, Accounts.users, Meteor.users, and the customCollection/remoteUsers variables are used to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/accounts-base/accounts_common.js`:
- Around line 300-307: The identity check for swapping user collections in the
options.collection branch is asymmetric and can miss equivalence when one side
is a Mongo.Collection instance and the other is a string name; update the logic
in the block that touches options.collection, this.users, _initializeCollection,
and _onUsersCollectionChanged so it treats equality as either the same instance
or the same collection name: compute isNewCollection by checking
(options.collection === this.users) OR (options.collection instanceof
Mongo.Collection && this.users instanceof Mongo.Collection && options.collection
=== this.users) OR (extractName(options.collection) ===
extractName(this.users)), where extractName returns the _name for Collection
instances or the string itself for names; if none match, proceed to call
this._initializeCollection(options) and this._onUsersCollectionChanged().
In `@packages/accounts-base/accounts_server.js`:
- Around line 1233-1235: The code simply swaps Meteor.users in
_onUsersCollectionChanged but does not retarget existing server bindings
(publications and token observers) that still close over the old collection;
update _onUsersCollectionChanged to not only set Meteor.users and call
setupUsersCollection(this.users) but also migrate server bindings: reinitialize
or rebind the current-user publication and any auth/session token observers to
use this.users (or a collection-getter) by restarting the publication handlers
and token observer logic (the code paths that created the publications around
the current-user pub and the token observers started near the token/session
logic), or alternatively force connected clients to re-authenticate by
invalidating existing sessions; implement this by adding a single migration
helper (e.g., migrateUserBindings or restartUserAuthBindings) and invoke it from
_onUsersCollectionChanged after setupUsersCollection(this.users).
---
Nitpick comments:
In `@packages/accounts-base/accounts_tests.js`:
- Around line 781-833: These tests mutate the global Accounts.users and can
leave state dirty or be flaky due to the fixed sleep; wrap each
Tinytest.addAsync body that calls Accounts.config(...) in a try/finally that
always restores the original collection (saved as origCollection) so cleanup
runs even on assertion failure, and replace the fixed Meteor.setTimeout(2000)
wait in the "indexes created" test with a polling loop that repeatedly calls
Accounts.users.rawCollection().indexes() (or rawCollection =
Accounts.users.rawCollection(); await rawCollection.indexes()) until the
expected index keys ('username' and 'emails.address') appear or a short overall
timeout elapses, failing the test if timeout is reached; ensure references to
Accounts.config, Accounts.users, Meteor.users, and the
customCollection/remoteUsers variables are used to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d38cb3be-8b10-4588-8757-e5f8ba577c7a
📒 Files selected for processing (5)
guide/source/accounts.mdpackages/accounts-base/accounts_client.jspackages/accounts-base/accounts_common.jspackages/accounts-base/accounts_server.jspackages/accounts-base/accounts_tests.js
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/accounts-base/accounts_server.js (1)
1233-1237:⚠️ Potential issue | 🟠 MajorServer publications still reference the original collection after swap.
The publications initialized in
_initServerPublications()(around line 830) destructureconst { users, ... } = this;and close over that variable. After_onUsersCollectionChanged()swapsthis.users, the existing publications continue to query the old collection. The same applies to token observers started via_setLoginToken. This can cause inconsistent behavior where new logins use the new collection but existing sessions query stale data.Consider either:
- Reinitializing the publications when the collection changes
- Having publications reference
this.usersvia a getter rather than a captured variable- Forcing existing connections to re-authenticate
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/accounts-base/accounts_server.js` around lines 1233 - 1237, Publications and token observers capture the original users collection and continue referencing it after _onUsersCollectionChanged swaps this.users; update the code so publications and observers use the current collection or are reinitialized: either (A) change _initServerPublications to avoid destructuring const { users } and instead reference this.users (or a getter) inside publication/observer closures so they always query the live collection, or (B) call _initServerPublications (and re-run _setLoginToken observers) from _onUsersCollectionChanged after setupUsersCollection(this.users). Ensure Meteor.users is updated first, then recreate or update observers to point to the new collection to prevent stale queries.
🧹 Nitpick comments (1)
packages/accounts-base/accounts_tests.js (1)
812-813: Hardcoded delay for async index creation may cause flaky tests.The 2000ms
setTimeoutis a best-effort wait that could fail on slower CI environments or succeed prematurely. Consider polling for index existence or increasing the timeout with a note explaining the rationale.Alternative: poll for index existence
- // Allow time for async index creation - await new Promise(resolve => Meteor.setTimeout(resolve, 2000)); + // Poll for index creation (max 5 seconds) + const maxAttempts = 10; + for (let i = 0; i < maxAttempts; i++) { + const indexes = await Accounts.users.rawCollection().indexes(); + if (indexes.some(idx => 'username' in idx.key)) break; + await new Promise(resolve => Meteor.setTimeout(resolve, 500)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/accounts-base/accounts_tests.js` around lines 812 - 813, Replace the hardcoded wait (the awaited Promise that uses Meteor.setTimeout) with an explicit poll that checks for the index existence instead of sleeping; implement a helper (e.g., pollIndexExists(collection, indexName, timeoutMs = 10000, intervalMs = 200)) that repeatedly calls collection.rawCollection().indexExists(indexName) (or collection.rawCollection().indexes()/getIndexes) until it returns true or the timeout elapses, and use await pollIndexExists(...) in place of the current await new Promise(resolve => Meteor.setTimeout(resolve, 2000)); ensure the helper throws on timeout so the test fails deterministically rather than flaking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/accounts-base/accounts_server.js`:
- Around line 1233-1237: In _onUsersCollectionChanged replace the
fire-and-forget setupUsersCollection(this.users).then() pattern with proper
error handling: call setupUsersCollection(this.users) and attach a .catch that
logs the error (or reports/handles it) and/or rethrows as appropriate so
index-creation failures are not silently discarded; ensure Meteor.users
assignment and the subsequent super._onUsersCollectionChanged() still occur or
are sequenced correctly around the handled promise, referencing
_onUsersCollectionChanged, setupUsersCollection, Meteor.users, and
super._onUsersCollectionChanged to locate the change.
In `@packages/accounts-oauth/oauth_common.js`:
- Around line 24-31: The loop inside Accounts.onUsersCollectionChanged currently
calls users.createIndexAsync for each service but does not await or handle
errors; update this to use a helper wrapper (similar to registerService pattern)
that awaits the Promise and logs or handles failures: ensure you call and await
users.createIndexAsync(`services.${name}.id`, {unique: true, sparse: true})
inside a try/catch, and on error call the same error handling/logging used by
registerService (or a shared helper) so index creation failures are not silently
ignored; reference Accounts.onUsersCollectionChanged, services, and
createIndexAsync when implementing the change.
In `@packages/accounts-password/password_server.js`:
- Around line 1314-1315: The onUsersCollectionChanged callback currently calls
createPasswordIndexes(users).then() without error handling, which swallows
rejections; update the Accounts.onUsersCollectionChanged registration to handle
promise failures by attaching a .catch(...) to createPasswordIndexes(users) (or
use async/await with try/catch) and log or surface the error via the same
logging mechanism used elsewhere (e.g., processLogger/error logger) so failures
creating indexes for the new collection are not silently ignored; ensure the
initial await createPasswordIndexes(Meteor.users) remains unchanged but consider
consistent error handling for both calls (reference createPasswordIndexes and
Accounts.onUsersCollectionChanged).
In `@packages/accounts-passwordless/passwordless_server.js`:
- Around line 249-250: The onUsersCollectionChanged callback is calling
createPasswordlessIndexes(users).then() with no error handling which silences
promise rejections; update the Accounts.onUsersCollectionChanged handler to
properly handle failures by attaching a .catch(...) to log or surface the error
(or convert the callback to async and await createPasswordlessIndexes(users)
inside a try/catch), referencing the createPasswordlessIndexes function and the
Accounts.onUsersCollectionChanged call so failures during index creation are not
silently ignored.
---
Duplicate comments:
In `@packages/accounts-base/accounts_server.js`:
- Around line 1233-1237: Publications and token observers capture the original
users collection and continue referencing it after _onUsersCollectionChanged
swaps this.users; update the code so publications and observers use the current
collection or are reinitialized: either (A) change _initServerPublications to
avoid destructuring const { users } and instead reference this.users (or a
getter) inside publication/observer closures so they always query the live
collection, or (B) call _initServerPublications (and re-run _setLoginToken
observers) from _onUsersCollectionChanged after
setupUsersCollection(this.users). Ensure Meteor.users is updated first, then
recreate or update observers to point to the new collection to prevent stale
queries.
---
Nitpick comments:
In `@packages/accounts-base/accounts_tests.js`:
- Around line 812-813: Replace the hardcoded wait (the awaited Promise that uses
Meteor.setTimeout) with an explicit poll that checks for the index existence
instead of sleeping; implement a helper (e.g., pollIndexExists(collection,
indexName, timeoutMs = 10000, intervalMs = 200)) that repeatedly calls
collection.rawCollection().indexExists(indexName) (or
collection.rawCollection().indexes()/getIndexes) until it returns true or the
timeout elapses, and use await pollIndexExists(...) in place of the current
await new Promise(resolve => Meteor.setTimeout(resolve, 2000)); ensure the
helper throws on timeout so the test fails deterministically rather than
flaking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11598dbe-8c8c-464e-86a5-4f6b1e84a4cb
📒 Files selected for processing (7)
packages/accounts-base/accounts_client.jspackages/accounts-base/accounts_common.jspackages/accounts-base/accounts_server.jspackages/accounts-base/accounts_tests.jspackages/accounts-oauth/oauth_common.jspackages/accounts-password/password_server.jspackages/accounts-passwordless/passwordless_server.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/accounts-base/accounts_client.js
- Fix asymmetric collection comparison: detect round-trip from instance back to string config by checking if the previous collection option was a Mongo.Collection instance - Add .catch() error handling to all fire-and-forget index creation callbacks (accounts-base, accounts-password, accounts-oauth, accounts-passwordless) to avoid silent unhandled rejections
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/accounts-base/accounts_common.js (1)
230-230: JSDoc should clarify thatcollectionis a startup-time option.The documentation says "Can be called after startup" but per learnings,
Accounts.config({ collection })is intentionally a startup-time API. It's not designed for hot-swapping while clients are actively connected. Publications and token observers created before the swap resolve on client reconnect.Consider adding a note like: "Should be configured early in the startup sequence, before clients connect."
Based on learnings: "Accounts.config({ collection }) is intentionally a startup-time API. It is not designed to hot-swap the users collection while clients are actively connected."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/accounts-base/accounts_common.js` at line 230, Update the JSDoc for Accounts.config to make clear that the collection option is a startup-time setting: change the description around the options.collection parameter (and the example usage) to state that Accounts.config({ collection }) must be set early in the startup sequence (before clients connect) and is not supported for hot-swapping the users collection at runtime; reference the Accounts.config API and the collection option name so readers can find the implementation and examples.packages/accounts-passwordless/passwordless_server.js (1)
249-249: Minor inconsistency: Startup initialization pattern.This file uses
Meteor.startup(() => createPasswordlessIndexes(...))whilepassword_server.jsuses top-levelawait createPasswordIndexes(...). Both work, but the top-level await pattern is slightly cleaner for modern Meteor.Not a blocking issue—just noting for consistency if you prefer to align them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/accounts-passwordless/passwordless_server.js` at line 249, Startup uses Meteor.startup wrapper instead of the project's preferred top-level await pattern—replace the Meteor.startup(() => createPasswordlessIndexes(Meteor.users)) call with a direct top-level await createPasswordlessIndexes(Meteor.users) so it matches the pattern used in password_server.js; keep the same createPasswordlessIndexes symbol and ensure the file is loaded as a module/runtime that supports top-level await (adjust module settings if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/accounts-base/accounts_common.js`:
- Line 230: Update the JSDoc for Accounts.config to make clear that the
collection option is a startup-time setting: change the description around the
options.collection parameter (and the example usage) to state that
Accounts.config({ collection }) must be set early in the startup sequence
(before clients connect) and is not supported for hot-swapping the users
collection at runtime; reference the Accounts.config API and the collection
option name so readers can find the implementation and examples.
In `@packages/accounts-passwordless/passwordless_server.js`:
- Line 249: Startup uses Meteor.startup wrapper instead of the project's
preferred top-level await pattern—replace the Meteor.startup(() =>
createPasswordlessIndexes(Meteor.users)) call with a direct top-level await
createPasswordlessIndexes(Meteor.users) so it matches the pattern used in
password_server.js; keep the same createPasswordlessIndexes symbol and ensure
the file is loaded as a module/runtime that supports top-level await (adjust
module settings if needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2102b293-3561-49da-a6ed-8e1aeb9f7e6a
📒 Files selected for processing (5)
packages/accounts-base/accounts_common.jspackages/accounts-base/accounts_server.jspackages/accounts-oauth/oauth_common.jspackages/accounts-password/password_server.jspackages/accounts-passwordless/passwordless_server.js
Add JSDoc guidance that the collection option should be configured early in the startup sequence, before clients connect, consistent with other Accounts.config() options.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12610 (yes, finally) 🥳
When calling Accounts.config({ collection }) after startup, three issues prevented it from working correctly:
The fix adds a _onUsersCollectionChanged() hook called by config() when the collection changes, overridden in AccountsServer (updates Meteor.users + runs setupUsersCollection) and AccountsClient (updates Meteor.users). This hook also triggers callbacks registered via the new onUsersCollectionChanged(callback) API, which accounts-password, accounts-oauth, and accounts-passwordless now use to recreate their indexes on the new collection.
Summary by CodeRabbit
New Features
Documentation
Tests