Skip to content

fix: Handled ctx=None for AvroDeserializer and ProtobufDeserializer #1974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 22, 2025

Conversation

and-ratajski
Copy link
Contributor

Handled None value for optional parameter called ctx in both AvroDeserializer and ProtobufDeserializer.

closes #1939
closes #1973


The "optional" ctx parameter in __call__ method in AvroDeserializer and ProtobufDeserializer isn't optional at all... Despite method annotation that specifies type of this parameter as ctx: Optional[SerializationContext] = None is was required in the method logic and None value wasn't handled. This misbehavior (compered to previous releases) was already noticed in the issues mentioned above.

This PR contains fix that handles None value and hence gives backward compatibility.

Checklist

  • Contains customer facing changes? Including API/behavior changes
  • Did you add sufficient unit test and/or integration test coverage for this PR?

References

GH:

Open questions / Follow-ups

If this behavior is intended for some reason I would suggest:

  • marking it as breaking change (e.g. version 3.0.0)
  • changing type annotation for ctx parameter

@Copilot Copilot AI review requested due to automatic review settings April 18, 2025 13:12
@and-ratajski and-ratajski requested review from a team as code owners April 18, 2025 13:12
@confluent-cla-assistant
Copy link

confluent-cla-assistant bot commented Apr 18, 2025

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

Copy link

@Copilot 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 the handling of a None value for the optional ctx parameter in both AvroDeserializer and ProtobufDeserializer, ensuring backward compatibility with previous releases.

  • Updated the construction of the subject variable by adding a conditional check for None.
  • Adjusted the dependency resolution to only execute when ctx is provided.
  • Revised the CHANGELOG to document the fixes.

Reviewed Changes

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

File Description
src/confluent_kafka/schema_registry/protobuf.py Updated subject resolution with a conditional check for None.
src/confluent_kafka/schema_registry/avro.py Updated subject resolution and dependency resolution with a conditional check for None.
CHANGELOG.md Documented the fixes regarding the ctx parameter.

@and-ratajski
Copy link
Contributor Author

In addition, I tested in my application that uses AvroDeserializer - works

@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 @and-ratajski , LGTM

@rayokota rayokota merged commit c1ad3d4 into confluentinc:master Apr 22, 2025
1 of 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.

Handling of null ctx in AvroDeserializer Handling of null ctx in ProtobufDeserializer
2 participants