Nevermore icon indicating copy to clipboard operation
Nevermore copied to clipboard

Fixed properties serialising when mapped to columns

Open YuKitsune opened this issue 3 years ago • 2 comments

Summary

In scenarios where derived classes have their own tables, the serialised JSON would include properties that were already mapped to columns. This is because the RelationalJsonContractResolver used the DeclaringType to determine what Type of model is being resolved. The issue with this is that DeclaringType will get the Type that the property was declared on. In the below example, the Registration property is declared on Vehicle, so the RelationalJsonContractResolver would look for a mapping for the Vehicle class when the mapping is actually be defined for the Car class.

public abstract class Vehicle 
{
  public string Registration { get; set; }
}

public class Car : Vehicle
{
}

Ideal Solution

The ideal solution would be to change DeclaringType to ReflectingType, which should return the Type for Car, but this won't work as Netwonsoft.Json actually gets the MemberInfo from the declaring type (vehicle), so that it can access any potential private getters/setters. (Issue open on the Newtonsoft.Json repo for this https://github.com/JamesNK/Newtonsoft.Json/issues/2488)

Chosen Solution

This PR addresses this issue by overriding the CreateProperties method (since the target Type is exposed as a parameter), then passing the target Type as a parameter to a private CreateProperty method.

YuKitsune avatar Mar 09 '21 01:03 YuKitsune

Have you tried creating an Octopus Server PR that uses this build? To see if anything has broken/changed because of this?

flin-8 avatar Mar 09 '21 04:03 flin-8

Have you tried creating an Octopus Server PR that uses this build? To see if anything has broken/changed because of this?

Octopus branch here with the changes: https://github.com/OctopusDeploy/OctopusDeploy/pull/8357 Tested with an existing and a fresh instance and it seemed to work as expected

YuKitsune avatar Mar 09 '21 06:03 YuKitsune

This never got actioned. Closing for clarity (please re-open if this is still in-progress).

MarkSiedle avatar Feb 16 '23 02:02 MarkSiedle