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

Second stab at adding JSON support #42

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

m-o-e
Copy link
Contributor

@m-o-e m-o-e commented Jul 17, 2021

Following the discussion in #22 (comment)
this adds to_json and from_json to all generated message-classes.

It works surprisingly well for me (even with nested objects etc.)
but the following caveats apply:

@m-o-e m-o-e changed the title First stab at adding JSON support ~First~ Second stab at adding JSON support Jul 17, 2021
@m-o-e m-o-e changed the title ~First~ Second stab at adding JSON support Second stab at adding JSON support Jul 17, 2021
@jgaskins
Copy link
Collaborator

Based on the link you gave to the JSON-encoding spec, this should actually be pretty trivial to solve if bytes is the only type it can't handle:

JSON value for a bytes type will be the data encoded as a string using standard base64 encoding with paddings. Either standard or URL-safe base64 encoding with/without paddings are accepted.

If we add a @[JSON::Field(converter: ::Protobuf::JSONBytesSerializer)] annotation to all bytes fields, we'll only need to implement JSONBytesSerializer module to automatically handle conversion to and from base64.

That link to the spec should make it easy to write up some tests for this. 💯

{% for tag, field in FIELDS %}
self.{{field[:name].id}} = @{{field[:name].id}}
{% end %}
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this change:

FooBar.from_json # => { oo_a: 1, oo_b: nil, my_oneof: nil }

After this change:

FooBar.from_json # => { oo_a: 1, oo_b: nil, my_oneof: "oo_a" }

Tested it manually a bunch and it appears to work reliably also with nested objects.

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