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

adds decode/3 with format option #70

Merged

Conversation

mw23
Copy link
Contributor

@mw23 mw23 commented Feb 10, 2021

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
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
Owner

Strech commented Feb 18, 2021

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
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.

Copy link
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

lib/avrora/encoder.ex Show resolved Hide resolved
lib/avrora/encoder.ex Outdated Show resolved Hide resolved
@@ -732,4 +788,8 @@ defmodule Avrora.EncoderTest do
defp payment_json_schema do
~s({"namespace":"io.confluent","name":"Payment","type":"record","fields":[{"name":"id","type":"string"},{"name":"amount","type":"double"}]})
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
Owner

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
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -706,6 +753,10 @@ defmodule Avrora.EncoderTest do
119, 32, 97, 114, 101, 32, 121, 111, 117, 63, 0>>
end

defp numeric_transfer_plain_message_0 do
Copy link
Owner

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
Contributor Author

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 Show resolved Hide resolved
test/avrora/encoder_test.exs Show resolved Hide resolved
lib/avrora/encoder.ex Outdated Show resolved Hide resolved
lib/avrora/encoder.ex Outdated Show resolved Hide resolved
mw23 and others added 2 commits February 19, 2021 09:09
Co-authored-by: Sergey Fedorov <oni.strech@gmail.com>
@mw23
Copy link
Contributor Author

mw23 commented Feb 19, 2021

@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
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
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
Owner

Strech commented Feb 27, 2021

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants