Aspnetcore: Improve the implementation on MediaType.cs

[javiercn]

In general, consumers of this type should use methods provided by the type like "IsSubsetOf" and these methods should be able to correctly handle incorrect input. Having an instance of MediaType doesn't mean you have a valid MediaType, it can be default(MediaType) or it might be a media type with valid type but invalid subtype, valid type and subtype and invalid parameter.

Go through the algorithms in this type (GetParameter, IsSubsetOf, etc) and make sure they return the appropriate result against invalid input (null and false for the previous cases). (For that, we need to iterate to the end of the parameter set or consider "recovery strategies" for relaxed parsing rules of our header (given the amount of invalid clients out there)

We should add more tests to cover scenarios with invalid media types and we should make sure that the q parameter is handled appropriately.

Finally, we should consider adding an IsValid computed property on MediaType that checks that Type != default(StringSegment), SubType != default(StringSegment) and that iterates through all the parameters and makes sure that they don't fail.

The goal of this type is to not allocate when possible and to be as lazy as possible as it's on a performance critical path. If we need to add more fields to this struct, (for example to cache the validity of a media type) we should measure the performance impact on the type.

The motivation for this bug is that we've seen some bugs in this area and inspection of the code has revealed some inconsistencies in different method implementations for the criteria specified above (some methods handle only correct input and have an "undefined/variable" behavior when it comes to incorrect input. (Usually considering the MediaType valid as long as the input is good "so far")

[dougbu]

Why not have the MediaType constructor leave its properties at default values unless the whole thing is valid? The other methods (e.g. IsSubsetOf()) already work reasonably for default(MediaType).

[dougbu]

Put another way, this struct does not have a clear (or usable) invariant w.r.t. validity where validity covers the type, subtype and all parameters. It's something like "the media type string passed to the constructor was completely valid iff CreateMediaTypeSegmentWithQuality() returns a MediaTypeSegmentWithQuality with MediaType.Length > 0." Why isn't the invariant "… valid iff its Type.Length > 0"?

[Eilon]

@javiercn / @dougbu / @jkotalik / @Tratcher - is there much left to do here now that we're unifying on a single implementation of this code in HttpAbstractions? If so, what?

[dougbu]

If MVC and HttpAbstractions have similar validation restrictions (or HttpAbstractions provides the APIs we need to maintain the MVC vigilance) and the new code we'll be using makes it as easy or easier to check validity, all is well and we could close this bug.

I believe the HttpAbstractions code mostly throws in the various constructors when given invalid input. Communicating via exceptions is ugly but it's at least easy to use.

At the moment however, the HttpAbstractions code has a loose idea of validity (see aspnet/HttpAbstractions#923 (note the title is more specific than the problem). I don't remember if MediaType is similarly loose. If there's a mismatch, aspnet/HttpAbstractions#923 might be an MVC adoption blocker.

[jkotalik]

@dougbu it seems that the MediaType itself cannot contain a quoted string, but we would need to watch out for parameters. The next step will be to reimplement MediaType as a wrapper around MediaTypeHeaderValue. There are a few small differences at this point, like here and here. Changing MediaType to MediaTypeHeaderValue would require major code changes across MVC, but this intermediate step would let us get perf and test feedback.

[javiercn]

@jkotalik Come find me tomorrow and we'll figure out what to do together

[mkArtakMSFT]

Closing as dupe https://github.com/aspnet/AspNetCore/issues/4850