spring-projects / spring-data-mongodb

Provides support to increase developer productivity in Java when using MongoDB. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-mongodb/
Apache License 2.0
1.61k stars 1.08k forks source link

Kotlin type-safe queries and updates are not type-safe at all #4798

Open pmatysek opened 6 days ago

pmatysek commented 6 days ago

Hi, spring-data-mongodb supports in Kotlin typed queries and recently introduced with my help typed updates too (#3028). While this API is handy because there's no need to keep field names in constants, it has one hidden drawback or bug - it's not type-safe at any manner. Even though docs (that one I wrote too) and its construction suggest it undeniably should be.

Problem explanation

I will show it by an example:

data class Book(val title: String)
val typed = Book::title isEqualTo 1234 // it should be prohibited by the compiler and it's not for now

isEqualTo infix fun is defined in a way that as I said undeniably suggests that it should be type-safe:

infix fun <T> KProperty<T>.isEqualTo(value: T) =
        Criteria(this.toDotPath()).isEqualTo(value)

Why the type of value is not checked properly by a compiler? The culprit is declaration-site variance Kotlin mechanism. And especially the fact that KProperty interface has its generic type marked with out variance annotation:

public expect interface KProperty<out V>

Possible solutions

While the problem seems to be complicated and rather lies on the Kotlin language side, there is a simple (but tricky) solution. JetBrains team has internal annotation (in kotlin.internal package) @OnlyInputTypes designed for cases like this. The solution would be as simple as just using it on a generic type:

infix fun <@OnlyInputTypes T> KProperty<T>.isEqualTo(value: T) =
        Criteria(this.toDotPath()).isEqualTo(value)

data class Book(val title: String)
val typed = Book::title isEqualTo 1234 // now isEqualTo is type-safe so this code will not compile

However, this solution has one drawback - while this annotation is in kotlin.internal package it can't be used directly outside the Kotlin project. There are two workarounds to use it, but both are tricky. First:

Second:

Both of them are rather workarounds with some drawbacks. Another drawback of fixing this bug is the fact that it breaks the backward compatibility of this API - some code will stop compiling (but let's say very error-prone code that uses this API in the wrong way)

I have prepared code with these solutions and can post PRs later on. Moreover, I did some research and saw that ex. KMongo uses @OnlyInputTypes too: https://github.com/Litote/kmongo/commit/413539e7f7364774d292f8a6332bd4e9a9d5bd14

What are your thoughts about that? Do you think that introducing a fix with @OnlyInputTypes would be OK?

Resources

https://youtrack.jetbrains.com/issue/KT-13198 https://discuss.kotlinlang.org/t/restrict-type-by-receiver/5492 https://kotlinlang.org/docs/generics.html https://stackoverflow.com/questions/54779553/kotlin-generic-constraints-require-param-to-be-of-same-type-as-other <-- there is workaround with wrapping KProperty into own type, but I can't see a way to use it in a clean and seamless way

mp911de commented 2 days ago

Thanks for reaching out. Since we decided to support Kotlin, we're constantly patching our code for the odd quirks that Kotlin requires other libraries to do in order to leverage Kotin functionality. By now, we have sprawled Kotlin functionality all over our code base.

Roman Elizarov: [The OnlyInputTypes annotation] is under-designed at the moment.

Focusing on the second part of Roman's reply is true for a lot of Kotlin aspects. While Kotlin provides a good experience on the user side, it creates a lot of damage to integrators that want to provide functionality for Kotlin users.

You might argue that adding another tiny bit to our already existing sprawl doesn't make a big difference – where do you draw the line? KT-13198 is 8 years old and the problem doesn't seem to be addressed soon.

For the time being, I would like not to introduce additional changes for Kotlin support until Kotlin sorts out these issues for itself.