Skip to content

upsertWithWhere method #1001

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

Merged
Amir-61 merged 1 commit into from
Aug 16, 2016
Merged

upsertWithWhere method #1001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 30 additions & 0 deletions lib/connectors/memory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,36 @@ Memory.prototype.updateOrCreate = function(model, data, options, callback) {
});
};

Memory.prototype.patchOrCreateWithWhere =
Memory.prototype.upsertWithWhere = function(model, where, data, options, callback) {
var self = this;
var primaryKey = this.idName(model);
var filter = { where: where };
var nodes = self._findAllSkippingIncludes(model, filter);
if (nodes.length === 0) {
return self._createSync(model, data, function(err, id) {
if (err) return process.nextTick(function() { callback(err); });
self.saveToFile(id, function(err, id) {
self.setIdValue(model, data, id);
callback(err, self.fromDb(model, data), { isNewInstance: true });
});
});
}
if (nodes.length === 1) {

@superkhau superkhau Aug 4, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be else if?

@superkhau superkhau Aug 4, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actually nevermind, i see the return on line 262 now

@superkhau superkhau Aug 4, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as a nit though, we should probably use else if since you use if/else right after anyways

var primaryKeyValue = nodes[0][primaryKey];
self.updateAttributes(model, primaryKeyValue, data, options, function(err, data) {
callback(err, data, { isNewInstance: false });
});
} else {
process.nextTick(function() {
var error = new Error('There are multiple instances found.' +
'Upsert Operation will not be performed!');
error.statusCode = 400;
callback(error);
});
}
};

Memory.prototype.findOrCreate = function(model, filter, data, callback) {
var self = this;
var nodes = self._findAllSkippingIncludes(model, filter);
Expand Down
166 changes: 166 additions & 0 deletions lib/dao.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,173 @@ DataAccessObject.upsert = function(data, options, cb) {
}
return cb.promise;
};
/**
* Update or insert a model instance based on the search criteria.
* If there is a single instance retrieved, update the retrieved model.
* Creates a new model if no model instances were found.
* Returns an error if multiple instances are found.
* * @param {Object} [where] `where` filter, like
* ```
* { key: val, key2: {gt: 'val2'}, ...}
* ```
* <br/>see
* [Where filter](https://docs.strongloop.com/display/LB/Where+filter#Wherefilter-Whereclauseforothermethods).
* @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 Updated model instance.
*/
DataAccessObject.patchOrCreateWithWhere =
DataAccessObject.upsertWithWhere = function(where, data, options, cb) {

@bajtos bajtos Jul 14, 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.

In longer term, we want to promote patchOrCreate instead of upsert/updateOrCreate. Can you please add an alias patchOrCreateWithWhere = upsertWithWhere? See how other aliases in this file are set up.

@mountain1234585 mountain1234585 Jul 15, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done!

var connectionPromise = stillConnecting(this.getDataSource(), this, arguments);
if (connectionPromise) { return connectionPromise; }
if (cb === undefined) {
if (typeof options === 'function') {
// upsertWithWhere(where, data, cb)
cb = options;
options = {};
}
}
cb = cb || utils.createPromiseCallback();
options = options || {};
assert(typeof where === 'object', 'The where argument must be an object');
assert(typeof data === 'object', 'The data argument must be an object');
assert(typeof options === 'object', 'The options argument must be an object');
assert(typeof cb === 'function', 'The cb argument must be a function');
if (Object.keys(data).length === 0) {
var err = new Error('data object cannot be empty!');
err.statusCode = 400;
process.nextTick(function() { cb(err); });
return cb.promise;
}
var hookState = {};
var self = this;
var Model = this;
var connector = Model.getConnector();
var modelName = Model.modelName;
var query = { where: where };
var context = {
Model: Model,
query: query,
hookState: hookState,
options: options,
};

@bajtos bajtos Jul 15, 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.

Please preserve consistency with other existing operation hooks. There is no filter context property. The access hook provides ctx.query, before save provides ctx.where, ctx.data, ctx.currentInstance in case of a partial update, and before save provides ctx.instance.

Ideally, the operation hooks invoked for upsertWithWhere should be the same regardless of whether an atomic connector-provided function is called, or whether a non-atomic find + updateAttributes is used.

Take a look at support/describe-operation-hooks.js, it creates a comprehensive table showing what context is provided for different DAO operations.

Model.notifyObserversOf('access', context, doUpsertWithWhere);
function doUpsertWithWhere(err, ctx) {
if (err) return cb(err);
ctx.data = data;
if (connector.upsertWithWhere) {
var context = {
Model: Model,
where: ctx.query.where,
data: ctx.data,
hookState: hookState,
options: options,
};
Model.notifyObserversOf('before save', context, function(err, ctx) {
if (err) return cb(err);
data = ctx.data;
var update = data;
var inst = data;
if (!(data instanceof Model)) {
inst = new Model(data, { applyDefaultValues: false });
}
update = inst.toObject(false);
Model.applyScope(query);
Model.applyProperties(update, inst);
Model = Model.lookupModel(update);
if (options.validate === false) {
return callConnector();
}
if (options.validate === undefined && Model.settings.automaticValidation === false) {
return callConnector();
}
inst.isValid(function(valid) {
if (!valid) return cb(new ValidationError(inst), inst);
callConnector();
}, update, options);

function callConnector() {
try {
ctx.where = removeUndefined(ctx.where);
ctx.where = Model._coerce(ctx.where);
update = removeUndefined(update);
update = Model._coerce(update);
} catch (err) {
return process.nextTick(function() {
cb(err);
});
}
context = {
Model: Model,
where: ctx.where,
data: update,
currentInstance: inst,
hookState: ctx.hookState,
options: options,
};
Model.notifyObserversOf('persist', context, function(err) {
if (err) return done(err);
connector.upsertWithWhere(modelName, ctx.where, update, options, done);
});
}
function done(err, data, info) {
if (err) return cb(err);
var contxt = {
Model: Model,
data: data,
isNewInstance: info && info.isNewInstance,
hookState: ctx.hookState,
options: options,
};
Model.notifyObserversOf('loaded', contxt, function(err) {

@Amir-61 Amir-61 Aug 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ummm not sure why you could not use context as opposed to contxt; probably it conflicts the name. How about using ctx instead of contxt which is missing e

@mountain1234585 mountain1234585 Aug 16, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

we are making use of ctx.hookstate , which is defined before we define the context properties for 'loaded'. Using ctx instead will make ctx.hookState undefined. Hence, I have left the variable name as contxt .

if (err) return cb(err);
var obj;
if (contxt.data && !(contxt.data instanceof Model)) {
inst._initProperties(contxt.data, { persisted: true });
obj = inst;
} else {
obj = contxt.data;
}
var context = {
Model: Model,
instance: obj,
isNewInstance: info ? info.isNewInstance : undefined,
hookState: hookState,
options: options,
};
Model.notifyObserversOf('after save', context, function(err) {
cb(err, obj);

@Amir-61 Amir-61 Jul 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why checking err again? You have already checked err on line 755!

@superkhau superkhau Jul 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On 750 now

@mountain1234585 mountain1234585 Jul 16, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done! will be available in the next commit.

});
});
}
});
} else {
var opts = { notify: false };
if (ctx.options && ctx.options.transaction) {
opts.transaction = ctx.options.transaction;
}
self.find({ where: ctx.query.where }, opts, function(err, instances) {
if (err) return cb(err);
var modelsLength = instances.length;

@Amir-61 Amir-61 Jul 15, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please return error from find

self.find(filter, function(err, instances) {
  if (err) return cb(err);
  ...
}

@mountain1234585 mountain1234585 Jul 15, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done! will be available in a commit in sometime.

if (modelsLength === 0) {
self.create(data, options, cb);
} else if (modelsLength === 1) {
var modelInst = instances[0];
modelInst.updateAttributes(data, options, cb);
} else {
process.nextTick(function() {

@bajtos bajtos Jul 14, 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.

ditto. Both find and findById already defers callback to the next tick, no need to call process.nextTick here.

@mountain1234585 mountain1234585 Jul 15, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done!

var error = new Error('There are multiple instances found.' +
'Upsert Operation will not be performed!');
error.statusCode = 400;
cb(error);
});
}
});
}
}

@bajtos bajtos Jul 14, 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.

Please make sure all return paths from the top-level methods always return cb.promise.

@mountain1234585 mountain1234585 Jul 15, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done!

return cb.promise;
};
/**
* Replace or insert a model instance: replace exiting record if one is found, such that parameter `data.id` matches `id` of model instance;
* otherwise, insert a new record.
Expand Down
63 changes: 63 additions & 0 deletions test/crud-with-options.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,69 @@ describe('crud-with-options', function() {

});

@Amir-61 Amir-61 Jul 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No space here please

@mountain1234585 mountain1234585 Jul 19, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Its in line with other methods in this file.

describe('upsertWithWhere', function() {
beforeEach(seed);
it('rejects upsertWithWhere (options,cb)', function(done) {
try {
User.upsertWithWhere({}, function(err) {
if (err) return done(err);
});
} catch (ex) {
ex.message.should.equal('The data argument must be an object');
done();
}
});

it('rejects upsertWithWhere (cb)', function(done) {
try {
User.upsertWithWhere(function(err) {
if (err) return done(err);
});
} catch (ex) {
ex.message.should.equal('The where argument must be an object');
done();
}
});

it('allows upsertWithWhere by accepting where,data and cb as arguments', function(done) {
User.upsertWithWhere({ name: 'John Lennon' }, { name: 'John Smith' }, function(err) {
if (err) return done(err);
User.find({ where: { name: 'John Lennon' }}, function(err, data) {
if (err) return done(err);
data.length.should.equal(0);
User.find({ where: { name: 'John Smith' }}, function(err, data) {
if (err) return done(err);
data.length.should.equal(1);
data[0].name.should.equal('John Smith');
data[0].email.should.equal('john@b3atl3s.co.uk');
data[0].role.should.equal('lead');
data[0].order.should.equal(2);
data[0].vip.should.equal(true);
done();
});
});
});
});

it('allows upsertWithWhere by accepting where, data, options, and cb as arguments', function(done) {
options = {};
User.upsertWithWhere({ name: 'John Lennon' }, { name: 'John Smith' }, options, function(err) {
if (err) return done(err);
User.find({ where: { name: 'John Smith' }}, function(err, data) {
if (err) return done(err);
data.length.should.equal(1);
data[0].name.should.equal('John Smith');
data[0].seq.should.equal(0);
data[0].email.should.equal('john@b3atl3s.co.uk');
data[0].role.should.equal('lead');
data[0].order.should.equal(2);
data[0].vip.should.equal(true);
done();
});
});
});
});

function seed(done) {
var beatles = [
{
Expand Down
Loading