micronaut-projects / micronaut-data

Ahead of Time Data Repositories
Apache License 2.0
467 stars 198 forks source link

@EntityGraph doesn't work in some cases. #960

Open ahimik opened 3 years ago

ahimik commented 3 years ago

Hi,

It seems like @EntityGraph annotation is just ignored in some cases, especially for methods without arguments like findAll() or list() and also in a conjunction with spring Specification. At the same time @Join works fine for findAll() with no arguments and doesn't work for findAll(Specification<T> spec).

Task List

Steps to Reproduce

  1. Create two simple entities with @ManyToOne relation:

    
    @Entity
    @Table(name = "portal.book")
    public class Book {
    
    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;
    
    @NotNull
    private String title;
    
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "author_id")
    private Author author;

}

@Entity @Table(name = "portal.author") public class Author {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@NotNull
private String name;

}

2. Define repository:

@Repository @Transactional public interface BookRepository extends CrudRepository<Book, Long> {

@Override
@EntityGraph(attributePaths = {"author"})
List<Book> findAll();

// or

@EntityGraph(attributePaths = {"author"})
List<Book> listAll();

}


### Expected Behaviour

Author must be eagerly fetched according to the `@EntityGraph` annotation in order to avoid n+1 query:

DEBUG org.hibernate.SQL - select book0_.id as id1_30, author1_.id as id1_21, book0_.author_id as author_i3_30, book0_.title as title2_30, author1_.name as name2_21 from portal.book book0 inner join portal.author author1 on book0_.authorid=author1.id


### Actual Behaviour

Author is not eagerly fetched. Addition query is executed:

DEBUG org.hibernate.SQL - select book0_.id as id13, book0_.author_id as author_i33, book0_.title as title23 from portal.book book0_

DEBUG org.hibernate.SQL - select author0_.id as id1_20, author0_.name as name2_20 from portal.author author0 where author0.id=?


### Environment Information

- **Micronaut Version:** 2.2.3
- **JDK Version:** 1.8

Some more test cases:

@Repository @Transactional public interface BookRepository extends CrudRepository<Book, Long> {

@Override
@EntityGraph(attributePaths = {"author"})
List<Book> findAll();

// ^ X. Doesn't work. Causes n+1 additional query

@Override
@Join(value = "author", type = Join.Type.FETCH)
List<Book> findAll();

// ^ V. Works fine!

@EntityGraph(attributePaths = {"author"})
List<Book> findAll(Specification<Book> spec);

// ^ X. Doesn't work. Causes n+1 additional query

@Join(value = "author", type = Join.Type.FETCH)
List<Book> findAll(Specification<Book> spec);

// ^ X. Doesn't work. Causes n+1 additional query

@EntityGraph(attributePaths = {"author"})
Page<Book> findAll(Specification<Book> spec, io.micronaut.data.model.Pageable pageable);

// ^ X. Doesn't compile. Causes: Unable to implement Repository method: BookRepository.findAll(Specification spec, Pageable pageable). Cannot query entity [Book] on non-existent property: spec

@EntityGraph(attributePaths = {"author"})
Page<Book> findAll(Specification<Book> spec, org.springframework.data.domain.Pageable pageable);

// ^ X. Compiles but causes n+1 additional query

@Join(value = "author", type = Join.Type.FETCH)
Page<Book> findAll(Specification<Book> spec, org.springframework.data.domain.Pageable pageable);

// ^ X. Compiles but causes n+1 additional query

@EntityGraph(attributePaths = {"author"})
List<Book> findByTitle(String title);

// ^ V. Works fine in all cases when there is at least 1 input parameter!!!

}

mstein commented 3 years ago

I looked a bit into this while I was working on a fix for #1160, to see if that would be something I could cover at the same time. Unfortunately, it seems like this problem would require a fair bit of rework to have the @EntityGraph annotation work in all cases. I'll just leave my findings here in case it can help. Note that I was testing on micronaut-data 3.0.1

It seems like it relates to how the methods are generated during the compilation.

Some of the signatures are translated into @Query, which are the ones that will support the @EntityGraph behavior and produce a DefaultStoredQuery (AbstractQueryInterceptor.java#L1001) that will then be sent to the HibernateJpaOperations#findAll implementation. Others will not result in a @Query being added, and will just produce a DefaultPagedQuery (AbstractQueryInterceptor.java#L953).

A DefaultPagedQuery always return an empty map for its getQueryHints() method, which is why the @EntityGraph is ignored since it requires that value to be not empty.

The class responsible to choose between the two types of query in the specific case of the findAll is the DefaultFindAllInterceptor, which specifically look for the presence of a @Query annotation.

Looking at the generated repository classes ($xxxRepository$Intercepted, $xxxRepository$Intercepted$Definition$Exec, ...) :

    @Override
    @EntityGraph(attributePaths = {"medias"})
    List<Franchise> findAll();
    // This doesn't generate a @Query
    // => EntityGraph ignored

    @Override
    @Join(value = "medias", type = Join.Type.FETCH)
    List<Franchise> findAll();
    // This generate a @Query, this is translated as : 
    // @Query('SELECT franchise_ FROM com.example.model.Franchise AS franchise_ JOIN FETCH franchise_.medias franchise_medias_')
    // => EntityGraph is working

I haven't tested with data-spring-jpa, but looking at the FindAllSpecificationInterceptor, it probably doesn't support neither @EntityGraph nor @Join, since it is not calling the jpaOperations#findAll method and rather build the query directly (because it need to translate the spring Specification into jpa criteria predicates), so it is not binding the query hints.

dstepanov commented 3 years ago

I think issue 960 should be done separately. I'm not yet sure how much has to be changed in order to support all the reported cases so I don't intend to cover this issue in this PR.

DefaultPagedQuery doesn't implements StoredQuery so it can't be used as is with the RepositoryOperations#getQueryHints method, or it would have to bypass it which is probably not desirable. We could add a signature that just take an AnnotationMetadataProvider for RepositoryOperations#getQueryHints since most of the hints are resolved from the annotations anyway and it would cover both StoredQuery and PagedQuery, but then we still have to change other places which don't call the bindQueryHints method in the first place, it seems like there are a couple Interceptors that would need to be updated. For example, the DefaultFindPageInterceptor will call the operations findPage method if there is no @Query, and HibernateJpaOperations#findPage does not seem to bind the hints on the criteria query.

Also all the data-spring-jpa interceptors don't produce a StoredQuery or PagedQuery in the first place so they would need to be handled differently.

Maybe I'll look into that part at a later date.

@mstein You are right, I would change RepositoryOperations#getQueryHints to AnnotationMetadata so it can be used in other places plus bindQueryHints to accept a map.

Also, I have noticed that this need to be updated to support jakarta namespace:

private static final String ENTITY_GRAPH_FETCH = "javax.persistence.fetchgraph";
private static final String ENTITY_GRAPH_LOAD = "javax.persistence.loadgraph";