Hitting `no case clause matching: :group_end` in Elixir app using Exprotobuf/gpb about gpb HOT 8 CLOSED

But the top-level wrapper to translate core errors into library-specific errors does seem like a great idea.

I'll look into doing it.

from gpb.

No, a group_end element will normally never occur before a group_start element if the input binary is well formed. Maybe the question is what should happen if the input binary to decode is not well formed?

from gpb.

Thanks for the reply! I'm still getting up to speed on protobuf and amqp so it's quite possible that this test is faulty. I only opened an issue because it seemed clearly possible that, since skip_field calls decode_wiretype and 4/group_end is a possible value there, that the case in skip_field should handle that, even if it is to just raise. It just seems weird (at least in Elixir world) to let it throw a CaseClauseError. But I can look into making the test more real-world. Thanks again!

from gpb.

Gpb can generate encoding/decoding code for a given set of protobuf definitions, I think this is how most people use it, but it also has an data-driven encoder/decoder, the gpb.erl module, which it appears exprotobuf uses. (It by dec). Both the generated code and the data-driven gpb.erl has the property that it either succeeds doing what it should on valid input, but fail on invalid input. However, the generated code fails with either a {gpb_error,Reason} or {gpb_type_error,Reason}, while the data-driven gpb.erl may crash with any reason, a function or case clause for instance, as you have seen.

I could (and think it would make sense) wrap the data-driven encoder/decoder in gpb.erl in some top-level try catch to make sure it, too, crashes with the same kind of gpb_errors that the generated code uses. So, the gpb would still crash on <<" BAD PAYLOAD">> but it with a {gpb_error, ...} instead of with a case_clause.

Most development work has gone into the code generator, and I've kept the data-driven encoder/decoder so far for what I thought was mostly internal reasons, but now I know it is used by others as well.

So I guess the bottom line when it comes to your part is that if you get some bad payload and feed it eventually into gpb for decoding, what you can rely on is that if it is some crash, then it was invalid input, but you should not rely on the exact crash term. Also, I don't know if exprotobuf wraps this in some way, but from the stacktrace, it appears that it does not. On the other hand, if there is no crash, it does not necessarily mean that the input was valid. You could for instance have decided that exacly one of two optional fields must be set, but if none of them is set, gpb will happily decode. For generated code, there is the verify_decode_required_present option, but this is not present with the data-driven gpb.erl (no such options are).

from gpb.

@tomas-abrahamsson thanks for the detailed explanation. I see what you mean about code intending to be internal vs external. And this test isn't very realistic either. I doubt we would actually hit this in live code. But the top-level wrapper to translate core errors into library-specific errors does seem like a great idea. @bitwalker for viz on the exprotobuf part...

from gpb.

Is the (temporary) branch gpb_error-for-gpb in line with what you had in mind? If so, I can merge it together with a few other very minor things that I have pending.

from gpb.

Well my erlang knowledge is very slim but this seems like the right idea: wrapping the group_end failure in a gpb_error. Hopefully you can get some erlangist eyes on it though. Thanks!

from gpb.

I've pushed gpb 4.19.0 which wraps errors in {gpb_error, Info}, so I'll close this.

For the archives, I'll note that this change concerns the data-driven encoder/decoder in the gpb module. The wrapping has been present in the generated code already since 3.28.1. (I'll remove the temporary branch shortly)

from gpb.