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
2.93k stars 1.39k forks source link

findById ignores @SQLRestriction annotation when performed in the same transaction as the save #3467

Closed khaledbaghdadii closed 1 month ago

khaledbaghdadii commented 1 month ago

When saving an entity with soft deleted flag as true and trying to fetch it in the same transaction using findById it is being returned. This behaviour isn't the case if transaction are disabled or for example if findAllById or findByIdAndCountLessThan (a custom method) used.

Here's an example:

I have this entity:

@Data
@Builder
@AllArgsConstructor
@NoArgsConstructor
@Entity
@Table(name = "item_entity")
@SQLDelete(sql = "UPDATE item_entity SET deleted = true WHERE id = ?")
@SQLRestriction("deleted=false")
public class ItemEntity {
    @Id
    @Builder.Default
    private String id = UUID.randomUUID().toString();

    private int count;
    @Builder.Default
    private boolean deleted  =false;
}

and this test

@DataJpaTest
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
@Testcontainers
@ContextConfiguration(initializers = SingletonPostgresDbContainerContextInitializer.class)
class TestItem {
    @Autowired
    private ItemJpaRepository repository;

    @Test
    void test(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);
        Optional<ItemEntity> actualItemEntity = repository.findById(itemEntity.getId());
        Assertions.assertThat(actualItemEntity).isEmpty();
    }
}

This is the ItemJpaRepository

@Repository
public interface ItemJpaRepository extends JpaRepository<ItemEntity,String> {
    Optional<ItemEntity> findByIdAndCountLessThan(String s, int count);
}

This test us failing with the following error Expecting an empty Optional but was containing value: ItemEntity(id=0d033adf-581a-42b5-9c60-8e141acfcfd3, count=1, deleted=true)

If transactions are disabled by adding this annotation to the test @Transactional(propagation = Propagation.NEVER) the test pass.

Also, if the original test is rewritted like that it passes even when the save and the find are in the same transaction

@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
@Testcontainers
@ContextConfiguration(initializers = SingletonPostgresDbContainerContextInitializer.class)
class TestItem {
    @Autowired
    private ItemJpaRepository repository;

    @Test
    void test(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);
        List<ItemEntity> itemEntities = repository.findAllById(List.of(itemEntity.getId()));
        Assertions.assertThat(itemEntities).isEmpty();
    }
}

I am using spring-boot-starter-data-jpa version 3.2.5

khaledbaghdadii commented 1 month ago

This is a detailed of all the tests created and all of them pass as expected except for the findById when transactions are enabled

@DataJpaTest
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
@Testcontainers
@ContextConfiguration(initializers = SingletonPostgresDbContainerContextInitializer.class)
class TestItem {
    @Autowired
    private ItemJpaRepository repository;

    @Test
    //This test passes
    void testFindAllById(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);
        List<ItemEntity> itemEntities = repository.findAllById(List.of(itemEntity.getId()));
        Assertions.assertThat(itemEntities).isEmpty();
    }

    @Test
        //This test fails
    void testFindById(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);
        Optional<ItemEntity> itemEntityOptional = repository.findById(itemEntity.getId());
        Assertions.assertThat(itemEntityOptional).isEmpty();
    }

    @Test
    @Transactional(propagation = Propagation.NEVER)
        //This test passes
    void testFindByIdNonTransactional(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);
        Optional<ItemEntity> itemEntityOptional = repository.findById(itemEntity.getId());
        Assertions.assertThat(itemEntityOptional).isEmpty();
    }

    @Test
        //This test passes
    void existsById(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);

        Assertions.assertThat(repository.existsById(itemEntity.getId())).isFalse();
    }

    @Test
        //This test passes
    void findByIdAndCountLessThan(){
        ItemEntity itemEntity = ItemEntity.builder()
                .count(1)
                .deleted(true)
                .build();
        repository.save(itemEntity);
        repository.deleteById(itemEntity.getId());

        Optional<ItemEntity> itemEntityOptional = repository.findByIdAndCountLessThan(itemEntity.getId(),999);
        Assertions.assertThat(itemEntityOptional).isEmpty();

    }
}
quaff commented 1 month ago

Have you tried using @org.hibernate.annotations.SoftDelete instead of @SQLDelete and @SQLRestriction?

khaledbaghdadii commented 1 month ago

This works but there are some cases where I'm using the @SQLRetriction for reasons other than soft delete, and also cases where I need the @SQLDelete(sql = "UPDATE item_entity SET deleted = true WHERE id = ?") without the @SQLRestriction to be able to fetch entities that are soft deleted if they are part of another entity that is not deleted.

I'm gonna rename the issue to match the problem with @SQLRestriction instead of specifically for soft delete cause I don't think this is the correct behaviour anyways if all other JPARepository methods are working fine except for it

quaff commented 1 month ago

@khaledbaghdadii Your test should pass if test method annotated with @Transactional(propagation = Propagation.NOT_SUPPORTED) or call entityManager.flush();entityManager.clear(); after save, without that the entity is in persistence context and not flush to database then findById will return the managed entity not querying database and applying @SQLRestriction.

khaledbaghdadii commented 1 month ago

I was able to make the test pass but I do belive there's a bug with the behaviour of findById since it is not cosistent with the other JPA methods

quaff commented 1 month ago

I was able to make the test pass but I do belive there's a bug with the behaviour of findById since it is not cosistent with the other JPA methods

findById is simply delegate to entityManager.find(domainType, id) which will find managed entity from persistence context first, if you insist on it's a bug, you should open an issue for Hibernate.

schauder commented 1 month ago

This is just a classical problem with JPA (not Spring Data JPA). It relies heavily on the cache und doesn't reload reload data as @quaff wrote in his comment. Of course Spring Data could apply some clear or flush magic, but that comes at the very least with a performance penalty and in many cases with surprising behaviour in other places.

Therefore there isn't much we can do about this.

khaledbaghdadii commented 1 month ago

Yeah fair enough, thank you both! I'll close this issue.