Set avro/json schema on use.latest.version=True #1704
Conversation
|
|
There was a problem hiding this comment.
Thanks for submitting the PR.
Please update the PR with the following:
- Rebase the PR
- Test cases for the fix
- CHANGELOG entry
Done review of the code part of the PR. I am going to test these changes once you have updated the PR with the requested changes and provide more comments if required.
I am checking the Protobuf part.
| raise TypeError('You must pass either schema string or schema object') | ||
|
|
||
| self._to_dict = to_dict |
There was a problem hiding this comment.
Is this necessary? The validation of to_dict happens at line 195. This change should be reverted.
There was a problem hiding this comment.
I will move the two validation IFs to the start of the method. We still need self._to_dict = to_dict because it is an instance variable required on other class methods
| if isinstance(schema_str, str): | ||
| self._schema = _schema_loads(schema_str) | ||
| else: # type=Schema (asserted on __init__) | ||
| self._schema = schema_str |
There was a problem hiding this comment.
This block is only applicable for __init__ function. We always get object of Schema type from the __call__ function. I think its better to keep this as it was earlier and assume that only Schema type object is received here.
There was a problem hiding this comment.
Correct, but to avoid calling that twice I moved that to _set_instance_variables so either be called on either __init__ or __call__
| # on __call__ if use.latest.version is set to True | ||
| self._set_instance_variables(schema_str) | ||
|
|
||
| def _set_instance_variables(self, schema_str, schema_id=None): |
There was a problem hiding this comment.
- use
schemainstead ofschema_strhere. - Name of the function should be more specific. Something like
update_schema_info
| Function to be called upon __init__ and when serializing a message (__call__) | ||
| if use.latest.version is set True |
There was a problem hiding this comment.
Add explanation of what this function does as well. This can be written after the description.
There was a problem hiding this comment.
These changes LGTM.
In the future, if we don't find a way to "compile" the string representation of the Protobuf schema, we can enhance SR to return the Protobuf schema in serialized form. I'll be looking at this when I add CSFLE support for the Python client. So let's not remove the use.latest.version config for Protobuf.
schema str/Object check upon __init__
|
@rayokota - You need to sign the CLA for the PR to get merged. |
|
Subsumed by #1852 |
The current
AvroSerializer(src/confluent_kafka/schema_registry/avro.py) andJSONSerializer(src/confluent_kafka/schema_registry/json_schema.py) classes do not override the schema whenuse.latest.schemais set toTrue, it currently only returns theschema_id. This PR is to address that.Please notice the same behaviour happens with
ProtobufSerializer(src/confluent_kafka/schema_registry/protobuf.py), however not sure howuse.latest.schemacould work with Protobuf schemas as it needs to be "compiled" and the SR stores the "uncompiled" version of it. Maybe we should drop that configuration parameter out?