Allow unpacked repeated primitives #224
Conversation
Codecov ReportBase: 89.04% // Head: 89.23% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #224 +/- ##
==========================================
+ Coverage 89.04% 89.23% +0.19%
==========================================
Files 25 25
Lines 2794 2797 +3
==========================================
+ Hits 2488 2496 +8
+ Misses 306 301 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Thank you for explanation! So if my understanding is correct, you are relying on an implementation detail. ProtoBuf tries to be very flexible in the way you serialize your messages, e.g. it does not require you to process fields in order which is nice bc you can start creating your payload as your data arrives. It also means that that binary payload is not guaranteed to be stable, as well as its hash. This section of docs https://developers.google.com/protocol-buffers/docs/encoding#order seem to try to warn people against depending on the exact byte output of a serialized message. AFAIK, for all intended use cases, the packed representation is the superior one. That being said I don't want to break your application, but I think we might want to make this strictly opt-in and hide it behind an options flag. |
|
Thank you for heads up. Existing code has everything in place. I just uncommented the section where [packed=true/false] option is ignored. I believe this PR (existing commented code) is already respecting it as a option. works for me with this [packed=false] option set after these changes. Sorry if you are aware of it and I am just repeating what you are trying to mention. But there are not tests around it. Is it ideal for you to have tests within this PR ? Let me know. I will give it a try. |
| @@ -30,15 +30,16 @@ function field_encode_expr(@nospecialize(f::FieldType), ctx::Context) | |||
| !isempty(encoding_val_type) && (encoding_val_type = ", Val{$encoding_val_type}") | |||
| # TODO: do we want to allow unpacked representation? Docs say that parsers must always handle both cases | |||
| # and since packed is strictly more efficient, currently we don't allow that. | |||
| # is_packed = parse(Bool, get(f.options, "packed", "false")) | |||
| # if is_packed | |||
| is_packed = parse(Bool, get(f.options, "packed", "false")) | |||
There was a problem hiding this comment.
I think we want get(f.options, "packed", "true") So people really have to make an effort to get the unpacked representation, i.e. they have to write [packed=false] in their proto files.
There was a problem hiding this comment.
Right. Sorry. In my case i needed it other way around and forgot about it.
|
Yes, adding tests would be great! |
|
I think this might mess up |
When supporting an existing protocol which uses unpacked representation for encoding and decoding we need unpacked representation support. If it is strictly encoding and decoding at the other end it is not important. But before decoding if hash is used to verify the object received in remote session then we would get into issues. The hash of the encoded representation will be different and will be rejected in our use case. For now I had to make a fork with these changes and continue using it. It would be ideal if changes are in the upstream. I was assuming you guys must have a reason and didn’t bother to raise an issue. I hope you are convinced about the use-case. If I am the protocol designer and implementer I would definitely avoid unpacked representation but to adhere to existing protocol it would make sense to support it. If it is very rare scenario and it complexifies the repo maintainance I understand.