Phase 2: Add metadata, addon packages and improve REST client error handling#26
Open
Phase 2: Add metadata, addon packages and improve REST client error handling#26
Conversation
…updated, and file.infected
Previously, any status not explicitly handled (e.g. 409, 403, 500, 502)
fell through to the JSON decode path, silently producing zero-value
results. Now all >= 400 statuses return an error.
Also fix Decode(&resdata) -> Decode(resdata) to avoid unnecessary
*interface{} indirection.
WalkthroughAdds new public packages Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (caller)
participant SDK as addon.Service
participant API as Uploadcare Addons API
rect rgba(0,128,255,0.5)
Client->>SDK: Execute(ctx, addonName, ExecuteParams{Target, Params})
end
rect rgba(0,200,150,0.5)
SDK->>API: POST /addons/{name}/execute/ (body: target + params)
API-->>SDK: 200 OK { request_id }
SDK-->>Client: ExecuteResult{request_id}
end
Note over Client,SDK: Later, client polls status
Client->>SDK: Status(ctx, addonName, requestID)
SDK->>API: GET /addons/{name}/execute/status/?request_id=...
API-->>SDK: 200 OK { status, result }
SDK-->>Client: StatusResult{status, rawResult}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
metadata/log.go (1)
11-19:EnableLogonly affects services created afterward.
NewServiceinmetadata/service.goLine 37 snapshots the currentlogintosvc.New, so reassigning the package variable later won't change existing service instances. If this is meant to be a runtime toggle, keep a shared logger object or document that logging must be configured beforeNewService.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata/log.go` around lines 11 - 19, NewService is capturing the package-level log value at creation so calling EnableLog/DisableLog later doesn't affect existing services; either make the package logger a shared mutable object or change NewService to reference the package logger dynamically. Update the implementation so the package-level "log" (used by EnableLog/DisableLog) is a single shared logger instance (or pointer) that you mutate in EnableLog/DisableLog, or modify NewService to store/lookup the package "log" at call time instead of snapshotting into svc.New; refer to EnableLog, DisableLog, the package-level log variable and NewService/svc.New when making the change.test/metadata.go (1)
14-14: Preferassert.NoErrorfor error assertions.Using
assert.Equal(t, nil, err)works but is less idiomatic. Consider usingassert.NoError(t, err)for clearer failure messages and consistency with the testify idioms used elsewhere (e.g., inucare/restapi_test.go). This applies to lines 14, 21, 28, and 35.♻️ Suggested refactor
- assert.Equal(t, nil, err) + assert.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metadata.go` at line 14, Replace the non-idiomatic error equality assertions in test/metadata.go (currently using assert.Equal(t, nil, err)) with testify's assert.NoError(t, err) at the four occurrences (lines shown in the review: 14, 21, 28, 35) so the tests report clearer failure messages; update each assertion in the test functions that use err to call assert.NoError with the same testing.T and err variables.addon/service_test.go (1)
18-59: Duplicated test client implementation.This
testClientis identical to the one inmetadata/service_test.go. As noted in that file's review, consider extracting this into a shared test utility to maintain DRY principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@addon/service_test.go` around lines 18 - 59, The testClient implementation (type testClient and its methods NewRequest and Do) is duplicated; extract it to a shared test utility package (e.g., testutil) and import it from both addon/service_test.go and metadata/service_test.go: move the type and methods into a single file in the testutil package, update callers to use testutil.testClient (or a constructor like testutil.NewTestClient) and adjust imports, and remove the duplicate definitions from each test file so both tests reuse the shared implementation.metadata/service_test.go (1)
18-61: Consider extracting shared test client to reduce duplication.The
testClientimplementation is nearly identical to the one inaddon/service_test.go. Consider extracting this into a shared test utility (e.g.,internal/testutil/client.go) to reduce code duplication and ensure consistent behavior across package tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata/service_test.go` around lines 18 - 61, The testClient implementation (testClient type and its methods NewRequest and Do) is duplicated across tests; extract it into a shared test utility (e.g., internal/testutil/client.go) and update callers to import and reuse it: move the testClient struct, NewRequest and Do implementations into the shared package, export or provide a constructor (e.g., NewTestClient) to set httpClient and baseURL, then replace the local testClient definitions in metadata/service_test.go and addon/service_test.go with calls to the shared constructor to remove duplication and keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@addon/service.go`:
- Around line 66-67: The Execute request body must be made replayable by
populating req.GetBody in addition to req.Body and ContentLength; update
executeBody.EncodeReq (which currently calls codec.EncodeReqBody) so the encoded
JSON bytes are saved (e.g., into a byte slice via the shared JSON encoder) and
set req.GetBody to a function that returns a fresh io.ReadCloser over those
bytes and set ContentLength accordingly; ensure this change complements
codec.EncodeReqBody (or replace its behavior for executeBody) so throttled
retries that call req.GetBody can reopen the body.
In `@group/group_test.go`:
- Line 13: The test imports the fail-fast require package which is unsafe inside
the httptest handler goroutine; change the import alias from assert
"github.com/stretchr/testify/require" to assert
"github.com/stretchr/testify/assert" and update all in-handler calls from
require.* to assert.* (specifically the assertions used inside the httptest
handler callback in this test file) so assertions in the handler use assert
instead of require.
In `@group/group.go`:
- Around line 32-37: The DELETE call to s.svc.ResourceOp passes nil for
resourceData which triggers restapi.Do to return early without resp.Body.Close;
change the call in the delete handler (the s.svc.ResourceOp invocation that
builds the deletePathFormat with id) to pass a non-nil empty payload (e.g., an
empty struct pointer or empty map/byte slice) instead of nil so restapi.Do
follows the normal response cleanup path and closes resp.Body; update the
invocation site (s.svc.ResourceOp(... fmt.Sprintf(deletePathFormat, id) ...)) to
provide that non-nil placeholder.
In `@metadata/service.go`:
- Line 63: The path-building code is interpolating raw metadata keys into URL
segments (e.g. the fmt.Sprintf calls that produce "/files/%s/metadata/%s/"),
causing inputs like "a/b" to alter routes; fix by validating or escaping the key
before interpolation: reuse the project's metadata key validation (the
validation function in metadata/metadata.go) to reject invalid keys and return
an error, or if acceptable, apply url.PathEscape on the key to safely encode
slashes and other reserved chars; update every place that constructs
"/files/.../metadata/%s/" (the fmt.Sprintf occurrences) to first validate/escape
the key and handle failures consistently.
- Around line 105-112: The encoder stringBody.EncodeReq currently sets req.Body
to a single-use reader, causing retries to fail; modify EncodeReq to capture the
marshaled raw []byte and set req.GetBody = func() (io.ReadCloser, error) {
return io.NopCloser(bytes.NewReader(raw)), nil } (and keep req.Body =
io.NopCloser(bytes.NewReader(raw)) and req.ContentLength = int64(len(raw))) so
HTTP clients that reopen the body (e.g., restAPIClient.Do on 429) can replay the
JSON payload.
In `@test/addon.go`:
- Around line 12-16: The helper addonClamAVExecute assumes artifacts and request
IDs exist and can panic when upstream setup fails; before calling
r.Addon.Execute (and in the similar helpers referenced on lines 24-27), validate
preconditions: check len(r.Artifacts.Files) > 0 and that r.AddonRequestID (or
the specific request ID field used) is non-empty, and if they are missing call
t.Fatal/t.Fatalf with a clear error message; fail fast with descriptive messages
referencing r.Artifacts.Files and AddonRequestID so the test shows a clear setup
error instead of a panic during r.Addon.Execute.
In `@test/group.go`:
- Around line 36-39: In groupDelete, guard against indexing into
r.Artifacts.GroupIDs when it's empty: check len(r.Artifacts.GroupIDs) == 0
before using r.Artifacts.GroupIDs[0] and fail the test with a clear message
(e.g., t.Fatal or t.Fatalf) if no GroupIDs are present; otherwise proceed to
call r.Group.Delete with r.Artifacts.GroupIDs[0]. This ensures groupDelete (the
function) doesn't panic when test setup omitted group artifacts.
---
Nitpick comments:
In `@addon/service_test.go`:
- Around line 18-59: The testClient implementation (type testClient and its
methods NewRequest and Do) is duplicated; extract it to a shared test utility
package (e.g., testutil) and import it from both addon/service_test.go and
metadata/service_test.go: move the type and methods into a single file in the
testutil package, update callers to use testutil.testClient (or a constructor
like testutil.NewTestClient) and adjust imports, and remove the duplicate
definitions from each test file so both tests reuse the shared implementation.
In `@metadata/log.go`:
- Around line 11-19: NewService is capturing the package-level log value at
creation so calling EnableLog/DisableLog later doesn't affect existing services;
either make the package logger a shared mutable object or change NewService to
reference the package logger dynamically. Update the implementation so the
package-level "log" (used by EnableLog/DisableLog) is a single shared logger
instance (or pointer) that you mutate in EnableLog/DisableLog, or modify
NewService to store/lookup the package "log" at call time instead of
snapshotting into svc.New; refer to EnableLog, DisableLog, the package-level log
variable and NewService/svc.New when making the change.
In `@metadata/service_test.go`:
- Around line 18-61: The testClient implementation (testClient type and its
methods NewRequest and Do) is duplicated across tests; extract it into a shared
test utility (e.g., internal/testutil/client.go) and update callers to import
and reuse it: move the testClient struct, NewRequest and Do implementations into
the shared package, export or provide a constructor (e.g., NewTestClient) to set
httpClient and baseURL, then replace the local testClient definitions in
metadata/service_test.go and addon/service_test.go with calls to the shared
constructor to remove duplication and keep behavior consistent.
In `@test/metadata.go`:
- Line 14: Replace the non-idiomatic error equality assertions in
test/metadata.go (currently using assert.Equal(t, nil, err)) with testify's
assert.NoError(t, err) at the four occurrences (lines shown in the review: 14,
21, 28, 35) so the tests report clearer failure messages; update each assertion
in the test functions that use err to call assert.NoError with the same
testing.T and err variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 317663cd-3d6f-4da0-a416-6141126ac185
📒 Files selected for processing (23)
CHANGELOG.mdaddon/addon.goaddon/log.goaddon/service.goaddon/service_test.gogroup/group.gogroup/group_test.gogroup/service.gometadata/log.gometadata/metadata.gometadata/service.gometadata/service_test.gotest/addon.gotest/group.gotest/integration_test.gotest/metadata.gotest/testenv/runner.goucare/error.goucare/restapi.goucare/restapi_test.gowebhook/service.gowebhook/webhook.gowebhook/webhook_test.go
Extract handleResponse method with defer resp.Body.Close() to guarantee the response body is closed on all paths including nil resdata, error decoding, and retry. Replace goto with for loop.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metadata/service_test.go`:
- Around line 58-59: The nil-check calls reflect.ValueOf(resdata).IsNil()
unguarded and can panic for non-nilable kinds; change the helper to first get v
:= reflect.ValueOf(resdata), then only call v.IsNil() after confirming v.Kind()
is one of the nilable kinds (Chan, Func, Interface, Map, Ptr, Slice,
UnsafePointer); keep the existing resdata == nil check and return the same nil
result when either resdata is nil or v.IsNil() for those kinds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34ebfc2d-42bc-4af8-bc86-73029accb0e9
📒 Files selected for processing (3)
metadata/service.gometadata/service_test.gometadata/validation.go
alxgsv
changed the title
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
1 participant
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.
group.Delete()method for deleting group metadata without deleting filesFileStored,FileDeleted,FileInfoUpdated,FileInfected)New packages:
metadata/- file metadata key-value CRUD, backed by/files/{uuid}/metadata/endpointsaddon/- execute addons and poll status via/addons/{addon}/execute/and/addons/{addon}/execute/status/REST client fix:
unexpectedStatusErrwith the status code.json.Decode(&resdata)→json.Decode(resdata)(unnecessary double-pointer)Tests:
ucare.ClientSummary by CodeRabbit
New Features
Improvements