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

Should the @Transactional annotation be added to the delete method in the SimpleJpaRepository class, which takes a parameter of type Specification<T>? Otherwise, it may cause a JDBC connection read-only exception when calling this method due to the @Transactional(readOnly = true) annotation on the class. #3188

Closed
suiheyu opened this issue Oct 9, 2023 · 4 comments · May be fixed by #3194
Assignees
Labels
status: superseded An issue that has been superseded by another type: bug A general bug

Comments

@suiheyu
Copy link

suiheyu commented Oct 9, 2023

In my work, I have been using the convenient features provided by Spring Data JPA's JpaRepository for rapid development. However, when I attempted to call the following code for the first time, it resulted in a java.sql.SQLException.

The declaration of my Repository

import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
import org.springframework.data.repository.ListCrudRepository;
import org.springframework.stereotype.Repository;

import java.util.List;

@Repository
public interface JpaResourceOwnershipRepository extends ListCrudRepository<ResourceOwnershipPO, String>, JpaSpecificationExecutor<ResourceOwnershipPO> {

    List<ResourceOwnershipPO> findAllByOwnershipPathStartingWith(String ownershipStartPath);
}

the logic of how I call JpaResourceOwnershipRepository

.........
    private final JpaResourceOwnershipRepository repository;

    public void delete(ResourceReference resourceReference) {
        Objects.requireNonNull(resourceReference);
        repository.delete(resourceReferenceSpec(resourceReference));
    }

    private Specification<ResourceOwnershipPO> resourceReferenceSpec(ResourceReference resourceReference) {
        return resourceReferenceSpec(resourceReference.resourceType(),
                (root, query, cb) -> cb.equal(root.get(resourceId), resourceReference.resourceId())
        );
    }
..........

The exception message "Connection is read-only. Queries leading to data modification are not allowed" indicates that a read-only transaction is in effect, even though you didn't explicitly declare any "read-only transactions" in your business code. Upon investigation, I discovered that Spring Data JPA, specifically within the org.springframework.data.repository.core.support.TransactionalRepositoryProxyPostProcessor.RepositoryAnnotationTransactionAttributeSource#computeTransactionAttribute method, obtains TransactionAttribute based on certain priorities. By default, it first checks whether your JpaResourceOwnershipRepository methods and classes have relevant transaction annotations. Then, it looks at the declarations in Spring Data JPA's SimpleJpaRepository class. Subsequently, I reviewed the source code for the corresponding method in SimpleJpaRepository. Please see the following code snippet:

SimpleJpaRepository source code

	@Override
	public long delete(Specification<T> spec) {

		CriteriaBuilder builder = this.entityManager.getCriteriaBuilder();
		CriteriaDelete<T> delete = builder.createCriteriaDelete(getDomainClass());

		if (spec != null) {
			Predicate predicate = spec.toPredicate(delete.from(getDomainClass()), null, builder);

			if (predicate != null) {
				delete.where(predicate);
			}
		}

		return this.entityManager.createQuery(delete).executeUpdate();
	}

Upon reaching this point, I wonder whether this method should have been annotated with @Transactional but it wasn't. The @Transactional annotation's default value for readOnly is false, and considering the semantics of the method name, "delete" is clearly a write operation. It should not use the default false value that be specified on SimpleJpaRepository class . To confirm my suspicion, I examined other methods in SimpleJpaRepository with deletion semantics, and I found that all the other methods have the @Transactional annotation, except for the one I am using. Is this intentional behavior, or is it indeed a bug?

SimpleJpaRepository source code

	@Transactional
	@Override
	public void deleteById(ID id) {

		Assert.notNull(id, ID_MUST_NOT_BE_NULL);

		findById(id).ifPresent(this::delete);
	}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 9, 2023
@quaff
Copy link
Contributor

quaff commented Oct 9, 2023

Good catch.

@quaff
Copy link
Contributor

quaff commented Oct 13, 2023

I fixed it with #3194, test case added to guard that all modifying methods are annotated with @Transactional.

@suiheyu
Copy link
Author

suiheyu commented Oct 23, 2023

ok ,thanks

@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 23, 2023
@mp911de mp911de assigned mp911de and unassigned gregturn Jun 13, 2024
@mp911de mp911de added the status: superseded An issue that has been superseded by another label Jun 13, 2024
@mp911de
Copy link
Member

mp911de commented Jun 13, 2024

That has been fixed via #3499

@mp911de mp911de closed this as completed Jun 13, 2024
quaff added a commit to quaff/spring-data-jpa that referenced this issue Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants