Skip to content

WIP:Expose replace methods 2.x #2330

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
Amir-61 wants to merge 5 commits into from
Closed

WIP:Expose replace methods 2.x #2330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c2858d
Initial commit
Amir-61 May 11, 2016
4c6e4bb
Apply some feedback (not all)
Amir-61 May 13, 2016
c6301d0
Fixup
Amir-61 May 13, 2016
0926e90
Fixup
Amir-61 May 13, 2016
228accf
Fixup
Amir-61 May 13, 2016
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
6 changes: 6 additions & 0 deletions common/models/user.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@
"permission": "ALLOW",
"property": "resetPassword",
"accessType": "EXECUTE"
},
{
"principalType": "ROLE",
"principalId": "$owner",
"permission": "ALLOW",
"property": "replaceById"
}
],
"relations": {
Expand Down
90 changes: 88 additions & 2 deletions lib/persisted-model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,18 @@ module.exports = function(registry) {
throwNotAttached(this.modelName, 'upsert');
};

/**
* Replace or insert a model instance
* @param {Object} data The model instance data to insert.
* @callback {Function} callback Callback function called with `cb(err, obj)` signature.
* @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
* @param {Object} model Replaced model instance.
*/

PersistedModel.replaceOrCreate = function replaceOrCreate(data, cb) {
throwNotAttached(this.modelName, 'replaceOrCreate');
};

/**
* Finds one record matching the optional filter object. If not found, creates
* the object using the data provided as second argument. In this sense it is
Expand Down Expand Up @@ -482,6 +494,21 @@ module.exports = function(registry) {
throwNotAttached(this.modelName, 'updateAttributes');
};

/**
* Replace all attributes of a model instance identified by the given ID.
* Also performs validation before replacing.
*
* @param {id} id Primary key value of the instance.
* @param {Object} data Data to replace.
* @callback {Function} callback Callback function called with `(err, instance)` arguments. Required.
* @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
* @param {Object} instance Replaced instance.
*/

PersistedModel.replaceById = function replaceById(id, data, cb) {
throwNotAttached(this.modelName, 'replaceById');
};

/**
* Reload object from persistence. Requires `id` member of `object` to be able to call `find`.
* @callback {Function} callback Callback function called with `(err, instance)` arguments. Required.
Expand Down Expand Up @@ -565,15 +592,43 @@ module.exports = function(registry) {
http: {verb: 'post', path: '/'}
});

// forward compatibility
setRemoting(PersistedModel, 'upsert', {
aliases: ['updateOrCreate'],
description: 'Update an existing model instance or insert a new one into the data source.',
accessType: 'WRITE',
accepts: {arg: 'data', type: 'object', description: 'Model instance data', http: {source: 'body'}},
returns: {arg: 'data', type: typeName, root: true},
http: {verb: 'put', path: '/'}
http: { verb: 'patch', path: '/' },
});

if (!options.replaceOnPUT) {
setRemoting(PersistedModel, 'upsert', {
aliases: ['updateOrCreate'],
description: 'Update an existing model instance or insert a new one into the data source.',
accessType: 'WRITE',
accepts: {arg: 'data', type: 'object', description: 'Model instance data', http: {source: 'body'}},
returns: {arg: 'data', type: typeName, root: true},
http: { verb: 'put', path: '/' },
});
}

var replaceOrCreateOptions = {
description: 'Replace an existing model instance or insert a new one into the data source.',
accessType: 'WRITE',
accepts: { arg: 'data', type: 'object', http: { source: 'body' }, description:
'Model instance data' },
returns: { arg: 'data', type: typeName, root: true },
http: { verb: 'post', path: '/replaceOrCreate' },
};

if (options.replaceOnPUT) {
replaceOrCreateOptions.http.verb = 'put';
replaceOrCreateOptions.http.path = '/';
}

setRemoting(PersistedModel, 'replaceOrCreate', replaceOrCreateOptions);

setRemoting(PersistedModel, 'exists', {
description: 'Check whether a model instance exists in the data source.',
accessType: 'READ',
Expand Down Expand Up @@ -619,6 +674,27 @@ module.exports = function(registry) {
rest: {after: convertNullToNotFoundError}
});

var replaceByIdOptions = {
description: 'Replace attributes for a model instance and persist it into the data source.',
accessType: 'WRITE',
accepts: [
{ arg: 'id', type: 'any', description: 'Model id', required: true,
http: { source: 'path' }},
{ arg: 'data', type: 'object', http: { source: 'body' }, description:
'Model instance data' },
],
returns: { arg: 'data', type: typeName },
http: { verb: 'post', path: '/:id/replace' },
rest: { after: convertNullToNotFoundError },

@bajtos bajtos May 11, 2016

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.

Could you please check that replaceById returns null when the id was not found? I don't remember if we implemented this feature. I think this may have been the case of prototype.updateAttributes because of the way how shared constructor works.

For replaceById, I think it makes more sense to raise not-found-error directly from replaceById implementation, so that we don't need this extra bit here.

@bajtos bajtos May 11, 2016

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.

Also please add an integration test to verify this case.

};

if (options.replaceOnPUT) {
replaceByIdOptions.http.verb = 'put';
replaceOrCreateOptions.http.path = '/:id';
}

setRemoting(PersistedModel, 'replaceById', replaceByIdOptions);

setRemoting(PersistedModel, 'find', {
description: 'Find all instances of the model matched by filter from the data source.',
accessType: 'READ',
Expand Down Expand Up @@ -692,9 +768,19 @@ module.exports = function(registry) {
accessType: 'WRITE',
accepts: {arg: 'data', type: 'object', http: {source: 'body'}, description: 'An object of model property name/value pairs'},
returns: {arg: 'data', type: typeName, root: true},
http: {verb: 'put', path: '/'}
http: {verb: 'patch', path: '/'}
});

if (!options.replaceOnPUT) {
setRemoting(PersistedModel.prototype, 'updateAttributes', {
description: 'Update attributes for a model instance and persist it into the data source.',
accessType: 'WRITE',
accepts: {arg: 'data', type: 'object', http: {source: 'body'}, description: 'An object of model property name/value pairs'},
returns: {arg: 'data', type: typeName, root: true},
http: {verb: 'put', path: '/'}
});
}

if (options.trackChanges || options.enableRemoteReplication) {
setRemoting(PersistedModel, 'diff', {
description: 'Get a set of deltas and conflicts since the given checkpoint.',
Expand Down
12 changes: 11 additions & 1 deletion test/access-control.integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ describe('access control - integration', function() {
});
});
lt.describe.whenCalledRemotely('PUT', '/api/users/:id', function() {
beforeEach(function(done) {
app.models.user.settings.replaceOnPUT = false;
app.models.user.setup();
done();
});
lt.it.shouldBeAllowed();
});
});
Expand Down Expand Up @@ -208,7 +213,8 @@ describe('access control - integration', function() {
lt.describe.whenLoggedInAsUser(CURRENT_USER, function() {
beforeEach(function(done) {
var self = this;

app.models.account.settings.replaceOnPUT = true;
app.models.account.setup();
// Create an account under the given user
app.models.account.create({
userId: self.user.id,
Expand All @@ -220,6 +226,10 @@ describe('access control - integration', function() {
});

});
//
// TODO: How to check Model.settings.options.replaceOnPUT
// to decide whether the following test should be for
// (POST for replace and (PATCH AND PUT) for update) OR (PUT for replace and PATCH for update)
lt.describe.whenCalledRemotely('PUT', '/api/accounts/:id', function() {
lt.it.shouldBeAllowed();
});
Expand Down
2 changes: 2 additions & 0 deletions test/model.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,8 @@ describe.onServer('Remote Methods', function() {
'upsert', 'updateOrCreate',
'exists',
'findById',
'replaceById',
'replaceOrCreate',
'find',
'findOne',
'updateAll', 'update',
Expand Down
41 changes: 40 additions & 1 deletion test/remoting.integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ describe('remoting - integration', function() {
})[0];
}

it('has expected remote methods', function() {
it('has expected remote methods without model.settings.replaceOnPUT', function() {
var storeClass = findClass('store');
var methods = storeClass.methods
.filter(function(m) {
Expand All @@ -126,8 +126,11 @@ describe('remoting - integration', function() {
var expectedMethods = [
'create(data:object):store POST /stores',
'upsert(data:object):store PUT /stores',
'upsert(data:object):store PATCH /stores',
'replaceOrCreate(data:object):store POST /stores/replaceOrCreate',
'exists(id:any):boolean GET /stores/:id/exists',
'findById(id:any,filter:object):store GET /stores/:id',
'replaceById(id:any,data:object):store POST /stores/:id/replace',
'find(filter:object):store GET /stores',
'findOne(filter:object):store GET /stores/findOne',
'updateAll(where:object,data:object):object POST /stores/update',
Expand All @@ -142,6 +145,42 @@ describe('remoting - integration', function() {
expect(methods).to.include.members(expectedMethods);
});

// TODO: apparently the way I set up model settings is not right?
it.skip('has expected remote methods with model.settings.replaceOnPUT', function() {
app.models.store.settings.replaceOnPUT = true;
app.models.store.setup();
var storeClass = findClass('store');
var methods = storeClass.methods
.filter(function(m) {
return m.name.indexOf('__') === -1;
})
.map(function(m) {
return formatMethod(m);
});

var expectedMethods = [
'create(data:object):store POST /stores',
'upsert(data:object):store PUT /stores',
'upsert(data:object):store PATCH /stores',
'replaceOrCreate(data:object):store PUT /stores',
'exists(id:any):boolean GET /stores/:id/exists',
'findById(id:any,filter:object):store GET /stores/:id',
'replaceById(id:any,data:object):store PUT /stores/:id',
'find(filter:object):store GET /stores',
'findOne(filter:object):store GET /stores/findOne',
'updateAll(where:object,data:object):object POST /stores/update',
'deleteById(id:any):object DELETE /stores/:id',
'count(where:object):number GET /stores/count',
'prototype.updateAttributes(data:object):store PUT /stores/:id',
'prototype.updateAttributes(data:object):store PATCH /stores/:id',
'createChangeStream(options:object):ReadableStream POST /stores/change-stream',
];

// TODO: update the doc for list of methods accordingly
// https://docs.strongloop.com/display/public/LB/Exposing+models+over+REST
expect(methods).to.include.members(expectedMethods);
});

it('has expected remote methods for scopes', function() {
var storeClass = findClass('store');
var methods = storeClass.methods
Expand Down