jakartaee
Add Query.getOptionalResult() or Query.getSingleResultOrNull()
Query.getSingleResult() is a widely used method. It's purpose is to "execute a SELECT query that returns a single untyped result". In case there is no result, it throws an unchecked NoResultException.
The problem with this method is that there is no obvious alternative method that does not throw an exception. So there is no immediate alternative that returns null when no result is found. (Returning java.util.Optional would be an even better option.) So clients are basically forced to use exception handling for flow control. Whereas EntityManager does support null for single results, such as in EntityManager.find(class, primaryId). Which is a bit strange in design, because one would expect that a method that finds something by a primary key would have a higher chance of not returning null, than a custom query. So the throwability of getSingleResult() will more often lead to runtime exceptions than if that throwability would exist in find(), when these methods would be used equally.
This is also inconsistent with Hibernate's Query.uniqueResult(), which does return null.
Also, forcing clients to use exception handling, is an anti-pattern. The well known Java engineer Joshua Bloch wrote in Effective Java (a de facto standard best-practices guide): "Use exceptions only for exceptional conditions; they should never be used for ordinary control flow". The absence of a value is not an exceptional condition. It also says: "A well-designed API must not force its clients to use exceptions for ordinary control flow". But getSingleResult() does force clients to use exception handling for control flow in common situations.
Authors of Hibernate wrote in the bible of JPA/Hibernate, Java Persistence with Hibernate, 2nd edition: "Now, the ugly bits: if there are no results, getSingleResult() throws a NoResultException. You’d expect a null for this type of perfectly benign query. This is rather tragic, because it forces you to guard this code with a try/catch block. In fact, it forces you to always wrap a call of getSingleResult(), because you can’t know whether the row(s) will be present." So even some authors of Hibernate (from which JPA was born and inspired), don't like it.
A lot of people look for solutions to this getSingleResult() problem: https://stackoverflow.com/questions/2002993/jpa-getsingleresult-or-null
If a method like Query.getSingleResultOrNull() or Query.getOptionalResult() would be available, than this problem would be gone because then clients are not forced anymore. An alternative name using find like EntityManger.find() instead of get, would be Query.findSingleResult(), but having "null" or "optional" would be more explicit/clearer. Without this solution, clients are forced to write helper methods to the reduce the boilerplate code caused by having to catch NoResultException every time, such as:
class JpaResultHelper {
public static Object getSingleResultOrNull(Query query) {
try {
return query.getSingleResult();
} catch (NoResultException ex) {
return null;
}
}
public static <T> Optional<T> getOptionalResult(TypedQuery<T> query) {
try {
return Optional.of(query.getSingleResult());
} catch (NoResultException ex) {
return Optional.empty();
}
}
}
Update: clients aren't fully "forced", because they can also do this: query.setMaxResults(1).getResultList().stream().findFirst().orElse(null); or since JPA 2.2 (2017): query.getResultStream().findFirst(). But the problem is that because of the existance of getSingleResult(), those other sub-methods aren't obvious to users. Also users are taught in the Hibernate book that getSingleResult() is the solution for zero or one results, and that exceptions should be used. If a method like getOptionalResult() would exist, things would be obvious immediately for Query clients, and would reduce the chance of unexpected NoResultExceptions from getSingleResult().
Yes, I like it too. But I think a better name for this method is getSingleResultOptional(Query query). Thus, as suggested it returns a Optional value.
Em ter., 1 de dez. de 2020 às 09:41, Valentin Brandl < [email protected]> escreveu:
I like this idea, but I really think the return type Should be Optional<T>
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jpa-api/issues/298#issuecomment-736527709, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACN424EFIBXDPYVWFRHLIODSSTQBJANCNFSM4UG3DDHA .
So look, since my name appears on the cover of that book, I guess I should clarify that I didn't write that quoted sentence, and I'm not at all sure I agree with it.
The problem is that in Java the null value isn't typesafe, and so APIs which return nulls can be the cause of runtime errors which happen far from the source of the bug, and are reported via the extremely generic and unhelpful NullPointerException. So it's often better if APIs don't return null.
Considered from the point of view of someone reading your code, a getSingleResult() which always returned null when there were zero results would make it hard for them to tell if:
- no result is normal and expected functioning, or
- if you just forgot to check explicitly for null and handle this case as erroneous.
EntityManager.find() suffers from this problem, and this is a source of bugs.
Whatever, the correct way to use this API is definitely not to call getSingleResult() and catch the exception! Indeed, I'm pretty sure that the spec explicitly states that you're not even allowed to do it that way. Exceptions in JPA are usually considered unrecoverable.
You should instead use getResultList() or getResultStream(). For example:
Book b = em.createQuery(queryString,
Book.class).getResultStream().findFirst().orElse(null);
Now, that's a little verbose, perhaps, since the annoying Optional is getting in the way, but it does the job. And it's explicit about the fact that you're anticipating that the book might not exist, and you're asking for null in the case, which makes it safer than if getSingleResult() just transparently returned null.
On the other hand, I would not mind seeing a getSingleResultOrNull() method to optimize the use case you're describing. As long as it's explicit about it's intention that no result row is explicitly to be represented by the null value.
On Mon, Nov 30, 2020 at 12:39 AM jtdev [email protected] wrote:
Query.getSingleResult() is a widely used method. It's purpose is to "execute a SELECT query that returns a single untyped result". In case there is no result, it throws an unchecked NoResultException.
The problem with this method is that there is no other alternative method that does not throw an exception. So there is no alternative that returns null when no result is found. (Returning java.util.Optional would also be an even better option.) So clients are forced to use exception handling for flow control. Whereas EntityManager does support null for single results, such as in EntityManager.find(class, primaryId). Which is a bit strange in design, because one would expect that a method that finds something by a primary key would have a higher chance of not returning null, than a custom query. So the throwability of getSingleResult() will more often lead to runtime exceptions than if that throwability would exist in find(), when these methods would be used equally.
This is also inconsistent with Hibernate's Query.uniqueResult(), which does return null.
Also, forcing clients to use exception handling, is an anti-pattern. The well known Java engineer Joshua Bloch wrote in Effective Java (a de facto standard best-practices guide): "Use exceptions only for exceptional conditions; they should never be used for ordinary control flow". The absence of a value is not an exceptional condition. It also says: "A well-designed API must not force its clients to use exceptions for ordinary control flow". But getSingleResult() does force clients to use exception handling for control flow in common situations.
The authors of Hibernate wrote in the bible of JPA/Hibernate, Java Persistence with Hibernate, 2nd edition: "Now, the ugly bits: if there are no results, getSingleResult() throws a NoResultException. You’d expect a null for this type of perfectly benign query. This is rather tragic, because it forces you to guard this code with a try/catch block. In fact, it forces you to always wrap a call of getSingleResult(), because you can’t know whether the row(s) will be present." So even the authors of Hibernate (from which JPA was born and inspired), don't like it.
A lot of people look for solutions to this getSingleResult() problem: https://stackoverflow.com/questions/2002993/jpa-getsingleresult-or-null
If a method like Query.getSingleResultOrNull() would be available, than this problem would be gone because then clients are not forced anymore. An alternative name using find like EntityManger.find() instead of get, would be Query.findSingleResult(). Without this solution, clients are forced to write helper methods to the reduce the boilerplate code caused by having to catch NoResultException every time, such as:
class JpaResultHelper {
public static Object getSingleResultOrNull(Query query) {
try {
return query.getSingleResult();
} catch (NoResultException ex) {
return null;
}
}
}
An even better method for the Query class would be one that return java.util.Optional, to reduce accidental NullPointerExceptions.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.
-- Gavin King IBM
There would be not much point to adding getOptionalResult() since you can already write:
em.createQuery(queryString, Book.class).getResultStream().findFirst()
If you want to obtain an Optional.
The request is to optimize the case where you do want a null, which is messy as soon as you have to deal with Optional, since you would have to write
em.createQuery(queryString, Book.class).getOptionalResult().orElse(null)
This is only just barely better than what you can already write today:
em.createQuery(queryString,
Book.class).getResultStream().findFirst().orElse(null)
On Tue, Dec 1, 2020 at 1:42 PM Valentin Brandl [email protected] wrote:
I like this idea, but I really think the return type Should be Optional<T>
Gavin King IBM
I agree that returning null can be a source of typesafe problems. But throwing an exception when beginning clients don't expect it and no obvious immediate alternative is available and similar API's that are running behind the scenes (Hibernate's uniqueResult()) have different behavior, can also be a problem.
"Whatever, the correct way to use this API is definitely not to call getSingleResult() and catch the exception! Indeed, I'm pretty sure that the spec explicitly states that you're not even allowed to do it that way. Exceptions in JPA are usually considered unrecoverable."
This is written as the solution in the book Java Persistence with Hibernate:
try {
TypedQuery<Item> query = em.createQuery("select i from Item i where i.id = :id", Item.class).setParameter("id", 1234l);
Item item = query.getSingleResult();
// ...
} catch (NoResultException ex) {
// ...
}
The book also says that NoResultException is a non-fatal JPA exception, so that the transaction won't be marked for rollback. The same is written for NonUniqueResultException and LockTimeoutException. The JPA 2.2 specification pdf confirms this.
About getResultStream(), this method is only available since JPA 2.2 (2017), so it isn't even mentioned in the book. If clients would like something similar to getResultStream(), then they would have to do this:
em.createQuery(queryString,Book.class)
.setMaxResults(1)
.getResultList()
.stream()
.findFirst()
.orElse(null);
Which isn't very obvious for a situation what widely occurs in database access code.
So getSingleResult() basically did push users to use exceptions, and it is provided as the solution in the Hibernate book.
em.createQuery(queryString, Book.class).getResultStream().findFirst() could be a solution, but the main problem that I have with it is that it isn't an obvious solution knowing that getSingleResult() exists, for:
-new users
-users who come from Hibernate
-users who come from pre-JPA 2.2
-users who have completely read the bible Java Persistence With Hibernate.
The most obvious method that they will pick when they expect one or zero results is getSingleResult(), and then hopefully wrap it with exception handeling. If a getOptionalResult() would exist, things would be more explicit, which reduces the occurs of bugs at runtime.
OK, so I didn't remember that.
Indeed, the recoverability of this exception apparently dates back well before JPA, to ancient versions of Hibernate:
/**
* Thrown when the application calls <tt>Query.uniqueResult()</tt> and
* the query returned more than one result. Unlike all other Hibernate
* exceptions, this one is recoverable!
*
* @author Gavin King
*/
public class NonUniqueResultException extends HibernateException { ... }
Funny. It's been a long time :-)
Nevertheless, I insist that this isn't the best way to write this code, no matter what it says in the book. If I'm not sure how many results there are going to be, I would always use getResultList() or getResultStream().
Here, getResultStream() seems to be your best bet.
To be clear, I'm not arguing against adding a getSingleResultOrNull() method to Query. Indeed, I think on balance it's probably a good idea.
On Tue, Dec 1, 2020 at 4:38 PM jtdev [email protected] wrote:
I agree that returning null can be a source of typesafe problems. But throwing an exception when beginning clients don't expect it and no obvious immediate alternative is available and similar API's that are running behind the scenes (Hibernate's uniqueResult()) have different behavior, can also be a problem.
"Whatever, the correct way to use this API is definitely not to call getSingleResult() and catch the exception! Indeed, I'm pretty sure that the spec explicitly states that you're not even allowed to do it that way. Exceptions in JPA are usually considered unrecoverable."
This is written as the solution in the book Java Persistence with Hibernate:
try { TypedQuery<Item> query = em.createQuery("select i from Item i where i.id = :id", Item.class).setParameter("id", 1234l); Item item = query.getSingleResult(); // ... } catch (NoResultException ex) { // ... }
The book also says that NoResultException is a non-fatal JPA exception, so that the transaction won't be marked for rollback. The same is written for NonUniqueResultException and LockTimeoutException. The JPA 2.2 specification pdf confirms this.
About getResultStream(), this method is only available since JPA 2.2 (2017), so it isn't even mentioned in the book. If clients would like something similar to getResultStream(), then they would have to do this:
em.createQuery(queryString,Book.class) .setMaxResults(1) .getResultList() .stream() .findFirst() .orElse(null);
Which isn't very obvious for a situation what widely occurs in database access code. So getSingleResult() basically did push users to use exceptions, and it is provided as the solution in the Hibernate book.
em.createQuery(queryString, Book.class).getResultStream().findFirst() could be a solution, but the main problem that I have with it is that it isn't an obvious solution knowing that getSingleResult() exists, for: -new users -users who come from Hibernate -users who come from pre-JPA 2.2 -users who have completely read the bible Java Persistence With Hibernate.
The most obvious method that they will pick when they expect one or zero results is getSingleResult(), and then hopefully wrap it with exception handeling. If a getOptionalResult() would exist, things would be more explicit, which reduces the occurs of bugs at runtime.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.
-- Gavin King IBM
query.getResultStream().findFirst() is not actually the same as
try {
return Optional.of(query.getSingleResult();
} catch (NoResultException _nre) {
return Optional.empty();
}
Since getSingleResult will throw if the query returns more than one result, while getResultStream will not. So the semantic is a bit different here...
In fact, the code below generates the same results as a method returning an optional.
em.createQuery(queryString, Book.class).getResultStream().findFirst().orElse(null)
However, I think it describes better what is being done.
em.createQuery(queryString, Book.class).getOptionalResult().orElse(null).
I think a method returning null is not so good and in fact, it is a source of bugs. Using the Optional it matches to cases that the client is querying to one-row data, but it can be not found in the database. In general, we don't need a "null" value, but just to know if the data exist or not and apply some task to the data if existing. Thus, in general, we'll not use orElse(null).
It is not a problem if we have the getSingleResult() to cases that the query should return data every time and getOptionalResult() to optional data.
Em ter., 1 de dez. de 2020 às 12:54, Gavin King [email protected] escreveu:
OK, so I didn't remember that.
Indeed, the recoverability of this exception apparently dates back well before JPA, to ancient versions of Hibernate:
/**
- Thrown when the application calls Query.uniqueResult() and
- the query returned more than one result. Unlike all other Hibernate
- exceptions, this one is recoverable!
- @author Gavin King */ public class NonUniqueResultException extends HibernateException { ... }
Funny. It's been a long time :-)
Nevertheless, I insist that this isn't the best way to write this code, no matter what it says in the book. If I'm not sure how many results there are going to be, I would always use getResultList() or getResultStream().
Here, getResultStream() seems to be your best bet.
To be clear, I'm not arguing against adding a getSingleResultOrNull() method to Query. Indeed, I think on balance it's probably a good idea.
On Tue, Dec 1, 2020 at 4:38 PM jtdev [email protected] wrote:
I agree that returning null can be a source of typesafe problems. But throwing an exception when beginning clients don't expect it and no obvious immediate alternative is available and similar API's that are running behind the scenes (Hibernate's uniqueResult()) have different behavior, can also be a problem.
"Whatever, the correct way to use this API is definitely not to call getSingleResult() and catch the exception! Indeed, I'm pretty sure that the spec explicitly states that you're not even allowed to do it that way. Exceptions in JPA are usually considered unrecoverable."
This is written as the solution in the book Java Persistence with Hibernate:
try { TypedQuery<Item> query = em.createQuery("select i from Item i where i.id = :id", Item.class).setParameter("id", 1234l); Item item = query.getSingleResult(); // ... } catch (NoResultException ex) { // ... }
The book also says that NoResultException is a non-fatal JPA exception, so that the transaction won't be marked for rollback. The same is written for NonUniqueResultException and LockTimeoutException. The JPA 2.2 specification pdf confirms this.
About getResultStream(), this method is only available since JPA 2.2 (2017), so it isn't even mentioned in the book. If clients would like something similar to getResultStream(), then they would have to do this:
em.createQuery(queryString,Book.class) .setMaxResults(1) .getResultList() .stream() .findFirst() .orElse(null);
Which isn't very obvious for a situation what widely occurs in database access code. So getSingleResult() basically did push users to use exceptions, and it is provided as the solution in the Hibernate book.
em.createQuery(queryString, Book.class).getResultStream().findFirst() could be a solution, but the main problem that I have with it is that it isn't an obvious solution knowing that getSingleResult() exists, for: -new users -users who come from Hibernate -users who come from pre-JPA 2.2 -users who have completely read the bible Java Persistence With Hibernate.
The most obvious method that they will pick when they expect one or zero results is getSingleResult(), and then hopefully wrap it with exception handeling. If a getOptionalResult() would exist, things would be more explicit, which reduces the occurs of bugs at runtime.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.
-- Gavin King IBM
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jpa-api/issues/298#issuecomment-736642518, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACN424GFRQ7GHQXGWHTQ2NDSSUGUBANCNFSM4UG3DDHA .
Sure, but the point I'm making is there's only two usecases here:
- We expect the row to exist, and if it doesn't exist we want to treat that as an "error".
- We're not sure if the row will be there or not, and we want to receive a null to indicate nonexistence, which we will treat as something "normal".
Case 1. is already perfectly well served by the existing method.
Case 2. would be well served by introducing getSingleResultOrNull(), which is clear and very explicit.
Introducing an Optional doesn't really help in either case: we don't need anything new for Case 1, and for Case 2 it's quite unwieldy because I have to write getOptionalResult().orElse(null). (And realistically it's never going to be anything other than null.)
So Optional just wouldn't be adding anything useful here.
On Tue, Dec 1, 2020 at 5:00 PM Valentin Brandl [email protected] wrote:
query.getResultStream().findFirst() is not actually the same as
try { return Optional.of(query.getSingleResult(); } catch (NoResultException _nre) { return Optional.empty(); }
Since getSingleResult will throw if the query returns more than one result, while getResultStream will not. So the semantic is a bit different here...
— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.
-- Gavin King IBM
On Tue, Dec 1, 2020 at 5:46 PM Rhuan Rocha [email protected] wrote:
I think a method returning null is not so good and in fact, it is a source
of bugs. Using the Optional it matches to cases that the client is querying to one-row data, but it can be not found in the database.
No, Optional is not typesafe, it's impossible to have a Haskell-style Maybe type in Java because Java doesn't have sum types.
So Optional is no more or less typesafe than getSingleResultOrNull(), it's merely more verbose.
-- Gavin King IBM
Null or Optional.... that can be a big discussion by itself.
Optional is not just intended for the use in Java Collection API results. Its intent is: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Optional.html "A container object which may or may not contain a non-null value." "API Note: Optional is primarily intended for use as a method return type where there is a clear need to represent "no result," and where using null is likely to cause errors."
So something like getOptionalResult() would exactly match this api intent.
Optional is embraced in the standard Java libary for code starting from Java 8. Java SE 8 Optional usage: https://docs.oracle.com/javase/8/docs/api/java/util/class-use/Optional.html vs Java SE 15 Optional usage: https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/class-use/Optional.html A usage increase trend is visible.
Also, Optional is also used in Spring Web and Spring Data. For example, Spring data repositories support both java.util.Optional, com.google.common.base.Optional, scala.Option, io.vavr.control.Option, so that you can write dynamic query methods like:
Optional<Book> findFirstOrderByPublicationDateDesc()
Another example with Optional in a Spring Data reference example: https://docs.spring.io/spring-data/commons/docs/current/reference/html/#auditing.auditor-aware
@Override
public Optional<User> getCurrentAuditor() {
return Optional.ofNullable(SecurityContextHolder.getContext())
.map(SecurityContext::getAuthentication)
.filter(Authentication::isAuthenticated)
.map(Authentication::getPrincipal)
.map(User.class::cast);
}
Spring's DataAccessUtils also has multiple singleResult helper methods:
<T> T nullableSingleResult(Collection<T> results)
<T> T requiredSingleResult(Collection<T> results)
<T> T requiredUniqueResult(Collection<T> results)
<T> T singleResult(Collection<T> results)
<T> T uniqueResult(Collection<T> results)
The CrudRepository interface from Spring Data also provides Optional<T> findById(ID primaryKey), which would be the same as the proposed getOptionalResult().
However, Kotlin users don't like Optional, because Kotlin has null-safety language features by itself, which are less verbose. So in Spring Data, Kotlin users have the possibility to also use a null-version of findById via an Kotlin extension method in CrudRepositoryExtensions.kt:
fun <T, ID> CrudRepository<T, ID>.findByIdOrNull(id: ID): T? = findById(id).orElse(null)
So that they can use code like:
val book: Book? = repository.findByIdOrNull(123) // null safe
or
val book: Book = repository.findByIdOrNull(123) ?: throw NoBookException(123) // null safe
So some people (like Kotlin users) would probably prefer getSingleResultOrNull(), while some others would prefer getOptionalResult().
But JPA should probably primarily be focussed on Java, so then Optional would be the way to go. But also adding a null version in addition, would even be better.
I think that the TypedQuery interface would benefit from having this:
public default Optional<T> getOptionalResult() {
try {
return Optional.of(getSingleResult());
} catch (NoResultException e) {
return Optional.empty();
}
}
BTW, as the Query interface itself, I think that direct uses of it should be considered something largely as deprecated and migration to TypedQuery, StoredProcedureQuery or to queries from criteria API should be recommended whenever possible.
I have seen uses of Query (in an admittedly legacy codebase) for direct queries (for the lack of a better term), returning an Object[] of all the queried fields. So you write complex queries with possible DBMS specific parts and just execute it via the Query API. I don't know if this (especially the DBMS specific part) is possible with TypedQuery or StoredProcedureQuery.
My experience is that the NoResultException is a RuntimeException and so when upgrading from a version where we were expecting a null value there is no way to know that this exception could be thrown until it happens. If it has been decided that the use of the exception should be used for flow control, then this really shouldn't be thrown only at runtime.
Why not have an overloaded version of getSingleResult that takes a value to return if one can't be found. So you could write query.getSingleResult(null) to get null if an entity can't be found. In either case since the methods don't return a collection it should still throw an exception if there's more than one entity found.
adding a getSingleResultOrNull() method to Query.
I'm in the camp that prefers returning null rather than throwing NoResultException. I believe returning null is more useful and easier to use and we can use @Nullable to assist.
So some people (like Kotlin users) would probably prefer getSingleResultOrNull(), while some others would prefer getOptionalResult().
I agreed. This is why for me the API tends to have 2 methods:
@NonNullApi
... {
// can return null, preferred by kotlin and people happy with null
@Nullable T findOne()
// used by code that prefers Optional
Optional<T> findOneOrEmpty()
}
For 3.2.0 release we add getSingleResultOrNull() method. We are not in favour adding explicit variants with Optional<T> as we think it is easy for user to call Optional.ofNullable(query.getSingleResultOrNull() himself. Also if we want to use Optional here, it would make sense to adopt this type through the rest of the persistence APIs.
IMO it would be nice to have persistence APIs return Optional where null could be returned but this would be a breaking change. Are there plans to change the APIs for the next major release?