spring-projects / spring-data-commons

Spring Data Commons. Interfaces and code shared between the various datastore specific implementations.
https://spring.io/projects/spring-data
Apache License 2.0
771 stars 672 forks source link

@DomainEvents method not being called on saveAll with Window<> #2938

Closed jiaheng closed 1 year ago

jiaheng commented 1 year ago

I have an entity which will registerEvent when I update the field, and MyClassModifyEventHandler.handle() is expected to be executed upon repository.save() or repository.saveAll()

@DynamicInsert
@DynamicUpdate
@Entity
public class MyClass extends AbstractAggregateRoot<MyClass> {

    @Id private UUID id;

    @Column(name = "field")
    private String field;

    @CreationTimestamp Instant createdAt;
    @UpdateTimestamp Instant updatedAt;
    @Version Integer version;

    public void setField(String field) {
        this.field = field;
        registerEvent(new MyClassModifyEvent(this));
    }
}
@Component
public class MyClassModifyEventHandler {
    @EventListener
    public void handle(final MyClassModifyEvent persistenceEvent) {
        // do something
    }
}

I have a method which will query a list of MyClass and update their respective fields.

    @Transactional
    public void eventHandlerNotBeingCalled() {
        // ..
        Window<MyClass> myClasses =
                myClassRepo.findFirst20(ScrollPosition.keyset());

        for (MyClass myClass : myClasses) {
            myClass.setField("newField");
        }
        myClassRepo.findFirst20.saveAll(myClasses);
        // working solution
        // myClassRepo.findFirst20.saveAll(myClasses.toList());
    }

So with this, the @EventListener is not working even though the event is registered before saveAll(). If I calling saveAll on List<MyClass> instead (ie myClassRepo.saveAll(myClasses.toList())), the MyClassModifyEventHandler.handle will be called. Is this expected behaviour?

quaff commented 1 year ago

Could you provide a minimal reproducer project?

jiaheng commented 1 year ago

Could you provide a minimal reproducer project?

@quaff here is the demo project https://github.com/jiaheng/spring-data-jpa-domain-event, and these are the steps to reproduce:

  1. Insert record(s) via POST /my-classes endpoint
    curl -X 'POST' \
    'http://localhost:8080/my-classes' \
    -H 'accept: */*' \
    -d ''
  2. Trigger batch update via POST /my-classes/created/update endpoint
    curl -X 'POST' \
    'http://localhost:8080/my-classes/created/update' \
    -H 'accept: */*' \
    -d ''

Based on the demo code EventHandler.handleMyClassUpdateEvent is not being called upon myClassRepository.saveAll(myClasses). By changing line here to myClassRepository.saveAll(myClasses.toList());, EventHandler.handleMyClassUpdateEvent will be called using same steps above.

quaff commented 1 year ago

It's not minimal, and doesn't contains any test case.

jiaheng commented 1 year ago

It's not minimal, and doesn't contains any test case.

@quaff I updated spring-data-jpa-domain-event repo to strip away some unnecessary code and component. Also I added 2 test cases to compare the behaviour of using List<> and Window<>. Let me know if this works for you.

quaff commented 1 year ago

It can be minified to this:

package com.example.demo.service;

import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.Id;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.event.EventListener;
import org.springframework.data.domain.AbstractAggregateRoot;
import org.springframework.data.domain.KeysetScrollPosition;
import org.springframework.data.domain.ScrollPosition;
import org.springframework.data.domain.Window;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.config.EnableJpaRepositories;
import org.springframework.stereotype.Repository;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.BDDMockito.then;

@EnableJpaRepositories(considerNestedRepositories = true)
@SpringBootTest
class DomainEventHandlerTest {

    @Autowired
    MyClassRepository myClassRepository;

    @Autowired
    MyClassService myClassService;

    @SpyBean
    EventHandler eventHandler;

    @Test
    void test() {
        myClassRepository.save(new MyClass());
        myClassService.update();
        then(eventHandler).should().handleMyClassUpdateEvent(any());
    }

    @TestConfiguration
    static class Config {
        @Bean
        MyClassService myClassService(MyClassRepository myClassRepository) {
            return new MyClassService(myClassRepository);
        }

        @Bean
        EventHandler eventHandler() {
            return new EventHandler();
        }
    }

    @Entity
    static class MyClass extends AbstractAggregateRoot<MyClass> {
        @Id
        @GeneratedValue
        private Integer id;

        private String field;

        public void setField(String field) {
            this.field = field;
            registerEvent(new MyClassUpdateEvent());
        }
    }

    @Repository
    interface MyClassRepository extends JpaRepository<MyClass, Integer> {
        Window<MyClass> findFirst1By(KeysetScrollPosition position);
    }

    static class MyClassUpdateEvent {
    }

    static class MyClassService {
        private final MyClassRepository myClassRepository;

        public MyClassService(MyClassRepository myClassRepository) {
            this.myClassRepository = myClassRepository;
        }

        public void update() {
            Window<MyClass> myClasses = myClassRepository.findFirst1By(ScrollPosition.keyset());
            for (MyClass myClass : myClasses) {
                myClass.setField("updated");
            }
            myClassRepository.saveAll(myClasses); // change myClasses to myClasses.toList() will pass
        }
    }

    static class EventHandler {
        @EventListener
        public void handleMyClassUpdateEvent(MyClassUpdateEvent event) {
        }
    }

}
quaff commented 1 year ago

I believe it's a bug of spring-data-commons, I'm going to fix it.

jiaheng commented 1 year ago

It can be minified to this:

package com.example.demo.service;

import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.Id;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.event.EventListener;
import org.springframework.data.domain.AbstractAggregateRoot;
import org.springframework.data.domain.KeysetScrollPosition;
import org.springframework.data.domain.ScrollPosition;
import org.springframework.data.domain.Window;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.config.EnableJpaRepositories;
import org.springframework.stereotype.Repository;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.BDDMockito.then;

@EnableJpaRepositories(considerNestedRepositories = true)
@SpringBootTest
class DomainEventHandlerTest {

  @Autowired
  MyClassRepository myClassRepository;

  @Autowired
  MyClassService myClassService;

  @SpyBean
  EventHandler eventHandler;

  @Test
  void test() {
        myClassRepository.save(new MyClass());
      myClassService.update();
        then(eventHandler).should().handleMyClassUpdateEvent(any());
  }

  @TestConfiguration
  static class Config {
      @Bean
      MyClassService myClassService(MyClassRepository myClassRepository) {
          return new MyClassService(myClassRepository);
      }

      @Bean
      EventHandler eventHandler() {
          return new EventHandler();
      }
  }

  @Entity
  static class MyClass extends AbstractAggregateRoot<MyClass> {
      @Id
      @GeneratedValue
      private Integer id;

      private String field;

      public void setField(String field) {
          this.field = field;
          registerEvent(new MyClassUpdateEvent());
      }
  }

  @Repository
  interface MyClassRepository extends JpaRepository<MyClass, Integer> {
      Window<MyClass> findFirst1By(KeysetScrollPosition position);
  }

  static class MyClassUpdateEvent {
  }

  static class MyClassService {
      private final MyClassRepository myClassRepository;

      public MyClassService(MyClassRepository myClassRepository) {
          this.myClassRepository = myClassRepository;
      }

      public void update() {
          Window<MyClass> myClasses = myClassRepository.findFirst1By(ScrollPosition.keyset());
          for (MyClass myClass : myClasses) {
              myClass.setField("updated");
          }
          myClassRepository.saveAll(myClasses); // change myClasses to myClasses.toList() will pass
      }
  }

  static class EventHandler {
      @EventListener
      public void handleMyClassUpdateEvent(MyClassUpdateEvent event) {
      }
  }

}

Got it. Thanks for the suggestion.

odrotbohm commented 1 year ago

This has been fixed for the 2.7, 3.0, 3.1 bugfix releases and the 3.2 RC1 coming. I've turned this into a bug as we already had test cases in place that described the feature as being expected to be available but ultimately didn't actually test it was.

This is now in place by handling calls to saveAll(…) explicitly and considering the Iterable handed to it as is.