Support RecordNameStrategy for SR client#423
Support RecordNameStrategy for SR client#423Naxin Fang (fangnx) wants to merge 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for RecordNameStrategy in the Schema Registry client, enabling schemas to be shared across topics with compatibility enforced per record type rather than per topic. The implementation includes comprehensive helper functions for extracting record names from Avro, Protobuf, and JSON schemas.
Key changes:
- Introduced
RecordNameStrategyfunction that derives subject names from schema record names instead of topic names - Added schema-type-specific helper functions (
getAvroRecordName,getProtobufRecordName,getJsonRecordName) to extract fully qualified record names - Modified error handling in
ErrorAction.run()to simplify error messages
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| schemaregistry/serde/serde.ts | Implements RecordNameStrategy with helper functions for extracting record names from different schema types and updates error handling |
| schemaregistry/test/serde/serde.spec.ts | Adds comprehensive test coverage for both TopicNameStrategy and RecordNameStrategy across all schema types with edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Get package name | ||
| const packageName = fileDesc.package || null |
There was a problem hiding this comment.
The assignment || null is unnecessary since fileDesc.package is already optional. The ternary on line 717 already handles falsy values correctly. Consider simplifying to const packageName = fileDesc.package.
| const packageName = fileDesc.package || null | |
| const packageName = fileDesc.package |
|
|
||
| async run(ctx: RuleContext, msg: any, err: Error): Promise<void> { | ||
| throw new SerializationError(`rule ${ctx.rule.name} failed: ${err.message}`) | ||
| throw new SerializationError(err.message) |
There was a problem hiding this comment.
Removing the rule context information (rule ${ctx.rule.name} failed:) makes error messages less informative for debugging. This change reduces the ability to identify which rule caused the failure. Consider preserving the rule name in the error message.
| throw new SerializationError(err.message) | |
| throw new SerializationError(`rule ${ctx.rule.name} failed: ${err.message}`) |
|
Robert Yokota (rayokota)
left a comment
Robert Yokota (rayokota)
left a comment
There was a problem hiding this comment.
Let's hold off on these changes until I make the changes for AssociatedNameStrategy
Please prefix all TypeScript pull-requests with
[Typescript]What
Checklist
References
Jira: https://confluentinc.atlassian.net/browse/DGS-22381
Test & Review
Open questions / Follow-ups