Unleash / unleash-client-dotnet

Unleash client SDK for .NET
Apache License 2.0
81 stars 39 forks source link

api/client/metrics return 400 when using System.Text.Json for seriaization. #197

Closed erandjo closed 8 months ago

erandjo commented 8 months ago

Describe the bug api/client/metrics return 400 when using System.Text.Json for Json serializing. The bug disappear when using Newtonsoft. This bug has similarities to a previously reported issue: https://github.com/Unleash/unleash-client-dotnet/issues/173. I have not debugged the error but I suspect that there is a problem with serializing the request body.

unleash-client-dotnet version: 4.1.1 and 4.1.2.

we began experiencing this bug at 2024-01-04T12:41:56Z

To Reproduce

Steps to reproduce the behavior:

  1. Create a simple app that reference "unleash.client".
  2. Add JsonSerializer = new TextJsonSerializer() to UnleashSettings.
  3. Wrap System.Text.Json serializer:
internal class TextJsonSerializer : IJsonSerializer
{

    T IJsonSerializer.Deserialize<T>(Stream stream)
    {
        var options = new JsonSerializerOptions
        {
            PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
        };
        var instance = JsonSerializer.Deserialize<T>(stream, options);
        if (instance == null)
        {
            throw new Exception("The result of serializing feature toggle is null");
        }

        return instance;
    }

    void IJsonSerializer.Serialize<T>(Stream stream, T instance)
    {
        JsonSerializer.Serialize<T>(stream, instance);
    }
}
  1. log or inspect the post request to api/client/metrics.

Expected behavior Should respond with 2xx. When posting the request directly to unleash we receive 202.

daveleek commented 8 months ago

Thank you @erandjo for raising this. We'll have a look! Interesting though, I'd have thought providing Naming policy camel case through JsonSerializerOptions would have sorted this

sjaanus commented 8 months ago

Couple of things here

  1. For serialization use also camel case options
  2. Setting stream position to 0 in the end of serialization

Although, this will eventually not work with Dependent Features, because of an issue in .NET runtime https://github.com/dotnet/runtime/issues/44428


class TextJsonSerializer : IJsonSerializer
{
    private readonly JsonSerializerOptions options;

    public TextJsonSerializer()
    {
        options = new JsonSerializerOptions
        {
            PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
        };
    }

    T IJsonSerializer.Deserialize<T>(Stream stream)
    {
        var instance = JsonSerializer.Deserialize<T>(stream, options);
        if (instance == null)
        {
            throw new Exception("The result of serializing feature toggle is null");
        }
        return instance;
    }

    void IJsonSerializer.Serialize<T>(Stream stream, T instance)
    {
        JsonSerializer.Serialize(stream, instance, options);
        stream.Position  = 0;
    }
}
erandjo commented 8 months ago

Wow. thank you. This seems to be a blunder on my part, forgetting to pass in options; I am sorry about that. I will close the the issue. Interesting side note about dependent features. Thanks again.

erandjo commented 8 months ago

I searched for dependent features on unleash and unleash dotnet sdk out of curiosity for what you meant with the comment about dependent features not working in the future. I couldnt find an exact reference to dependent features (with exception of some tests).

These tests lead me to the FeatureToggle constructor:

        public FeatureToggle(string name, string type, bool enabled, bool impressionData, List<ActivationStrategy> strategies, List<VariantDefinition> variants = null, List<Dependency> dependencies = null)
        {
            Name = name;
            Type = type;
            Enabled = enabled;
            ImpressionData = impressionData;
            Strategies = strategies ?? new List<ActivationStrategy>();
            Variants = variants ?? new List<VariantDefinition>();
            Dependencies = dependencies ?? new List<Dependency>();
        }

where I found that dependencies was optional and defaulting to an empty lists of dependencies (similar to the issue you shared). Is this the reason why dependent features wont work when using System.Text.Json, and will this also be true for variants?

daveleek commented 8 months ago

where I found that dependencies was optional and defaulting to an empty lists of dependencies (similar to the issue you shared). Is this the reason why dependent features wont work when using System.Text.Json, and will this also be true for variants?

Hello @erandjo! No, it's inside dependency implementation. Spec requires enabled to default to true if not set in the JSON payload, and we didn't want to expose a nullable boolean on the dependency itself, leading to System.Text.Json not being able to resolve the type/property/parameter since once is a bool? and the other a regular bool. Ends up with runtime deserialization errors. (https://github.com/dotnet/runtime/issues/44428)

Docs on dependent features is here by the way: https://docs.getunleash.io/reference/dependent-features