BE: Restore SchemaRegistry auto-selection for Avro topics (#1833)#1842
BE: Restore SchemaRegistry auto-selection for Avro topics (#1833)#1842joshuaNathaniel wants to merge 1 commit into
Conversation
kapybro
Bot
added
status/triage
📝 WalkthroughWalkthroughClusterSerdes selection was split into three ordered passes (topic patterns → cluster defaults → auto-detection), Serde.couldBePreferable now defaults to false (opt-in), and SchemaRegistrySerde recognizes default and topic-record-name subjects when deciding preferability. ChangesSerde Auto-Selection and Preference Detection
Sequence DiagramsequenceDiagram
participant Caller
participant ClusterSerdes
participant SerdeInstances
participant ClusterConfig
Caller->>ClusterSerdes: findSerdeByPatternsOrDefault(topic, target, additionalCheck)
ClusterSerdes->>SerdeInstances: Pass 1 - topic regex matches (registration order)
ClusterSerdes->>ClusterConfig: Pass 2 - check configured defaultKeySerde/defaultValueSerde
ClusterSerdes->>SerdeInstances: Pass 3 - couldBePreferable + additionalCheck
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/main/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerde.java`:
- Around line 406-409: The current prefix match in SchemaRegistrySerde
(schemaSubject / getSchemaSubjects usage) treats subjects like
"orders-dlq-value" as matching "orders" because it only checks startsWith(topic
+ "-"); change the predicate so that a subject only matches the topic prefix if
it is the defaultSubject OR it startsWith(topic + "-") AND does NOT end with
"-key" or "-value" (i.e., exclude subjects suffixed with "-key"/"-value" from
counting as a topic-prefix match for TopicRecordNameStrategy); update the stream
predicate in SchemaRegistrySerde and add a regression test asserting that
"orders" does not match "orders-dlq-value" while still accepting legitimate
topic subjects.
In `@api/src/main/resources/static/openapi/kafbat-ui-api.yaml`:
- Around line 3328-3331: The openAPI schema for the property "maxRetries"
currently allows negative integers; add a floor by adding "minimum: 0" to the
"maxRetries" schema so generated clients and validators reject negative retry
counts (keep the existing "type: integer", "format: int32", and "default: 1").
Locate the "maxRetries" property in the OpenAPI YAML and insert the minimum
constraint directly under its schema block.
- Around line 3316-3317: ApplicationConfig currently exposes sensitive fields in
the response schema; mark the OAuth clientSecret as writeOnly so it is not
serialized in GET /api/config responses by adding writeOnly: true to the
clientSecret property in the OpenAPI schema, and do the same for the sibling
password property (both referenced as clientSecret and password in the
ApplicationConfig schema) to ensure neither is returned to callers while still
allowing them to be provided on input.
🪄 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: 3c2c084f-48a2-44ed-94fa-c091dfed304a
📒 Files selected for processing (7)
api/src/main/java/io/kafbat/ui/serdes/ClusterSerdes.javaapi/src/main/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerde.javaapi/src/main/resources/static/openapi/kafbat-ui-api.yamlapi/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.javaapi/src/test/java/io/kafbat/ui/serdes/SerdesInitializerTest.javaapi/src/test/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerdeTest.javaserde-api/src/main/java/io/kafbat/ui/serde/api/Serde.java
joshuaNathaniel
force-pushed
the
issues/1833
branch
from
b030e07 to
9020193
Compare
|
@joshuaNathaniel Thank you for working on the issue! In version 1.4.2, the SchemaRegistry value serde was automatically selected even for topics using the RecordNameStrategy where the schema must be inferred from the messages themselves and not only from the topic name. Will this also be the case in version 1.5.0 with this fix? |
This is a good callout @IversenMichael. Unfortunately, no, it does not. This is why I am looking for feedback on it. I believe the prior behavior was to always default topics using key and value serde to the schema registry for every topic. I think the intention was to fix the lying dropdown before and we used couldBePreferable to only nominate SR when there were actually linked subjects. But I think there is a bit of a performance concern mixed in here for RecordNameStrategy. Yeah, I made a comment in that issue and I deleted it. I'm starting to regret that. I think @germanosin has to weigh in on this. We can certainly restore the behavior but how we do it has trade offs. |
joshuaNathaniel
force-pushed
the
issues/1833
branch
from
9020193 to
ca2e62a
Compare
|
Popping back in here. It's been a few days. I just need a wee bit of direction on where we want to go in regards to my last comment. I'm more than happy to do the work and make everyone happy and whole again. :D |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.java (1)
46-49: 💤 Low valueConsider adding a comment to explain the empty method.
The empty
configureimplementation is intentional for this test stub, but SonarCloud flags it as unclear. A brief comment would document the intent.📝 Suggested clarification
`@Override` public void configure(PropertyResolver serdeProperties, PropertyResolver kafkaClusterProperties, PropertyResolver globalProperties) { + // No configuration needed for test stub }🤖 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/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.java` around lines 46 - 49, The empty configure(PropertyResolver serdeProperties, PropertyResolver kafkaClusterProperties, PropertyResolver globalProperties) method in ClusterSerdesTest is intentional but unclear to static analysis; add a concise inline comment inside the method body explaining that this no-op is deliberate for the test stub (e.g., “no-op on purpose for test stub”) so reviewers and SonarCloud understand the intent, referencing the configure method in ClusterSerdesTest.
🤖 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.
Nitpick comments:
In `@api/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.java`:
- Around line 46-49: The empty configure(PropertyResolver serdeProperties,
PropertyResolver kafkaClusterProperties, PropertyResolver globalProperties)
method in ClusterSerdesTest is intentional but unclear to static analysis; add a
concise inline comment inside the method body explaining that this no-op is
deliberate for the test stub (e.g., “no-op on purpose for test stub”) so
reviewers and SonarCloud understand the intent, referencing the configure method
in ClusterSerdesTest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59a855bd-2b10-431c-9ec4-d4927783b751
📒 Files selected for processing (6)
api/src/main/java/io/kafbat/ui/serdes/ClusterSerdes.javaapi/src/main/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerde.javaapi/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.javaapi/src/test/java/io/kafbat/ui/serdes/SerdesInitializerTest.javaapi/src/test/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerdeTest.javaserde-api/src/main/java/io/kafbat/ui/serde/api/Serde.java
✅ Files skipped from review due to trivial changes (1)
- api/src/test/java/io/kafbat/ui/serdes/SerdesInitializerTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/main/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerde.java
- api/src/test/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerdeTest.java
- api/src/main/java/io/kafbat/ui/serdes/ClusterSerdes.java
joshuaNathaniel
force-pushed
the
issues/1833
branch
from
ca2e62a to
f31321f
Compare
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/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.java`:
- Around line 46-49: The override of configure(...) in StubSerde should include
an explicit no-op note to satisfy static analysis; update the
StubSerde.configure(PropertyResolver serdeProperties, PropertyResolver
kafkaClusterProperties, PropertyResolver globalProperties) method to keep the
empty implementation but add a clear comment stating it is intentionally a no-op
(e.g., "no-op for test stub" or "intentionally left blank") so Sonar/CI
recognizes it as deliberate and not an accidental omission.
🪄 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: d5e2e5f7-e1ab-406f-803a-eafe72c937d6
📒 Files selected for processing (6)
api/src/main/java/io/kafbat/ui/serdes/ClusterSerdes.javaapi/src/main/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerde.javaapi/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.javaapi/src/test/java/io/kafbat/ui/serdes/SerdesInitializerTest.javaapi/src/test/java/io/kafbat/ui/serdes/builtin/sr/SchemaRegistrySerdeTest.javaserde-api/src/main/java/io/kafbat/ui/serde/api/Serde.java
| public void configure(PropertyResolver serdeProperties, | ||
| PropertyResolver kafkaClusterProperties, | ||
| PropertyResolver globalProperties) { | ||
| } |
There was a problem hiding this comment.
Add an explicit no-op note in StubSerde.configure.
Line 46 overrides configure(...) with an empty body; this is valid for a test stub, but it currently triggers Sonar and can fail CI quality gates.
Suggested patch
`@Override`
public void configure(PropertyResolver serdeProperties,
PropertyResolver kafkaClusterProperties,
PropertyResolver globalProperties) {
+ // no-op: test stub intentionally does not require configuration
}📝 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.
| public void configure(PropertyResolver serdeProperties, | |
| PropertyResolver kafkaClusterProperties, | |
| PropertyResolver globalProperties) { | |
| } | |
| public void configure(PropertyResolver serdeProperties, | |
| PropertyResolver kafkaClusterProperties, | |
| PropertyResolver globalProperties) { | |
| // no-op: test stub intentionally does not require configuration | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 46-46: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.
🤖 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/src/test/java/io/kafbat/ui/serdes/ClusterSerdesTest.java` around lines 46
- 49, The override of configure(...) in StubSerde should include an explicit
no-op note to satisfy static analysis; update the
StubSerde.configure(PropertyResolver serdeProperties, PropertyResolver
kafkaClusterProperties, PropertyResolver globalProperties) method to keep the
empty implementation but add a clear comment stating it is intentionally a no-op
(e.g., "no-op for test stub" or "intentionally left blank") so Sonar/CI
recognizes it as deliberate and not an accidental omission.
3 participants
Fixes #1833
What changes did you make? (Give an overview)
In 1.5.0 the value serde stopped auto-selecting SchemaRegistry for Avro topics and defaulted to String. This brings back the 1.4.2 behavior.
The fix is in three places:
ClusterSerdes.findSerdeByPatternsOrDefaultnow runs three passes: explicittopicKeysPattern/topicValuesPatternfirst, thendefaultKeySerde/defaultValueSerde, then any serde that says it should be preferred viacouldBePreferableand passescanSerialize/canDeserialize. PreviouslycouldBePreferablewas wired as acontinueinside the pattern loop, so it never actually selected anything (auto-configured SR has a null pattern), and as a side effect it could silently skip explicit user patterns too.SchemaRegistrySerde.couldBePreferablenow matches both TopicNameStrategy and TopicRecordNameStrategy. RecordNameStrategy is left out because subjects there are bare FQNs with no relationship to the topic, so we can't safely guess which topics use them. SR still shows up in the dropdown for those topics (viacanSerialize), it's just not the default.Default for
Serde#couldBePreferableflipped fromtruetofalse. With pass 3 in place, leaving it attruewould have madeStringSerdewin for everything (it's registered first andcanDeserializeis unconditionally true). The opt-in default also matches whatSchemaRegistrySerdewas already doing as the only override.Tests:
ClusterSerdesTest: there wasn't any coverage on the selection logic, which is why this regression slipped through. Pins the three-pass behavior and the original bug.SchemaRegistrySerdeTest.CouldBePreferableTests: covers each naming strategy plus a few edge cases (opposite target, mixed registry state, subjects belonging to other topics).SerdesInitializerTest: assertion thatdefaultValueSerdestays null when not configured, so nobody tries to "fix" things by re-adding the old implicit fallback.Is there anything you'd like reviewers to focus on?
findSerdeByPatternsOrDefault. Want to make sure user config still wins over implicit detection.couldBePreferablenarrower thancanSerialize. Comment explains it but worth a second pair of eyes.couldBePreferableinterface default flip. It's inserde-apiso technically a contract change, but in 1.5.0 the default was effectively unused (the only consumer was the deadcontinueinside pass 1), so anyone upgrading without overriding the method should see no behavioral difference.How Has This Been Tested?
SchemaRegistry; RecordNameStrategy defaults toString.Checklist
A picture of a cute animal (not mandatory but encouraged)
Summary by CodeRabbit
Bug Fixes
Tests