nytimes / gziphandler

Go middleware to gzip HTTP responses
https://godoc.org/github.com/NYTimes/gziphandler
Apache License 2.0
868 stars 129 forks source link

Whitelist by mime type #68

Closed meirf closed 6 years ago

meirf commented 6 years ago

ContentTypes(types []string) option whitelists by the full content type. In my case, the encoding and boundary don't matter to me. All I care about is the MIME type. I'd rather not have to declare all the encodings for each MIME type.

Is there an interest in accepting a PR which adds something like one of these:

This would use the standard method for parsing the mime type.

(@jprobinson, @adammck do you have thoughts on this?)

meirf commented 6 years ago

@tmthrgd, do you have an opinion on this?

jprobinson commented 6 years ago

Could we perhaps make just 1 new option, func AcceptsGzip(func(*http.Request) bool) option and then folks can do whatever they please within it?

adammck commented 6 years ago

I agree that we should have an Accepts option as @jprobinson suggests, but I suspect that the most common usage of that would be to implement MimeTypes([]string). So perhaps we should just add that option, too. In hindsight, that seems more useful than ContentTypes. Maybe deprecate that.

jprobinson commented 6 years ago

From what our wise friend @adammck says, I think the MimeTypes(types []string) option would be worth bringing in but if you have the will to make all of the above, go for it.

tmthrgd commented 6 years ago

I'd be in favour of this. :+1:

In my fork this is how I chose to implement ContentTypes already. (See here and here).

It looks like the existing behaviour is somewhat broken anyway. text/html;charset=utf-8 won't match text/html; charset=utf-8 (note the space), yet they are semantically identical.

I also can't see a situation where anyone would depend on, say, compressing charset=utf-8 but not compressing charset=us-ascii.

My one concern is the proliferation of exported symbols in this package (compare NYTimes/gziphandler with tmthrgd/gziphandler). Considering the documentation of ContentTypes doesn't explicitly state it's behaviour, perhaps it would be acceptable to change ContentTypes to strip the directives and whitespace before comparisions.

That's my 2c anyway.

meirf commented 6 years ago

I’ve uploaded a PR with what I think are the best ideas y’all have presented: not changing the api, backwards compatible, ignoring white space and allowing mime-only comparison.

One thing I didn’t include was subtype catch-all comparisons, e.g. "text/*". Though some people might not differentiate between text/plain and text/html, I think some would. But more importantly this type of pattern leaves the comfort of the standard library’s parsing and has much higher likelihood of introducing bugs in our code and in clients'. I think MIME type is a happy medium between what we had before and ignore subtype, but if you disagree with this, I'm happy to change the PR. Also, I think the subtype thing is something we could add later without breaking what's in this new PR.

meirf commented 6 years ago

Merged https://github.com/NYTimes/gziphandler/pull/69 Thanks!