-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[SchemaRegistry] fix avro type leaks in serializer #21004
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,15 @@ | |
| from io import BytesIO | ||
| from typing import Any, Dict, Mapping | ||
| import avro | ||
|
|
||
| from avro.schema import ( | ||
| SchemaParseException, | ||
| AvroException, | ||
| InvalidName, | ||
| PRIMITIVE_TYPES, | ||
| ) | ||
| from avro.io import AvroTypeException | ||
|
|
||
| from .exceptions import SchemaParseException as AzureSchemaParseException | ||
| from ._constants import SCHEMA_ID_START_INDEX, SCHEMA_ID_LENGTH, DATA_START_INDEX | ||
| from ._avro_serializer import AvroObjectSerializer | ||
|
|
||
|
|
@@ -53,16 +61,18 @@ def __init__(self, **kwargs): | |
| # type: (Any) -> None | ||
| try: | ||
| self._schema_group = kwargs.pop("group_name") | ||
| self._schema_registry_client = kwargs.pop("client") # type: "SchemaRegistryClient" | ||
| self._schema_registry_client = kwargs.pop( | ||
| "client" | ||
| ) # type: "SchemaRegistryClient" | ||
| except KeyError as e: | ||
| raise TypeError("'{}' is a required keyword.".format(e.args[0])) | ||
| self._avro_serializer = AvroObjectSerializer(codec=kwargs.get("codec")) | ||
| self._auto_register_schemas = kwargs.get("auto_register_schemas", False) | ||
| self._auto_register_schema_func = ( | ||
| self._schema_registry_client.register_schema | ||
| if self._auto_register_schemas | ||
| else self._schema_registry_client.get_schema_id | ||
| ) | ||
| self._schema_registry_client.register_schema | ||
| if self._auto_register_schemas | ||
| else self._schema_registry_client.get_schema_id | ||
| ) | ||
| self._user_input_schema_cache = {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cache topic: do we need this cache since we now use lru_cache? |
||
|
|
||
| def __enter__(self): | ||
|
|
@@ -121,10 +131,12 @@ def serialize(self, value, **kwargs): | |
| Encode data with the given schema. The returns bytes are consisted of: The first 4 bytes | ||
| denoting record format identifier. The following 32 bytes denoting schema id returned by schema registry | ||
| service. The remaining bytes are the real data payload. | ||
| Schema must be a Avro RecordSchema: | ||
| https://avro.apache.org/docs/1.10.0/gettingstartedpython.html#Defining+a+schema | ||
|
|
||
| :param value: The data to be encoded. | ||
| :type value: Mapping[str, Any] | ||
| :keyword schema: Required. The schema used to encode the data. | ||
| :keyword schema: Required. The Avro RecordSchema used to encode the data. | ||
| :paramtype schema: str | ||
| :rtype: bytes | ||
| """ | ||
|
|
@@ -135,12 +147,37 @@ def serialize(self, value, **kwargs): | |
| try: | ||
| cached_schema = self._user_input_schema_cache[raw_input_schema] | ||
| except KeyError: | ||
| parsed_schema = avro.schema.parse(raw_input_schema) | ||
| try: | ||
| parsed_schema = avro.schema.parse(raw_input_schema) | ||
| except ( | ||
| SchemaParseException, | ||
| AvroTypeException, | ||
| AvroException, | ||
| InvalidName, | ||
| ) as e: | ||
| raise AzureSchemaParseException(e) | ||
| self._user_input_schema_cache[raw_input_schema] = parsed_schema | ||
| cached_schema = parsed_schema | ||
|
|
||
| # TODO: check if there's a more pythonic way, if we need it at all | ||
| if cached_schema.type in PRIMITIVE_TYPES: | ||
| raise ValueError( | ||
| "Expected schema of type `Record`, got instead primitive: {}".format( | ||
| raw_input_schema | ||
| ) | ||
| ) | ||
|
|
||
| record_format_identifier = b"\0\0\0\0" | ||
| schema_id = self._get_schema_id(cached_schema.fullname, str(cached_schema), **kwargs) | ||
| try: | ||
| schema_id = self._get_schema_id( | ||
| cached_schema.fullname, str(cached_schema), **kwargs | ||
| ) | ||
| except AttributeError: | ||
| raise ValueError( | ||
| "Expected schema of type `Record`, got instead schema of type: {}".format( | ||
| cached_schema.type | ||
| ) | ||
| ) | ||
| data_bytes = self._avro_serializer.serialize(value, cached_schema) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be safe here, we could abstract the underlying avro serializer library
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could make a matrix for comparison: 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) |
||
|
|
||
| stream = BytesIO() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # -------------------------------------------------------------------------- | ||
| # | ||
| # Copyright (c) Microsoft Corporation. All rights reserved. | ||
| # | ||
| # The MIT License (MIT) | ||
| # | ||
| # Permission is hereby granted, free of charge, to any person obtaining a copy | ||
| # of this software and associated documentation files (the ""Software""), to | ||
| # deal in the Software without restriction, including without limitation the | ||
| # rights to use, copy, modify, merge, publish, distribute, sublicense, and/or | ||
| # sell copies of the Software, and to permit persons to whom the Software is | ||
| # furnished to do so, subject to the following conditions: | ||
| # | ||
| # The above copyright notice and this permission notice shall be included in | ||
| # all copies or substantial portions of the Software. | ||
| # | ||
| # THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING | ||
| # FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS | ||
| # IN THE SOFTWARE. | ||
| # | ||
| # -------------------------------------------------------------------------- | ||
| from azure.core.exceptions import AzureError | ||
|
|
||
| class SchemaParseException(AzureError): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name sounds very generic. I'm leaning towards to having a common shared error, less types |
||
| """Error parsing a JSON schema. | ||
|
|
||
| :ivar message: The error message. | ||
| :vartype message: str | ||
| :ivar error: The error condition, if available. | ||
| :vartype error: str | ||
|
Comment on lines
+31
to
+34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) | ||
|
Comment on lines
+37
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could remove the |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| interactions: | ||
| - request: | ||
| body: '"{\"type\": \"error\", \"name\": \"User\", \"namespace\": \"example.avro.error\", | ||
| \"fields\": [{\"type\": \"string\", \"name\": \"name\"}]}"' | ||
| headers: | ||
| Accept: | ||
| - application/json | ||
| Accept-Encoding: | ||
| - gzip, deflate | ||
| Connection: | ||
| - keep-alive | ||
| Content-Length: | ||
| - '140' | ||
| Content-Type: | ||
| - application/json | ||
| User-Agent: | ||
| - azsdk-python-azureschemaregistry/1.0.0b1 Python/3.9.0 (Windows-10-10.0.19041-SP0) | ||
| X-Schema-Type: | ||
| - Avro | ||
| method: PUT | ||
| uri: https://fake_resource.servicebus.windows.net/$schemagroups/fakegroup/schemas/example.avro.error.User?api-version=2017-04 | ||
| response: | ||
| body: | ||
| string: '{"id":"4e9f5888b6f8411abaa4ce6c110b2fb3"}' | ||
| headers: | ||
| content-type: | ||
| - application/json | ||
| date: | ||
| - Fri, 01 Oct 2021 08:50:26 GMT | ||
| location: | ||
| - https://swathip-test-eventhubs.servicebus.windows.net:443/$schemagroups/fakegroup/schemas/example.avro.error.User/versions/1?api-version=2017-04 | ||
| server: | ||
| - Microsoft-HTTPAPI/2.0 | ||
| strict-transport-security: | ||
| - max-age=31536000 | ||
| transfer-encoding: | ||
| - chunked | ||
| x-schema-id: | ||
| - 4e9f5888b6f8411abaa4ce6c110b2fb3 | ||
| x-schema-id-location: | ||
| - https://swathip-test-eventhubs.servicebus.windows.net:443/$schemagroups/getschemabyid/4e9f5888b6f8411abaa4ce6c110b2fb3?api-version=2017-04 | ||
| x-schema-type: | ||
| - Avro | ||
| x-schema-version: | ||
| - '1' | ||
| x-schema-versions-location: | ||
| - https://swathip-test-eventhubs.servicebus.windows.net:443/$schemagroups/swathip-test-schema/schemas/example.avro.error.User/versions?api-version=2017-04 | ||
| status: | ||
| code: 200 | ||
| message: OK | ||
| version: 1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| interactions: | ||
| - request: | ||
| body: '"{\"type\": \"record\", \"aliases\": [\"Resu\"], \"name\": \"User_aliases\", | ||
| \"namespace\": \"example.avro\", \"fields\": [{\"type\": \"string\", \"name\": | ||
| \"name\"}]}"' | ||
| headers: | ||
| Accept: | ||
| - application/json | ||
| Accept-Encoding: | ||
| - gzip, deflate | ||
| Connection: | ||
| - keep-alive | ||
| Content-Length: | ||
| - '168' | ||
| Content-Type: | ||
| - application/json | ||
| User-Agent: | ||
| - azsdk-python-azureschemaregistry/1.0.0b1 Python/3.9.0 (Windows-10-10.0.19041-SP0) | ||
| X-Schema-Type: | ||
| - Avro | ||
| method: PUT | ||
| uri: https://fake_resource.servicebus.windows.net/$schemagroups/fakegroup/schemas/example.avro.User_aliases?api-version=2017-04 | ||
| response: | ||
| body: | ||
| string: '{"id":"83a1c5b1e3f049acae089ed34eb5191c"}' | ||
| headers: | ||
| content-type: | ||
| - application/json | ||
| date: | ||
| - Fri, 01 Oct 2021 07:29:00 GMT | ||
| location: | ||
| - https://swathip-test-eventhubs.servicebus.windows.net:443/$schemagroups/fakegroup/schemas/example.avro.User_aliases/versions/1?api-version=2017-04 | ||
| server: | ||
| - Microsoft-HTTPAPI/2.0 | ||
| strict-transport-security: | ||
| - max-age=31536000 | ||
| transfer-encoding: | ||
| - chunked | ||
| x-schema-id: | ||
| - 83a1c5b1e3f049acae089ed34eb5191c | ||
| x-schema-id-location: | ||
| - https://swathip-test-eventhubs.servicebus.windows.net:443/$schemagroups/getschemabyid/83a1c5b1e3f049acae089ed34eb5191c?api-version=2017-04 | ||
| x-schema-type: | ||
| - Avro | ||
| x-schema-version: | ||
| - '1' | ||
| x-schema-versions-location: | ||
| - https://swathip-test-eventhubs.servicebus.windows.net:443/$schemagroups/swathip-test-schema/schemas/example.avro.User_aliases/versions?api-version=2017-04 | ||
| status: | ||
| code: 200 | ||
| message: OK | ||
| version: 1 |
Uh oh!
There was an error while loading. Please reload this page.