Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BodyDecoded() for request and responses #1308

Merged
merged 2 commits into from Jun 6, 2022

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Jun 3, 2022

The new method returns a body and uncompress if it's gzipped.

For some reason I thought that the GetBody() will do the uncomression for me itself. And today I had a new client who sends me compressed requests. So I implemented the method but it should be in the library for simplicity. Also this allows to developers to see it in auto-completion list so they will check it and avoid the mistake that I made.

@stokito stokito force-pushed the uncompress branch 2 times, most recently from d82ada1 to df6f75a Compare Jun 3, 2022
http.go Outdated
// BodyDecoded returns body data and if needed decompress it from gzip, deflate or Brotli.
//
// This method may be used if the response header contains
// 'Content-Encoding' for reading uncompressed request body.
// Use Body for reading the raw request body.
func (req *Request) BodyDecoded() ([]byte, error) {
Copy link
Collaborator

@erikdubbelboer erikdubbelboer Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// BodyDecoded returns body data and if needed decompress it from gzip, deflate or Brotli.
//
// This method may be used if the response header contains
// 'Content-Encoding' for reading uncompressed request body.
// Use Body for reading the raw request body.
func (req *Request) BodyDecoded() ([]byte, error) {
// BodyUncompressed returns the request body and if needed decompresses it.
func (req *Request) BodyUncompressed() ([]byte, error) {

I think this is a better name that better describes what it does. The rest of the comment is also not super useful. If people want to see what exactly it does they can easily look at the source.

Copy link
Contributor Author

@stokito stokito Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I firstly called it Uncompressed but everywhere in specs this called Encoding. I totally fine to call it Uncompressed but still have a feeling that this may confuse peoples who don't expect compressed requests/responses. Ideally the method must be a default in documentation and only those who know what they are doing may use the plain Body().
Maybe call it something just like BodyBytes()?

Now I think that also for the BodyBytes() it may be good to add complimentary method BodyString() that internally will convert charset if it's not UTF8. The string conversion is very popular thing but everyone just ignores a charset which sometimes may be dangerous. Also internally the method may use the b2s() method.

Copy link
Contributor Author

@stokito stokito Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is not useful but still it will be shown in IDE when hover over the method. In fact this comment is similar to BodyInflate(). Please feel free to change the comment as you wish.

Copy link
Collaborator

@erikdubbelboer erikdubbelboer Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of Uncompressed because that's what is used in net/http as well: https://pkg.go.dev/net/http#Response.Uncompressed

Doing b2s and returning a string is a really bad idea as people will keep references to the string which is then going to change once the handler returns.

BodyBytes is probably confusing as Body also returns bytes.

Doing the UTF8 conversion is not something for fasthttp. To be honest if you're dealing with UTF8 conversions you shouldn't be using fasthttp!. fasthttp is really only for super high QPS endpoints like advertising RTB or high frequency trading. It's not meant to be a general http framework for the open web.

Copy link
Contributor Author

@stokito stokito Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a goo idea to reuse terminology. The Uncompressed in the net/http works differently. How about BodyData()? This looks more precise for me. At least less confusing than BodyUncompressed(). Also easier to remember (decompressed? decoded? ah! Uncompressed!). And shorter :)

Another option is As/To notation similar to what is used in Java's OkHttp, new HttpClient or Spring WebClient.ResponseSpec :

  • BodyAsBytes() // decompress if needed
  • BodyAsString() // many will say thank you if this covers decompression+charset
  • BodyAsJson(*struct) // unmarshal with encode/json
  • BodyAsFile() - upload to tmp, return the file
  • BodyAs(Decoder) // unmarshal using encoding/gob

Having such short methods that covers 90% of basic needs allows to write more idiomatic, simpler and mostly faster code. Especially this simplifies writing of non critical code like unit tests.

Copy link
Contributor Author

@stokito stokito Jun 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should create the EnableUncompression option as in the net/http? But there it's already enabled so it's called conversely Transport.DisableCompression.
Also in this case we may do similarly and remove the Content-Encoding and Content-Length headers to avoid confusion.

Users may keep their code unchanged and if needed simply add the option. As a downside they may not notice the option and get an error once they start receiving compressed requests and responses (this shouldn't happen if Accept-Encoding missing).

Ideally we may make the fasthttp works the same as the net/http and enable uncompressing by default. This theoretically may have impact because some users may not check the CE header and always decompress it. I think that this is a low possibility of this error.
The change may be done by increasing minor version to let developers know. Also inside of the BodyInflate(), BodyUnbrotli() we may add a flag to avoid double uncompressing.
What do you think about this?

Copy link
Collaborator

@erikdubbelboer erikdubbelboer Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely not enable uncompressing by default as it could heavily impact the performance of some users. And bumping the minor is what we always do. And we can't bump the major version number with how Go works.

I still think BodyUncompressed is the cleanest and most descriptive option. BodyDecoded just sounds like it does things like remove URL encoding. And things like BodyAsString will just help users shoot their own foot even easier. For BodyAsJson we would have to choose the JSON decoder for the user. For example anyone using encoding/json really shouldn't be using fasthttp!

fasthttp isn't some faster version of net/http. It's for some really specific use cases. 99.9% of the users doing http with Go shouldn't be using fasthttp!

Copy link
Contributor Author

@stokito stokito Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uncompression by default won't affect perf if the CE header is empty. Everything will continue to work as previously. But some users may want to get the raw compressed body e.g. to uncompress themselves or to store it as is on a disk.
For them we may add a BodyRaw() method.

Copy link
Collaborator

@erikdubbelboer erikdubbelboer Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very big use case for fasthttp is for proxies which would never want to uncompress the body. Having their performance degrade by a lot when upgrading is something I don't want to risk. We really can't change this and have backwards incompatibility.

stokito added 2 commits Jun 5, 2022
For Response the CE header is stored into a separate field because compressed responses are often used.
But for the Request let's just peek and store it from headers map
The new method returns a body and uncompress if it's gzipped
@stokito stokito requested a review from erikdubbelboer Jun 5, 2022
@erikdubbelboer erikdubbelboer merged commit 35aca7b into valyala:master Jun 6, 2022
14 checks passed
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Jun 6, 2022

Thanks!

@stokito stokito deleted the uncompress branch Jun 8, 2022
bbenzikry pushed a commit to bbenzikry/fasthttp that referenced this issue Sep 11, 2022
* header.go ContentEncoding() getter and setters

For Response the CE header is stored into a separate field because compressed responses are often used.
But for the Request let's just peek and store it from headers map

* http.go: New BodyUncompressed() method for request and responses

The new method returns a body and uncompress if it's gzipped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants