Cosium / spring-data-jpa-entity-graph

Spring Data JPA extension allowing full dynamic usage of EntityGraph on repositories
MIT License
469 stars 52 forks source link

EntityGraph applied on QueryDsl Pagination count query #1

Closed Klinton90 closed 7 years ago

Klinton90 commented 7 years ago

First of all, many thanks for your extension, idea is just great. However, I have issue with Pageable JPA repository.

In short, you can read that topic that describes same things that I found during my investigation.

In 2 words: Pageable JpaRepository takes original query and turns it into select count(main_entity.id) ... just by replacing original select field1, field2, ..., fieldN part. As result, final statement looks like: select count(c.id) from Company c INNER JOIN FETCH c.employees WHERE c.id = :companyId" But Hibernate doesn't like "useless joins", i.e. when fields from c.employees is not part of select.

It leads to Hibernate Exception org.hibernate.QueryException: query specified join fetching, but the owner of the fetched association was not present in the select list.

Just wondering if you have any workaround for that case or probably I'm missing something...

reda-alaoui commented 7 years ago

Thank you for your kind words :)

Your case is interesting.

I had a similar issue when I started to build this library. The issue occured when I was using any paginated method with an EntityGraph. The following test was failing with your exception because of the count query:

@Transactional
@Test
public void given_brand_eg_when_findAll_paginated_then_brand_should_be_loaded(){
  Page<Product> productPage = productRepository.findAll(new PageRequest(0, 10), EntityGraphUtils.fromName(Product.PRODUCT_BRAND_EG));
  assertThat(productPage.getContent()).isNotEmpty();
  for(Product product: productPage.getContent()){
    assertThat(Hibernate.isInitialized(product.getBrand())).isTrue();
  }
}

I resolved this issue like this:

A proxy on the EntityManager determines whether the received query is a Count Query. If it is a count query, the EntityGraph is ignored.

Our issue was only occuring on 1.10.x, not 1.11.x.

However, if you use a query string containing FETCH word with our library, you will obtain the same exception, because I was only interested in our EntityGraph object and not FETCH word.

Therefore, naively, I can make a patch on our library to remove FETCH word from the Count Query.

But did you want a generic advice or this feature be implement in this library?

I will implement it anyway, but it is just out of curiosity ;)

reda-alaoui commented 7 years ago

After analysis, there are two parts.

"FETCH keyword bug on count query" to open on Spring Data Tracker

I think that you should open a bug about this on the Spring Data JPA Tracker. They should be able to solve it by removing FETCH in org.springframework.data.jpa.repository.query.QueryUtils.

Here is the problematic method:

public static String createCountQueryFor(String originalQuery, String countProjection) {

        Assert.hasText(originalQuery, "OriginalQuery must not be null or empty!");

        Matcher matcher = COUNT_MATCH.matcher(originalQuery);
        String countQuery = null;

        if (countProjection == null) {

            String variable = matcher.matches() ? matcher.group(VARIABLE_NAME_GROUP_INDEX) : null;
            boolean useVariable = variable != null && StringUtils.hasText(variable) && !variable.startsWith("new")
                    && !variable.startsWith("count(") && !variable.contains(",");

            String replacement = useVariable ? SIMPLE_COUNT_VALUE : COMPLEX_COUNT_VALUE;
            countQuery = matcher.replaceFirst(String.format(COUNT_REPLACEMENT_TEMPLATE, replacement));
        } else {
            countQuery = matcher.replaceFirst(String.format(COUNT_REPLACEMENT_TEMPLATE, countProjection));
        }

        return countQuery.replaceFirst(ORDER_BY_PART, "");
    }

Removing FETCH via the proxy would be too hacky because the count query is validated at startup. If they can't for any reason, feel free to comment this issue again.

Workaround with this extension

Meanwhile, with this extension, you can use a pagination object with an EntityGraph object and without passing FETCH in the method. i.e :

@Query("Select p from Product p INNER JOIN p.brand")
    Page<Product> findAllWithoutFetch(Pageable pageable, EntityGraph entityGraph);
@Transactional
@Test
public void given_products_when_findallwithoutfetch_and_with_eg_then_it_should_work(){
Page<Product> productPage = productRepository.findAllWithoutFetch(new PageRequest(0, 10), EntityGraphUtils.fromName(Product.PRODUCT_BRAND_EG));
    assertThat(productPage.getContent()).isNotEmpty();
    for(Product product: productPage.getContent()){
        assertThat(Hibernate.isInitialized(product.getBrand())).isTrue();
    }
}
reda-alaoui commented 7 years ago

Feel free to comment if you want this issue to be reopened ;)

Klinton90 commented 7 years ago

Thanks for fast reply. I'll try it again after hours. My case was simple. Below is pseudo code:

@Entity
@NamedEntityGraphs(value = {
    @NamedEntityGraph(name = "Category.some", attributeNodes = {
        @NamedAttributeNode("subCategories")
    })
})
public class Category{
    private Integer id;
    private String name;
    ...
    @OneToMany(mappedBy = "parentCategory", orphanRemoval = true)
    private List<SubCategory> subCategories;
}

@Entity
@NamedEntityGraphs(value = {
    @NamedEntityGraph(name = "SubCategory.some", attributeNodes = {
        @NamedAttributeNode("parentCategory")
    })
})
public class SubCategory{
    private Integer id;
    private String name;
    ...
    @ManyToOne
    @JoinColumn(name = "category_id")
    private Category parentCategory;
}

@NoRepositoryBean
public interface CategoryRepository implements JpaEntityGraphRepository<Category, Integer>, JpaEntityGraphQueryDslPredicateExecutor<Category>{

}

@Service
public class CategoryService{

    @Autowired
    private repository CategoryRepository;

    public List<Category> findCategoryBySomething(Pageable pageable, String[] someValues){
        Predicate predicate = getDefaultPredicate();
        //logic to create custom QueryDSL predicate using 'someValues'
        ...
        //Failing there. Exact JpaQueryDsl method is:
        //org.springframework.data.jpa.repository.support.QueryDslJpaRepository.createCountQuery()
        //Have to look on it after work
        return repository.findAll(predicate, pageable, EntityGraphUtils.fromName("Category.some"));
    }

    private Predicate getDefaultPredicate(){
        Predicate builder = new BooleanBuilder();
        //add shared WHERE logic
        ...
        return builder;
    }
}

I didn't have time yesterday to look all over your/JpaQueryDsl implementation, but I as per my understanding, EntityGraph is resolved to Hints, which lately are utilized by QueryDslJpaRepository... And seems like there is no quick way to filter Hints per request type...

reda-alaoui commented 7 years ago

Ok I better understand. I will take a look at your case asap.

reda-alaoui commented 7 years ago

What version of the extension are you using?

Klinton90 commented 7 years ago

I'm not sure (forgot to commit changes to git). Might be 1.11.x for extension and 1.10.3 for SpringDataJpa... And now I see my problem :) Thanks for pointing out. First thing I'll try to play with Jpa/Extension versions... I'm dumb

reda-alaoui commented 7 years ago

Ok, waiting then for your test result on the latest version :)

Klinton90 commented 7 years ago

Initial tests were made on dependencies:

springDataJpa = "1.10.3.RELEASE"
extension = "1.10.11"

Current dependencies:

    springBootVersion = "2.0.0.BUILD-SNAPSHOT"
    springDataJpa = "1.11.0.BUILD-SNAPSHOT"
    springDataCommons = "1.13.0.BUILD-SNAPSHOT"
    springBootGraphVersion = "1.11.00-SNAPSHOT"

Query works until Page.size() < result.size(). If you take a look on QueryDslJpaRepository.findAll() and PageableExecutionUtils.getPage(), you will see, that JPA team tweaked these methods. In previous build countQuery has been executed for every request. In 1.11.0 build they changed code, so if result contains less items than requested page, countQuery is skipped.

Klinton90 commented 7 years ago

Sorry... Previously I posted that Pageable QueryDSL select with EntityGraph fetch doesn't work at all (i.e. I thought that problem is on spring-data-jpa side). However, this morning I found that tests I was running are wrong (screwed up my gradle.build... forgot that I compiled your extension instead of using existing release jar from github).

In general new test results for repository:

public interface CategoryTestRepository extends JpaRepository<Category, Integer>, QueryDslPredicateExecutor<Category>{

    @EntityGraph(value = "Category.default", type = EntityGraph.EntityGraphType.LOAD)
    @Override
    Page<Category> findAll(Predicate predicate, Pageable pageable);

    @EntityGraph(value = "Category.default", type = EntityGraph.EntityGraphType.LOAD)
    @Override
    Page<Category> findAll(Pageable pageable);

}

Both methods works fine in all cases when your Extension is not added to project dependencies. However, with your extension included in project (as per my investigation even without using JpaEntityGraphRepository and JpaEntityGraphQueryDslPredicateExecutor on CustomRepository at some point Interceptors take care of EntityGraphs for all JpaRepositories) only findAll(Pageable pageable) works fine for all cases that I tested. But findAll(Predicate predicate, Pageable pageable) fails when I have more items in DB than in 1 page (see my previous comment).

Klinton90 commented 7 years ago

Example of my test data set and code:

category_id name is_active
1 Categ1 1
2 Categ2 1
3 Categ3 1
4 Categ4 0
public class CategoryTestService{
    @Autowired
    CategoryTestRepository categoryTestRepository;
    ...
    public Page<Category> findSomePageable(boolean showActive, Pageable pageable){
        PathBuilder<T> pathBuilder = new PathBuilder(domainClass, entName);
        BooleanBuilder where = new BooleanBuilder();
        if(showActive){
            where = pathBuilder.getBoolean("isActive").eq(true);
        }
        return categoryTestRepository.findAll(where, pageable);
    }

    public Page<Category> findSome(Pageable pageable){
        return categoryTestRepository.findAll(pageable);
    }
}

public class SomeTest{

    @Inject
    private CategoryTestService service;

    //this works fine, as I have only 4 items in DB/dataset but page size is 5.
    public void someTestOnePage(){
                int page = 0;
                int size = 5;
                Pageable pageable = new PageRequest(page, size);
                Page<Category> categories = service.findAll(pageable);
        Page<Category> QCategories = service.findAllPageable(true, pageable);
                ...
    }

    //this doesn't work, as in this case "count" query has to be executed
    public void someTestMultiPage(){
        int page = 0;
        int size = 2;
        Pageable pageable = new PageRequest(page, size);
        Page<Category> categories = service.findAll(pageable);
    Page<Category> QCategories = service.findAllPageable(true, pageable);
        ...
    }
}

Tested same with latest build for SpringBoot/JPA/Spring (2.0.0/2.0.0/5.0.0). Now exception contains actual hql with problem and it doesn't have fetch in it :)

select count(category)
from Category category
where category.isActive = ?1
reda-alaoui commented 7 years ago

Thanks for your information.

The issue is fixed ;) I am going to perform a new release 1.10.12 containing the fix.

Klinton90 commented 7 years ago

thank you

dezet commented 5 years ago

I still have this issue with 2.1.x branch. Are you planning on updating it aswell?

reda-alaoui commented 5 years ago

Latest release 2.1.x should already contain the fix. If you still have an issue, please open a new issue.

tianlongJ commented 10 months ago

Latest release 2.1.x should already contain the fix. If you still have an issue, please open a new issue.

I reproduced this problem in version 2.2.4. I added nodes to the subgraph. There were four ManyToOne nodes. Two of them were added without problems, and the other two gave an error.