[SchemaRegistry] fix avro type leaks in serializer #21004
Conversation
ghost
added
the
|
/azp run python - schemaregistry - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if self._auto_register_schemas | ||
| else self._schema_registry_client.get_schema_id | ||
| ) | ||
| self._user_input_schema_cache = {} |
There was a problem hiding this comment.
cache topic: do we need this cache since we now use lru_cache?
as OOB discussion: this is to store schema -> normalized schema pair.
I think we could either remove this cache first or wrap the logic with a lru_cache decorated method.
...maregistry-avroserializer/azure/schemaregistry/serializer/avroserializer/_avro_serializer.py
Show resolved
Hide resolved
| :ivar message: The error message. | ||
| :vartype message: str | ||
| :ivar error: The error condition, if available. | ||
| :vartype error: str |
There was a problem hiding this comment.
why "error" ivar here? since the AzureError already has inner_exception ivar
| def __init__(self, message, **kwargs): | ||
| self.message = message | ||
| self.error = kwargs.get("error") | ||
| super(SchemaParseException, self).__init__(self.message, **kwargs) |
There was a problem hiding this comment.
I think we could remove the __init__ method completely if there's no reason to have error ivar different than the inner_exception
| # -------------------------------------------------------------------------- | ||
| from azure.core.exceptions import AzureError | ||
|
|
||
| class SchemaParseException(AzureError): |
There was a problem hiding this comment.
The name sounds very generic.
Do we want this error to be library specific or a general one that could be shared among different serializer libraries in the future -- which means it's to be placed under schema registry client library like azure.schemaregistry.serializer.exceptions.SchemaParseException.
I'm leaning towards to having a common shared error, less types
| cached_schema.type | ||
| ) | ||
| ) | ||
| data_bytes = self._avro_serializer.serialize(value, cached_schema) |
There was a problem hiding this comment.
to be safe here, we could abstract the underlying avro serializer library
- parse schema
- _fast_avro_serializer.parse_schema, normal_avro_serializer.parse_schema
- try:
_fast_avro_serializer.parse_schema()
exception Exception:
raise SchemaParseException()
- serialize
try:
_fast_avro_serializer.serialize()
except Exception:
SerializationException() - deserialize
DeserializationException
There was a problem hiding this comment.
we could make a matrix for comparison:
fastavro avro
missing property: raising Keyerror raise SchemaParseException
wrong format: ...
but I'm afraid it would take much time to traverse all the cases (especially comers cases which are difficult to think about, and may not be worth the efforts)
2 participants
fixes: #20818