Skip to content

Conversation

@mbellade
Copy link
Member

@mbellade mbellade commented Nov 3, 2025

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


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.


@beikov
Copy link
Member

beikov commented Nov 3, 2025

Is there a reason for changing join types of existing tests?

@mbellade
Copy link
Member Author

mbellade commented Nov 4, 2025

Is there a reason for changing join types of existing tests?

Yes: the two tests I had to change both followed the same pattern:

from Root r
left join fetch r.mid
left join fetch r.mid.child

With my change, the second left join fetch r.mid.* now produced an inner join for the implicit path r.mid, thus triggering an owner of the fetched association was not present in the select list error.

@beikov
Copy link
Member

beikov commented Nov 4, 2025

I think we should discuss this during our next meeting.

With my change, the second left join fetch r.mid.* now produced an inner join for the implicit path r.mid, thus triggering an owner of the fetched association was not present in the select list error.

Shouldn't it rather produce a IllegalStateException in SqmUtil#findCompatibleFetchJoin in this case?

Even though this interpretation (implicit => inner join) is correct according to the JPA spec section 4.4.4, I think it might be desirable to allow reusing unaliased left join fetches for implicit joined paths, because:

  • Such unaliased fetch joins can't have an ON clause and are hence a safe substitute for implicit joins in principle
  • If we don't reuse the join fetch, there would be no "JPA spec compliant" way to join fetch nested optional associations

I understand that "JPA spec compliant" is a stretch, since section 4.4.4 explicitly mentions the semantics:

Path expression navigability is composed using “inner join” semantics. That is, if the value of a non-terminal field in the path expression is null, the path is considered to have no value, and does not participate in the determination of the result.

I'm not advocating for one way or another, but we have to keep in mind that this is a change in behavior.

@mbellade
Copy link
Member Author

mbellade commented Nov 4, 2025

Shouldn't it rather produce a IllegalStateException in SqmUtil#findCompatibleFetchJoin in this case?

It's not doing that, because being a nested path isTerminal is false, so it actually doesn't look through fetched joins but rather all the lhs' compatible joins. We can change and check fetched first so that we produce this error instead, which might be more correct.

If we don't reuse the join fetch, there would be no "JPA spec compliant" way to join fetch nested optional associations

The one way you could do it now would be avoid using implicit joins (i.e. nested paths with . separator), and instead use explicit left outer fetch joins:

from Root r
left join fetch r.mid m
left join fetch m.child

But I do agree it is a change in behavior, though the ParsingException we're producing now in 7.1 should be fixed IMO.

@beikov
Copy link
Member

beikov commented Nov 4, 2025

Shouldn't it rather produce a IllegalStateException in SqmUtil#findCompatibleFetchJoin in this case?

It's not doing that, because being a nested path isTerminal is false, so it actually doesn't look through fetched joins but rather all the lhs' compatible joins. We can change and check fetched first so that we produce this error instead, which might be more correct.

Yeah, maybe.

The one way you could do it now would be avoid using implicit joins (i.e. nested paths with . separator), and instead use explicit left outer fetch joins:

from Root r
left join fetch r.mid m
left join fetch m.child

Unfortunately, aliasing fetch joins is forbidden in JPQL.

But I do agree it is a change in behavior, though the ParsingException we're producing now in 7.1 should be fixed IMO.

Totally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants