Fix dynamic shared methods can't hide

[clark0x]

I'm trying to fix #94

Put the following code in a boot script. It should hide all the routes except POST /api/Users/login.

User.sharedClass.methods().each(method) {
  method.shared = false;
});
User.sharedClass.find('login', true).shared = true;

Signed-off-by: Clark Wang clark.wangs@gmail.com

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[clark0x]

Have to use name instead of fn. fn might different from sharedMethod.fn even they have the same name.

e.g., in Model.belongsToRemoting(), this.prototype[relationName] becomes sharedMethod.fn, and '__get__' + relationName becomes sharedMethod.name. This will cause that SharedMethod.prototype.getFunction() will return ctor.prototype['__get__' + relationName], and SharedMethod.prototype.isDelegateFor() will always return false.

[clark0x]

e.g., in Model.scopeRemoting(), the sharedMethods will be defined WITHOUT fn. So, SharedMethod.prototype.isDelegateFor(fn) will always return false. Unless call with name.

[raymondfeng]

I think the inconsistency is caused by two different versions of remoting definition.

1. The 1st version is to define all remoting metadata directly on the function itself. It creates a constraint that a remote function has to be referenced by a property on the model class/prototype. In the case of relations, we had to add a dummy method to the model class prototype. That's why you see ctor.prototype['__get__' + relationName]. The other limitation is that the remote operation name has to be same as the method name.
2. The 2nd version is to define all remoting metadata on the SharedClass/SharedMethod so that the target function can be decoupled from the model class/prototype. The new approach allows us to connect any functions to a model class/prototype and make them remote. The target function can be invoked with the model class or instance as the receiver (this) without requiring the function to be physically defined on the model class/prototype. For example, ctor.prototype['__get__' + relationName] should be removed.
3. To keep the backward compatibility, both styles are supported. That creates some confusions too.
4. I think SharedMethod.prototype.getFunction() should return this.fn if it is present before looking up from the model class/prototype by name.
5. loopback.Model now uses 2nd version of remoting definition. For belongsToRemoting, it connects the ctor.prototype[relationName] to '__get__' + relationName. For scopeRemoting, it only provides the name and expects the function will be resolved from the model class prototype by name. I think we can fix that to make the resolved scope functions explicit from the prototype.

I think your patch mostly looks good. Can you add some tests?

[clark0x]

@raymondfeng thanks for explain such thorough. I have added a few tests. But there is one more issue for shared = false to work, which is: where should we put these codes?

For accessing any kinds of shared methods, the target model and all models that have relation with the target model have to be attached to a data source, and before rest and explorer middlewares. So, attached event and dataSourceAttached event are both too earlier.

Currently, my workaround is create a boot script all-attached.js for emitting an allAttached event to every model, then in each model's JS file do shared = false stuff in a listener. Any better approach?

BTW: is it possible that creating relations and scopes after loopback has been booted ? if so, there is no way to aware of that currently. It would be better to have an event such as configChanged on model.

[raymondfeng]

It's completely possible to add/remove/update shared classes/methods after rest/explorer are initiated. I think we should emit such events from the SharedClass instance so that:

• REST adapter can add/remove/update the route
• API explorer can update the api spec

[clark0x]

Hmm... I suggest that let juggler to emit events such as configChanged when a relation or a scope been created, changed or removed. Then the SharedClass(or Model itself) could listen to these events to add/change/remove corresponding shared methods. This will replace the need of SharedClass.resolve(). And SharedClass could emit events for REST adapter and API Explorer to adjust.

[raymondfeng]

@clarkorz Your proposal sounds good to me. The model builder (should be refactored into its own module) is responsible for defining the JS class for a given model. It should emit events whenever the class changes.

[ritch]

I don't think we can take this approach until we refactor relation remoting. Currently we use a strong remoting resolver to dynamically define the methods. I'd like to remove this in favor of a static list of SharedClass#methods().

First we need an event that ensures the relations have been defined. I'm thinking of something like app.on('ready', ...).

[clark0x]

@ritch I agree. relation remoting need to be refactored.

And also loopback.remoteMethod(), it using the old style which defines all remoting metadata directly on the function itself. These remote methods are also discovered by sharedMethod.method() dynamically.

The ready event LGTM

[ritch]

This has been superseded by sharedClass.disableMethod().