Skip to content
Closed
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
17 changes: 9 additions & 8 deletions lib/dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ var util = require('util');
var assert = require('assert');
var BaseModel = require('./model');
var debug = require('debug')('loopback:dao');
DataAccessObject.patchById = require('./patch-by-id');

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 move the new file under a new folder, i.e. dao/patch-by-id.js.


/**
* Base class for all persistent objects.
Expand All @@ -48,9 +49,9 @@ function DataAccessObject() {
}
}

function idName(m) {
var idName = module.exports.idName = function(m) {
return m.definition.idName() || 'id';
}
};

function getIdValue(m, data) {
return data && data[idName(m)];
Expand Down Expand Up @@ -80,7 +81,7 @@ function convertSubsetOfPropertiesByType(inst, data) {
* Apply strict check for model's data.
* Notice: Please note this method modifies `inst` when `strict` is `validate`.
*/
function applyStrictCheck(model, strict, data, inst, cb) {
var applyStrictCheck = module.exports.applyStrictCheck = function(model, strict, data, inst, cb) {
var props = model.definition.properties;
var keys = Object.keys(data);
var result = {}, key;
Expand All @@ -96,7 +97,7 @@ function applyStrictCheck(model, strict, data, inst, cb) {
}
}
cb(null, result);
}
};

function setIdValue(m, data, value) {
if (data) {
Expand All @@ -111,15 +112,15 @@ function byIdQuery(m, id) {
return query;
}

function isWhereByGivenId(Model, where, idValue) {
var isWhereByGivenId = module.exports.isWhereByGivenId = function(Model, where, idValue) {
var keys = Object.keys(where);
if (keys.length != 1) return false;

var pk = idName(Model);
if (keys[0] !== pk) return false;

return where[pk] === idValue;
}
};

DataAccessObject._forDB = function(data) {
if (!(this.getDataSource().isRelational && this.getDataSource().isRelational())) {
Expand Down Expand Up @@ -408,7 +409,7 @@ DataAccessObject.create = function(data, options, cb) {
return cb.promise || obj;
};

function stillConnecting(dataSource, obj, args) {
var stillConnecting = module.exports.stillConnecting = function(dataSource, obj, args) {
if (typeof args[args.length - 1] === 'function') {
return dataSource.ready(obj, args);
}
Expand All @@ -423,7 +424,7 @@ function stillConnecting(dataSource, obj, args) {
} else {
return false;
}
}
};

/**
* Update or insert a model instance: update exiting record if one is found, such that parameter `data.id` matches `id` of model instance;
Expand Down
187 changes: 187 additions & 0 deletions lib/patch-by-id.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
// Copyright IBM Corp. 2015,2016. All Rights Reserved.
// Node module: loopback-datasource-juggler
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

var ValidationError = require('./validations').ValidationError;
var assert = require('assert');
var dao = require('./dao');

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.

Cyclic dependencies are better to avoid. Please move the helper functions into a standalone file, e.g. lib/dao/util.js.

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.

I see we already have lib/utils.js, so we can either move the helpers there, but since they are DAO-specific, I think lib/dao/helpers.js would be better. Just keep the filename consistent with what we already have (plural helpers.js).

var utils = require('./utils');
var idEquals = utils.idEquals;
var removeUndefined = utils.removeUndefined;

module.exports = function patchById(id, data, options, cb) {
var connectionPromise = dao.stillConnecting(this.getDataSource(), this, arguments);
if (connectionPromise) {
return connectionPromise;
}
assert(arguments.length >= 2, 'At least two arguments are required');

var Model = this;
var where = {};
var strict = this.settings.strict;
// TODO: the methods which we have imported from dao (idName(),
// stillConnecting(), isWhereByGivenId(),...), may need to be
// moved to a more appropriate file (utils.js?)
var idPropertyName = dao.idName(Model);

where[idPropertyName] = id;
if (cb === undefined) {
if (typeof options === 'function') {
// patchById(id, data, cb)
cb = options;
options = {};
}
}

data = data || {};
cb = cb || utils.createPromiseCallback();
options = options || {};

assert((typeof data === 'object') && (data !== null),
'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');

var connector = Model.getDataSource().connector;
assert(typeof connector.update === 'function',

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.

Since we call connector.updateAttributes instead of connector.update, you should change this check too.

'update() must be implemented by the connector');

// Make sure id cannot be changed
checkId(data[idPropertyName], id, idPropertyName, cb);

// Question[1]:
// Please see QUESTION-test[1] in`test\dao.suit\patch-by-id.suits.js` for failing test
// data = data.toObject();
// data = removeUndefined(data);

var hookState = {};
this.applyProperties(data);

var context = {
Model: Model,
where: where,
data: data,

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.

Should we report isNewInstance:false too?

hookState: hookState,
options: options,
};

Model.notifyObserversOf('before save', context, function(err, ctx) {
if (err) return cb(err);
var isOriginalQuery = dao.isWhereByGivenId(Model, ctx.where, id);

if (!isOriginalQuery) {
var err = new Error('ctx.where:' + JSON.stringify(ctx.where) + ' is not valid; the `' +
idPropertyName + '` original value is ' + id);

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 improve the error message to mention that patchById does not allow hooks to change the where query to point to a different instance.

err.statusCode = 403;
process.nextTick(function() {
cb(err);
});
return cb.promise;
}

if (strict) {
var instInfo = { '__unknownProperties': [] };
dao.applyStrictCheck(Model, strict, data,
instInfo, validateAndCallConnector);

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.

This doesn't make sense to me, because AFAICT you are discarding __unknownProperties and thus they will be never reported.

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.

Hmm, I see you have a test case for this and I assume it passes. That means I do not fully understand what this piece of code is doing. Could you please explain a bit?

} else {
validateAndCallConnector();
}

function validateAndCallConnector(err) {
if (err) return cb(err);
if (options.validate === false) {
return doUpdate(ctx.where, ctx.data);
}

// only when options.validate is not set, take model-setting into consideration
if (options.validate === undefined && Model.settings.automaticValidation === false) {
return doUpdate(ctx.where, ctx.data);
}

// Make sure id is not changed in the context of `before save`
checkId(ctx.data[idPropertyName], id, idPropertyName, cb);

dataObj = new Model(ctx.data);
dataObj.unsetAttribute(idPropertyName);
// validation required
dataObj.isValid(function(valid) {

@bajtos bajtos Apr 27, 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.

This will not work in most cases. Consider a model with required "name" and optional "age" properties, and then call patchById setting only age:42. The request is perfectly valid, but the validation will fail.

Ideally, we could run validators only on the properties affected by the patch operation. However, I think that's nontrivial and should be left out of scope of this pull request.

OTOH, because we are skipping validation, calling patchById makes it easy to get the data into inconsistent state. I think it may be better to not expose this method in the public REST API by default. In which case, we can probably left patchById out of 3.0 completely and wait until we have more time available to implement it fully with validations.

Thoughts?

@Amir-61 Amir-61 Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea I noticed that restriction too and unfortunately I don't think if there is any easy efficient alternative either... The only alternative I can think of is calling findById to get instance, which definitely does not make sense to call another DB query and it makes patchById more computational expensive... On the other hand skipping the validation makes data inconsistent and as you mentioned I don't think if that's a rational decision either.

Initially the decision for loopback 3.0 was to map PUT to static replaceById and PATCH to static patchById instead of prototype.updateAttributes; however now that we cannot have the right validation for static patchById as a result of not having access to instance I agree with you that we should map PUT to static replaceById and PATCH to prototype.updateAttributes and we can say implementation of static patchById is out of scope of this milestone.

if (valid) {
doUpdate(ctx.where, ctx.data);
} else {
cb(new ValidationError(dataObj), dataObj);
}
});
}
});

function doUpdate(where, data) {
try {
data = removeUndefined(data);
data = Model._coerce(data);
} catch (err) {
return process.nextTick(function() {
cb(err);
});
}

var ctx = {
Model: Model,
where: where,
data: data,
isNewInstance: false,
hookState: hookState,
options: options,
};
Model.notifyObserversOf('persist', ctx, function(err) {
if (err) return cb (err);
if (connector.update.length === 5) {
connector.updateAttributes(Model.modelName, where[idPropertyName], ctx.data, options, patchCallback);
} else {
connector.updateAttributes(Model.modelName, where[idPropertyName], ctx.data, patchCallback);
}
});

function patchCallback(err, info) {
if (err) return cb(err);
var ctx = {
Model: Model,
data: data,

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.

Are other DAO methods reporting isNewInstance for loaded hook?

hookState: hookState,
options: options,
};
Model.notifyObserversOf('loaded', ctx, function(err) {

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.

This does not make sense to me. The loaded hook is called with the data loaded from the database, which is not the case of patchById. I think we should not trigger this hook at all.

if (err) return cb(err);
var context = {
Model: Model,
isNewInstance: false,

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 add where and data.

hookState: hookState,
options: options,
};
Model.notifyObserversOf('after save', context, function(err) {
if (!err) Model.emit('changed', info);
// Just for consistency we return { count: 1} for all connectors
cb(err, { count: 1 });

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.

Hmm, this is not correct. What if the record-to-be-updated was deleted? I think we should report 0 or 1 depending on the result from the database. Unless the operation fails when the record no longer exists. Please add a unit-test to verify this edge case.

});
});
}
}
return cb.promise;
};

var idValidationFailure;

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.

I find this suspicious, isn't this a module-global singleton shared by all invocations of checkId? I don't think that will work.

function checkId(idData, id, idPropertyName, cb) {
if (!idValidationFailure && idData !== undefined && !idEquals(idData, id)) {
idValidationFailure = true;
var err = new Error('id property (' + idPropertyName + ') ' +
'cannot be updated from ' + id + ' to ' + idData);
// return with 403 (forbidden) error status code
err.statusCode = 403;
process.nextTick(function() {
cb(err);
});
return cb.promise;
} else {
return false;
}
};
17 changes: 17 additions & 0 deletions test/dao.suite/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
var debug = require('debug')('test');
var fs = require('fs');
var path = require('path');

module.exports = function(dataSource, should, connectorCapabilities) {
var operations = fs.readdirSync(__dirname);
operations = operations.filter(function(it) {
return it !== path.basename(__filename) &&
!!require.extensions[path.extname(it).toLowerCase()];
});
for (var ix in operations) {
var name = operations[ix];
var fullPath = require.resolve('./' + name);
debug('Loading test suite %s (%s)', name, fullPath);
require(fullPath).apply(this, arguments);
}

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.

Makes me wonder, can we put this code into a shared helper?

module.exports = function(dataSource, should, connectorCapabilities) {
  helpers.runAllSuitesInDirExceptFile(__dirname, __filename);
};

};
Loading