feat: add request/response middleware pipeline - #2247
Conversation
Add a 4-phase middleware pipeline (Initialize → Serialize → Finalize → Deserialize) matching AWS SDK middleware.Stack. Users inject custom logic into the S3 request lifecycle. Signed-off-by: Amr-Shams <amr.shams2015.as@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a configurable S3 request middleware pipeline with lifecycle hooks for initialization, serialization, finalization, and deserialization. It also adds integration coverage and reformats existing calls and error expressions without changing their behavior. ChangesMiddleware Pipeline
Formatting-only cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant executeMethod
participant InitializeMiddleware
participant newRequest
participant SerializeMiddleware
participant FinalizeMiddleware
participant HTTPServer
participant DeserializeMiddleware
Caller->>executeMethod: call S3 operation
executeMethod->>InitializeMiddleware: Initialize(ctx, execCtx)
executeMethod->>newRequest: create request
newRequest->>SerializeMiddleware: Serialize(ctx, execCtx, req)
executeMethod->>FinalizeMiddleware: Finalize(ctx, execCtx, req)
executeMethod->>HTTPServer: execute signed request
HTTPServer-->>executeMethod: HTTP response
executeMethod->>DeserializeMiddleware: Deserialize(ctx, execCtx, resp, err)
executeMethod-->>Caller: response or joined error
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Amr-Shams <amr.shams2015.as@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api.go`:
- Line 1266: The issue is revive’s indent-error-flow warning caused by
unnecessary else blocks after return in the object-handling branches. In api.go,
update the conditional paths around the affected branches (the ones using
isObject in the same flow as the GET/other handlers) so each if block returns
directly and the else body is outdented into a following standalone block. Apply
the same pattern to all matching return-ending branches so the control flow is
flatter and golangci-lint passes.
- Around line 785-802: The DeserializeMiddleware error handling in api.go is
being lost on non-2xx responses because the code still proceeds into response
parsing and overwrites the joined error. Update the response handling around the
DeserializeMiddleware loop and the successStatus check so that if mwError is not
nil, the flow short-circuits before parsing the body into ErrorResponse,
preserving the joined error. Keep the fix localized to the existing
DeserializeMiddleware/error aggregation path and the success branch that
currently calls closeResponse and returns err.
- Around line 229-232: AddMiddleware currently mutates c.middlewares while
request handling reads that slice elsewhere, so middleware registration can race
with in-flight calls. Either make AddMiddleware setup-only and document that it
must be called before the Client is used, or add synchronization around all
middleware access in Client and the request path to prevent concurrent reads and
writes.
In `@middleware_test.go`:
- Around line 68-73: The Options struct literal passed to New in
middleware_test.go has gofumpt alignment issues, specifically the Middlewares
field spacing is inconsistent with the other fields. Reformat that Options
literal to match gofumpt’s standard struct-field alignment style, keeping the
existing New call and field values unchanged.
- Line 68: The test setup in New should not rely on a fixed-length slice of
server.URL to remove the scheme, because that is brittle if the URL format
changes. Replace the direct server.URL[7:] usage with a clearer prefix removal
approach such as trimming the "http://" prefix before passing the address into
New, and update the client construction in middleware_test.go accordingly.
- Around line 54-101: Add middleware error-path coverage to
TestMiddlewareIntegration by introducing subtests that force mockMiddleware to
fail in each phase. Verify that errors from Initialize, Serialize, and Finalize
immediately short-circuit the request and prevent later phases from running, and
that a Deserialize failure is returned even when the httptest server responds
with 200 OK. Use the existing mockMiddleware and client.GetBucketPolicy setup so
the assertions clearly exercise the middleware contract.
- Around line 78-80: The `err` assigned from `client.GetBucketPolicy(...)` in
`middleware_test.go` is never used, so either ignore it explicitly if the test
only cares that the request was made, or assert the expected error from
`GetBucketPolicy` instead of leaving an ineffectual assignment. Use the
`client.GetBucketPolicy` call site in the middleware test to either replace the
assignment with a discard or add a meaningful check so the test remains valid
and the linter warning is removed.
In `@middleware.go`:
- Around line 30-38: The `SerializeMiddleware` comment is incorrect about retry
behavior: `Serialize()` is invoked from `newRequest()` inside `executeMethod()`
on every retry attempt, not once per API call. Update the documentation on
`SerializeMiddleware` to remove the “safe phase for one-shot logging” claim and
make it clear that only `InitializeMiddleware` runs once before retries, using
the `SerializeMiddleware` and `InitializeMiddleware` symbols to locate the doc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1ffc6c8-35f5-4675-be13-77ef5b05aa65
📒 Files selected for processing (3)
api.gomiddleware.gomiddleware_test.go
|
Honestly, this seems overengineered for what it does. Usually I'd leave the overengineering to AWS. Also the list-of-operations is a maintenance chore that I'd rather avoid. |
yea i see what you're pointing at. the 4-phase interface stack + type assertions is a lot of ceremony for what it does. but tbh the concept is useful, RoundTripper hacks lose context (bucket name, operation type, etc.). |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api.go (1)
940-952: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
ExecutionContextconstruction.
newRequestrebuilds its ownExecutionContextfrommetadataidentical to the one already constructed inexecuteMethod(Lines 721-726). Consider threading the existingexecCtxthrough as a parameter tonewRequestinstead of reconstructing it, keeping a single source of truth and avoiding drift if fields are added later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api.go` around lines 940 - 952, `newRequest` is rebuilding an `ExecutionContext` from `metadata`, duplicating the one already created in `executeMethod`. Update `newRequest` to accept the existing `execCtx` as an argument and use it directly in the `SerializeMiddleware` flow instead of reconstructing it, so `ExecutionContext` stays a single source of truth and won’t drift if fields change later.
♻️ Duplicate comments (5)
middleware_test.go (3)
83-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIneffectual assignment to
errpersists.
errfromclient.GetBucketPolicy(...)is reassigned but never checked, confirmed by theineffassignhint. Discard explicitly or assert the expected error.🧹 Proposed fix
- _, err = client.GetBucketPolicy(context.Background(), "mybucket") + _, _ = client.GetBucketPolicy(context.Background(), "mybucket")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware_test.go` at line 83, The `middleware_test.go` test has an ineffectual reassignment of `err` from `client.GetBucketPolicy`, so either stop assigning it or immediately assert the expected result. Update the test around `client.GetBucketPolicy` to explicitly check the returned error (or discard it if it is intentionally unused) so the `err` value is not reassigned without being validated.Source: Linters/SAST tools
62-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMissing coverage for middleware error-handling contract.
Only the all-succeed happy path is tested. The PR objectives specify that
Initialize/Serialize/Finalizeerrors short-circuit the request andDeserializeerrors abort even on 2xx responses — none of this is exercised, same gap flagged previously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware_test.go` around lines 62 - 112, Add tests in TestMiddlewareIntegration and related middleware test helpers to cover the error-handling contract for mockMiddleware: verify that failures from Initialize, Serialize, and Finalize stop the request before it reaches the server, and that Deserialize errors are returned even when the HTTP response is 2xx. Use the existing mockMiddleware methods and client.GetBucketPolicy flow to assert the short-circuit behavior and error propagation for each middleware stage.
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrittle URL prefix stripping remains.
server.URL[7:]assumes a fixed"http://"length;strings.TrimPrefix(server.URL, "http://")is safer against scheme changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware_test.go` around lines 73 - 78, The New call in the middleware test still relies on brittle fixed-length URL slicing. Replace the manual server.URL[7:] trimming with a prefix-safe approach so the test works regardless of the scheme, and update the setup around New and the mw middleware initialization to use the resulting host string without assuming "http://".api.go (2)
229-233: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRace between
AddMiddlewareand in-flight requests remains unresolved.
c.middlewaresis appended here without synchronization, whileexecuteMethod/newRequestiterate the same slice concurrently for in-flight requests. This was flagged previously and remains unaddressed in this revision — either documentAddMiddlewareas setup-only (call before the client is used) or add synchronization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api.go` around lines 229 - 233, `Client.AddMiddleware` still mutates `c.middlewares` without any protection while `executeMethod`/`newRequest` may read it concurrently during active requests. Fix this by either making `AddMiddleware` setup-only and documenting that it must be called before the client is used, or by adding synchronization around middleware reads and writes so the slice cannot race. Use the `AddMiddleware`, `executeMethod`, and `newRequest` symbols to keep the behavior consistent across all middleware access paths.
777-823: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
mwErroris still dropped on the non-2xx /expect200OKWithErrorpath.
mwErroris joined intoerrat Line 786, but thaterris unconditionally overwritten byio.ReadAll(...)at Line 800, then either silently discarded whenapiErr == nil(Line 817-819 returnsres, nil) or overwritten again byerr = errResponseat Line 823. This is the exact gap raised in the earlier thread on this code (with a recorded learning thatmwErrormust be folded into whichever error is ultimately returned, e.g. viaerrors.Join(errResponse, mwError)or returned directly whenapiErr == nil), and it has not been fixed in this revision.🐛 Suggested fix preserving mwError
- apiErr := httpRespToErrorResponse(res, metadata.bucketName, metadata.objectName) + apiErr := httpRespToErrorResponse(res, metadata.bucketName, metadata.objectName) + var deserializeErr error + if mwError != nil { + deserializeErr = mwError + } // Save the body back again. bodySeeker.Seek(0, 0) // Seek back to starting point. res.Body = io.NopCloser(bodySeeker) if apiErr == nil { + if deserializeErr != nil { + closeResponse(res) + return nil, deserializeErr + } return res, nil } // For errors verify if its retryable otherwise fail quickly. errResponse := ToErrorResponse(apiErr) - err = errResponse + err = errResponse + if deserializeErr != nil { + err = errors.Join(errResponse, deserializeErr) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api.go` around lines 777 - 823, The non-2xx and expect200OKWithError flow in the response handling path still drops `mwError` after it is joined into `err`. Update the logic in this return path so the middleware deserialize errors from `DeserializeMiddleware.Deserialize` are preserved in whichever error is ultimately returned: keep them attached when `httpRespToErrorResponse` produces `apiErr`, when `ToErrorResponse` sets `errResponse`, and when `apiErr == nil` but middleware errors exist. Use the existing `mwError`, `err`, `apiErr`, and `errResponse` handling in this block to ensure `errors.Join` is applied before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware.go`:
- Around line 10-15: ExecutionContext.QueryValues is currently sharing the same
url.Values map as requestMetadata.queryValues, so any middleware that calls
Serialize, Finalize, or Deserialize and mutates it will affect the original
request state. Fix this in the ExecutionContext construction path in api.go by
cloning the query map before assigning it, or otherwise make QueryValues
explicitly read-only; use the ExecutionContext and requestMetadata.queryValues
symbols to locate the shared-reference handoff.
- Around line 9-15: The ExecutionContext struct in middleware.go has gofumpt
alignment issues because some fields have inline comments and others do not.
Reformat the struct definition so the field declarations in ExecutionContext are
consistently aligned and any comment spacing matches gofumpt expectations,
keeping the identifiers ExecutionContext, Method, BucketName, ObjectName, and
QueryValues intact.
---
Outside diff comments:
In `@api.go`:
- Around line 940-952: `newRequest` is rebuilding an `ExecutionContext` from
`metadata`, duplicating the one already created in `executeMethod`. Update
`newRequest` to accept the existing `execCtx` as an argument and use it directly
in the `SerializeMiddleware` flow instead of reconstructing it, so
`ExecutionContext` stays a single source of truth and won’t drift if fields
change later.
---
Duplicate comments:
In `@api.go`:
- Around line 229-233: `Client.AddMiddleware` still mutates `c.middlewares`
without any protection while `executeMethod`/`newRequest` may read it
concurrently during active requests. Fix this by either making `AddMiddleware`
setup-only and documenting that it must be called before the client is used, or
by adding synchronization around middleware reads and writes so the slice cannot
race. Use the `AddMiddleware`, `executeMethod`, and `newRequest` symbols to keep
the behavior consistent across all middleware access paths.
- Around line 777-823: The non-2xx and expect200OKWithError flow in the response
handling path still drops `mwError` after it is joined into `err`. Update the
logic in this return path so the middleware deserialize errors from
`DeserializeMiddleware.Deserialize` are preserved in whichever error is
ultimately returned: keep them attached when `httpRespToErrorResponse` produces
`apiErr`, when `ToErrorResponse` sets `errResponse`, and when `apiErr == nil`
but middleware errors exist. Use the existing `mwError`, `err`, `apiErr`, and
`errResponse` handling in this block to ensure `errors.Join` is applied before
returning.
In `@middleware_test.go`:
- Line 83: The `middleware_test.go` test has an ineffectual reassignment of
`err` from `client.GetBucketPolicy`, so either stop assigning it or immediately
assert the expected result. Update the test around `client.GetBucketPolicy` to
explicitly check the returned error (or discard it if it is intentionally
unused) so the `err` value is not reassigned without being validated.
- Around line 62-112: Add tests in TestMiddlewareIntegration and related
middleware test helpers to cover the error-handling contract for mockMiddleware:
verify that failures from Initialize, Serialize, and Finalize stop the request
before it reaches the server, and that Deserialize errors are returned even when
the HTTP response is 2xx. Use the existing mockMiddleware methods and
client.GetBucketPolicy flow to assert the short-circuit behavior and error
propagation for each middleware stage.
- Around line 73-78: The New call in the middleware test still relies on brittle
fixed-length URL slicing. Replace the manual server.URL[7:] trimming with a
prefix-safe approach so the test works regardless of the scheme, and update the
setup around New and the mw middleware initialization to use the resulting host
string without assuming "http://".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7054872-e879-4121-8d2a-4ed628e7cf5d
📒 Files selected for processing (3)
api.gomiddleware.gomiddleware_test.go
|
@coderabbitai help |
4e2c194 to
3babbb5
Compare
3babbb5 to
398b260
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
middleware_test.go (3)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBrittle URL prefix stripping still present.
server.URL[7:]assumes a fixed"http://"prefix length. Preferstrings.TrimPrefix(server.URL, "http://")for clarity and safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware_test.go` at line 90, Update the New call in the middleware test to replace the fixed server.URL[7:] slicing with strings.TrimPrefix(server.URL, "http://"), adding the required strings import if needed.
100-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIneffectual assignment to
errstill present.
errfromclient.GetBucketPolicy(...)is never checked. Static analysis confirmsineffassignat line 100. Since the test only exercises the middleware lifecycle, discard explicitly with_, _.🧹 Proposed fix
- _, err = client.GetBucketPolicy(context.Background(), "mybucket") + _, _ = client.GetBucketPolicy(context.Background(), "mybucket")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware_test.go` at line 100, Update the GetBucketPolicy call in the middleware lifecycle test to explicitly discard both return values with _, _, removing the ineffectual assignment to err while preserving the existing test flow.Source: Linters/SAST tools
102-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMissing error-path coverage still unaddressed.
The test only exercises the happy path. The PR objectives specify that
Initialize/Serialize/Finalizeerrors short-circuit the request andDeserializeerrors are joined with transport errors viaerrors.Join(even on 2xx). None of this behavior is verified.Consider adding subtests where a middleware returns an error at each phase, asserting short-circuiting for the first three and error surfacing for
Deserializeeven when the server responds 200 OK.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware_test.go` around lines 102 - 129, Expand the middleware test around the existing initialized, serialized, finalized, and deserialized assertions with subtests that inject an error from each phase. Verify Initialize, Serialize, and Finalize errors immediately short-circuit the request and prevent later phases, and verify Deserialize errors are returned through errors.Join with the transport error, including when the server responds 200 OK.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@middleware_test.go`:
- Line 90: Update the New call in the middleware test to replace the fixed
server.URL[7:] slicing with strings.TrimPrefix(server.URL, "http://"), adding
the required strings import if needed.
- Line 100: Update the GetBucketPolicy call in the middleware lifecycle test to
explicitly discard both return values with _, _, removing the ineffectual
assignment to err while preserving the existing test flow.
- Around line 102-129: Expand the middleware test around the existing
initialized, serialized, finalized, and deserialized assertions with subtests
that inject an error from each phase. Verify Initialize, Serialize, and Finalize
errors immediately short-circuit the request and prevent later phases, and
verify Deserialize errors are returned through errors.Join with the transport
error, including when the server responds 200 OK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dc14c13-5f1b-4825-a1f1-a891771038c7
📒 Files selected for processing (3)
api.gomiddleware.gomiddleware_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api.go (2)
354-354: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCopy the caller-owned middleware slice.
The client retains
opts.Middlewares’ backing array. Replacing its elements afterNewsilently changes the active chain and can race with requests.Proposed fix
- clnt.middlewares = opts.Middlewares + clnt.middlewares = append([]Middleware(nil), opts.Middlewares...)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api.go` at line 354, Update the middleware assignment in New so the client stores an independent copy of opts.Middlewares rather than retaining the caller-owned slice backing array. Preserve the middleware order and existing behavior while preventing later caller mutations from changing clnt.middlewares.
769-787: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvoke
DeserializeMiddlewarefor transport failures.Lines 770-775 return or retry before the hooks run, so
Deserializealways receives a nil transport error. This violates the new contract to expose and join transport errors. Run the hooks immediately afterc.do, before deciding whether to retry or return, and cover the nil-response transport-error path in tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api.go` around lines 769 - 787, The request loop around c.do and the DeserializeMiddleware invocation must run deserialization hooks immediately after every transport attempt, including when c.do returns an error or nil response. Pass the actual transport error to Deserialize, join transport and middleware errors, then apply retry or return decisions using the combined error as appropriate; add coverage for the nil-response transport-error path.middleware.go (1)
39-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not claim that
Initializecan alter metadata.
ExecutionContextis passed by value, so changes toMethod,BucketName, orObjectNameare discarded. Describe the metadata as inspectable unless the API intentionally propagates an updated context.Proposed documentation fix
// InitializeMiddleware runs BEFORE the HTTP request is built. -// It can mutate the context or alter metadata. +// It can derive a new context or inspect operation metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware.go` around lines 39 - 43, Update the documentation for InitializeMiddleware to state that it can inspect metadata, but cannot alter Method, BucketName, or ObjectName because ExecutionContext is passed by value. Remove the claim that metadata changes are propagated, unless the API is changed to intentionally return or otherwise propagate an updated context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api.go`:
- Line 354: Update the middleware assignment in New so the client stores an
independent copy of opts.Middlewares rather than retaining the caller-owned
slice backing array. Preserve the middleware order and existing behavior while
preventing later caller mutations from changing clnt.middlewares.
- Around line 769-787: The request loop around c.do and the
DeserializeMiddleware invocation must run deserialization hooks immediately
after every transport attempt, including when c.do returns an error or nil
response. Pass the actual transport error to Deserialize, join transport and
middleware errors, then apply retry or return decisions using the combined error
as appropriate; add coverage for the nil-response transport-error path.
In `@middleware.go`:
- Around line 39-43: Update the documentation for InitializeMiddleware to state
that it can inspect metadata, but cannot alter Method, BucketName, or ObjectName
because ExecutionContext is passed by value. Remove the claim that metadata
changes are propagated, unless the API is changed to intentionally return or
otherwise propagate an updated context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: caa26599-6f2c-4e10-947c-383d04dbfb32
📒 Files selected for processing (8)
200OKwithError_test.goapi-compose-object.goapi-get-options.goapi.gocore_test.gofunctional_tests.gomiddleware.gomiddleware_test.go
398b260 to
4e29fa4
Compare
Signed-off-by: Amr-Shams <amr.shams2015.as@gmail.com>
4e29fa4 to
8745b8c
Compare
|
@harshavardhana PTAL |
| clnt.maxRetries = opts.MaxRetries | ||
| } | ||
|
|
||
| clnt.middlewares = opts.Middlewares |
There was a problem hiding this comment.
The client keeps the caller's slice header rather than a copy, so if the caller reuses or edits its own Middlewares slice after New the edit lands in this client's active chain — and it lands while in-flight requests may already be ranging over it at 728, 760, 779 and 947.
| clnt.middlewares = opts.Middlewares | |
| clnt.middlewares = append([]Middleware(nil), opts.Middlewares...) |
| // AddMiddleware adds a middleware to the chain. | ||
| func (c *Client) AddMiddleware(m Middleware) { | ||
| c.middlewares = append(c.middlewares, m) | ||
| } |
There was a problem hiding this comment.
AddMiddleware appends to the same slice that executeMethod and newRequest range over (728, 760, 779, 947), with no synchronisation on either side, so calling it once the client is live is a data race. Documenting it as setup-only is the cheap option; a mutex would mean taking a lock on all four read paths. Consider which you'd rather have.
| // AddMiddleware adds a middleware to the chain. | |
| func (c *Client) AddMiddleware(m Middleware) { | |
| c.middlewares = append(c.middlewares, m) | |
| } | |
| // AddMiddleware adds a middleware to the chain. | |
| // | |
| // It is not safe for concurrent use: call it during client setup, before the | |
| // client issues any request. | |
| func (c *Client) AddMiddleware(m Middleware) { | |
| c.middlewares = append(c.middlewares, m) | |
| } |
| @@ -0,0 +1,77 @@ | |||
| /* | |||
| * MinIO Go Library for Amazon S3 Compatible Cloud Storage | |||
| * Copyright 2015-2017 MinIO, Inc. | |||
There was a problem hiding this comment.
This file is new in this PR, so a 2015-2017 range doesn't describe it. Other files added to the repo recently use the current year.
| * Copyright 2015-2017 MinIO, Inc. | |
| * Copyright 2026 MinIO, Inc. |
| "net/url" | ||
| ) | ||
|
|
||
| // ExecutionContext provides context metadata about the S3 operation currently running. |
There was a problem hiding this comment.
The struct is copied per call, which makes it look inert, but QueryValues is a map — it points at the same values makeTargetURL reads, and the request metadata is reused across retries. A middleware calling Set or Del on it is editing shared request state, which is surprising enough to be worth a line here.
| // ExecutionContext provides context metadata about the S3 operation currently running. | |
| // ExecutionContext provides context metadata about the S3 operation currently running. | |
| // | |
| // It is passed by value, but QueryValues is the same url.Values the request | |
| // URL is built from, not a copy: treat it as read-only. |
| // InitializeMiddleware runs BEFORE the HTTP request is built. | ||
| // It can mutate the context or alter metadata. |
There was a problem hiding this comment.
execCtx arrives by value, so assigning to Method, BucketName or ObjectName inside Initialize is discarded — only the returned context survives, which makes "alter metadata" misleading. Separately: a middleware that returns (nil, nil) here puts a nil context straight into newRetryTimer, which calls Done() on it and panics the request. Stating the non-nil requirement is cheaper than a runtime check.
| // InitializeMiddleware runs BEFORE the HTTP request is built. | |
| // It can mutate the context or alter metadata. | |
| // InitializeMiddleware runs BEFORE the HTTP request is built. | |
| // It can derive a new context. execCtx is a copy, so assigning to its fields | |
| // has no effect outside the middleware. Implementations must return a non-nil | |
| // context; returning ctx unchanged is valid. |
| // FinalizeMiddleware runs AFTER the request is signed and ready to go out. | ||
| // Ideal for logging request sizes, outbound traffic, or tracing. | ||
| // | ||
| // NOTE: This fires on every retry attempt. Only Initialize runs once. |
There was a problem hiding this comment.
Signing happens inside newRequest (around 956-1001) and Finalize runs after it returns, at 760. Since the phase hands out a mutable *http.Request, it is easy to set a header here and get a signature mismatch back from the server with nothing pointing at the cause.
| // FinalizeMiddleware runs AFTER the request is signed and ready to go out. | |
| // Ideal for logging request sizes, outbound traffic, or tracing. | |
| // | |
| // NOTE: This fires on every retry attempt. Only Initialize runs once. | |
| // FinalizeMiddleware runs AFTER the request is signed and ready to go out. | |
| // Ideal for logging request sizes, outbound traffic, or tracing. | |
| // | |
| // NOTE: This fires on every retry attempt. Only Initialize runs once. | |
| // The request is already signed, so mutating headers here invalidates the | |
| // signature; use Serialize for changes that need to be signed. |
There was a problem hiding this comment.
Correction to my own suggestion above — the "use Serialize for changes that need to be signed" half is incomplete, and I would rather flag that than let it bite you.
Serialize runs at api.go:949, but several Header.Set calls come after it in the same function and overwrite unconditionally: x-amz-s3session-token (963), setUserAgent (1005), the metadata.customHeader loop (1009), Content-Md5 (1031), and X-Amz-Content-Sha256 (1037 and 1077). A Serialize middleware that sets any of those header names is silently discarded before signing. Serialize also sees req.Body == nil and ContentLength == 0, since both are assigned at 1017-1023, after the hook runs.
So the accurate version is narrower than what I wrote: Serialize is the right phase for headers the SDK does not set itself, and neither phase is a place to touch the SDK-managed ones or to inspect the body. Worth folding into the doc if you take the suggestion.
| // NOTE: This fires on every retry attempt. Only Initialize runs once. | ||
| // Errors from all Deserialize middleware stack via errors.Join with the transport | ||
| // error. If any middleware returns a non-nil error, the request aborts even on | ||
| // 2xx responses. |
There was a problem hiding this comment.
Two corrections and an addition. Transport failures are handled at api.go:770-776, before this loop, so Deserialize never sees one — a middleware registered against a refused dial records zero calls, and the transport error joined at 788 is always nil. The resp.Body note is new: the body is still unread here, so a middleware that reads it consumes it for the caller. I also dropped "the request aborts even on 2xx responses" rather than reword it, because which paths abort is exactly the open question in my other comment — worth restating once you've settled that.
| // NOTE: This fires on every retry attempt. Only Initialize runs once. | |
| // Errors from all Deserialize middleware stack via errors.Join with the transport | |
| // error. If any middleware returns a non-nil error, the request aborts even on | |
| // 2xx responses. | |
| // NOTE: This fires on every attempt that produced a response. Only Initialize | |
| // runs once; a transport failure short-circuits before this phase, so | |
| // Deserialize never observes one. resp.Body has not been read yet, so | |
| // consuming it here consumes it for the caller as well. Errors from all | |
| // Deserialize middleware are joined via errors.Join. |
| @@ -0,0 +1,133 @@ | |||
| /* | |||
| * MinIO Go Library for Amazon S3 Compatible Cloud Storage | |||
| * Copyright 2015-2017 MinIO, Inc. | |||
There was a problem hiding this comment.
Same as middleware.go: the file is new in this PR, so the 2015-2017 range doesn't describe it.
| * Copyright 2015-2017 MinIO, Inc. | |
| * Copyright 2026 MinIO, Inc. |
| if len(mw.deserialized) != 1 { | ||
| t.Fatalf("Expected 1 deserialized call, got %d", len(mw.deserialized)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error contract the description sets out — the first three phases stopping the request before it goes out, and a Deserialize error aborting an otherwise successful response — isn't asserted anywhere, so a regression in any of it would stay green. This table covers all four phases, passes against the branch as it stands, and needs no import changes. It fails if the Finalize hook is removed, so it pins the behaviour rather than just exercising it. It deliberately has no non-2xx row, because that case does not pass today (see my comment on executeMethod) — worth adding once that is settled. Consider and test this.
| } | |
| } | |
| type phaseError struct{ phase string } | |
| func (e phaseError) Error() string { return "middleware " + e.phase + " failed" } | |
| type phaseErrMiddleware struct { | |
| mu sync.Mutex | |
| failOn string | |
| phases []string | |
| } | |
| func (m *phaseErrMiddleware) ID() string { return "phase-err-middleware" } | |
| func (m *phaseErrMiddleware) record(phase string) error { | |
| m.mu.Lock() | |
| defer m.mu.Unlock() | |
| m.phases = append(m.phases, phase) | |
| if m.failOn == phase { | |
| return phaseError{phase: phase} | |
| } | |
| return nil | |
| } | |
| func (m *phaseErrMiddleware) Initialize(ctx context.Context, _ ExecutionContext) (context.Context, error) { | |
| return ctx, m.record("initialize") | |
| } | |
| func (m *phaseErrMiddleware) Serialize(_ context.Context, _ ExecutionContext, _ *http.Request) error { | |
| return m.record("serialize") | |
| } | |
| func (m *phaseErrMiddleware) Finalize(_ context.Context, _ ExecutionContext, _ *http.Request) error { | |
| return m.record("finalize") | |
| } | |
| func (m *phaseErrMiddleware) Deserialize(_ context.Context, _ ExecutionContext, _ *http.Response) error { | |
| return m.record("deserialize") | |
| } | |
| func TestMiddlewareErrorPropagation(t *testing.T) { | |
| for _, tc := range []struct { | |
| failOn string | |
| wantPhases string | |
| wantRequests int | |
| }{ | |
| {failOn: "initialize", wantPhases: "initialize", wantRequests: 0}, | |
| {failOn: "serialize", wantPhases: "initialize,serialize", wantRequests: 0}, | |
| {failOn: "finalize", wantPhases: "initialize,serialize,finalize", wantRequests: 0}, | |
| {failOn: "deserialize", wantPhases: "initialize,serialize,finalize,deserialize", wantRequests: 1}, | |
| } { | |
| t.Run(tc.failOn, func(t *testing.T) { | |
| var requestsMu sync.Mutex | |
| requests := 0 | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| requestsMu.Lock() | |
| requests++ | |
| requestsMu.Unlock() | |
| w.WriteHeader(http.StatusOK) | |
| })) | |
| defer server.Close() | |
| mw := &phaseErrMiddleware{failOn: tc.failOn} | |
| client, err := New(strings.TrimPrefix(server.URL, "http://"), &Options{ | |
| Creds: credentials.NewStaticV4("mockkey", "mocksecret", ""), | |
| Secure: false, | |
| Region: "us-east-1", | |
| Middlewares: []Middleware{mw}, | |
| }) | |
| if err != nil { | |
| t.Fatalf("Failed to create minio client: %v", err) | |
| } | |
| _, err = client.GetBucketPolicy(context.Background(), "mybucket") | |
| if err == nil || !strings.Contains(err.Error(), phaseError{phase: tc.failOn}.Error()) { | |
| t.Fatalf("Expected the %s error to reach the caller, got %v", tc.failOn, err) | |
| } | |
| mw.mu.Lock() | |
| defer mw.mu.Unlock() | |
| if got := strings.Join(mw.phases, ","); got != tc.wantPhases { | |
| t.Errorf("Expected phases %q, got %q", tc.wantPhases, got) | |
| } | |
| requestsMu.Lock() | |
| defer requestsMu.Unlock() | |
| if requests != tc.wantRequests { | |
| t.Errorf("Expected %d server request(s), got %d", tc.wantRequests, requests) | |
| } | |
| }) | |
| } | |
| } |
| if mwError != nil { | ||
| err = errors.Join(err, mwError) | ||
| } | ||
| success := successStatus.Contains(res.StatusCode) | ||
| if success && !metadata.expect200OKWithError { | ||
| // We do not expect 2xx to return an error return. | ||
| if mwError != nil { | ||
| closeResponse(res) | ||
| return nil, err | ||
| } | ||
| return res, nil |
There was a problem hiding this comment.
A Deserialize error only reaches the caller on a plain 2xx. Everywhere else it is discarded: the value joined here is overwritten by io.ReadAll at 801 and then by err = errResponse at 824, and the apiErr == nil branch at 818 returns res, nil. Two measurements against this branch —
- 404 with a middleware returning a sentinel:
errors.Is(err, sentinel)is false; the caller sees onlyNoSuchBucket. RemoveObjects(which setsexpect200OKWithError, api-remove.go:638) returning 200: the hook runs once andRemoveObjectsreports no error at all, so the failure is invisible on a 2xx too.
Worth knowing before picking a direction: the obvious repair — joining the middleware error into errResponse — has a trap. ToErrorResponse (api-error-response.go:79) is a bare type switch on ErrorResponse with a default returning an empty struct, so it does not unwrap, and errors.Join wraps even a single error. Any join on that path makes ToErrorResponse(err).Code come back "" for every caller that inspects it. That is already observable on the 2xx path today: with a failing Deserialize middleware and a 200, the returned error is an *errors.joinError and the code reads empty. Note this also makes the join at 788 redundant in its own right — err is provably nil here, since c.do's error paths at 770-776 either retry or return.
So the choice looks like: surface the middleware error only where there is no S3 error to report, or teach ToErrorResponse to try errors.As first and then join freely. Both seem like calls for you to make rather than something to patch inline.
There was a problem hiding this comment.
One case I left out of the list above, and it happens to be the one that most constrains the fix, so it is worth adding.
A retryable non-2xx discards the middleware error on every attempt, not just once. Measured on this branch with a 503/SlowDown handler and MaxRetries: 3: 3 server hits, 3 Deserialize calls, and errors.Is(err, sentinel) false — the caller sees only slow down.
That rules out the shape I would otherwise have suggested, hoisting the abort above the success check, because it would turn a retryable throttle into an immediate failure. Whatever direction you pick needs to keep the retry path intact and still not lose the error once retries are exhausted.
4 participants
Summary
Adds a middleware pipeline to the minio-go client. You hook into
four phases: Initialize, Serialize, Finalize, Deserialize.
Pulled from AWS SDK's middleware.Stack. Same shape.
Architecture
The pipeline lives inside
executeMethod()andnewRequest().No public API changes. Each middleware implements
Middleware(
ID() string) plus one or more phase interfaces:InitializeMiddleware → Initialize(ctx, execCtx)
SerializeMiddleware → Serialize(ctx, execCtx, req)
FinalizeMiddleware → Finalize(ctx, execCtx, req)
DeserializeMiddleware → Deserialize(ctx, execCtx, resp, err)
ExecutionContextcarriesOperation,BucketName,ObjectName. Scope middleware logic to specific S3 operations(verify integrity on GetObject, skip on ListBuckets).
Execution order
Initialize (once, before retry loop)
retry loop
Serialize once per attempt, before signing
Finalize once per attempt, before do()
do()
Deserialize once per attempt, after response
Error handling
via
errors.Join. Any error aborts the request, even on 2xx.Use this for response validation.
Why this instead of the old way
Before this, if you wanted custom headers, request logging, or
response verification, you had three options:
http.RoundTripper. You lose bucket name, operationtype, anything S3-specific.
Related
Prerequisite for #2246 (GetObject/FGetObject integrity verification).
The verifier will be a DeserializeMiddleware that wraps resp.Body
with a hash reader and checks against ETag or x-amz-checksum-*
headers.
Summary by CodeRabbit
AddMiddleware.