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

Switch to JPA 3.2 Query.getSingleResultOrNull() #3701

Closed
Tracked by #3673 ...
david-kubecka opened this issue Dec 3, 2024 · 9 comments
Closed
Tracked by #3673 ...

Switch to JPA 3.2 Query.getSingleResultOrNull() #3701

david-kubecka opened this issue Dec 3, 2024 · 9 comments
Assignees
Labels
type: enhancement A general enhancement

Comments

@david-kubecka
Copy link

david-kubecka commented Dec 3, 2024

I'm talking about this code: https://github.com/spring-projects/spring-data-jpa/blob/main/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java#L224. As we know, getSingleResult throws an exception upon a non-existing entity. Since this is an allowed situation the exception is caught here https://github.com/spring-projects/spring-data-jpa/blob/main/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java#L92.

I wonder why this try/catch approach (generally frowned upon) is used here instead of an alternative approach using getResultList, see e.g. https://stackoverflow.com/a/2885024/13015027. Apart from a slight performance penalty (not measured), the biggest problem of the current approach is that the exception is captured by telemetry agents (see open-telemetry/opentelemetry-java-instrumentation#12125).

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

mp911de commented Dec 3, 2024

the biggest problem of the current approach is that it pollutes the logs.

Care to elaborate? There should be no logging involved from our side. We could actually switch to uniqueResultOptional. Just noticed that uniqueResultOptional is Hibernate-specific and not something that JPA defines.

Looking at the getSingleResult implementations, implementations use it merely as wrapper for getResultList post-processing.

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Dec 3, 2024
@david-kubecka
Copy link
Author

I've clarified the issue with a link (which I've found meanwhile) to an opentelemtry issue.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 3, 2024
@mp911de mp911de added the for: team-attention An issue we need to discuss as a team to make progress label Dec 4, 2024
@mp911de
Copy link
Member

mp911de commented Dec 4, 2024

This ticket is a duplicate of #3633

@david-kubecka
Copy link
Author

Ok, the linked ticket has a much better description of the underlying OTEL problem.

Still, I would like to understand why it wouldn't be possible to replace the current getSingleResult call with an approach similar to the linked SO answer. AFAIK that one doesn't rely on anything Hbernate specific, utilizing plain JPA only.

@mp911de
Copy link
Member

mp911de commented Dec 9, 2024

We comply with the JPA spec by using getSingleResult(). If we now change (after using that functionality for almost 15 years) to a different strategy, I am pretty sure that we will break someone else's application because, in some particular cases, Hibernate (or Eclipselink) is doing things in getSingleResult() slightly differently than in getResultList().

@mp911de mp911de closed this as not planned Won't fix, can't repro, duplicate, stale Dec 9, 2024
@mp911de mp911de added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged for: team-attention An issue we need to discuss as a team to make progress status: feedback-provided Feedback has been provided labels Dec 9, 2024
@david-kubecka
Copy link
Author

We comply with the JPA spec by using getSingleResult().

Can you please point me to a guideline that mandates that you must use getSingleResult in this situation?

@mp911de
Copy link
Member

mp911de commented Dec 9, 2024

From the Javadoc:

Execute a SELECT query that returns a single untyped result

and for getResultList:

Execute a SELECT query and return the query results as an untyped List

Going forward, the spec mentions:

NoResultException

The NoResultException is thrown by the persistence provider when Query.getSingleResult is invoked and there is no result to return. This exception will not cause the current transaction, if one is active, to be marked for rollback.

NonUniqueResultException

The NonUniqueResultException is thrown by the persistence provider when Query.getSingleResult or Query.getSingleResultOrNull is invoked and there is more than one result from the query. This exception will not cause the current transaction, if one is active, to be marked for rollback.

With JPA 3.2, we can switch to getSingleResultOrNull().

@david-kubecka
Copy link
Author

Yes, I read that doc snippets. And I see nothing in them that would indicate that using getResultList in your case is wrong or unsafe (assuming, of course, that providers implement the specification correctly).

Anyway, looks like getSingleResultOrNull could be available soon so if you are willing to use that then perhaps you can do that under this ticket (I can add getSingleResultOrNull as another option to the description).

@mp911de mp911de changed the title SingleEntityExecution should not use getSingleResult Switch to JPA 3.2 Query. getSingleResultOrNull() Dec 9, 2024
@mp911de mp911de added type: enhancement A general enhancement and removed status: duplicate A duplicate of another issue labels Dec 9, 2024
@mp911de mp911de self-assigned this Dec 9, 2024
@mp911de mp911de added this to the 4.0 M1 (2025.1.0) milestone Dec 9, 2024
@mp911de mp911de changed the title Switch to JPA 3.2 Query. getSingleResultOrNull() Switch to JPA 3.2 Query.getSingleResultOrNull() Dec 9, 2024
@mp911de mp911de reopened this Dec 9, 2024
@mp911de mp911de mentioned this issue Dec 9, 2024
5 tasks
@mp911de
Copy link
Member

mp911de commented Dec 9, 2024

Alright, I repurposed this ticket and marked it for inclusion for our 4.0.x branch.

mp911de added a commit that referenced this issue Dec 9, 2024
We now use getSingleResultOrNull() to avoid NoResultException handling.

Closes #3701
@mp911de mp911de mentioned this issue Dec 9, 2024
6 tasks
mp911de added a commit that referenced this issue Dec 11, 2024
We now use getSingleResultOrNull() to avoid NoResultException handling.

Closes #3701
christophstrobl pushed a commit that referenced this issue Dec 20, 2024
We now use getSingleResultOrNull() to avoid NoResultException handling.

Closes: #3701
Original Pull Request: #3695
mp911de added a commit that referenced this issue Jan 14, 2025
We now use getSingleResultOrNull() to avoid NoResultException handling.

Closes: #3701
Original Pull Request: #3695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants