feat: allow passing URL as X-Sendfile header value
#99
Conversation
WalkthroughThe recent update enhances the way our plugin manages file operations. It now discerns between HTTP URLs and local file paths, fetching files over the web or from the local storage accordingly. Furthermore, it introduces smarter buffer handling by adjusting the allocation size based on the file's size, optimizing memory usage and performance. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if strings.HasPrefix(path, "http") { | ||
| resp, err := http.Get(path) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| f, err := os.OpenFile(path, os.O_RDONLY, 0) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| defer func() { | ||
| _ = f.Close() | ||
| }() | ||
| if resp.StatusCode != http.StatusOK { | ||
| http.Error(w, http.StatusText(resp.StatusCode), resp.StatusCode) | ||
| return | ||
| } | ||
|
|
||
| f = resp.Body | ||
| size = resp.ContentLength | ||
| } else { | ||
| // do not allow paths like ../../resource, security | ||
| // only specified folder and resources in it | ||
| // see: https://lgtm.com/rules/1510366186013/ | ||
| if strings.Contains(path, "..") { | ||
| w.WriteHeader(http.StatusForbidden) | ||
| return | ||
| } | ||
|
|
||
| // check if the file exists | ||
| fs, err := os.Stat(path) | ||
| if err != nil { | ||
| http.Error(w, "not found", http.StatusNotFound) | ||
| return | ||
| } | ||
|
|
||
| f, err = os.OpenFile(path, os.O_RDONLY, 0) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| defer func() { | ||
| _ = f.Close() | ||
| }() | ||
|
|
||
| size = fs.Size() | ||
| } |
There was a problem hiding this comment.
Consider using http.Client with a timeout instead of http.Get to avoid hanging requests.
- resp, err := http.Get(path)
+ client := &http.Client{Timeout: time.Second * 10}
+ resp, err := client.Get(path)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if strings.HasPrefix(path, "http") { | |
| resp, err := http.Get(path) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| f, err := os.OpenFile(path, os.O_RDONLY, 0) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| defer resp.Body.Close() | |
| defer func() { | |
| _ = f.Close() | |
| }() | |
| if resp.StatusCode != http.StatusOK { | |
| http.Error(w, http.StatusText(resp.StatusCode), resp.StatusCode) | |
| return | |
| } | |
| f = resp.Body | |
| size = resp.ContentLength | |
| } else { | |
| // do not allow paths like ../../resource, security | |
| // only specified folder and resources in it | |
| // see: https://lgtm.com/rules/1510366186013/ | |
| if strings.Contains(path, "..") { | |
| w.WriteHeader(http.StatusForbidden) | |
| return | |
| } | |
| // check if the file exists | |
| fs, err := os.Stat(path) | |
| if err != nil { | |
| http.Error(w, "not found", http.StatusNotFound) | |
| return | |
| } | |
| f, err = os.OpenFile(path, os.O_RDONLY, 0) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| defer func() { | |
| _ = f.Close() | |
| }() | |
| size = fs.Size() | |
| } | |
| if strings.HasPrefix(path, "http") { | |
| client := &http.Client{Timeout: time.Second * 10} | |
| resp, err := client.Get(path) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| http.Error(w, http.StatusText(resp.StatusCode), resp.StatusCode) | |
| return | |
| } | |
| f = resp.Body | |
| size = resp.ContentLength | |
| } else { | |
| // do not allow paths like ../../resource, security | |
| // only specified folder and resources in it | |
| // see: https://lgtm.com/rules/1510366186013/ | |
| if strings.Contains(path, "..") { | |
| w.WriteHeader(http.StatusForbidden) | |
| return | |
| } | |
| // check if the file exists | |
| fs, err := os.Stat(path) | |
| if err != nil { | |
| http.Error(w, "not found", http.StatusNotFound) | |
| return | |
| } | |
| f, err = os.OpenFile(path, os.O_RDONLY, 0) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| defer func() { | |
| _ = f.Close() | |
| }() | |
| size = fs.Size() | |
| } |
| fs, err := os.Stat(path) | ||
| if err != nil { | ||
| http.Error(w, "not found", http.StatusNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing error handling for os.Stat to distinguish between different types of errors (e.g., permission issues).
- if err != nil {
+ if os.IsNotExist(err) {
+ http.Error(w, "not found", http.StatusNotFound)
+ } else if err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| fs, err := os.Stat(path) | |
| if err != nil { | |
| http.Error(w, "not found", http.StatusNotFound) | |
| return | |
| } | |
| fs, err := os.Stat(path) | |
| if os.IsNotExist(err) { | |
| http.Error(w, "not found", http.StatusNotFound) | |
| } else if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| } |
| for { | ||
| n, err := f.ReadAt(buf, int64(off)) | ||
| n, err := f.Read(buf) |
There was a problem hiding this comment.
Use of goto for control flow is generally discouraged due to readability and maintainability concerns.
- if n > 0 {
- goto out
- }
+ if n > 0 {
+ _, err = w.Write(buf[:n])
+ if err != nil {
+ p.log.Error("write response", zap.Error(err))
+ return
+ }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| n, err := f.Read(buf) | |
| if n > 0 { | |
| _, err = w.Write(buf[:n]) | |
| if err != nil { | |
| p.log.Error("write response", zap.Error(err)) | |
| return | |
| } | |
| } |
Error handling inside the loop does not account for non-EOF errors properly, potentially leading to silent failures.
- if errors.Is(err, io.EOF) {
+ if err != nil {
+ if !errors.Is(err, io.EOF) {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ return
+ }
+ if n > 0 {
+ _, err = w.Write(buf[:n])
+ if err != nil {
+ p.log.Error("write response", zap.Error(err))
+ return
+ }
+ }
+ break
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| n, err := f.Read(buf) | |
| if err != nil { | |
| if !errors.Is(err, io.EOF) { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| if n > 0 { | |
| _, err = w.Write(buf[:n]) | |
| if err != nil { | |
| p.log.Error("write response", zap.Error(err)) | |
| return | |
| } | |
| } | |
| break | |
| } |
rustatian
changed the title
URL as X-Sendfile header value
|
Hey @rmikalkenas 👋 Thank you for the PR 👍 |
@rustatian I'm trying to understand how would it work.. Could you elaborate more on your idea? |
|
Sure @rmikalkenas 😃 But, I guess since the payload should be streamed as the response... Not sure now. I mean, is that something you currently need to solve some needs using RR at work? |
|
Since we already discussed this functionality, do you @rmikalkenas still need this PR to be opened? |
Reason for This PR
A POC of discussion: https://github.com/orgs/roadrunner-server/discussions/1842
Probably it would be good to abstract if/else block and have some sort of generic struct, but since it's a POC - haven't done it yet
Description of Changes
Allow to pass url as X-Sendfile header value and let roadrunner stream files.
Usage case: backend app generates presigned S3 url and responds with X-Sendfile header, instead of streaming file by itself.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.