Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect results for pageable EntityGraph with spring data and spring-boot v3.0.1 #2744

Closed
arvgord opened this issue Dec 31, 2022 · 11 comments
Labels
for: external-project For an external project and not something we can fix

Comments

@arvgord
Copy link

arvgord commented Dec 31, 2022

Bug description

I created test application with custom repository with the help of spring boot v3.0.1 and org.springframework.boot:spring-boot-starter-data-jpa.
With findAll function in repository I want to load related entities with entity graph. When I call this method I get single SQL request to DB with offset and fetch at the end. It lead to getting wrong result from db because with entity graph I get a cartesian product fom DB as a result. For example I can get different totalElements result for same data in DB.

@Repository
interface ClientRepository : JpaRepository<ClientEntity, Long> {

    @EntityGraph(attributePaths = ["accounts", "deposits"])
    override fun findAll(pageable: Pageable): Page<ClientEntity>
}

Client entity class

@Entity
@Table(name = "CLIENT")
class ClientEntity(

    @Id
    @Column(name = "ID")
    var id: Long? = null,

    @Embedded
    var clientName: ClientName? = null,

    @Embedded
    var clientAddress: Address? = null,

    @OneToMany(mappedBy = "client")
    var accounts: Set<AccountEntity> = mutableSetOf(),

    @OneToMany(mappedBy = "client")
    var deposits: Set<DepositEntity> = mutableSetOf()
)

Expected behavior

Got that result with spring boot 2.7.7
r.r.b.c.g.n.v.BankDemoNProblemController : getClientList(page: 0, size: 3, extracting_strategy: EXTRACTING_STRATEGY_ENTITY_GRAPH)
WARN 94791 --- [atcher-worker-1] o.h.h.internal.ast.QueryTranslatorImpl : HHH000104: firstResult/maxResults specified with collection fetch; applying in memory!
Hibernate: select cliententi0_.id as id1_2_0_, deposits1_.id as id1_3_1_, accounts2_.id as id1_0_2_, cliententi0_.address_city as address_2_2_0_, cliententi0_.address_flat as address_3_2_0_, cliententi0_.address_house as address_4_2_0_, cliententi0_.address_street as address_5_2_0_, cliententi0_.client_first_name as client_f6_2_0_, cliententi0_.client_last_name as client_l7_2_0_, cliententi0_.client_middle_name as client_m8_2_0_, deposits1_.amount as amount2_3_1_, deposits1_.client_id as client_i4_3_1_, deposits1_.rate as rate3_3_1_, deposits1_.client_id as client_i4_3_0__, deposits1_.id as id1_3_0__, accounts2_.amount as amount2_0_2_, accounts2_.client_id as client_i4_0_2_, accounts2_.number as number3_0_2_, accounts2_.client_id as client_i4_0_1__, accounts2_.id as id1_0_1__ from client cliententi0_ left outer join deposit deposits1_ on cliententi0_.id=deposits1_.client_id left outer join account accounts2_ on cliententi0_.id=accounts2_.client_id
Hibernate: select count(cliententi0_.id) as col_0_0_ from client cliententi0_

You can see that SQLrequest presented above without offset ? rows fetch first ? rows only
As a result I get right total elements and content size for findAll(pageable: Pageable) with different pageable parameters.
test4
test3
You can see above the same total count for page 1 and 2 (for three clients in a database).
I got WARN 94791 --- [atcher-worker-1] o.h.h.internal.ast.QueryTranslatorImpl : HHH000104: firstResult/maxResults specified with collection fetch; applying in memory! it's not very efficient but it works

Actual behavior

Got that result with spring boot 3.0.1
r.r.b.c.g.n.v.BankDemoNProblemController : getClientList(page: 0, size: 3, extracting_strategy: EXTRACTING_STRATEGY_ENTITY_GRAPH)
Hibernate: select c1_0.id,a1_0.client_id,a1_0.id,a1_0.amount,a1_0.number,c1_0.address_city,c1_0.address_flat,c1_0.address_house,c1_0.address_street,c1_0.client_first_name,c1_0.client_last_name,c1_0.client_middle_name,d1_0.client_id,d1_0.id,d1_0.amount,d1_0.rate from client c1_0 left join account a1_0 on c1_0.id=a1_0.client_id left join deposit d1_0 on c1_0.id=d1_0.client_id offset ? rows fetch first ? rows only

You can see that SQLrequest presented above with offset ? rows fetch first ? rows only
As a result I get wrong total elements for findAll(pageable: Pageable) with different pageable parameters.
test2
test1
You can see different total count and wrong content size for page 1 and page 2 (for three clients in a database).

I use

Kotlin - 1.7.21
Spring-boot - 2.7.7 and 3.0.1
Spring dependency management - 1.1.0
PostgreSQL 14.5

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 31, 2022
@schauder
Copy link
Contributor

schauder commented Jan 2, 2023

That sounds like a Hibernate issue to me.

If you think this is a Spring Data JPA issue, please provide a reproducer that.

  1. demonstrates the issue present when using Spring Data JPA
  2. demonstrates the equivalent issue is not present when using JPA directly, i.e. via EntityManager.

@schauder schauder added for: external-project For an external project and not something we can fix status: waiting-for-feedback We need additional information before we can continue labels Jan 2, 2023
@nt-gt
Copy link

nt-gt commented Jan 6, 2023

Hi,

We tried to upgrade some of our Spring-Boot projects from 2.7 to 3.0 and noted the 3.0 version did not produce the correct output. It has the same symptoms as this issue but we were using a specification and not a graph entity, so it might be a different bug (if so, let me know and I will open a new issue for it).

We managed to reduce the size of our problem to a small reproducer that is available at https://github.com/dcsaorg/Spring-Bug-Reproducer (comparing 2.7 to 3.0). The specification is:

    private static Specification<RootEntity> withFilters(String withSubEntityName) {
        return (root, query, builder) -> {
            var predicates = new ArrayList<Predicate>();
            // Do the JOIN unconditionally to avoid hiding the difference when there is no filtering.
            var subEntityJoin = root.join("subEntities");
            if (withSubEntityName != null) {
                predicates.add(builder.equal(subEntityJoin.get("name"), withSubEntityName));
            }
            return builder.and(predicates.toArray(Predicate[]::new));
        };
    }

(The original example had a lot more complex filtering. A join across a @OneToMany is required to trigger the issue)

Let me know if you feel this is the same bug or a different. :)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 6, 2023
@schauder
Copy link
Contributor

schauder commented Jan 6, 2023

Please create a separate issue.

Also the comment from above applies:

That sounds like a Hibernate issue to me.

If you think this is a Spring Data JPA issue, please provide a reproducer that.

demonstrates the issue present when using Spring Data JPA
demonstrates the equivalent issue is not present when using JPA directly, i.e. via EntityManager.

Please remove everything not required in the reproducer that is not necessary to reproduce it. Especially any web related stuff.
Include a failing test for demonstrating the issue.
And finally, please include the database either as a in memory database (H2 for example) or if that is not possible using Testcontainers.

@schauder schauder added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 6, 2023
@tjuchniewicz
Copy link

We have the same problem as described by @arvgord. To make it more complex, it fails only on PostgreSQL we use to run our app. Unit tests based on H2 are fine! This means we should probably move focus to Hibernate.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 12, 2023
@schauder schauder added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 12, 2023
@arvgord
Copy link
Author

arvgord commented Jan 12, 2023

We have the same problem as described by @arvgord. To make it more complex, it fails only on PostgreSQL we use to run our app. Unit tests based on H2 are fine! This means we should probably move focus to Hibernate.

You're right. I didn't specify that I got that issue with PostgreSQL.

demonstrates the equivalent issue is not present when using JPA directly, i.e. via EntityManager.

I reprodused that issue with entityManager:

@Entity
@Table(name = "CLIENT")
@NamedEntityGraph(
    name = "ClientEntityTest",
    attributeNodes = [NamedAttributeNode("accounts"), NamedAttributeNode("deposits")]
)
class ClientEntity(

    @Id
    @Column(name = "ID")
    var id: Long? = null,

    @Embedded
    var clientName: ClientName? = null,

    @Embedded
    var clientAddress: Address? = null,

    @OneToMany(mappedBy = "client")
    var accounts: Set<AccountEntity> = mutableSetOf(),

    @OneToMany(mappedBy = "client")
    var deposits: Set<DepositEntity> = mutableSetOf()
)
val graph = entityManager.createEntityGraph("ClientEntityTest")
val result = entityManager.createQuery("select c from ClientEntity c", ClientEntity::class.java)
    .setFirstResult(0)
    .setMaxResults(2)
    .setHint("jakarta.persistence.loadgraph", graph)
    .resultList

@tjuchniewicz I created issue for Hibernate
https://hibernate.atlassian.net/browse/HHH-15964

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 12, 2023
@arvgord
Copy link
Author

arvgord commented Jan 12, 2023

I created application to reproduce that issue without spring.
You can run the IssueTest and see the results.
https://github.com/arvgord/hibernate-cartesian-issue
I also found that it doesn't work for MySQL too.
In branch hibernate-core_5.6.14.Final with org.hibernate:hibernate-core:5.6.14.Final dependency everything works fine.

@schauder
Copy link
Contributor

Thanks for confirming that this is a Hibernate issue.

@schauder schauder closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2023
@schauder schauder removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jan 13, 2023
@vlsi
Copy link

vlsi commented Feb 19, 2023

@schauder , what if I re-state the issue as follows? spring-data-jpa is prone to errors when using @EntityGraph and Pageable at the same time.

I guess it has been duplicated in #2794

@EntityGraph is something recommended to improve performance when fetching data.
Pageable is something recommended when the amount of data is big.

Apparently, users might want combining both things together.
It turns out that both Hibernate 5 and Hibernate 6 do not implement that query efficiently.

The comment from Hibernate team is "they would implement in-memory paging": https://hibernate.atlassian.net/browse/HHH-15964?focusedCommentId=110593

I believe it is unfair from spring-data-jpa to close the issue.

What do you think if spring-data-jpa split the query in two:

  1. Issue the first request to Hibernate to fetch ids of the entities with the proper paging, however, skip entity graph for the request to prevent the join
  2. Load entities with entity graph hints

Even though that might be suboptimal from the performance point of view, it should be relatively easy to implement on spring-data-jpa side.

A more sophisticated solution might be improving Hibernate so it implements those types of entity queries efficiently.

@schauder
Copy link
Contributor

@vlsi If you frame it this way it would be indeed a feature request. Feel free to create new issue.

@vlsi
Copy link

vlsi commented Feb 20, 2023

@mp911de , would you please reopen this issue then?

@schauder
Copy link
Contributor

The close reason still holds.

And we also discussed the perspective @vlsi and decided that while, it might be possible in theory, we don't want to spend the resources to work around limitations of the JPA spec or bugs in the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix
Projects
None yet
Development

No branches or pull requests

6 participants