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

Open
suiheyu opened this issue Oct 9, 2023 · 3 comments · May be fixed by #3194
Assignees
Labels
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 added a commit to quaff/spring-data-jpa that referenced this issue Oct 13, 2023
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants