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.

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

Scopes with foreign key #9735

Merged
merged 17 commits into from Oct 28, 2018
Merged

Scopes with foreign key #9735

merged 17 commits into from Oct 28, 2018

Conversation

@u9r52sld
Copy link
Contributor

@u9r52sld u9r52sld commented Jul 29, 2018

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

Confirming options after foreign key is added.

See issue: #6245

@codecov
Copy link

@codecov codecov bot commented Jul 29, 2018

Codecov Report

Merging #9735 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9735      +/-   ##
==========================================
+ Coverage    96.3%   96.32%   +0.01%     
==========================================
  Files          63       63              
  Lines        9413     9408       -5     
==========================================
- Hits         9065     9062       -3     
+ Misses        348      346       -2
Impacted Files Coverage Δ
lib/model.js 96.8% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 122bd17...df69b87. Read the comment docs.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jul 30, 2018

Thanks for this fix. I want to do some tests on my local setup after that I will merge this PR

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 1, 2018

Tested this locally, this wont work with defaultScope but I have no good suggestion to handle that case right now. Apart from that it works well with other scopes defined by options.scope

@u9r52sld I can merge this in current state or would you like to fix defaultScope case as well?

@u9r52sld
Copy link
Contributor Author

@u9r52sld u9r52sld commented Aug 1, 2018

@sushantdhiman Thank you for testing. Of course I'll fix defaultScope case. Please hold on.

@u9r52sld
Copy link
Contributor Author

@u9r52sld u9r52sld commented Aug 7, 2018

Someone please tell me why the codecov/project failed. What should we do?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 7, 2018

Someone please tell me why the codecov/project failed. What should we do?

You can ignore it in this case.

LGTM, I'll again test a few things locally and merge

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 15, 2018

Finally got the chance to test this (basically was not active with Sequelize for past week). Looks like attributes mixing is broken after this, consider this case

const Model = sequelize.define('model', {
  name: Sequelize.STRING,
  value: Sequelize.INTEGER,
  password: Sequelize.STRING
}, {
    defaultScope: {
      attributes: {
        exclude: ['password']
      }
    },
    scopes: {
      aScope: {
        attributes: {
          exclude: ['value']
        }
      }
    }
  });

console.log(
  Model._scope
);

console.log(
  Model.scope('aScope', 'defaultScope')._scope
);
{ attributes: { exclude: [ 'password' ] } } // correct
{ attributes: { exclude: [ 'value' ] } } // incorrect, exclude should include both `password`, `value`
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 21, 2018

I think we should conform scope just before merging them that will solve all concerns I have raised so far. Just conform scopes before merging with other scopes and keep merge/assign logic same as before (as much as possible), otherwise it can become an unintended breaking change.

@u9r52sld
Copy link
Contributor Author

@u9r52sld u9r52sld commented Aug 22, 2018

I think we should conform scope just before merging them that will solve all concerns I have raised so far

Could you please tell me the reason?

I think we should not conform scopes like exclude or include before queries are executed because it can be added new foreign_key fields by associations after that.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 22, 2018

May be we can merge attributes in that style, but not for include.

Conforming will convert both include: [{ model: ABC }] and include: [ABC] to same format so we don't have duplicate associations.

Conform standardise includes and other properties of scope so merge is predictable

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 31, 2018

@u9r52sld Thanks, now it looks almost ready.

Some concerns related to merge strategy changes, we have discussed it in #9087 for a few months. I have asked participants from that discussion to share their ideas here. I don't use scopes in my projects so not sure what merge strategy works well. I hope we can figure out something that works for everyone.

P.S. I'll be out for a few days, after that we can complete our discussion and merge this to v5

@papb
Copy link
Member

@papb papb commented Aug 31, 2018

Hello, I'm the one who opened #9087, I believe sushantdhiman wanted me to take a look here because this is also related to scopes. I will try to take a look as soon as possible (in less than 48 hours probably). In the meantime, if you could read my concerns on #9087 as well that would be great :) I'll be happy to discuss further and answer questions.

@papb
Copy link
Member

@papb papb commented Sep 1, 2018

Hello again, I took a closer look, this PR seems tangentially related to my issue #9087 because it is also related to scopes, but if I'm not mistaken that's about it. @u9r52sld, what do you think? If you could take a look as well that would be great.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 4, 2018

@u9r52sld @papb wrt to changes in this PR we should only target how scopes are merged among themselves. That is under scope of this PR.

For overall scope overhaul next we need to decide how scopes are merged with finder options, but that should be a different PR.

This PR is changing both scope's internal representation (like attributes are no longer converted from include/exclude to plain format etc) and strategy how to merge with other scopes. I want your suggestion (possibly others may be), if this current merge strategy (for merging scopes among themselves) is ok, or need any more changes (which btw looks good to me)

@papb
Copy link
Member

@papb papb commented Sep 4, 2018

Oh, the way that scopes are merged among themselves does concern my issue as well.

Currently, two scopes with different includes do not merge correctly. I show this in my issue #9087.

Once I have more time, I can prepare an specific test that fails. But I believe this should be straightforward from my issue.

If anyone has questions let me know. In the weekend I'll have time to write the failing test.

@papb
Copy link
Member

@papb papb commented Sep 10, 2018

Hello, I didn't have time to write the test as I was thinking, but please take a look at this other test that I wrote. It is about a merge of a scope with the findAll options, but the same idea for merging two scopes fails the exact same way.

Let me know if you have any other questions or want clarifications.

@u9r52sld
Copy link
Contributor Author

@u9r52sld u9r52sld commented Sep 14, 2018

@sushantdhiman @papb I'm sorry for the late reply. Thank you for your concern!
I'll check issue #9087 and fix it.

@papb
Copy link
Member

@papb papb commented Sep 14, 2018

@u9r52sld Thank you very much! Let me know if you need any further clarifications.

}, { override: true });

expect(Company._scope).to.deep.equal({
include: [{ model: Project }]
});
});

it('works with exclude and include attributes', () => {
it('should not work with exclude and include attributes', () => {

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 30, 2018
Contributor

should keep exclude and include attributes


// Reverse so we consider the latest include first.
// This is used if several scopes specify the same include - the last scope should take precedence
for (const scopeInclude of scope.include.reverse()) {

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 30, 2018
Contributor

What is the new merge strategy now (wrt https://github.com/sequelize/sequelize/blob/master/docs/scopes.md#merging), as far as I can tell it is still the same?

This comment has been minimized.

@u9r52sld

u9r52sld Oct 25, 2018
Author Contributor

Yeah, it's almost the same. I just updated for issues #9087,#6245

For example, current merge strategy is as follows.

Company.addScope('scopeA', {
  where: { name: 'A', something: true },
  attributes: {
    include: ['name']
  },
  include: [
    {
      model: Project,
      where: { name: 'A', something: true },
    },
    Project
  ],
  limit: 5,
  order: [
    ['name', 'DESC'],
  ]
})

Company.addScope('scopeB', {
  where: { name: 'B' },
  attributes: {
    exclude: ['something']
  },
  include: [
    {
      model: Project,
      as: 'Pj'
    },
    {
      model: Project,
      where: { name: 'B' }
    },
  ],
  limit: 10,
  order: [
    ['something', 'ASC'],
  ]
})

expect(Company.scope(['scopeA', 'scopeB'])._scope).to.deep.equal({
  where: { name: 'B', something: true }, // 'where' is shallowly merged
  attributes: { // just override
    exclude: ['something']
  },
  include: [ // 'include' is shallowly merged with the same model and alias(as)
    {
      model: Project,
      where: { name: 'B', something: true },
    },
    {
      model: Project,
      as: 'Pj'
    },
  ],
  limit: 10, // just override
  order: [ // Array is united
    ['name', 'DESC'],
    ['something', 'ASC']
  ]
})

This comment has been minimized.

@sushantdhiman

sushantdhiman Oct 25, 2018
Contributor

Hmm I thought we discussed to have attributes.include / exclude merge #9735 (comment)

This comment has been minimized.

@u9r52sld

u9r52sld Oct 25, 2018
Author Contributor

Oh, i'm sorry. I fixed it. e31be36

Company.addScope('scopeA', {
  attributes: {
    include: ['name']
  }
})

Company.addScope('scopeB', {
  attributes: {
    include: ['something'],
    exclude: ['password']
  }
})

expect(Company.scope(['scopeA', 'scopeB'])._scope).to.deep.equal({
  attributes: { // merged
    include: ['name', 'something'],
    exclude: ['password']
  }
})
@papb
Copy link
Member

@papb papb commented Oct 27, 2018

This is looking very good 👍

Thanks @u9r52sld! I tested your changes against my examples in #9087 and I'm very happy with the results. I also tested more complicated and nested combinations and they also worked as I'd like. Thank you very much! 😁

@sushantdhiman sushantdhiman merged commit 88a340d into sequelize:master Oct 28, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 96.3%)
Details
codecov/project 96.32% (+0.01%) compared to 122bd17
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Oct 28, 2018

Thanks @u9r52sld for delivering after this 3 months long review process :) & thanks @papb for your additional comments and review

@harpcio
Copy link

@harpcio harpcio commented Nov 24, 2018

Are there any chances for this bugfix to be in v4?

@papb
Copy link
Member

@papb papb commented Nov 24, 2018

@harpcio I'd guess that probably not, because although I agree that the previous behavior was wrong and that this can be seen as a "bug fix", in fact this is a breaking change, and breaking changes should not be added without a change in the major version number (following semver).

@papb
Copy link
Member

@papb papb commented Jan 17, 2019

Extra documentation for this PR was added in #10312

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

Successfully merging this pull request may close these issues.

None yet

4 participants