Skip to content

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

Merged
sushantdhiman merged 4 commits intosequelize:masterfrom
mrrinot:minify-include-aliases-over-limit
Mar 16, 2020
Merged

feat(postgres): minify include aliases over limit#11940
sushantdhiman merged 4 commits intosequelize:masterfrom
mrrinot:minify-include-aliases-over-limit

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