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

add Request.CloneWithBody and Request.SetBody #105

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Conversation

joeshaw
Copy link
Member

@joeshaw joeshaw commented Mar 7, 2024

These methods wrap provided io.Readers that aren't already also closers in our own nopCloser type, which we already do in NewRequest. This extends our ability to send Content-Length with certain in-memory types to other uses of requests.

We note in the documentation now that using SetBody is preferred over setting the Body field directly.)

Fixes #104

@joeshaw
Copy link
Member Author

joeshaw commented Mar 7, 2024

I wonder if we should also have a SetBody method and prefer that to assigning to .Body directly for the same reason?

Copy link
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

I just made the change in my code to use Clone yesterday, and almost forgot the post body. This is timely!

@cee-dub
Copy link
Collaborator

cee-dub commented Mar 8, 2024

I prefer your suggestion of exposing the type, with a clear doc string explaining how it enables the content-length header. That leaves the author with the most flexibility.

@joeshaw
Copy link
Member Author

joeshaw commented Mar 8, 2024

@cee-dub My concern about adding fsthttp.NopCloser is that it adds an additional type you need to care about and think about when to use it vs. io.NopCloser. I think SetBody gets us the same benefits with just an additional method. I think we can convey the same meaning in the godoc on both the Body field and SetBody method.

@joeshaw joeshaw changed the title add Request.CloneWithBody add Request.CloneWithBody and Request.SetBody Mar 8, 2024
@joeshaw
Copy link
Member Author

joeshaw commented Mar 8, 2024

I pushed a replacement commit that adds SetBody. You could make the argument that we no longer need CloneWithBody now.

These methods wrap provided `io.Reader`s that aren't already also
closers in our own `nopCloser` type, which we already do in NewRequest.
This extends our ability to send `Content-Length` with certain in-memory
types to other uses of requests.

We note in the documentation now that using `SetBody` is preferred over
setting the `Body` field directly.)

Fixes #104
Comment on lines +223 to +227
// If the body is set from an in-memory reader such as [bytes.Buffer],
// [bytes.Reader], or [strings.Reader], the runtime will send the
// request with a Content-Length header instead of Transfer-Encoding:
// chunked.
func (req *Request) SetBody(body io.Reader) {
Copy link
Collaborator

@cee-dub cee-dub Mar 8, 2024

Choose a reason for hiding this comment

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

A bit of a learning question here, but is it possible to have the case of an incoming request with a content-length brought to this body assignment as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Realized this isn't super clear, but I think Clone already handles this by copying the header if present.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but the runtime will remove Content-Length and Transfer-Encoding before sending the request unless ManualFramingMode is true.

Comment on lines +257 to +259
// CloneWithBody returns a copy of the request, with the Body field set
// to the provided io.Reader. Its URL will have a nil User field.
func (req *Request) CloneWithBody(body io.Reader) *Request {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree this could be removed now, perhaps by including the two lines of code into the doc of SetBody.

@joeshaw joeshaw merged commit 1a69131 into main Mar 26, 2024
12 checks passed
@joeshaw joeshaw deleted the joeshaw/clone-with-body branch March 26, 2024 18:23
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.

Add Request.CloneWithBody(io.Reader)
2 participants