Fix issue when adding a new complex value to a generic dictionary

[rmja]

This pr fixes an issue that it shown by the added test where it is not possible to add a complex object to a typed dictionary.

The fix prefers to use the IDictionary<TKey, TValue> proxy over the IDictionary proxy as e.g. Dictionary<TKey, TValue> implements both.

[rmja]

One thing that I would like to change but that could be breaking is to not find the PropertyType from an instance in the dictionary:

https://github.com/rmja/SystemTextJsonPatch/blob/generic-dictionary/SystemTextJsonPatch/Internal/Proxies/DictionaryTypedPropertyProxy.cs#L44-L56

public Type PropertyType => typeof(TValue);
{
    get
    {
        _dictionary.TryGetValue(_propertyName, out var val);
        if (val == null)
        {
            return typeof(TValue);
        }

        return val.GetType();
    }
}

This should instead be public Type PropertyType => typeof(TValue); to allow for the case where TValue may be a base type to which as custom converter is attached. By using the type from the current instance as is the current approach, this limits the ability to replace an instance of one derived type with another.

[Havunen]

Hi,

Thanks for the PR.

This should instead be public Type PropertyType => typeof(TValue); to allow for the case where TValue may be a base type to which as custom converter is attached. By using the type from the current instance as is the current approach, this limits the ability to replace an instance of one derived type with another.

That type is not considered public, so it should be safe to change as long as it works as expected

[rmja]

Hi,

Thanks for the PR.

This should instead be public Type PropertyType => typeof(TValue); to allow for the case where TValue may be a base type to which as custom converter is attached. By using the type from the current instance as is the current approach, this limits the ability to replace an instance of one derived type with another.

That type is not considered public, so it should be safe to change as long as it works as expected

The compatibility problem if we chose to have Type PropertyType => typeof(TValue) is the following:

Old behavior: If we update an entry in the dictionary then we try and convert to the entry value instance type. Consider for example Dictionary<string, AnimalBase> where we could possible convert to typeof(Dog) if a dog entry exists, where Dog: AnimalBase.

The result, we try and convert to typeof(Dog).

New behavor: We always try and convert to typeof(AnimalBase).

The problem with the old behavior is if we want to replace the entry to some other type, e.g. Dog -> Cat. That would only be possible if we convert to the base type (Type PropertyType => typeof(TValue)). One can argue that the current behavior is a bug, in which case, if we decide to change it, is not a breaking change:)

[rmja]

I have included the Type PropertyType => typeof(TValue) change and added a corresponding test. And, the ctor is internal again.

[Havunen]

This is now available in Nuget v3.2.0