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

Fix content negotiation content-type check for InputFormatter #3138

Closed
andresmoschini opened this issue Sep 15, 2015 · 10 comments
Closed

Fix content negotiation content-type check for InputFormatter #3138

andresmoschini opened this issue Sep 15, 2015 · 10 comments
Assignees
Milestone

Comments

@andresmoschini
Copy link

From InputFormatter.cs

public virtual bool CanRead(InputFormatterContext context)
{
    // . . .
    return SupportedMediaTypes
        .Any(supportedMediaType => supportedMediaType.IsSubsetOf(requestContentType));
}

If I am not wrong, to be able to read a request, requestContentType should be a subset of any of InputFormatter's SupportedMediaTypes.

@andresmoschini
Copy link
Author

For example, I set:

formatter.SupportedMediaTypes.Add(new MediaTypeHeaderValue("*/*"));

and I get Unsupported content type 'text/plain; charset=UTF-8'..

But it works if I set:

formatter.SupportedMediaTypes.Add(new MediaTypeHeaderValue("text/plain"));

Is this behavior expected?

@dougbu
Copy link
Contributor

dougbu commented Sep 15, 2015

It's possible there's a bug in how IsSubsetOf() handles */* however the real answer for your scenario is to override CanRead() to return true since that seems to be what you want.

@dougbu
Copy link
Contributor

dougbu commented Sep 15, 2015

Put another way, InputFormatter handles a specific selection of concrete formatters. A catch-all formatter should probably implement IInputFormatter directly.

@andresmoschini
Copy link
Author

Thanks @dougbu, but it was only an example.

I think that an InputFormatter should be capable to read all request with content-type that are subtypes of SupportedMediaTypes. Am I missing something?

@dougbu
Copy link
Contributor

dougbu commented Sep 15, 2015

Do you have a concrete example that fails which doesn't involve */*? Better yet do you have a small repro project we could use to investigate what you're seeing -- even if with */*?

Note what you're doing with the catch-all formatter (i.e. that accepts */*) should work. My alternate implementation was based on the semantics you need rather than a thought your issue was invalid.

@andresmoschini
Copy link
Author

Thanks for your help @dougbu, since I need to ignore request content type, in my project I will override CanRead() as you mention.

BUT, I am pretty sure that I am not understanding how to InputFormaters deal with content-type header, OR there is a mistake in the implementation.

It is not necessary a small project because there are already a test.

In my opinion that test is wrong because it says that if request's content-type is application/* should be accepted by JsonInputFormatter. It seems to be valid for accept header, but not for content-type.

CC: @harshgMSFT

@danroth27 danroth27 added this to the 6.0.0-beta8 milestone Sep 16, 2015
@danroth27
Copy link
Member

Looks like the IsSubset check is backwards - we should check that the request media type is a subset of the supported media type, not the other way around.

@dougbu
Copy link
Contributor

dougbu commented Sep 23, 2015

Yes, the check is incorrect. Some non-exact matches start working after reversing it.

None of the in-box input formatters support wildcard media types or subtypes. This reduces the impact slightly.

Confirmed the Web.API 3 check matches what we have in MVC 6. See that version's MediaTypeFormatterCollection.FindReader() and HttpContentExtensions.ReadAsync(). We carried this bug forward.

@dougbu dougbu modified the milestones: 6.0.0-rc1, 6.0.0-beta8 Sep 25, 2015
@dougbu
Copy link
Contributor

dougbu commented Sep 25, 2015

Moving to early in RC1.

The reversed check hid a couple of other bugs. While I have an implementation that covers most issues, we've revised the design to improve correctness, especially around formatter selection. Will take a bit more work...

dougbu added a commit to aspnet/HttpAbstractions that referenced this issue Oct 13, 2015
- aspnet/Mvc#3138 part 1/2
  - check parameters with same polarity as type and subtype
    - ignore quality factors
  - bug was obscured because MVC has no formatters supporting wildcard media types

nits:
- add doc comments
- spelling
- correct typo in a `project.json` file
dougbu added a commit that referenced this issue Oct 13, 2015
…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 added a commit to aspnet/HttpAbstractions that referenced this issue Oct 14, 2015
- aspnet/Mvc#3138 part 1/2
  - check parameters with same polarity as type and subtype
    - ignore quality factors
  - bug was obscured because MVC has no formatters supporting wildcard media types

nits:
- add doc comments
- spelling
- correct typo in a `project.json` file
dougbu added a commit that referenced this issue Oct 14, 2015
…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

dougbu commented Oct 14, 2015

@dougbu dougbu closed this as completed Oct 14, 2015
@Eilon Eilon changed the title content-type check seems to be wrong in InputFormatter Fix content negotiation content-type check for InputFormatter Nov 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants