Handle parameters in Accepts mediatypes in DefaultProblemDetailsWriter

[MythoclastBM]

Address #52577

Fixed issue where DefaultProblemDetailWriter doesn't handle media type subsets. Added test cases including media type subsets. The corresponds with Issue#52577 tagged help wanted.

[MythoclastBM]

#52577

[adityamandaleeka]

Is there a reason you left the third clause (_jsonMediaType.IsSubsetOf(acceptHeaderValue)) in this check?

[MythoclastBM]

Yes. If the accepted media header is a superset of jsonMediaHeader it will return false when it should return true. With the absence of the check: the test for case */* will fail.

[adityamandaleeka]

Are we meant to be able to write this at all then? cc @captainsafia

Seems like this shouldn't work.

[captainsafia]

According to the RFC, it looks like applicaton/*is a content-type that we should match against.

The asterisk "" character is used to group media types into ranges, with "/" indicating all media types and "type/" indicating all subtypes of that type. The media-range can include media type parameters that are applicable to that range.

I believe some of the weirdness here might be related to the way that comparisons happen for parameters in the IsSubsetOf extension method. Previously, it was checking to see if all the parameters defined in acceptsHeaderValue appeared in _jsonMediaType. Since _jsonMediaType has no parameters defined, this would always return false, hence the inversion eeded.

We need to retain the _jsonMediaType.IsSubsetOf(acceptHeaderValue) because MediaTypeHeaderValue handles comparisons for subtypes differently (it requires the argument provided to be more permissive).

TL;DR: I think the issue here is the confusing IsSubsetOf APIs on MediaTypeHeaderValue.

[adityamandaleeka]

Thanks @MythoclastBM! I left some comments. Apart from some minor style stuff, I'm curious why you left an extra clause in the check that was there before. I assumed you'd just flip the two existing ones.

[captainsafia]

To cover our bases, can we add a check for */* as well?

[MythoclastBM]

Already do.

[tore-hammervoll]

Will we see this change in a patch release of .Net 8? Or do we have to wait for .Net 9?

[captainsafia]

@tore-hammervoll This change isn't being back ported. We typically only backport things if there is a big enough demand for it. If this is blocking you, let me know.

[tore-hammervoll]

We got around most cases of this issue by ensuring we write the problems with DefaultApiProblemDetailsWriter instead of DefaultProblemDetailsWriter. This handles content negotiation in a much more consistent way. The missing piece of the puzzle was passing the original endpoint metadata on to the IProblemDetailsService.TryWrite, from our custom IExceptionHandler implementations similarly to the way it's done in ExceptionHandlerMiddlewareImpl:

aspnetcore/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs

Line 194 in aefa4ca

AdditionalMetadata = exceptionHandlerFeature.Endpoint?.Metadata,

The only case I've discovered that falls back to the DefaultProblemDetailsWriter now is requests to paths that don't hit any controller action. These errors will then use the lacking content negotiation of this implementation, and may result in an empty body or a simple textual error. If a problem details response is written, it will also be missing the traceId field due to #54325. We don't use minimal apis yet, only controllers still, so this issue is very minor for us.

[captainsafia]

The only case I've discovered that falls back to the DefaultProblemDetailsWriter now is requests to paths that don't hit any controller action.

I see. If this becomes a serious problem, you can file an issue on the repo and we can make a decision around backporting with that specific context in mind.