Skip to content

Update schema naming strategy to allow key and value based namings #42

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

Merged
merged 1 commit into from
Apr 26, 2021
Merged

Update schema naming strategy to allow key and value based namings #42

merged 1 commit into from
Apr 26, 2021

Conversation

mohitpali
Copy link
Contributor

Issue #, #33

Description of changes: Extend customization of schema naming strategy by key and value

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -213,6 +213,7 @@
<!-- Ignore exception classes -->
<exclude>**/AWSIncompatibleDataException*</exclude>
<exclude>**/AWSSchemaRegistryException*</exclude>
<exclude>**/AWSSchemaNamingStrategy*</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to exclude this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is in an interface and by default we don't call default methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test it by creating an implementation that doesn't call it?

@blacktooth blacktooth merged commit 448d4c3 into awslabs:master Apr 26, 2021
default String getSchemaName(String transportName,
Object data,
boolean isKey) {
return isKey ? getSchemaName(transportName) : getSchemaName(transportName, data);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, by default data is ignored? I assume you did this for backwards compatibility, but it leads to issues discussed in #93

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.

3 participants