Loopback: Problems extending User model with loopback v3.3.0

Created on 21 Feb 2017  路  19Comments  路  Source: strongloop/loopback

Description/Steps to reproduce

  1. Scaffold a new loopback project using the CLI (eg. lb)
  2. In server/model-config.json find the builtin model reference:
  "User": {
    "dataSource": "db"
  },

and replace with the custom model reference:

  "user": {
    "dataSource": "db"
  },

the user name for the custom model as it is used as seen in other examples, I will retest with a custom model name as well

  1. Create a new model setting User as the base value (for example see loopback-example-access-control), an overly simplified example might look like:
{
  "name": "user",
  "base": "User",
  "hidden": ["password", "verificationToken"]
}
  1. Create a boot script (eg. /server/boot/setup.js) with the following
'use strict';

module.exports = function(app) {
  var User = app.models.user;

  User.create({
    username: 'foo',
    email: '[email protected]',
    password: 'changeme'
  }, (err, user) => {
    if (err) {
      throw err;
    }

    user.email = '[email protected]';
    user.save((saveErr, user) => {
      if (saveErr) {
        throw saveErr;
      }
      // won't make it here, error is thrown
      // TypeError: Cannot read property 'polymorphic' of undefined
    });
  });
};
  1. Run the loopback application

The following will produce an error such as:

/code/loopback-sample/node_modules/loopback/common/models/user.js:695
    var isRelationPolymorphic = relatedUser.polymorphic && !relatedUser.modelTo;
                                           ^

TypeError: Cannot read property 'polymorphic' of undefined
    at Function.User._invalidateAccessTokensOfUsers (/code/loopback-sample/node_modules/loopback/common/models/user.js:695:44)
    at invalidateOtherTokens (/code/loopback-sample/node_modules/loopback/common/models/user.js:927:15)
    at notifySingleObserver (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:104:22)
    at /code/loopback-sample/node_modules/async/dist/async.js:3025:16
    at replenish (/code/loopback-sample/node_modules/async/dist/async.js:881:17)
    at /code/loopback-sample/node_modules/async/dist/async.js:885:9
    at eachLimit$1 (/code/loopback-sample/node_modules/async/dist/async.js:3114:22)
    at Object.<anonymous> (/code/loopback-sample/node_modules/async/dist/async.js:917:16)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:101:11)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:99:49)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:99:49)
    at doNotify (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:99:49)
    at Function.ObserverMixin._notifyBaseObservers (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:122:5)
    at Function.ObserverMixin.notifyObserversOf (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:97:8)
    at Function.ObserverMixin._notifyBaseObservers (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:120:15)
    at Function.ObserverMixin.notifyObserversOf (/code/loopback-sample/node_modules/loopback-datasource-juggler/lib/observer.js:97:8)

Expected result

The user should save without any errors.

Additional information

This example is not an issue for v2.x or v3.x up to 3.2.1, this only surfaced with v3.3.0.

Based on the outcome of issue 103 for loopback-example-access-control this may not actually be an issue at all and just needs some updates to the docs?

For v3.3.0 the bug is _not_ present when you have a custom model with User as base and retain the builtin User model reference in model-config.json. If it became a requirement to always have User model in situations like this the documentation should be updated accordingly. It should also then be discouraged for using builtin models as the base for custom models with the same name (eg. User builtin vs user custom).

bug major team-asteroid
Source
picture greaterweb
👍1

Most helpful comment

Based on my comment above, the first set of instructions should be corrected to the following:

So when setting up a custom User model while using the built-in AccessToken model, one needs to re-configure AccessToken's "belongsTo" relation to target the new User model. This can be configured in server/model-config.json as follows (assuming the new User model is called user):

{
  "AccessToken": {
    "dataSource": "db",
    "public": false,
    "relations": {
      "user": {
        "type": "belongsTo",
        "model": "user",
        "foreignKey": "userId"
      }
    }
  }
}
picture bajtos on 24 Feb 2017
👍9

All 19 comments

The offending code is introduced by https://github.com/strongloop/loopback/commit/9fe084fffd1d431a9af94b498003b725e4e73abe.

Does your AccessToken model has a relation to User so that https://github.com/strongloop/loopback/commit/9fe084fffd1d431a9af94b498003b725e4e73abe#diff-5d81af95449574544e7317f3eda2badeR693 won't have undefined relatedUser?

raymondfeng picture raymondfeng on 21 Feb 2017

@raymondfeng thank you for the prompt response!

I've updated the original ticket to include a few more details on steps to reproduce.

To answer your question, with my example I've not defined any custom relationships with the builtin AccessToken model. I'll create a quick repo with sample code and see if I can resolve the issue through configuration.

greaterweb picture greaterweb on 21 Feb 2017

In your case, built-in AccessToken has a belongsTo relation user to the built-in User model. Since you remove User from model-config.json, the relation is not resolved for AccessToken. You probably need to create a subclass for AccessToken with a user relation to your new user model.

raymondfeng picture raymondfeng on 21 Feb 2017

I guess this opens the flood gates to architecture/design discussions related to extending the User model (I know it's something your team has discussed at length before). What is the best way to allow developers to extend the builtin User model and retain all of the "out of the box" benefits such as wiring to other models such as AccessToken and integration with components such as loopback-component-passport? That's an open question in general, certainly not one expected to be resolved in the scope of this ticket ;-)

In the mean time I'll create a sample repo for this ticket which simulates the issue and also provides some options to resolve it.

greaterweb picture greaterweb on 21 Feb 2017

@raymondfeng @superkhau

I've created a sample repo to explore the issue, please see the branches below.

A rather simple user setup boot script has been added to trigger the issue. If you run npm install and then node . on any of the branches the desired output is:

sample user created
 {
  "username": "foo",
  "email": "[email protected]",
  "id": 1
}
sample user saved
 {
  "username": "foo",
  "email": "[email protected]",
  "id": 1
}

Worth noting, I have a project which originally alerted me to this issue. The project has a user model which extends User. I have pretty decent test coverage of the project and only started seeing tests failing when v3.3.0 was released. When I attempt to go the route of the dev/mask-issue approach I actually get more tests failing. This leads me to believe there is room for problems if both the builtin User model and any custom models extending User are present in model-config.json at the same time.

greaterweb picture greaterweb on 21 Feb 2017

cc @bajtos

superkhau picture superkhau on 21 Feb 2017

cc @ebarault who is the author of 9fe084f which introduced this regression.

@greaterweb thank you for such detailed bug report!

I think we should:

  1. Fix the code to handle the case where the user model does not have "hasMany AccessToken" relation set up at all. While this may be a wrong configuration (a problem at user side), we shouldn't be crashing applications after upgrade to a newer LoopBack version.

  2. Report a warning when the user model does not have any "hasMany" relation to AccessToken, the message should either include instructions on how to fix the problem or point to a (new?) doc page containing those instructions.

  3. Modify loopback-workspace project template to scaffold new applications with custom user and accessToken models already prepared for the developers. This way we can also enable "injectOptionsFromRemoteContext" setting for the user model, which is needed to preserve current session when logging out other sessions after and email/password change.

  4. Investigate a bit more the last claim:

    When I attempt to go the route of the dev/mask-issue approach I actually get more tests failing. This leads me to believe there is room for problems if both the builtin User model and any custom models extending User are present in model-config.json at the same time.

Thoughts?

bajtos picture bajtos on 21 Feb 2017

@bajtos with regards to point 4, I can certainly help provide some additional context with more specific examples for tests that once worked which now fail. I'm traveling this week for work so it may be a little delayed but I will try to update the sample repo with some of the test scenarios. A quick look though at the one project I tried the solution from dev/mask-issue, the failing tests seem to be related to AccessTokens and REST api calls. Some snippets from the test results (these are my custom tests which previously passed):

  6) User Model Users REST should allow the user to edit their username:
     Error: expected 200 "OK", got 401 "Unauthorized"

  7) User Model Users REST should allow the user to edit their email:
     Error: expected 200 "OK", got 401 "Unauthorized"

  8) User Model Users REST should allow the user to edit their password:
     Error: expected 200 "OK", got 401 "Unauthorized"

 9) User Model Users REST should allow the user to recover their password:
     Uncaught AssertionError: expected NaN to equal '6ff48030-f7d8-11e6-ad75-814a1514de9b'

Without looking too closely at this the problem is likely related to the fact I have a custom model user which extends User, when both user and User are present in the model-config.json. This is essentially a naming conflict where loopback.models.User represents an instance of my custom model when User _is not included_ in model-config.json but it then represents the builtin User model when that _is included_.

greaterweb picture greaterweb on 22 Feb 2017

I know there are pretty good docs for managing users but I think these need to be extended to provide specific guidance on scenarios when you extend the User model.

In my use case, I extend the base model for a variety of reasons, here are some:

  • I have custom properties (eg. firstName, lastName)
  • I override instance methods such as verify
  • I apply custom logic to email field validation

Ideally I think most devs in my situation are looking for any custom model extending the base User model to gain all of the out of the box benefits of that model with little configuration (eg. I don't want to recreate all the ACL and relationships). Essentially I want my custom model to be a drop in replacement for User and everything works the same without thinking twice about it.

greaterweb picture greaterweb on 22 Feb 2017

I know there are pretty good docs for managing users but I think these need to be extended to provide specific guidance on scenarios when you extend the User model.

+1

@greaterweb would you like to contribute this improvement to our documentation? I can help you to fill missing pieces once you have something to start with. See http://loopback.io/doc/en/contrib/doc-contrib.html to get started.

In my use case, I extend the base model for a variety of reasons

All your reasons make perfect sense to me.

Ideally I think most devs in my situation are looking for any custom model extending the base User model to gain all of the out of the box benefits of that model with little configuration (eg. I don't want to recreate all the ACL and relationships). Essentially I want my custom model to be a drop in replacement for User and everything works the same without thinking twice about it.

+1

There are quite many things that models don't inherit right now, see these related issues:

Many of these changes would break backwards compatibility :(

I think our best option for the short-term is to modify our project template in loopback-workspace to come with a subclassed User and AccessToken models that are already correctly preconfigured.

bajtos picture bajtos on 23 Feb 2017

@bajtos I wouldn't object to assisting with the documentation but to be perfectly honest, I still think I need a little clarification as to what the actual best practices are for extending the User model.

For the Loopback project I cite in above comments with the failing tests, all now seem to passing once again when using the fix/invalidate-tokens branch. The passing tests on the surface seem to be a good thing but what concerns me is that I don't explicitly define any relationships with the AccessTokens model. The other part of the equation is my custom model name is user, does that somehow influence the situation? Worth noting again, I do _not_ include the User model reference in model-config.json, only that of my custom model user. I'm curious though, with all things equal if I were to swap out my custom user model name with Account would all of the tests pass or fail?

As time permits I'll do some exploration on this and keep you posted.

greaterweb picture greaterweb on 24 Feb 2017

I still think I need a little clarification as to what the actual best practices are for extending the User model.

By default, the built-in User and AccessToken models have the following relations configured:

The first relation is actively used inside methods like User.login. AFAICT, the other relation was not used anywhere in LoopBack core until #2971 was landed; I guess the relation was configured for completeness only.

So when setting up a custom User model (the model name should not matter at all - both user and Account should behave the same, at least I believe so), one should re-configure both relations:

  • user (or Account) needs hasMany relation to AccessToken, this can and should be configured in common/models/user.json (or common/models/account.json) file
  • AccessToken needs belongsTo relation to user (or Account). I believe this can be configured in server/model-config.json.

Alternatively, the app can create a custom access-token model too, let's call it accesToken. In that case the instructions become:

  • user needs hasMany relation to accessToken, configured in common/models/user.json
  • accessToken needs belongsTo relation to user, configured in common/models/accessToken.json
  • The token middleware should be configured to use accessToken as the model. I think this may not be necessary as the middleware should be able to pick the subclassed accessToken model automatically.

(I am typing this from my head, right now I don't have bandwidth to write a sample app to verify. There may be mistakes, sorry for that!)

In #2971, @ebarault added support for polymorphic "belongsTo" relation where a single AccessToken model belongs to one of multiple User models. The setup instructions would be slightly different in that case.

bajtos picture bajtos on 24 Feb 2017

While implementing the warning message, I learned few more interesting facts:

Model relations are inherited. If the application has custom user model but uses the built-in AccessToken model, the relation "user hasMany accessTokens" will be configured correctly, no extra work is needed in user definition.

The problem starts when the app is extending both User and AccessToken models. In that case, the inherited relations are incorrectly pointing to base models, i.e. user.accessTokens points to the built-int AccessToken model instead of the custom model used by the app. As a result, login method fails with Uncaught TypeError: Cannot read property 'create' of undefined when calling user.accessTokens.create().

bajtos picture bajtos on 24 Feb 2017

Based on my comment above, the first set of instructions should be corrected to the following:

So when setting up a custom User model while using the built-in AccessToken model, one needs to re-configure AccessToken's "belongsTo" relation to target the new User model. This can be configured in server/model-config.json as follows (assuming the new User model is called user):

{
  "AccessToken": {
    "dataSource": "db",
    "public": false,
    "relations": {
      "user": {
        "type": "belongsTo",
        "model": "user",
        "foreignKey": "userId"
      }
    }
  }
}
bajtos picture bajtos on 24 Feb 2017
👍9

Thank you, @bajtos, very helpful information!

As mentioned in the misconfigured access token PR something seems to be breaking the rules a bit in the custom implementation for my personal project. I might try to prune out some of the business logic and publish a version of the loopback backend for review.

I'll investigate a bit further and report back.

greaterweb picture greaterweb on 26 Feb 2017
👍1

Worked for me as well. I had an error when updating the extended usermodels email.

 Uncaught TypeError: Cannot read property 'polymorphic' of undefined
  at Function.User._invalidateAccessTokensOfUsers (xx\node_modules\loopback\common\models\user.js:695:44)
barocsi picture barocsi on 27 Feb 2017
  • Modify loopback-workspace project template to scaffold new applications with custom user and accessToken models already prepared for the developers.
  • Investigate a bit more the last claim:

I'll defer these two tasks for later.

@greaterweb if you manage to trim down your app to something that can be shared, then please open a new issue.

bajtos picture bajtos on 3 Mar 2017

I have the similar situation with extend the user model, where can I upload this ?
Regards
Eduardo

emazzu picture emazzu on 20 Oct 2017

Thanks, @bajtos
Your solution for model-config.json really fixed my long-term trouble!!!

suisun2015 picture suisun2015 on 7 Sep 2018
😄1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

nmklong picture nmklong  路  3Comments
devotox picture devotox  路  4Comments
ImanMh picture ImanMh  路  4Comments
crandmck picture crandmck  路  3Comments
htmlauthor picture htmlauthor  路  3Comments