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.

Sign up for GitHub

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

feat: add default values parameter to to_json #164

Merged
merged 2 commits into from
Nov 20, 2020
Merged

feat: add default values parameter to to_json #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8ce3ae
feat: add default values parameter to to_json
yon-mg Nov 20, 2020
d20b585
fix: typing & stylecheck issues
yon-mg Nov 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions proto/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,13 @@ def deserialize(cls, payload: bytes) -> "Message":
"""
return cls.wrap(cls.pb().FromString(payload))

def to_json(cls, instance, *, use_integers_for_enums=True) -> str:
def to_json(
cls,
instance,
*,
use_integers_for_enums=True,
including_default_value_fields=True
Copy link
Contributor

Choose a reason for hiding this comment

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

@yon-mg Could you open a follow up PR to update the docstring below?

This will result in a change in behavior for someone already using this method Protobuf defaults to False for this option. https://googleapis.dev/python/protobuf/latest/google/protobuf/json_format.html#google.protobuf.json_format.MessageToJson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. I did not change the way google.protobuf.json_format.MessageToJson works. In fact this method message.MessageMeta.to_json should call MessageToJson in the same way as it did before if no changes are made to the additional parameter I added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I did neglect to update the docstring for this method which I will update shortly if that's what you mean.

) -> str:
"""Given a message instance, serialize it to json

Args:
Expand All @@ -343,7 +349,7 @@ def to_json(cls, instance, *, use_integers_for_enums=True) -> str:
return MessageToJson(
cls.pb(instance),
use_integers_for_enums=use_integers_for_enums,
including_default_value_fields=True,
including_default_value_fields=including_default_value_fields,
)

def from_json(cls, payload, *, ignore_unknown_fields=False) -> "Message":
Expand Down
23 changes: 23 additions & 0 deletions tests/test_json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,29 @@ class Zone(proto.Enum):
assert json2 == '{"zone":"EPIPELAGIC"}'


def test_json_default_values():
class Squid(proto.Message):
mass_kg = proto.Field(proto.INT32, number=1)
name = proto.Field(proto.STRING, number=2)

s = Squid(name="Steve")
json1 = (
Squid.to_json(s, including_default_value_fields=False)
.replace(" ", "")
.replace("\n", "")
)
assert json1 == '{"name":"Steve"}'

json2 = Squid.to_json(s).replace(" ", "").replace("\n", "")
assert (
json2 == '{"name":"Steve","massKg":0}' or json2 == '{"massKg":0,"name":"Steve"}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ordering not stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No but it shouldn't matter for json objects right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should matter, I just was curious if you were running into problems during testing.

)

s1 = Squid.from_json(json1)
s2 = Squid.from_json(json2)
assert s == s1 == s2


def test_json_unknown_field():
# Note that 'lengthCm' is unknown in the local definition.
# This could happen if the client is using an older proto definition
Expand Down