handle gzipped graphql requests#14391
Conversation
mrnugget
left a comment
There was a problem hiding this comment.
Regarding location: I'm thinking that maybe we don't need your snippet of code and can instead use the gziphandler middlware we use for the extensions endpoint already:
And here's where I'd wrap the GraphQL handler:
The advantage of the gzip handler would be that it encodes/decodes request and response bodies.
|
Cool @mrnugget, I didn't know that helper existed. I think what Chris and I asked ourselves initially when hacking on this was: Do we want to allow that encoding on the entire GraphQL API? It probably won't hurt, but also won't make things necessarily easier to understand. |
Why? |
|
Just because it adds some more complexity, nothing in particular. |
|
Yeah, but where? I think I'm missing something. Do you mean the complexity in code, as in: adding the handler? Or do you mean complexity of usage? |
|
I mean complexity as in
then this path is triggered. Especially because we only use it for one thing but it lives in some global piece of code. But I think it is okay. |
|
I see. We should ask @sourcegraph/cloud whether this is okay, but I agree, I think it's okay. |
|
Stumbled across this PR and ❤️ it, my 2ct: I think this would be great to support and is pretty standard with HTTP. There are all well-defined mechanisms in HTTP for announcing what a client or server supports and what a message is using, and they are commonly used (gzip in particular). As long as we respect/use those I think this is great. If anything it can be frustrating to think "oh, this body is large, so I'm going to send a gzipped body with the appropiate standard HTTP headers" only to discover our backend rejects it. |
| return err | ||
| } | ||
|
|
||
| r.Body = reader |
There was a problem hiding this comment.
I think we need to be careful regarding Closing the original reader. According to the docs the underlying reader won't be closed, only the gzip reader.
There was a problem hiding this comment.
I don't remember we need to close the underlying reader ourselves while using net/http? 🤔
The other problem I'm seeing though, is that we should close the gzip reader with reader.Close(), otherwise it's a resource leak. Not sure if this is what you intended to say?
There was a problem hiding this comment.
Well, now I'm confused. 😄
First, I can confirm that the gzip reader is not being closed. (And if it was closed, it wouldn't close the underlying reader-formerly-known-as-r.Body.) I can wrap the gzip reader so that, if closed, it will close the underlying reader. (But is unknwon saying I don't need to do that at all?)
It sounds like, however, I do need to find a place to close the gzip reader. I assume it's safe to do this after the called to ServeHTTP()?
There was a problem hiding this comment.
(But is unknwon saying I don't need to do that at all?)
I believe so: https://github.com/golang/go/blob/9449a125e8b25d21c0d522435be44a4a6e7af2d3/src/net/http/request.go#L174-L178
It sounds like, however, I do need to find a place to close the gzip reader. I assume it's safe to do this after the called to
ServeHTTP()?
Yes, that's what I'm thinking of.
There was a problem hiding this comment.
OK, it now closes the gzip reader in the latest commit. Thanks!
Codecov Report
@@ Coverage Diff @@
## main #14391 +/- ##
==========================================
- Coverage 51.87% 51.87% -0.01%
==========================================
Files 1535 1535
Lines 78120 78126 +6
Branches 7085 7085
==========================================
+ Hits 40526 40527 +1
- Misses 33935 33941 +6
+ Partials 3659 3658 -1
*This pull request uses carry forward flags. Click here to find out more.
|
This allows the backend to handle gzipped GraphQL requests. But perhaps this is not the right place to do this? I'm curious to know your thoughts.
This partially fixes #12840.