feat(postgres): minify aliases option

[mrrinot]

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

Add alias minifier to fix the following issue: #2084

All aliases are replaced with an auto increment alias (_1, _2, _3 ...) before querying and immediately replaced after the query has been run.

This does pose a colliding problem if any column is named accordingly to the alias and I could see the case for making it customizable or just more complex.

I didn't found the right place in the doc to warn users about that but I would be happy to write it down somewhere appropriate.

(Also thanks to @mattp94 for helping me do this)

[codecov]

Codecov Report

Merging #11095 into master will increase coverage by 0.03%.
The diff coverage is 94%.

@@            Coverage Diff             @@
##           master   #11095      +/-   ##
==========================================
+ Coverage   96.25%   96.29%   +0.03%     
==========================================
  Files          94       94              
  Lines        9106     9143      +37     
==========================================
+ Hits         8765     8804      +39     
+ Misses        341      339       -2

Impacted Files	Coverage Δ
lib/sequelize.js	95.95% <ø> (ø)	⬆️
lib/dialects/postgres/query.js	97.77% <100%> (+0.07%)	⬆️
lib/dialects/abstract/query-generator.js	97.41% <92.68%> (-0.2%)	⬇️
lib/dialects/postgres/connection-manager.js	95.77% <0%> (+3.52%)	⬆️

---

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 d5667e0...cfc9225. Read the comment docs.

[shawnbissell]

@mrrinot I've found an issue related to running raw queries ... I get the following error:
TypeError: Cannot read property 'get' of undefined
at Object.entries.reduce (/.../web/node_modules/sequelize/lib/dialects/postgres/query.js:80:49)
Here is my query:
models.sequelize.query(select exists(select * from pg_proc where proname = 'ssl_is_used');,
{ type: models.sequelize.QueryTypes.SELECT });

[eseliger]

Can you add a test case which would not have worked before?

[mrrinot]

@eseliger I'm currently looking through the tests to find where to add my tests.
Do you have a specific place in mind ?

I put them in a new file query.js in the meantime.

[mrrinot]

@eseliger Tests have been added

[mtryba]

@mrrinot I checked it and generally looks very good, but when I use belongsToMany so query didn't use aliases. Can you confirm it?

[mrrinot]

@mtryba I'm sorry I didn't understand your message. Do you mean it doesn't work with belongsToMany ?

[mtryba]

@mrrinot It means that when I have relations one to one everything is ok, aliases work correct but I have one relation many to many and aliases in the relation are without changes. It looks like

"deliveryMaterialInspections->inspectionRejectReasons"."updatedAt" AS "_34",
"deliveryMaterialInspections->inspectionRejectReasons->deliveryMaterialInspectionRejectReason"."id" AS "deliveryMaterialInspections.inspectionRejectReasons.deliveryMaterialInspectionRejectReason.id",

the second line is the without alias

[mrrinot]

Ho I see, I'll look into that and add a test case.

[mrrinot]

At first I thought that table name aliasing wasn't affected by the Postgres limit of 64 but it seems otherwise. I need to fix that too and it complexify the problem quite a bit.

Although I don't know if this fix is worth it because you would need a single alias to be > 64 character to break. The problem usually arise when chaining includes and each table's name is shorter than 64 characters.

I specifically encountered the problem in my test case because I used these aliases:

    const alias = 'AnActualVeryLongAliasThatShouldBreakthePostgresLimitOfSixtyFourCharacters';
    const longerAlias = 'AnActualVeryLongAliasThatShouldBreakthePostgresLimitOfSixtyFourCharactersButThisTimeForBelongsToMany';

I'll push the fix for the belongsToMany columns not being aliased. As for the table alias problem, I think it could be done in a separate PR, I don't really know.

[mtryba]

@mrrinot I checked it and I think everything is ok ❤️ Probably you safe my life 😄

[mtryba]

@eseliger Can we merge it?

[shawnbissell]

I've been testing this with a large codebase (hundreds of different queries and many deeply nested) without problems. How do we get this reviewed and merged to master? @sushantdhiman @mickhansen @papb

[eseliger]

looking good, can you add the option minifyAliases to the type definitions? 🙂

[mrrinot]

Ok, so now all integration tests are good, there is a lot of unit tests failing.
Is there a way to run only the integration tests ? Do I create new test that check the aliasing system and run them separately ?

[sushantdhiman]

@mrrinot Just integration tests should be enough, once build is green I'll review this once again.

[mrrinot]

@sushantdhiman I suppose I need to modify the travis.yml file but I don't know what to change.

[mrrinot]

Tests appear OK @sushantdhiman, I don't understand the codecov report failing though.

[Telokis]

Any idea how much work is left before we get this feature?

[sushantdhiman]

Coverage is down a bit but can ignore that

[sushantdhiman]

🎉 This PR is included in version 5.18.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[mjy78]

Can the minifyAliases option be passed to individual queries? eg. Model.findAll({ where {...}, include: {...}, minifyAliases: true });

[mjy78]

Can the minifyAliases option be passed to individual queries? eg. Model.findAll({ where {...}, include: {...}, minifyAliases: true });

Tested this and confirmed you cannot pass minifyAliases: false in for individual queries.

[mrrinot]

No you cannot, I thought it would be more of a global options than a individual one.

[dhruvingbc]

If I pass minifyAliases : false , it will minify the output and does the vice-versa.

[henrywoody]

I'm running into this issue but for the table names, rather than table + column names. Meaning that the names of the joined tables are already too long for Postgres, so I get an error saying the table name is specified more than once.

Can we add handling for table names in addition to column names?

[papb]

Hi @henrywoody, please open a new issue about this so we can track progress properly, thanks

[Nick-Riggs]

Bug opened for minifyAliases not applying to the FROM clause aliases: #11937