api/v2: support TOML output for changefeed query via content negotiation#5357
api/v2: support TOML output for changefeed query via content negotiation#5357JesseZhuang wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds TOML text serialization for API time/duration types, adds TOML struct tags across changefeed config models, implements Accept-based content negotiation via ChangesChangefeed TOML API Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
the
release-note
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces API-layer TOML support for v2 changefeed endpoints, enabling content negotiation via the Accept header to return changefeed configurations in TOML format. It adds toml tags to the configuration models, implements text marshaling/unmarshaling for custom duration and time types to ensure human-readable outputs, and includes comprehensive tests. The feedback suggests making the Accept header check case-insensitive to align with RFC standards, and updating RegionCountRefreshInterval to use *JSONDuration instead of *time.Duration so that it serializes as a human-readable string in TOML.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ti-chi-bot
Bot
added
contribution
|
Hi @JesseZhuang. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/v2/helper.go`:
- Around line 111-122: The TOML branch in respondWithFormat currently calls
c.Error and returns on toml.NewEncoder(...).Encode failure, leaving no HTTP
response; update respondWithFormat so that when toml encoding fails (inside the
wantsTOML branch) it both attaches the error and writes an appropriate HTTP
error response (e.g., use c.AbortWithStatusJSON or c.String/c.Data with status
500 and a brief error message) before returning; ensure you reference
respondWithFormat and the toml encoder error path so the client always receives
a status code and body on failure.
🪄 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: 30502969-440d-4d3d-bdab-5a144bda95f1
📒 Files selected for processing (8)
api/v2/changefeed.goapi/v2/changefeed_toml_test.goapi/v2/helper.goapi/v2/model.gopkg/api/util.gotests/integration_tests/changefeed_query_toml_api/conf/overrides.tomltests/integration_tests/changefeed_query_toml_api/run.shtests/integration_tests/run_light_it_in_ci.sh
JesseZhuang
force-pushed
the
jessezhuang/changefeed-query-toml-api
branch
from
cc5a6da to
2d10447
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/v2/helper.go (1)
112-123:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTOML encoding error still leaves client with no HTTP response.
The past review comment remains valid: when
toml.Encodefails at Line 115, the function callsc.Errorand returns without writing a response body or status code. Gin'sc.Erroronly attaches the error to the context—it doesn't send an HTTP response. The client receives an incomplete HTTP response (no status, no body).You must write an HTTP error response before returning.
🔧 Proposed fix
func respondWithFormat(c *gin.Context, code int, obj any) { if wantsTOML(c) { var buf bytes.Buffer if err := toml.NewEncoder(&buf).Encode(obj); err != nil { _ = c.Error(errors.Trace(err)) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to encode TOML"}) return } c.Data(code, mimeTOML+"; charset=utf-8", buf.Bytes()) return } c.JSON(code, obj) }🤖 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 `@api/v2/helper.go` around lines 112 - 123, respondWithFormat currently calls c.Error on toml encoding failure inside the wantsTOML branch and returns without writing a real HTTP response; update that branch so when toml.NewEncoder(&buf).Encode(obj) returns an error you both attach the error to the context and send an HTTP error response (e.g., use c.String or c.JSON with an appropriate 500 status and a short error message or the error.Error()) before returning; modify the error handling around toml.NewEncoder/Encode in respondWithFormat so the client always receives a status and body even on encoding failures, keeping wantsTOML, toml.NewEncoder, c.Error and c.Data usage intact elsewhere.
🧹 Nitpick comments (1)
tests/integration_tests/changefeed_query_toml_api/run.sh (1)
14-14: 💤 Low valueQuote variables to prevent word splitting and globbing.
Shellcheck flags unquoted variables at Lines 14, 71, 73, 74, and 299. While test environments typically use safe paths, quoting is best practice to prevent unexpected failures if paths contain spaces or glob characters.
📝 Proposed fixes
-source $CUR/../_utils/test_prepare +source "$CUR/../_utils/test_prepare"- rm -rf $WORK_DIR && mkdir -p $WORK_DIR + rm -rf "$WORK_DIR" && mkdir -p "$WORK_DIR"- start_tidb_cluster --workdir $WORK_DIR - run_cdc_server --workdir $WORK_DIR --binary $CDC_BINARY + start_tidb_cluster --workdir "$WORK_DIR" + run_cdc_server --workdir "$WORK_DIR" --binary "$CDC_BINARY"-check_logs $WORK_DIR +check_logs "$WORK_DIR"Also applies to: 71-71, 73-74, 299-299
🤖 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 `@tests/integration_tests/changefeed_query_toml_api/run.sh` at line 14, The shell script uses unquoted variable expansions (notably CUR) when sourcing or using paths which can cause word-splitting/globbing; update occurrences like source $CUR/../_utils/test_prepare and other uses at the same file (lines referenced) to wrap variable expansions in double quotes (e.g., source "$CUR/../_utils/test_prepare" and similarly quote "$CUR", "$BIN_DIR", or other variables used at lines 71, 73, 74, 299) so all path variables are quoted consistently to prevent splitting/globbing.
🤖 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.
Duplicate comments:
In `@api/v2/helper.go`:
- Around line 112-123: respondWithFormat currently calls c.Error on toml
encoding failure inside the wantsTOML branch and returns without writing a real
HTTP response; update that branch so when toml.NewEncoder(&buf).Encode(obj)
returns an error you both attach the error to the context and send an HTTP error
response (e.g., use c.String or c.JSON with an appropriate 500 status and a
short error message or the error.Error()) before returning; modify the error
handling around toml.NewEncoder/Encode in respondWithFormat so the client always
receives a status and body even on encoding failures, keeping wantsTOML,
toml.NewEncoder, c.Error and c.Data usage intact elsewhere.
---
Nitpick comments:
In `@tests/integration_tests/changefeed_query_toml_api/run.sh`:
- Line 14: The shell script uses unquoted variable expansions (notably CUR) when
sourcing or using paths which can cause word-splitting/globbing; update
occurrences like source $CUR/../_utils/test_prepare and other uses at the same
file (lines referenced) to wrap variable expansions in double quotes (e.g.,
source "$CUR/../_utils/test_prepare" and similarly quote "$CUR", "$BIN_DIR", or
other variables used at lines 71, 73, 74, 299) so all path variables are quoted
consistently to prevent splitting/globbing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ebfdd24-e206-4b8c-97e3-4c2771cc7cff
📒 Files selected for processing (8)
api/v2/changefeed.goapi/v2/changefeed_toml_test.goapi/v2/helper.goapi/v2/model.gopkg/api/util.gotests/integration_tests/changefeed_query_toml_api/conf/overrides.tomltests/integration_tests/changefeed_query_toml_api/run.shtests/integration_tests/run_light_it_in_ci.sh
✅ Files skipped from review due to trivial changes (1)
- tests/integration_tests/run_light_it_in_ci.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- api/v2/changefeed.go
- tests/integration_tests/changefeed_query_toml_api/conf/overrides.toml
- pkg/api/util.go
- api/v2/changefeed_toml_test.go
- api/v2/model.go
Add API-layer TOML support for the GET single changefeed endpoint, the foundation for issue pingcap#4936 (changefeed query as TOML / clean config export). - Add kebab-case `toml` struct tags across the api/v2 ReplicaConfig tree and the ChangeFeedInfo top-level fields, matching the internal config.ReplicaConfig tags that `changefeed create --config` parses, so the output round-trips. - Add MarshalText/UnmarshalText to JSONDuration (api/v2) and JSONTime (pkg/api) so durations and timestamps serialize as human-readable strings under TOML instead of empty tables; JSON output is unchanged (still nanoseconds). - Omit the runtime-only GID field from TOML (toml:"-"). - Add a respondWithFormat helper that negotiates on the Accept header: application/toml returns TOML, otherwise JSON (the unchanged default). - Fix two pre-existing JSON tag inconsistencies on SinkConfig: advance_timeout -> advance_timeout_in_sec, send-all-bootstrap-at-start -> send_all_bootstrap_at_start. Unit tests cover duration/time round-trips, full-default-config round-trip into the internal config (guards all tags against drift), and Accept negotiation. A new integration test (changefeed_query_toml_api) exercises the HTTP endpoint and re-imports the exported TOML via `changefeed create --config`. Issue Number: ref pingcap#4936 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JesseZhuang
force-pushed
the
jessezhuang/changefeed-query-toml-api
branch
from
2d10447 to
6dcca2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@api/v2/changefeed_toml_test.go`:
- Around line 166-169: The test in ChangeFeedInfo contains a t.Parallel() call
that enables parallel execution, but the gin.SetMode() call on the subsequent
line mutates global process state which can cause race conditions and test
flakiness. Remove the t.Parallel() call from the beginning of this test since it
is incompatible with the global state mutation caused by
gin.SetMode(gin.TestMode).
In `@tests/integration_tests/changefeed_query_toml_api/run.sh`:
- Line 14: Quote all unquoted variable expansions to prevent breaking on
whitespace or globbing in workspace paths. In the run.sh file, add double quotes
around variable expansions at lines 14 (the $CUR variable in the source
command), lines 71, 73-74, and line 299. For example, change `source
$CUR/../_utils/test_prepare` to `source "$CUR"/../_utils/test_prepare` to ensure
the path is treated as a single argument even if it contains spaces or special
characters.
🪄 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: 3b4097b5-aef6-4946-807e-945f03543c99
📒 Files selected for processing (8)
api/v2/changefeed.goapi/v2/changefeed_toml_test.goapi/v2/helper.goapi/v2/model.gopkg/api/util.gotests/integration_tests/changefeed_query_toml_api/conf/overrides.tomltests/integration_tests/changefeed_query_toml_api/run.shtests/integration_tests/run_light_it_in_ci.sh
✅ Files skipped from review due to trivial changes (1)
- tests/integration_tests/changefeed_query_toml_api/conf/overrides.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- api/v2/changefeed.go
- pkg/api/util.go
- api/v2/helper.go
- tests/integration_tests/run_light_it_in_ci.sh
- api/v2/model.go
| t.Parallel() | ||
|
|
||
| gin.SetMode(gin.TestMode) | ||
| obj := &ChangeFeedInfo{ID: "cf-1", SinkURI: "blackhole://"} |
There was a problem hiding this comment.
Avoid parallelizing a test that mutates Gin global state.
Line 166 runs in parallel while Line 168 calls gin.SetMode(...), which mutates global process state and can make sibling tests flaky.
As per coding guidelines, "Prefer focused deterministic tests; see docs/agents/testing.md before adding or changing tests."
Suggested fix
func TestRespondWithFormat(t *testing.T) {
- t.Parallel()
-
gin.SetMode(gin.TestMode)
obj := &ChangeFeedInfo{ID: "cf-1", SinkURI: "blackhole://"}📝 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.
| t.Parallel() | |
| gin.SetMode(gin.TestMode) | |
| obj := &ChangeFeedInfo{ID: "cf-1", SinkURI: "blackhole://"} | |
| func TestRespondWithFormat(t *testing.T) { | |
| gin.SetMode(gin.TestMode) | |
| obj := &ChangeFeedInfo{ID: "cf-1", SinkURI: "blackhole://"} |
🤖 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 `@api/v2/changefeed_toml_test.go` around lines 166 - 169, The test in
ChangeFeedInfo contains a t.Parallel() call that enables parallel execution, but
the gin.SetMode() call on the subsequent line mutates global process state which
can cause race conditions and test flakiness. Remove the t.Parallel() call from
the beginning of this test since it is incompatible with the global state
mutation caused by gin.SetMode(gin.TestMode).
Source: Coding guidelines
| set -euo pipefail | ||
|
|
||
| CUR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | ||
| source $CUR/../_utils/test_prepare |
There was a problem hiding this comment.
Quote path/binary variables used as arguments.
Line 14, Line 71, Line 73, Line 74, and Line 299 use unquoted expansions; this can break on whitespace/globbing in workspace paths.
Suggested fix
-source $CUR/../_utils/test_prepare
+source "$CUR/../_utils/test_prepare"
@@
- rm -rf $WORK_DIR && mkdir -p $WORK_DIR
+ rm -rf "$WORK_DIR" && mkdir -p "$WORK_DIR"
@@
- start_tidb_cluster --workdir $WORK_DIR
- run_cdc_server --workdir $WORK_DIR --binary $CDC_BINARY
+ start_tidb_cluster --workdir "$WORK_DIR"
+ run_cdc_server --workdir "$WORK_DIR" --binary "$CDC_BINARY"
@@
-check_logs $WORK_DIR
+check_logs "$WORK_DIR"Also applies to: 71-71, 73-74, 299-299
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 14-14: Not following: ./../_utils/test_prepare was not specified as input (see shellcheck -x).
(SC1091)
[info] 14-14: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@tests/integration_tests/changefeed_query_toml_api/run.sh` at line 14, Quote
all unquoted variable expansions to prevent breaking on whitespace or globbing
in workspace paths. In the run.sh file, add double quotes around variable
expansions at lines 14 (the $CUR variable in the source command), lines 71,
73-74, and line 299. For example, change `source $CUR/../_utils/test_prepare` to
`source "$CUR"/../_utils/test_prepare` to ensure the path is treated as a single
argument even if it contains spaces or special characters.
Source: Linters/SAST tools
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lidezhu, wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
cdc cli changefeed create --configconsumes TOML, but the changefeed query API and CLI only return JSON. This asymmetry makes changefeed migration, config review, and version control more manual than necessary.Per the design discussion in #4936, the feature is implemented at the API layer (content negotiation) rather than as a CLI-only conversion. This PR is the foundation for that.
Issue Number: ref #4936
What is changed and how it works?
Add
Accept: application/tomlcontent negotiation to the single changefeed GET endpoint (GET /api/v2/changefeeds/:changefeed_id). With noAcceptheader orAccept: application/json, the response is JSON exactly as before; withAccept: application/toml, the same data is returned as TOML.How it works:
tomltags across theReplicaConfigtree and the top-levelChangeFeedInfofields, matching the internalconfig.ReplicaConfigtags thatchangefeed create --configparses, so the output round-trips back into create.MarshalText/UnmarshalTexttoJSONDuration(api/v2) andJSONTime(pkg/api) so TOML renders durations as10m0sand timestamps as2006-01-02 15:04:05.000instead of empty tables/raw nanoseconds. JSON output is unchanged —JSONDurationstill marshals to nanoseconds in JSON.respondWithFormathelper reads theAcceptheader and emits TOML viac.Data(...)or falls back toc.JSON(...). OnlyGetChangeFeeduses it; LIST and all other endpoints are unchanged.toml:"-"): itsuint64low/highcan exceed TOML's signed int64 range so the encoded value would not parse back, and it is internal runtime metadata, not part of a creatable config. It is still present in the JSON response (unchanged).SinkConfig(agreed in cdc cli: support TOML output and migration-optimized diff for changefeed query #4936):advance_timeout→advance_timeout_in_sec,send-all-bootstrap-at-start→send_all_bootstrap_at_start.This is PR 1 of 3 for #4936. PR 2 will add the
?view=configquery parameter (diff-against-defaults, stripping runtime fields). PR 3 will add the CLI--formatand--viewflags.Check List
Tests
New unit tests cover:
JSONDuration/JSONTimetext round-trips; encoding the full default config to TOML and decoding it back into the internalconfig.ReplicaConfigwith zero undecoded keys (guards all ~200 tags against drift); andAcceptheader content negotiation (default/json/toml/toml-with-charset).Integration tests
Added
changefeed_query_toml_api(wired intotests/integration_tests/run_light_it_in_ci.sh), run locally on macOS against a real cluster — all sub-tests passing:Accept: application/jsonboth return JSONAccept: application/tomlreturns a TOML body with kebab-case top-level keys;gidomitted[config]/[config.*]JSONTimefields render as readable timestampschangefeed create --configQuestions
Will it cause performance regression or break compatibility?
No performance impact. The default (JSON) response path is unchanged. The two
SinkConfigJSON tag renames change the field names in API responses (advance_timeout→advance_timeout_in_sec,send-all-bootstrap-at-start→send_all_bootstrap_at_start) — please confirm these are acceptable.Do you need to update user documentation, design documentation or monitoring documentation?
User documentation for TOML output will be added once the CLI flags land in PR 3.
Notes for reviewer
state,error,checkpoint-time); stripping them is handled by?view=configin PR 2.GIDis omitted from TOML (toml:"-"): itsuint64low/highcan exceed TOML's signed int64 range, so the encoded value would not parse back, and it is internal runtime metadata rather than part of a creatable config. It remains present in the JSON response (unchanged).Release note
Summary by CodeRabbit
New Features
Accept: application/toml, including parameterized headers).Bug Fixes
200payload.Tests