Skip to content

kotlin coroutines: Improve documentation on when suspend is required on repository methods #2503

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

Closed
RobertHeim opened this issue Nov 29, 2021 · 5 comments
Assignees
Labels
in: kotlin Kotlin support type: documentation A documentation update

Comments

@RobertHeim
Copy link

RobertHeim commented Nov 29, 2021

As we see in CoroutineCrudRepository most operations are marked as suspend. The findX methods however return Flow<X> and are not marked as suspend.

The documentation should make more clear why and when the suspend is required / can/should be omitted.

The doc currently states the general translation to coroutines API:

For return values, the translation from Reactive to Coroutines APIs is the following:

fun handler(): Mono<Void> becomes suspend fun handler()

fun handler(): Mono<T> becomes suspend fun handler(): T or suspend fun handler(): T? depending on if the Mono can be empty or not (with the advantage of being more statically typed)

fun handler(): Flux<T> becomes fun handler(): Flow<T>

https://docs.spring.io/spring-data/r2dbc/docs/current/reference/html/#kotlin.coroutines.reactive

but from a developers perspective it would be very helpful to not only have the translation but provide it from the perspective of coroutines API (i.e. "use suspend for all methods unless it returns a Flow" or am I wrong?)

E.g. a non-suspending deleteX is not executed. In such case it also would be great to have a compile time error (but that might be not possible in the scope of the project - which is why I think it should be better documented because that is hard to catch).

E.g. in reference documentation 17.5.3 Repositories it remains unclear if findX methods should be suspending or not and what the implications are.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 29, 2021
@mp911de
Copy link
Member

mp911de commented Nov 29, 2021

The main difference is that methods returning Flow might not require suspend. A Flow is activated upon calling collect which is a suspended method on its own. The example returning List must be suspended because a List doesn't participate in coroutines and hence it cannot be materialized later.

Methods returning Flow can be suspended in case the method execution itself does something expensive and you want to defer that or make the method execution subject to coroutine participation instead of having the method run all code to assemble Flow. Hence a suspended method returning Flow is being deferred twice. Since all of this is how Coroutines work, there's no need to document general coroutine behavior on the level of Spring Data.

E.g. a non-suspending deleteX is not executed. In such case it also would be great to have a compile time error (but that might be not possible in the scope of the project - which is why I think it should be better documented because that is hard to catch).

You have a valid point. We should investigate whether we can validate Coroutine repositories early so that all method either return Flow or are suspended. Compile errors likely won't work because Spring Data doesn't interact with the compiler.

@broo2s
Copy link

broo2s commented Nov 29, 2021

As I explained in my answer in the StackOverflow question linked by you, your main concerns aren't really related to Spring Data, but rather to what does it mean that the function is suspend or that it returns a Flow. I'm not sure if the documentation of Spring should elaborate on this as this is really the knowledge related to Kotlin coroutines and not Spring specifically. Maybe it could at least link to the Kotlin documentation, but it may not be that simple, because understanding of coroutines really requires to read its docs fully.

But still, it seems weird to me that non-suspending delete function is not executed for you and there are no errors or warnings. If this is not a mistake then I guess this should be considered a bug in Spring.

@RobertHeim
Copy link
Author

RobertHeim commented Nov 30, 2021

I do understand coroutines in Kotlin and that involves that calling a non-suspending (e.g. a repository interface deleteByY(y: Y)) should execute that method when called from a suspending function (though it is discouraged because it blocks the dispatcher thread). I am confused from the behavior that the delete is not performed when I missed the suspend (in the end I am happy it did not work, because it should suspend, but the error was hard to catch and is easily repeated).

I think the spring docs should not explain coroutines but how the generated repository implementations make use of them and what the "catch-ya"s are. It would be of great benefit to make this more explicit in the context of Spring Data docs by providing more hints and maybe more details on some examples so developers run less often in this. Best would be if the compiler would hint the developer, but @mp911de Yes, I think compile errors are very unlikely to be implemented. I don't know if we can detect it at runtime either but if so a hint at runtime would also be helpful.

@RobertHeim
Copy link
Author

I created a minimal example for the delete case (see readme in the project):

https://github.com/RobertHeim/spring-data-suspend-delete

@mp911de mp911de added in: kotlin Kotlin support type: documentation A documentation update and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 20, 2022
@mp911de mp911de added this to the 2.5.9 (2021.0.9) milestone Jan 25, 2022
@pollux-
Copy link

pollux- commented Jun 1, 2023

@RobertHeim where can I see the implementation details for

deleteBy or any other method, its all seems to be magically bound to together, I'm trying to wrap my head around it. I had used Room database before, I can see all the generated classes the Room creates while defining the repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: kotlin Kotlin support type: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

5 participants