HHH-14097 Fix redundant SQLs issue for fetch entity graph

[NathanQingyangXu]

https://hibernate.atlassian.net/browse/HHH-14097

The root cause is we lack fetch entity graph validation. For the initial multiple hydrated objects, only the first one is applicable to root entity graph.

Imported andrea's testing code from #3452. Tons of thanks to him!

Remember only the testing case is needed to be merged into v6 for entity graph implementation is incompatible between v5 and v6 and the code changes except the testing case are not needed in v6 (actually I've carefully deleted the v5 implementation in v6 previously).

[Sanne]

I'm not sure about this, are we entirely sure that the fetch graph is no longer needed at this point?
Wouldn't it be possible for the fetch grapth to need to span multiple load events?

IF this is correct, should line 285 be removed as it seems redundant?

[NathanQingyangXu]

For non-toplevel traversal, Line #285 will take effect to backtrack to upper level after the above statement. The above statement will impact only top level (I made it explicitly by including "on top level" in the comments above). I admit the code change is difficult to understand for basically 'fetch graph' implementing is sort of based on hack (existing v5 infrastructure makes it difficult to implement elegantly), but v6 makes the implementation much easier and more elegant (it has well-designed context info accessible). After this PR gets merged, I'll port it into v6 to see whether the issue still remains.

[NathanQingyangXu]

I doubled checked and found the backtracking is working as expected (without it our graph testing cases would fail). Could you take a new look at this PR? Thanks.

[dreab8]

Hi @NathanQingyangXu still not sure we really need session.setFetchGraphLoadContext( null ); as discussed privately I tried to add the kind of test you think may need this call

@Test
	public void testQueryByIdWithLoadGraph3() {
		Statistics statistics = entityManagerFactory().unwrap( SessionFactory.class ).getStatistics();
		statistics.clear();
		doInJPA(
				this::entityManagerFactory, entityManager -> {
					EntityGraph<AEntity> entityGraph = entityManager.createEntityGraph( AEntity.class );
					entityGraph.addAttributeNodes( "cList" );
					entityGraph.addSubgraph( "cList" ).addAttributeNodes( "a" );

					TypedQuery<AEntity> query = entityManager.createQuery(
							"select a from AEntity as a where a.id = :aid ",
							AEntity.class
					);
					query.setHint( GraphSemantic.LOAD.getJpaHintName(), entityGraph );
					query.setParameter( "aid", 1 );

					AEntity cEntity = query.getSingleResult();

					assertTrue( Hibernate.isInitialized( cEntity.getcList() ) );
					assertTrue( Hibernate.isInitialized( cEntity.getcList().get( 0 ) ) );

					assertEquals( 1L, statistics.getPrepareStatementCount() );
				} );
	}

if the test is correct, it passes with just

if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
			LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
			fetchGraphContext = null;
}

[NathanQingyangXu]

Hi @NathanQingyangXu still not sure we really need session.setFetchGraphLoadContext( null ); as discussed privately I tried to add the kind of test you think may need this call

@Test
	public void testQueryByIdWithLoadGraph3() {
		Statistics statistics = entityManagerFactory().unwrap( SessionFactory.class ).getStatistics();
		statistics.clear();
		doInJPA(
				this::entityManagerFactory, entityManager -> {
					EntityGraph<AEntity> entityGraph = entityManager.createEntityGraph( AEntity.class );
					entityGraph.addAttributeNodes( "cList" );
					entityGraph.addSubgraph( "cList" ).addAttributeNodes( "a" );

					TypedQuery<AEntity> query = entityManager.createQuery(
							"select a from AEntity as a where a.id = :aid ",
							AEntity.class
					);
					query.setHint( GraphSemantic.LOAD.getJpaHintName(), entityGraph );
					query.setParameter( "aid", 1 );

					AEntity cEntity = query.getSingleResult();

					assertTrue( Hibernate.isInitialized( cEntity.getcList() ) );
					assertTrue( Hibernate.isInitialized( cEntity.getcList().get( 0 ) ) );

					assertEquals( 1L, statistics.getPrepareStatementCount() );
				} );
	}

if the test is correct, it passes with just

if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
			LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
			fetchGraphContext = null;
}

The reason your test didn't expose issue might be that cList will fail in type testing, ending up with context being set to null already. I only realized my original proposal won't work when I created my own.
I added a new testing case to showcase why session.setFetchGraphLoadContext( null ) is needed. It is hard to explain and even confusing, but it ensures that entity graph is disabled for scenario wherein it is not applicable.

[dreab8]

Thanks @NathanQingyangXu for the additional test, may be I'm missing something but I tried it with just

GraphImplementor fetchGraphContext = session.getFetchGraphLoadContext();
		if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
			LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
			fetchGraphContext = null;

		}

and it passes.

[NathanQingyangXu]

Thanks @NathanQingyangXu for the additional test, may be I'm missing something but I tried it with just

GraphImplementor fetchGraphContext = session.getFetchGraphLoadContext();
		if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
			LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
			fetchGraphContext = null;

		}

and it passes.

Sorry for my premature pushing. Now I updated the testing case and I am confident it should be fine.

[NathanQingyangXu]

The above content was copied from v6's counterpart. Thought the basic explanation of the two types of entity graph modes is supposed to exist in v5 as well.
Given the content has existed in v6 already, we don't need to bother merging this part to v6 as well.

[dreab8]

just a minor but I think the session.setFetchGraphLoadContext( null ); is redundant

[NathanQingyangXu]

I think it is necessary because without setting it, the downstream will still try to locate some property as per the context grabbed from session.getFetchGraphLoadContext(), not from the local variable of fetchGraphContext. See the org.hibernate.engine.internal.TwoPhaseLoad#isEagerFetchGraph(SharedSessionContractImplementor session, String associationName, Type associationType) for details.

I am sorry the current fetch entity graph implementation is so difficult to understand (and maintain). Luckily its status is improved much more in v6.

[Sanne]

thanks! Merging