fix(storage): resolve endpoint parsing and properly manage http client#14446
fix(storage): resolve endpoint parsing and properly manage http client#14446
Conversation
* Resolves duplicated logic by extracting endpoint verification to an `ensureEndpoint` function * Ensures the previous HTTP client connections are closed by invoking `hc.CloseIdleConnections()` before creating a new one * Fixed vet test format issues * Title properly follows fix(storage): standard Co-authored-by: cpriti-os <202586561+cpriti-os@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request introduces an ensureEndpoint utility to automatically fix schemeless endpoints by prepending "https://" and ensuring the "/storage/v1/" path suffix. This logic is integrated into NewClient and newHTTPStorageClient to trigger a redial when the endpoint is modified. Feedback suggests decoupling the path suffix logic from the scheme check to ensure that endpoints provided with a scheme but without the storage path are also correctly formatted.
| if !strings.Contains(ep, "://") { | ||
| ep = "https://" + ep | ||
| if !strings.Contains(ep, "/storage/v1") { | ||
| if !strings.HasSuffix(ep, "/") { | ||
| ep += "/" | ||
| } | ||
| ep += "storage/v1/" | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for ensuring the /storage/v1/ path suffix is currently nested inside the scheme check. This means that if a user provides an endpoint with a scheme but without the path (e.g., https://my-proxy.com), the path suffix will not be appended, which may cause the client to fail to construct valid API requests. Additionally, the trailing slash is not ensured if the path is already present but lacks it (e.g., my-proxy.com/storage/v1).
Consider moving the path check outside the scheme check and ensuring a trailing slash is always present to maintain consistency and correctness for the underlying REST client.
if !strings.Contains(ep, "://") {
ep = "https://" + ep
}
if !strings.Contains(ep, "/storage/v1") {
if !strings.HasSuffix(ep, "/") {
ep += "/"
}
ep += "storage/v1/"
}
if !strings.HasSuffix(ep, "/") {
ep += "/"
}
PR title: fix(storage): resolve endpoint parsing and properly manage http client
ensureEndpointinternal utility function.CloseIdleConnections()for re-dialed HTTP connections.PR created automatically by Jules for task 12433437419531665012 started by @cpriti-os