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

Neo4jTemplate.DefaultExecutableQuery.getSingleResult() always executes in non-readonly transaction #2953

Open
mkoertgen opened this issue Oct 4, 2024 · 5 comments · May be fixed by #2962
Open
Labels
status: waiting-for-triage An issue we've not yet triaged

Comments

@mkoertgen
Copy link

mkoertgen commented Oct 4, 2024

When activating Neo4jTransactionManager.setValidateExistingTransaction(true) a simple findById() will result in an IllegalStateException. This is due to getSingleResult() always executing in the default transactional-template

https://github.com/spring-projects/spring-data-neo4j/blob/main/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java#L1277

whereas the outer call findById() executes in a readonly-transaction.

https://github.com/spring-projects/spring-data-neo4j/blob/main/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java#L338

How to reproduce:

  1. activate transaction validation as described in @michael-simons tips (see below)
  2. call findById() on a neo4j-template or -repository

See also

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 4, 2024
@meistermeier
Copy link
Collaborator

Thanks for reporting this. Unfortunately in this case, there is -at least from my current perspective- less we can do about this situation.
A while ago we have seen users struggling with multiple database queries getting invoked by the Neo4jTemplate for a single operation. Their code were not running within the same transaction because they did not manually introduce (Spring's) transactional boundaries e.g. by using @Transactional. To get a better user experience and less surprises, we decided to introduce also the transaction template in the Neo4jTemplate as a good will.
What you have discovered is somehow a consequence of this behaviour. While the findById flow should definitely be read-only, the getSingleResult could be part of a data-manipulating query because there are other flows that can lead here without a transaction in the first place.
But as I have said in the beginning, this is was just a first analysis from my side. I will keep the issue open to investigate this deeper and see if we could come up with a solution for this situation.

@mkoertgen
Copy link
Author

Hi @meistermeier,

Thanks for the prompt response. From what I can see at first glance, it looks promising to first fix the call-scopes in Neo4jTemplate directly. One way to go about this, would be to allow the transaction to be optionally passed down to getSingleResult() via method injection. If you don't mind I would submit a PR for this.

@meistermeier
Copy link
Collaborator

If you want to do this in a PR, I am happy to have a look.
Please keep in mind that we want to avoid breaking API changes at all cost. This means that you cannot just add a parameter to getSingleResult() but need to find a way around this. Maybe a sibling method with less visibility that accepts this from the internal calls in Neo4jTemplate only. (Just thinking loud here).

Personal addition: When you filed your PR, please don't expect a quick response within the next two weeks because I will be on vacation.

@mkoertgen
Copy link
Author

Sounds good to me, thx. 👍

mkoertgen added a commit to mkoertgen/spring-data-neo4j that referenced this issue Oct 18, 2024
mkoertgen added a commit to mkoertgen/spring-data-neo4j that referenced this issue Oct 18, 2024
mkoertgen added a commit to mkoertgen/spring-data-neo4j that referenced this issue Oct 18, 2024
@mkoertgen mkoertgen linked a pull request Oct 18, 2024 that will close this issue
@mkoertgen
Copy link
Author

mkoertgen commented Oct 18, 2024

@meistermeier Have a look at #2962 at your convenience. Happy to update if you have any remarks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants