fix: setting 64bit fields from strings supported by software-dov · Pull Request #267 · googleapis/proto-plus-python

software-dov · 2021-10-20T22:52:47Z

Due to limitations in certain browser/javascript combinations,
fields that are 64 bit integer types are converted to strings when
encoding a message to JSON or a dict.
Decoding from JSON handles this explicitly, but it makes converting
back from a dict an error.

This fix adds support for setting these 64 bit fields explicitly from
strings.

E.g.

classSquid(proto.Message):
mass_kg=proto.Field(proto.INT64, number=1)
s=Squid(mass_kg=10)
s_dict=Squid.to_dict(s)
asserts==Squid(s_dict)
s.mass_kg="20"# Supported

codecov · 2021-10-20T22:53:47Z

Codecov Report

Merging #267 (b50c91d) into master (6a43682) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #267 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 20 22 +2 Lines 862 961 +99 Branches 149 170 +21 =========================================
+ Hits 862 961 +99

Impacted Files	Coverage Δ
proto/modules.py	`100.00% <ø> (ø)`
proto/enums.py	`100.00% <100.00%> (ø)`
proto/fields.py	`100.00% <100.00%> (ø)`
proto/marshal/collections/repeated.py	`100.00% <100.00%> (ø)`
proto/marshal/marshal.py	`100.00% <100.00%> (ø)`
proto/marshal/rules/bytes.py	`100.00% <100.00%> (ø)`
proto/marshal/rules/message.py	`100.00% <100.00%> (ø)`
proto/marshal/rules/stringy_numbers.py	`100.00% <100.00%> (ø)`
proto/message.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update debd16f...b50c91d. Read the comment docs.

Due to limitations in certain browser/javascript combinations, fields that are 64 bit integer types are converted to strings when encoding a message to JSON or a dict. Decoding from JSON handles this explicitly, but it makes converting back from a dict an error. This fix adds support for setting these 64 bit fields explicitly from strings. E.g. class Squid(proto.Message): mass_kg = proto.Field(proto.INT64, number=1) s = Squid(mass_kg=10) s_dict = Squid.to_dict(s) assert s == Squid(**s_dict) # Supported assert s == Squid(s_dict) # NOT supported for performance reasons s.mass_kg = "20" # Supported

This includes 1) Do not include asyncio tests in the generated tests, because rest transport does not have asynio client. 2) Generate body field mock values for generated tests (otherwise grpc transcodding logic would fail). 3) Make `always_use_jwt_access=True` default for rest clients (grpc already does that) to match expected calls in generated tests. 4) Fix mypy errors for `AuthorizedSession` by ignoring it 5) Include operations_v1 conditionally, only if the client has lro There are few more fixes left, which are expected to be fixed in separate PRs. 1) `message->to_dict->message` roundrtip problem for int64 types is expected to be fixed by googleapis/proto-plus-python#267 2) builtins conflicts (`license_` vs `license` as field name) is expected to be fixed by a TBD PR

Co-authored-by: Tres Seaver <tseaver@palladion.com>

google-cla · 2021-10-25T16:16:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tseaver · 2021-10-25T16:26:06Z

@googlebot I consent.

google-cla · 2021-10-25T16:26:11Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tseaver · 2021-10-25T16:26:39Z

@googlebot I fixed it.

google-cla · 2021-10-25T16:26:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tseaver · 2021-10-25T16:27:11Z

@software-dov Ugh, I'm sorry -- the CLA bot has lost its wig again.

Add test and impl for deeply nested messages with 64 bit int fields Remove doc changes

…-plus-python into stringy-int64

google-cla · 2021-10-25T17:43:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-10-25T17:52:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tseaver · 2021-10-25T18:18:13Z

+ # If we have a type error,
+ # try the slow path in case the error
+ # was an int64/string issue
+ return self._wrapper(value)._pb


Yay! that is a waaaay better devx.

tseaver · 2021-10-25T18:18:54Z

+
+ c2 = Clam(c_dict)
+
+ assert c == c2


Excellent roundtrip demo.

tseaver · 2021-10-25T18:19:39Z

@software-dov You'll have to flip the cla: no label to cla: yes manually.

* fix: Fix rest transport logic This includes 1) Do not include asyncio tests in the generated tests, because rest transport does not have asynio client. 2) Generate body field mock values for generated tests (otherwise grpc transcodding logic would fail). 3) Make `always_use_jwt_access=True` default for rest clients (grpc already does that) to match expected calls in generated tests. 4) Fix mypy errors for `AuthorizedSession` by ignoring it 5) Include operations_v1 conditionally, only if the client has lro There are few more fixes left, which are expected to be fixed in separate PRs. 1) `message->to_dict->message` roundrtip problem for int64 types is expected to be fixed by googleapis/proto-plus-python#267 2) builtins conflicts (`license_` vs `license` as field name) is expected to be fixed by a TBD PR * fix integration tests

software-dov requested review from a team, arithmetic1728 and busunkim96 October 20, 2021 22:52

google-claBot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2021

software-dov force-pushed the stringy-int64 branch from e86d00c to 0a582e6Compare October 20, 2021 22:54

busunkim96 reviewed Oct 21, 2021
View reviewed changes

Comment threaddocs/marshal.rst Outdated

kbandes approved these changes Oct 21, 2021
View reviewed changes

tseaver reviewed Oct 21, 2021
View reviewed changes

Comment threaddocs/marshal.rst Outdated
Comment threaddocs/marshal.rst Outdated

vam-google mentioned this pull request Oct 22, 2021
fix: Fix rest transport logic googleapis/gapic-generator-python#1039
Merged

Update docs/marshal.rst
996a8f9
Co-authored-by: Tres Seaver <tseaver@palladion.com>

google-claBot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Oct 25, 2021

software-dov added 2 commits October 25, 2021 10:38

Integrate reviews and handle deeply nested messages
15cb33d
Add test and impl for deeply nested messages with 64 bit int fields Remove doc changes

Merge branch 'stringy-int64' ofhttps://github.com/software-dov/proto…
6966256
…-plus-python into stringy-int64

Satiate stylecheck
b50c91d

software-dov requested review from busunkim96 and tseaver October 25, 2021 17:56

tseaver approved these changes Oct 25, 2021
View reviewed changes

software-dov added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 25, 2021

software-dov merged commit ea7b911 into googleapis:masterOct 25, 2021

software-dov deleted the stringy-int64 branch October 25, 2021 18:54

software-dov mentioned this pull request Oct 26, 2021
Invalid type for user_id field returned by TransferConfig.to_dict #240
Closed

software-dov linked an issue Oct 26, 2021 that may be closed by this pull request
Invalid type for user_id field returned by TransferConfig.to_dict #240
Closed

tseaver mentioned this pull request Oct 27, 2021
64bit fields no longer support None values in v1.19.6 #271
Closed

vam-google mentioned this pull request Aug 2, 2022
Proto JSON cannonical encoding is not supported for special types #333
Closed

Uh oh!

Conversation

software-dov commented Oct 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Oct 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

google-claBot commented Oct 25, 2021

Uh oh!

tseaver commented Oct 25, 2021

Uh oh!

google-claBot commented Oct 25, 2021

Uh oh!

tseaver commented Oct 25, 2021

Uh oh!

google-claBot commented Oct 25, 2021

Uh oh!

tseaver commented Oct 25, 2021

Uh oh!

google-claBot commented Oct 25, 2021

Uh oh!

google-claBot commented Oct 25, 2021

Uh oh!

tseaverOct 25, 2021

Choose a reason for hiding this comment

Uh oh!

tseaverOct 25, 2021

Choose a reason for hiding this comment

Uh oh!

tseaver commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

software-dov commented Oct 20, 2021•
edited
Loading

codecovBot commented Oct 20, 2021•
edited
Loading