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

@schiffermtq
Copy link
Contributor

@schiffermtq schiffermtq 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([]).

Loading

Copy link
Contributor Author

@schiffermtq schiffermtq Feb 4, 2020

Choose a reason for hiding this comment

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

Thanks, fixed.

Loading

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

@paambaati paambaati commented Feb 7, 2020

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

Loading

Copy link

@adriancable adriancable left a comment

This looks (and works) great.

Loading

@paambaati
Copy link

@paambaati paambaati commented Jul 3, 2020

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

Loading

@adriancable
Copy link

@adriancable adriancable commented Jul 3, 2020

Bump.

Loading

@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
9 checks passed
Loading
@adriancable
Copy link

@adriancable adriancable commented Jul 10, 2020

YES! Thank you so much.

Loading

taylorcode added a commit to taylorcode/protobuf.js that referenced this issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants