-
Notifications
You must be signed in to change notification settings - Fork 925
Add missing endpoints (/compatibility, /mode) to SchemaRegistryClient #2024
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
base: master
Are you sure you want to change the base?
Conversation
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Add missing endpoints /compatibility
and /mode
to SchemaRegistryClient with comprehensive test coverage. This PR expands the Schema Registry client functionality by implementing several new API methods for both sync and async clients.
- Adds new endpoints for schema operations:
get_schema_string
,get_schema_types
,get_schema_versions
- Adds new endpoints for subject operations:
get_version_schema_string
,get_referenced_by
- Adds compatibility testing against all versions:
test_compatibility_against_all
- Updates existing methods to accept new optional parameters for filtering
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
tests/schema_registry/conftest.py |
Adds mock handlers and regex patterns for new endpoints |
tests/schema_registry/_sync/test_api_client.py |
Adds comprehensive test coverage for new sync client methods |
tests/schema_registry/_async/test_api_client.py |
Adds comprehensive test coverage for new async client methods |
tests/integration/schema_registry/_sync/test_proto_serializers.py |
Updates test to use new RegisteredSchema return type |
tests/integration/schema_registry/_async/test_proto_serializers.py |
Updates test to use new RegisteredSchema return type |
src/confluent_kafka/schema_registry/common/schema_registry_client.py |
Adds SchemaVersion class and refactors field ordering |
src/confluent_kafka/schema_registry/_sync/schema_registry_client.py |
Implements new API methods for sync client |
src/confluent_kafka/schema_registry/_async/schema_registry_client.py |
Implements new API methods for async client |
DEVELOPER.md |
Removes trailing whitespace |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
query={'normalize': normalize, 'verbose': verbose}, | ||
body=request, | ||
) | ||
return response['is_compatible'] # TODO: should it return entire response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment indicates incomplete implementation. Consider either returning the full response or documenting why only 'is_compatible' is returned.
return response['is_compatible'] # TODO: should it return entire response | |
return response |
Copilot uses AI. Check for mistakes.
query={'normalize': normalize, 'verbose': verbose}, | ||
body=request, | ||
) | ||
return response['is_compatible'] # TODO: should it return entire response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment indicates incomplete implementation. Consider either returning the full response or documenting why only 'is_compatible' is returned.
return response['is_compatible'] # TODO: should it return entire response | |
return response |
Copilot uses AI. Check for mistakes.
subject_name (str): Name of the schema against which compatibility is to be tested. | ||
schema (Schema): Schema instance. | ||
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs | ||
verbose (bool): Wehther to return detailed error messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: 'Wehther' should be 'Whether'.
verbose (bool): Wehther to return detailed error messages. | |
verbose (bool): Whether to return detailed error messages. |
Copilot uses AI. Check for mistakes.
subject_name (str): Name of the schema against which compatibility is to be tested. | ||
schema (Schema): Schema instance. | ||
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs | ||
verbose (bool): Wehther to return detailed error messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: 'Wehther' should be 'Whether'.
verbose (bool): Wehther to return detailed error messages. | |
verbose (bool): Whether to return detailed error messages. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have copilot find typos to save reading for them :D
Args: | ||
subject_name (str): Name of the schema against which compatibility is to be tested. | ||
schema (Schema): Schema instance. | ||
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment suggests incomplete documentation. Consider either updating the documentation or removing the comment if it's not actionable.
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs | |
normalize (bool): Whether to normalize the input schema. |
Copilot uses AI. Check for mistakes.
Args: | ||
subject_name (str): Name of the schema against which compatibility is to be tested. | ||
schema (Schema): Schema instance. | ||
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment suggests incomplete documentation. Consider either updating the documentation or removing the comment if it's not actionable.
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs | |
normalize (bool): Whether to normalize the input schema. |
Copilot uses AI. Check for mistakes.
ddf1826
to
dc8d820
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
subject_name (str): Name of the schema against which compatibility is to be tested. | ||
schema (Schema): Schema instance. | ||
normalize (bool): Whether to normalize the input schema. # TODO: missing in cp + cc docs | ||
verbose (bool): Wehther to return detailed error messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have copilot find typos to save reading for them :D
) | ||
return response['is_compatible'] # TODO: should it return entire response (including error messages)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe return a tuple [bool, Response] so it's easy to have a simple response vs a parsing the complex object? But I think this should probably return the full response by itself with a reference in the docstring to the is_compatible
as a likely attribute for consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is this would be a breaking change, although in this case I think it might be necessary to just make the change (or add a separate method and mark this one as @deprecated): not sure how customers usually interact with test_compatibility, but intuitively I think the "messages" field for test_compatibility is important to include
cc @rayokota if you have any thoughts on this :)
`GET Global Mode API Reference <https://docs.confluent.io/current/schema-registry/develop/api.html#get--mode>`_ | ||
""" # noqa: E501 | ||
result = self._rest_client.get('mode') | ||
return result['mode'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a debug log here with the result if you're not returning the full payload so the contents aren't lost (or maybe generally have a logging debug level option for all rest requests made to SR service)
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
7a24c92
to
9b468a7
Compare
What
Add the missing endpoints and update the existing ones to SR client, for /compatibility and /mode endpoints.
Checklist
References
JIRA: https://confluentinc.atlassian.net/browse/DGS-21591
Test & Review
Open questions / Follow-ups