PocoData.ForType() caching

[asherber]

When I call PocoData.ForType(type, mapper) for the first time, the resulting PocoData object is stored in the cache keyed only by the type. This means that if I later call PocoData.ForType(type, otherMapper), I get back the initial PD object, which was created with a different mapper.

This is probably something of an edge case, but imagine a situation where a single type is used for queries in two different databases, each with its own mapper. The first database to use the class will cache its PD, and then both databases will always get that object.

I think it would be better if the cache had a composite key of type and mapper type. (Even then, it's possible for an instance of a mapper to have its properties changed between uses, but I would think that's even rarer.)

I'm happy to work on a PR for this.

[pleb]

Nice find. Seems like a logical bug.

[Curlack]

I have a similar issue where even if you use the same mapper but call a stored procedure that returns a different poco i.e. different names same number of columns or different number of columns, you always get back the cached poco.

I was able to modify the IL code to always read from the data record in the case of type == typeof(object), not sure if it's enough or correct. I don't think we'll have this issue with strongly typed objects since their definition doesn't change at runtime.

This is how I fixed it:

1. Get a reference to the GetName method of IDataRecord (copy paste from fnGetValue and modify) e.g. private static MethodInfo fnGetName = typeof(IDataRecord).GetMethod("GetName", new Type[] { typeof(int) });
2. In PocoData.GetFactory modify the IL (line138 approx) to get the field name from the record via fnGetName e.g. Replace il.Emit(OpCodes.Ldstr, reader.GetName(i)); //obj, obj, fieldname with the following three lines of code: il.Emit(OpCodes.Ldarg_0); //obj, obj, rdr il.Emit(OpCodes.Ldc_I4, i); //obj, obj, rdr, i il.Emit(OpCodes.Callvirt, fnGetName); //obj, obj, fieldname I'm not an expert in writing IL, but I think this loads the reader and the counter onto the stack so that fnGetName.Invoke(object, params object[]) can find it. Then we call fnGetName (the two arguments are popped off the stack and the return value is then loaded onto the stack. From the comments next to these lines of code, it will leave the stack intact.

If you agree with this change I can't do a PR for you, if you want. Bare in mind that this would be my first PR

[bp2008]

I can't comment on the stored procedure results issue, but I encountered the original bug myself and devised this simple working solution. My apologies, I don't have time for unit tests and a pull request.

Update PetaPoco/Core/PocoData.cs

- private static Cache<Type, PocoData> _pocoDatas = new Cache<Type, PocoData>();
+ private static Cache<PocoDataCacheKey, PocoData> _pocoDatas = new Cache<PocoDataCacheKey, PocoData>();

…

- return _pocoDatas.Get(type, () => new PocoData(type, defaultMapper));
+ return _pocoDatas.Get(new PocoDataCacheKey(defaultMapper, type), () => new PocoData(type, defaultMapper));

New file: PetaPoco/Core/PocoDataCacheKey.cs

using System;

namespace PetaPoco.Core
{
	/// <summary>
	/// Combines an <see cref="IMapper"/> and a <see cref="System.Type"/> into one object for dictionary keying purposes.
	/// </summary>
	internal class PocoDataCacheKey
	{
		public readonly IMapper Mapper;
		public readonly Type Type;
		public PocoDataCacheKey(IMapper mapper, Type type)
		{
			Mapper = mapper;
			Type = type;
		}
		public override int GetHashCode()
		{
			return Mapper.GetHashCode() ^ Type.GetHashCode();
		}
		public override bool Equals(object obj)
		{
			if (obj is PocoDataCacheKey)
			{
				PocoDataCacheKey other = (PocoDataCacheKey)obj;
				return other.Mapper == Mapper && other.Type == Type;
			}
			return false;
		}
		public override string ToString()
		{
			return Mapper.ToString() + ", " + Type.ToString();
		}
	}
}

It occurs to me this caching method would leak cache keys (and IMapper instances) which could be a problem if your app produces a lot of them and doesn't reuse them.