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.

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

Meteor.users.find() in core without field specifier could be a performance issue #10469

Closed
wildhart opened this issue Feb 25, 2019 · 11 comments
Closed
Labels
confirmed Project:Accounts (in user apps)

Comments

@wildhart
Copy link
Contributor

@wildhart wildhart commented Feb 25, 2019

Meteor core contains several instances of Meteor.users().find() and Meteor.users().findOne() without a field specifier in situations where only certain fields are required.

If a developer stores custom data on the Meteor.users() collection (as recommended in the Guide) then this will cause unnecessary data transfer between the db server and the meteor server. This could be significant if lots of custom data is stored in the Meteor.users collection.

These examples definitely only need specific fields:

These may need the full user object to pass to the callbacks, I'm not sure:

This one has a definite error by omitting the fields: prefix:

There will probably be other instances in other files/packages - I haven't done a thorough search.

@wildhart
Copy link
Contributor Author

@wildhart wildhart commented Feb 25, 2019

The occurrences of .find()/.findOne() at accounts_server L191, L320 are L401 are only really used to pass a user object to the onLogin, onLoginFailure, onLogout and validateLoginAttempt callbacks.

Limiting these to certain fields could be a breaking change for some users. I think a way could be found of specifying which fields are required (or not required) by these callbacks (maybe similar to the undocumented addAutopublishFields).

Either that or the documentation at "Custom data about users" should include a warning about the full user object being retrieved from the db upon every login/logout/refresh so the developer can make an informed decision about keeping large data sets in a separate collection.

Note that the full user data retrieved for the onLogout callback at L191 is retrieved even if there are no callbacks registered. This could be improved with:

_successfulLogout(connection, userId) {
    if (Object.keys(this._onLogoutHook.callbacks).length===0) {
        return;
    }
    const user = userId && this.users.findOne(userId);
    this._onLogoutHook.each(callback => {
      callback({ user, connection });
      return true;
    });
  };

But I'm not sure if .callbacks is intended to be an exposed part of the Hooks class.

@wildhart
Copy link
Contributor Author

@wildhart wildhart commented Feb 25, 2019

Here's more:

These would need getUserById() to take an additional fields parameter.

Ideally Accounts.findUserByUsername() and Accounts.findUserByEmail() should take an optional parameter to specify which fields are required, as should Meteor.user() as already covered by meteor/meteor-feature-requests#82

@wildhart
Copy link
Contributor Author

@wildhart wildhart commented Sep 5, 2019

I disagree that this impacts few - I suspect that most Meteor apps store some amount of data on the user object so this will be a minor performance bug for many and a major performance bug to few apps.

Following a short discussion in the forum I am keen to create a PR to fix this. However, doing it properly would require some new public API which would be backwards compatible but would need to be documented and maintained. So before I do that I would like to gauge whether or not new API would be accepted and if so, what format it should take.

Therefore I would be grateful if @klaussner or @benjamn could comment before I start.

Proposed API

The new API would involve an optional additional parameter to Meteor.user(), Accounts.findUserByEmail(), Accounts.findUserByUsername(), Accounts.onLogin(), Accounts.onLoginFailure() and Accounts.onLogout() to specify which fields are required.

If no parameter is specified for Meteor.user(), Accounts.findUserByEmail(), Accounts.findUserByUsername() the full user object is fetched, to maintain backwards compatibility.

Callback default fields

However, for the callbacks I suggest that by default the fields are limited to the standard fields username, emails, createdAt, profile, services - the reason for this is because this data is fetched within accounts-server.js even if no callbacks are registered. However, this could be a breaking change for some users who register a callback expecting their custom data to be there, but don't notice any errors when it suddenly isn't after the upgrade.

A solution to this could be to allow the user to opt-in to limited fields on the callback handlers, but this opt-in would require additional API. For example, a user could set Accounts.limitCallBackFields = true or Accounts.defaultCallBackFields = {profile: 1, emails: 1} to opt-in, otherwise the full object is returned. This would allow users like me who store lots of data on the user object and notice performance problems to opt-in, leaving everyone else opted out.

Additional parameter format

There are various options for the format of the additional parameter:

  1. {profile: 1, emails: 1} or {myBigArray: 0}
  2. {fields: {profile: 1, emails: 1}} or {fields: {myBigArray: 0}}
  3. "profile" or ["profile", "emails"]
  4. "profile,emails"

Options 1 and 2 are more consistent with standard mongo parameters.

Option 1 is compatible with the user-extra package by @mitar and feature request #82.

Option 3 is compatible with the userCache package by @msavin.

Or it may be necessary to support at least both of options 1 and 3 to maintain compatibility with those other packages.

Thoughts?

Optional logging to find unlimited queries

I would also like to add some optional logging to help the user identify code or packages which call (e.g.) Meteor.user() without any field specifiers. They could then fix their own code or ask package authors to improve their packages.

This could simply be Accounts.warnOnFetchFullUserObject = true which would console.warn() any function calls without field specifiers, providing a stack trace to help locate the code.

Tests and documentation

My PR would include updated tests and documentation.

[edit: typos]

@stale
Copy link

@stale stale bot commented Dec 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 10, 2019
@mitar
Copy link
Contributor

@mitar mitar commented Dec 10, 2019

Still relevant.

@stale stale bot removed the stale-bot label Dec 10, 2019
@wildhart
Copy link
Contributor Author

@wildhart wildhart commented Dec 10, 2019

I'm still keen to create a PR for this but would like guidance on the new API. @filipenevola, would you be able to get someone to respond to my comment above?

@filipenevola filipenevola added the confirmed label Dec 11, 2019
@filipenevola
Copy link
Member

@filipenevola filipenevola commented Dec 11, 2019

@wildhart I agree with your ideas, go ahead with a PR.

It's very important to keep backward compatible (100%).

@mitar
Copy link
Contributor

@mitar mitar commented Dec 11, 2019

@filipenevola I think the question is which of possible APIs to use here, see options 1-3 above.

@filipenevola
Copy link
Member

@filipenevola filipenevola commented Dec 12, 2019

I prefer 2 as it keeps the fields as in the options object on MongoDB and it would be easy to add new options without introducing breaking changes.

@wildhart
Copy link
Contributor Author

@wildhart wildhart commented Dec 12, 2019

Thanks @fillipenevola, I'll get started on a PR...

wildhart added a commit to wildhart/meteor that referenced this issue Dec 13, 2019
wildhart added a commit to wildhart/meteor that referenced this issue Dec 16, 2019
wildhart added a commit to wildhart/meteor that referenced this issue Dec 17, 2019
wildhart added a commit to wildhart/meteor that referenced this issue Dec 18, 2019
wildhart added a commit to wildhart/meteor that referenced this issue Dec 18, 2019
including new Meteor.user() options parameter and Accounts.config({defaultFieldSelector...})
filipenevola added a commit that referenced this issue Feb 25, 2020
Limit fetching full user objects (performance) Fixes #10469
@filipenevola
Copy link
Member

@filipenevola filipenevola commented Feb 25, 2020

PR #10818 is merged. It will be available on 1.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed Project:Accounts (in user apps)
Projects
None yet
Development

No branches or pull requests

4 participants