protobuf
protobuf copied to clipboard
elixir-protobuf
[Feature Request] JSON Encoding validate the Protobuffer Message as well
For context: https://github.com/elixir-protobuf/protobuf/pull/301
At the moment they are two different behaviors between MyMessage.encode/1 and Protobuf.JSON.encode!.
MyMessage.encode/1 does validate the structure of the message, while Protobuf.JSON.encode! does not. See https://github.com/elixir-protobuf/protobuf/pull/301 for more context and examples.
This caused some issues where we are using Protobuf for some json-protobuf-driven API where we trusted we did it correctly because it didn't fail. Still, it was incorrect because we technically added the wrong data to it.
Proposal
Protobuf.JSON.encode! should have the same behavior as MyMessage.encode/1 and validate the structure of the JSON.
I think we can implement this. Would you be willing to contribute this feature?
Yeah, I was intending to continue the work in #301 until I made it to work. I just wanted to confirm first that I wasn't crazy and was something that it should be happening
@ericmj should we validate when we use Message.new! the content? As I am fixing the issue, I thought about the fact why the factory function even allow me to create a broken message.
Maybe we should prevent issues about encoding even before getting to the Encoder. I am not sure.
@yordis validating in a pass before encoding turns into a significant performance hit, since we have to "walk" the struct to encode twice, once for validation and once for encoding.
should we validate when we use Message.new! the content?
What do you mean?
When I call new/1 from the Protobuf messages, I would expect that reject any invalid information. Like, why new/1 factory function of the protobuf structs would not fail when given invalid information?
@yordis validating in a pass before encoding turns into a significant performance hit, since we have to "walk" the struct to encode twice, once for validation and once for encoding.
I get that, but one of the encodings does it, which I am guessing is because the encoding must walk the struct anyway.
@yordis yes exactly, we can do it when encoding because we are walking the struct already anyways.
new/1 is something we want to get rid of in the future, in favor of building structs manually (like %MyMessage{...}).
So what should I do?
We would like to leverage the protobuf message on some Web-gRPC API (provided to us) by encoding using protobuf struct rather than maps to avoid silly mistakes of API calls.
We already ran into some unexpected behavior that we instead pay the cost of validating such a message.
It seems that https://github.com/elixir-protobuf/protobuf/blob/885f39e2eccd5468d885d62955fcf695ac6f1fce/lib/protobuf/json/encode.ex#L17-L22 already walk the struct thou
I got it working for the string at least, so probably better to move the conversation to the PR for now