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: check column references in ORDER BY (#1411) #1915

Merged
merged 7 commits into from
Jun 8, 2023

Conversation

akutschera
Copy link
Contributor

@akutschera akutschera commented Oct 27, 2022

MySQL and Sqlite store the information for the order by columns in a WindowClause while PostgreSQL stores it in a SortClause.
That's why I added two new if-statements.

Refs: #1411

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

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

Adding additional validation like this is great, but it will cause queries that used to work to fail. If the validation code is wrong, users won't be able to get their queries working.

Let's add a new configuration option here called validate_order_by that defaults to true (make it a pointer to a bool). Then we can update the error message telling users they can turn it off by setting validate_order_by: false.

@@ -0,0 +1,104 @@
package compiler
Copy link
Collaborator

Choose a reason for hiding this comment

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

I take a bit of a different approach to testing sqlc, opting mainly for end-to-end tests that don't verify internal behavior. Can you take these tests and move them into internal/endtoend/testdata/order_by_non_existing_column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That I can do. We actually only need these tests for order by. The group by tests are in internal/endtoend/testdata/invalid_group_by_reference already

@akutschera
Copy link
Contributor Author

akutschera commented Nov 10, 2022

Adding additional validation like this is great, but it will cause queries that used to work to fail. If the validation code is wrong, users won't be able to get their queries working.

Let's add a new configuration option here called validate_order_by that defaults to true (make it a pointer to a bool). Then we can update the error message telling users they can turn it off by setting validate_order_by: false.

If we default it to "true" and we break existing queries, we need to either give people the possibility to also configure this in v1-files or we force them to upgrade their config to v2. I think both are valid options.
We could also default it to false (similar to "strict_function_checks"). This way we won't break existing uses and everyone who wants to use this feature needs to upgrade to v2.

@kyleconroy
Copy link
Collaborator

sqlc getting more strict over time is a good thing. strict_function_checks works in V1 as well, so we can put validate_order_by in the same spot.

@akutschera
Copy link
Contributor Author

I added the validate_order_by config option. It works even though it feels a bit awkward to pass this value through multiple function calls. I am open to suggestions to improve this before it gets merged.
I will do the documentation in a separate PR because I also want to add documentation for strict_function_checks in V1 and I don't think this belongs to this PR.

@kyleconroy kyleconroy merged commit c4e4b68 into sqlc-dev:main Jun 8, 2023
@Stumble
Copy link

Stumble commented Jun 30, 2023

Hey @akutschera ,

There might be a minor bug in checking ambiguous references when joning tables while using alias, see #2398

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

Successfully merging this pull request may close these issues.

3 participants