Cannot use JSON source generator with (HttpValidation)ProblemDetails

[martincostello]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

If the JSON source generator is enabled for the default JSON serializer options and an application uses the Results.Problem() or Results.ValidationProblem() methods, it is not possible to make the two work together.

If the HttpValidationProblemDetails or ProblemDetails is not registered with the JsonSerializerContext, then an exception is thrown at runtime:

NotSupportedException: Metadata for type 'Microsoft.AspNetCore.Mvc.ProblemDetails' was not provided by TypeInfoResolver of type 'ApplicationJsonSerializerContext'. If using source generation, ensure that all root types passed to the serializer have been indicated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically.

If a developer attempts to add either type using [JsonSerializable], then the application fails to compile:

Project\Repro\System.Text.Json.SourceGeneration\System.Text.Json.SourceGeneration.JsonSourceGenerator\ApplicationJsonSerializerContext.ProblemDetails.g.cs(24,120): error CS0122: 'ProblemDetailsJsonConverter' is inaccessible due to its protection level

Expected Behavior

An application is successfully able to use Results.Problem() or Results.ValidationProblem() with JSON source generators and Minimal APIs.

Steps To Reproduce

A full repro project is available at martincostello/problemdetails-json-source-generator-repro.

To reproduce, clone the project and run dotnet build.

Exceptions (if any)

No response

.NET Version

7.0.100-preview.7.22377.5

Anything else?

I think something somewhere in the JSON source generator has become more strict beginning in .NET 7 preview 7, which is what brought this to my attention.

Previously I think the source generator would just fallback to using runtime serialization so things would "just work" without you needing to add the [JsonSerializable] attribute.

It seems like it would be fairly trivial to fix this use case by making HttpValidationProblemDetailsJsonConverter and ProblemDetailsJsonConverter public instead of internal, but as they're shared source that might cause other complications with multiple public types with the same name and namespace in different assemblies.

[brunolins16]

@martincostello I worked with the JSON source generation for problem details when developing the ProblemDetailsService and just to let you know that probably you will face another problem when it is fixed because, by default, it will not fallback to reflection-based the Extensions property fail to serialize.

aspnetcore/src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

Line 54 in 7cb457b

if (context.ProblemDetails.Extensions is { Count: 0 })

Unless if you knowo all types that will be there at runtime and added then to your context.

https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-source-generation?pivots=dotnet-6-0#source-generation-support-in-aspnet-core

[captainsafia]

I think something somewhere in the JSON source generator has become more strict beginning in .NET 7 preview 7, which is what brought this to my attention.

Did this issue not occur prior to preview7?

[martincostello]

In the context of where it was occurring where I found it, no.

I have a bit of custom code in an example app that's used to generate examples for the OpenAPI docs with Swashbuckle.AspNetCore that was using the default JSON serializer which had a serializer context associated with it. Since updating to preview 7, that started to throw.

To "fix" it, I had to reconfigure the filter to use the copy-constructor to create a separate options and unassociate the context: martincostello/api@6d37785

[BrennanConroy]

@dotnet/area-system-text-json do you know what might be going on here? Did something change in preview7 to cause this problem when using the source generator?

aspnetcore/src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

Lines 69 to 70 in 846f0fd

	[JsonSerializable(typeof(ProblemDetails))]
	internal sealed partial class ProblemDetailsJsonContext : JsonSerializerContext

aspnetcore/src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs

Line 11 in 7cb457b

internal sealed class ProblemDetailsJsonConverter : JsonConverter<ProblemDetails>

[eiriktsarpalis]

If the HttpValidationProblemDetails or ProblemDetails is not registered with the JsonSerializerContext, then an exception is thrown at runtime:

This seems related to an intentional breaking change we made in Preview 7. See dotnet/runtime#71714 for more details and a workaround.

If a developer attempts to add either type using [JsonSerializable], then the application fails to compile:

The error message appears related to a bug fixed recently in dotnet/runtime#71714, cc @krwq.

[martincostello]

Thanks for the pointer at the workaround @eiriktsarpalis - I got this back to working how it was in preview 6 with this change: martincostello/api@b43a46c

[pinkfloydx33]

I just struggled through the same exact issue with ValidationProblemDetails/ProblemDetails for my MVC-based API. I tried adding the types to my JsonSerializerContext but couldn't due to the internal converters. I eventually came to the same work-around using the TypeInfoResolver. but I'm trying to figure out if there's any downside to not using AddContext<> as that is what the docs say to use.

I started a discussion over in dotnet/runtime asking about the implications. It would be nice to have a way around this without needing to fallback to the reflection resolver. Though... perhaps it won't be an issue once Problem Details service is in the mix

[eiriktsarpalis]

FWIW I should say that adding reflection-based fallback defeats the purpose of using source generators. It seems unfortunate that we need to rely on reflection (accidentally in .NET 6, explicitly starting with .NET 7) to add serialization support for the validation models. It sounds to me like MVC might want to expose a JsonSerializerContext for common model types so that users can add them to the mix without relying on reflection:

builder.Services.ConfigureRouteHandlerJsonOptions((options) =>
{
    options.TypeInfoResolver = JsonTypeInfoResolver.Combine(ApplicationJsonSerializerContext.Default, ValidationContext.Default);
});

cc @davidfowl

[pinkfloydx33]

Those of us migrating from @khellang's ProblemDetailsMiddleware will be stuck with the reflection approach, assuming we want to keep our output as is. That middleware adds a traceId extension which would immediately disqualify us from source gen and force us into reflection mode anyways. Even with a context exposed for "combining" we'd still be SOL.

The options would be:

• stop outputting traceId
• somehow replace the converter

As the converters are specified as class-level attributes the second option doesn't seem possible. If it were, we'd likely need (or at least want) a way to subclass the current internal converters to add our well-known extension data, though the current code path would still exclude source gen due to the extension length check. We could always replace the IProblemDetailsService with our own implementation that elided that check, but we'd still need a way around the converter issue.

It seems like our only options are to either remove the property or be at peace with the fact that we need the reflection fallback just to cover this case. That may be alright, but it could cause other types to slip through the cracks if we accidentally miss annotating them as we'd no longer receive the source-gen errors that we get with the latest previews.

[khellang]

Adding the traceId extension was merely a mirroring of ASP.NET's default behavior:

aspnetcore/src/Mvc/Mvc.Core/src/Infrastructure/DefaultProblemDetailsFactory.cs

Lines 95 to 99 in f6362e0

	var traceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier;
	if (traceId != null)
	{
	problemDetails.Extensions["traceId"] = traceId;
	}

For my middleware, it's easily excluded by setting GetTraceId and returning null 😄

[pinkfloydx33]

Adding the traceId extension was merely a mirroring of ASP.NET's default behavior:

In that case, wouldn't it cause all ProblemDetails (or at least those coming from controllers) to have the traceId extension and thus forced into reflection-mode? I can't tell if the new services are limited to minimal-APIs or apply to MVC as well so perhaps it not a... problem.

For what it's worth, the new services uses ProblemDetailsDefaults.Apply and it looks like minimal API IResult-based results use HttpResultsHelper.ApplyProblemDetailsDefaults to "set up" the ProblemDetails. Neither of these set any of the extensions, so it seems limited to the factory/MVC-based production.

For my middleware, it's easily excluded by setting GetTraceId and returning null 😄

I don't want to remove it; I'd like to keep it =)

[khellang]

Then what hits this code path? 🤔

aspnetcore/src/Mvc/Mvc.Core/src/Infrastructure/DefaultApiProblemDetailsWriter.cs

Lines 54 to 70 in f6362e0

	// Recreating the problem details to get all customizations
	// from the factory
	var problemDetails = _problemDetailsFactory.CreateProblemDetails(
	context.HttpContext,
	context.ProblemDetails.Status ?? context.HttpContext.Response.StatusCode,
	context.ProblemDetails.Title,
	context.ProblemDetails.Type,
	context.ProblemDetails.Detail,
	context.ProblemDetails.Instance);
	if (context.ProblemDetails?.Extensions is not null)
	{
	foreach (var extension in context.ProblemDetails.Extensions)
	{
	problemDetails.Extensions[extension.Key] = extension.Value;
	}
	}

[pinkfloydx33]

Then what hits this code path? 🤔

I blame early morning coffee-less code interpreting =)

[halter73]

Triage question: Is there anything we can or should do to help with (HttpValidation)ProblemDetails serialization with the source generator in aspnetcore for .NET 7?

builder.Services.ConfigureRouteHandlerJsonOptions((options) =>
{
    options.TypeInfoResolver = JsonTypeInfoResolver.Combine(ApplicationJsonSerializerContext.Default, ValidationContext.Default);
});

cc @davidfowl

Are you suggesting that we add a new public ValidationContext.Default? So we have to define our own IJsonTypeInfoResolver? How complicated is that? Should we add it by default Does this affect more than just (HttpValidation)ProblemDetails?

Would it be okay to just call ProblemDetailsJsonConverter.Write directly from our default ProblemDetailsService? Would that also fix this source gen issue?

aspnetcore/src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs

Lines 43 to 48 in f1353bd

	public override void Write(Utf8JsonWriter writer, ProblemDetails value, JsonSerializerOptions options)
	{
	writer.WriteStartObject();
	WriteProblemDetails(writer, value, options);
	writer.WriteEndObject();
	}

[eiriktsarpalis]

So we have to define our own IJsonTypeInfoResolver? How complicated is that?

JsonSerializerContext already implements that interface via the source generator, so it really just boils down to defining your own source generated context for the required types.

[pinkfloydx33]

Would that also fix this source gen issue?

The problem (at least in my case) is that I'm using source-gen with my MVC API. By default HttpValidation/Validation/ProblemDetails all cause serialization to fail since they've not been defined on the JsonSerializerContext I've bound to Mvc's JsonOptions.

The work-around is to enable the reflection-fallback but that seems like it defeats the purpose. If aspnet exposed a JsonSerializerContext for the *ProblemDetails types we can combine that with our own context without needing to enable the reflection-fallback mode.

[davidfowl]

Are you using source generators for performance reasons?

[pinkfloydx33]

Sort of. Prior to source-gen we had custom converters for all of our types after a couple of my colleagues did some extensive profiling and saw enough improvement that it was felt to be warranted. From there we saw minor improvements with fast-path source-gen serialization, not enough to justify it on its own, but the cost of not needing to maintain custom converters is what caused the switch.

Going forward, we are starting to look at getting our apps trimmable. We aren't there yet, but are trying to avoid adding anything new that could bring trimmer warnings.

[brunolins16]

JsonSerializerContext already implements that interface via the source generator, so it really just boils down to defining your own source generated context for the required types.

@eiriktsarpalis So basically, what you are saying is we could make something like this public (probably including (Http)ValidationProblemDetails as well), right?

aspnetcore/src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

Lines 69 to 70 in 846f0fd

	[JsonSerializable(typeof(ProblemDetails))]
	internal sealed partial class ProblemDetailsJsonContext : JsonSerializerContext

As I mentioned before, the ProblemDetails.Extensions is a Dictionary<string, object?>, and if I understood correctly, we need to declare all expected runtime types (even very common types like string, int, etc.) in JsonSerializerContext that is very annoying. Is there anything that we can do to avoid it? Also, should some of them be declared in the ProblemDetailsContext?

[eiriktsarpalis]

if I understood correctly, we need to declare all expected runtime types (even very common types like string, int, etc.) in JsonSerializerContext that is very annoying. Is there anything that we can do to avoid it?

Unfortunately, no. Relevant issue: dotnet/runtime#58134. Even if we do address it for common primitives, fundamentally the experience won't change: any runtime types will need to be explicitly opted in.

[brunolins16]

Unfortunately, no.

😟 So, since MVC adds the traceId we'll probably need to add, at least, string to the public context (to make the user experience a little bit smoother).

aspnetcore/src/Mvc/Mvc.Core/src/Infrastructure/DefaultProblemDetailsFactory.cs

Lines 95 to 99 in f6362e0

	var traceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier;
	if (traceId != null)
	{
	problemDetails.Extensions["traceId"] = traceId;
	}

[brunolins16]

@martincostello In .NET 8, we changed to when AddProblemDetails is called we combine our internal ProblemDetailsJsonContext, so, in most cases that will be enough. However, I am still trying to have this #44132 that will allow add ProblemDetails to your JsonContext

[brunolins16]

@martincostello #44132 is completed and you should be able from .NET 8 Preview 2 source-gen a problemdetails, if needed