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

SubSelectFetchTest fails #1502

Closed
DavideD opened this issue Mar 13, 2023 · 8 comments · Fixed by #1746
Closed

SubSelectFetchTest fails #1502

DavideD opened this issue Mar 13, 2023 · 8 comments · Fixed by #1746
Assignees
Labels
regression Something that was working before. A test that we have disabled waiting We are waiting for another PR or issue to be solved before merging this one
Milestone

Comments

@DavideD
Copy link
Member

DavideD commented Mar 13, 2023

It only load the fetched collection and not all the associations that are part of the entity.

See

@Ignore // see https://github.com/hibernate/hibernate-reactive/issues/1502

DavideD added a commit that referenced this issue Mar 13, 2023
It only fetches one association and not all of them as expected
@DavideD DavideD added regression Something that was working before. A test that we have disabled and removed Hibernate Reactive 2.0 labels Apr 5, 2023
@DavideD DavideD added this to the next milestone Jun 15, 2023
@blafond blafond assigned blafond and unassigned blafond Aug 2, 2023
@blafond
Copy link
Member

blafond commented Aug 3, 2023

So I've tested reactive 1.1 version of SubselectFetchTest and as well as a ORM_SubselectFetchTest version and the results for session.createQuery( "from Node n order by id", Node.class ).getResultList() on both return a result set with 2 entities and neither are Proxy's.

In our current reactive I ran the equivalent tests and they both return one concrete entity (Node) and one that is a HibernateProxy object.

Was this change expected?

@blafond
Copy link
Member

blafond commented Aug 3, 2023

So I created/ran .. basically, our reactive test over in ORM test suite. The second result in the result list is a HibernateProxy too, so something did change in ORM.

Note that in our test, I can make it work by calling fetch( n2.getElements() instead of the original fetch( n1.getElements()

@blafond
Copy link
Member

blafond commented Aug 15, 2023

Appears that the generated select + subselect query we used in HR 1.1 is not getting generated correctly in HR 2.x which doesn't load the collections in the primary/parent node.

see: SqlClientConnection.selectQuery(..)

OLD SQL : with paramValues array is EMPTY)

select 
	elements0_.node_id as node_id2_0_1_, 
	elements0_.id as id1_0_1_, 
	elements0_.id as id1_0_0_, 
	elements0_.node_id as node_id2_0_0_ 
from Element elements0_ 
        where elements0_.node_id in (select subselectf0_.id from Node subselectf0_ )

NEW SQL: with paramValues array has one value of '1')

select 
	e1_0.node_id,
	e1_0.id
from Element e1_0 
	where e1_0.node_id=$1

If I intercept the generated SQL and replace the string with the following and remove the '1' parameter, then this test succeeds.

select 
	e1_0.node_id as node_id2_0_1_, 
	e1_0.id as id1_0_1_, 
	e1_0.id as id1_0_0_, 
	e1_0.node_id as node_id2_0_0_ 
from Element e1_0 
        where e1_0.node_id in (select subselectf0_.id from Node subselectf0_ )

So there's a step or 2 missing in the 'select' sql generation for this use-case.

@blafond
Copy link
Member

blafond commented Aug 15, 2023

@davide In our 6.2.7 ORM version, the SubselectLoader isn't being set.

Looks like there was some work done on the SubselectLoader logic in AbstractCollectionPersister.resolveSubselectLoader() in the main branch that may have fixed this.

@DavideD
Copy link
Member Author

DavideD commented Aug 15, 2023

This test has never worked since we upgraded to ORM 6.
Probably, I haven't changed something correctly, or I haven't implemented something that's needed for this use case.

In Hibernate Reactive we extend the subclasses of AbstractCollectionPersister and override createSubSelectLoader so that we can plug in our reactive loaders.

I think you should check which implementation of AbstractCollectionPersister gets called in Hibernate ORM and check if the reactive equivalent gets called when using Hibernate Reactive. If it doesn't, you can investigate where they diverge and propose a fix.

What we know at the moment is that this works in Hibernate ORM (we already tested this in the past), and that the test we have in Hibernate Reactive is correct (it was working with ORM 5 and shouldn't require any changes to fix this issue).

@blafond blafond added the waiting We are waiting for another PR or issue to be solved before merging this one label Aug 24, 2023
@blafond
Copy link
Member

blafond commented Aug 29, 2023

PR merged in ORM's 6.2 branch

@blafond blafond closed this as completed Aug 29, 2023
@DavideD
Copy link
Member Author

DavideD commented Aug 30, 2023

I'm going to reopen this until we upgrade to the next ORM 6.2.
So that we don't forget to re-enable our test.

But there is no more work needed from our side to fix this issue

@DavideD DavideD reopened this Aug 30, 2023
@DavideD
Copy link
Member Author

DavideD commented Sep 1, 2023

This issue has been fixed for Hibernate Reactive 2.0, but it won't work in 2.1
I will create a separate issue

DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Sep 1, 2023
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Sep 1, 2023
DavideD pushed a commit to DavideD/hibernate-reactive that referenced this issue Sep 1, 2023
The upgrade to Hibernate ORM 6.2.8.Final fixes this issue.
See https://hibernate.atlassian.net/browse/HHH-17130
DavideD pushed a commit that referenced this issue Sep 1, 2023
The upgrade to Hibernate ORM 6.2.8.Final fixes this issue.
See https://hibernate.atlassian.net/browse/HHH-17130
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Sep 1, 2023
This reverts commit fc23d87.

Something has changed in Hibrnate ORM 6.3 and the previous
fix is not enough anymore
DavideD added a commit that referenced this issue Sep 1, 2023
This reverts commit fc23d87.

Something has changed in Hibrnate ORM 6.3 and the previous
fix is not enough anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something that was working before. A test that we have disabled waiting We are waiting for another PR or issue to be solved before merging this one
Projects
None yet
2 participants