Skip to content

Allow defining remote method without isStatic flag #173

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

Merged
merged 1 commit into from
May 31, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions lib/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,24 @@ exports.checkPropertyName = function(name) {
return true;
};

/**
* Validate remote method name. Allows `.` in method name for prototype
* @param {String} name The user input
* @returns {String|Boolean}
*/
exports.validateRemoteMethodName = function(name) {
if (name.match(/\./) && !name.match(/^prototype\.([^.]*)$/)) {
return 'Name cannot contain "." characters except' +
' the dot in "prototype." prefix';
}
var result = exports.validateOptionalName(name, /[\/@\s\+%:]/);
if (result !== true) return result;
if (RESERVED_PROPERTY_NAMES.indexOf(name) !== -1) {
return name + ' is a reserved keyword. Please use another name';
}
return true;
};

/**
* Validate required name for properties, data sources, or connectors
* Empty name could not pass
Expand All @@ -84,9 +102,13 @@ exports.validateRequiredName = function (name) {
* @param {String} name The user input
* @returns {String|Boolean}
*/
exports.validateOptionalName = function (name) {
if (name.match(/[\/@\s\+%:\.]/)) {
return 'Name cannot contain special characters (/@+%:. ): ' + name;
exports.validateOptionalName = function (name, unallowedCharacters) {
if (!unallowedCharacters) {
unallowedCharacters = /[\/@\s\+%:\.]/;
}
if (name.match(unallowedCharacters)) {
return 'Name cannot contain special characters ' + unallowedCharacters +
name;
}
if (name !== encodeURIComponent(name)) {
return 'Name cannot contain special characters escaped by ' +
Expand Down
15 changes: 10 additions & 5 deletions remote-method/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var actions = require('../lib/actions');
var helpers = require('../lib/helpers');
var validateRequiredName = helpers.validateRequiredName;
var validateOptionalName = helpers.validateOptionalName;
var checkPropertyName = helpers.checkPropertyName;
var validateRemoteMethodName = helpers.validateRemoteMethodName;
var typeChoices = helpers.getTypeChoices();
var ModelDefinition = require('loopback-workspace').models.ModelDefinition;

Expand Down Expand Up @@ -96,22 +96,27 @@ module.exports = yeoman.generators.Base.extend({
message: 'Enter the remote method name:',
required: true,
default: name,
validate: checkPropertyName
validate: validateRemoteMethodName
},
{
name: 'isStatic',
message: 'Is Static?',
required:true,
required: false,
type: 'confirm',
default: true
default: function(answers) {
return !answers.methodName.match(/^prototype\.(.*)$/);
Copy link
Member

Choose a reason for hiding this comment

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

If we allow methodName to start with prototype. prefix, then I think we need to remove that prefix before we pass the name to ModelMethod constructor. See strongloop/loopback-workspace#273 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

If we allow methodName to start with prototype. prefix, then I think we need to remove that prefix before we pass the name to ModelMethod constructor. See strongloop/loopback-workspace#273 (comment)

@0candy ^^^ thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, hence I removed the prefix before passing the name to the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

}
},
{
name: 'description',
message: 'Description for method:'
}
];
this.prompt(prompts, function(answers) {
this.methodName = answers.methodName;
var m = answers.methodName.match(/^prototype\.(.*)$/);
var isStatic = !m;
var baseName = isStatic ? answers.methodName : m[1];
this.methodName = baseName;
this.isStatic = answers.isStatic;
this.description = answers.description;
done();
Expand Down
31 changes: 31 additions & 0 deletions test/helpers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var validateAppName = helpers.validateAppName;
var validateOptionalName = helpers.validateOptionalName;
var validateRequiredName = helpers.validateRequiredName;
var checkRelationName = helpers.checkRelationName;
var validateRemoteMethodName = helpers.validateRemoteMethodName;
require('chai').should();
var expect = require('chai').expect;
var promise = require('bluebird');
Expand Down Expand Up @@ -76,6 +77,36 @@ describe('helpers', function() {
});
});

// test validateRemoteMethodName()
describe('validateRemoteMethodName()', function() {
it('should accept good names', function() {
testValidationAcceptsValue(validateRemoteMethodName, 'prop');
testValidationAcceptsValue(validateRemoteMethodName, 'prop1');
testValidationAcceptsValue(validateRemoteMethodName, 'my_prop');
testValidationAcceptsValue(validateRemoteMethodName, 'my-prop');
testValidationAcceptsValue(validateRemoteMethodName, 'prototype.method');
});

it('should report errors for a name containing special chars', function() {
testValidationRejectsValue(validateRemoteMethodName, 'my prop');
testValidationRejectsValue(validateRemoteMethodName, 'my/prop');
testValidationRejectsValue(validateRemoteMethodName, 'my@prop');
testValidationRejectsValue(validateRemoteMethodName, 'my+prop');
testValidationRejectsValue(validateRemoteMethodName, 'my%prop');
testValidationRejectsValue(validateRemoteMethodName, 'my:prop');
testValidationRejectsValue(validateRemoteMethodName, 'm.prop');
});

it('should accept empty name', function() {
testValidationAcceptsValue(validateOptionalName, '');
});

it('should not accept name with . after `prototype.`', function() {
testValidationRejectsValue(validateRemoteMethodName,
'prototype.dotted.method');
});
});

// test checkRelationName()
describe('checkRelationName()', function() {
var sampleModelDefinition = new ModelDefinition([
Expand Down
27 changes: 27 additions & 0 deletions test/remote-method.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,33 @@ describe('loopback:remote-method generator', function() {
});
});

it('method name with `prototype.` should be removed', function(done) {
var methodGenerator = givenMethodGenerator();
helpers.mockPrompt(methodGenerator, {
model: 'Car',
methodName: 'prototype.myRemote',
isStatic: 'false',
desription: 'This is my first remote method',
httpPath: '',
acceptsArg: '',
returnsArg: ''
});

methodGenerator.run(function() {
var definition = common.readJsonSync('common/models/car.json');
var methods = definition.methods || {};
expect(methods).to.have.property('myRemote');
expect(methods).to.not.have.property('prototype.myRemote');
expect(methods.myRemote).to.eql({
isStatic: false,
Copy link
Member

Choose a reason for hiding this comment

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

I find this weird, if your test is assuming static/prototype flag is encoded in the name, then why is there isStatic property set too?

accepts: [],
returns: [],
http: []
});
done();
});
});

function givenMethodGenerator() {
var name = 'loopback:remote-method';
var path = '../../remote-method';
Expand Down