-
Notifications
You must be signed in to change notification settings - Fork 751
fix: EXPOSED-815 Check types for QueryAlias.get() at runtime #2532
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
| ?: error("Column not found in original table") | ||
|
|
||
| operator fun <T : Any?> get(original: Expression<T>): Expression<T> { | ||
| if (original is Column<T>) return get(original) |
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.
As far as I know Column extends ExpressionWithColumnType, so these 2 checks could be joined
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.
Ah, you are right. I've updated it to also handle Columns that are casted to Expression or ExpressionWithColumnType
| operator fun <T : Any?> get(original: Expression<T>): Expression<T> { | ||
| if (original is Column<T>) return get(original) | ||
| if (original is ExpressionWithColumnType<T>) return get(original) | ||
| val aliases = query.set.fields.filterIsInstance<ExpressionAlias<T>>() |
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.
It would be nice also to filter it for IExpressionAlias
|
It looks like there is a problem with PR. The test |
|
Fixed by inverting the if. This did make me realise you can't get a literal on an alias, and I'm not sure how it should be fixed (probably not in this PR anyway). select my_alias.??? from (select 10 from some_table) my_aliasThe union also generates SELECT unionAlias.id, exp1, exp2 FROM (SELECT users.id, 10 exp1, 'aaa' exp2 FROM users WHERE users.id = 'andrey' UNION ALL SELECT users.id, 100, 'bbb' FROM users WHERE users.id = 'andrey') unionAliaswhile it should probably reference |
| fun Expression<*>.toExpressionString() = QueryBuilder(true).also { this.toQueryBuilder(it) }.toString() | ||
|
|
||
| withTables(tester) { | ||
| assertEquals("all_weight.weight", query[column].toExpressionString()) |
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.
It would be nice to replace it with (added lowercase()):
assertEquals("all_weight.weight", query[column].toExpressionString().lowercase())
assertEquals("all_weight.weight", query[column as Expression<Int>].toExpressionString().lowercase())
assertEquals("all_weight.weight", query[column as ExpressionWithColumnType<Int>].toExpressionString().lowercase())otherwise it fails on some dialects like H2
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.
Oops, fixed, my apologies
|
In general PR looks good, the added test fails for some databases but it fixable. I'd merge the PR if you fix the test to make it working for all the dialects. |
|
Thank you for contributing, it would be delivered in the next version (should be |
Description
Check types for QueryAlias.get() at runtime.
Relying purely on compile time checks causes issues when trying to dynamically select fields. E.g.:
Type of Change
Please mark the relevant options with an "X":
Updates/remove existing public API methods:
Affected databases:
Checklist
Related Issues