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

fix(postgres): schema for index and relations #11274

Merged
merged 3 commits into from Aug 31, 2019

Conversation

vanthome
Copy link
Contributor

@vanthome vanthome commented Aug 2, 2019

Relations and Indexes created from models that have set a schema are not properly prefixed with the schema. This patch solves this.

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 update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

In several cases, the schema prefix was not applied for generated indexes and related tables. This PR fixes this.

Fixes #11276.

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #11274 into master will decrease coverage by 2.1%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11274      +/-   ##
==========================================
- Coverage   96.31%    94.2%   -2.11%     
==========================================
  Files          94       94              
  Lines        9106     9115       +9     
==========================================
- Hits         8770     8587     -183     
- Misses        336      528     +192
Impacted Files Coverage Δ
lib/model.js 96.46% <ø> (ø) ⬆️
lib/dialects/abstract/query-generator.js 97.08% <50%> (-0.53%) ⬇️
lib/dialects/postgres/query-generator.js 94.37% <87.5%> (-0.16%) ⬇️
lib/dialects/mysql/query.js 15.12% <0%> (-83.2%) ⬇️
lib/dialects/mysql/connection-manager.js 29.09% <0%> (-67.28%) ⬇️
lib/dialects/mysql/data-types.js 43.75% <0%> (-54.69%) ⬇️
lib/sequelize.js 92.83% <0%> (-3.12%) ⬇️
lib/dialects/mysql/query-generator.js 96.47% <0%> (-1.33%) ⬇️
lib/dialects/abstract/query.js 91.44% <0%> (-0.33%) ⬇️
... and 2 more

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 218f8cd...3b8593d. Read the comment docs.

@vanthome
Copy link
Contributor Author

vanthome commented Aug 2, 2019

From my perspective this is complete now, please merge it.

@papb papb added dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). status: awaiting maintainer type: bug labels Aug 2, 2019
@papb
Copy link
Member

papb commented Aug 2, 2019

@vanthome I see you did not add any test... Please add at least one simple test that wouldn't pass before your PR and passes after your PR. Thanks.

@papb papb added status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action and removed status: awaiting maintainer labels Aug 2, 2019
@vanthome
Copy link
Contributor Author

vanthome commented Aug 5, 2019

Added a test. For whatever reason now the tests fail although they run locally!?

@vanthome
Copy link
Contributor Author

vanthome commented Aug 5, 2019

Seems like MySQL tests fail due to some code that I integrated through rebasing. So will you merge this?

@papb
Copy link
Member

papb commented Aug 5, 2019

@vanthome The test you added is running for all dialects, and failing for every dialect which is not postgres. You have to add your test in a way that it will run only for postgres. Let me know if you need further help.

@papb
Copy link
Member

papb commented Aug 5, 2019

@vanthome Just add your test here instead and it should work.

Relations and Indexes created from models that have set a schema are not properly prefixed with the schema. This patch solves this.
@vanthome
Copy link
Contributor Author

vanthome commented Aug 6, 2019

Strange because previously the tests passed I think. Anyways, I have moved it there.

@vanthome
Copy link
Contributor Author

vanthome commented Aug 6, 2019

Now the PG native test seems to fail which has nothing to do with my change. What to do now?

@papb
Copy link
Member

papb commented Aug 6, 2019

@vanthome Nothing else to do! That error in postgres-native is an intermittent error that has been happening in Sequelize for a while... We can all ignore it.

Thank you very much for the PR! Looks ready to merge.

Let's wait for @sushantdhiman

@vanthome
Copy link
Contributor Author

vanthome commented Aug 6, 2019

Great, looking forward.

@papb
Copy link
Member

papb commented Aug 21, 2019

@vanthome There are conflicts with master, can you please resolve them?

@vanthome
Copy link
Contributor Author

Oh crap, I just pushed another update to my local branch, I did not want to add this to this PR. Do you know how I can revert this?

@papb
Copy link
Member

papb commented Aug 21, 2019

@vanthome Ah, that explains the last commit 😬

You can just revert the commit normally (git revert), don't worry about the "ugly git history" because everything will be squashed on merge anyway.

@vanthome
Copy link
Contributor Author

I have undone the malicious change. When will this be merged now?

@papb
Copy link
Member

papb commented Aug 25, 2019

Hi! There are still conflicts...

@vanthome
Copy link
Contributor Author

Well this was w/o conflicts. why has it not been merged then?
I mean why should I put more effort into this, is it does not get merged anyway?

@papb
Copy link
Member

papb commented Aug 25, 2019

Well this was w/o conflicts.

Indeed! But as time passes, conflicts may appear, it's normal.

why has it not been merged then?

Because maintainers are busy

I mean why should I put more effort into this, is it does not get merged anyway?

You don't have to do anything. In open source no one is forced to anything... I, like you, would like to see every PR merged as well.

@papb
Copy link
Member

papb commented Aug 25, 2019

I fixed the conflict for you 👍

@papb papb added status: awaiting maintainer and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Aug 25, 2019
@papb papb assigned papb and sushantdhiman and unassigned papb Aug 25, 2019
@vanthome
Copy link
Contributor Author

Great, looking forward for a merge.

@sushantdhiman sushantdhiman merged commit 4816005 into sequelize:master Aug 31, 2019
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 5.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). released type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres: Schema does not default to current schema for references when model is set only on parent model
3 participants