Fix Upload width/height: spec truth over wire quirk#171
Conversation
Spec Change ImpactChanges in Spec
SDK Regeneration
Breaking API Change
SDK Update Checklist:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="go/pkg/types/flexint.go">
<violation number="1" location="go/pkg/types/flexint.go:18">
P2: Use `math.Round(f)` instead of truncating. Truncation toward zero means a value like `1023.9999999999998` (a plausible floating-point artifact for `1024`) would become `1023`. Rounding gives the semantically correct result for values that are conceptually integers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec8d4679d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR fixes a wire-format mismatch for Upload.width/Upload.height by keeping the Smithy/OpenAPI spec semantically correct (integers) while making the Go SDK tolerant of the API’s float-encoded integer responses (e.g., 1024.0).
Changes:
- Reverts
Upload.width/heightin Smithy/OpenAPI from double/number back to integer. - Introduces
types.FlexIntin Go to unmarshal integer-like JSON floats into an integer type. - Extends the OpenAPI enhancement script to apply
x-go-type: types.FlexIntforwidth/height, and regenerates the Go client/types accordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/basecamp.smithy | Changes Upload.width/height back to Integer to keep the spec truthful. |
| scripts/enhance-openapi-go-types.sh | Adds jq rule to map width/height integer schema fields to types.FlexInt. |
| openapi.json | Updates Upload.width/height schema to integer + Go type overrides (types.FlexInt). |
| go/pkg/types/flexint.go | Adds FlexInt JSON marshal/unmarshal implementation. |
| go/pkg/types/flexint_test.go | Adds unit tests for FlexInt JSON behavior. |
| go/pkg/generated/client.gen.go | Updates generated Upload model to use types.FlexInt for width/height. |
| go/pkg/basecamp/vaults.go | Updates public SDK Upload model to use types.FlexInt for width/height. |
| go/pkg/basecamp/vaults_test.go | Updates assertions/formatting for new integer-like width/height type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The BC3 API serializes pixel dimensions as 1024.0 rather than 1024. Go's encoding/json rejects float literals into int fields, unlike every other language. FlexInt bridges this: an int32 that unmarshals from any JSON number whose value is integral and in range. Follows the types.Date pattern for wire-format mismatches resolved via x-go-type. Non-integral values and int32 overflow are rejected with errors, matching the int32 schema in openapi.json.
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous fix (4d7ec42) changed the spec from Integer to Double to work around Go's strict JSON unmarshaling. That made the spec lie — pixel dimensions are integers; 1024.0 is a wire encoding detail. Revert the Smithy spec to Integer and instead wire types.FlexInt through the enhancement script's x-go-type mechanism, the same pattern used for types.Date. The spec now tells the truth while Go handles the wire quirk transparently. FlexInt stays in the generated layer only — the hand-written Upload struct exposes plain int, with conversion at the boundary in uploadFromGenerated. The enhancement rule is scoped to the Upload schema specifically, not applied by field name globally.
Summary
types.FlexInt, a custom Go type that unmarshals from any JSON number whose value is integral and within int32 rangeenhance-openapi-go-types.shviax-go-type, following the establishedtypes.Datepattern for wire-format mismatchesUploadstruct exposes plainint, with conversion at the boundaryThe previous fix (4d7ec42) changed the spec itself to Double, making it lie about the data type. This fix keeps the spec honest and handles the Go-specific unmarshaling quirk where it belongs — in the Go type system.
Closes #168
Design decisions
1024.5) and int32 overflow (1e20) with errors, matching theint32schema in openapi.json. No silent truncation or saturation.Upload.WidthandUpload.Heightremain plainintin the hand-written struct. FlexInt is an internal detail of the generated client layer, converted at theuploadFromGeneratedboundary.components.schemas.Upload.propertiesspecifically, not allwidth/heightfields globally, avoiding surprises if integer width/height appears elsewhere in the spec.Test plan
go test ./go/pkg/types/...— FlexInt unit tests (int, float, zero, negative, fractional rejection, overflow rejection, marshal, round-trip)go test ./go/pkg/basecamp/...— Upload unmarshal tests go through generated→conversion path with1024.0fixturesmake smithy-check— OpenAPI in sync with Smithymake go-check-drift— Service layer in sync with generated clientmake check— Full CI suite passes