Bug: Hooks only work when model name is the same as endpoint name

[nabilfreeman]

This commit:

bdd45d6

appears to have broken hooks in situations where the lowercase modelName would not be the same as the endpoint name.

I think this is because on this line, the hooks for all the routes are being retrieved with the key modelNameLower:

kalamata/src/index.js

Line 52 in bdd45d6

var beforeHooks = hooks[opts.endpointName].before;

var beforeHooks = hooks[modelNameLower].before;
var afterHooks = hooks[modelNameLower].after;

Also, it looks like beforeHooks and afterHooks are actually being re-declared and overwriting a different value a few lines up.

but in hookFn, all the hooks are being attached with this key:

kalamata/src/index.js

Line 392 in bdd45d6

hooks[opts.endpointName][prefix][type].push(fn);

hooks[opts.endpointName][prefix][type].push(fn);

So it seems that the code either needs to use modelNameLower in both places, or use the endpointName in both places. I think I managed to fix it simply by deleting the re-declaration of beforeHooks and afterHooks but this might have consequences I haven't recognised.

[nabilfreeman]

@mikec I know this was a while back, but can you advise on the above? If there is a bug I would be happy to PR the fix, otherwise I think I will have to run off a forked version with lines 52 and 53 removed 😟

[nabilfreeman]

Here is an example where hooks are broken (they just don't run):

this.kalamata.expose(require('./some_bookshelf_model.js'), {
	modelName: 'WorkingHour',
	collectionName: 'WorkingHours',
	endpointName: 'working_hours'
});

And one where it works properly:

this.kalamata.expose(require('./other_bookshelf_model.js'), {
	modelName: 'User',
	collectionName: 'Users',
	endpointName: 'users'
});

In both situations the hooks are created (and are visible on the kalamata object), but they only run for ones like the second example.

I have a fixed fork at https://github.com/nabilfreeman/kalamata/blob/master/src/index.js, just removed the code that was overwriting the hooks basically.

[mikec]

@nabilfreeman thanks for finding this, that's a pretty nasty bug!

I think this will fix it #7

Try that out for a bit, if it's working for you then I'll merge it in!

Wish that I had added better unit testing and more modularization in this code base. Having everything in index.js is a little ridiculous!

[nabilfreeman]

Hey @mikec, thanks for getting back to me! Really appreciate the maintenance. I'm trying your branch today and will let you know how it works 👍

[nabilfreeman]

Update: works well! Thanks for the help 👌 @mikec

[mikec]

@nabilfreeman awesome! 🎉