Skip to content

Handle parameters in Accepts mediatypes in DefaultProblemDetailsWriter #54158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 4, 2024
Merged

Handle parameters in Accepts mediatypes in DefaultProblemDetailsWriter #54158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93c8a15
changed default problem details writer and tests
MythoclastBM Feb 20, 2024
f9f4fb1
Fixed issue where DefaultProblemDetailsWriter doesn't handle media ty…
MythoclastBM Feb 20, 2024
e3de497
style points
MythoclastBM Feb 24, 2024
ab1d4b6
Update src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs
MythoclastBM Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ public bool CanWrite(ProblemDetailsContext context)
{
return true;
}

for (var i = 0; i < acceptHeader.Count; i++)
{
var acceptHeaderValue = acceptHeader[i];

if (_jsonMediaType.IsSubsetOf(acceptHeaderValue) ||
_problemDetailsJsonMediaType.IsSubsetOf(acceptHeaderValue))
// Check to see if the Accepted header values support `application/json` or `application/problem+json`
// with support for argument parameters. Support handling `*/*` and `application/*` as Accepts header values.
// Application/json is a subset of */* but */* is not a subset of application/json
if (acceptHeaderValue.IsSubsetOf(_jsonMediaType) || acceptHeaderValue.IsSubsetOf(_problemDetailsJsonMediaType) || _jsonMediaType.IsSubsetOf(acceptHeaderValue))
{
return true;
}
Expand Down
2 changes: 2 additions & 0 deletions src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,8 @@ await writer.WriteAsync(new ProblemDetailsContext()
[InlineData("application/*")]
Copy link
Member

@adityamandaleeka adityamandaleeka Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Seems like this shouldn't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[InlineData("application/json")]
[InlineData("application/problem+json")]
[InlineData("application/json; charset=utf-8")]
[InlineData("application/json; v=1.0")]
public void CanWrite_ReturnsTrue_WhenJsonAccepted(string contentType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already do.

{
// Arrange
Expand Down