Skip to content

Customize/Normalize class-level remoting http path - #199

Merged
raymondfeng merged 4 commits into
loopbackio:masterfrom
fabien:feature/normalize-url
Aug 4, 2014
Merged

Customize/Normalize class-level remoting http path#199
raymondfeng merged 4 commits into
loopbackio:masterfrom
fabien:feature/normalize-url

Conversation

@fabien

@fabien fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor

A way to specify the http path:

// Path setting
var User = ds.define('UserAccount', {name: String, bio: String}, { path: 'accounts' }); // setting
console.log(User.http.path); // result: '/accounts'

// Relation path setting - final api: /accounts/1/public-photos
User.hasMany('Photo', { 
  as: 'publicPhotos', 
  scope: { where: { public: true } }, 
  options: { http: { path: 'public-photos' } }
});

@fabien

fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng this should fix one of the leftovers that has been long overdue:

  • all URLs should be lowercase / dasherized

strongloop/loopback#391

It'd be great if we could enable ModelBuilder.normalizePathName in LB config.json.

Also, it appears that the options path or normalizecurrently are not passed-through from model-config.json.

@fabien

fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng see also: strongloop/loopback#427

@raymondfeng

Copy link
Copy Markdown
Contributor

@fabien I don't think loopback-datasource-juggler is the right place to provide the hook to customize the path normalization logic. loopback-datasource-juggler should be dealing with ORM. Some of the options for http/rest mapping should be just a pass-through to the model class.

What about moving it into strong-remoting, especially SharedClass/SharedMethod. They can be enhanced to provide a way to customize path mapping. cc @ritch

@fabien

fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng I see, perhaps you or @ritch could look into this? It's really an issue that I'd like to see fixed properly. Also, how would I go about setting the path/plural of the built-in models, like User, at the moment? I cannot change it from models-config.json, and adding a 'user.js' file to models/ doesn't work.

@fabien

fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng I don't really see how you could achieve this without having juggler pass in/through the right name parameter? So yes, the normalization code should go into strong-remoting, but juggler needs to give precedence to an explicit path option over the pluralName (which is something strong-remoting is unaware of).

@raymondfeng

Copy link
Copy Markdown
Contributor

@fabien I'm NOT against the idea to have juggler pass through some options to the model class as I mentioned:

Some of the options for http/rest mapping should be just a pass-through to the model class.

@fabien

fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng thanks for the clarification. Shall I give it a shot and transfer the normalization bits over to strong-remoting? Do you have any comments on that, based on what I did now?

I didn't mean to complain, it was actually a question I'm still interested in: how would I go about setting the path/plural of the built-in models, like User, at the moment? I cannot change it from models-config.json, and adding a 'user.js' file to models/ doesn't work.

@raymondfeng

Copy link
Copy Markdown
Contributor

It should be configurable in model json files. See:

{
  "name": "m2",
  "plural": "models",
  "base": "PersistedModel",
  "properties": {},
  "validations": [],
  "relations": {},
  "acls": [],
  "methods": []
}

@fabien

fabien commented Aug 1, 2014

Copy link
Copy Markdown
Contributor Author

yes that works, but what about doing this for the built-in User model for example? The default path is: /Users, but I'd like /users, but there doesn't seem to be a good way to change that now, although I did that in LB 1.x. without a problem. I'll look into that some more, but tips on modifying the built-in models are welcome.

@raymondfeng

Copy link
Copy Markdown
Contributor

You are right. We might bring the 1.x way (declaring a submodel of User) back. The other option to allow configuration of existing models.

@fabien

fabien commented Aug 4, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng please consider merging this, it is now in tune with strongloop/strong-remoting#90 (and still allows an explicit path option)

Comment thread lib/model-builder.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that we put the path property under http, for example:

http: {
   path: '/my-models'
} 

This will be consistent with other mappings, such as:

mongodb: {
  collection: 'my-collection'
}

@fabien

fabien commented Aug 4, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng changed the options to http: { path: ... } - it should be OK now

@raymondfeng

Copy link
Copy Markdown
Contributor

Cool, thanks!

raymondfeng added a commit that referenced this pull request Aug 4, 2014
Customize/Normalize class-level remoting http path
@raymondfeng
raymondfeng merged commit c0dce8f into loopbackio:master Aug 4, 2014
@ritch

ritch commented Aug 4, 2014

Copy link
Copy Markdown
Contributor

I actually think this belongs in loopback itself. We've been trying to move all the remoting related metadata there. Any reason we couldn't just support the option in loopback-boot or app.model()?

@fabien

fabien commented Aug 4, 2014

Copy link
Copy Markdown
Contributor Author

@ritch I believe not all remoting options have moved to loopback itself - feel free to refactor it later. I just fixed it because it was long overdue ...

@raymondfeng

Copy link
Copy Markdown
Contributor

Hmm, I would love to have it supported in model-config.json and app.model(). Please note it needs to allow relation level too. Do we build SharedClass/RestAdapter during app.model()?

@fabien

fabien commented Aug 4, 2014

Copy link
Copy Markdown
Contributor Author

@raymondfeng @ritch I'd like to point out that I cannot really claim that my contributions are always super elegant - I'm scratching itches that I cannot longer ignore. So yes, there's some frustration, but for the most part I'm happy that my contributions are accepted. I try to aim for a good 'public api' implementation, but of course, the internals might be refactored later.

And I cannot currently do all of that at once, as you guys probably can imagine.

@ritch

ritch commented Aug 4, 2014

Copy link
Copy Markdown
Contributor

I try to aim for a good 'public api' implementation, but of course, the internals might be refactored later

Good point and I feel the same way for my contributions.

And I cannot currently do all of that at once, as you guys probably can imagine.

I don't think we should do all at once actually. I actually like the idea of landing the feature and cleaning up the internals afterward. This approach has the benefit of quickly iterating and deferring the endless nit pick of implementation until after we have the feature.

@ritch

ritch commented Aug 4, 2014

Copy link
Copy Markdown
Contributor

@raymondfeng

Do we build SharedClass/RestAdapter during app.model()

We build it here when we setup loopback.Model.

It is added to the RemoteObjects here in app.model().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants