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

A better way for customizing built-in models and models from components #397

Closed
bajtos opened this issue Jul 24, 2014 · 46 comments
Closed
Assignees

Comments

@bajtos
Copy link
Member

bajtos commented Jul 24, 2014

Consider an application with a "User" model that has few extra properties or methods compared to the built-in User model.

At the moment, the recommended solution is to create a user model extending the built-in User model, which is rather silly.

What we would like to have instead

Support redefine:true as an alternative to base: "User". A redefined "User" model is defined in common/models/user.json and scaffolded by yo loopback. This will allow redefinition of common models in the server facet.

In the long term (loopback 3.x), we were discussing a mixin-based approach, where User would be just a thin wrapper mixing in login/logout and other features provided by the current User implementation.


Original issue description

Possible solutions:

  1. Implement namespaces, move all built-in models to loopback namespace. Application models can live in the global namespace, i.e. User extends loopback.User.
  2. Implement means allowing applications to modify the built-in models, so that they can use them directly.

See also loopbackio/loopback-datasource-juggler#119

A probably related problem: the canonical project layout has three facets - "common", "server", "client". In many cases it is needed to provide different implementation of model methods on the server. The current approach is to create a new "server" model extending the shared "common" model and make any changes in this subclass. While this keeps the design clean, it's also rather cumbersome.

@bajtos
Copy link
Member Author

bajtos commented Aug 12, 2014

@altsang this issue is still present and we should address it. I am reopening the issue and also added an item to Roadmap brainstorming.

@bajtos bajtos reopened this Aug 12, 2014
@clark0x
Copy link
Contributor

clark0x commented Aug 12, 2014

@bajtos How about this workaround: manually create a BaseUser model extending User model, just before app is created. Then create a User model extending BaseUser model via JSON file.

Detail:

// server/server.js
var loopback = require('loopback');
var boot = require('loopback-boot');

loopback.createModel('BaseUser', {}, { base: 'User' });

var app = module.exports = loopback();
....


// common/user.json
{
  "name": "User",
  "plural": "users",
  "base": "BaseUser",
  ...
}

@bajtos
Copy link
Member Author

bajtos commented Aug 12, 2014

I don't think that solves the problem. As @raymondfeng wrote in loopbackio/loopback-datasource-juggler#119, model names are unique and models must not be overriden/replaced.

@clark0x
Copy link
Contributor

clark0x commented Aug 13, 2014

@bajtos I realize that this issue is about to discuss a "better way". While I am just finding and leveraging workarounds for the moment. Sorry about this.

BTW: create a user model extending the built-in User model This workaround requires use "user" as model name in all places, use "User" to retrieve model may return the built-in model in some case (loopback.findModel('User') !== loopback.findModel('user'), but app.models['user'] === app.models['User'], check #464).

@fabien
Copy link
Contributor

fabien commented Aug 19, 2014

Here's an idea: given the new dynamic mixin functionality, it could be beneficial to implement some of the core models' functionality as mixins. In general, mixins are to be used when inheritance proves difficult.

For example, instead of:

var Member = modelBuilder.extend('User', { ... });

Do:

var Member = modelBuilder.extend('PersistedModel', { ... });
Member.mixin('User', { ... });
// and also:
var Client = modelBuilder.extend('PersistedModel', { ... });
Client.mixin('User', { ... });

Example mixin: user.js

module.exports = function(Model, options) {
   var fk = inflection.camelize(Model.modelName + '_id', true);
   Model.hasMany('AccessToken', { foreignKey: fk });
   ...
   Model.confirm = function (uid, token, redirect, fn) {
     ...;
   };
   ...
};

@frankcarey
Copy link

This is more than silly, it's a real pain when trying to use the passport stuff. Just to change the User model, I have to extend about 5 other models? Why can't we extend/modify the base model at least in code within models/user.js ? I tried that and it didn't seem to pick up the file when no user.json file was installed as well.

@clark0x
Copy link
Contributor

clark0x commented Sep 3, 2014

@frankcarey Are you using loopback.component-passport? It allows to setup a custom model as User model as long as it's a sub model of User.

And for extending built-in models, I'm currently using the second solution that @bajtos introduced. It's simple and works with loopback.component-passport, but you have to extend User model programmatically. You can create a boot script under server/boot to do that. E.g.:

module.exports = function (app) {
  var User = app.models.User;
  User.defineProperty(...);
  User.validatesFormatOf(...);
  User.hasMany(...);
  User.myLogin = function () {...};
  ...
}

Note the script name, make sure it runs before any other boot scripts.

@frankcarey
Copy link

That seems to be working.. currently just trying to add a relationship of hasMany to User.. why so hard?

Note: to be explicit for others using this technique, I called the script above to /server/boot/0-user-override.js so that it runs before anything else.

UPDATE:
For those looking for the specific code:

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

 // Example to add a relationship to User to some "Other" Model
 User.hasMany(SomeOther, {as: 'others', foreignKey: 'userId'});

 //console.log('override user');
};

@frankcarey
Copy link

@clarkorz THANK YOU! I'd been banging my head against that for a few days.. even started switching to sails instead because I couldn't progress.

@frankcarey
Copy link

It would be helpful to understand WHY we can't just extend the base User model while preserving the name? @clarkorz solution seems to be working well enough for me.

@bajtos
Copy link
Member Author

bajtos commented Sep 12, 2014

It would be helpful to understand WHY we can't just extend the base User model while preserving the name?

My understanding is that loopback-datasource-juggler does not fully support extending existing models. @raymondfeng can you confirm?

If that's not the case, then it makes sense to provide a first-class support for extending models in both loopback and loopback-boot.

@raymondfeng
Copy link
Member

We don’t allow you to create a sub model with the same name as the base. It will create a lot of confusions.

But the juggler does allow you to change the existing model. For loopback, we just need to have a way to express ‘redefine’ or ‘customize’ an existing model.

Thanks,


Raymond Feng
Co-Founder and Architect @ StrongLoop, Inc.

StrongLoop makes it easy to develop APIs in Node, plus get DevOps capabilities like monitoring, debugging and clustering.

On Sep 12, 2014, at 11:16 AM, Miroslav Bajtoš [email protected] wrote:

It would be helpful to understand WHY we can't just extend the base User model while preserving the name?

My understanding is that loopback-datasource-juggler does not fully support extending existing models. @raymondfeng can you confirm?

If that's not the case, then it makes sense to provide a first-class support for extending models in both loopback and loopback-boot.


Reply to this email directly or view it on GitHub.

@bajtos
Copy link
Member Author

bajtos commented Oct 1, 2014

Here's an idea: given the new dynamic mixin functionality, it could be beneficial to implement some of the core models' functionality as mixins. In general, mixins are to be used when inheritance proves difficult.

I like this idea a lot. However, it is a breaking change, I'd like to come up with a solution that works in current loopback 2.x.

I have two approaches in my mind.

Option A

Customize a model at the definition time. Add a new option patch:true to model definition json. Such definition will trigger a special handling in loopback:

  • no new model is created, but an existing model is modified (customized) instead
  • the base class cannot be changed, or perhaps can be changed only to a model extending the original base class
  • there may be more limitations necessary to preserve sane behaviour

Example:

// server/models/user.json
{
  "name": "User",
  "patch": true,
}

// server/models/user.js
module.exports = function(User) {
  // override methods, etc.
}

Option B

Patch the model at the configuration time. It is already possible to modify certain aspects of model relations via server/model-config.json, we need to add an option to specify a custom script file to execute.

// server/model-config.json
{
  "User": {
    "dataSource": "db",
    "runAfterAttach": "model-configs/user.js"
  }
}

// server/model-configs/user.js
module.exports = function(User, app) {
  // any code to be called after the model is attached
}

Dispute

Now that I wrote it down, I am strongly preferring the first option:

  • Option A is more consistent, as all model-related scripts are run at the definition time, before the model is attached to an app and/or a datasource. Option B runs the code after the model was attached, which makes certain changes impossible (e.g. relation changes). This adds complexity that will be difficult to explain to new users.
  • It seems to me that Option A will be easier to implement in tools like generator-loopback and StrongLoop Studio, since it adds just another option to the existing workflow for creating models. On the other hand, Option B adds a completely new workflow.

@ritch @raymondfeng et all watchers: What is your opinion? Can you come up with a third option? I'd like to get the feedback by the end of this week.

@faridnsh
Copy link
Contributor

I'm trying to make it work in postgresql. It would work, if I built it in the schema myself but then I want loopback build the schemas.

@bajtos
Copy link
Member Author

bajtos commented Apr 16, 2015

I want to add a property to the built-in Role model. Any idea how to do this currently?

You should be able to do it using the following workaround:

// server/server.js

var loopback = require('loopback');
// etc.

// ADD THIS LINE BEFORE CALLING boot()
loopback.Role.defineProperty('newPropertyName', { type: 'string' });

boot(app, __dirname);
// etc.

@mikesparr
Copy link

If anyone needs example of how to move the built-in user models to a datasource, I used the example-mysql repo exercise and added a few more automigrate functions to the script and changed DS for the built-in models. https://github.com/mikesparr/loopback-example-users-to-mysql Hope this is helpful for others.

@clarkbw
Copy link

clarkbw commented Aug 8, 2015

Didn't notice this discussion previously, I made an initial pass at creating a User model purely from mixins in #1578. I agree with the above that defining everything in JS instead of the JSON does seem like a lot of overhead, if a mixin could provide a JSON definition file to be merged that might be an interesting way to clean up some of the code. You can see in my I created a 'properties' directory (email propery) because I assumed I'd be reusing those properties in higher level mixins

@superkhau superkhau removed the #fib-5 label Feb 4, 2016
@baktun14
Copy link

baktun14 commented Mar 9, 2016

Hi, has this been resolved?

@DragonTorus
Copy link

Hi! Is there any oficial, normal way(not workaround) to redefine base Model and its relations in loopback 2.27.0 & jugler 2.45.2? Thanks.

@JonnyBGod
Copy link
Contributor

JonnyBGod commented Dec 2, 2016

How is this feature coming up? Any timeframe for this to be implemented?

The lack of this feature is a big burden especially when you are crating a publicly available API where model names should make sense. Client is very different from User...

And using lower cased "user" breaks naming norms and integrations with third party libraries.

@jcolemorrison
Copy link

I was content to just leave it as user for now, despite the fact it's pretty goofy and causes confusion. But when just setting up something to autoupdate the base User model and the extended user model, both seem to automigrate each other. Therefore just wound up having to switch the user over to Client.

And @JonnyBGod is on point. Above anything else it creates a ridiculous goof in terms of naming and public facing APIs. As silly as it sounds, this one limitation is a constant pain point I tend to be annoyed with more than most other things in loopback.

@stale stale bot added the stale label Aug 23, 2017
@stale
Copy link

stale bot commented Aug 23, 2017

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 closed this as completed Sep 6, 2017
@stale
Copy link

stale bot commented Sep 6, 2017

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@MichaelPlaxico
Copy link

This is still a relevant issue that causes serious problems in terms of API design and integration with third-party libraries. Is a fix being planned?

@bajtos
Copy link
Member Author

bajtos commented Oct 24, 2017

This is still a relevant issue that causes serious problems in terms of API design and integration with third-party libraries. Is a fix being planned?

Not in the current code base. We are working on a new version of LoopBack -
see https://github.com/strongloop/loopback-next - where issues like this one should be easier to address.

@RishiMahendru
Copy link

Hi @bajtos is there a way to override persistedModel class methods and add our custom logic to used across all custom models

@bajtos
Copy link
Member Author

bajtos commented Apr 27, 2018

@RishiMahendru sure. Create your own base model inheriting from PersistedModel, and then use this model as a base for all your application models.

This question is pretty off-topic here, please post in on StackOverflow if you need more information.

@strongloop strongloop locked and limited conversation to collaborators Apr 27, 2018
@strongloop strongloop unlocked this conversation Apr 27, 2018
@strongloop strongloop locked as resolved and limited conversation to collaborators Apr 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests