Skip to content
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

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

Merged
merged 1 commit into from
Jan 31, 2017
Merged

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

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
16 changes: 16 additions & 0 deletions doc/usage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ It defaults to `v1.0` and the default endpoint URLs are of the form:

https://<host-url>/v1.0/<api-endpoint>

Additionally, there is an optional `hooks` configuration parameter that let's you define database hooks. This is an example of a database hook that adds a property to a `Things` instance that is being created:
```
const config = {
db: {
hooks: {
'beforeCreate': (instance, options) => {
if (instance.$modelOptions.name.plural === 'Things') {
instance.properties = Object.assign({}, instance.properties, {
customProperty: 'custom property value'
});
}
}
}
}
};
```
For more details about the API you can check this [document](https://github.com/mozilla-sensorweb/sensorthings/blob/master/doc/API.md).

## Example requests
Expand Down
5 changes: 4 additions & 1 deletion src/models/db.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ export default config => {
host,
port,
dialect: 'postgres',
logging: false
logging: false,
define: {
hooks: config.hooks
}
});

db = {};
Expand Down
35 changes: 32 additions & 3 deletions test/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,45 @@ let app = express();
app.use(bodyParser.urlencoded({ extended: false }));
app.use(bodyParser.json());

let hooks = {};
const config = {
db: {
host: 'localhost',
port: 5432,
name: 'sensorthingstest',
user: 'postgres',
pass: '12345678'
pass: '12345678',
hooks: {
beforeCreate: () => {
hooks.beforeCreate = true
},
afterCreate: () => {
hooks.afterCreate = true
},
beforeDestroy: () => {
hooks.beforeDestroy = true
},
afterDestroy: () => {
hooks.afterDestroy = true
},
beforeUpdate: () => {
hooks.beforeUpdate = true
},
afterUpdate: () => {
hooks.afterUpdate = true
}
}
}
Copy link
Collaborator Author

@russnicoletti russnicoletti Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

};
}

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

exports = module.exports = app;
export default app;

exports.resetHooks = () => {
hooks = {}
};

exports.hookInvoked = name => {
return hooks[name] !== undefined
};
73 changes: 73 additions & 0 deletions test/test_client_db_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import db from '../src/models/db';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

import * as CONST from './constants';
import { hookInvoked, resetHooks } from './server';

// Database hooks (sequelize)
const dbHooks = [
'beforeCreate',
'afterCreate',
'beforeDestroy',
'afterDestroy',
'beforeUpdate',
'afterUpdate'
];

db().then(models => {
describe('Database hooks', () => {
beforeEach(() => {
resetHooks();
return models.sequelize.transaction(transaction => {
return Promise.all(Object.keys(CONST.entities).map(name => {
return models[name].destroy({ transaction, where: {} });
}));
});
});

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

// These tests fail. See:
// https://github.com/mozilla-sensorweb/sensorthings/issues/246
Object.keys(CONST.entities).forEach(entity => {
xit(entity + ' "destroy" hooks', done => {
models[entity].create(CONST[entity + 'Entity']).then(
instance => {
const id = instance.id;
models[entity].destroy({
where: { id } }).then(() => {
dbHooks.slice(2, 4).forEach(hook => {
hookInvoked(hook).should.be.true();
});
done();
});
});
})
});

Object.keys(CONST.entities).forEach(entity => {
it(entity + ' "update" hooks', done => {
models[entity].create(CONST[entity + 'Entity']).then(
instance => {
models[entity].update({ name: 'newname' }, {
where: { id: instance.id },
individualHooks: true
}).then(() => {
dbHooks.slice(4).forEach(hook => {
hookInvoked(hook).should.be.true();
});
done();
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Maybe try something easier:

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

})
});
});
});