Bug: custom serializer settings are ignored

[bart-degreed]

When setting custom serialization settings on JsonApiOptions, they are not used. In our scenario we have a few enums exposed as attributes and we want them to serialize into their text representation instead of numeric.

Example:

public enum ScreenOrientation
{
    Horizontal, Vertical
}

public class DemoResource : Identifyable
{
    [Attr]
    public ScreenOrientation Orientation { get; set; }
}

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddJsonApi<AppDbContext>(options =>
        {
            options.SerializerSettings.Converters = new List<JsonConverter>
            {
                new StringEnumConverter()
            };
        });
    }
}

Putting [JsonConverter(typeof(StringEnumConverter))] on ScreenOrientation works, but the global setting does not.

Using the global setting should work, according to https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/master/docs/usage/options.md#custom-serializer-settings.

[maurei]

Thanks for your involvement and looking into this. In v3 it was possible to configure serialization through Newtonsoft, but this has been removed during the big rework of the serialization layer (also see the related wiki article). It looks like JsonApiSettings.JsonSerializerSettings is an artifact that I forgot to remove... (and ofc the docs haven't been updated yet)

The reason for the removal was that, in short, with changes (even small ones!) in JsonSerializerSettings it's not really possible to ensure the serializers output will still be conform spec. This is not always a problem: I think if a dev wants to change the serialization output then the frameworks extensibility should facilitate that. But small changes like setting NullValueHandling to Ignore shouldn't be enough break the spec, but it was. As such I decided against configuring Newtonsoft. See the long answer below for an indepth review of the issue related to your PR. Let me know what you think of this design choice taking all that into account.

The rework introduced ISerializerOptions (which is inherited by IJsonApiOptions) and these allow for handling null values and default values while ensuring the serializers output is json:api compliant. I think we should expand on this and come up with a nice way to cover the use-case you're describing in your PR. Let me know if you want me to put thought in that.

--
BTW are you sure that using [JsonConverter(typeof(StringEnumConverter))] attribute works? I'm actually surprised to hear that, because the value of the attributes are being copied right here, "as is" into an entirely new object. And from this point and on, there is no more connection to the original JsonConverterAttribute anymore by the time that object is consumed by JsonConvert.SerialzeObject( ... ) right here

[maurei]

More elaborative answer: we can't really rely on NewtonSoft.JsonConvert in the first place because the transformation from a regular (efcore/c#) model to a json:api resource object and then to wrap it in a json:api document is pretty non-trivial and too complicated for Newtonsoft.JsonConvert to handle, and even if we managed it wouldn't really be extensible or reusable. That is the primary motive for having builders for resource objects and document objects. And because the meat of the work that is performed in these builders is not at all related to Newtonsoft, any config we would allow of Newtonsoft can only have limited impact.

There's a few examples in which configuring NullValueHandlingOption.Ignore breaked the spec in v3, but an easy one to see that I recall is encoded in these unit tests and the surrounding ones. It boils down to the following.

From this bit in the spec

“relationship object” MUST contain at least one of the following:

it is implied that the following partial is invalid

"relationships": {
	"author": { },  <--- this line is not valid
	"reviewer": { "data": { "id": 1, "type": "people" } }
}

This will occur if the to-one relationship author is not populated (links are disabled globally in these examples); the payload would have been { "data": null } but it got stripped away by NullValueHandlingOption.Ignore. For it to be complaint with the spec, the "author" member should be omitted completely. And to make it more complicated: if the reviewer relationship did not exist, the entire relationship member should have been omitted. That is to say: unless links would have been available, in which case the author member should have remained with only link data. In #688 this bit of the spec is now violated by the helper method introduced here if a user decides to use chooses NullValueHandlingOption.Ignore .

Another way to look at the problem is to recognise that a violation of SRP has been introduced: the responsibility for constructing json:api resource objects is within the ResourceObject builder (this one in the case of server-side serialization, to be precise), i.e. there shouldn't be any other objects that structurally alter ResourceObjects. The ResponseSerializer (which is a DocumentBuilder) is responsible for building the final json:api response by wrapping the (externally fabricated) ResourceObjects in a json:api Document. However in #688, through the expression of JsonApiSettings.JsonSerializerSettings in the helper method that is introduced, the ResponseSerializer can now also structurally change the resource objects. (The edge case that I described with the snippet above is unit tested on the level of ResourceObject builders, not for Document builders, which is why no test failed even though the latter has changes that can violate these edge case)

[bart-degreed]

BTW are you sure that using [JsonConverter(typeof(StringEnumConverter))] attribute works?

Actually, it does. The reason is because the enum value is stored as object at your first code reference. If that were a ToString() call instead, it would not have preserved the runtime type. At the second code reference, the runtime type is detected and scanned for the converter attribute by Newtonsoft.Json.

we can't really rely on NewtonSoft.JsonConvert in the first place

I agree with your point that null value handling is complicated and needs to be special-cased, in order to be compliant with the json:api spec. That is exactly why I introduced the scope class: it prevents outsiders to override null-value handling in cases when JADNC knows better. Me not knowing all the internals, I may have missed some spots that are not covered by tests.

I think if a dev wants to change the serialization output then the frameworks extensibility should facilitate that.

However, there is a lot more in Newtonsoft.Json.JsonSerializerSettings that can be useful to override. Enum conversion, culture settings, tracing, timezone handling, dealing with circular references etc. can be valuable to be able to override. These are advanced use cases.

I believe you are trying to abstract away the actual serialization implementation by introducing ISerializerOptions. At this moment, I think that is a wrong choice because it combines the worst of both: not being able to choose an alternative serialization library, but also not being able to tweak the current one.

If, in the future, the underlying serialization library becomes pluggable, that would be a good time to remove explicit dependencies on Newtonsoft.Json and wrap its settings.

Either way, we should add tests for uncovered cases. Can you identify these?

[bart-degreed]

Update: After discussion, we decided to keep the Newtonsoft.Json setting and remove ISerializerOptions.

[bart-degreed]

To clarify: The work that needs to be done here, in additional to the changes in #688, is to remove the JsonApiDotNetCore.Configuration.ISerializerOptions interface and update its usages to use Newtonsoft.Json.JsonSerializerSettings instead.