fix: EXPOSED-266 Between() accepts arguments of different type than column type #1983
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…olumn type between() currently accepts arguments of types that do not match the type stored in the column on which it is called. In the event that this is being taken advantage of by users, rather than introducing an immediate breaking change, the upper bounds have been restricted to show a compiler warning. A function override has also been introduced to ensure that this warning does not show when invoked on an EntityID column with literal arguments.
…olumn type Replace deprecated datetime functions in tests.
bog-walk
commented
Feb 1, 2024
Comment on lines
-399
to
+402
| fun <T, S : T?> ExpressionWithColumnType<S>.between(from: T, to: T): Between = Between(this, wrap(from), wrap(to)) | ||
| fun <T, S : T?> ExpressionWithColumnType<in S>.between(from: T, to: T): Between = Between(this, wrap(from), wrap(to)) | ||
|
|
||
| /** Returns `true` if this [EntityID] expression is between the values [from] and [to], `false` otherwise. */ | ||
| fun <T : Comparable<T>, E : EntityID<T>?> ExpressionWithColumnType<E>.between(from: T, to: T): Between = Between(this, wrap(from), wrap(to)) |
There was a problem hiding this comment.
As mentioned above, I'm not sure how to test for the presence of a compiler warning as all existing tests use arguments with the correct type. And any test purposefully set up to show a warning would fail when we upgrade the Kotlin version.
Please let me know if there's any type of test that should be added.
e5l
approved these changes
Feb 5, 2024
Comment on lines
-399
to
+402
| fun <T, S : T?> ExpressionWithColumnType<S>.between(from: T, to: T): Between = Between(this, wrap(from), wrap(to)) | ||
| fun <T, S : T?> ExpressionWithColumnType<in S>.between(from: T, to: T): Between = Between(this, wrap(from), wrap(to)) | ||
|
|
||
| /** Returns `true` if this [EntityID] expression is between the values [from] and [to], `false` otherwise. */ | ||
| fun <T : Comparable<T>, E : EntityID<T>?> ExpressionWithColumnType<E>.between(from: T, to: T): Between = Between(this, wrap(from), wrap(to)) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
between()currently accepts arguments of types that do not match the type stored in the column on which it is called.This means that the following is technically possible to compile:
In the event that this is being taken advantage of by users (for example with numeric or UUID types), rather than introducing an immediate breaking change, the upper bounds have been restricted to highlight the offending function and show a compiler warning.
A function overload has also been introduced to ensure that this warning does not show when invoked on an
EntityIDcolumn with appropriate literal arguments.Note: No tests have been added and all tests for
between()in the datetime modules provide matching argument types with no warnings.Additional: