Skip to content

adds decode/3 with format option#70

Merged
Strech merged 5 commits intoStrech:masterfrom
mw23:fix/plain_binary_decoding_when_schema_is_given
Feb 22, 2021
Merged

adds decode/3 with format option#70
Strech merged 5 commits intoStrech:masterfrom
mw23:fix/plain_binary_decoding_when_schema_is_given

Conversation

@mw23
Copy link
Copy Markdown
Contributor

@mw23 mw23 commented Feb 10, 2021 •
edited
Loading

This fixes a bug where decoding fails for a data that was encoded with a supplied schema_name and :plain format

If the encoded message starts with the magic byte (0x00) due to the data that was encoded, the wrong codec is selected by
https://github.com/Strech/avrora/blob/master/lib/avrora/encoder.ex#L76
when decoding it later.
This causes the decoding to fail sporadically dependent on the encoded data.

I propose to fix this by including an optional format argument to the decode function.

Example: New test case that broke before adding the format: plain parameter

  1) test when decoding plain message that starts with what looks like a magic byte (Avrora.EncoderTest)
     test/avrora/encoder_test.exs:405
     Assertion with == failed
     code:  assert key == "io.confluent.numeric_transfer"
     left:  3906769129
     right: "io.confluent.numeric_transfer"
     stacktrace:
       test/avrora/encoder_test.exs:411: anonymous fn/1 in Avrora.EncoderTest."test when decoding plain message that starts with what looks like a magic byte"/1
       (avrora 0.17.0) lib/avrora/resolver.ex:47: Avrora.Resolver.resolve/1
       (avrora 0.17.0) lib/avrora/resolver.ex:26: anonymous fn/1 in Avrora.Resolver.resolve_any/1
       (elixir 1.10.4) lib/stream.ex:572: anonymous fn/4 in Stream.map/2
       (elixir 1.10.4) lib/enum.ex:3686: Enumerable.List.reduce/3
       (elixir 1.10.4) lib/stream.ex:1609: Enumerable.Stream.do_each/4
       (elixir 1.10.4) lib/enum.ex:1027: Enum.find_value/3
       (avrora 0.17.0) lib/avrora/codec/schema_registry.ex:57: Avrora.Codec.SchemaRegistry.decode/2
       test/avrora/encoder_test.exs:437: (test)

@mw23 mw23 force-pushed the fix/plain_binary_decoding_when_schema_is_given branch 2 times, most recently from 23d93b1 to 1163f20 Compare February 11, 2021 16:31
@Strech
Copy link
Copy Markdown
Owner

Strech commented Feb 12, 2021

Hi @mw23 👋🏼

Thanks a lot for your contribution, I appreciate it! Let me think a bit about proposed changes over weekends

@Strech
Copy link
Copy Markdown
Owner

Strech commented Feb 18, 2021 •
edited
Loading

Thanks for the patience @mw23 I've read the code one more time and I think I have something to rise.

decode/3 makes sense only for plain format because all the others must ignore the given option as they heavily rely on the message itself (registry – magic byte, of – special header byte) and it just doesn't make much sense.

At the same time, I think, we should be able to enforce decoding for the plain format to overcome the collision. But I think it should be done in some sort of special case (exception) and not become a generic approach which might confuse.

As a suggestion, maybe we should come up with decode_plain exceptional function or find another way to enforce the plain format

UPD: Also by guess format you never can end with plain format and maybe it makes sense to extract both encode_plain/decode_plain at all ... but I'm not sure

@mw23 mw23 force-pushed the fix/plain_binary_decoding_when_schema_is_given branch from 1163f20 to 4d93474 Compare February 18, 2021 22:13
@mw23
Copy link
Copy Markdown
Contributor Author

mw23 commented Feb 18, 2021

Hi @Strech, thank you for considering this.

I see your point and agree that separate function(s) might be a way to go to make this cleaner so I went ahead and added encode_plain/2 as well as decode_plain/2 to have a mirroring API.

To your argument about handling this as exception in decode/2 - I think that might be less clean and could be a bit confusing to the user -- Our use-case would always be handled after an exception was raised in decode, which is a bit odd.

Please let me know what you think.

@Strech Strech mentioned this pull request Feb 19, 2021
Open
Strech
Strech requested changes Feb 19, 2021
View reviewed changes
Copy link
Copy Markdown
Owner

@Strech Strech left a comment

Choose a reason for hiding this comment

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

Thanks for your effort. I would like to request one thing in addition to the comments.

Can you two made up calls here to validate an external interface

test/avrora/encoder_test.exs Outdated
end

defp numeric_transfer_json_schema do
~s({"namespace":"io.confluent","name":"numeric_transfer","type":"record","fields":[{"name":"link_is_enabled","type":"boolean"},{"name":"updated_at","type":"int"},{"name":"updated_by_id","type":"int"}]})
Copy link
Copy Markdown
Owner

@Strech Strech Feb 19, 2021

Choose a reason for hiding this comment

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

May I ask you to follow the guideline for naming (it's not so important here, but oh my OCD) and rename the schema name to NumericTransfer

Copy link
Copy Markdown
Contributor Author

@mw23 mw23 Feb 19, 2021

Choose a reason for hiding this comment

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

done

test/avrora/encoder_test.exs Outdated
119, 32, 97, 114, 101, 32, 121, 111, 117, 63, 0>>
end

defp numeric_transfer_plain_message_0 do
Copy link
Copy Markdown
Owner

@Strech Strech Feb 19, 2021

Choose a reason for hiding this comment

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

Maybe to emphasize the issue we can name it numeric_transfer_plain_message_with_fake_magic_byte

Copy link
Copy Markdown
Contributor Author

@mw23 mw23 Feb 19, 2021

Choose a reason for hiding this comment

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

done

mw23 and others added 2 commits February 19, 2021 09:09
@mw23 @Strech
Apply suggestions from code review
240c968 
Co-authored-by: Sergey Fedorov <oni.strech@gmail.com>
applies changes requested in MR
eb3bff4
@mw23
Copy link
Copy Markdown
Contributor Author

mw23 commented Feb 19, 2021 •
edited
Loading

@Strech Thanks for your review. I incorporated the changes you requested.
I have struggled with enforcing the line length -- see my comment in the code above.

@Strech
Copy link
Copy Markdown
Owner

Strech commented Feb 20, 2021

@mw23 Thanks a lot for such a quick update. Everything looks good to me. I will merge it on a Sunday 💙

@mw23
Copy link
Copy Markdown
Contributor Author

mw23 commented Feb 21, 2021

@Strech - that's great news! Thank you!

@Strech Strech merged commit 89d18b3 into Strech:master Feb 22, 2021
@Strech
Copy link
Copy Markdown
Owner

Strech commented Feb 27, 2021 •
edited
Loading

Thanks @mw23 for your contribution! The changes you've proposed released in a v0.18 version 🎉 🍾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Strech Strech Strech approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mw23 @Strech