Handle empty context properly for Avro deserializer #2035
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
| except AttributeError as e: | ||
| if "'NoneType' object has no attribute 'headers'" in str(e): | ||
| pytest.fail("Should not raise AttributeError for None context") | ||
| except Exception: |
There was a problem hiding this comment.
Don't put generic Exception: pass into code. It'll mask ugly errors and failure modes.
There was a problem hiding this comment.
Use https://docs.pytest.org/en/stable/reference/reference.html#pytest.raises instead for this test pattern and assert to correct exception positively rather than wrong exception negatively
There was a problem hiding this comment.
Thanks for the review! I've refactored the tests
| except AttributeError as e: | ||
| if "'NoneType' object has no attribute 'headers'" in str(e): | ||
| pytest.fail("Should not raise AttributeError for None context") | ||
| except Exception: |
| Returns: | ||
| bytes: The payload | ||
| """ | ||
| if ctx is None: |
There was a problem hiding this comment.
You should instead do a header assignment conditional on ctx so that the serialization choice is made once and keep the logic DRY.
This comment has been minimized.
This comment has been minimized.
| query = {} | ||
| if fmt is not None: | ||
| query = {'format': fmt} | ||
| query['format'] = fmt |
There was a problem hiding this comment.
Found this bug when I was going through the code paths
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug where Avro deserializers with ctx=None would throw 'NoneType' object has no attribute 'headers' errors by implementing proper null context handling.
- Added null context validation in
header_schema_id_serializerto throw clear error messages - Modified
dual_schema_id_deserializerto gracefully handle None context by skipping header checks - Fixed an unrelated bug in schema registry clients where undefined
queryvariable could cause errors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/schema_registry/test_schema_id.py | Added unit tests to verify null context handling in both serializer and deserializer functions |
| src/confluent_kafka/schema_registry/init.py | Updated type hints and implemented null context handling logic for serializer/deserializer functions |
| src/confluent_kafka/schema_registry/_sync/schema_registry_client.py | Fixed undefined variable bug by initializing query dictionary before conditional assignment |
| src/confluent_kafka/schema_registry/_async/schema_registry_client.py | Fixed undefined variable bug by initializing query dictionary before conditional assignment |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ServerConfig | ||
| ) | ||
| from ..serialization import SerializationError, MessageField | ||
| from ..serialization import SerializationError, MessageField, SerializationContext |
There was a problem hiding this comment.
The import of SerializationContext is added but Optional is used in type hints without importing it from typing module. Consider adding 'from typing import Optional' to ensure proper type annotation support.
There was a problem hiding this comment.
from typing import Optional already exists in line 19
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Analysis Details1 Issue
Coverage and Duplications
Project ID: confluent-kafka-python |
What
Problem: Deserializers with
ctx=Nonethrew'NoneType' object has no attribute 'headers'Fix:
dual_schema_id_deserializer: HandleNonecontext gracefullyheader_schema_id_serializer: Throw clearSerializationErrorfor required contextChecklist
References
JIRA: https://confluentinc.atlassian.net/browse/DGS-22029
Issue: #2034
Test & Review
Open questions / Follow-ups