spring-projects / spring-data-jpa

Simplifies the development of creating a JPA-based data access layer.
https://spring.io/projects/spring-data-jpa/
Apache License 2.0
3.01k stars 1.42k forks source link

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 closed 5 months ago

suiheyu commented 1 year ago

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 {

List<ResourceOwnershipPO> findAllByOwnershipPathStartingWith(String ownershipStartPath);

}


>the logic of how I call JpaResourceOwnershipRepository
```java
.........
    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
```java
    @Transactional
    @Override
    public void deleteById(ID id) {

        Assert.notNull(id, ID_MUST_NOT_BE_NULL);

        findById(id).ifPresent(this::delete);
    }
quaff commented 1 year ago

Good catch.

quaff commented 1 year ago

I fixed it with https://github.com/spring-projects/spring-data-jpa/pull/3194, test case added to guard that all modifying methods are annotated with @Transactional.

suiheyu commented 1 year ago

ok ,thanks

mp911de commented 5 months ago

That has been fixed via #3499