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

Add Message.from_json #411

Open
wants to merge 7 commits into
base: 3-10-stable
Choose a base branch
from

Conversation

aslakhellesoy
Copy link

@aslakhellesoy aslakhellesoy commented Jan 6, 2020

This PR adds a new Message.from_json class method that turns a JSON string into a message object.

It addresses three shortcomings:

  • Lower camel case field names (fooBarZap) in the JSON are converted to underscore (fooBarZap)
  • Values for bytes fields are Base64 decoded unless they are already in "binary" format. This is because Protobuf's JSON representation uses Base64 encoding for bytes fields.
  • Enum values in the JSON can be either string or number - they will both be deserialised properly

@aslakhellesoy
Copy link
Author

Please hold off on merging this while I fix the failing tests. I'm also adding a bugfixes for enum fields and JSON.

@aslakhellesoy
Copy link
Author

aslakhellesoy commented Jan 7, 2020

With this change, bytes values have to be encoded as Encoding::ASCII_8BIT, which in Ruby means "binary" encoding. Any other encoding will cause the value to be Base64 decoded.

This is because the JSON representation of bytes is always Base64. When deserialising JSON we don't have the protobuf field info, so the decoding is (unfortunately) happening inside Protobuf::Field::BytesField, based on whether the value is a "binary" string or a "base64" string.

when Array
ob.map { |value| normalize_json(value) }
when Hash
Hash[*ob.flat_map { |key, value| [key.underscore, normalize_json(value)] }]
Copy link
Member

Choose a reason for hiding this comment

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

The underscore thing seems like we'd want to make that optional?

@@ -48,7 +48,18 @@ def wire_type

def coerce!(value)
case value
when String, Symbol
when String
if value.encoding == Encoding::ASCII_8BIT
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make this configurable at the from_json method? You can specify if your uses Base64 encoded binary blobs? Maybe true by default but allow the caller to turn it off? That way we don't have to guess.

@jackorp
Copy link

jackorp commented Mar 17, 2021

Ping... Hi, what's the status on this? I am packaging this library for Fedora as a part of an effort to update cucumber and it would be nice if this PR and PR#415 were merged.

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.

3 participants