Skip to content

Conversation

@jekel
Copy link
Contributor

@jekel jekel commented Aug 13, 2025

What

Schema Registry server on new schema registration with specifiied subject could return no subject name in response,
like Confluent Schema Registry, so, register_schema_full_response method does not set it correctly, and returned registered schema is without subject.
This pull request fixs this issue.

Copilot AI review requested due to automatic review settings August 13, 2025 18:24
@jekel jekel requested review from a team and MSeal as code owners August 13, 2025 18:24
@confluent-cla-assistant
Copy link

confluent-cla-assistant bot commented Aug 13, 2025

🎉 All Contributor License Agreements have been signed. Ready to merge.
✅ jekel
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where the register_schema_full_response method doesn't correctly set the subject name on the registered schema when the Schema Registry server returns a response without a subject name (as allowed by the Confluent Schema Registry API).

  • Adds logic to set the subject name on the registered schema if it's missing from the server response
  • Ensures the returned registered schema object has the correct subject name populated

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/confluent_kafka/schema_registry/_sync/schema_registry_client.py Adds subject name fallback logic in synchronous schema registry client
src/confluent_kafka/schema_registry/_async/schema_registry_client.py Adds identical subject name fallback logic in asynchronous schema registry client

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

registered_schema = RegisteredSchema.from_dict(response)

if registered_schema.subject is None:
registered_schema.subject = subject_name
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

[nitpick] Direct mutation of the registered_schema object could lead to unexpected side effects. Consider creating a new RegisteredSchema instance with the correct subject or using a method that returns a properly initialized object.

Suggested change
registered_schema.subject = subject_name
registered_schema = RegisteredSchema(
registered_schema.schema_id,
registered_schema.schema,
registered_schema.guid,
subject_name,
registered_schema.version
)

Copilot uses AI. Check for mistakes.
registered_schema = RegisteredSchema.from_dict(response)

if registered_schema.subject is None:
registered_schema.subject = subject_name
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

[nitpick] Direct mutation of the registered_schema object could lead to unexpected side effects. Consider creating a new RegisteredSchema instance with the correct subject or using a method that returns a properly initialized object.

Suggested change
registered_schema.subject = subject_name
registered_schema = RegisteredSchema(
registered_schema.schema_id,
registered_schema.schema,
registered_schema.guid,
subject_name,
registered_schema.version
)

Copilot uses AI. Check for mistakes.
@jekel jekel force-pushed the fix-sr-subject-register-full-response branch from ec8d303 to c265afb Compare August 13, 2025 18:46
@rayokota
Copy link
Member

/sem-approve

@rayokota
Copy link
Member

/sem-approve

Copy link
Member

@rayokota rayokota left a comment

Choose a reason for hiding this comment

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

Thanks @jekel , LGTM

@rayokota rayokota merged commit c3cec41 into confluentinc:master Aug 13, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants