Skip to content
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

Only include schemaType and References when needed #98

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

adamtabrams
Copy link
Contributor

@adamtabrams adamtabrams commented Jan 24, 2023

Older versions of the Confluent Schema Registry API don't support fields schemaType or References. Right now, this package always includes these in requests, even if they aren't being used. Since these fields were added to the API, the documentation has stated that they are optional fields. And it's documented that schemaType is always assumed to be Avro when it isn't specified.

This PR ensures that these fields are only included in the request when they're not empty. And it does this with minimal changes to the package.

I need this behavior in order to use this package for my use case. And I've found a few previous issues that had problems that this PR would address:
#11 #19 #21

In addition to the unit tests added in this PR, I've also tested these changes with integration tests specific to my use case.

Please feel free to mention any questions or concerns.

@adamtabrams
Copy link
Contributor Author

Just bumping this PR
@riferrei @AtakanColak

@AtakanColak
Copy link
Collaborator

Hi Adam, thank you for your contribution. I'm inclined to merge this change, except the part I've commented on. Could you please either walk me through the cases where empty schema type would be required or revert the change?

@AtakanColak AtakanColak merged commit 96a09e7 into riferrei:master Mar 16, 2023
@AtakanColak
Copy link
Collaborator

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants