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

feat(postgres): minify include aliases over limit #11940

Merged

Conversation

mrrinot
Copy link
Contributor

@mrrinot mrrinot commented Feb 19, 2020

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

Proposal to make the minifyAliases option also alias includes that go over the 63 POSTGRES character limit.

Closes #11937

@mrrinot
Copy link
Contributor Author

mrrinot commented Feb 19, 2020

Can't see the Codecov details, someone might be able to help me

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #11940 into master will decrease coverage by 10.17%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #11940       +/-   ##
===========================================
- Coverage   96.23%   86.05%   -10.18%     
===========================================
  Files          95       90        -5     
  Lines        9196     8816      -380     
===========================================
- Hits         8850     7587     -1263     
- Misses        346     1229      +883
Impacted Files Coverage Δ
lib/dialects/abstract/query-generator.js 95.35% <100%> (-1.77%) ⬇️
lib/dialects/postgres/query.js 97.83% <100%> (+0.06%) ⬆️
lib/dialects/sqlite/query-generator.js 2.91% <0%> (-93.69%) ⬇️
lib/dialects/mssql/query-generator.js 2.82% <0%> (-92.79%) ⬇️
lib/dialects/mssql/query-interface.js 8.69% <0%> (-91.31%) ⬇️
lib/dialects/mssql/query.js 5.58% <0%> (-89.95%) ⬇️
lib/dialects/sqlite/query-interface.js 15.62% <0%> (-84.38%) ⬇️
lib/dialects/mssql/resource-lock.js 22.22% <0%> (-77.78%) ⬇️
lib/dialects/mssql/data-types.js 30.12% <0%> (-69.88%) ⬇️
lib/dialects/sqlite/data-types.js 35.89% <0%> (-64.11%) ⬇️
... and 15 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 41aba1f...a3a4471. Read the comment docs.

@mrrinot mrrinot force-pushed the minify-include-aliases-over-limit branch from 2c14f98 to a3a4471 Compare February 20, 2020 11:42
@Nick-Riggs
Copy link

👍 Looking forward to having this merged. Since creating the issue, I've found a few more cases where our project is suffering from the length limit in the FROM clause. And it's becoming an issue having to put in hacks to get around the problem. Any chance this will be merged soon? What are the next steps?

@mrrinot
Copy link
Contributor Author

mrrinot commented Feb 25, 2020

I am waiting for a review and then hoping the tests pass

@sushantdhiman sushantdhiman merged commit f1ba280 into sequelize:master Mar 16, 2020
@Nick-Riggs
Copy link

I'm happy to see this merged. This will remove a lot of hackery from our codebase. Thanks for your work on it @mrrinot.

Question: will this merge be included in a v5 release? Or just a future v6 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minifyAliases does not fully resolve PostgreSQL alias identifier length constraints
3 participants