Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Correct polarity of MediaTypeHeaderValue.IsSubsetOf()` checks and remove one conneg fallback #3317

Closed
wants to merge 3 commits into from
Closed

Correct polarity of MediaTypeHeaderValue.IsSubsetOf()` checks and remove one conneg fallback #3317

wants to merge 3 commits into from

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Oct 13, 2015

  • Fix content negotiation content-type check for InputFormatter #3138 part 2/2
  • request's Content-Type header must be a subset of what an IInputFormatter can consume
    • [Consumes] is similar
  • what an IOutputFormatter produces must be a subset of the request's Accept header
    • FormatFilter and ObjectResult are similar
  • ObjectResult no longer falls back to Content-Type header if no Accept value is acceptable
  • left WebApiCompatShim code alone for consistency with down-level System.Net.Http.Formatting
  • correct tests to match new behaviour
    • do not test Accept values containing a charset parameter; that case is not valid

nits:

  • add InputFormatterTests
  • add / update comments and doc comments
  • correct xUnit attributes in ActionResultTest; odd it doesn't show up in command-line runs

dougbu added 2 commits October 12, 2015 15:25
@dougbu
Get locally-installed Microsoft.Net.Http.Headers package
74269d7
@dougbu
Correct polarity of MediaTypeHeaderValue.IsSubsetOf()` checks and rem…
e075a58 
…ove one conneg fallback

- #3138 part 2/2
- request's Content-Type header must be a subset of what an `IInputFormatter` can consume
  - `[Consumes]` is similar
- what an `IOutputFormatter` produces must be a subset of the request's Accept header
  - `FormatFilter` and `ObjectResult` are similar
- `ObjectResult` no longer falls back to `Content-Type` header if no `Accept` value is acceptable
- left `WebApiCompatShim` code alone for consistency with down-level `System.Net.Http.Formatting`
- correct tests to match new behaviour
  - do not test `Accept` values containing a `charset` parameter; that case is not valid

WIP:
- four test failures; something about comparing media types w/ charset included
- why do some localization tests fail in VS?

nits:
- add `InputFormatterTests`
- add / update comments and doc comments
- correct xUnit attributes in `ActionResultTest`; odd it doesn't show up in command-line runs
@dougbu
Copy link
Contributor Author

dougbu commented Oct 13, 2015

/cc @Tratcher @loudej @Eilon @rynowak

dougbu
dougbu reviewed Oct 13, 2015
View reviewed changes
src/Microsoft.AspNet.Mvc.Abstractions/project.json
@@ -25,7 +25,8 @@
"Microsoft.Extensions.PropertyHelper.Sources": {
"version": "1.0.0-*",
"type": "build"
}
},
"Microsoft.Net.Http.Headers": "1.0.0-*"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore this change from the first commit. Did this only to ensure build picks up the local changes to HttpAbstractions.

@dougbu dougbu closed this Oct 14, 2015
@dougbu dougbu deleted the dougbu/match.contenttype.3138 branch October 14, 2015 19:10
@dougbu
Copy link
Contributor Author

dougbu commented Oct 14, 2015

03625c3

@pranavkm
Copy link
Contributor

No :shipit:s?

@Eilon
Copy link
Contributor

Eilon commented Oct 14, 2015

I talked to @dougbu this does still need to be reviewed.

rynowak
rynowak reviewed Oct 14, 2015
View reviewed changes
src/Microsoft.AspNet.Mvc.Core/ConsumesAttribute.cs
@@ -56,10 +56,10 @@ public void OnResourceExecuting(ResourceExecutingContext context)
MediaTypeHeaderValue requestContentType = null;
MediaTypeHeaderValue.TryParse(context.HttpContext.Request.ContentType, out requestContentType);

// Only execute if this is the last filter before calling the action.
Copy link
Member

Choose a reason for hiding this comment

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

Does this comment go with another block in this method? Your new comment is great, but methinks the previous comment wandered away from home.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a duplicate comment. Open your window on this diff just a bit wider, look at https://github.com/aspnet/Mvc/pull/3317/files#diff-1b5675f1b083c83b1f98dd3e87cf08f6R52

Copy link
Member

Choose a reason for hiding this comment

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

👍

@rynowak
Copy link
Member

rynowak commented Oct 15, 2015

⌚

@dougbu dougbu restored the dougbu/match.contenttype.3138 branch October 15, 2015 15:13
@dougbu
Copy link
Contributor Author

dougbu commented Oct 15, 2015

Reopening so we can more-easily see this discussion 'til it completes.

@dougbu dougbu reopened this Oct 15, 2015
@rynowak
Copy link
Member

rynowak commented Oct 15, 2015

:shipit:

@dougbu
Address PR comments for 2e2043f (PR #3317)
514054a 
- remove redundant `null` check in `InputFormatter`
- improve comments
- rename `ObjectResult.SelectFormatterBasedOnTypeMatch()` -> `SelectFormatterNotUsingAcceptHeaders()`
@dougbu
Copy link
Contributor Author

dougbu commented Oct 16, 2015

@rynowak know you've signed off. But I'd like your opinion on method renaming in new 3rd commit.

Addressed other comments as well.

dougbu added a commit that referenced this pull request Oct 16, 2015
@dougbu
Address PR comments for 2e2043f (PR #3317)
9b00461 
- remove redundant `null` check in `InputFormatter`
- improve comments
- rename `ObjectResult.SelectFormatterBasedOnTypeMatch()` -> `SelectFormatterNotUsingAcceptHeaders()`
@dougbu
Copy link
Contributor Author

dougbu commented Oct 16, 2015

AppVeyor and Travis should fail here. Pushed dougbu/followup.contenttype branch with just 3rd commit on a dev base for real checks.

@rynowak
Copy link
Member

rynowak commented Oct 16, 2015

But I'd like your opinion on method renaming in new 3rd commit.

Name change is a small improvement over what we have IMO. You should go ahead with your changes and we can discuss when you're in the office.

@dougbu
Copy link
Contributor Author

dougbu commented Oct 16, 2015

Comments addressed in 9b00461

@dougbu dougbu closed this Oct 16, 2015
@dougbu dougbu deleted the dougbu/match.contenttype.3138 branch October 16, 2015 04:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dougbu @pranavkm @Eilon @rynowak @dnfclas