Skip to content

Define remote methods using json schema definitions. Prototype for #209 #1163

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.

Sign up for GitHub

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

Closed
mrfelton wants to merge 4 commits into from
Closed

Define remote methods using json schema definitions. Prototype for #209 #1163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0faff12
Define remote methods using json schema definitions. Prototype for #209
Mar 4, 2015
caa2828
Add basic test case to proove that remote methods defined via json ar…
Mar 12, 2015
1f207de
Merge branch 'master' of github.com:strongloop/loopback into feature/…
Apr 8, 2015
210d282
Merge branch 'master' of github.com:strongloop/loopback into feature/…
Apr 17, 2015
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
2 changes: 1 addition & 1 deletion lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ app.model = function(Model, config) {
Model = registry.createModel(modelConfig);

// delete config options already applied
['relations', 'base', 'acls', 'hidden'].forEach(function(prop) {
['relations', 'base', 'acls', 'methods', 'hidden'].forEach(function(prop) {
delete config[prop];
if (config.options) delete config.options[prop];
});
Expand Down
12 changes: 12 additions & 0 deletions lib/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,24 @@ Registry.prototype.configureModel = function(ModelCtor, config) {
'must be an array of objects', modelName);
}

// Remote methods
if (typeof settings.methods === 'object' && settings.methods !== null) {
var methods = settings.methods = settings.methods || {};
Object.keys(settings.methods).forEach(function(key) {
ModelCtor.remoteMethod(key, settings.methods[key]);

@bajtos bajtos Mar 11, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rework this part according to #209 (comment), so that:

  • key prototype.methodName is mapped to remoteMethod('methodName', { isStatic: false, ... })
  • key methodName is mapped to remoteMethod('methodName', { isStatic: true, ... })

@ritch ritch Mar 11, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bajtos please consider the workspace implementation. Lets not make it impractical to implement the generator via the workspace. I'm not against prototype.methodName itself... but we might be making more work for ourselves in the workspace without much benefit.

I'd really like to see remote method creation support added to workspace alongside this addition... These JSON files are specifically designed to serve the workspace well so this new feature is no exception.

@bajtos bajtos Mar 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please consider the workspace implementation. Lets not make it impractical to implement the generator via the workspace.

Can you please elaborate more why my proposal is impractical for loopback-workspace to implement? Are you afraid of parsing prototype.{name} in the workspace?

We can modify the implementation as follows if you think it will make the workspace implementation simpler:

  • methodName is mapped to isStatic:true unless the config object provides a different value
  • prototype.methodName is always mapped to isStatic:false, regardless of the value in the config object
// static methods
methods: { "work": { isStatic: true, /*...*/ } }
methods: { "work": { /*...*/ } }

// prototype methods
methods: { "work": { isStatic: false, /*...*/ } }
methods: { "prototype.work": { /*...*/ } }

What I really want to avoid is adding more stuff that we will have to rework when fixing #209 and thus causing #209 to introduce even more breaking changes.

@ritch ritch Mar 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please elaborate more why my proposal is impractical for loopback-workspace to implement

The workspace uses keys as identifiers for entities (in the case a method would be an entity).

prototype.methodName is always mapped to isStatic:false, regardless of the value in the config object

I like where you are going with this... but consider what the workspace does. It loads properties, and other Entities and also writes them back to files. We would have to store a bit that says "this method was defined using prototype.methodName" and recall that during serialization. Of course this is all possible... but probably not worth it to get rid of the annoying isStatic property.

});
} else if (settings.methods != null) {
console.warn('The methods property of `%s` configuration ' +
'must be an object', modelName);
}

// Settings
var excludedProperties = {
base: true,
'super': true,
relations: true,
acls: true,
methods: true,
dataSource: true
};
if (typeof config.options === 'object' && config.options !== null) {
Expand Down
34 changes: 34 additions & 0 deletions test/fixtures/simple-integration-app/models.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,40 @@
"model": "widget",
"type": "hasMany"
}
},
"methods": {
"details": {
"description": "Get additional details about a store.",
"accepts": [{
"arg": "id",
"type": "any",
"required": true,
"description": "PersistedModel Id"
}],
"returns": {
"arg": "store",
"type": "store",
"root": true
},
"http": {
"path": "/:id/details",
"verb": "get"
}
},
"test": {
"description": "Get additional information about a store.",
"isStatic": false,
"accepts": [],
"returns": {
"arg": "store",
"type": "store",
"root": true
},
"http": {
"path": "/test",
"verb": "get"
}
}
}
}
},
Expand Down
4 changes: 3 additions & 1 deletion test/remoting.integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,9 @@ describe('remoting - integration', function() {
'updateAll(where:object,data:object) POST /stores/update',
'deleteById(id:any) DELETE /stores/:id',
'count(where:object):number GET /stores/count',
'prototype.updateAttributes(data:object):store PUT /stores/:id'
'prototype.updateAttributes(data:object):store PUT /stores/:id',
'details(id:any):store GET /stores/:id/details',
'prototype.test():store GET /stores/:id/test'
];

// The list of methods is from docs:
Expand Down