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

How can I manually configure *resty.Request.RawRequest.ContentLength? #971

Open
BoneAsh opened this issue Feb 7, 2025 · 11 comments
Open
Assignees

Comments

@BoneAsh
Copy link

BoneAsh commented Feb 7, 2025

go 1.23.6
resty v3

I tested it using the net/http package, and when I send an HTTP request with the body as an io.Reader type, setting the Content-Length in the header does not work. I need to set req.ContentLength directly.

util.MyReader is a custom struct that implements the io.Reader interface, specifically the Read(p []byte) (n int, err error) method.

req, err := http.NewRequest("PUT", url, &util.MyReader{Data: []byte{0x00, 0x00}})
if err != nil {
	panic(err)
}

req.ContentLength = 2 // it works
#req.Header.Set("Content-Length", "2")  // it does not work

resp, err := client.Do(req)
if err != nil {
	panic(err)
}
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
if err != nil {
	panic(err)
}
@BoneAsh
Copy link
Author

BoneAsh commented Feb 7, 2025

Typically, net/http will also automatically set req.ContentLength, but this is limited to when the body type is *bytes.Buffer, *bytes.Reader, or *strings.Reader.
net/http/request.go#L923

if body != nil {
switch v := body.(type) {
case *bytes.Buffer:
	req.ContentLength = int64(v.Len())
	buf := v.Bytes()
	req.GetBody = func() (io.ReadCloser, error) {
		r := bytes.NewReader(buf)
		return io.NopCloser(r), nil
	}
case *bytes.Reader:
	req.ContentLength = int64(v.Len())
	snapshot := *v
	req.GetBody = func() (io.ReadCloser, error) {
		r := snapshot
		return io.NopCloser(&r), nil
	}
case *strings.Reader:
	req.ContentLength = int64(v.Len())
	snapshot := *v
	req.GetBody = func() (io.ReadCloser, error) {
		r := snapshot
		return io.NopCloser(&r), nil
	}
default:
	// This is where we'd set it to -1 (at least
	// if body != NoBody) to mean unknown, but
	// that broke people during the Go 1.8 testing
	// period. People depend on it being 0 I
	// guess. Maybe retry later. See Issue 18117.
}
// For client requests, Request.ContentLength of 0
// means either actually 0, or unknown. The only way
// to explicitly say that the ContentLength is zero is
// to set the Body to nil. But turns out too much code
// depends on NewRequest returning a non-nil Body,
// so we use a well-known ReadCloser variable instead
// and have the http package also treat that sentinel
// variable to mean explicitly zero.
if req.GetBody != nil && req.ContentLength == 0 {
	req.Body = NoBody
	req.GetBody = func() (io.ReadCloser, error) { return NoBody, nil }
}

@jeevatkm
Copy link
Member

jeevatkm commented Feb 7, 2025

@BoneAsh Thanks for trying out v3 and sharing your feedback.

Typically, net/http will also automatically set req.ContentLength, but this is limited to when the body type is *bytes.Buffer, *bytes.Reader, or *strings.Reader.

This is true for Resty v3


Regarding the Content-Length value, I will validate the resty v3 and get back to you.

@jeevatkm jeevatkm self-assigned this Feb 7, 2025
@jeevatkm jeevatkm added this to the v3.0.0 Milestone milestone Feb 7, 2025
@jeevatkm
Copy link
Member

jeevatkm commented Feb 7, 2025

@BoneAsh, I’ve identified the root cause. It’s a side effect of removing the v2 flow. I’ll add the new content-length flow for explicit setting of a value. Once ready, you could try the branch before I create a PR.

@jeevatkm
Copy link
Member

@BoneAsh The implementation is available in this branch set-content-length. Can you try it out with various approaches and share your feedback on content length?

@BoneAsh
Copy link
Author

BoneAsh commented Feb 11, 2025

@BoneAsh The implementation is available in this branch set-content-length. Can you try it out with various approaches and share your feedback on content length?

ok, I will try it out.

@BoneAsh
Copy link
Author

BoneAsh commented Feb 11, 2025

This issue has been fixed. I used the Resty library to call the PUT method to upload file slices to MinIO, and the body uses my own implementation of *io.Reader. The Content-Length value is already set in the headers. Thank you very much! You can merge this modification into the main branch of version 3. @jeevatkm

@BoneAsh BoneAsh closed this as completed Feb 11, 2025
@jeevatkm
Copy link
Member

@BoneAsh Thanks for the feedback. I need to do some final touches before the PR creation.

@jeevatkm jeevatkm reopened this Feb 12, 2025
@BoneAsh
Copy link
Author

BoneAsh commented Feb 13, 2025

@jeevatkm I found that when the body is multipart/form-data, the Content-Length is still problematic. I pinpointed the code at line 219 in middleware.go, where r.Body is nil, so the Content-Length is set directly to 0. However, at this point, the data is in r.bodyBuf, and this code is not executed. Moreover, the length of the body cannot be calculated from the outside because the boundary is generated by the Resty package itself. So please check if there is a logical issue here.

middleware.go#L219

if r.Body == nil {
	r.ContentLength = 0
} else if r.bodyBuf != nil {
	r.ContentLength = int64(r.bodyBuf.Len())
} else if b, ok := r.Body.(*bytes.Reader); ok {
	r.ContentLength = int64(b.Len())
}

@BoneAsh
Copy link
Author

BoneAsh commented Feb 13, 2025

I am not sure if there are cases where both r.Body and r.bodyBuf have values, so please conduct compatibility tests for all body types and combinations, such as SetMultipartFormData + SetMultipartField

@BoneAsh
Copy link
Author

BoneAsh commented Feb 13, 2025

@jeevatkm I found that when the body is multipart/form-data, the Content-Length is still problematic. I pinpointed the code at line 219 in middleware.go, where r.Body is nil, so the Content-Length is set directly to 0. However, at this point, the data is in r.bodyBuf, and this code is not executed. Moreover, the length of the body cannot be calculated from the outside because the boundary is generated by the Resty package itself. So please check if there is a logical issue here.

middleware.go#L219

if r.Body == nil {
	r.ContentLength = 0
} else if r.bodyBuf != nil {
	r.ContentLength = int64(r.bodyBuf.Len())
} else if b, ok := r.Body.(*bytes.Reader); ok {
	r.ContentLength = int64(b.Len())
}

I just realized that when the body is multipart/form-data, it is not possible to set the Content-Length using the SetContentLength method. However, this is indeed correct; the length should be calculated by Resty.

@jeevatkm
Copy link
Member

jeevatkm commented Feb 13, 2025

@BoneAsh I'm in the process of further testing since your last feedback. FYI, Resty v3 by default does streaming for multipart upload cases, so when streaming happens Resty cannot figure out the content length, so it is -1. I already covered the non-files multipart cases for content length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants