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

JS: parse (un)packed fields conditionally #7379

Merged
merged 1 commit into from Aug 31, 2020

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Apr 15, 2020

The current JS implementation doesn't comply with the following criteria:

Note that although there's usually no reason to encode more than one key-value pair for a packed repeated field, encoders must be prepared to accept multiple key-value pairs. In this case, the payloads should be concatenated. Each pair must contain a whole number of elements.
https://developers.google.com/protocol-buffers/docs/encoding#packed

Protocol buffer parsers must be able to parse repeated fields that were compiled as packed as if they were not packed, and vice versa. This permits adding [packed=true] to existing fields in a forward- and backward-compatible way.

https://developers.google.com/protocol-buffers/docs/encoding#packed

In this PR, the parser checks WireType before parsing packable fields, and then always append to the existing array, instead of replacing it.

Fixes #1701.

@qnighy

This comment has been minimized.

@qnighy
Copy link
Contributor Author

qnighy commented Apr 17, 2020

@TeBoring

@qnighy
Copy link
Contributor Author

qnighy commented May 5, 2020

@TeBoring I removed the relevant lines from conformance/failure_list_js.txt to enable conformance tests. Is this enough?

@qnighy
Copy link
Contributor Author

qnighy commented Jun 12, 2020

@TeBoring we're occasionally hit by this problem (since our JavaScript client communicates with Ruby servers, where the current implementation always encodes repeated fields unpacked). It's great if you have time to look at this.

@qnighy
Copy link
Contributor Author

qnighy commented Jun 16, 2020

@haberman sorry for the sudden mention, but could you or someone else look at this? The diff is essentially about 30 lines and it should be relatively easy to review...

We're actively applying changes to our internal protobuf messages which are sent between JS and Ruby, and #1701 is a real annoyance since failing to add [packed = false] to a newly defined repeated scalar field ruins everything.

@izumin5210
Copy link

@haberman @TeBoring @dlj-NaN can you help to review?

@qnighy
Copy link
Contributor Author

qnighy commented Aug 20, 2020

@haberman thanks for reviewing and kicking the CI! The only failures are Java things (which I think is unrelated to this PR) and the lack of language label (which I cannot help), so I guess there's nothing I can do now. Right?

スクリーンショット 2020-08-20 11 39 42

@qnighy
Copy link
Contributor Author

qnighy commented Aug 31, 2020

@haberman could you merge this?

@perezd
Copy link
Contributor

perezd commented Aug 31, 2020

Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaScript parser fails to parse packed repeated field as not packed (and vice versa)
6 participants