NH-2016 - Fix joining to association twice using criteria

[gmalyshev]

Joining to association twice using criteria.

I'm added criteria alias for unique identification join statement. This makes it possible to declare complex expressions join.

Deviations in the standard behavior of the rest of the functionality I hadn't noticed

Jira issue NH-3472 (dup of NH-2016, #990).

Fixes #990

[hazzik]

Can you please enable Allow edits from maintainers PR option?

[gmalyshev]

Done

[fredericDelaporte]

Rebased, async code generated, and cleaned up. Especially I have made some renaming to improve code readability.

The change does the job, but maybe it could be done in a simpler manner. @bahusoid, you are more used than me to the criteria code, may you have a look?

[bahusoid]

Will be able to check code during next week.

But it seems those changes might lead to unexpected behavior if query is mixed with Fetch and JoinAlias/JoinQueryOver calls for the same properties. I mean something like this:

Child child = null;
session.QueryOver<Obj>
.Fetch(o => o.Child).Eager
.Fetch(o => o.Child.SubChild).Eager

.JoinAlias(o => o.Child, () => child)
.Where(x => child.IsActive == true);

It is my understanding the query above after this change will start to generate two joins to Child table. Am I right?

Also after this change it seems query can contain multiple joins to fetch the same collection persister (with added multiple left join aliases for the same collection). I wonder how it will be handled.

P.S. Actually the desired behavior is already possible via "Entity Join" - JoinEntityAlias/JoinEntityQueryOver (but yeah you need to specify explicitly full join condition).

[fredericDelaporte]

It is my understanding the query above after this change will start to generate two joins to Child table. Am I right?

No I do not think so. And testing it, it does not, see the new commit.

Also after this change it seems query can contain multiple joins to fetch the same collection persister (with added multiple left join aliases for the same collection). I wonder how it will be handled.

I think this will somewhat "union" the fetches, with de-duplication in case of a set, an error in case of a bag (Cannot simultaneously fetch multiple bags). But that is a somewhat weird use case, I am not sure we should specify the expected output in such case. Normally it could always be rewritten for using a single alias with a more complex restriction. Should really a test for it be added? So I have set the test as Explicit only.

[bahusoid]

Additional unexpected join was my main concern regarding this PR - if it's not a problem then it looks good to me. It can be useful.

Regarding "Entity Join" changes - they seem unrelated to this PR. Am I wrong?

And there is another catch - only one fetch mode (SelectMode) can be applied for such joins. Otherwise it seems alias support needs to be added for CriteriaImpl.GetSelectMode. Not sure if it's worth it.

[fredericDelaporte]

Regarding "Entity Join" changes - they seem unrelated to this PR. Am I wrong?

EntityJoinInfo had to change to avoid casting ICriteria to Subcriteria while it is currently guaranteed to always have only Subcriteria in it. (Path property is only available on Subcriteria, not on ICriteria). Moreover the class was publicly exposed without any need for this. So changing the property would have been a breaking change. So I have just fully obsoleted it and replaced by a new, internal one.
Or are you referring to something else?

And there is another catch - only one fetch mode (SelectMode) can be applied for such joins. Otherwise it seems alias support needs to be added for CriteriaImpl.GetSelectMode. Not sure if it's worth it.

True, but well, I do not think it would be worth it. This will be surely a bit complex for what looks to me as corner cases. (Fetching with a filter a collection while also projecting another part of it somehow? Maybe better load it fully and then process it in memory then.)

[bahusoid]

EntityJoinInfo had to change to avoid casting ICriteria to Subcriteria

I see no such places related to this PR. It seems this change was done only to remove the following line:

nhibernate-core/src/NHibernate/Loader/Criteria/CriteriaJoinWalker.cs

Line 83 in 8649f9a

var criteriaPath = entityJoinInfo.Criteria.Alias; //path for entity join is equal to alias

If you want to make such refactoring maybe it's better to do as another pr to better see actual changes for this PR.

[fredericDelaporte]

The cast is not there thanks to the change. Try compiling this line with an ICriteria Criteria instead of a Subcriteria:

nhibernate-core/src/NHibernate/Loader/Criteria/CriteriaJoinWalker.cs

Line 83 in b6ede57

var criteriaPath = entityJoinInfo.Criteria.Path;

It will fail.

[fredericDelaporte]

nhibernate-core/src/NHibernate/Loader/Criteria/CriteriaJoinWalker.cs

Line 83 in 8649f9a

var criteriaPath = entityJoinInfo.Criteria.Alias; //path for entity join is equal to alias

This line has not been removed. It has been changed to the correct call, required for robustly supporting the change since the Alias may be something else than the Path.

That is in the "Fix compilation after rebase" commit. It was not compiling after the rebase, and this code had to change anyway. I have not checked that the change was also causing this place to potentially have an Alias not equal to the Path, but changing translator.GetJoinType(.Alias) to translator.GetJoinType(.Alias, .Alias) instead of translator.GetJoinType(.Path, .Alias) was just looking wrong, especially since the change is about allowing different aliases for the same path.

So this entity join change is not an unrelated refactoring, it is a legit change for this PR.

[bahusoid]

required for robustly supporting the change since the Alias may be something else than the Path.

Alias == Path is "Entity Join" specific. Which is not affected by this PR. This PR is about ability to create multiple aliases for the same association path. It makes no sense for "entity join" as there is no association path. You still can't create "entity join" where Alias != Path. But seeing those changes here made me think that "entity join" somehow affected.

So I still think that all you need to do in this PR is add additional var entityAlias = entityJoinInfo.Criteria.Alias variable and use it. And if you want refactor "entity join" code to eliminate "dependency on internal details" it's better to do as separate PR (or left it here just as separate commit)

[bahusoid]

So just to be clear I think that EntityJoinInfo -> EntityJoinInformation refactoring is unrelated to this PR. See suggested clean up (feel free to drop it if you are not convinced) - but I think this change gives the wrong idea of the scope of this PR.

[fredericDelaporte]

I find this a bit overstated, but well, so be it.