Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/v2/changefeed.go
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ func (h *OpenAPIV2) GetChangeFeed(c *gin.Context) {

taskStatus := make([]config.CaptureTaskStatus, 0)
detail := CfInfoToAPIModel(cfInfo, status, taskStatus)
c.JSON(http.StatusOK, detail)
respondWithFormat(c, http.StatusOK, detail)
}

func shouldShowRunningError(state config.FeedState) bool {
Expand Down
197 changes: 197 additions & 0 deletions api/v2/changefeed_toml_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
// Copyright 2026 PingCAP, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// See the License for the specific language governing permissions and
// limitations under the License.

package v2

import (
"bytes"
"net/http/httptest"
"testing"
"time"

"github.com/BurntSushi/toml"
"github.com/gin-gonic/gin"
"github.com/pingcap/ticdc/pkg/config"
"github.com/pingcap/ticdc/pkg/util"
"github.com/stretchr/testify/require"
)

// TestJSONDurationTextRoundTrip verifies JSONDuration serializes to and parses
// from a human-readable string, which is what makes TOML output round-trippable.
func TestJSONDurationTextRoundTrip(t *testing.T) {
t.Parallel()

cases := []time.Duration{
10 * time.Minute,
24 * time.Hour,
5 * time.Second,
30*time.Minute + 15*time.Second,
}
for _, dur := range cases {
d := JSONDuration{duration: dur}
text, err := d.MarshalText()
require.NoError(t, err)

var d2 JSONDuration
require.NoError(t, d2.UnmarshalText(text))
require.Equal(t, d.duration, d2.duration,
"round-trip failed for %v: got text %q", dur, string(text))
}
}

// TestJSONDurationUnmarshalTextInvalid verifies invalid duration text is rejected.
func TestJSONDurationUnmarshalTextInvalid(t *testing.T) {
t.Parallel()

var d JSONDuration
require.Error(t, d.UnmarshalText([]byte("not-a-duration")))
require.Error(t, d.UnmarshalText([]byte("")))
require.Error(t, d.UnmarshalText([]byte("1d"))) // Go doesn't support "d" unit
}

// TestJSONDurationTOMLNotEmptyTable verifies a JSONDuration field encodes as a
// readable string rather than an empty TOML table (the failure mode that the
// MarshalText/UnmarshalText methods fix).
func TestJSONDurationTOMLNotEmptyTable(t *testing.T) {
t.Parallel()

cfg := &ReplicaConfig{
SyncPointInterval: &JSONDuration{duration: 10 * time.Minute},
}
var buf bytes.Buffer
require.NoError(t, toml.NewEncoder(&buf).Encode(cfg))
out := buf.String()
require.Contains(t, out, "sync-point-interval")
require.Contains(t, out, "10m0s")
require.NotContains(t, out, "[sync-point-interval]") // not an (empty) table
}

// TestChangeFeedInfoTOMLRoundTripToInternal is the highest-value test: it encodes
// a ChangeFeedInfo to TOML, then decodes the config back into the internal
// config.ReplicaConfig (the exact target that `changefeed create --config`
// parses), proving the TOML output is import-compatible.
func TestChangeFeedInfoTOMLRoundTripToInternal(t *testing.T) {
t.Parallel()

info := &ChangeFeedInfo{
ID: "test-cf",
SinkURI: "blackhole://",
StartTs: 449999999999999999,
Config: &ReplicaConfig{
MemoryQuota: util.AddressOf(uint64(1024)),
CaseSensitive: util.AddressOf(true),
ForceReplicate: util.AddressOf(true),
CheckGCSafePoint: util.AddressOf(false),
SyncPointInterval: &JSONDuration{duration: 10 * time.Minute},
Integrity: &IntegrityConfig{
IntegrityCheckLevel: util.AddressOf("correctness"),
CorruptionHandleLevel: util.AddressOf("warn"),
},
Consistent: &ConsistentConfig{
Level: util.AddressOf("eventual"),
MaxLogSize: util.AddressOf(int64(128)),
FlushIntervalInMs: util.AddressOf(int64(2000)),
Storage: util.AddressOf("s3://test"),
},
},
}

var buf bytes.Buffer
require.NoError(t, toml.NewEncoder(&buf).Encode(info))
out := buf.String()

// Top-level kebab-case keys and runtime field omissions.
require.Contains(t, out, `sink-uri = "blackhole://"`)
require.Contains(t, out, "start-ts")
require.NotContains(t, out, "gid") // GID is omitted from TOML (toml:"-")

// The [config] section must decode into the internal ReplicaConfig used by
// `changefeed create --config`.
var wrapper struct {
Config config.ReplicaConfig `toml:"config"`
}
_, err := toml.Decode(out, &wrapper)
require.NoError(t, err)
require.Equal(t, uint64(1024), util.GetOrZero(wrapper.Config.MemoryQuota))
require.True(t, util.GetOrZero(wrapper.Config.CaseSensitive))
require.True(t, util.GetOrZero(wrapper.Config.ForceReplicate))
require.NotNil(t, wrapper.Config.SyncPointInterval)
require.Equal(t, 10*time.Minute, *wrapper.Config.SyncPointInterval)
require.Equal(t, "correctness", util.GetOrZero(wrapper.Config.Integrity.IntegrityCheckLevel))
require.Equal(t, "eventual", util.GetOrZero(wrapper.Config.Consistent.Level))
}

// TestDefaultConfigTOMLRoundTripToInternal encodes the full default replica
// config to TOML and decodes it into the internal config.ReplicaConfig, then
// asserts that no config-section key is left undecoded. This proves every TOML
// tag added to the api/v2 config tree matches what `changefeed create --config`
// parses — a guard against tag drift across the ~200 tagged fields.
func TestDefaultConfigTOMLRoundTripToInternal(t *testing.T) {
t.Parallel()

info := &ChangeFeedInfo{ID: "cf", SinkURI: "blackhole://", Config: GetDefaultReplicaConfig()}
var buf bytes.Buffer
require.NoError(t, toml.NewEncoder(&buf).Encode(info))

var wrapper struct {
Config config.ReplicaConfig `toml:"config"`
}
meta, err := toml.Decode(buf.String(), &wrapper)
require.NoError(t, err)

// Top-level runtime fields (id, sink-uri, ...) are expected to be undecoded
// against this wrapper; only config.* keys must all map to the internal type.
var cfgUndecoded []string
for _, k := range meta.Undecoded() {
if len(k) > 0 && k[0] == "config" {
cfgUndecoded = append(cfgUndecoded, k.String())
}
}
require.Empty(t, cfgUndecoded, "config keys not decodable into internal config: %v", cfgUndecoded)
}

// TestRespondWithFormat verifies content negotiation: the default and explicit
// JSON requests yield JSON, while Accept: application/toml yields TOML.
func TestRespondWithFormat(t *testing.T) {
t.Parallel()

gin.SetMode(gin.TestMode)
obj := &ChangeFeedInfo{ID: "cf-1", SinkURI: "blackhole://"}
Comment on lines +166 to +169

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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


cases := []struct {
name string
accept string
wantCType string
wantContain string
}{
{"default json", "", "application/json", `"id":"cf-1"`},
{"explicit json", "application/json", "application/json", `"id":"cf-1"`},
{"toml", "application/toml", "application/toml", `id = "cf-1"`},
{"toml with charset", "application/toml; charset=utf-8", "application/toml", `id = "cf-1"`},
{"toml mixed case", "Application/TOML", "application/toml", `id = "cf-1"`},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
w := httptest.NewRecorder()
c, _ := gin.CreateTestContext(w)
c.Request = httptest.NewRequest("GET", "/", nil)
if tc.accept != "" {
c.Request.Header.Set("Accept", tc.accept)
}
respondWithFormat(c, 200, obj)
require.Equal(t, 200, w.Code)
require.Contains(t, w.Header().Get("Content-Type"), tc.wantCType)
require.Contains(t, w.Body.String(), tc.wantContain)
})
}
}
33 changes: 33 additions & 0 deletions api/v2/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@
package v2

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"

"github.com/BurntSushi/toml"
"github.com/gin-gonic/gin"
"github.com/pingcap/log"
"github.com/pingcap/ticdc/pkg/config"
Expand Down Expand Up @@ -89,6 +92,36 @@ func isFromV1API(c *gin.Context) bool {
return c.GetHeader("from-ticdc-api-v1") == "true"
}

// mimeTOML is the media type used to request and return TOML payloads.
const mimeTOML = "application/toml"

// wantsTOML reports whether the client requested a TOML response via the
// Accept header. A missing header, or anything other than application/toml,
// keeps the default JSON behavior. A simple substring match is sufficient
// while only JSON and TOML are supported; it tolerates parameters such as
// "application/toml; charset=utf-8". Media types are case-insensitive
// (RFC 7231 §3.1.1.1), so the header is lowercased before matching.
func wantsTOML(c *gin.Context) bool {
return strings.Contains(strings.ToLower(c.GetHeader("Accept")), mimeTOML)
}
Comment thread
JesseZhuang marked this conversation as resolved.

// respondWithFormat writes obj to the response, negotiating the encoding from
// the Accept header: application/toml yields a TOML body, anything else yields
// JSON (the unchanged default). It is the shared seam for content negotiation
// across v2 endpoints.
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))
return
}
c.Data(code, mimeTOML+"; charset=utf-8", buf.Bytes())
return
}
c.JSON(code, obj)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func getStatus(c *gin.Context) int {
if isFromV1API(c) {
return http.StatusAccepted
Expand Down
Loading
Loading