fix: use --normalize flag value when registering schemas#3332
fix: use --normalize flag value when registering schemas#3332David Novicki (david-novicki) merged 5 commits intoconfluentinc:mainfrom
Conversation
RegisterSchemaWithAuth hardcoded `true` for the normalize parameter since confluentinc#2449 (schema-registry-sdk-go upgrade), causing every schema registration to send normalize=true regardless of the --normalize flag. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
Fixes schema registration so the Schema Registry normalize query parameter follows the CLI’s --normalize flag (instead of always forcing normalization on), aligning behavior with the documented default.
Changes:
- Forward
RegisterSchemaConfigs.NormalizefromRegisterSchemaWithAuthintoclient.Register(...)to honor the caller’s normalization choice.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| response, err := client.Register(schemaCfg.Subject, request, true) | ||
| response, err := client.Register(schemaCfg.Subject, request, schemaCfg.Normalize) |
There was a problem hiding this comment.
RegisterSchemaWithAuth now forwards schemaCfg.Normalize. Any existing callers that do not explicitly set RegisterSchemaConfigs.Normalize will now register schemas with normalization disabled (Go zero value), which is a behavior change from the previous hard-coded true. Please audit/update call sites to set Normalize intentionally (even if defaulting to false) so behavior doesn’t depend on an omitted field.
There was a problem hiding this comment.
Good catch — addressed in fabd0a9: both command_topic_produce.go call sites now set Normalize: false explicitly instead of relying on the struct zero value. The only other caller (command_schema_create.go) was already passing the flag value explicitly.
| } | ||
|
|
||
| response, err := client.Register(schemaCfg.Subject, request, true) | ||
| response, err := client.Register(schemaCfg.Subject, request, schemaCfg.Normalize) |
There was a problem hiding this comment.
There are existing integration tests for schema-registry schema create, but none appear to assert that the normalize query parameter matches the --normalize flag. Adding an integration/unit test that fails when the flag value isn’t forwarded would prevent regressions of this bug.
There was a problem hiding this comment.
Agreed — added in 443d924: pkg/schemaregistry/schema_test.go / TestRegisterSchemaWithAuth_ForwardsNormalizeFlag stands up an httptest Schema Registry and asserts the outbound normalize query parameter matches RegisterSchemaConfigs.Normalize for both true and false. Verified it fails on the pre-fix hard-coded true.
…WithAuth Covers the regression fixed in 8578b04: a hard-coded `true` in RegisterSchemaWithAuth caused the `normalize` query parameter to always be `true` regardless of `RegisterSchemaConfigs.Normalize`. The test stands up an httptest Schema Registry and asserts the outbound `normalize` query parameter matches the struct value for both `true` and `false`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per PR review: now that RegisterSchemaWithAuth forwards schemaCfg.Normalize instead of hard-coding true, callers that omit the field silently get the Go zero value. Both topic-produce call sites (cloud and on-prem) have no --normalize flag surface, so make their intent explicit with Normalize: false rather than relying on struct zero values. No user-visible behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Matches the existing --normalize flag on `confluent schema-registry schema create`. Previously, schema registration during `kafka topic produce` had no user-controllable path for schema normalization — it was implicitly forced on by the bug fixed earlier in this PR, and after that fix was implicitly off. The flag now lets users opt in to normalization in both the Cloud and on-prem produce flows (default: false, matching `schema create`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `TestProduceCommand_NormalizeFlag` that constructs the produce command via newProduceCommand and asserts the --normalize flag is registered with the expected name, default (false), description, and round-trips correctly via cmd.Flags().GetBool. Verified the test fails when the flag declaration is removed. Also updates the produce-help and produce-help-onprem golden fixtures consumed by the TestHelp integration suite so the auto-generated help output matches after adding the new flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0131aad
into
confluentinc:main
Drive-by: srsdk was in the "default" group alongside testify; gci wants it in its own "github.com/confluentinc/" group per .golangci.yml. Pre-existing from #3332; unblocks CI here.
Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
Applies to both Confluent Cloud and Confluent Platform.
Bug fix
pkg/schemaregistry.RegisterSchemaWithAuthwas passing a hard-codedtrueas thenormalizeargument toclient.Register, so every schema registration went through the Schema Registry normalization path regardless of what the user set for--normalize. Callers populateRegisterSchemaConfigs.Normalizefrom the flag (or zero value), but that value was being dropped at the call site.The
Normalizefield is also now set explicitly at theinternal/kafka/command_topic_produce.gocall sites per review feedback, so no caller silently relies on the struct zero value.New flag:
confluent kafka topic produce --normalizeTo keep
kafka topic producesymmetrical withschema-registry schema create, a--normalizeflag is now available on the produce command. Before this PR, the produce command had no user-facing way to control normalization — registration during produce was implicitly forced on by the bug above, and would have been implicitly off after the bug fix. Exposing the flag directly is both clearer and preserves user control. Flag default isfalse, matchingschema create.Default behavior after this fix
The
--normalizeflag is declared with a default offalseon both commands, so after this PR:confluent schema-registry schema createwithout--normalizesendsnormalize=false— schemas are registered without normalization by default.confluent schema-registry schema create --normalizesendsnormalize=true.confluent kafka topic producewithout--normalizelikewise sendsnormalize=false.confluent kafka topic produce --normalizesendsnormalize=true.This is a visible change from the previous (buggy) behavior, which always sent
normalize=trueregardless of the flag (onschema create) or ignored normalization entirely from user control (ontopic produce).Blast Radius
confluent schema-registry schema create(Cloud and on-prem) will now get the un-normalized registration behavior by default, matching the flag's documented default offalse.--normalizewill see a behavior change. They can restore the prior behavior by passing--normalize.confluent kafka topic produceschema-registration paths are likewise affected: they now sendnormalize=falseby default instead of the previous forcedtrue. Users who want normalization on produce can now opt in via the new--normalizeflag.References
Test & Review
Unit tests added (two):
pkg/schemaregistry/schema_test.go—TestRegisterSchemaWithAuth_ForwardsNormalizeFlag. Stands up anhttptestSchema Registry and asserts that the outboundnormalizequery parameter matchesRegisterSchemaConfigs.Normalizefor bothtrueandfalse. Verified this test fails on the pre-fix code (hard-codedtrue) and passes on the fixed code — a genuine regression test for the bug.internal/kafka/command_topic_produce_test.go—TestProduceCommand_NormalizeFlag. Constructs the produce command vianewProduceCommandand asserts the--normalizeflag is registered with the expected name, default (false), description, and round-trips correctly viacmd.Flags().GetBool. Verified this test fails when the flag declaration is removed from the command — also a genuine regression test.Help-output golden fixtures updated:
test/fixtures/output/kafka/topic/produce-help.goldentest/fixtures/output/kafka/topic/produce-help-onprem.goldenThese are consumed by
test/help_test.go'sTestHelp, which auto-walks the command tree and diffs--helpoutput against the goldens. The new--normalizeline is inserted right after--schema-id-headerto match cobra's output order.Other verification:
go build -o confluent ./cmd/confluent— build clean. Confirmed./confluent schema-registry schema create --helpand./confluent kafka topic produce --helpboth render the--normalizeflag correctly.go test -timeout 0 -count=1 $(go list ./... | grep -v .../test)). All packages touched by this PR (pkg/schemaregistry,internal/kafka,internal/schema-registry) pass, including both new tests. The only failure is a pre-existing, unrelated flake inpkg/flink/internal/controller(details below).go veton the changed packages (pkg/schemaregistry/...,internal/kafka/...,cmd/confluent/...) — no issues. (Did not run the fullgolangci-lintsuite locally; that will run in CI.)Known pre-existing CI failure (unrelated to this PR)
Semaphore CI (
ci/semaphoreci/pr: Confluent CLI) is reporting a failure on this PR. I reproduced the failure locally by running the full unit-test target ($(go list ./... | grep -v .../test)) and traced it to a single pre-existing, unrelated test:pkg/flink/internal/controller.TestInteractiveOutputControllerTestSuite/TestCloseTableViewOnUserInputpanic: close of nil channeloriginating ingithub.com/gdamore/tcell/v2/tscreen.go:586→tview.(*Application).Stopwhen the test goroutine sends anEscape/CtrlQkey event. Looks like a Go 1.26 / tcell 2.7.4 / tview interaction when no real TTY is attached.Evidence it is pre-existing and not caused by this PR:
origin/main— I checked out themainversion ofpkg/flink/internal/controller/and re-ran the test in isolation; same panic, same stack, same line.tcell/tviewcode — onlypkg/schemaregistry/schema.go,pkg/schemaregistry/schema_test.go,internal/kafka/command_topic_produce.go,internal/kafka/command_topic_produce_test.go, and the twokafka/topic/produce-help*.goldenfixtures.mainshow the same CI failure — e.g. PR Update github.com/confluentinc/confluent-kafka-go/v2 to v2.14.1 and add back #3325 #3327 was merged withci/semaphoreci/pr: FAILURE, so the team has been merging past this flake.pkg/schemaregistry,internal/kafka,internal/schema-registry) pass unit tests locally.Proposed path: merge this PR through the same route other recent merges took (the flake is not a regression from this change), and track the flink controller test separately. Happy to file that ticket if one doesn't already exist.