Add association hook #9590

mirkojotic · 2018-06-25T02:03:12Z

Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Added hooks for association creation as per #9113

mirkojotic · 2018-06-25T02:05:25Z

I may be way off on this one so feel free to critique it, I did it as I understood the issue but that could be very wrong.

I've added some new integration tests but I might have missed another place where I should add them.

codecov · 2018-06-25T02:09:27Z

Codecov Report

Merging #9590 into master will increase coverage by <.01%.
The diff coverage is 100%.

sushantdhiman

beforeAssociate / afterAssociate, emit association / options in afterAssociate

mirkojotic · 2018-07-04T13:19:12Z

@sushantdhiman I've made changes to the name of the hooks and changed what gets emitted with afterAssociate not sure if that's what your comment meant.

sushantdhiman · 2018-07-04T13:21:32Z

lib/associations/mixin.js

    // the id is in the foreign table
    const association = new Type(source, target, _.extend(options, source.options));
    source.associations[association.associationAccessor] = association;

    association._injectAttributes();
    association.mixin(source.prototype);

+    if (options.useHooks) {
+      this.runHooks('afterAssociate', target, options);


Instead of target, pass association (that is what I meany by emit association :p)

Ah, sorry about that. Do we still need to pass the source in after?

No, we just want to know what association got created, source and target info is available on this object

@sushantdhiman Ok. Here is another stab at it. I'm not passing source or target anymore since they are available within hook through this. We're passing association object now. I'm open to more suggestions. Tests should be passing but they might be a bit wonky so I'll accept any tips.

sushantdhiman · 2018-07-05T05:41:05Z

I was thinking more about usecase of these hooks, I think we need to support

beforeHasOneAssociate / afterHasOneAssociate
beforeHasManyAssociate / afterHasManyAssociate
beforeBelongsToAssociate / afterBelongsToAssociate
beforeBelongsToManyAssociate / afterBelongsToManyAssociate

Each before* hook should provide target / options, where as each after* hook should only provide association

Just unit tests will be enough for these hooks, you can add them in respective association test file https://github.com/sequelize/sequelize/tree/master/test/unit/associations

I'm not passing source or target anymore since they are available within hook through this

I agree source is available as this but how target is available?

mirkojotic · 2018-07-05T15:50:19Z

@sushantdhiman Ok. I've tried to change stuff up as per your direction. Tests are passing locally on all dialects but for whatever reason they've failed on Travis. I haven't touched any files that might mess them up...

mickhansen · 2018-07-06T08:56:47Z

I'd suggest just one before/after hook for associations, let userland inspect the association for type, less surface area.

mickhansen · 2018-07-06T08:58:35Z

lib/associations/mixin.js

@@ -26,13 +26,21 @@ const Mixin = {

    options = _.extend(options, _.omit(source.options, ['hooks']));

+    if (options.useHooks) {
+      this.runHooks('beforeHasManyAssociate', target, options);


Rather than target, options i'd suggest.

{source, target, ...options, association}, association only for after of course.
/cc @sushantdhiman

We can have {source, target} for before* hooks, {source, target, association} for after* hooks, I would like to keep options separate as in future we may have conflicting options

sushantdhiman · 2018-07-06T12:00:14Z

I'd suggest just one before/after hook for associations, let userland inspect the association for type, less surface area.

I don't see a good way to identify what kind of association is being created, especially in before* hooks. We can try

different before hooks, single after hook
single before after hook, with association constructor passed as type

mirkojotic · 2018-07-06T14:21:42Z

@mickhansen @sushantdhiman

I'm willing to work with either of these options, just let me know what you guys think is a better option.

sushantdhiman · 2018-07-17T04:27:53Z

@mirkojotic Sorry for this wait, please implement this format if you still like to continue.

beforeAssociate, two options passed {source, target, type}, options
afterAssociate, two options passed {source, target, type, association}, options

mirkojotic · 2018-07-19T02:05:31Z

Hey @sushantdhiman I gave it another shot. Let me know if further modifications are required.

mirkojotic added 2 commits Jun 25, 2018

feat(add-association-hook): association creation hooks

e85601b

feat(add-association-hook): association creation hooks

Loading status checks…

a3a06f7

sushantdhiman reviewed Jul 4, 2018

View changes

feat(add-association-hook): association creation hooks

Loading status checks…

573736c

sushantdhiman reviewed Jul 4, 2018

View changes

feat(add-association-hook): association creation hooks

Loading status checks…

aecb1ff

feat(add-association-hooks): association creation hooks

Loading status checks…

7a1644f

sushantdhiman approved these changes Jul 6, 2018

View changes

sushantdhiman requested a review from sequelize/orm Jul 6, 2018

mickhansen reviewed Jul 6, 2018

View changes

mirkojotic added 2 commits Jul 19, 2018

feat(add-association-hook): association creation hooks

fcd25e8

feat(add-association-hooks): association creation hooks

Loading status checks…

5e40f82

sushantdhiman approved these changes Jul 19, 2018

View changes

sushantdhiman mentioned this pull request Jul 19, 2018

add association hooks #9113

Closed

sequelize / sequelize

Add association hook #9590

Add association hook #9590

mirkojotic commented Jun 25, 2018

mirkojotic commented Jun 25, 2018

codecov bot commented Jun 25, 2018 •

edited

sushantdhiman left a comment

mirkojotic commented Jul 4, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sushantdhiman commented Jul 5, 2018

mirkojotic commented Jul 5, 2018

mickhansen commented Jul 6, 2018

This comment has been minimized.

This comment has been minimized.

sushantdhiman commented Jul 6, 2018

mirkojotic commented Jul 6, 2018

sushantdhiman commented Jul 17, 2018

mirkojotic commented Jul 19, 2018

sequelize / sequelize

Add association hook #9590

Add association hook #9590

Conversation

mirkojotic commented Jun 25, 2018

Pull Request check-list

Description of change

mirkojotic commented Jun 25, 2018

codecov bot commented Jun 25, 2018 • edited

Codecov Report

sushantdhiman left a comment

mirkojotic commented Jul 4, 2018

This comment has been minimized.

sushantdhiman Jul 4, 2018 Contributor

This comment has been minimized.

mirkojotic Jul 4, 2018 Author Contributor

This comment has been minimized.

sushantdhiman Jul 4, 2018 Contributor

This comment has been minimized.

mirkojotic Jul 4, 2018 Author Contributor

sushantdhiman commented Jul 5, 2018

mirkojotic commented Jul 5, 2018

mickhansen commented Jul 6, 2018

This comment has been minimized.

mickhansen Jul 6, 2018 Contributor

This comment has been minimized.

sushantdhiman Jul 6, 2018 Contributor

sushantdhiman commented Jul 6, 2018

mirkojotic commented Jul 6, 2018

sushantdhiman commented Jul 17, 2018

mirkojotic commented Jul 19, 2018

codecov bot commented Jun 25, 2018 •

edited

sushantdhiman Jul 4, 2018
Contributor

mirkojotic Jul 4, 2018
Author Contributor

sushantdhiman Jul 4, 2018
Contributor

mirkojotic Jul 4, 2018
Author Contributor

mickhansen Jul 6, 2018
Contributor

sushantdhiman Jul 6, 2018
Contributor