MimeTypeFilter instead of allowed MimeType list

[andrewslavin]

Provide more granular control over the MIME types of HTTP responses that should or should not be compressed, via populating ResponseCompressionOptions.MimeTypeFilter, as described here: #121.

[andrewslavin]

@Tratcher @glennc Please have a look.

[andrewslavin]

@Tratcher Thank you for the feedback. I now intend to do the following:

1. Separate logic and storage.
2. Make logic overridable.
3. Make MIME type filter optional when configuring ResponseCompressionOptions, so if you do not configure it, you get the current behaviour.

With point 3 above, will this still be too breaking for 2.1 release? If not, how much time do I have?

[Tratcher]

That would certainly be an improvement. I still need to get this cleared with our PM @glennc.

We're trying to wrap up preview2 next week, after that it will be more difficult to accept new API surface like this.

[andrewslavin]

@Tratcher, @glennc I've made the discussed changes: separated logic from data, made logic overridable, made the filter an optional opt-in. Please have a look.

[andrewslavin]

@Tratcher, @glennc Any update?

[Tratcher]

Thanks for the reminder and the work you've put in so far. The current version still has some design issues we'll need to iterate on. We're busy wrapping up 2.1 and decided we don't have bandwidth to tackle this until 2.2. In the meantime I'll try to give some periodic feedback.

[andrewslavin]

Thanks @Tratcher, all comments make sense and will make this PR much smaller and simpler.

I have one question - regarding StringSegments and MediaTypeHeaderValue.IsSubsetOf(). I do not see how I can use them, and ALSO preserve the current approach of looking up the hashsets using HashSet.Contains. To check if response MIME type is in the hashset, I will have to do something like this:

var separatorPos = mimeType.IndexOf(';');
var mimeTypeStringSegment = new StringSegment(mimeType).Subsegment(0, separatorPos).Trim();
var contains = hashset.Any(mt => mimeTypeStringSegment.Equals(mt, StringComparison.OrdinalIgnoreCase));

I did some simple performance testing. As expected, the speed of allocating a string and calling HashSet.Contains does not depend on the hashset size, while the above approach takes longer as the hashset size increases. Hashset size when both approaches take the same time is ~15 elements.

Do you still want me to proceed with StringSegments using the above implementation? Or is there a different way I'm missing?

[Tratcher]

Don't worry about the string segment too much, if there's an easy way to use it then do so.

[Tratcher]

Much better. Want to update the example to show wildcards as well as the new excludes option?

[andrewslavin]

@Tratcher I have made the requested changes, the whole thing is much simpler now.

On a different note, I was curious about StringSegments. I noticed that you replaced string.Equals() with StringSegment.Equals() in one of the PRs. The arguments to the function are two strings, so no new sting allocations happen in either case, while StringSegment.Equals() results in two StringSegment instances created out of two strings. I did some quick perf. testing, and though both functions are obviously very fast, string.Equals() is 3-4 times faster than StringSegment.Equals() in this case.

[Tratcher]

Where are we using StringSegment.Equals with strings? Can you give a link?

[andrewslavin]

@Tratcher regarding StringSegment.Equals with strings - sorry, my mistake. I was talking about this PR: https://github.com/aspnet/BasicMiddleware/pull/235/files, but one of the two arguments there is actually a StringSegment.

[andrewslavin]

Hi @Tratcher, I've made all the requested changes. Please have a look.

I would also like to provide some feedback about the implementation we ended up with. It tries to fix middleware configurations that we presume to be misconfigurations - namely uses default MIME times if no included MIME types were specified, and ignores excluded */* if included */* was specified. While I see the merit of this approach, as a middleware consumer I personally would prefer the "what you see is what you get" behaviour, i.e. what I specified when configuring options is what will get used. Changing the configured options behind the scenes is unexpected behaviour, and the end result is not necessarily what the consumer might have wanted. A less likely scenario is that the incl. and excl. MIME types come from an external source and/or get calculated using some logic, and the incl. list ends up empty. A more likely scenario is that I want to temporarily turn compression off to investigate some issue, and I do it via setting included MIME types to empty list or null.

I think having MimeTypeFilter separately from MimeTypes was actually a good design. It was providing an "I know what I'm doing and want full control" alternative to the "safe" MimeTypes list approach.

[Tratcher]

Understood on the options fixup. I'd have to dig into the original discussions from when we designed it this way, there were some trade offs we were trying to ballance.

I do like the simplicity of the current design and the code looks good. I've asked @glennc to take a second look at the usability.

[andrewslavin]

Yes, this is very simple and I like that too. If it was not for the two options overrides, I'd be very happy with this implementation. The */* override is new and we can back out of it, but we cannot back out of the empty list override. That's the only reason I'd consider MimeTypeFilter. We can significantly simplify it, like you already mentioned in your comments.

[andrewslavin]

Hi @Tratcher, any update on this PR?

[Tratcher]

@andrewslavin sorry about the delay. I've rebased this on release/2.2, resolved the conflicts, and merged it. Thanks for the new feature.