ExpressionKeyVisitor does not produce unique keys for anonymous types coming from different assemblies

[mkasperski]

The Linq.Visitors.ExpressionKeyVisitor.Visit() method does not guarantee a unique key generation for expressions that are based on compiler generated anonymous types of same shape, but coming from different assemblies.

Consider this code:

session.Query<MyEntity>().Select(e => new { e.Name }).ToList();

Upon compilation, the compiler will generate an anonymous type into the assembly where the query code is located. The type will be named something like (simplified) AnonymousTypeX`1[[System.String]] (where X is a sequential number within the assembly) and will be placed in global namespace (that is, will have no namespace at all). It will also be internal to that assembly.

Now, if the same query is also placed in code in different assembly within same app, the compiler will generate same-shaped anonymous type in that other assembly. If it happens that the sequential X in that assembly aligns with the X in the first assembly, both anonymous types will have the same resulting names. When inspected via System.Type.FullName they will be presented as exactly the same, because they both don't have any namespace to differentiate them. (N.B., this can also happen with user defined, same-named internal types that are in separate assemblies, but are declared in the same namespace).

Unfortunately, the ExpressionKeyVisitor uses .FullName when processing generic methods in its .VisitMethod(), like:

nhibernate-core/src/NHibernate/Linq/Visitors/ExpressionKeyVisitor.cs

Lines 235 to 244 in 49b998d

	private void VisitMethod(MethodInfo methodInfo)
	{
	_string.Append(methodInfo.Name);
	if (methodInfo.IsGenericMethod)
	{
	_string.Append('[');
	_string.Append(string.Join(",", methodInfo.GetGenericArguments().Select(a => a.FullName).ToArray()));
	_string.Append(']');
	}
	}

This would produce (simplified) ToList[AnonymousType42`1[[System.String]] (given X = 42) for both anon types. In effect this will cause the generated query plan cache keys to be the same for two essentially different types, and will rightfully blow in runtime.

The quick solution is to use .AssemblyQualifiedName here, which will properly distinguish both types (downside being slightly higher memory usage for cache keys, because of longer type identifiers).

I'm able to PR this change, along with tests - albeit a bit unorthodox ones, as they need to simulate internal anonymous types from two assemblies, but they do work.

[fredericDelaporte]

Thanks for your report. You are welcome to contribute a PR. You can find guidelines in contributing.

[bahusoid]

I think you need to add test with runtime exception you mentioned. As now you are simply testing changes that you've just made. It's not clear why it matters.

[fredericDelaporte]

It matters because his test demonstrates a cache key collision for two different entries.
A cache key collision does not forcibly result in an exception (although in the case of the query plan cache, it is very likely to cause one): it may "just" result in corrupted data.

Two different entries have to get different keys. For me, his test is enough.

[bahusoid]

Ok. It's just not clear to me what cache and when it's filled :) So I'm curious to see full setup required to reproduce the issue. But whatever...

[fredericDelaporte]

It is about the query plan cache, in the case of LINQ queries. Query plan are generated for queries (be it Criteria, HQL, LINQ, ...) and are cached in the session factory for avoiding re-generating them when the same queries (eventually with different parameters) are re-executed. The query plan cache, for LINQ queries, uses ExpressionKeyVisitor for generating the query key.