fix: decode API responses before exporting + CI verbosity#43
Merged
Conversation
Writing a json.RawMessage directly to the yaml.v3 encoder serializes the bytes as a list of integers, not a map. PR #31 fixed this for `plugin-metadata get` and PR #39's review fixup did it for one stream-route create path, but the same anti-pattern lived in 21 other sites across 12 resources (every flag/file create and update path for route, service, consumer, credential, ssl, secret, global-rule, proto, plugin-metadata, stream-route, gateway-group). - Add cmdutil.Exporter.WriteAPIResponse(body []byte) that decodes once via encoding/json and writes through the existing Write() formatter. - Replace all 21 `.Write(json.RawMessage(body))` call sites with `.WriteAPIResponse(body)`. - Drop now-unused encoding/json imports from consumer create/update. - Unit tests for the helper cover YAML emits a map (the regression guard), JSON round-trips an object, and invalid JSON returns a clear error. Verified live against API7 EE 3.9.12: service/route file-based create `-o yaml` now emit proper maps instead of integer sequences. Closes #32.
Without -v, `go test` prints nothing between tests; combined with a silent TestMain (binary build + dashboard health-wait + group resolve) the CI log shows nothing for the first 2+ minutes of a healthy run. - Add -v to both `make test-e2e` and `make test-e2e-full` so each test emits === RUN / --- PASS lines as it runs. - Print three progress markers to stderr in TestMain so the dark setup phase has visible breadcrumbs: building binary, waiting for dashboard, resolving gateway group. Closes #41.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Exporter.WriteAPIResponse to decode raw API JSON responses before formatting, applies it across CLI create/update file-based flows replacing json.RawMessage usage, adds tests for the exporter, and increases e2e test verbosity with setup progress messages. ChangesAPI Response Exporter Fix and Application
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🤖 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 `@pkg/cmd/ssl/create/create.go`:
- Line 159: The current code falls back to
cmdutil.NewExporter(...).WriteAPIResponse(body) when decoding api.SSL fails,
risking unredacted secret fields; change the control flow so that if decoding
api.SSL returns an error you do NOT call WriteAPIResponse(body) — instead
return/propagate the decode error (or send a generic error response) and only
call WriteAPIResponse after successfully decoding and applying api.RedactSSL on
the SSL struct; specifically, locate the branch where api.SSL is decoded and
replace the fallback to WriteAPIResponse(body) with error handling (or a
sanitized response) and ensure you call api.RedactSSL(&ssl) before any
serialization.
In `@pkg/cmd/ssl/update/update.go`:
- Line 182: The fallback path currently calls cmdutil.NewExporter(format,
out).WriteAPIResponse(body) which can export unredacted SSL secrets; update the
fallback to first call api.RedactSSL on the response payload (or construct a
redacted copy) and pass that redacted value to WriteAPIResponse so cert/key
material is never serialized, ensuring any SSL-bearing struct uses its
Redact/Marshal implementations; look for WriteAPIResponse, cmdutil.NewExporter,
and api.RedactSSL in the update handler and replace the direct
WriteAPIResponse(body) usage with a call that redacts (or converts to a redacted
struct) before exporting.
In `@pkg/cmdutil/exporter_test.go`:
- Line 1: This new Go test file is missing the project Apache 2.0 license
header; add the standard Apache 2.0 source header comment block at the very top
of the file above the existing "package cmdutil" declaration so the file begins
with the license header followed by the package statement, ensuring formatting
and year/owner placeholders follow the project's header template.
In `@pkg/cmdutil/exporter.go`:
- Around line 42-47: The Exporter.WriteAPIResponse currently uses json.Unmarshal
into an interface{}, which converts numbers to float64 and can lose precision;
change it to create a json.Decoder on an io.Reader (e.g.,
bytes.NewReader(body)), call decoder.UseNumber() before Decode(&decoded) so
numeric values are preserved as json.Number, then return e.Write(decoded) as
before; also add a unit test that sends a JSON payload with an integer > 2^53
through WriteAPIResponse and asserts the exported output preserves the exact
integer string to catch regressions.
🪄 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: ee665c17-ce9a-4499-89f0-ecacc6fad52e
📒 Files selected for processing (23)
Makefilepkg/cmd/consumer/create/create.gopkg/cmd/consumer/update/update.gopkg/cmd/credential/create/create.gopkg/cmd/credential/update/update.gopkg/cmd/gateway-group/create/create.gopkg/cmd/gateway-group/update/update.gopkg/cmd/global-rule/create/create.gopkg/cmd/global-rule/update/update.gopkg/cmd/plugin-metadata/create/create.gopkg/cmd/plugin-metadata/update/update.gopkg/cmd/proto/create/create.gopkg/cmd/proto/update/update.gopkg/cmd/route/create/create.gopkg/cmd/route/update/update.gopkg/cmd/service/create/create.gopkg/cmd/service/update/update.gopkg/cmd/ssl/create/create.gopkg/cmd/ssl/update/update.gopkg/cmd/stream-route/update/update.gopkg/cmdutil/exporter.gopkg/cmdutil/exporter_test.gotest/e2e/setup_test.go
The previous fallback dumped the raw response body via WriteAPIResponse, bypassing api.RedactSSL and exporting the private key in plaintext when the body didn't decode into api.SSL. Return an error in that path so the key is never serialized unredacted.
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.
Two changes bundled — closes #32 and #41.
1. Decode API responses before exporting (closes #32)
Writing a
json.RawMessagedirectly toyaml.v3serializes the bytes as a list of integers, not a map. PR #31 fixed this forplugin-metadata getand PR #39's review fixup patched onestream-route createpath, but the same anti-pattern lived in 21 other call sites across 12 resources.Audit result:
All 21 sites silently produced byte-sequence YAML under
-o yaml.Fix:
cmdutil.Exporter.WriteAPIResponse(body []byte)decodes once viaencoding/jsonthen writes through the existingWrite()formatter..Write(json.RawMessage(body))→.WriteAPIResponse(body).encoding/jsonimports fromconsumer/createandconsumer/update.TestExporter_WriteAPIResponse_YAML_DecodesObject— the regression guard (output must parse as a YAML map, not a byte sequence).TestExporter_WriteAPIResponse_JSON_RoundTripsObject— JSON path still emits a proper object.TestExporter_WriteAPIResponse_InvalidJSON_ReturnsError— clear error on undecodable body.Verified live against API7 EE 3.9.12:
service create -f -o yamlandroute create -f -o yamlnow emit YAML maps instead of integer sequences.2. CI verbosity (closes #41)
Without
-v,go testprints nothing between tests; combined with a silentTestMain(binary build + dashboard health-wait + group resolve), the CI log is dark for the first 2+ minutes of a healthy run.-vto bothmake test-e2eandmake test-e2e-full.TestMain: "Building a7 binary ...", "Waiting for API7 EE dashboard at ...", "Resolving gateway group ...".Verified live: progress markers fire in order, per-test
--- PASS:lines stream out.Test plan
go build,go vet, fullgo test ./...green locally.-o yamlproduce proper YAML maps.TestPluginMetadata_GetYAML,TestStreamRoute_CreateFromFileYAML,TestRoute_DescFlagCRUD) still pass.Part of #22.
Summary by CodeRabbit
Tests
Refactor
Bug Fixes