-
Notifications
You must be signed in to change notification settings - Fork 962
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 |
* fix: check column references in ORDER BY (sqlc-dev#1411) * test: move test cases to endtoend tests * feat: add validate_order_by config option sqlc-dev#1411 * feat: expand error message sqlc-dev#1411 Tell the uses how to switch off validation here. * feat: add expanded error message to test sqlc-dev#1411 * compiler: Add functions to the compiler struct Don't pass configuration around as a parameter --------- Co-authored-by: Kyle Conroy <kyle@conroy.org>
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