NH-4078 - LINQ fetched collections aren't cached

[nhibernate-bot]

Tahir K Ahmadov created an issue — 12th September 2017, 15:20:16:

Config:

var automappingConfiguration = new AutomappingConfiguration();
return Fluently.Configure()
    .Database(MsSqlConfiguration.MsSql2012.ConnectionString(
        ConfigurationManager.ConnectionStrings<"Main">.ConnectionString))
    .Cache(c => c.UseQueryCache().UseSecondLevelCache().ProviderClass<SysCacheProvider>())
    .Mappings(m =>
        m.AutoMappings.Add(
            AutoMap.AssemblyOf<User>(automappingConfiguration)
                .IgnoreBase<EntityBase>()
                .Conventions
                .Setup(c =>
                {
                    c.Add(ForeignKey.Format((member, type) =>
                    {
                        // standard foreign key, straight forward
                        if (member != null)
                            return member.Name <ins> "Id";

                        // member is null, because it's trying to find the "reverse" column name for a collection
                        // it doesn't find the "reverse" property when it exists, so we find it ourselves
                        var properties = type.GetProperties();
                        var collectionProperties = properties.Where(p => p.PropertyType.IsGenericType && new[] { typeof(ISet<>), typeof(IList<>) }.Contains(p.PropertyType.GetGenericTypeDefinition()));
                        // unfortunately, we don't know which collection property it is and have to guess and go with the first one
                        var firstCollectionProperty = collectionProperties.First();
                        var collectionItem = firstCollectionProperty.PropertyType.GetGenericArguments()<0>;
                        var reversePropertyInfo = collectionItem.GetProperties().First(p => p.PropertyType == type);
                        return reversePropertyInfo.Name </ins> "Id";
                    }));
                    c.Add<EnumConvention>();
                    c.Add(Cache.Is(ci => ci.ReadWrite()));
                })
            )
    )
    .BuildSessionFactory();

Query:

session.Query<User>()
    .FetchMany(u => u.DataSources).ThenFetchMany(uds => uds.ParameterLimits).ThenFetchMany(udsp => udsp.Options).ThenFetch(udspo => udspo.Option)
    .FetchMany(u => u.DataSources).ThenFetchMany(uds => uds.ParameterLastValues).ThenFetchMany(udsplv => udsplv.Options).ThenFetch(o => o.Option)
    .Where(u => u.Email == email).ToList().First()

Cold cache - correct operation (almost: I have to run around and dedupe some collections, and First() doesn't work - it limits all collections to 1 item only, too - have to do .Where(...).ToList().First(); perhaps I should create another bug for these).

Warm cache - user is retrieved from cache, but then all other collections are retrieved as if lazy loaded:

exec sp*executesql N'SELECT datasource0_.UserId as UserId2_2_1_, datasource0_.Id as Id1_2_1_, datasource0_.Id as Id1_2_0_, datasource0_.UserId as UserId2_2_0_, datasource0_.DataSourceId as DataSo3_2_0_, datasource0_.LastGraphModuleId as LastGr4_2_0_, datasource0_.LastGridModuleId as LastGr5_2_0_ FROM <UserDataSource> datasource0_ WHERE datasource0*.UserId=@p0',N'@p0 int',@p0=1
exec sp*executesql N'SELECT parameterl0_.UserDataSourceId as UserDa2_5_1_, parameterl0_.Id as Id1_5_1_, parameterl0_.Id as Id1_5_0_, parameterl0_.UserDataSourceId as UserDa2_5_0_, parameterl0_.DataSourceParameterId as DataSo3_5_0_ FROM [UserDataSourceParameterLimit] parameterl0_ WHERE parameterl0*.UserDataSourceId=@p0',N'@p0 int',@p0=1

---

Frédéric Delaporte added a comment — 12th September 2017, 16:48:43:

(almost: I have to run around and dedupe some collections, and First() doesn't work - it limits all collections to 1 item only, too - have to do .Where(...).ToList().First(); perhaps I should create another bug for these)

Cartesian products occur when fetching multiple collections (many FetchMany): do not fetch multiple collections. This is an old known limitation of fetching with NHibernate.

First applies to the whole query and as such does not actually support fetching collections.

Collection fetching support in NHibernate is far from some other ORM (which do instead generate quite complex queries for supporting it better, sometime at the expense of generating badly performing queries). But I suspect this is partly because NHibernate provides other efficient ways of avoiding N+1 fetching issues. You may read my StackOverflow answer on those other ways here.

Regarding the bug you report, please provide a simple test case demonstrating this. Are you sure your current auto-mapping correctly flag each collection and each involved entity as cacheable? (I am not used to auto-mapping.)

It looks to me it is well likely to be a "work as intended" case. Eager fetching do not use cache. Eager fetching always fetch. Eager fetching can not guess if the collection it will have to fetch are already sitting in the cache or not. It needs to have retrieved the queried root entities list first, but that is in contradiction with its contract, which is to retrieve the fetched collections/entities altogether. Added to the fact that it has currently limitations such as duplicating results in case of multiple collection fetching, I would not be surprised if putting its result to cache for fetched collections was disabled. (I would have to check that point.)

Switching to minor.

---

Tahir K Ahmadov added a comment — 12th September 2017, 17:11:26:

Regarding the .First() limitation, I think there is quite a big design flaw: instead of supporting it properly (which may or may not be desirable from performance perspective as you mentioned) OR refusing to execute it with an exception, NH just returns some result which is incorrect. The standard here is how would an IEnumerable.First() call work; obviously NH's First() behavior is different, therefore wrong. It should just throw and exception and explain to the user that it's not supported and why/etc.
Similarly, when I'm using the .Where().ToList().First() workaround, collections getting multiple items is incorrect behavior; if NH cannot fetch multiple collections in one LINQ query, it should throw an exception, not quietly produce incorrect results.
Regarding my bug, yes all entities are cached (because I don't have any special logic for User class as opposed to all of the other classes). I'll try to put together a test case sometime today or tomorrow.
Overall, unfortunately it seems that the collection and caching features are a long way from being production ready. I don't want to come off as being too critical but I downloaded NH with the expectation that a lot of these scenarios have been worked out over the 10+ years of its existence, yet we're encountering big limitations in very commonly used, basic features, such as collections and caching.

---

Frédéric Delaporte added a comment — 12th September 2017, 17:53:47:

No, Linq-to-Object is not a standard. Better not assume other Linq-to-xxx will behave like Linq-to-object. Anyway the First case is already reported as NH-3295 which is somewhat almost a dup of NH-3204, which just confirms that current NHibernate does not support mixing paging and fetching collections. (First is just a special case of paging.)

About your rant on NH, as I have tried to explain you, caching and fetching does not play well together. This is a bit contradictory to mix them. Normal use case is caching with lazy-loading. Are you experiencing issues when using caching and lazy-loading? (You may, since there is a number of errors a user can do. Such as not using transactions: cache is disabled when not using transaction.) But this is not the right place here for discussing all that. If you need support with caching, there is the nh-users group or nhibernate.info and the reference documentation, or StackOverflow. If you want to discuss design, there is the nh-development group.