Rewrite HTTP Get options #482
Conversation
jchadwick-buf
commented
Mar 21, 2023
•
jchadwick-buf
commented
- No longer a requirement to specify URL size limit
- Separate URL size limit option adds fallback parameter
There was a problem hiding this comment.
I have some line-level comments, but looking at this diff overall it seems that we may have overcomplicated the exported API. (That's probably my fault.)
We want a simple way for clients to opt into GETs. We also want a way to cap the size of GETs, with the default being "send any size request." Finally, we want clients to be able to require GETs, with no POST fallback (primarily for tests).
I think we can get away with just WithHTTPGet() and WithMaxGetURLSize(bytes int, fallback bool). This is super-simple to opt into, and defaults to unlimited URL size. Clients can configure a max size if necessary, and they can indicate whether they'd like to fall back to POST or not when they exceed the max. I think colocating the max and fallback will also clarify the docs and guide the user a bit more effectively.
jchadwick-buf
changed the title
Along with a bit of copy-editing, clarify that all the GET-related options apply only to the Connect protocol.
Minor nit - this is part of the house style guide.
Reverse the conditional to reduce nesting.
Make three small readability improvements to `marshalWithGet`: - Factor out a named `isTooBig` bool. - Add inline comments for `buildGetURL`'s naked bool param. - For users, add some guidance on how to resolve message-too-big errors.
We should assume that compressed messages are binary and need base64-encoding.
Responses to GET requests differ depending on the client's Accept-Encoding header, so we should help the user out and automatically set Vary on responses.
There was a problem hiding this comment.
LGTM - thanks for bearing with me through some irritating changes of direction, @jchadwick-buf. I pushed some copy edits and Go nit fixes, along with two small-but-substantive changes:
- In bufbuild@a552844, I made
buildGetURLbase64-encode all compressed messages (since they're probably binary). - In bufbuild@432894f, I had unary Connect-protocol handlers automatically set the Vary response header (since the response content changes depending on the client's Accept-Encoding). This felt like a nice quality-of-life improvement - if users want to enable HTTP caching, they should now only need to worry about the request headers they're using in their application logic.
If the commits I've added look good to you, this PR is ready to merge.
This reverts commit f4ed49d. I'm a dummy.
|
@akshayjshah I wound up adding a couple more changes:
Otherwise, it's good to go from my end. |
|
Nice final catches, John! The base64 dialect is a big one :)
Good point. They're only used in the connect protocol implementation, so I moved the query param stuff to the bottom of |
