-
Notifications
You must be signed in to change notification settings - Fork 860
fix: check column references in ORDER BY (#1411) #1915
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
Conversation
There was a problem hiding this 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
.
internal/compiler/parse_test.go
Outdated
@@ -0,0 +1,104 @@ | |||
package compiler |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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. |
9f13d1f
to
a49bddd
Compare
sqlc getting more strict over time is a good thing. |
I added the |
Tell the uses how to switch off validation here.
Don't pass configuration around as a parameter
Hey @akutschera , There might be a minor bug in checking ambiguous references when joning tables while using alias, see #2398 |
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