Allow glob-style patterns for remote options

[zbarbuto]

Description

This PR achieves two things:

1. By moving setSharedMethodSharedProperties to after registry configuration, shared methods on model relations can correctly be disabled (White/Black listing methods generated by relations #2860)
2. By using a regex instead of hard-coded '*' we can use glob-style expressions to blanket disable methods (e.g. __*__someRelation to disable all remote methods of a model relationship)

Note, the reason I used string replacement for * instead of straight regular expressions is to make it backwards compatible ('*' will still enable/disable all methods)

Related issues

• 2860

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

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

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[bajtos]

@slnode ok to test

[bajtos]

Hello @zbarbuto, thank you for the pull request! The proposed changes make sense at high level. Could you please add few tests to verify them? Also, I think it's not enough to replace * with .*, you should escape other characters that have special meaning in RegExp (e.g. . and ?).

[zbarbuto]

@bajtos Have sanitized incoming strings for reserved regex characters (excluding * of course).

Have added tests but this has revealed a problem that the globs still don't seem to be working for related methods. I can't figure out why. In debugging I can see that it's definitely setting the correct methods to shared = false but TestModel.sharedClass.methods() in the test is still returning them as shared: true...

Possibly something further down the chain is re-enabling them again (such as strong-remoting)? Any ideas?

[bajtos]

Please extract these 8 lines of code into a shared helper function. E.g. listPublicMethods(TestModel).

[zbarbuto]

Done

[bajtos]

Have added tests but this has revealed a problem that the globs still don't seem to be working for related methods. I can't figure out why. In debugging I can see that it's definitely setting the correct methods to shared = false but TestModel.sharedClass.methods() in the test is still returning them as shared: true

Remoting metadata for relation methods is defined via strong-remoting "resolver", which is a function called by sharedClass.methods() to describe additional remote methods that were not defined via SharedMethod API nor via properties on the function objects (e.g. MyModel.myMethod.shared = true).

The part in strong-remoting: https://github.com/strongloop/strong-remoting/blob/5b89fb2ab79a77c5b4d25b4009e8033951eb4852/lib/shared-class.js#L133-L136

The part in LoopBack:

loopback/lib/model.js

Lines 261 to 296 in 839c586

	// resolve relation functions
	sharedClass.resolve(function resolver(define) {
	var relations = ModelCtor.relations || {};
	var defineRaw = define;
	define = function(name, options, fn) {
	if (options.accepts) {
	options = extend({}, options);
	options.accepts = setupOptionsArgs(options.accepts);
	}
	defineRaw(name, options, fn);
	};
	// get the relations
	for (var relationName in relations) {
	var relation = relations[relationName];
	if (relation.type === 'belongsTo') {
	ModelCtor.belongsToRemoting(relationName, relation, define);
	} else if (
	relation.type === 'hasOne' ||
	relation.type === 'embedsOne'
	) {
	ModelCtor.hasOneRemoting(relationName, relation, define);
	} else if (
	relation.type === 'hasMany' ||
	relation.type === 'embedsMany' ||
	relation.type === 'referencesMany') {
	ModelCtor.hasManyRemoting(relationName, relation, define);
	}
	}
	// handle scopes
	var scopes = ModelCtor.scopes || {};
	for (var scopeName in scopes) {
	ModelCtor.scopeRemoting(scopeName, scopes[scopeName], define);
	}
	});

As I understand this code, the SharedMethod entries created by these resolvers are temporary, they are always created from fresh whenever sharedClass.methods() is called, and discarded by GarbageCollector afterwards.

BTW similar issue exists for methods with remoting metadata defined via function properties, e.g. MyModel.myMethod.shared = true - new temporary SharedMethod entries are created for them, any changes made to these temporary entries are discarded.

You may be wondering why we don't define relation remote methods immediately at model setup time, similarly how regular PersistedModel methods are defined. So am I :) I think the original reason was that relations are defined by connectors when the model is attached to a datasource, which typically happens later, possibly even asynchronously (e.g. for connectors like SOAP or Swagger that need to fetch WSDL/Swagger description of the API first). I am not sure if that's still relevant though.

As a short-term workaround, perhaps you could use a different approach for disabling remote methods and call sharedMethod.sharedClass.disableRemoteMethodByName(...) instead of sharedMethod.shared = false.

Thoughts?

cc @ritch as the original author of strong-remoting resolvers (IIRC)
cc @superkhau as the author of setSharedMethodSharedProperties (IIRC)

[zbarbuto]

@bajtos Sorry for the delay in updating this. Finally managed to get it working with disableMethodByName as you suggested. Have also rebased on master.

[zbarbuto]

FTR - this line needed to be added otherwise tests relating to default definitions in model-config.json were failing.

[zbarbuto]

Same again here.

[bajtos]

Nice! The changes LGTM now. Could you please squash the commits into a single one and provide a descriptive commit message following our 50/72 rule?

If you are not familiar with git history rewriting, here are the commands to run:

git rebase -i master
git push -f

[zbarbuto]

@bajtos No problems. Done.

[zbarbuto]

Rebased off latest master.

[bajtos]

Landed, thank you for the contribution! 🎉