fix(export): reject unsupported -o format on empty resource lists#53
Closed
shreemaan-abhishek wants to merge 1 commit into
Closed
fix(export): reject unsupported -o format on empty resource lists#53shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
|
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 (12)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR introduces early export format validation by centralizing format checking in a ChangesExport Format Validation
🎯 2 (Simple) | ⏱️ ~8 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: 2
🧹 Nitpick comments (1)
test/e2e/permutation_report_test.go (1)
58-64: 💤 Low valueByte-based truncation may split multi-byte UTF-8 characters.
len(s)returns byte count ands[:500]slices by bytes. If stderr contains multi-byte UTF-8 at the boundary, this could produce invalid UTF-8 in reports. For CLI output this is typically ASCII, so impact is minimal.If you want UTF-8-safe truncation:
♻️ Optional: rune-safe truncation
func first500(s string) string { s = strings.TrimSpace(s) - if len(s) > 500 { - return s[:500] + runes := []rune(s) + if len(runes) > 500 { + return string(runes[:500]) } return s }🤖 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 `@test/e2e/permutation_report_test.go` around lines 58 - 64, The function first500 uses byte-based len and slicing which can split multi-byte UTF-8 characters; change it to perform rune-safe truncation: TrimSpace the input, convert to a rune slice (e.g., runes := []rune(s)), and if len(runes) > 500 return string(runes[:500]) else return s (or string(runes)) so the returned string is valid UTF-8; update the function first500 accordingly.
🤖 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 `@test/e2e/docker-compose.yml`:
- Around line 15-16: The port mappings "9090:9090" and "9100:9100" expose
services on all interfaces; update those Docker Compose port entries to bind to
loopback by prefixing the host port with 127.0.0.1 (e.g., change "9090:9090" to
"127.0.0.1:9090:9090" and "9100:9100" to "127.0.0.1:9100:9100") so the
control/stream endpoints are only accessible locally.
In `@test/e2e/permutation_test.go`:
- Around line 154-159: The TCP dial block currently discards the error returned
by conn.Close(); update the logic around net.DialTimeout and conn.Close so you
capture and handle Close's error instead of ignoring it—for example, check the
error returned by conn.Close() and treat a non-nil close error as a failure
(return false or propagate an error) so resource/teardown failures aren't
suppressed; apply the same change pattern to the other occurrences that discard
Close errors (the other conn.Close usages at the noted locations).
---
Nitpick comments:
In `@test/e2e/permutation_report_test.go`:
- Around line 58-64: The function first500 uses byte-based len and slicing which
can split multi-byte UTF-8 characters; change it to perform rune-safe
truncation: TrimSpace the input, convert to a rune slice (e.g., runes :=
[]rune(s)), and if len(runes) > 500 return string(runes[:500]) else return s (or
string(runes)) so the returned string is valid UTF-8; update the function
first500 accordingly.
🪄 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: 29f78c5c-4d5c-4a99-a1be-e44121ad8001
📒 Files selected for processing (21)
.gitignoreMakefilepkg/cmd/consumer/export/export.gopkg/cmd/consumergroup/export/export.gopkg/cmd/globalrule/export/export.gopkg/cmd/pluginconfig/export/export.gopkg/cmd/proto/export/export.gopkg/cmd/route/export/export.gopkg/cmd/service/export/export.gopkg/cmd/ssl/export/export.gopkg/cmd/streamroute/export/export.gopkg/cmd/upstream/export/export.gopkg/cmdutil/exporter.gopkg/cmdutil/exporter_test.gotest/e2e/apisix_conf/config-docker.yamltest/e2e/docker-compose.permutation.ymltest/e2e/docker-compose.ymltest/e2e/permutation_cleanup_test.gotest/e2e/permutation_matrix_test.gotest/e2e/permutation_report_test.gotest/e2e/permutation_test.go
Comment on lines
+154
to
+159
| conn, err := net.DialTimeout("tcp", u.Host, 500*time.Millisecond) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| _ = conn.Close() | ||
| return true |
There was a problem hiding this comment.
Handle cleanup/close errors instead of discarding them.
Several calls currently suppress errors, which can hide resource-leak and teardown failures in permutation runs.
🔧 Suggested fix pattern
func canReach(rawURL string) bool {
@@
- _ = conn.Close()
- return true
+ if err := conn.Close(); err != nil {
+ return false
+ }
+ return true
}
@@
- DeferCleanup(func() {
- _, _ = adminDeleteByID(spec.AdminPath, id)
- })
+ DeferCleanup(func() {
+ _, err := adminDeleteByID(spec.AdminPath, id)
+ Expect(err).NotTo(HaveOccurred())
+ })
@@
- DeferCleanup(func() {
- _, _ = adminDeleteByID("/apisix/admin/plugin_metadata", plugin)
- })
+ DeferCleanup(func() {
+ _, err := adminDeleteByID("/apisix/admin/plugin_metadata", plugin)
+ Expect(err).NotTo(HaveOccurred())
+ })
@@
- DeferCleanup(func() {
- _, _ = adminDeleteByID("/apisix/admin/routes", id)
- })
+ DeferCleanup(func() {
+ _, err := adminDeleteByID("/apisix/admin/routes", id)
+ Expect(err).NotTo(HaveOccurred())
+ })As per coding guidelines, Never suppress errors; always handle and propagate them.
Also applies to: 304-306, 397-399, 463-465
🤖 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 `@test/e2e/permutation_test.go` around lines 154 - 159, The TCP dial block
currently discards the error returned by conn.Close(); update the logic around
net.DialTimeout and conn.Close so you capture and handle Close's error instead
of ignoring it—for example, check the error returned by conn.Close() and treat a
non-nil close error as a failure (return false or propagate an error) so
resource/teardown failures aren't suppressed; apply the same change pattern to
the other occurrences that discard Close errors (the other conn.Close usages at
the noted locations).
Every `<resource> export` command short-circuited with "No X found." and returned exit 0 when the collection was empty, even if -o named an unsupported format. Format validation happened lazily inside Exporter.Write, which was never reached on the empty path. The result was an inconsistent contract: `a6 route export -o table` errored loudly when routes existed but silently succeeded otherwise. Move the validation up. Add cmdutil.ValidateExportFormat and call it at the top of every export command so an invalid -o is rejected before any I/O, regardless of result-set size. Surfaced by the new permutation suite (tier 4).
shreemaan-abhishek
force-pushed
the
cli-permutation-suite
branch
from
6279ee6 to
380edfe
Compare
shreemaan-abhishek
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent behavior across resource export commands where an unsupported -o/--output format could silently succeed (exit 0) when the exported collection was empty, by validating the output format before any early-return paths.
Changes:
- Added
cmdutil.ValidateExportFormatto validate supported export formats (json,yaml, and""as a special case). - Invoked
ValidateExportFormatat the start of everyexportRunimplementation across all export commands. - Added unit tests for
ValidateExportFormat.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmdutil/exporter.go | Adds ValidateExportFormat helper for early format validation. |
| pkg/cmdutil/exporter_test.go | Adds unit tests covering accepted/rejected formats for ValidateExportFormat. |
| pkg/cmd/upstream/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/streamroute/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/ssl/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/service/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/route/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/proto/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/pluginconfig/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/globalrule/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/consumergroup/export/export.go | Validates output format at start of exportRun. |
| pkg/cmd/consumer/export/export.go | Validates output format at start of exportRun. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+40
| func ValidateExportFormat(format string) error { | ||
| switch format { | ||
| case "", "json", "yaml": | ||
| return nil | ||
| default: | ||
| return fmt.Errorf("unsupported output format: %s", format) | ||
| } | ||
| } |
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Comment on lines
52
to
+56
| func exportRun(opts *Options) error { | ||
| if err := cmdutil.ValidateExportFormat(opts.Output); err != nil { | ||
| return err | ||
| } | ||
|
|
Contributor
Author
|
Closing without merging. After discussion, the empty-collection + invalid-format scenario isn't a real bug: "No X found." is higher-priority information for an interactive user than the format error, and the probability is low (typo on For reference, the inconsistency this PR addressed is:
Accepting that inconsistency as documented behaviour. The follow-up permutation suite will treat the empty-collection case as Thanks @coderabbitai for the duplication / regression-test feedback — both fair points, but they're moot now that we're not changing the behaviour. |
|
Tip For best results, initiate chat on the files or code changes.
Appreciated the context, and good luck with the follow-up permutation suite! 🐇 ✏️ Learnings added
|
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
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.
Summary
Every
<resource> exportcommand short-circuited with "No X found." and returned exit 0 when the collection was empty, even if-onamed an unsupported format. Format validation lived insideExporter.Write, which the empty path never reached. Result:a6 route export -o tableerrored loudly when routes existed but silently succeeded otherwise. Same pattern across all 10 export commands.The fix adds
cmdutil.ValidateExportFormatand calls it at the top of every export command, so an invalid-ois rejected before any I/O — independent of result-set size.Test plan
go test ./pkg/cmdutil/...— new unit test forValidateExportFormat(accepts""/json/yaml, rejectstable/garbage).a6 route export -o garbageon an empty server now exits non-zero withunsupported output format: garbage. Was exit 0 +No routes found.before.How it was found
Surfaced by a tiered combinatorial CLI stability suite I'm prepping in a follow-up PR. The output-format tier ran every export command against an empty server, and 7 of them silently passed with
-o garbage-format. Same root cause across all of them, so fixing them together as one PR keeps the contract consistent.The follow-up that adds the suite itself depends on this fix landing so its CI is green.
Summary by CodeRabbit
Bug Fixes
Tests