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.

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

Do not inject include twice, expand and validate include in aggregate #5106

Merged
merged 4 commits into from Jan 5, 2016

Conversation

3 participants
@Verdier
Contributor

Verdier commented Dec 28, 2015

Refactor $injectScope to better checks existence of include in options.include based on the related association. This PR also fix a bug where include are not validated/expanded in model.aggregate.

Fix #4986
Fix #5121
Fix Cannot read property 'association' of undefined

@Verdier

This comment has been minimized.

Contributor

Verdier commented Dec 29, 2015

@mickhansen could you review my pull request?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Dec 29, 2015

Applying the same scope twice should ideally not fail.
Looks like PR might break using multiple scopes?

@Verdier

This comment has been minimized.

Contributor

Verdier commented Dec 29, 2015

You are right @mickhansen applying scope twice should be neutral. I'll have a look.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Dec 29, 2015

@mickhansen I have found the problem, $injectScope badly check the presence of the included model into options.include:

If $injectScope is called but options.include[i].as is defined and equal to the default value of the related association, the implementation does not detect that models are equal. This is fixed by checking the related association.as property.

To do that, I have made $injectScope an instance method.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Dec 29, 2015

@mickhansen every thing is green. I think it's ready to be merged.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Dec 29, 2015

Gonna need @janmeier to take a look on this one.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Dec 31, 2015

@janmeier @sushantdhiman this PR fix #4986 (like #5090) but is a better solution, because it check equality based on the related association.

Compared to this one, #5090 can result in false equality. If sameModel is true and item.as is defined but not scopeInclude.as include will be considered as equal even if item.as does not refer to the default association as. In this case includes are not equal, and both of them must be pushed in options.include.

@janmeier could you review?

@Verdier Verdier changed the title from Do not inject scope twice, fix count with include to Do not inject include twice, expand and validate include in aggregate Dec 31, 2015

return item.as === scopeInclude.as;
} else {
var association = scopeInclude.association || self.getAssociation(scopeInclude.model, scopeInclude.as);
return association ? item.as === association.as : false;

This comment has been minimized.

@Verdier

Verdier Dec 31, 2015

Contributor

@janmeier the new logic is here, every thing else is to have self available.

if (!options.include.length) {
delete options.include;
}
}

This comment has been minimized.

@mickhansen

mickhansen Jan 4, 2016

Contributor

Did anything actually change here other than changing it away from short circuiting?

This comment has been minimized.

@Verdier

Verdier Jan 4, 2016

Contributor

Mmmm, yes... I think it's really more readable like this. But we could revert...

This comment has been minimized.

@mickhansen

mickhansen Jan 4, 2016

Contributor

I just prefer to keep style changes out of PR's that fix/change functionality as much as possible, easier to focus on the matter at hand.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 4, 2016

Preferred having it as a non-prototype method but @janmeier will have to review and decide.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

Ok, waiting for @janmeier then

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

Reverted @mickhansen

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

The only job of $injectScope is to inject this.$scope into options.include. It's probably better that the method was on the prototype.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 4, 2016

$injectScope's job was to inject an arbitrary scope into an arbitrary options set was it not?
However you are right that that's all it's doing at the moment.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

Yep, but the used scope is alway set as this.$scope and, at the end, only this.$scope is injected. That should remain like this forever, otherwise it's probably an error. Setting $injectScope as instance method without scope parameter ensure that the function is not misused.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 4, 2016

It's true that the current use of the method is like that. However i'm sure that the intention was for it work for arbitrary scopes and options. But @janmeier will have to say since he wrote it.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

I suspect that just was for testing purpose, but waiting for @janmeier.

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 4, 2016

I guess it was for testing purposes - I agree with @Verdier that if you ever try to inject other than this.$scope its probably an error

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 4, 2016

Just needs a couple of changelog entries, then this should be good to go

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

@mickhansen could you add the changelog entries?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 4, 2016

@Verdier You might aswell since this needs a squash aswell, just add a notice about fixing the issue at hand here and reference the PR/issues

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 4, 2016

No problem, I will do that tomorrow. I was thinking that's you who keep the
change log in sync.

You want me to squash the test commits?

Le lun. 4 janv. 2016 17:54, Mick Hansen notifications@github.com a écrit :

@Verdier https://github.com/Verdier You might aswell since this needs a
squash aswell, just add a notice about fixing the issue at hand here and
reference the PR/issues


Reply to this email directly or view it on GitHub
#5106 (comment).

@Verdier

This comment has been minimized.

Contributor

Verdier commented Jan 5, 2016

Squashed and changelog added @mickhansen.

mickhansen added a commit that referenced this pull request Jan 5, 2016

Merge pull request #5106 from Verdier/master
Do not inject include twice, expand and validate include in aggregate

@mickhansen mickhansen merged commit aee7fec into sequelize:master Jan 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 5, 2016

Thanks @Verdier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment