SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
890 stars 46 forks source link

The System.Text.Json serializer for Guid backed objects throws System.ArgumentNullException instead of System.Text.Json.JsonException when the property value is null #581

Closed matthewvarley closed 7 months ago

matthewvarley commented 7 months ago

Describe the bug

System.Text.Json expects custom converters to throw JsonException or NotSupportedException. With a Guid backed ValueObject, the generated converter will throw ArgumentNullException for when presented with a null value in the raw JSON. This happens because the generated code calls into Guid.Parse instead of Guid.TryParse and then handling the result.

// This is the generated code
public override ObjectType Read(ref global::System.Text.Json.Utf8JsonReader reader, global::System.Type typeToConvert, global::System.Text.Json.JsonSerializerOptions options)
{
     return ObjectType.Deserialize(System.Guid.Parse(reader.GetString()));
}

This is a problem because throwing an ArgumentNullException here will cause ASP.NET WebApi endpoints to generate a 500 response instead of a 400. A minimal repro to generate the ArgumentNullException is included in the steps to reproduce.

Steps to reproduce

using System.Text.Json;
using Vogen;

namespace MinimalRepro
{
    public class Program
    {
        public static void Main()
        {
            // Throws ArgumentNullException instead of JsonException
            JsonSerializer.Deserialize<ObjectContainer>("{\"Ot\":null}");
        }
    }

    public class ObjectContainer
    {
        public required ObjectType Ot { get; set; }
    }

    [ValueObject<Guid>(Conversions.Default)]
    public readonly partial struct ObjectType
    {
    }
}

Expected behaviour

The expectation is for a System.Text.Json.Serialization.JsonConverter to throw System.Text.Json.JsonException. Manually changing the generated read method to something like this generates the expected behavior and triggers ASP.NET to return a 400.

public override ObjectType Read(ref global::System.Text.Json.Utf8JsonReader reader, global::System.Type typeToConvert, global::System.Text.Json.JsonSerializerOptions options)
{
  if (System.Guid.TryParse(reader.GetString(), out Guid g))
  {
    return ObjectType.Deserialize(g);
  }
  else
  {
    throw new System.Text.Json.JsonException("Unable to parse");
  }
}
SteveDunn commented 7 months ago

Thanks very much for the feedback and repro. This has now been fixed - good spot! It will be in the next release, which is being built and should be available in an hour or so (once all the snapshot tests are finished! 🥱 )