AddApiVersioning with a custom IProblemDetailsWriter breaks both the custom and decorated IProblemDetailsWriter

[EmperorArthur]

trafficstars

Is there an existing issue for this?

• [x] I have searched the existing issues

Describe the bug

Cross posting from https://github.com/dotnet/aspnetcore/issues/62186

Reported there, since the code explicitly mentions https://github.com/dotnet/aspnetcore/issues/52577.

tldr: TryAddProblemDetailsRfc7231Compliance searches for "DefaultProblemDetailsWriter" but always replaces the first "IProblemDetailsWriter" registered with the service provider.

Expected Behavior

I can use a custom IProblemDetailsWriter before the default writer.

Steps To Reproduce

See https://github.com/dotnet/aspnetcore/issues/62186

Exceptions (if any)

None

.NET Version

9.0.203

Anything else?

See https://github.com/dotnet/aspnetcore/issues/62186

[commonsensesoftware]

It looks like this might finally be fixed in .NET 10:

https://github.com/dotnet/aspnetcore/pull/54158

Due to the way the IProblemDetailWriter works, it's an unsafe assumption to decorate any registered instance. It's kind of yucky and I wish I had a better answer, but the best path forward at the moment would be to fork:

https://github.com/dotnet/aspnet-api-versioning/blob/main/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Rfc7231ProblemDetailsWriter.cs

and merge it into your writer. It's not ideal, but that will unblock you. It's small and straight forward. This type is internal because it was a mitigation to a known bug and not something that should be public and return in future breaking changes.. I'll look to merge this with the ASP.NET fix when it's released.

[EmperorArthur]

The logic error is services.FirstOrDefault( IsDefaultProblemDetailsWriter ) doesn't match the behavior of services.Replace. So, it's replacing the wrong service.

Looking at the code, the problem you're trying to solve is the default ProblemDetailsWriter CanWrite isn't quite broad enough, Correct?

How about this?

• Remove the services.Add
  • Breaks normal contract if AddApiVersioning is called twice, or the service already existed some other way.
  • Doesn't provide any value.
  • Doesn't match with the behavior I'm describing.
• Change services.Replace to services.TryAddEnumerable
  • 2nd IProblemDetailsWriter will catch the edge cases missed by the first.
• Adjust TryAddErrorObjectJsonOptions` so it will work with multiple items.
  • Moving away from the notion that there's a single IProblemDetailsWriter to be modified.

Edit: Another option is to do what "Asp.Versioning.MVC" does in "IApiVersioningBuilderExtensions.cs" That has a TryReplace<TService, TImplementation, TReplacement> function.

[EmperorArthur]

Worked around the issue by injecting my CustomProblemDetailsWriter using Insert.

This preserves the original functionality. I prefer that, since my custom writer, is meant only for users, not for APIs.

builder.Services.AddProblemDetails();
builder.Services.AddApiVersioning();
builder.Services.Insert(0,
    new ServiceDescriptor(typeof(IProblemDetailsWriter), typeof(CustomProblemDetailsWriter), ServiceLifetime.Singleton)
    );

[commonsensesoftware]

Yeah, I'm not in love with out the ASP.NET team designed out the writers get added. There's no concept of ordering, which makes the DI registration order matter and not particularly easy to reason about.

With regard to your proposal, there's some nuance that may not be readily obvious. The biggest thing is how the default ProblemDetailsService works for IProblemDetailsService. The simply enumerates over the provided IProblemDetailsWriter in order and uses the first match where CanWrite returns true.

https://github.com/dotnet/aspnetcore/blob/040b4df1211fe5a0f2591e5904b94e74a3203fd8/src/Http/Http.Extensions/src/ProblemDetailsService.cs#L38

If Rfc7231ProblemDetailsWriter was simply added, it would be after DefaultProblemDetailsWriter and never run because they match the same things. If I clear all services, there are other side effects. DefaultProblemDetailsWriter is internal sealed. I can neither inherit from it nor decorate it with imperative code. I could fork it's code, but that type of approach has bitten me before (notably with OData).

The only feasible workaround is to use the following process:

1. Decorate if any only if DefaultProblemDetailsWriter is still registered a. This can only be matched by type name since the type is internal
2. When a match is found, add DefaultProblemDetailsWriter as a service which resolves to itself a. e.g. DefaultProblemDetailsWriter → DefaultProblemDetailsWriter (instead of IProblemDetailsWriter)
3. Replace the IProblemDetailsWriter with custom activation function that decorates over the original ServiceDescriptor a. This can be problematic because the order of registration matters b. The DI extension have no formal support or ordering semantics for decorating registered services c. Not using the built-in Replace (as you suggested) may be able to provide a better experience with fewer surprises

It may not be super obvious, but your solution to insert your writer first appears to correct, expected setup. If you are trying to get in front of the other writers that match the same CanWrite checks, this is the only option your really have unless you clear all the registered writers and/or decorate the default writer in the same way I've had to do. Since you're inserting a 0 instead of adding, that seems to be the case.

I'll definitely consider improving this experience, but there's only so much I can do. It looks like we'll get the correct behavior is .NET 10 and this workaround will go away. I'm happy to discuss further, but it appears you're unblocked.

[EmperorArthur]

Agreed. The lack of any sort of priority is frustrating. Especially since I can't find anything which actually guarantees that ordering will be preserved when dealing with an IServiceProvider.

I think I'm missing something. It seems Rfc7231ProblemDetailsWriter only expands the CanWrite match, and doesn't touch the contents.

Unfortunately, there is a logic error in the current code. services.FirstOrDefault( IsDefaultProblemDetailsWriter ) has different match criteria compared to services.Replace(...).

IServiceCollection does implement IList<ServiceDescriptor>. Which is a potential solution:

var descriptor = services.FirstOrDefault( IsDefaultProblemDetailsWriter );
...
var index = services.IndexOf(descriptor);
services.RemoveAt(index);
services.Insert(index, ...);

[commonsensesoftware]

Apologies for not replying to your last post. If I'm understanding what you're saying, the decorated/replaced writer should occur at the same index so that the order is retained? I'm onboard with that change. Is this still an issue for you?

It looks like this has finally be fixed once and for all in .NET 10: https://github.com/dotnet/aspnetcore/blob/13e009dc4992c3bcf34913831c845d77876883a8/src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs#L26

I intend to completely remove the Rfc7231ProblemDetailsWriter in the next major version.