Phase 7: Upload API User Agent#31
Open
alxgsv wants to merge 20 commits into
Open
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.
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.
Derive CDN base URL from the public key (SHA256 → base36 → first 10 chars). Config.CDNBase can be set explicitly to override the default. Shallow-copy Config in resolveConfig to avoid mutating a shared template when creating clients for multiple projects.
New projectapi/ package covering projects CRUD, secret keys management, and usage metrics. Adds ucare.NewBearerClient() for bearer token auth used by the Project API.
Pagination next URLs point to a different host (app.uploadcare.com), so List and ListSecrets now use codec.ResultBuf iterators that automatically follow next links. Bearer client fallback routes unknown hosts through the same auth backend.
Pagination next URLs point to a different host (app.uploadcare.com). NewRequest was failing with errNoClient before Do/fallbackDo could run. Add fallbackNewReq to route unknown endpoints through the bearer auth client for both request construction and execution.
WalkthroughThis pull request introduces breaking API changes, expands SDK functionality with new packages for Project API, metadata CRUD, and addon services, implements retry configuration and typed errors, adds metadata support to uploads, updates webhook and file services, and includes comprehensive test coverage across all changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant UAREClient as ucare.Client<br/>(Bearer)
participant ProjectAPI as projectAPIClient
participant HTTPServer as HTTP Server<br/>(Project API)
Client->>UAREClient: NewBearerClient(token)
UAREClient->>ProjectAPI: Initialize with token
Client->>UAREClient: List Projects
UAREClient->>ProjectAPI: NewRequest()
ProjectAPI->>ProjectAPI: Set Authorization: Bearer <token>
ProjectAPI->>HTTPServer: GET /projects/
HTTPServer-->>ProjectAPI: JSON response
ProjectAPI->>ProjectAPI: handleResponse()
ProjectAPI-->>UAREClient: *ProjectList
UAREClient-->>Client: *ProjectList
Client->>UAREClient: Get Project by PubKey
UAREClient->>ProjectAPI: NewRequest()
ProjectAPI->>HTTPServer: GET /projects/{pubkey}/
HTTPServer-->>ProjectAPI: JSON Project
ProjectAPI->>UAREClient: Project
UAREClient-->>Client: Project
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 5
🧹 Nitpick comments (7)
file/service.go (1)
21-21: Consider compatibility impact of changing exported interface signatures.Updating
file.Service.Infoto require*InfoParamsforces all external implementers/mocks to change. If backward compatibility is a goal, consider keepingInfo(ctx, id)and adding a new method for optional params.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@file/service.go` at line 21, Changing the exported method signature of file.Service.Info to accept *InfoParams breaks backward compatibility for external implementers and mocks; restore the original signature Info(ctx context.Context, id string) (Info, error) on the file.Service interface and add a new method (e.g., InfoWithParams(ctx context.Context, id string, params *InfoParams) (Info, error)) or provide a default-wrapper implementation that calls the new params-aware function so existing callers/implementers do not need to change; update any internal uses to call the new method where params are required and keep the original Info as a shim delegating to the new implementation when params are nil/default.test/integration_test.go (1)
110-113: Bound the Project API list call with a timeout context.Using
context.Background()here can hang CI indefinitely on network stalls. A short timeout makes failures deterministic.⏱️ Suggested change
import ( "context" "os" "testing" + "time" @@ - list, err := svc.List(context.Background(), nil) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + list, err := svc.List(ctx, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration_test.go` around lines 110 - 113, Replace the use of context.Background() when calling svc.List with a bounded context created via context.WithTimeout to avoid hanging tests; create a ctx, cancel := context.WithTimeout(context.Background(), <reasonableDuration>) and defer cancel(), then call svc.List(ctx, nil) and handle the error as before (t.Fatal on error). Update the test function surrounding the svc.List call to import/time-reference if needed and choose an appropriate timeout (e.g., a few seconds) to make CI deterministic.projectapi/usage.go (1)
32-42: Validatemetricbefore building the request path.
GetUsageMetricdocuments allowed values but currently accepts any string. A fast client-side check gives clearer errors and avoids invalid requests.✅ Suggested guard
func (s service) GetUsageMetric( ctx context.Context, pubKey string, metric string, params UsageDateRange, ) (data UsageMetric, err error) { + switch metric { + case "traffic", "storage", "operations": + // ok + default: + return data, fmt.Errorf("invalid metric %q", metric) + } + err = s.svc.ResourceOp( ctx, http.MethodGet, fmt.Sprintf(usageMetricPathFmt, pubKey, metric), ¶ms, &data, ) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projectapi/usage.go` around lines 32 - 42, Add a client-side validation in GetUsageMetric to reject invalid metric names before calling service.svc.ResourceOp: check the incoming metric string against the documented allowed set (e.g., a small whitelist or map of allowed metric keys) and return a descriptive error immediately if not present. Place this guard at the top of GetUsageMetric (before fmt.Sprintf(usageMetricPathFmt, pubKey, metric)) so the request path is never built for invalid metrics; use the same error type/format conventions the service uses for input validation.upload/upload_test.go (1)
70-85: Assert the expected HTTP verb in these handlers.These test servers route only by path right now. A regression from
POSTto another method on/base/,/multipart/start/, or/multipart/complete/would still pass, so the tests can miss a real request-construction bug.Also applies to: 110-124, 150-168, 195-212, 240-254, 280-297, 323-337, 359-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upload/upload_test.go` around lines 70 - 85, The test HTTP handler currently only switches on r.URL.Path, so regressions in HTTP verbs will be missed; update the httptest server handler (the function passed to httptest.NewServer where srv is created) to check r.Method for each path (e.g., assert POST for "/base/", "/multipart/start/", "/multipart/complete/" and GET for "/info/") and return a 405 Method Not Allowed (or fail the test) when the method is unexpected, incrementing the appropriate counters only after method validation; apply the same change to the other test server handler blocks referenced (lines handling "/base/", "/info/", "/multipart/start/", "/multipart/complete/") so all paths validate the expected HTTP verb.ucare/uploadapi_test.go (1)
72-103: Assert the full Upload API User-Agent contract here.These checks only verify substrings. They would still pass if the upload client dropped the public-key component or if the header stopped matching the internally constructed
userAgent, which is the regression this PR is meant to prevent.🔍 Suggested assertion upgrade
func TestUploadAPIClient_UserAgent(t *testing.T) { t.Parallel() client := newUploadAPIClient(testCreds(), resolveConfig(nil, testCreds())) @@ ) assert.NoError(t, err) - assert.Contains(t, req.Header.Get("User-Agent"), "UploadcareGo/") + ua := req.Header.Get("User-Agent") + assert.Equal(t, client.userAgent, ua) + assert.Contains(t, ua, testCreds().PublicKey) } func TestUploadAPIClient_CustomUserAgent(t *testing.T) { @@ ) assert.NoError(t, err) - assert.Contains(t, req.Header.Get("User-Agent"), "UploadcareGo/") - assert.Contains(t, req.Header.Get("User-Agent"), "MyApp/1.0") + ua := req.Header.Get("User-Agent") + assert.Equal(t, client.userAgent, ua) + assert.Contains(t, ua, testCreds().PublicKey) + assert.Contains(t, ua, "MyApp/1.0") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ucare/uploadapi_test.go` around lines 72 - 103, Update the two tests TestUploadAPIClient_UserAgent and TestUploadAPIClient_CustomUserAgent to assert the full User-Agent header equals the constructed userAgent string rather than only checking substrings: retrieve the client’s configured userAgent (the same value built by newUploadAPIClient/resolveConfig using Config.UserAgent and the internal UploadcareGo prefix/public-key component) and compare req.Header.Get("User-Agent") to that exact expected string (include both the UploadcareGo/<version> + public-key component and, for the custom case, the appended " MyApp/1.0" piece) so the tests fail on any deviation from the intended userAgent contract.CHANGELOG.md (1)
39-40: Call out the Upload API header fix explicitly.This notes the new config field, but not the behavior change from this PR: Upload API requests now send the SDK/application
User-Agentinstead of Go's default header. Adding that release note would make the change much easier to spot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 39 - 40, Add a concise release note line calling out that Upload API requests now send the SDK/application User-Agent instead of Go's default header; reference the new UserAgent field on ucare.Config and mention "Upload API" and "User-Agent header" so readers see the behavioral change introduced by this PR.projectapi/projects.go (1)
25-26: ReturnnilwhenListfails.
Listcurrently returns a non-nil*ProjectListeven whens.svc.Listerrors. That wrapper has a nil iterator underneath, soNext()will panic if the caller keeps it around after an error.Suggested change
resbuf, err := s.svc.List(ctx, projectsPath, enc) - return &ProjectList{raw: resbuf}, err + if err != nil { + return nil, err + } + return &ProjectList{raw: resbuf}, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projectapi/projects.go` around lines 25 - 26, The wrapper always returns &ProjectList{raw: resbuf} even when s.svc.List returns an error, which leaves ProjectList.raw nil and causes Next() to panic; change the return path in the function that calls s.svc.List so that if err != nil you return nil, err (instead of returning a non-nil *ProjectList). Update the block around the s.svc.List call to check err and only construct &ProjectList{raw: resbuf} when err == nil so callers do not receive a dangling ProjectList with a nil iterator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conversion/path.go`:
- Around line 60-76: The size and thumbs path fragments are being built
incorrectly: instead of emitting ResizeMode as a separate trailing segment after
the size, combine it into the size fragment (e.g. produce
"-/size/{size}/{resizeMode}/" in the same fmt call) so the transformation string
remains well-formed (adjust the fmt.Fprintf usage that writes "-/size/%s/" and
the conditional that writes opts.ResizeMode), and change the thumbs fragment to
use slashes instead of a tilde by replacing the "-/thumbs~%d/" fmt with
"-/thumbs/%d/" (update the fmt.Fprintf that references opts.Thumbs accordingly).
In `@projectapi/projectapi.go`:
- Around line 58-63: The UploadSettings struct is causing null serializations
for unset optional limits; update the JSON tags for FilesizeLimit and
ImageResolutionLimit in UploadSettings so they include `omitempty` (similar to
Autostore and IsSignedUploadEnabled) so that when UpdateProjectParams sends
partial updates those unset pointer fields are omitted instead of serializing as
null; locate the UploadSettings type and modify the `json:"filesize_limit"` and
`json:"image_resolution_limit"` tags to use `omitempty`.
In `@ucare/client.go`:
- Around line 147-157: The current resolveBearerConfig (and the similar
resolveConfig helper used for NewClient) shallow-copies the Config struct so the
Retry pointer is shared; change these functions to deep-copy the Retry field by
creating a new RetryConfig when conf.Retry != nil and assigning its value to the
cloned config (e.g., allocate new variable like copied := *conf; if conf.Retry
!= nil { r := *conf.Retry; copied.Retry = &r } ), and ensure HTTPClient
defaulting remains as-is so the returned *Config is fully independent of the
caller's Retry pointer.
In `@ucare/projectapi.go`:
- Around line 116-123: The current logic enforces c.retry.MaxWaitSeconds only
when Retry-After is present, but the fallback expBackoff(tries) can exceed the
cap; update the retry path in the function that computes wait (variables
retryAfter, wait and call to expBackoff(tries)) to enforce the cap for both
cases by applying the MaxWaitSeconds limit after computing wait (e.g., if
c.retry.MaxWaitSeconds > 0 then set wait = min(wait, c.retry.MaxWaitSeconds))
and return ThrottleError{RetryAfter: wait} or proceed with sleeping using the
capped wait so the configured maximum is always honored.
In `@ucare/restapi.go`:
- Around line 135-155: The 400/401/403/404 case branches currently return the
json.Decoder error directly when json.NewDecoder(resp.Body).Decode(...) fails,
losing the HTTP status and typed error info; update each branch (the 400/404
APIError handling, the 401 AuthError handling, and the 403 ForbiddenError
handling) to catch Decode errors and instead return the corresponding typed
error with StatusCode set (and a sensible default Message/Detail populated, e.g.
from a fallback string or resp.Status) similar to how the generic ">=400" path
builds a fallback error; do not let decoder errors escape as-is—only return
decode errors if you still want to wrap them, otherwise return the typed error
with StatusCode/default detail.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 39-40: Add a concise release note line calling out that Upload API
requests now send the SDK/application User-Agent instead of Go's default header;
reference the new UserAgent field on ucare.Config and mention "Upload API" and
"User-Agent header" so readers see the behavioral change introduced by this PR.
In `@file/service.go`:
- Line 21: Changing the exported method signature of file.Service.Info to accept
*InfoParams breaks backward compatibility for external implementers and mocks;
restore the original signature Info(ctx context.Context, id string) (Info,
error) on the file.Service interface and add a new method (e.g.,
InfoWithParams(ctx context.Context, id string, params *InfoParams) (Info,
error)) or provide a default-wrapper implementation that calls the new
params-aware function so existing callers/implementers do not need to change;
update any internal uses to call the new method where params are required and
keep the original Info as a shim delegating to the new implementation when
params are nil/default.
In `@projectapi/projects.go`:
- Around line 25-26: The wrapper always returns &ProjectList{raw: resbuf} even
when s.svc.List returns an error, which leaves ProjectList.raw nil and causes
Next() to panic; change the return path in the function that calls s.svc.List so
that if err != nil you return nil, err (instead of returning a non-nil
*ProjectList). Update the block around the s.svc.List call to check err and only
construct &ProjectList{raw: resbuf} when err == nil so callers do not receive a
dangling ProjectList with a nil iterator.
In `@projectapi/usage.go`:
- Around line 32-42: Add a client-side validation in GetUsageMetric to reject
invalid metric names before calling service.svc.ResourceOp: check the incoming
metric string against the documented allowed set (e.g., a small whitelist or map
of allowed metric keys) and return a descriptive error immediately if not
present. Place this guard at the top of GetUsageMetric (before
fmt.Sprintf(usageMetricPathFmt, pubKey, metric)) so the request path is never
built for invalid metrics; use the same error type/format conventions the
service uses for input validation.
In `@test/integration_test.go`:
- Around line 110-113: Replace the use of context.Background() when calling
svc.List with a bounded context created via context.WithTimeout to avoid hanging
tests; create a ctx, cancel := context.WithTimeout(context.Background(),
<reasonableDuration>) and defer cancel(), then call svc.List(ctx, nil) and
handle the error as before (t.Fatal on error). Update the test function
surrounding the svc.List call to import/time-reference if needed and choose an
appropriate timeout (e.g., a few seconds) to make CI deterministic.
In `@ucare/uploadapi_test.go`:
- Around line 72-103: Update the two tests TestUploadAPIClient_UserAgent and
TestUploadAPIClient_CustomUserAgent to assert the full User-Agent header equals
the constructed userAgent string rather than only checking substrings: retrieve
the client’s configured userAgent (the same value built by
newUploadAPIClient/resolveConfig using Config.UserAgent and the internal
UploadcareGo prefix/public-key component) and compare
req.Header.Get("User-Agent") to that exact expected string (include both the
UploadcareGo/<version> + public-key component and, for the custom case, the
appended " MyApp/1.0" piece) so the tests fail on any deviation from the
intended userAgent contract.
In `@upload/upload_test.go`:
- Around line 70-85: The test HTTP handler currently only switches on
r.URL.Path, so regressions in HTTP verbs will be missed; update the httptest
server handler (the function passed to httptest.NewServer where srv is created)
to check r.Method for each path (e.g., assert POST for "/base/",
"/multipart/start/", "/multipart/complete/" and GET for "/info/") and return a
405 Method Not Allowed (or fail the test) when the method is unexpected,
incrementing the appropriate counters only after method validation; apply the
same change to the other test server handler blocks referenced (lines handling
"/base/", "/info/", "/multipart/start/", "/multipart/complete/") so all paths
validate the expected HTTP verb.
🪄 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: CHILL
Plan: Pro
Run ID: 15e475e3-ed5e-427e-a617-1409e68d28c6
📒 Files selected for processing (65)
CHANGELOG.mdREADME.mdaddon/addon.goaddon/log.goaddon/service.goaddon/service_test.goconversion/conversion.goconversion/conversion_test.goconversion/path.goconversion/path_test.gofile/file.gofile/list.gofile/service.gofile/service_test.gogroup/group.gogroup/group_test.gogroup/service.gointernal/codec/codec.gointernal/codec/codec_test.gointernal/config/config.gometadata/log.gometadata/metadata.gometadata/service.gometadata/service_test.gometadata/validation.goprojectapi/log.goprojectapi/params.goprojectapi/projectapi.goprojectapi/projects.goprojectapi/secrets.goprojectapi/service.goprojectapi/service_test.goprojectapi/usage.gotest/addon.gotest/file.gotest/group.gotest/integration_test.gotest/metadata.gotest/projectapi.gotest/testenv/runner.gotest/webhook.goucare/cdn.goucare/cdn_test.goucare/client.goucare/doc.goucare/error.goucare/error_test.goucare/projectapi.goucare/projectapi_test.goucare/restapi.goucare/restapi_test.goucare/retry.goucare/retry_test.goucare/uploadapi.goucare/uploadapi_test.goupload/direct.goupload/fromurl.goupload/multipart.goupload/service.goupload/upload.goupload/upload_test.gowebhook/service.gowebhook/service_test.gowebhook/webhook.gowebhook/webhook_test.go
💤 Files with no reviewable changes (1)
- internal/config/config.go
Comment on lines
+60
to
+76
| if opts.Size != "" { | ||
| fmt.Fprintf(&b, "-/size/%s/", opts.Size) | ||
| if opts.ResizeMode != "" { | ||
| fmt.Fprintf(&b, "%s/", opts.ResizeMode) | ||
| } | ||
| } | ||
|
|
||
| if opts.Quality != "" { | ||
| fmt.Fprintf(&b, "-/quality/%s/", opts.Quality) | ||
| } | ||
|
|
||
| if opts.CutStart != "" && opts.CutLength != "" { | ||
| fmt.Fprintf(&b, "-/cut/%s/%s/", opts.CutStart, opts.CutLength) | ||
| } | ||
|
|
||
| if opts.Thumbs > 0 { | ||
| fmt.Fprintf(&b, "-/thumbs~%d/", opts.Thumbs) |
There was a problem hiding this comment.
ResizeMode and Thumbs currently generate malformed video paths.
ResizeMode is appended as a bare segment after size, and thumbs uses ~ instead of /. Both options will build invalid transformation strings when set.
🔧 Proposed fix
if opts.Size != "" {
fmt.Fprintf(&b, "-/size/%s/", opts.Size)
if opts.ResizeMode != "" {
- fmt.Fprintf(&b, "%s/", opts.ResizeMode)
+ fmt.Fprintf(&b, "-/resize_mode/%s/", opts.ResizeMode)
}
}
@@
if opts.Thumbs > 0 {
- fmt.Fprintf(&b, "-/thumbs~%d/", opts.Thumbs)
+ fmt.Fprintf(&b, "-/thumbs/%d/", opts.Thumbs)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if opts.Size != "" { | |
| fmt.Fprintf(&b, "-/size/%s/", opts.Size) | |
| if opts.ResizeMode != "" { | |
| fmt.Fprintf(&b, "%s/", opts.ResizeMode) | |
| } | |
| } | |
| if opts.Quality != "" { | |
| fmt.Fprintf(&b, "-/quality/%s/", opts.Quality) | |
| } | |
| if opts.CutStart != "" && opts.CutLength != "" { | |
| fmt.Fprintf(&b, "-/cut/%s/%s/", opts.CutStart, opts.CutLength) | |
| } | |
| if opts.Thumbs > 0 { | |
| fmt.Fprintf(&b, "-/thumbs~%d/", opts.Thumbs) | |
| if opts.Size != "" { | |
| fmt.Fprintf(&b, "-/size/%s/", opts.Size) | |
| if opts.ResizeMode != "" { | |
| fmt.Fprintf(&b, "-/resize_mode/%s/", opts.ResizeMode) | |
| } | |
| } | |
| if opts.Quality != "" { | |
| fmt.Fprintf(&b, "-/quality/%s/", opts.Quality) | |
| } | |
| if opts.CutStart != "" && opts.CutLength != "" { | |
| fmt.Fprintf(&b, "-/cut/%s/%s/", opts.CutStart, opts.CutLength) | |
| } | |
| if opts.Thumbs > 0 { | |
| fmt.Fprintf(&b, "-/thumbs/%d/", opts.Thumbs) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conversion/path.go` around lines 60 - 76, The size and thumbs path fragments
are being built incorrectly: instead of emitting ResizeMode as a separate
trailing segment after the size, combine it into the size fragment (e.g. produce
"-/size/{size}/{resizeMode}/" in the same fmt call) so the transformation string
remains well-formed (adjust the fmt.Fprintf usage that writes "-/size/%s/" and
the conditional that writes opts.ResizeMode), and change the thumbs fragment to
use slashes instead of a tilde by replacing the "-/thumbs~%d/" fmt with
"-/thumbs/%d/" (update the fmt.Fprintf that references opts.Thumbs accordingly).
Comment on lines
+58
to
+63
| // UploadSettings holds file upload settings for a project. | ||
| type UploadSettings struct { | ||
| FilesizeLimit *int64 `json:"filesize_limit"` | ||
| Autostore *bool `json:"autostore,omitempty"` | ||
| ImageResolutionLimit *int64 `json:"image_resolution_limit"` | ||
| IsSignedUploadEnabled *bool `json:"is_signed_upload_enabled,omitempty"` |
There was a problem hiding this comment.
Add omitempty to the optional upload limits.
UpdateProjectParams is pointer-based so callers can send partial updates, but these two fields will still serialize as null whenever Features.Uploads is present and the limits are left unset. That turns a focused update like toggling Autostore into an explicit write for unrelated fields.
Suggested change
type UploadSettings struct {
- FilesizeLimit *int64 `json:"filesize_limit"`
+ FilesizeLimit *int64 `json:"filesize_limit,omitempty"`
Autostore *bool `json:"autostore,omitempty"`
- ImageResolutionLimit *int64 `json:"image_resolution_limit"`
+ ImageResolutionLimit *int64 `json:"image_resolution_limit,omitempty"`
IsSignedUploadEnabled *bool `json:"is_signed_upload_enabled,omitempty"`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projectapi/projectapi.go` around lines 58 - 63, The UploadSettings struct is
causing null serializations for unset optional limits; update the JSON tags for
FilesizeLimit and ImageResolutionLimit in UploadSettings so they include
`omitempty` (similar to Autostore and IsSignedUploadEnabled) so that when
UpdateProjectParams sends partial updates those unset pointer fields are omitted
instead of serializing as null; locate the UploadSettings type and modify the
`json:"filesize_limit"` and `json:"image_resolution_limit"` tags to use
`omitempty`.
Comment on lines
+147
to
+157
| func resolveBearerConfig(conf *Config) *Config { | ||
| if conf == nil { | ||
| conf = &Config{} | ||
| } else { | ||
| copied := *conf | ||
| conf = &copied | ||
| } | ||
| if conf.HTTPClient == nil { | ||
| conf.HTTPClient = http.DefaultClient | ||
| } | ||
| return conf |
There was a problem hiding this comment.
Deep-copy RetryConfig when cloning Config.
These helpers only copy the top-level struct. Since Retry is a pointer, mutating the caller's conf.Retry after NewClient or NewBearerClient still changes the live client's retry policy and can race with in-flight requests.
🔧 Proposed fix
func resolveBearerConfig(conf *Config) *Config {
if conf == nil {
conf = &Config{}
} else {
copied := *conf
conf = &copied
}
+ if conf.Retry != nil {
+ retryCopy := *conf.Retry
+ conf.Retry = &retryCopy
+ }
if conf.HTTPClient == nil {
conf.HTTPClient = http.DefaultClient
}
return conf
}
func resolveConfig(conf *Config, creds APICreds) *Config {
if conf == nil {
conf = &Config{}
} else {
copied := *conf
conf = &copied
}
+ if conf.Retry != nil {
+ retryCopy := *conf.Retry
+ conf.Retry = &retryCopy
+ }
if conf.APIVersion == "" {
conf.APIVersion = defaultAPIVersion
}Also applies to: 160-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ucare/client.go` around lines 147 - 157, The current resolveBearerConfig (and
the similar resolveConfig helper used for NewClient) shallow-copies the Config
struct so the Retry pointer is shared; change these functions to deep-copy the
Retry field by creating a new RetryConfig when conf.Retry != nil and assigning
its value to the cloned config (e.g., allocate new variable like copied :=
*conf; if conf.Retry != nil { r := *conf.Retry; copied.Retry = &r } ), and
ensure HTTPClient defaulting remains as-is so the returned *Config is fully
independent of the caller's Retry pointer.
Comment on lines
+116
to
+123
| if c.retry.MaxWaitSeconds > 0 && | ||
| retryAfter > c.retry.MaxWaitSeconds { | ||
| return false, ThrottleError{RetryAfter: retryAfter} | ||
| } | ||
| wait := retryAfter | ||
| if wait <= 0 { | ||
| wait = expBackoff(tries) | ||
| } |
There was a problem hiding this comment.
Honor MaxWaitSeconds for fallback backoff too.
MaxWaitSeconds is only enforced when Retry-After is present. If that header is missing or invalid, expBackoff(tries) can still exceed the configured cap and sleep longer than the caller allowed.
Suggested fix
wait := retryAfter
if wait <= 0 {
wait = expBackoff(tries)
}
+ if c.retry.MaxWaitSeconds > 0 && wait > c.retry.MaxWaitSeconds {
+ return false, ThrottleError{RetryAfter: wait}
+ }
select {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ucare/projectapi.go` around lines 116 - 123, The current logic enforces
c.retry.MaxWaitSeconds only when Retry-After is present, but the fallback
expBackoff(tries) can exceed the cap; update the retry path in the function that
computes wait (variables retryAfter, wait and call to expBackoff(tries)) to
enforce the cap for both cases by applying the MaxWaitSeconds limit after
computing wait (e.g., if c.retry.MaxWaitSeconds > 0 then set wait = min(wait,
c.retry.MaxWaitSeconds)) and return ThrottleError{RetryAfter: wait} or proceed
with sleeping using the capped wait so the configured maximum is always honored.
Comment on lines
135
to
+155
| case 400, 404: | ||
| var err respErr | ||
| if e := json.NewDecoder(resp.Body).Decode(&err); e != nil { | ||
| return e | ||
| var apiErr APIError | ||
| if e := json.NewDecoder(resp.Body).Decode(&apiErr); e != nil { | ||
| return false, e | ||
| } | ||
| resp.Body.Close() | ||
| return err | ||
| apiErr.StatusCode = resp.StatusCode | ||
| return false, apiErr | ||
| case 401: | ||
| var err authErr | ||
| if e := json.NewDecoder(resp.Body).Decode(&err); e != nil { | ||
| return e | ||
| var authErr AuthError | ||
| if e := json.NewDecoder(resp.Body).Decode(&authErr); e != nil { | ||
| return false, e | ||
| } | ||
| authErr.StatusCode = 401 | ||
| return false, authErr | ||
| case 403: | ||
| var forbiddenErr ForbiddenError | ||
| if e := json.NewDecoder(resp.Body).Decode(&forbiddenErr); e != nil { | ||
| return false, e | ||
| } | ||
| resp.Body.Close() | ||
| return err | ||
| forbiddenErr.StatusCode = 403 | ||
| return false, forbiddenErr |
There was a problem hiding this comment.
Don't let empty or non-JSON error bodies escape as decoder errors.
These branches return the json.Decoder error directly for 400/401/403/404, so callers lose the HTTP status and typed error information if the upstream body is empty, truncated, or plain text. Please fall back to a typed error with StatusCode/default detail here, the same way the generic >=400 path already does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ucare/restapi.go` around lines 135 - 155, The 400/401/403/404 case branches
currently return the json.Decoder error directly when
json.NewDecoder(resp.Body).Decode(...) fails, losing the HTTP status and typed
error info; update each branch (the 400/404 APIError handling, the 401 AuthError
handling, and the 403 ForbiddenError handling) to catch Decode errors and
instead return the corresponding typed error with StatusCode set (and a sensible
default Message/Detail populated, e.g. from a fallback string or resp.Status)
similar to how the generic ">=400" path builds a fallback error; do not let
decoder errors escape as-is—only return decode errors if you still want to wrap
them, otherwise return the typed error with StatusCode/default detail.
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.
userAgentfield touploadAPIClientand build it fromUserAgentPrefix,ClientVersion, andPublicKey— matching the existing pattern inrestAPIClientandprojectAPIClientUser-Agentheader inuploadAPIClient.NewRequest, which was previously missing entirelyConfig.UserAgentfor custom suffixes (e.g.UploadcareCLI/1.0.0)Without this fix, Upload API requests send Go's default
Go-http-client/2.0as the User-Agent, making it impossible to identify the SDK or any consumer application.Summary by CodeRabbit
New Features
Improvements