verdnatura-mirror
/
loopback-datasource-juggler
mirror of https://github.com/loopbackio/loopback-datasource-juggler
0
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix Operation Hooks to propagate data changes 

Before this change, many Operation Hooks were not correctly propagating
changes made to `ctx.data` via reassigning ctx.data to a new object.

This change modifies existing tests to account for this different
scenario, adds few more tests for scenarios that were not covered by
tests before and finally fixes the problem discovered.
This commit is contained in:
Miroslav Bajtoš 2018-06-26 13:39:00 +02:00
parent fa0039c6a8
commit 9af79cf51a
No known key found for this signature in database
GPG Key ID: 6F2304BA9361C7E3
2 changed files with 200 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
lib/dao.js
View File

@ -467,9 +467,10 @@ DataAccessObject.create = function(data, options, cb) {
hookState: hookState,
options: options,
};
Model.notifyObserversOf('persist', context, function(err) {
Model.notifyObserversOf('persist', context, function(err, ctx) {
if (err) return cb(err);
invokeConnectorMethod(connector, 'create', Model, [obj.constructor._forDB(context.data)],
val = ctx.data;
invokeConnectorMethod(connector, 'create', Model, [obj.constructor._forDB(ctx.data)],
options, createCallback);
});
}, obj, cb);
@ -1131,9 +1132,10 @@ DataAccessObject.findOrCreate = function findOrCreate(query, data, options, cb)
hookState: hookState,
options: options,
};
Model.notifyObserversOf('loaded', context, function(err) {
Model.notifyObserversOf('loaded', context, function(err, ctx) {
if (err) return cb(err);
data = ctx.data;
var obj, Model = self.lookupModel(data);
if (data) {
@ -2547,9 +2549,10 @@ DataAccessObject.prototype.save = function(options, cb) {
hookState: hookState,
options: options,
};
Model.notifyObserversOf('loaded', context, function(err) {
Model.notifyObserversOf('loaded', context, function(err, ctx) {
if (err) return cb(err);
data = ctx.data;
inst._initProperties(data, {persisted: true});
var context = {
@ -2579,8 +2582,9 @@ DataAccessObject.prototype.save = function(options, cb) {
options: options,
};
Model.notifyObserversOf('persist', context, function(err) {
Model.notifyObserversOf('persist', context, function(err, ctx) {
if (err) return cb(err);
data = ctx.data;
invokeConnectorMethod(connector, 'save', Model, [Model._forDB(data)],
options, saveCallback);
});
@ -2756,6 +2760,7 @@ DataAccessObject.updateAll = function(where, data, options, cb) {
};
Model.notifyObserversOf('persist', context, function(err, ctx) {
if (err) return cb(err);
data = ctx.data;
invokeConnectorMethod(connector, 'update', Model, [where, data], options, updateCallback);
});
}
@ -3084,6 +3089,7 @@ DataAccessObject.replaceById = function(id, data, options, cb) {
function replaceCallback(err, data) {
if (err) return cb(err);
context.data = data;
if (typeof connector.generateContextData === 'function') {
context = connector.generateContextData(context, data);
}
@ -3132,7 +3138,7 @@ DataAccessObject.replaceById = function(id, data, options, cb) {
};
Model.notifyObserversOf('persist', ctx, function(err) {
if (err) return cb(err);
invokeConnectorMethod(connector, 'replaceById', Model, [id, Model._forDB(context.data)],
invokeConnectorMethod(connector, 'replaceById', Model, [id, Model._forDB(ctx.data)],
options, replaceCallback);
});
}
@ -3277,6 +3283,7 @@ function(data, options, cb) {
function updateAttributesCallback(err, data) {
if (err) return cb(err);
context.data = data;
if (typeof connector.generateContextData === 'function') {
context = connector.generateContextData(context, data);
}
@ -3330,7 +3337,7 @@ function(data, options, cb) {
Model.notifyObserversOf('persist', ctx, function(err) {
if (err) return cb(err);
invokeConnectorMethod(connector, 'updateAttributes', Model,
[getIdValue(Model, inst), Model._forDB(context.data)],
[getIdValue(Model, inst), Model._forDB(ctx.data)],
options, updateAttributesCallback);
});
}, data, cb);

216
test/persistence-hooks.suite.js
View File

@ -163,7 +163,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook when near filter is used', function(done) {
GeoModel.observe('loaded', function(ctx, next) {
ctx.data.name = 'Berlin';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'Berlin'});
next();
});
@ -181,8 +182,10 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates to one specific instance from `loaded` hook when near filter is used',
function(done) {
GeoModel.observe('loaded', function(ctx, next) {
if (ctx.data.name === 'Rome')
ctx.data.name = 'Berlin';
if (ctx.data.name === 'Rome') {
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'Berlin'});
}
next();
});
@ -199,7 +202,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook when near filter is not used', function(done) {
TestModel.observe('loaded', function(ctx, next) {
ctx.data.name = 'Paris';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'Paris'});
next();
});
@ -213,8 +217,10 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates to one specific instance from `loaded` hook when near filter is not used',
function(done) {
TestModel.observe('loaded', function(ctx, next) {
if (ctx.data.name === 'first')
ctx.data.name = 'Paris';
if (ctx.data.name === 'first') {
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'Paris'});
}
next();
});
@ -313,7 +319,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
TestModel.find(
@ -472,7 +479,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `persist` hook', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
// By default, the instance passed to create callback is NOT updated
@ -536,7 +544,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
// By default, the instance passed to create callback is NOT updated
@ -872,7 +881,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
if (dataSource.connector.findOrCreate) {
it('applies updates from `persist` hook when found', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
TestModel.findOrCreate(
@ -905,7 +915,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `persist` hook when not found', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
TestModel.findOrCreate(
@ -1003,7 +1014,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
if (dataSource.connector.findOrCreate) {
it('applies updates from `loaded` hook when found', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
TestModel.findOrCreate(
@ -1021,7 +1033,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook when not found', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
// Unoptimized connector gives a call to `create. But,
@ -1198,7 +1211,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `persist` hook', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
existingInstance.save(function(err, instance) {
@ -1240,7 +1254,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
existingInstance.save(function(err, instance) {
@ -1364,8 +1379,11 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `before save` hook', function(done) {
TestModel.observe('before save', function(ctx, next) {
ctx.data.extra = 'extra data';
ctx.data.name = 'hooked name';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {
extra: 'extra data',
name: 'hooked name',
});
next();
});
@ -1418,7 +1436,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `persist` hook', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
// By default, the instance passed to updateAttributes callback is NOT updated
@ -1429,7 +1448,11 @@ module.exports = function(dataSource, should, connectorCapabilities) {
existingInstance.updateAttributes({name: 'changed'}, function(err, instance) {
if (err) return done(err);
instance.should.have.property('extra', 'hook data');
done();
TestModel.findById(existingInstance.id, (err, found) => {
if (err) return done(err);
found.should.have.property('extra', 'hook data');
done();
});
});
});
@ -1459,7 +1482,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
ctx.data.address.should.be.type('object');
ctx.data.address.should.not.be.instanceOf(Address);
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
// By default, the instance passed to updateAttributes callback is NOT updated
@ -1505,7 +1529,7 @@ module.exports = function(dataSource, should, connectorCapabilities) {
if (err) return done(err);
ctxRecorder.records.should.eql(aCtxForModel(TestModel, {
data: {name: 'changed'},
data: {id: existingInstance.id, name: 'changed'},
isNewInstance: false,
}));
@ -1525,7 +1549,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook updateAttributes', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
// By default, the instance passed to updateAttributes callback is NOT updated
@ -1583,7 +1608,7 @@ module.exports = function(dataSource, should, connectorCapabilities) {
});
if (!dataSource.connector.replaceById) {
describe.skip('replaceById - not implemented', function() {});
describe.skip('replaceAttributes - not implemented', function() {});
} else {
describe('PersistedModel.prototype.replaceAttributes', function() {
it('triggers hooks in the correct order', function(done) {
@ -1722,7 +1747,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
ctx.data.address.should.be.type('object');
ctx.data.address.should.not.be.instanceOf(Address);
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
existingUser.replaceAttributes(
@ -1775,7 +1801,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `loaded` hook replaceAttributes', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.name = 'changed in hook';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'changed in hook'});
}));
existingInstance.replaceAttributes({name: 'changed'}, function(err, instance) {
@ -2025,7 +2052,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `before save` hook on update', function(done) {
TestModel.observe('before save', function(ctx, next) {
ctx.data.name = 'hooked';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'hooked'});
next();
});
@ -2043,7 +2071,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
if (ctx.instance) {
ctx.instance.name = 'hooked';
} else {
ctx.data.name = 'hooked';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'hooked'});
}
next();
});
@ -2556,6 +2585,53 @@ module.exports = function(dataSource, should, connectorCapabilities) {
});
});
it('applies updates from `persist` hook on create', function(done) {
TestModel.observe('persist', (ctx, next) => {
// it's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
next();
});
// By default, the instance passed to create callback is NOT updated
// with the changes made through persist/loaded hooks. To preserve
// backwards compatibility, we introduced a new setting updateOnLoad,
// which if set, will apply these changes to the model instance too.
TestModel.settings.updateOnLoad = true;
TestModel.replaceOrCreate(
{name: 'a name'},
function(err, instance) {
if (err) return done(err);
instance.should.have.property('extra', 'hook data');
TestModel.findById(instance.id, (err, found) => {
if (err) return done(err);
found.should.have.property('extra', 'hook data');
done();
});
});
});
it('applies updates from `persist` hook on update', function(done) {
TestModel.observe('persist', (ctx, next) => {
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
next();
});
existingInstance.name = 'changed';
const data = existingInstance.toObject();
TestModel.replaceOrCreate(data, function(err, instance) {
if (err) return done(err);
instance.should.have.property('extra', 'hook data');
TestModel.findById(existingInstance.id, (err, found) => {
if (err) return done(err);
found.should.have.property('extra', 'hook data');
done();
});
});
});
it('triggers `loaded` hook on create', function(done) {
TestModel.observe('loaded', ctxRecorder.recordAndNext());
@ -2665,6 +2741,83 @@ module.exports = function(dataSource, should, connectorCapabilities) {
});
}
if (!dataSource.connector.replaceById) {
describe.skip('replaceById - not implemented', function() {});
} else {
describe('PersistedModel.replaceById', function() {
it('triggers hooks in the correct order on create', function(done) {
monitorHookExecution();
existingInstance.name = 'replaced name';
TestModel.replaceById(
existingInstance.id,
existingInstance.toObject(),
function(err, record, created) {
if (err) return done(err);
hookMonitor.names.should.eql([
'before save',
'persist',
'loaded',
'after save',
]);
done();
});
});
it('triggers `persist` hook', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext());
existingInstance.name = 'replaced name';
TestModel.replaceById(
existingInstance.id,
existingInstance.toObject(),
function(err, instance) {
if (err) return done(err);
const expected = {
where: {id: existingInstance.id},
data: {
id: existingInstance.id,
name: 'replaced name',
},
currentInstance: {
id: existingInstance.id,
name: 'replaced name',
extra: undefined,
},
};
const expectedContext = aCtxForModel(TestModel, expected);
expectedContext.isNewInstance = false;
ctxRecorder.records.should.eql(expectedContext);
done();
});
});
it('applies updates from `persist` hook', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
existingInstance.name = 'changed';
TestModel.replaceById(
existingInstance.id,
existingInstance.toObject(),
function(err, instance) {
if (err) return done(err);
instance.should.have.property('extra', 'hook data');
TestModel.findById(existingInstance.id, (err, found) => {
if (err) return done(err);
found.should.have.property('extra', 'hook data');
done();
});
});
});
});
}
describe('PersistedModel.deleteAll', function() {
it('triggers `access` hook with query', function(done) {
TestModel.observe('access', ctxRecorder.recordAndNext());
@ -3037,7 +3190,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `persist` hook', function(done) {
TestModel.observe('persist', ctxRecorder.recordAndNext(function(ctx) {
ctx.data.extra = 'hook data';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {extra: 'hook data'});
}));
TestModel.updateAll(
@ -3282,7 +3436,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('applies updates from `before save` hook on update', function(done) {
TestModel.observe('before save', function(ctx, next) {
ctx.data.name = 'hooked';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'hooked'});
next();
});
@ -3300,7 +3455,8 @@ module.exports = function(dataSource, should, connectorCapabilities) {
if (ctx.instance) {
ctx.instance.name = 'hooked';
} else {
ctx.data.name = 'hooked';
// It's crucial to change `ctx.data` reference, not only data props
ctx.data = Object.assign({}, ctx.data, {name: 'hooked'});
}
next();
});