jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
189 stars 55 forks source link

Provide API returning Optional as an alternative to methods allowed to return null #479

Open lukasj opened 10 months ago

lukasj commented 10 months ago

issue #298 added getSingleResultOrNull() method to Query (and relevant subtypes) to avoid the requirement of explicit exception handling in case no result is found through the call to getSingleResult().

This is a follow up issue explicitly asking to support Optional where appropriate as opposed to supporting it in one particular method for one particular use-case as was asked for in #298.

gavinking commented 10 months ago

So here's my reaction to this, FWIW, and I'm well aware that some other people are likely to strongly disagree with what I'm about to say.

When Optional was first proposed, many of us strongly criticized it as a completely broken and untypesafe ad hoc attempt to implement a Haskell-style Maybe in Java. [A correct implementation of Maybe in Java is impossible because Java has no sum types.]

At the time, those proposing Optional defended it on the basis that no, this is just a little convenience thing for use with the new streams API, and that it wasn't supposed to be a whole new approach to dealing with null in Java. I was at the time highly skeptical of this claim, but to my pleasant surprise, they've largely kept this promise, and with only a handful of unfortunate exceptions, Optional has not really leaked into the Java APIs I use every day. Most API developers have (wisely, in my view) chosen to avoid it, and I just don't have to deal with it in my code.

[Ironically, the Java steams API itself has turned out to be far less useful than most—myself definitely included—predicted, and so I don't even really deal with Optional very often even in that context.]

So, now, why am I saying all this?

Well, we could certainly provide overloaded operations which return Optional in JPA, but:

  1. when Optional was introduced, we were promised that it was not going to be used in this way,
  2. it makes adds incremental complexity to our APIs without (a) adding new functionality, (b) improving typesafety, nor even, in my opinion (c) enhancing ergonomics, and
  3. it gives Optional an extra beach head from where it can quite easily metastasize into exactly the sort of broken "general purpose" approach to null handling that it was never supposed to be, and for which it is misconceived and mis-designed.

What I'm saying is that JPA is one of the most prominent Java APIs, and in my opinion we should set an example here. Our embracing this abomination would send a signal to other Jakarta projects and other libraries in the Java ecosystem at large that this is the way to go, and in a few years time my code will be filled Optional.

So that's the case against doing this.

gavinking commented 10 months ago

[Another day I'll write up why I think that there is indeed an issue with how EntityManager.find() treats the unfound case, and why we maybe should indeed do something about that, and why that "something" should not be findOptional().]

gavinking commented 10 months ago

Anticipating arguments against the view I've stated above, let me state what I think is the best case in favor of the use of Optional in APIs like JPA:

  1. For casual users of the API, it acts as excellent documentation of the fact that an operation can return no result.
  2. It lets you can combine getSingleResult()/getSingleResultOrNull() into one method declaration.
  3. Even if it isn't completely typesafe, it still forces callers to explicitly do at least something about the null case, so users are forced to at least consider it.

Unfortunately, none of those motivations are very convincing in our case where we already have an operation which doesn't return Optional, and we're considering adding an overload.

  1. the new overload doesn't do much at all to clarify the null semantics of the original method,
  2. it doesn't reduce the number of methods, you need getOptionalSingleResult() or whatever, and
  3. the sort of lazy user who is inclined to not think about null is almost certain to just call getSingleResult() instead of getSingleResultOrNull(), especially given the lack of ergonomics of Java's Optional. This comes back to my principle that "features for type safety are most useful when they're opt-out, not when they're opt-in".

That's ... pretty devastating to the case for Optional, IMO, at least in our context.

The direction we went with getSingleResult() + getSingleResultOrNull() looks pretty good by comparison:

gavinking commented 10 months ago

Now, OTOH, the case I'm concerned about is find(). I fear that we messed up the semantics of this method years ago, and that it should throw when the entity is not found, just like getSingleResult().

So I would love there to be two versions of find(), just like there's two versions of getSingleResult(). Except ... the naming just doesn't work out very well. [FTR, even findOptional() doesn't work as a method name.]

An idea I've been playing with for quite a long time is to have a non-null version of find() called load(). But I still have not managed to convince even myself that this is the right solution.

dmatej commented 10 months ago

Outside Persistence I use this convention:

Then getSingleResultOrNull could be named just findSingleResult, which would exactly describe what it does for "select" queries, but there are also SQL procedures, where the result might be just a side effect - and that is probably why we have getResultList and getSingleResult. Then getSingleResultOrNull is fine and useful, but I don't see any advantage in using Optional.

There are few cases when Optional helped readability, but too often it simply doesn't bring a value as caller still has to resolve all variants (and sometimes even more).

Btw that throwing exception from getSingleResult() is quite unpleasant experience for years - usually the caller has some business logic around, using own "exception set", and this just complicated the processing.

gavinking commented 10 months ago

find() vs get() is a sensible convention indeed.

victorwss commented 9 months ago

I understand that there is an opinion that Optional is an abomination. There are people on the other hand, that consider that the null itself is an abomination (aka the 1 billion dollar mistake) and that whenever possible, methods should never return nor accept nulls.

Anyway, Optional is here to stay, some APIs work with it and some people use them. Manually wrapping nulls into Optionals, besides being easy and trivial is cumbersome and error-prone, just as boxing/unboxing was prior to Java 5.

Even being somewhat deceptive in its capabilities, using Optional and never returning null forces the developer to test if a result is present or not, which avoids NullPointerExceptions down the stream when someone forget, neglect or misunderstand that a result might not be there (a very common mistake). However, when using an Optional, failing to handle that case results in a compile error about mismatching types, something that is much harder to forget about or to neglect. In the end of the day, this results in a more trustworthy and less buggy code for API's consumers. And that is the main advantage of the Optional: Forcing the consumer to test if the result is there or not (without relying on checked exceptions that are plagued with other problems).

So, this would be very convenient in the TypedQuery interface:

    public default Optional<T> getOptionalResult() {
        return Optional.ofNullable(getSingleResultOrNull());
    }

Anyway, nobody would be forced or obliged to use this, it is just a convenience. People who dislike Optional could just use getSingleResultOrNull() instead (or whatever is its name).

gavinking commented 9 months ago

I understand that there is an opinion that Optional is an abomination. There are people on the other hand, that consider that the null itself is an abomination

Well that's a false dichotomy. Both of these things are terrible—untypesafe null values being the original "abomination", and Optional being the ugly band-aid non-solution that does little to improve the situation.

Some of us have been advocating for typesafe null for, oooh, something like twenty years now. (Some of us even designed whole programming languages 🐘 to show that such a construct works really well in practice.) That doesn't mean we're obligated to agree that Optional was a good idea, or that it makes sense in the context of a given API.

beikov commented 9 months ago

Just fyi, I think that https://openjdk.org/jeps/8316779 will help deal with nulls in all APIs in the future. The discussion to add type restrictions to Java was the last piece missing for us to start null annotating Hibernate ORM. So when this becomes available, we can easily make the switch to the language provided nullability feature. This would IMO be the way forward which also forces people to do something about nullable values.