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): add custom order direction to subQuery ordering with minified alias #15046

Merged
merged 2 commits into from Sep 26, 2022

Conversation

evanrittenhouse
Copy link
Member

@evanrittenhouse evanrittenhouse commented Sep 24, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

Fix for new issue raised at #14723.

Essentially, we weren't pushing the order direction when pushing the alias for the sub-query field. This PR adds logic to do so, as well as tests for ascending/descending subquery ordering with aliases.

@evanrittenhouse evanrittenhouse requested review from ephys and a team and removed request for ephys September 24, 2022 23:25
@WikiRik
Copy link
Member

WikiRik commented Sep 24, 2022

Could you add the unit test that was in the original comment as well?

@fzn0x fzn0x changed the title Minify aliases fix fix: minify aliases Sep 25, 2022
@evanrittenhouse
Copy link
Member Author

evanrittenhouse commented Sep 26, 2022

@WikiRik Sorry, what unit test do you mean? I believe the original issue's case is covered in the orderByDescSubquery test found here, unless I'm misunderstanding the issue they're having.

WikiRik
WikiRik previously approved these changes Sep 26, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was just confused. This integration test indeed covers this just fine

@WikiRik WikiRik changed the title fix: minify aliases fix: add custom order direction to subQuery ordering when minifyAliases is true Sep 26, 2022
@WikiRik WikiRik changed the title fix: add custom order direction to subQuery ordering when minifyAliases is true fix(postgres): add custom order direction to subQuery ordering when minifyAliases is true Sep 26, 2022
@WikiRik WikiRik changed the title fix(postgres): add custom order direction to subQuery ordering when minifyAliases is true fix(postgres): add custom order direction to subQuery ordering with minified alias Sep 26, 2022
@evanrittenhouse
Copy link
Member Author

Sweet, once the checks pass I'll merge and notify Ashby on the issue thread.

@evanrittenhouse
Copy link
Member Author

@WikiRik can you please re-approve? Rebased on top of main so I can merge if (fingers crossed when) the checks pass.

@WikiRik
Copy link
Member

WikiRik commented Sep 26, 2022

For next time; you can just merge main in and push the new commit. If there are no conflicts you need to resolve, it won't discard my review.
And we squash all commits upon merging so that's not an issue.

Also; can you backport this to v6 as well?

@WikiRik WikiRik merged commit acffe83 into sequelize:main Sep 26, 2022
@ephys
Copy link
Member

ephys commented Sep 26, 2022

OK if I trigger a v7 release that includes this change?

@WikiRik
Copy link
Member

WikiRik commented Sep 26, 2022

Go ahead! Could be the final pre-DataTypes release

@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.18 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants