ikyriak / IdempotentAPI

A .NET library that handles the HTTP write operations (POST and PATCH) that can affect only once for the given request data and idempotency-key by using an ASP.NET Core attribute (filter).
MIT License
270 stars 39 forks source link

Feature: Configurable JSON serializers #74

Closed angularsen closed 3 months ago

angularsen commented 5 months ago

We use NodaTime in our DTOs for things like Instant and LocalDate, which requires a custom JSON serializer NodaTime.Serialization.SystemTextJson.

Problem: JsonSerializationException on unsupported property types in DTO

IdempotentAPI does serialize the first successful request, but fails to deserialize the second request with Newtonsoft.Json.JsonSerializationException:

Newtonsoft.Json.JsonSerializationException: Error converting value 5/7/2024 5:28:54 AM to type 'NodaTime.Instant'.

Path '['Context.Result'].ResultValue.Jobs.$values[0].StatusUpdated', line 1, position 3312.\n ---> System.ArgumentException: Could not cast or convert from System.DateTime to NodaTime.Instant.

called from

IdempotentAPI.Helpers.Utils.DeSerialize<T>(byte[] compressedBytes)
IdempotentAPI.Core.Idempotency.ApplyPreIdempotency(ActionExecutingContext context)
IdempotentAPI.Filters.IdempotencyAttributeFilter.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)

It seems Newtonsoft JSON is used internally and without any support for configurable JSON serializers.

Proposal

  1. Allow configuring JSON serializer externally, for example see how FusionCache adds its own serializer interface to support both Newtonsoft and System.Text.Json and external configuration a. FusionCacheNewtonsoftJsonSerializerExtensions.AddFusionCacheNewtonsoftJsonSerializer()
    b. FusionCacheSystemTextJsonSerializerExtensions.AddFusionCacheSystemTextJsonSerializer().
  2. Without yet understanding the internal details, could IdempotentAPI not just reuse FusionCache to deserialize the response? It is already configured to support our DTOs.

Our setup

services.AddStackExchangeRedisCache(/*...*/);          
services.AddFusionCacheStackExchangeRedisBackplane(/*...*/);        
services.AddFusionCache().TryWithAutoSetup();        

// Configure FusionCache with System.Text.Json, NodaTime serializer and more.
services.AddFusionCacheSystemTextJsonSerializer(new JsonSerializerOptions
{
    DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
    Converters =
    {
        new JsonStringEnumConverter(),
    }
}.ConfigureForNodaTime(DateTimeZoneProviders.Tzdb));

// Reuse FusionCache in IdempotentAPI
services.AddIdempotentAPI();
services.AddIdempotentAPIUsingRegisteredFusionCache();
ikyriak commented 5 months ago

Hello @angularsen

Thank you for your time and proposal. Allowing the external configuration of a JSON serializer is a great idea. Internally, the Newtonsoft.Json is configured with TypeNameHandling.All to include the .NET type name when serializing. I will look into it and prepare a pre-release for you to test.

angularsen commented 5 months ago

That's awesome, I'm happy to give it a test

ikyriak commented 4 months ago

Hello @angularsen,

I appreciate your patience. As a first step, we can configure the existing Newtonsoft SerializerSettings to be backward compatible. Afterward, in a major release, we could configure the serialization as you described (configuring JSON serializer externally).

I have prepared the IdempotentAPI v2.5.0-issue-74-01 pre-release version, in which we can configure the Newtonsoft SerializerSettings as follows in the Startup.cs:

IdempotencyOptions idempotencyOptions = new()
{
    CacheOnlySuccessResponses = true,
    DistributedLockTimeoutMilli = 2000,
    SerializerSettings = new JsonSerializerSettings(){}
        .ConfigureForNodaTime(DateTimeZoneProviders.Tzdb)
};

// Register the IdempotentAPI Core services.
services.AddIdempotentAPI(idempotencyOptions);
angularsen commented 4 months ago

Thanks, serialization seems to work now with the pre-release nuget.

However, I ran into a new problem of getting 406 Not Accepted and 0 bytes written: https://github.com/ikyriak/IdempotentAPI/issues/78

angularsen commented 4 months ago

I created a separate issue for this: https://github.com/ikyriak/IdempotentAPI/issues/78

ikyriak commented 3 months ago

This issue is resolved in the stable version of the IdempotentAPI v2.5.0.