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

fix: handling of map entries with omitted key or value #1348

Merged
merged 2 commits into from
Jul 10, 2020

Conversation

tq-schifferm
Copy link
Contributor

@tq-schifferm tq-schifferm commented Jan 20, 2020

According to [1], map encoding must be compatible with a repeated message
using indices 1 and 2 for key and value. In particular this implies that
both key and value may be omitted when they are equal to the default
value - which some protobuf implementations like protobuf-c actually do.

The comments in the added testcase are based on the output of
protobuf-inspector [2].

[1] https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility
[2] https://github.com/jmendeth/protobuf-inspector

This is a cleaned up version of #1087:

  • Added detailed commit message
  • Improved coding style
  • Added test case

Supersedes #845
Supersedes #1087
Fixes #843
Fixes #960
Fixes #1087

src/decoder.js Outdated
if (type === "string") gen
("value=\"%s\"", types.defaults[type]);
else if (types.defaults[type] !== undefined) gen
("value=%s", types.defaults[type]);

Choose a reason for hiding this comment

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

if type === "bytes" then this tries to insert the default value of an empty array with %s and it ends up inserting empty string "" instead of "[]". This is because %s calls String([]) when we need %j here so we use JSON.stringify([]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

According to [1], map encoding must be compatible with a repeated message
using indices 1 and 2 for key and value. In particular this implies that
both key and value may be omitted when they are equal to the default
value - which some protobuf implementations like protobuf-c actually do.

The comments in the added testcase are based on the output of
protobuf-inspector [2].

[1] https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility
[2] https://github.com/jmendeth/protobuf-inspector

Based-on-patch-by: Shrimpz <Shrimpz@qq.com>
@paambaati
Copy link

@schiffermtq Thank you so much for this PR! This should also fix #1293

Copy link

@adriancable adriancable left a comment

Choose a reason for hiding this comment

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

This looks (and works) great.

@paambaati
Copy link

@nicolasnoble @alexander-fenster Anything we can do to get this PR reviewed/merged?

@adriancable
Copy link

Bump.

@alexander-fenster alexander-fenster changed the title Fix handling of map entries with omitted key or value fix: handling of map entries with omitted key or value Jul 10, 2020
@alexander-fenster alexander-fenster merged commit b950877 into protobufjs:master Jul 10, 2020
@adriancable
Copy link

YES! Thank you so much.

taylorcode pushed a commit to taylorcode/protobuf.js that referenced this pull request Oct 16, 2020
According to [1], map encoding must be compatible with a repeated message
using indices 1 and 2 for key and value. In particular this implies that
both key and value may be omitted when they are equal to the default
value - which some protobuf implementations like protobuf-c actually do.

The comments in the added testcase are based on the output of
protobuf-inspector [2].

[1] https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility
[2] https://github.com/jmendeth/protobuf-inspector

Based-on-patch-by: Shrimpz <Shrimpz@qq.com>

Co-authored-by: Alexander Fenster <fenster@google.com>
This was referenced May 20, 2022
@github-actions github-actions bot mentioned this pull request Jul 8, 2022
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.

map doesn't work for empty values Map fields with empty string as key are handled incorrectly
5 participants