Skip to content
Permalink
Browse files
fix: handling of map entries with omitted key or value (#1348)
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>
  • Loading branch information
schiffermtq and alexander-fenster committed Jul 10, 2020
1 parent 7a25398 commit b950877c86676399674821fca4cf444f046b5acb
Showing with 81 additions and 15 deletions.
  1. +37 −15 src/decoder.js
  2. +44 −0 tests/comp_maps.js
@@ -19,7 +19,7 @@ function decoder(mtype) {
var gen = util.codegen(["r", "l"], mtype.name + "$decode")
("if(!(r instanceof Reader))")
("r=Reader.create(r)")
("var c=l===undefined?r.len:r.pos+l,m=new this.ctor" + (mtype.fieldsArray.filter(function(field) { return field.map; }).length ? ",k" : ""))
("var c=l===undefined?r.len:r.pos+l,m=new this.ctor" + (mtype.fieldsArray.filter(function(field) { return field.map; }).length ? ",k,value" : ""))
("while(r.pos<c){")
("var t=r.uint32()");
if (mtype.group) gen
@@ -37,22 +37,44 @@ function decoder(mtype) {

// Map fields
if (field.map) { gen
("r.skip().pos++") // assumes id 1 + key wireType
("if(%s===util.emptyObject)", ref)
("%s={}", ref)
("k=r.%s()", field.keyType)
("r.pos++"); // assumes id 2 + value wireType
if (types.long[field.keyType] !== undefined) {
if (types.basic[type] === undefined) gen
("%s[typeof k===\"object\"?util.longToHash(k):k]=types[%i].decode(r,r.uint32())", ref, i); // can't be groups
else gen
("%s[typeof k===\"object\"?util.longToHash(k):k]=r.%s()", ref, type);
} else {
if (types.basic[type] === undefined) gen
("%s[k]=types[%i].decode(r,r.uint32())", ref, i); // can't be groups
else gen
("%s[k]=r.%s()", ref, type);
}
("var c2 = r.uint32()+r.pos");

if (types.defaults[field.keyType] !== undefined) gen
("k=%j", types.defaults[field.keyType]);
else gen
("k=null");

if (types.defaults[type] !== undefined) gen
("value=%j", types.defaults[type]);
else gen
("value=null");

gen
("while(r.pos<c2){")
("var tag2=r.uint32()")
("switch(tag2>>>3){")
("case 1: k=r.%s(); break", field.keyType)
("case 2:");

if (types.basic[type] === undefined) gen
("value=types[%i].decode(r,r.uint32())", i); // can't be groups
else gen
("value=r.%s()", type);

gen
("break")
("default:")
("r.skipType(tag2&7)")
("break")
("}")
("}");

if (types.long[field.keyType] !== undefined) gen
("%s[typeof k===\"object\"?util.longToHash(k):k]=value", ref);
else gen
("%s[k]=value", ref);

// Repeated fields
} else if (field.repeated) { gen
@@ -92,6 +92,50 @@ tape.test("maps", function(test) {
test.end();
});

test.test(test.name + " - omitted fields", function(test) {

var mapRoot = protobuf.Root.fromJSON({
nested: {
MapMessage: {
fields: {
value: {
keyType: "int32",
type: "string",
id: 1
}
}
}
}
});

var MapMessage = mapRoot.lookup("MapMessage");

var value = {
value: {
0: ''
}
};
var dec;

// 1 <chunk> = message(1 <varint> = 0, 2 <chunk> = empty chunk)
dec = MapMessage.decode(Uint8Array.of(0x0a, 0x04, 0x08, 0x00, 0x12, 0x00));
test.deepEqual(dec, value, "should correct decode the buffer without omitted fields");

// 1 <chunk> = message(1 <varint> = 0)
dec = MapMessage.decode(Uint8Array.of(0x0a, 0x02, 0x08, 0x00));
test.deepEqual(dec, value, "should correct decode the buffer with omitted value");

// 1 <chunk> = message(2 <chunk> = empty chunk)
dec = MapMessage.decode(Uint8Array.of(0x0a, 0x02, 0x12, 0x00));
test.deepEqual(dec, value, "should correct decode the buffer with omitted key");

// 1 <chunk> = empty chunk
dec = MapMessage.decode(Uint8Array.of(0x0a, 0x00));
test.deepEqual(dec, value, "should correct decode the buffer with both key and value omitted");

test.end();
});

test.end();
});

0 comments on commit b950877

Please sign in to comment.