[Issue #228] consumers to add model hooks #235

russnicoletti · 2017-01-21T00:02:00Z

No description provided.

coveralls · 2017-01-21T00:05:51Z

Coverage decreased (-0.4%) to 95.478% when pulling e5955a8 on russnicoletti:set-source-of-data into 5125096 on mozilla-sensorweb:master.

russnicoletti · 2017-01-21T00:12:59Z

example/example.js

-app.use('/', SensorThings(config));
+const clientHooks = {
+  'Things': {
+    'beforeCreate': (instance, options) => {


I have organized the hooks by Entity because using global hooks I wasn't able to determine on which Entity the hook was operating on.

You should be able to get the entity model by inspecting instance.$modelOptions.name.plural. Like in

sensorthings/src/models/association.js

Line 227 in 4a10133

const modelName = instance.$modelOptions.name.plural;

for example.

const hooks = { 'beforeCreate': instance => { console.log(instance.$modelOptions.name); } };

Hmm, I don't know why I had trouble with that before. I thought I tried it but I somehow came to the incorrect conclusion that it didn't work. I can't explain it. In any event, it does, of course, work as you say.

russnicoletti · 2017-01-21T00:13:43Z

src/models/db.js

+  'afterRestore',
+  'beforeUpdate',
+  'afterUpdate'
+];


I've started with the "instance" hooks because those seem most relevant to a client.

ferjm

Thank you Russ. Good start.

ferjm · 2017-01-23T10:31:19Z

example/example.js

-app.use('/', SensorThings(config));
+const clientHooks = {
+  'Things': {
+    'beforeCreate': (instance, options) => {


You should be able to get the entity model by inspecting instance.$modelOptions.name.plural. Like in

sensorthings/src/models/association.js

Line 227 in 4a10133

const modelName = instance.$modelOptions.name.plural;

for example.

const hooks = { 'beforeCreate': instance => { console.log(instance.$modelOptions.name); } };

ferjm · 2017-01-23T10:35:32Z

src/models/db.js

@@ -28,6 +28,19 @@ const IDLE           = 0
 const INITIALIZING   = 1;
 const READY          = 2;

+const dbHooks = [


s/dbHooks/hooks/g

(You are already in db related code)

ferjm · 2017-01-23T10:37:43Z

src/models/db.js

@@ -82,6 +96,23 @@ export default config => {
    if ('associate' in db[modelName]) {
      db[modelName].associate(db);
    }
+
+    const addDbHooks = () => {


You don't need to define a function for each iteration of the loop. Just exec its content directly.

ferjm · 2017-01-23T10:42:49Z

src/models/db.js

+        return;
+      }
+
+      if (clientHooks[modelName]) {


As mentioned before, you can get the model name inside the hook itself. So you would simply do:

Object.keys(db).forEach(modelName => { const model = db[modelName]; if ('associate' in model) { model.associate(db); } if (!clientHooks) { return; } hooks.forEach(hook => { const clientHook = clientHooks[hook] clientHook && model.hook(hook, clientHook); } });

ferjm · 2017-01-23T10:43:43Z

src/sensorthings.js

@@ -24,14 +24,14 @@ import dataArrayRouter from './extensions/data_array';

 const API_VERSION = 'v1.0';

-module.exports = (config) => {
+module.exports = (config, clientHooks) => {
  if (!config || !config.db) {
    throw new Error('Missing or malformed config');
  }



if (clientHooks && typeof clientHooks !== 'object') { throw new Error('Malformed client hooks'); }

ferjm · 2017-01-23T10:47:01Z

src/sensorthings.js

@@ -24,14 +24,14 @@ import dataArrayRouter from './extensions/data_array';

 const API_VERSION = 'v1.0';

-module.exports = (config) => {
+module.exports = (config, clientHooks) => {


Maybe move clientHooks inside the config object.

coveralls · 2017-01-24T04:04:45Z

Coverage decreased (-0.3%) to 95.572% when pulling 26824b2 on russnicoletti:set-source-of-data into 5026612 on mozilla-sensorweb:master.

coveralls · 2017-01-24T04:05:42Z

Coverage decreased (-0.3%) to 95.572% when pulling 26824b2 on russnicoletti:set-source-of-data into 5026612 on mozilla-sensorweb:master.

coveralls · 2017-01-24T04:06:27Z

Coverage decreased (-0.3%) to 95.572% when pulling 26824b2 on russnicoletti:set-source-of-data into 5026612 on mozilla-sensorweb:master.

ferjm

Looks good!

We need some tests before merging though.

russnicoletti · 2017-01-25T05:46:25Z

test/server.js

+          afterUpdate: 'afterUpdate'
+        });
+      }
+    }
  }


I wasn't able to initialize the db hooks only for the "db hooks" test (because it is essentially a singleton, I don't see a way to initialize the db with hooks for the "db hooks" test and without db hooks for all the other tests). Therefore, many tests fail because instances have properties the tests are not expecting. I will think about possible solutions but I would appreciate your input on this issue.

Just don't use server.js. Create a dedicated express app for the hook tests inside test_client_db_hooks.js

As I'm addressing your other comments it occurs to me the hooks test don't need an express app. The hooks test is using sequelize to create the model instances.

In any event, even if it did use a dedicated express app, I'm finding that doing so results in the hooks not be set on the models. I believe this is because the dedicated express app ends up using the same 'db' module as the common express app (server.js) used by the other tests, and the 'db' module acts like a singleton: once it is instantiated and becomes 'READY', it will return a promise containing its instance. The 'db' instance used by the hooks test will not have the hooks in place because the common express app does not specify the hooks and it is the common express app that first instantiates the 'db' module.

For now, I will have the common express app create the db hooks (using your suggestion from here: https://github.com/mozilla-sensorweb/sensorthings/pull/235/files#r97766628 so that the hooks won't add additional properties that break other tests).

As I was creating a dedicated express app I realized the hook tests don't need one -- it is using sequelize to operate on the models directly. But event if it did, it seems to me the issue of enabling the hooks for the hooks tests but not for the other tests is related to the db module (db.js) essentially being a singleton; once it is instantiated and becomes READY, subsequent instantiations will return a promise the the originally instantiated instance. Since the majority of the tests will instantiate the db without the hooks, when the hooks test instantiates the db with a config that has the hooks, the db instance returned will be the one without the hooks because that instance was created first. I don't see a way around that.

russnicoletti · 2017-01-25T05:49:38Z

test/test_client_db_hooks.js

+     [ things, ThingsEntity ]
+    ].forEach(testObj => {
+      it(testObj[0] + ' db \"create\" hooks', done => {
+        models[testObj[0]].create(testObj[1]).then(instance => {


If this approach is ok, I would add "destroy" and "update" tests to exercise the other hooks (I'm not sure what action will trigger the "Restore" hooks).

ferjm · 2017-01-25T12:07:15Z

test/server.js

+          afterUpdate: 'afterUpdate'
+        });
+      }
+    }
  }


Just don't use server.js. Create a dedicated express app for the hook tests inside test_client_db_hooks.js

ferjm · 2017-01-25T12:08:32Z

test/test_client_db_hooks.js

+        return Promise.all(Object.keys(entities).map(name => {
+          return models[name].destroy({ transaction, where: {} });
+        }));
+      }).then(() => done());


I know we have this pattern all over the code, but beforeEach takes a Promise as well. So you can get rid of done and do

beforeEach(() => { return models.sequelize.transaction(...); });

ferjm · 2017-01-25T12:09:49Z

test/test_client_db_hooks.js

+  SensorsEntity,
+  things,
+  ThingsEntity
+} from './constants';


These are a lot of constants :D

import * as CONST from './constants';

ferjm · 2017-01-25T12:10:45Z

test/test_client_db_hooks.js

+     [ observedProperties, ObservedPropertiesEntity ],
+     [ sensors, SensorsEntity ],
+     [ things, ThingsEntity ]
+    ].forEach(testObj => {


Isn't this the same as

Object.keys(entities).forEach(entity => {...});

?
You can do entity + 'Entity'

ferjm · 2017-01-25T12:13:35Z

src/models/db.js

+  'afterRestore',
+  'beforeUpdate',
+  'afterUpdate'
+];


Maybe add these as constants in constants.js, so you can use them from the tests as well.

ferjm · 2017-01-25T12:18:40Z

test/server.js

+    pass: '12345678',
+    hooks: {
+      'beforeValidate': (instance, options) => {
+        instance.properties = Object.assign({}, instance.properties, {


Why .properties? :) This is simpler:

instance.beforeValidate = 'beforeValidate';

ferjm · 2017-01-25T12:22:33Z

test/server.js

+      'beforeDestroy': (instance, options) => {
+        instance.properties = Object.assign({}, instance.properties, {
+          beforeDestroy: 'beforeDestroy'
+        });


I suspect setting an instance value on instance destruction hooks won't be useful for us. Destroying an instance does not return the instance, so you won't be able to check that the value was set on the instance. You need a different approach.

ferjm · 2017-01-25T12:22:48Z

test/server.js

+          afterDestroy: 'afterDestroy'
+        });
+      },
+      'beforeRestore': (instance, options) => {


ferjm · 2017-01-25T12:24:35Z

src/models/db.js

+    hooks.forEach(hook => {
+      const clientHook = config.hooks[hook];
+      clientHook && db[modelName].hook(hook, clientHook);
+    });


It seems that we can create global/universal hooks \o/ So you don't need to do this for each model.

ferjm · 2017-01-25T12:28:11Z

test/test_client_db_hooks.js

+            const instanceHook = instanceProperties[i + propIndex][1];
+            expectedHook.should.be.equal(instanceHook);
+          }
+        });


Maybe try something easier:

[CONST.beforeValidate, CONST.afterValidate, CONST.beforeCreate, CONST.afterCreate].forEach(hook => { instance[hook].should.be.equal(hook); });

russnicoletti · 2017-01-25T19:39:43Z

test/server.js

+          afterUpdate: 'afterUpdate'
+        });
+      }
+    }
  }


As I'm addressing your other comments it occurs to me the hooks test don't need an express app. The hooks test is using sequelize to create the model instances.

In any event, even if it did use a dedicated express app, I'm finding that doing so results in the hooks not be set on the models. I believe this is because the dedicated express app ends up using the same 'db' module as the common express app (server.js) used by the other tests, and the 'db' module acts like a singleton: once it is instantiated and becomes 'READY', it will return a promise containing its instance. The 'db' instance used by the hooks test will not have the hooks in place because the common express app does not specify the hooks and it is the common express app that first instantiates the 'db' module.

For now, I will have the common express app create the db hooks (using your suggestion from here: https://github.com/mozilla-sensorweb/sensorthings/pull/235/files#r97766628 so that the hooks won't add additional properties that break other tests).

russnicoletti · 2017-01-26T01:40:57Z

src/models/db.js

+  'afterDestroy',
+  'beforeUpdate',
+  'afterUpdate'
+];


I removed beforeRestore, and afterRestore. My thinking was they would not be run during the use of our ST API and therefore it would be dead code to add the hooks and let the clients set them.

russnicoletti · 2017-01-26T01:43:12Z

test/server.js

+      // associations with `onDelete: 'cascade'` and the option `hooks: true`"
+      // TBD It seems our ST API implementation does not exercise these hooks.
+      // Perhaps we should not implement them.
+/*


The Sequelize documentation makes me think the beforeDestroy and afterDestroy hooks will not be run during the use of our ST API. And I didn't see any evidence of them run during our coverage tests. If they won't run during the use of our ST API, should we not implement them?

Maybe what is wrong is the way we define associations, we should be deleting on cascade... File an issue to investigate this and reference it here, please.

Issue created: #246

russnicoletti · 2017-01-26T01:44:47Z

test/test_client_db_hooks.js

+
+    xit('db "update" hooks', done => {
+      // TODO The 'before/afterUpdate' hooks are not being run during this
+      // test. Why not?


I saw that the 'before/afterUpdate' hooks were running during other tests but during this test they do not run. I haven't been able to figure out why they don't run during this test.

Not sure what's wrong, try:

models[CONST.datastreams].create(CONST.DatastreamsEntity) .then(instance => { models[CONST.datastreams].update({ name: 'russ' }, { where: { id: instance.id }, individualHooks: true }).then(() => done()); });

russnicoletti · 2017-01-26T16:20:33Z

test/test_client_db_hooks.js

@@ -0,0 +1,62 @@
+import db         from '../src/models/db';


As I was creating a dedicated express app I realized the hook tests don't need one -- it is using sequelize to operate on the models directly. But event if it did, it seems to me the issue of enabling the hooks for the hooks tests but not for the other tests is related to the db module (db.js) essentially being a singleton; once it is instantiated and becomes READY, subsequent instantiations will return a promise the the originally instantiated instance. Since the majority of the tests will instantiate the db without the hooks, when the hooks test instantiates the db with a config that has the hooks, the db instance returned will be the one without the hooks because that instance was created first. I don't see a way around that.

You are right! Sorry, I didn't think about that.

I guess one way to workaround this would be to use a different db name for these tests... but it is probably not the best solution and definitely not worth the effort. Let's just use server.js.

The reason why I suggested not using server.js for the hooks is because we are modifying the instances within the hooks and that may end up affecting the tests (i.e. we are not currently controlling when we send extra parameters within API responses, but we should at some point do it). You already have to try a different approach for the update case, so I think it'll probably be better to use it for all the other cases. Check my suggestion for the update case above.

I completely agree that modifying the responses of all the tests is not a good idea; in fact, it is a very bad idea :), and it is one reason I like your suggestion for the update case; I will use it for all the hooks.

ferjm · 2017-01-26T16:46:33Z

test/server.js

+      // associations with `onDelete: 'cascade'` and the option `hooks: true`"
+      // TBD It seems our ST API implementation does not exercise these hooks.
+      // Perhaps we should not implement them.
+/*


Maybe what is wrong is the way we define associations, we should be deleting on cascade... File an issue to investigate this and reference it here, please.

ferjm · 2017-01-26T16:52:52Z

test/test_client_db_hooks.js

@@ -0,0 +1,62 @@
+import db         from '../src/models/db';


You are right! Sorry, I didn't think about that.

I guess one way to workaround this would be to use a different db name for these tests... but it is probably not the best solution and definitely not worth the effort. Let's just use server.js.

The reason why I suggested not using server.js for the hooks is because we are modifying the instances within the hooks and that may end up affecting the tests (i.e. we are not currently controlling when we send extra parameters within API responses, but we should at some point do it). You already have to try a different approach for the update case, so I think it'll probably be better to use it for all the other cases. Check my suggestion for the update case above.

ferjm · 2017-01-26T17:02:34Z

test/server.js

+      'afterUpdate': (instance) => {
+        // TODO Updating an instance does not return the instance. Therefore,
+        // it still needs to be determined how to test this hook.
+      }


One thing you could do here is to keep an object where you add the hooks that has been executed as properties. Something like:

let hooks = {}; const config = { db: { [...] hooks: { beforeCreate: () => { hooks[beforeCreate] = true }, afterCreate: () => { hooks[afterCreate] = true } } } }

Then you can export two functions (I'm sure you'll find better names ;)):

export const resetHooks = () => { hooks = {} }; export const hooked = name => { return hook[name] !== undefined };

So before each test, you can call resetHooks and during the test you can check if the hook was executed or not.

ferjm · 2017-01-26T17:08:21Z

src/models/db.js

+      sequelize.addHook(hook, clientHook);
+    }
+  });
+


Did you check this comment #235 (comment) ?

Yes, that's why I'm not adding the hooks within the forEach loop above. I'm adding the hooks one time, using addHook, not once per model. Perhaps I'm missing the point of the above comment.

I see. I was thinking more about something like:

const sequelize = new Sequelize(name, user, password, { host, port, dialect: 'postgres', define: { hooks: config.hooks }, logging: false });

at https://github.com/mozilla-sensorweb/sensorthings/blob/master/src/models/db.js#L62

Aha, somehow I didn't pick up on that approach from the documentation. I agree it is the best approach (most concise). One benefit of the previous implementation is we were only setting the hooks that we were testing. Now, a client can set any hook while we are only testing a limited number of them. I'm ok with that if you're ok with that.

ferjm · 2017-01-26T17:18:26Z

test/test_client_db_hooks.js

+
+    xit('db "update" hooks', done => {
+      // TODO The 'before/afterUpdate' hooks are not being run during this
+      // test. Why not?


Not sure what's wrong, try:

models[CONST.datastreams].create(CONST.DatastreamsEntity) .then(instance => { models[CONST.datastreams].update({ name: 'russ' }, { where: { id: instance.id }, individualHooks: true }).then(() => done()); });

coveralls · 2017-01-28T01:17:53Z

Coverage increased (+0.02%) to 95.927% when pulling c859a05 on russnicoletti:set-source-of-data into e271773 on mozilla-sensorweb:master.

ferjm

Thank you Russ. LGTM with the comments addressed.

ferjm · 2017-01-30T14:32:48Z

test/server.js


 app.use('/', SensorThings(config));

-exports = module.exports = app;
+exports.app = app;


use export default app. This way you don't need to do import { app } from './server'; everywhere.

ferjm · 2017-01-30T14:35:45Z

test/test_client_db_hooks.js

+    Object.keys(CONST.entities).forEach(entity => {
+      it(entity + ' "create" hooks', done => {
+        models[entity].create(CONST[entity + 'Entity']).then(() => {
+          CONST.dbHooks.slice(0, 2).forEach(hook => {


nit: Do not make the test dependent on the order of an array that is not within this file, please.

[CONST.beforeCreate, CONST.afterCreate].forEach(...);

I'm going to move the dbHooks from src/constants.js (where they are not needed anymore based on #235 (comment)) into test_client_db_hooks.js).

ferjm · 2017-01-30T14:36:55Z

It would be great if you could briefly document this on README.md.

coveralls · 2017-01-30T21:18:09Z

Coverage remained the same at 95.906% when pulling 2f2a528 on russnicoletti:set-source-of-data into e271773 on mozilla-sensorweb:master.

coveralls · 2017-01-30T21:27:13Z

Coverage remained the same at 95.906% when pulling 105ea1c on russnicoletti:set-source-of-data into e271773 on mozilla-sensorweb:master.

coveralls · 2017-01-30T21:44:05Z

Coverage remained the same at 95.906% when pulling 54c5b8c on russnicoletti:set-source-of-data into e271773 on mozilla-sensorweb:master.

russnicoletti commented Jan 21, 2017

View reviewed changes

russnicoletti requested review from albertopq and ferjm January 21, 2017 00:14

ferjm reviewed Jan 23, 2017

View reviewed changes

russnicoletti force-pushed the set-source-of-data branch 3 times, most recently from 091d392 to 26824b2 Compare January 24, 2017 04:02

ferjm reviewed Jan 24, 2017

View reviewed changes

russnicoletti force-pushed the set-source-of-data branch 2 times, most recently from efe2f7c to 638ea7b Compare January 25, 2017 05:41

russnicoletti commented Jan 25, 2017

View reviewed changes

ferjm reviewed Jan 25, 2017

View reviewed changes

russnicoletti force-pushed the set-source-of-data branch from 638ea7b to 9d653f2 Compare January 26, 2017 01:36

russnicoletti commented Jan 26, 2017

View reviewed changes

russnicoletti requested a review from ferjm January 26, 2017 16:19

russnicoletti commented Jan 26, 2017

View reviewed changes

ferjm reviewed Jan 26, 2017

View reviewed changes

russnicoletti force-pushed the set-source-of-data branch 2 times, most recently from 30f8b18 to c859a05 Compare January 28, 2017 01:14

russnicoletti requested a review from ferjm January 28, 2017 01:18

ferjm approved these changes Jan 30, 2017

View reviewed changes

russnicoletti force-pushed the set-source-of-data branch from c859a05 to 2f2a528 Compare January 30, 2017 21:12

russnicoletti force-pushed the set-source-of-data branch from 2f2a528 to 105ea1c Compare January 30, 2017 21:19

[Issue mozilla-sensorweb#228] consumers to add model hooks

54c5b8c

russnicoletti force-pushed the set-source-of-data branch from 105ea1c to 54c5b8c Compare January 30, 2017 21:37

ferjm merged commit 77b44b6 into mozilla-sensorweb:master Jan 31, 2017

ferjm mentioned this pull request Jan 31, 2017

Implement a way for consumers to add model hooks #228

Closed

[Issue #228] consumers to add model hooks #235

[Issue #228] consumers to add model hooks #235

Conversation

russnicoletti commented Jan 21, 2017

coveralls commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 24, 2017

coveralls commented Jan 24, 2017

coveralls commented Jan 24, 2017

ferjm left a comment

Choose a reason for hiding this comment

russnicoletti Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russnicoletti Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russnicoletti Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferjm Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

russnicoletti Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 28, 2017

ferjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russnicoletti Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

ferjm commented Jan 30, 2017

coveralls commented Jan 30, 2017

coveralls commented Jan 30, 2017

coveralls commented Jan 30, 2017

russnicoletti Jan 25, 2017 •

edited

Loading

russnicoletti Jan 26, 2017 •

edited

Loading

russnicoletti Jan 26, 2017 •

edited

Loading

ferjm Jan 30, 2017 •

edited

Loading

russnicoletti Jan 30, 2017 •

edited

Loading

russnicoletti Jan 30, 2017 •

edited

Loading