Fix Criteria caching filtered collections

[zalewskil]

When a cacheable criteria, or query-over, eager-loads a collection with restrictions, causing the collection to be partially loaded, the collection is nonetheless put in the second level cache. This causes lazy-loads of the collection done by other sessions to retrieve an invalid state for the collection.

This is a partial fix of #1341, for QueryOver and Criteria.

[fredericDelaporte]

This sounds good in principle. But binary breaking changes have to be avoided, and there is a bunch of formatting issues.

[fredericDelaporte]

As this is a partial fix of #1341, I have re-tagged it as a stand-alone issue and I have changed the description for avoiding GitHub auto-closing #1341.

[bahusoid]

This place looks hacky to me:

• We already have place where we collect all query collection persisters (see CreateCriteriaCollectionPersisters). I think it's a good place to collect this skip cache collection flags. And maybe collect them similarly tocriteriaCollectionPersisters in uncacheableCollectionPersisters
• We should not rely on Left join anymore as now we support fetching collections with inner join too (with SelectMode.Fetch).

[IwanowM]

It seems to me, ShouldFetchCollectionPersister() is exactly what we need to determine whether collection will be fetched or not.
From other side, we can map HasRestrictions flags from subcriteries to assotiations, to determine if persister should be cached or not.
That is why I have moved initialization of uncacheableCollectionPersisters to CriteriaJoinWalker.
Is it ok for you?

[bahusoid]

By "hacky" I meant we shouldn't use "magic strings" like $"{rootCriteria.EntityOrClassName}.{subcriteria.Path}". Will it work for multilevel fetch like obj.Child.Collection (it seems we need a test for this case, maybe it will work but it's not obvious to me)?

So instead I think it's better to collect directly all collection persisters ICollectionPersister with restrictions. That's why I suggested to look in to CreateCriteriaCollectionPersisters as that's the place where actual collection persisters are enumerated (though I'm not saying it's the only and best place to do the job)

To me this task looks like the following:

1. In translator (or walker) we need to find which collection persisters have restrictions;
2. And for loader we need to map "restricted" collection perstisters with persisters in collectionPerstisters and prepare UncachedCollectionPersisters

So to me it looks like we don't need to bother "to determine whether collection will be fetched or not" - as long as we can obtain ICollectionPersister for given criteria. And I don't see why OuterJoinableAssociation needs to be modified - it seems all information can be obtained from translator/walker.

[IwanowM]

Ok, if I understood well, we shouldn't add collection into cache if:

1. There are restrictions and subcriteria.JoinType == JoinType.LeftOuterJoin
2. There are restrictions and subcriteria.JoinType == JoinType.InnerJoin and SelectMode == SelectMode.Fetch, right?

And it wasn't clear for me how to get selection mode for subcriteria, but it seems that I can get it in this way - rootCriteria.GetSelectMode(subcriteria.Path), it isn't?

[IwanowM]

Could you have a look at my last commit, please? Is it right direction?

[bahusoid]

Yep. But as I already said I think that logic that checks Join types and SelectMode is unnecessary here. No need to duplicate code - those checks will be executed by the routine that prepares collection persisters to fetch.

I've refactored and made a few clean ups to it (see my latest commit). Let me know if you disagree or see any issues.

[hazzik]

I don't quite agree with this check because this obfuscates things by making stuff dependant on a index. Although it looks as an optimization it produces a bigger array.

[bahusoid]

Actually I didn't mean it as optimization but merely to make it processed the same way as others properties (see CollectionAliases, CollectionOwners, CollectionPersisters,...). And that's collection fetches - it seems big arrays should not be an issue.

[hazzik]

@bahusoid I've done with my changes

[bahusoid]

lgtm

@hazzik I disagree that index based checks obfuscate things - it's pretty standard checks in Loader. Though have no objections with your version.

[fredericDelaporte]

LinkedHashMap guarantees the dictionary elements will be iterated in the same order they were added. This is not the case of Dictionary, which, with current implementations, will have a different iteration order as soon as some elements were removed.

So this change is not neutral. But it looks like preserving the order has no purpose here, so this is ok. (Similarly associationPathJoinTypesMap does not need to be a LinkedHashMap.)