Skip to content
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

Add a findByIdOrNull(…) Kotlin extension to CrudRepository [DATACMNS-1346] #1782

Closed
spring-projects-issues opened this issue Jun 26, 2018 · 3 comments

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Sébastien Deleuze opened DATACMNS-1346 and commented

In Kotlin, it is idiomatic to deal with return value that could have or not a result with nullable types since they are natively supported by the language. This commit add a findByIdOrNull variant to CrudRepository#findById that returns T? instead of Optional<T>


Referenced from: pull request #299

Backported to: 2.1.4 (Lovelace SR4)

15 votes, 4 watchers

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Oliver Drotbohm commented

I think we had that discussion before, didn't we? You can already declare a Foo findFooById(…) in a repository and it will just return the entity or null in case of absence

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Sébastien Deleuze commented

I think we had some discussion about not imposing to Java developers some Kotlin idioms when Spring Data switched from returning a nullable Foo to Optional<Foo>, and indeed you can declare custom methods, but the lack of this out of the box continues to be puzzling IMO from a Kotlin developer POV. Custom methods make sense to provide customization specific to your use case, but here we are speaking about the most basic one, and the OrNull suffix seems to me a pretty good one to express that + the Kotlin extension make it invisible to Java developers.

Side note: the ideal solution would avoid Optional wrapper and directly exposed the nullable value but I am not sure how this could be possible without exposing that change to Java developers so as a trade-off I submitted this PR. If you have a better idea I would be interested

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 5, 2018

Sébastien Deleuze commented

Could we please move forward on that issue?

It is now the 2nd most voted Spring Data Commons issue, and I have met a lot of Kotlin developers recently that are very frustrated by this issue. An extension will have no impact on Java developers, and will make Spring Data much more friendly to Kotlin by providing out of the box an idiomatic API for this very basic functionality.

Also to quote you Oliver Drotbohm (from this tweet):

In Spring Data, we consistently observed Streams of any kind (and Optional) to add significant overhead over foreach loops so that we strictly avoid them for hot code paths.

From a user perspective, findById can be in the application hot path, and Kotlin is designed to promote this kind of 0 cost runtime abstraction, so the current API is a kind of bad practice from a Kotlin developer POV because you are promoting an API that make you paying a cost in term of performance and code readability for no benefit. I would also add that this is a regression since the nullable return value version was provided before. Sure it is possible to define manually a nullable one, but how would you feel in Java if you had to define the Optional variant for every repository?

If you agree, I would like to improve my pull request by avoiding using Optional in the extension to return the nullable value in order to provide more than just syntactic sugar. Could you please provide me some guidance about how that do that (if Spring Data internals are not optional based that seems doable to me)? Also do you have a better name than findByIdOrNull to suggest or are you ok with that one?

 

 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants