Add ConvertNetHttpRequestToFastHttpRequest adaptor function#2104
Add ConvertNetHttpRequestToFastHttpRequest adaptor function#2104aaydin-tr wants to merge 7 commits intovalyala:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new ConvertNetHttpRequestToFastHttpRequest function to enable conversion from net/http.Request to fasthttp.RequestCtx. This provides the inverse operation of the existing ConvertRequest function, allowing users to adapt standard Go HTTP requests for use with fasthttp.
Key Changes:
- Added new conversion function
ConvertNetHttpRequestToFastHttpRequestthat handles method, URI, protocol, host, headers, body, and remote address conversion - Implemented helper functions
parseRemoteAddrandparsePortto handle remote address parsing - Added comprehensive test suite covering basic conversion, header handling, body streaming, and remote address formats
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| fasthttpadaptor/request.go | Implements the core conversion function and remote address parsing helpers |
| fasthttpadaptor/request_test.go | Adds benchmark and comprehensive unit tests for various conversion scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'm sorry I don't have the time to look into this closely. On an initial glance the function seems kinda small, are you sure it supports all fields and cases (such as the various way a fasthttp request can have a body)? |
if r.Body != nil {
ctx.Request.SetBodyStream(r.Body, int(r.ContentLength))
}this is how i pass net/http request body to fasthttp request body. indeed fasthttp request has various ways to set the body, but as far as i see, |
|
@erikdubbelboer Can you trigger copilot to also check this? |
Body Streams larger than 4GB will fail with this, since those need an |
if r.Body != nil {
contentLength := int(r.ContentLength)
if r.ContentLength >= int64(math.MaxInt) {
contentLength = -1
}
ctx.Request.SetBodyStream(r.Body, contentLength)
}i make this change if |
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@erikdubbelboer is there any update for this pr |
| } | ||
|
|
||
| func parseRemoteAddr(addr string) net.Addr { | ||
| if tcpAddr, err := net.ResolveTCPAddr("tcp", addr); err == nil { |
There was a problem hiding this comment.
There is no timeout on this and it could potentially hang. We have this code here, I'm not sure if it should somehow be reused since it's not exposed:
Line 479 in 3471acf
| // | ||
| // After calling this function, you should treat r.Body as effectively owned by | ||
| // ctx.Request for the lifetime of that context. | ||
| func ConvertNetHTTPRequestToFastHTTPRequest(r *http.Request, ctx *fasthttp.RequestCtx) { |
There was a problem hiding this comment.
The function doesn't reset ctx.Request. This should be documented that users need to do this themselves if needed.
There was a problem hiding this comment.
Also why have a fasthttp.RequestCtx as argument? From the function name I would expect a fasthttp.Request as argument, which makes much more sense and makes it much more usable.
| } | ||
|
|
||
| ctx.Request.Header.SetProtocol(r.Proto) | ||
| ctx.Request.SetHost(r.Host) |
There was a problem hiding this comment.
r.Host can be empty in which case it should be r.URL.Host.
| addr := parseRemoteAddr(r.RemoteAddr) | ||
| ctx.SetRemoteAddr(addr) | ||
| } | ||
| } |
There was a problem hiding this comment.
You are missing some properties that aren't being copied right now. For example r.TLS and r.Close, r.TransferEncoding, r.Trailer, and r.URL.Scheme which are all used by net/http.
One of my personal project i need to convert net http request to fasthttp request instead of doing it directly in my own project i want to add this functionality to fasthttp for possible future uses and i also looking for a review if its correct way to convert both request object.