HHH-19096 Adjust SelectionQuery#setEntityGraph(..) to accept entity graphs of supertypes

[marko-bekhta]

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

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[mbladel]

Thanks @marko-bekhta! Would be great if we could add a test for this though, just to ensure that 1. setEntityGraph accepts graphs of subtypes of the queried entity and 2. said graphs work as expected.

[gavinking]

Are you sure this makes sense? Usually an EntityGraph for a supertype makes sense, not an EntityGraph for a subtype.

[marko-bekhta]

this felt like a more user-friendly way to apply that graph, but if you don't like it I'll drop it 😃

[gavinking]

I guess I'm not very keen on it because it pushes people toward using string-based names whereas we're actively trying to discourage them from using strings.

What we really want them to do is use Entity_._NamedGraph to get a typesafe reference to the named entity graph from the static metamodel. We prefer that they don't use these string-based lookups.

So, while I'm fine with adding one new operation I'm not sure we want to bloat out the very important SelectionQuery interface with operations which only streamline the untypesafe usage.

[marko-bekhta]

🤷🏻 😃 it's just when I was adding that test example:

SelectionQuery<Aa> query = s.createSelectionQuery( "from Aa", Aa.class );
query
    .setEntityGraph( 
        //  here:
        query.getNamedEntityGraph( "a-graph" ), GraphSemantic.FETCH )
    .getResultList();

above looks a bit odd to pass something to a query by getting it from the very same query.
IDK how often people generate the static metamodel in their projects, but if "this is the way" 😃 I'm ok with reverting this bit 😃 just let me know

[gavinking]

above looks a bit odd to pass something to a query by getting it from the very same query.

Yes I understand that.

IDK how often people generate the static metamodel in their projects, but if "this is the way"

It's the new way in JPA 3.2. We want to encourage them to use it.

[gavinking]

Why did you make these changes?

[marko-bekhta]

😖🙈 hmm, I thought I've reverted all of these (IDE made the change when I updated the method signature through "refactoring" 🤦🏻 ) will revert these

[gavinking]

@marko-bekhta a second option, which has some downsides, and which I'm not claiming is better, is to add the following method to QueryProducer:

	<R> Query<R> createQuery(String queryString, EntityGraph<R> resultClass);

This is a bit more consistent with EntityManager.find().

Here's what it looks like to use this method:

var query = session.createQuery( "from Client",
        session.getFactory().getNamedEntityGraphs( Client.class )
	        .get( "MyClientGraph" ) );
for ( Client client : query.getResultList() ) {
    String name = client.getName();
    ...
}

and according to an pre-existing proposal for JPA 4, this would simplify to:

var query = session.createQuery( "from Client",
        session.getNamedEntityGraphs( Client.class ).get( "MyClientGraph" ) );
for ( Client client : query.getResultList() ) {
    String name = client.getName();
    ...
}

which doesn't compare that unfavorably to what we have in this PR:

var query = session.createQuery( "from Client", Client.class );
query.setEntityGraph( query.getNamedEntityGraph( "MyClientGraph" ) );
for ( Client client : query.getResultList() ) {
    String name = client.getName();
    ...
}

[beikov]

How about we generate static fields for known entity graphs in the annotation processor and inject proxy implementations that disallow mutations? Then this could be as simple as this:

var query = session.createQuery( "from Client", Client_.MyClientGraph );

[marko-bekhta]

TLDR: let's merge the ? super R part 😃

---

I went back to see what Search needed, as it's been a while 😃. Entity graphs affect internals, API ( but indirectly) and tests.

For internals, we might be building either a criteria or hql queries, and all we really need there is this change (? super R):

Query<R> setEntityGraph(EntityGraph<? super R> graph, GraphSemantic semantic);

For API, we let the user get an "ORM query" from the search one:

SearchQuery<IndexedEntity> searchQuery = ...
Query<IndexedEntity> query = Search.toOrmQuery( searchQuery );

so the users could then do

query.setEntityGraph( ... )

any way they wish 😃 (not a Search problem anymore).

Tests: for Search tests, we aren't building the static metamodel, and I wouldn't want to start doing that. I can live with

query.setEntityGraph(
    session.callSomethingEvenIfItDoesntLookGreatJustToGetTheGraphIWant( IndexedEntity.class, IndexedEntity.GRAPH_LAZY ),
    GraphSemantic.LOAD
);

With that said, the other changes beyond the ? super R were only to make it easier for users to get the graphs they want, hence not my call to make what's best 😃

[gavinking]

How about we generate static fields for known entity graphs in the annotation processor and inject proxy implementations that disallow mutations?

We do already do that. It's a standard feature of JPA 3.2!

@marko-bekhta is worried about a different use case.

[gavinking]

FTR, this is what I have currently proposed for JPA 4:

https://github.com/jakartaee/persistence/pull/705/files

[gavinking]

On the other hand, thinking this through, there are some reasonably good arguments for adding an overload of createSelectionQuery(). (See Christian's code example.)

I went back to see what Search needed

@marko-bekhta When we're designing a public-facing API for ORM, we're not primarily thinking about what Search needs, OK?

[gavinking]

On the other hand, thinking this through, there are some reasonably good arguments for adding an overload of createSelectionQuery().

Actually I think whatever we do with this proposal, we should add that overload anyway.

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

[marko-bekhta]

I've updated this PR to only include the ? super R change.

[gavinking]

LGTM

[gavinking]

@marko-bekhta I would say you can just go ahead and merge this now

[marko-bekhta]

ok, thanks! 🥳