Skip to content

Notify find 2.x #1061

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 23, 2016
Merged

Notify find 2.x #1061

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
61 changes: 37 additions & 24 deletions lib/dao.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1633,15 +1633,25 @@ DataAccessObject.find = function find(query, options, cb) {
// using all documents
// TODO [fabien] use default scope here?

var context = {
Model: self,
query: query,
hookState: hookState,
options: options,
};
self.notifyObserversOf('access', context, function(err, ctx) {
if (err) return cb(err);
if (options.notify === false) {
queryGeo(query);
} else {
withNotifyGeo();
}

function withNotifyGeo() {
var context = {
Model: self,
query: query,
hookState: hookState,
options: options,
};
self.notifyObserversOf('access', context, function(err, ctx) {
if (err) return cb(err);
queryGeo(ctx.query);
});
}
function queryGeo(query) {
function geoCallback(err, data) {
var memory = new Memory();
var modelName = self.modelName;
Expand All @@ -1662,7 +1672,7 @@ DataAccessObject.find = function find(query, options, cb) {
});

// FIXME: apply "includes" and other transforms - see allCb below
memory.all(modelName, ctx.query, options, cb);
memory.all(modelName, query, options, cb);
} else {
cb(null, []);
}
Expand All @@ -1673,10 +1683,9 @@ DataAccessObject.find = function find(query, options, cb) {
} else {
connector.all(self.modelName, {}, geoCallback);
}
});

// already handled
return cb.promise;
// already handled
return cb.promise;
}
}
}

Expand Down Expand Up @@ -1717,19 +1726,23 @@ DataAccessObject.find = function find(query, options, cb) {
}
}
if (obj !== undefined) {
context = {
Model: Model,
instance: obj,
isNewInstance: false,
hookState: hookState,
options: options,
};
if (options.notify === false) {
next(null, obj);
} else {
context = {
Model: Model,
instance: obj,
isNewInstance: false,
hookState: hookState,
options: options,
};

Model.notifyObserversOf('loaded', context, function(err) {
if (err) return next(err);
Model.notifyObserversOf('loaded', context, function(err) {
if (err) return next(err);

next(null, obj);
});
next(null, obj);
});
}
} else {
next();
}
Expand Down
152 changes: 59 additions & 93 deletions test/persistence-hooks.suite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,45 @@ module.exports = function(dataSource, should, connectorCapabilities) {
});
});

it('should not trigger hooks, if notify is false', function(done) {
monitorHookExecution();
TestModel.find(
{ where: { id: '1' }},
{ notify: false },
function(err, list) {
if (err) return done(err);
hookMonitor.names.should.be.empty();
done();
});
});

it('should not trigger hooks for geo queries, if notify is false',
function(done) {
monitorHookExecution();

TestModel.find(
{ where: { geo: [{ near: '10,20' }] }},
{ notify: false },
function(err, list) {
if (err) return done(err);
hookMonitor.names.should.be.empty();
done();
});
});

it('should apply updates from `access` hook', function(done) {
TestModel.observe('access', function(ctx, next) {
ctx.query = { where: { name: 'second' }};
next();
});

TestModel.find({ name: 'first' }, function(err, list) {
if (err) return done(err);
list.map(get('name')).should.eql(['second']);
done();
});
});

it('triggers `access` hook', function(done) {
TestModel.observe('access', ctxRecorder.recordAndNext());

Expand Down Expand Up @@ -123,10 +162,10 @@ module.exports = function(dataSource, should, connectorCapabilities) {
it('triggers `access` hook for geo queries', function(done) {
TestModel.observe('access', ctxRecorder.recordAndNext());

TestModel.find({ where: { geo: { near: '10,20' }}}, function(err, list) {
TestModel.find({ where: { geo: [{ near: '10,20' }] }}, function(err, list) {
if (err) return done(err);
ctxRecorder.records.should.eql(aCtxForModel(TestModel, {
query: { where: { geo: { near: '10,20' }}},
query: { where: { geo: [{ near: '10,20' }] }},
}));
done();
});
Expand Down Expand Up @@ -1664,24 +1703,13 @@ module.exports = function(dataSource, should, connectorCapabilities) {
{ id: existingInstance.id, name: 'new name' },
function(err, record, created) {
if (err) return done(err);
if (dataSource.connector.updateOrCreate) {
hookMonitor.names.should.eql([
'access',
'before save',
'persist',
'loaded',
'after save',
]);
} else {
hookMonitor.names.should.eql([
'access',
'loaded',
'before save',
'persist',
'loaded',
'after save',
]);
}
hookMonitor.names.should.eql([
'access',
'before save',
'persist',
'loaded',
'after save',
]);
done();
});
});
Expand Down Expand Up @@ -2015,26 +2043,14 @@ module.exports = function(dataSource, should, connectorCapabilities) {
},
}));
} else {
// For Unoptimized connector, the callback function `contextRecorder.recordAndNext`
// is called twice. As a result, contextRecorder.records
// returns an array and NOT a single instance.
ctxRecorder.records.should.eql([
aCtxForModel(TestModel, {
instance: {
id: existingInstance.id,
name: 'first',
extra: null,
},
isNewInstance: false,
options: { notify: false },
}),
ctxRecorder.records.should.eql(
aCtxForModel(TestModel, {
data: {
id: existingInstance.id,
name: 'updated name',
},
}),
]);
})
);
}
done();
});
Expand Down Expand Up @@ -2118,33 +2134,13 @@ module.exports = function(dataSource, should, connectorCapabilities) {
{ id: existingInstance.id, name: 'new name' },
function(err, record, created) {
if (err) return done(err);
if (dataSource.connector.replaceOrCreate) {
hookMonitor.names.should.eql([
'access',
'before save',
'persist',
'loaded',
'after save',
]);
} else {
// TODO: Please see loopback-datasource-juggler/issues#836
//
// loaded hook is triggered twice in non-atomic version:
// 1) It gets triggered once by "find()" in this chain:
// "replaceORCreate()->findOne()->find()",
// which is a bug; Please see this ticket:
// loopback-datasource-juggler/issues#836.
// 2) It, also, gets triggered in "replaceAttributes()"
// in this chain replaceORCreate()->replaceAttributes()
hookMonitor.names.should.eql([
'access',
'loaded',
'before save',
'persist',
'loaded',
'after save',
]);
};
hookMonitor.names.should.eql([
'access',
'before save',
'persist',
'loaded',
'after save',
]);
done();
});
});
Expand Down Expand Up @@ -2451,37 +2447,7 @@ module.exports = function(dataSource, should, connectorCapabilities) {
expected.isNewInstance =
connectorCapabilities.replaceOrCreateReportsNewInstance ?
false : undefined;

if (dataSource.connector.replaceOrCreate) {
ctxRecorder.records.should.eql(aCtxForModel(TestModel, expected));
} else {
// TODO: Please see loopback-datasource-juggler/issues#836
//
// loaded hook is triggered twice in non-atomic version:
// 1) It gets triggered once by "find()" in this chain:
// "replaceORCreate()->findOne()->find()",
// which is a bug; Please see this ticket:
// loopback-datasource-juggler/issues#836.
// 2) It, also, gets triggered in "replaceAttributes()"
// in this chain replaceORCreate()->replaceAttributes()
ctxRecorder.records.should.eql([
aCtxForModel(TestModel, {
data: {
id: existingInstance.id,
name: 'first',
},
isNewInstance: false,
options: { notify: false },
}),
aCtxForModel(TestModel, {
data: {
id: existingInstance.id,
name: 'replaced name',
},
isNewInstance: false,
}),
]);
}
ctxRecorder.records.should.eql(aCtxForModel(TestModel, expected));
done();
});
});
Expand Down