api: migrate response types to std::expected<T, Error> (or tl::expected for C++17)#238
Merged
Merged
Conversation
…ted for C++17) api: migrate response types to std::expected<T, Error> (or tl::expected for C++17)
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughThis PR introduces ChangesResult migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tmpfile.19088 (1)
1-89: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStray temp file appears accidentally committed.
tmpfile.19088looks like a local scratch artifact (e.g., piped output of a file-listing command) rather than an intentional source file. Recommend removing it from the PR.🤖 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 `@tmpfile.19088` around lines 1 - 89, Remove the accidentally committed scratch file from the change set: tmpfile.19088 appears to be a transient file-listing artifact, not a real source artifact. Delete it from the PR and keep the rest of the affected project files unchanged; if needed, verify no build or test references depend on tmpfile.19088.tests/tests.cc (1)
1319-1326: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck the async results before reading
exist.fe1.get()->exist/fe2/fe3dereference theResult<BucketExistsResponse>without an error check; if any call fails, this block can crash instead of failing the test cleanly. Store eachget()result, check!resp, then read->exist.🤖 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 `@tests/tests.cc` around lines 1319 - 1326, The concurrent BucketExistsAsync() test reads fe1.get()->exist, fe2.get()->exist, and fe3.get()->exist without first verifying the returned Result objects succeeded. Update the test in tests.cc to capture each get() result from BucketExistsAsync, check each response for failure before accessing exist, and only then evaluate the bucket existence values. Keep the cleanup and runtime_error path in the same test block so failures are reported cleanly instead of crashing.src/client.cc (1)
1378-1393: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReturn the multipart error before dereferencing
cmu_resp.CompleteMultipartUploadcan fail here; the abort still runs, butPutObjectResponse(std::move(*cmu_resp))dereferences a disengagedResultand drops the error.🤖 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 `@src/client.cc` around lines 1378 - 1393, Handle the failure from CompleteMultipartUpload in client.cc before constructing PutObjectResponse: cmu_resp can be empty when multipart completion fails, so return or propagate that error instead of unconditionally dereferencing it. Keep the AbortMultipartUpload path in the same branch, and update the PutObjectResponse return site to only use std::move(*cmu_resp) after confirming cmu_resp is engaged.
🤖 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 `@include/miniocpp/baseclient.h`:
- Around line 191-192: Update the stale API comments in baseclient.h to match
the current migration: the async overloads in the relevant BaseClient
declarations now return std::future<Result<T>>, and the sync methods also return
Result<T>, so remove the claim that they are “non-breaking” and that sync
methods remain unchanged. Keep the wording aligned with the actual signatures
around the async overload section so maintainers are not misled about
compatibility.
- Line 138: BaseClient::GetBucketPolicy has an invalid error path because
Execute(req) can return a failing Result<Response> with no response value, so
dereferencing resp in the failure branch is unsafe and loses the real error.
Update GetBucketPolicy in BaseClient to propagate the failure by returning
tl::make_unexpected(resp.error()) when Execute(req) fails, while keeping the
successful path unchanged and using the existing
GetBucketPolicyResponse/Execute(req) symbols to locate the fix.
- Line 144: The Windows GetObject wrappers are still using the old return type,
so update the `_WIN32` `GetObjectA` and `GetObjectW` methods in `BaseClient` to
return `Result<GetObjectResponse>` and forward the `GetObject(args)` result
directly. Make sure the wrapper signatures and any matching implementation
details stay aligned with the new `GetObject()` return type so Windows builds
compile cleanly.
In `@src/baseclient.cc`:
- Around line 791-796: The error handling in GetBucketNotification is inverted:
when Execute(req) fails, the code currently dereferences an empty resp and
returns a normal response instead of propagating the failure. Update the
GetBucketNotificationResponse path so the success branch parses resp->data, and
the failure branch returns tl::make_unexpected(resp.error()) using the
Execute(req) result. Keep the fix localized to the GetBucketNotification logic
in src/baseclient.cc and preserve the existing ParseXML response construction on
success.
- Around line 1426-1432: Guard the failure path in the bucket-creation flow
inside the `Execute`/`MakeBucketResponse` sequence in `baseclient.cc`: `resp`
may hold an error, so do not unconditionally dereference it with
`std::move(*resp)`. Update the logic to return the error immediately when
`Execute(req)` fails, and only enter the `region_map_` update and
`MakeBucketResponse` construction when `resp` is valid.
- Around line 843-847: The error branch in Execute handling is dereferencing an
empty Result<Response> by using std::move(*resp) after a failed call. Update the
GetBucketReplicationResponse path to return the underlying failure using
tl::make_unexpected on the Execute result’s error instead, and keep the
successful ParseXML branch in GetBucketReplicationResponse unchanged.
- Around line 1038-1045: In GetObjectRetentionResponse handling after
Execute(req), the error path currently dereferences a valueless resp by moving
*resp, which is undefined behavior. Update the if (!resp) branch to return the
error directly using resp.error() via tl::make_unexpected, while keeping the
existing NoSuchObjectLockConfiguration special case and preserving the
surrounding GetObjectRetentionResponse logic.
- Around line 869-873: The failure path in GetBucketTagsResponse handling is
dereferencing resp after Execute(req) returns a non-success result, which can
cause undefined behavior instead of propagating the request error. Update the
logic around Execute(req) and GetBucketTagsResponse so the error response is
returned directly from resp without dereferencing it, preserving the failure
details and avoiding invalid access.
- Around line 817-821: The GetBucketPolicyResponse handling in the Execute
failure path is dereferencing a disengaged Result, which can cause undefined
behavior and hide the real error. Update the response flow in the bucket policy
method so the failure branch returns the error from Execute directly instead of
using std::move(*resp), and keep the success branch using
GetBucketPolicyResponse(resp->data) as-is.
In `@src/client.cc`:
- Around line 216-219: The `ListObjectsResponse` fallback in `src/client.cc`
contains a dead `if (resp)` branch because all successful `resp` paths already
return earlier in the function. Simplify the logic around the `ListObjects`
handling by removing the unreachable condition and keeping a single return of
the empty `ListObjectsResponse`, using the existing `resp` check and the
surrounding region-lookup flow to locate the block.
- Around line 813-821: Propagate the `CompleteMultipartUpload` failure before
constructing the final response: in `PutObject` after the
`CompleteMultipartUpload(cmu_args)` call, check the `resp` `Result` and return
the error immediately if it is not successful. Only run the progress callback
and build `PutObjectResponse` when `resp` contains a value, using the
`PutObject`/`CompleteMultipartUpload` flow and the `resp` symbol to locate the
fix.
In `@src/providers.cc`:
- Around line 262-265: The AssumeRole parsing path is discarding the real parse
error and then rechecking creds_ in a way that can treat valid-but-empty
credentials as failure. Update the Credentials::ParseXML handling in the
AssumeRole fetch flow to follow the same pattern as LdapIdentityProvider::Fetch
and CertificateIdentityProvider::Fetch: keep the parse result, inspect its
success directly, and propagate parse_res.error() instead of replacing it with a
generic Credentials::error::Error message. Apply the same fix to both affected
call sites in the provider implementation so diagnostics stay consistent.
---
Outside diff comments:
In `@src/client.cc`:
- Around line 1378-1393: Handle the failure from CompleteMultipartUpload in
client.cc before constructing PutObjectResponse: cmu_resp can be empty when
multipart completion fails, so return or propagate that error instead of
unconditionally dereferencing it. Keep the AbortMultipartUpload path in the same
branch, and update the PutObjectResponse return site to only use
std::move(*cmu_resp) after confirming cmu_resp is engaged.
In `@tests/tests.cc`:
- Around line 1319-1326: The concurrent BucketExistsAsync() test reads
fe1.get()->exist, fe2.get()->exist, and fe3.get()->exist without first verifying
the returned Result objects succeeded. Update the test in tests.cc to capture
each get() result from BucketExistsAsync, check each response for failure before
accessing exist, and only then evaluate the bucket existence values. Keep the
cleanup and runtime_error path in the same test block so failures are reported
cleanly instead of crashing.
In `@tmpfile.19088`:
- Around line 1-89: Remove the accidentally committed scratch file from the
change set: tmpfile.19088 appears to be a transient file-listing artifact, not a
real source artifact. Delete it from the PR and keep the rest of the affected
project files unchanged; if needed, verify no build or test references depend on
tmpfile.19088.
🪄 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: fd0bfa6e-d757-4dd5-8f03-87ce572a5499
📒 Files selected for processing (15)
include/miniocpp/baseclient.hinclude/miniocpp/client.hinclude/miniocpp/credentials.hinclude/miniocpp/error.hinclude/miniocpp/response.hinclude/miniocpp/result.hinclude/tl/expected.hppsrc/baseclient.ccsrc/client.ccsrc/credentials.ccsrc/providers.ccsrc/response.ccsrc/select.cctests/tests.cctmpfile.19088
apply suggestion
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 (22)
examples/DeleteBucketEncryption.cc (1)
36-44: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
autofor the delete-bucket-encryption response
client.DeleteBucketEncryption(args)returnsResult<DeleteBucketEncryptionResponse>, so this should match the other examples:auto resp = client.DeleteBucketEncryption(args);. The currentDeleteBucketEncryptionResponse respwill not compile.🤖 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 `@examples/DeleteBucketEncryption.cc` around lines 36 - 44, The delete-bucket-encryption example is using the concrete response type instead of the returned Result type, which will not compile. Update the response declaration in DeleteBucketEncryptionResponse handling to use auto with client.DeleteBucketEncryption(args), matching the other examples and keeping the existing success/error handling unchanged.examples/SelectObjectContent.cc (1)
56-64: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
autoforresp
examples/SelectObjectContent.cc:56-57 —client.SelectObjectContent(args)returnsResult<SelectObjectContentResponse>, so this concreteSelectObjectContentResponsedeclaration is a type mismatch and the example won’t compile.Proposed fix
- minio::s3::SelectObjectContentResponse resp = - client.SelectObjectContent(args); + auto resp = client.SelectObjectContent(args);🤖 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 `@examples/SelectObjectContent.cc` around lines 56 - 64, The SelectObjectContent example declares resp as the concrete SelectObjectContentResponse even though client.SelectObjectContent(args) returns a Result<SelectObjectContentResponse>, which causes a compile-time type mismatch. Update the SelectObjectContentResponse resp declaration in SelectObjectContent to use auto so the result type is inferred correctly, while keeping the existing success/error handling logic unchanged.examples/DeleteBucketLifecycle.cc (1)
36-46: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
Result<DeleteBucketLifecycleResponse>here
Client::DeleteBucketLifecycle()returnsResult<DeleteBucketLifecycleResponse>, so the currentDeleteBucketLifecycleResponsedeclaration doesn’t match andresp.error()won’t compile. Useauto resp = client.DeleteBucketLifecycle(args);or an explicitResult<DeleteBucketLifecycleResponse>.🤖 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 `@examples/DeleteBucketLifecycle.cc` around lines 36 - 46, The DeleteBucketLifecycle example is using the response type directly instead of the Result wrapper returned by Client::DeleteBucketLifecycle. Update the resp declaration in DeleteBucketLifecycle.cc to use auto or an explicit Result<DeleteBucketLifecycleResponse>, then keep the existing success/error handling against the Result object so resp.error() compiles correctly.examples/DeleteBucketNotification.cc (1)
36-46: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
autofor the delete response
client.DeleteBucketNotification(args)returnsResult<DeleteBucketNotificationResponse>, so this declaration won’t compile andresp.error()is only available on the result type.🤖 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 `@examples/DeleteBucketNotification.cc` around lines 36 - 46, The DeleteBucketNotification example is declaring the return value as the concrete response type instead of the Result wrapper returned by client.DeleteBucketNotification(args), which breaks compilation and prevents access to error(). Update the resp declaration in DeleteBucketNotification to use auto so it preserves the Result<DeleteBucketNotificationResponse> type, and keep the existing success/error handling logic unchanged.examples/DeleteBucketReplication.cc (1)
36-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
autoforresphere too. It matchesDeleteBucketPolicy.ccandDeleteBucketTags.ccand avoids repeating the concrete response type.🤖 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 `@examples/DeleteBucketReplication.cc` around lines 36 - 46, The response variable in the DeleteBucketEncryption example is using an explicit concrete type, but it should match the other examples and use auto instead. Update the resp declaration in the DeleteBucketEncryption example to use auto, keeping the existing call to client.DeleteBucketEncryption(args) and the subsequent response handling unchanged.examples/DisableObjectLegalHold.cc (1)
37-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
autoforDisableObjectLegalHoldresultclient.DisableObjectLegalHold(args)returnsResult<DisableObjectLegalHoldResponse>, so the explicitDisableObjectLegalHoldResponsedeclaration makes the later.error()access invalid. Switch toautohere, as in the other updated examples.🔧 Proposed fix
- minio::s3::DisableObjectLegalHoldResponse resp = - client.DisableObjectLegalHold(args); + auto resp = client.DisableObjectLegalHold(args);🤖 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 `@examples/DisableObjectLegalHold.cc` around lines 37 - 46, The DisableObjectLegalHold example is declaring the result as DisableObjectLegalHoldResponse, but client.DisableObjectLegalHold(args) returns a Result wrapper, so the later resp.error() access is invalid. Update the variable in DisableObjectLegalHold.cc to use auto for the DisableObjectLegalHold call result, matching the pattern used in the other examples, and keep the existing success/error handling unchanged.examples/DeleteObjectLockConfig.cc (1)
36-45: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
autofor theDeleteObjectLockConfigresult
client.DeleteObjectLockConfig(args)returnsResult<DeleteObjectLockConfigResponse>, so the explicitDeleteObjectLockConfigResponsedeclaration drops theerror()accessor and breaks this branch. Match the other updated examples withauto resp = client.DeleteObjectLockConfig(args);🤖 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 `@examples/DeleteObjectLockConfig.cc` around lines 36 - 45, The DeleteObjectLockConfig example is using the concrete response type for the result of client.DeleteObjectLockConfig(args), which strips the Result wrapper and prevents access to resp.error() in the failure branch. Update the declaration in DeleteObjectLockConfig example to use auto for the DeleteObjectLockConfigConfig/response result so it preserves the Result<DeleteObjectLockConfigResponse> type, matching the other updated examples and keeping both the success and error handling paths valid.examples/EnableObjectLegalHold.cc (1)
37-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
autofor the result here.EnableObjectLegalHoldResponseis the concrete response type, soif (resp)andresp.error()won’t work on this declaration; keep it as the returned result type instead.🤖 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 `@examples/EnableObjectLegalHold.cc` around lines 37 - 46, The response variable in EnableObjectLegalHold should be declared with auto rather than the concrete EnableObjectLegalHoldResponse type, since the returned result object is what supports both the boolean check and error() access. Update the declaration in the EnableObjectLegalHold example to preserve the client.EnableObjectLegalHold(args) return type, then keep the existing if (resp) / resp.error() handling unchanged.examples/SetBucketReplication.cc (1)
50-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
autoforresphere —client.SetBucketReplication(args)returnsResult<SetBucketReplicationResponse>, so the explicitSetBucketReplicationResponsetype makesresp.error()invalid. This matches the same pattern asDeleteObjectLockConfig.cc.🤖 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 `@examples/SetBucketReplication.cc` around lines 50 - 58, The SetBucketReplication example is using the concrete response type for resp, but client.SetBucketReplication(args) returns a Result, so the explicit type makes resp.error() invalid. Update the SetBucketReplicationResponse handling in SetBucketReplication to use auto for resp, matching the pattern used in DeleteObjectLockConfig, so the success check and error() call work correctly.examples/GetBucketReplication.cc (2)
20-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace the encryption example with the replication API.
examples/GetBucketReplication.ccstill buildsGetBucketEncryptionArgs, callsclient.GetBucketEncryption(args), and prints encryption fields, so the example does not demonstrate bucket replication at all. Developers will get the wrong API shape from this file.🤖 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 `@examples/GetBucketReplication.cc` around lines 20 - 50, The example is still using the encryption API instead of replication, so update the logic in main to use the bucket replication types and call the replication method on minio::s3::Client. Replace GetBucketEncryptionArgs/GetBucketEncryptionResponse and the encryption-specific field printing with the corresponding GetBucketReplicationArgs/GetBucketReplicationResponse and replication output so this file correctly demonstrates bucket replication.
36-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the replication API call here
GetBucketReplication.ccstill usesGetBucketEncryptionArgs,GetBucketEncryptionResponse, andclient.GetBucketEncryption(args), so this example isn’t exercising replication yet.🤖 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 `@examples/GetBucketReplication.cc` around lines 36 - 47, The GetBucketReplication example is still wired to the encryption API instead of replication. Update the GetBucketReplication.cc example to use the replication-specific types and call path in place of GetBucketEncryptionArgs, GetBucketEncryptionResponse, and client.GetBucketEncryption(args), and adjust the response handling to print replication-related fields via the corresponding replication response object so the example exercises the replication API.examples/GetBucketNotification.cc (1)
36-45: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse the result wrapper for
GetBucketNotification.client.GetBucketNotification(args)needs to be stored asautosoresp.error()andresp->configmatch the returned result type;GetBucketNotificationResponse resp = ...leaves the response/payload access mismatched.🤖 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 `@examples/GetBucketNotification.cc` around lines 36 - 45, `GetBucketNotification` is being stored as the concrete response type instead of the result wrapper, which makes `resp.error()` and payload access inconsistent. Update the `GetBucketNotification` call site to keep the returned result in `auto resp` so the success path uses `resp->config` and the failure path uses `resp.error()` against the same wrapper type. Use the `GetBucketNotification` symbol and the `resp` handling in `main` to locate and align the access pattern.examples/GetBucketEncryption.cc (1)
36-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
Result<GetBucketEncryptionResponse>here
client.GetBucketEncryption(args)returnsResult<GetBucketEncryptionResponse>, so the currentGetBucketEncryptionResponse resp = ...declaration makes bothif (resp)andresp.error()invalid. Switch this toauto resp = client.GetBucketEncryption(args);and access the payload withresp->config.🤖 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 `@examples/GetBucketEncryption.cc` around lines 36 - 47, `GetBucketEncryptionResponse resp = client.GetBucketEncryption(args);` is using the wrong type for `Client::GetBucketEncryption`, which returns `Result<GetBucketEncryptionResponse>`; update the variable in `GetBucketEncryption` to keep the result wrapper (for example via `auto resp = ...`) so `if (resp)` and `resp.error()` are valid, and then read the response fields through the result payload using `resp->config` instead of direct member access.examples/SetObjectLockConfig.cc (1)
38-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
autoforresphereSetObjectLockConfigreturnsResult<SetObjectLockConfigResponse>, so the explicitSetObjectLockConfigResponsedeclaration makesresp.error()invalid.🤖 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 `@examples/SetObjectLockConfig.cc` around lines 38 - 47, `SetObjectLockConfig` returns a Result wrapper, so the explicit `SetObjectLockConfigResponse` type on `resp` in `SetObjectLockConfig` is wrong and makes `resp.error()` invalid. Update the `resp` declaration to use type deduction with `auto` so the Result type is preserved, and keep the existing success/error handling in the `if (resp)` block unchanged.examples/SetBucketVersioning.cc (1)
37-46: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
autoforresphere.client.SetBucketVersioning(args)returnsResult<SetBucketVersioningResponse>, so the explicitSetBucketVersioningResponsedeclaration makesresp.error()invalid.🤖 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 `@examples/SetBucketVersioning.cc` around lines 37 - 46, Use auto for resp in SetBucketVersioning so it keeps the Result<SetBucketVersioningResponse> type returned by client.SetBucketVersioning(args); the explicit minio::s3::SetBucketVersioningResponse declaration breaks resp.error() in the success/failure handling. Update the resp declaration in SetBucketVersioning accordingly and keep the existing if (resp) / resp.error().String() logic unchanged.examples/GetObjectLockConfig.cc (1)
36-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the
Resulttype here
client.GetObjectLockConfig(args)returnsResult<GetObjectLockConfigResponse>, butGetObjectLockConfigResponseis just a plain struct.if (resp)andresp.error()only work on theResult, so this declaration should beauto resp = ...orResult<GetObjectLockConfigResponse> resp = ....🤖 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 `@examples/GetObjectLockConfig.cc` around lines 36 - 57, The response from client.GetObjectLockConfig(args) is being stored as the plain GetObjectLockConfigResponse struct, but the subsequent if (resp) and resp.error() logic requires the Result<GetObjectLockConfigResponse> wrapper. Update the declaration in GetObjectLockConfig.main so resp is declared as auto or as minio::s3::Result<GetObjectLockConfigResponse>, then keep the existing success/error handling unchanged.examples/GetBucketVersioning.cc (1)
36-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
Result<GetBucketVersioningResponse>here —client.GetBucketVersioning(args)returnsminio::Result<GetBucketVersioningResponse>, so binding it to plainminio::s3::GetBucketVersioningResponsedrops the.error()path and won’t compile.auto resp = ...(or the explicitminio::Result<...>) matches the API.🤖 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 `@examples/GetBucketVersioning.cc` around lines 36 - 44, The GetBucketVersioning example is binding the return value of client.GetBucketVersioning(args) to minio::s3::GetBucketVersioningResponse, which drops the error-handling wrapper and breaks the resp.error() path. Update the response variable in GetBucketVersioning to use minio::Result<GetBucketVersioningResponse> or auto so it matches the API and preserves both success and error handling.examples/GetObjectRetention.cc (1)
40-49: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
resp->...when reading theGetObjectRetentionResponse.
respis aResult<GetObjectRetentionResponse>(tl::expected), soresp.retention_modeandresp.retain_until_datewon’t compile. Use pointer access on the wrapped value instead.🐛 Proposed fix
if (resp) { std::cout << "Retention Mode: " - << minio::s3::RetentionModeToString(resp.retention_mode) + << minio::s3::RetentionModeToString(resp->retention_mode) << std::endl; std::cout << "Retain Until Date: " - << resp.retain_until_date.ToHttpHeaderValue() << std::endl; + << resp->retain_until_date.ToHttpHeaderValue() << std::endl; } else {🤖 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 `@examples/GetObjectRetention.cc` around lines 40 - 49, In GetObjectRetention.cc, the code is treating GetObjectRetentionResponse as if it were the Result wrapper itself; update the success branch to access the wrapped response via resp->... instead of resp.retention_mode and resp.retain_until_date. Use the GetObjectRetentionResponse result value returned by the response object consistently, and keep the error branch using resp.error().String() for the failure case.examples/StatObject.cc (1)
40-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the remaining
respfield accesses
respis aResult<StatObjectResponse>, so the remainingresp.reads (last_modified,retention_mode,retention_retain_until_date,legal_hold,delete_marker) won’t compile; switch them toresp->...for consistency.🤖 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 `@examples/StatObject.cc` around lines 40 - 76, The remaining `resp` accesses in `StatObject` are using dot syntax on a `Result<StatObjectResponse>` and will not compile. Update the `last_modified`, `retention_mode`, `retention_retain_until_date`, `legal_hold`, and `delete_marker` reads in the `if (resp)` block to use `resp->...` consistently, matching the existing `version_id`, `etag`, `size`, and `user_metadata` access style.examples/GetPresignedPostFormData.cc (1)
43-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
resp->form_datahere
client.GetPresignedPostFormData(policy)returnsResult<GetPresignedPostFormDataResponse>, soresp.form_datawon’t compile. Useresp->form_datain the success branch.🤖 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 `@examples/GetPresignedPostFormData.cc` around lines 43 - 53, The success branch in GetPresignedPostFormData is accessing the Result object incorrectly; client.GetPresignedPostFormData(policy) returns a Result<GetPresignedPostFormDataResponse>, so use the dereferenced response when iterating form_data. Update the resp handling in the example to read form_data from the successful response object (the one checked by if (resp)) and keep the error branch using resp.error().String() unchanged.examples/ListenBucketNotification.cc (1)
44-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
autoforListenBucketNotification
ListenBucketNotificationreturnsResult<ListenBucketNotificationResponse>, soListenBucketNotificationResponse resp = ...is a type mismatch and won’t compile. Keep this asauto resp = client.ListenBucketNotification(args);soresp.error()stays valid.🤖 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 `@examples/ListenBucketNotification.cc` around lines 44 - 51, The ListenBucketNotification example is using the wrong variable type, since ListenBucketNotification returns a Result<ListenBucketNotificationResponse> rather than a raw ListenBucketNotificationResponse. Update the declaration in ListenBucketNotification to use auto for the result returned by client.ListenBucketNotification(args), so the existing resp.error() handling remains valid and the code compiles.examples/IsObjectLegalHoldEnabled.cc (1)
37-45: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the
Result<IsObjectLegalHoldEnabledResponse>form here
client.IsObjectLegalHoldEnabled(args)returnsResult<IsObjectLegalHoldEnabledResponse>, soIsObjectLegalHoldEnabledResponse resp = ...andresp.enableddon’t match the API. Switch toauto resp = ..., readresp->enabled, and keepresp.error()for the failure path.🤖 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 `@examples/IsObjectLegalHoldEnabled.cc` around lines 37 - 45, The example is using the wrong return type for client.IsObjectLegalHoldEnabled(args), so update IsObjectLegalHoldEnabledResponse resp to the Result<IsObjectLegalHoldEnabledResponse> form by using auto resp = ...; then access the success value through resp->enabled instead of resp.enabled, and keep the existing resp.error().String() handling in the failure branch.
🤖 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 `@examples/GetBucketLifecycle.cc`:
- Around line 36-44: `GetBucketLifecycle` is using the expected payload
incorrectly in the success branch. Update the response handling in
`GetBucketLifecycle` to access the lifecycle configuration through
`resp->config` (or `(*resp).config`) instead of `resp.config`, while leaving the
`resp.error()` usage in the failure branch unchanged.
In `@examples/GetBucketPolicy.cc`:
- Around line 36-43: The response handling in GetBucketPolicy uses the
Result<GetBucketPolicyResponse> object directly instead of the contained
response; update the successful branch in GetBucketPolicy to access the policy
through the dereferenced result object, matching the pattern used in
GetBucketLifecycle. Keep the error branch unchanged, and make sure the fix is
applied wherever resp is treated as a Result so members are read from the
underlying response rather than from resp itself.
---
Outside diff comments:
In `@examples/DeleteBucketEncryption.cc`:
- Around line 36-44: The delete-bucket-encryption example is using the concrete
response type instead of the returned Result type, which will not compile.
Update the response declaration in DeleteBucketEncryptionResponse handling to
use auto with client.DeleteBucketEncryption(args), matching the other examples
and keeping the existing success/error handling unchanged.
In `@examples/DeleteBucketLifecycle.cc`:
- Around line 36-46: The DeleteBucketLifecycle example is using the response
type directly instead of the Result wrapper returned by
Client::DeleteBucketLifecycle. Update the resp declaration in
DeleteBucketLifecycle.cc to use auto or an explicit
Result<DeleteBucketLifecycleResponse>, then keep the existing success/error
handling against the Result object so resp.error() compiles correctly.
In `@examples/DeleteBucketNotification.cc`:
- Around line 36-46: The DeleteBucketNotification example is declaring the
return value as the concrete response type instead of the Result wrapper
returned by client.DeleteBucketNotification(args), which breaks compilation and
prevents access to error(). Update the resp declaration in
DeleteBucketNotification to use auto so it preserves the
Result<DeleteBucketNotificationResponse> type, and keep the existing
success/error handling logic unchanged.
In `@examples/DeleteBucketReplication.cc`:
- Around line 36-46: The response variable in the DeleteBucketEncryption example
is using an explicit concrete type, but it should match the other examples and
use auto instead. Update the resp declaration in the DeleteBucketEncryption
example to use auto, keeping the existing call to
client.DeleteBucketEncryption(args) and the subsequent response handling
unchanged.
In `@examples/DeleteObjectLockConfig.cc`:
- Around line 36-45: The DeleteObjectLockConfig example is using the concrete
response type for the result of client.DeleteObjectLockConfig(args), which
strips the Result wrapper and prevents access to resp.error() in the failure
branch. Update the declaration in DeleteObjectLockConfig example to use auto for
the DeleteObjectLockConfigConfig/response result so it preserves the
Result<DeleteObjectLockConfigResponse> type, matching the other updated examples
and keeping both the success and error handling paths valid.
In `@examples/DisableObjectLegalHold.cc`:
- Around line 37-46: The DisableObjectLegalHold example is declaring the result
as DisableObjectLegalHoldResponse, but client.DisableObjectLegalHold(args)
returns a Result wrapper, so the later resp.error() access is invalid. Update
the variable in DisableObjectLegalHold.cc to use auto for the
DisableObjectLegalHold call result, matching the pattern used in the other
examples, and keep the existing success/error handling unchanged.
In `@examples/EnableObjectLegalHold.cc`:
- Around line 37-46: The response variable in EnableObjectLegalHold should be
declared with auto rather than the concrete EnableObjectLegalHoldResponse type,
since the returned result object is what supports both the boolean check and
error() access. Update the declaration in the EnableObjectLegalHold example to
preserve the client.EnableObjectLegalHold(args) return type, then keep the
existing if (resp) / resp.error() handling unchanged.
In `@examples/GetBucketEncryption.cc`:
- Around line 36-47: `GetBucketEncryptionResponse resp =
client.GetBucketEncryption(args);` is using the wrong type for
`Client::GetBucketEncryption`, which returns
`Result<GetBucketEncryptionResponse>`; update the variable in
`GetBucketEncryption` to keep the result wrapper (for example via `auto resp =
...`) so `if (resp)` and `resp.error()` are valid, and then read the response
fields through the result payload using `resp->config` instead of direct member
access.
In `@examples/GetBucketNotification.cc`:
- Around line 36-45: `GetBucketNotification` is being stored as the concrete
response type instead of the result wrapper, which makes `resp.error()` and
payload access inconsistent. Update the `GetBucketNotification` call site to
keep the returned result in `auto resp` so the success path uses `resp->config`
and the failure path uses `resp.error()` against the same wrapper type. Use the
`GetBucketNotification` symbol and the `resp` handling in `main` to locate and
align the access pattern.
In `@examples/GetBucketReplication.cc`:
- Around line 20-50: The example is still using the encryption API instead of
replication, so update the logic in main to use the bucket replication types and
call the replication method on minio::s3::Client. Replace
GetBucketEncryptionArgs/GetBucketEncryptionResponse and the encryption-specific
field printing with the corresponding
GetBucketReplicationArgs/GetBucketReplicationResponse and replication output so
this file correctly demonstrates bucket replication.
- Around line 36-47: The GetBucketReplication example is still wired to the
encryption API instead of replication. Update the GetBucketReplication.cc
example to use the replication-specific types and call path in place of
GetBucketEncryptionArgs, GetBucketEncryptionResponse, and
client.GetBucketEncryption(args), and adjust the response handling to print
replication-related fields via the corresponding replication response object so
the example exercises the replication API.
In `@examples/GetBucketVersioning.cc`:
- Around line 36-44: The GetBucketVersioning example is binding the return value
of client.GetBucketVersioning(args) to minio::s3::GetBucketVersioningResponse,
which drops the error-handling wrapper and breaks the resp.error() path. Update
the response variable in GetBucketVersioning to use
minio::Result<GetBucketVersioningResponse> or auto so it matches the API and
preserves both success and error handling.
In `@examples/GetObjectLockConfig.cc`:
- Around line 36-57: The response from client.GetObjectLockConfig(args) is being
stored as the plain GetObjectLockConfigResponse struct, but the subsequent if
(resp) and resp.error() logic requires the Result<GetObjectLockConfigResponse>
wrapper. Update the declaration in GetObjectLockConfig.main so resp is declared
as auto or as minio::s3::Result<GetObjectLockConfigResponse>, then keep the
existing success/error handling unchanged.
In `@examples/GetObjectRetention.cc`:
- Around line 40-49: In GetObjectRetention.cc, the code is treating
GetObjectRetentionResponse as if it were the Result wrapper itself; update the
success branch to access the wrapped response via resp->... instead of
resp.retention_mode and resp.retain_until_date. Use the
GetObjectRetentionResponse result value returned by the response object
consistently, and keep the error branch using resp.error().String() for the
failure case.
In `@examples/GetPresignedPostFormData.cc`:
- Around line 43-53: The success branch in GetPresignedPostFormData is accessing
the Result object incorrectly; client.GetPresignedPostFormData(policy) returns a
Result<GetPresignedPostFormDataResponse>, so use the dereferenced response when
iterating form_data. Update the resp handling in the example to read form_data
from the successful response object (the one checked by if (resp)) and keep the
error branch using resp.error().String() unchanged.
In `@examples/IsObjectLegalHoldEnabled.cc`:
- Around line 37-45: The example is using the wrong return type for
client.IsObjectLegalHoldEnabled(args), so update
IsObjectLegalHoldEnabledResponse resp to the
Result<IsObjectLegalHoldEnabledResponse> form by using auto resp = ...; then
access the success value through resp->enabled instead of resp.enabled, and keep
the existing resp.error().String() handling in the failure branch.
In `@examples/ListenBucketNotification.cc`:
- Around line 44-51: The ListenBucketNotification example is using the wrong
variable type, since ListenBucketNotification returns a
Result<ListenBucketNotificationResponse> rather than a raw
ListenBucketNotificationResponse. Update the declaration in
ListenBucketNotification to use auto for the result returned by
client.ListenBucketNotification(args), so the existing resp.error() handling
remains valid and the code compiles.
In `@examples/SelectObjectContent.cc`:
- Around line 56-64: The SelectObjectContent example declares resp as the
concrete SelectObjectContentResponse even though
client.SelectObjectContent(args) returns a Result<SelectObjectContentResponse>,
which causes a compile-time type mismatch. Update the
SelectObjectContentResponse resp declaration in SelectObjectContent to use auto
so the result type is inferred correctly, while keeping the existing
success/error handling logic unchanged.
In `@examples/SetBucketReplication.cc`:
- Around line 50-58: The SetBucketReplication example is using the concrete
response type for resp, but client.SetBucketReplication(args) returns a Result,
so the explicit type makes resp.error() invalid. Update the
SetBucketReplicationResponse handling in SetBucketReplication to use auto for
resp, matching the pattern used in DeleteObjectLockConfig, so the success check
and error() call work correctly.
In `@examples/SetBucketVersioning.cc`:
- Around line 37-46: Use auto for resp in SetBucketVersioning so it keeps the
Result<SetBucketVersioningResponse> type returned by
client.SetBucketVersioning(args); the explicit
minio::s3::SetBucketVersioningResponse declaration breaks resp.error() in the
success/failure handling. Update the resp declaration in SetBucketVersioning
accordingly and keep the existing if (resp) / resp.error().String() logic
unchanged.
In `@examples/SetObjectLockConfig.cc`:
- Around line 38-47: `SetObjectLockConfig` returns a Result wrapper, so the
explicit `SetObjectLockConfigResponse` type on `resp` in `SetObjectLockConfig`
is wrong and makes `resp.error()` invalid. Update the `resp` declaration to use
type deduction with `auto` so the Result type is preserved, and keep the
existing success/error handling in the `if (resp)` block unchanged.
In `@examples/StatObject.cc`:
- Around line 40-76: The remaining `resp` accesses in `StatObject` are using dot
syntax on a `Result<StatObjectResponse>` and will not compile. Update the
`last_modified`, `retention_mode`, `retention_retain_until_date`, `legal_hold`,
and `delete_marker` reads in the `if (resp)` block to use `resp->...`
consistently, matching the existing `version_id`, `etag`, `size`, and
`user_metadata` access style.
🪄 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: fd049af4-4ae9-4cf7-be9e-b36f90260ee5
📒 Files selected for processing (52)
examples/BucketExists.ccexamples/ComposeObject.ccexamples/CopyObject.ccexamples/DeleteBucketEncryption.ccexamples/DeleteBucketLifecycle.ccexamples/DeleteBucketNotification.ccexamples/DeleteBucketPolicy.ccexamples/DeleteBucketReplication.ccexamples/DeleteBucketTags.ccexamples/DeleteObjectLockConfig.ccexamples/DeleteObjectTags.ccexamples/DisableObjectLegalHold.ccexamples/DownloadObject.ccexamples/EnableObjectLegalHold.ccexamples/GetBucketEncryption.ccexamples/GetBucketLifecycle.ccexamples/GetBucketNotification.ccexamples/GetBucketPolicy.ccexamples/GetBucketReplication.ccexamples/GetBucketTags.ccexamples/GetBucketVersioning.ccexamples/GetObject.ccexamples/GetObjectLockConfig.ccexamples/GetObjectProgress.ccexamples/GetObjectRetention.ccexamples/GetObjectTags.ccexamples/GetPresignedObjectUrl.ccexamples/GetPresignedPostFormData.ccexamples/GetPutRDMA.ccexamples/IsObjectLegalHoldEnabled.ccexamples/ListBuckets.ccexamples/ListObjects.ccexamples/ListenBucketNotification.ccexamples/MakeBucket.ccexamples/PutObject.ccexamples/PutObjectProgress.ccexamples/RemoveBucket.ccexamples/RemoveObject.ccexamples/RemoveObjects.ccexamples/SelectObjectContent.ccexamples/SetBucketEncryption.ccexamples/SetBucketLifecycle.ccexamples/SetBucketNotification.ccexamples/SetBucketPolicy.ccexamples/SetBucketReplication.ccexamples/SetBucketTags.ccexamples/SetBucketVersioning.ccexamples/SetObjectLockConfig.ccexamples/SetObjectRetention.ccexamples/SetObjectTags.ccexamples/StatObject.ccexamples/UploadObject.cc
harshavardhana
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ted for C++17)
fix issue
Summary by CodeRabbit
New Features
Bug Fixes