Merge pull request #4687 from acozzette/js-map-parsing-fix

Fixed JS parsing of unspecified map keys

4 files changed, 87 insertions, 6 deletions

diff --git a/js/map.js b/js/map.js
index 7b5b2c38..2fb14837 100644
--- a/js/map.js
+++ b/js/map.js
@@ -443,7 +443,8 @@ jspb.Map.prototype.serializeBinary = function(
 /**
  * Read one key/value message from the given BinaryReader. Compatible as the
  * `reader` callback parameter to jspb.BinaryReader.readMessage, to be called
- * when a key/value pair submessage is encountered.
+ * when a key/value pair submessage is encountered. If the Key is undefined,
+ * we should default it to 0.
  * @template K, V
  * @param {!jspb.Map} map
  * @param {!jspb.BinaryReader} reader
@@ -457,12 +458,17 @@ jspb.Map.prototype.serializeBinary = function(
  *    readMessage, in which case the second callback arg form is used.
  *
  * @param {?function(V,!jspb.BinaryReader)=} opt_valueReaderCallback
- *    The BinaryReader parsing callback for type V, if V is a message type.
+ *    The BinaryReader parsing callback for type V, if V is a message type
+ *
+ * @param {K=} opt_defaultKey
+ *    The default value for the type of map keys. Accepting map
+ *    entries with unset keys is required for maps to be backwards compatible
+ *    with the repeated message representation described here: goo.gl/zuoLAC
  *
  */
 jspb.Map.deserializeBinary = function(map, reader, keyReaderFn, valueReaderFn,
-                                      opt_valueReaderCallback) {
-  var key = undefined;
+                                      opt_valueReaderCallback, opt_defaultKey) {
+  var key = opt_defaultKey;
   var value = undefined;
 
   while (reader.nextField()) {
@@ -470,6 +476,7 @@ jspb.Map.deserializeBinary = function(map, reader, keyReaderFn, valueReaderFn,
       break;
     }
     var field = reader.getFieldNumber();
+
     if (field == 1) {
       // Key.
       key = keyReaderFn.call(reader);
diff --git a/js/maps_test.js b/js/maps_test.js
index e8dd2f21..e496f446 100755
--- a/js/maps_test.js
+++ b/js/maps_test.js
@@ -35,6 +35,11 @@ goog.require('goog.userAgent');
 goog.require('proto.jspb.test.MapValueEnum');
 goog.require('proto.jspb.test.MapValueMessage');
 goog.require('proto.jspb.test.TestMapFields');
+goog.require('proto.jspb.test.TestMapFieldsOptionalKeys');
+goog.require('proto.jspb.test.MapEntryOptionalKeysStringKey');
+goog.require('proto.jspb.test.MapEntryOptionalKeysInt32Key');
+goog.require('proto.jspb.test.MapEntryOptionalKeysInt64Key');
+goog.require('proto.jspb.test.MapEntryOptionalKeysBoolKey');
 
 // CommonJS-LoadFromFile: test_pb proto.jspb.test
 goog.require('proto.jspb.test.MapValueMessageNoBinary');
@@ -76,7 +81,7 @@ function toArray(iter) {
  * Helper: generate test methods for this TestMapFields class.
  * @param {?} msgInfo
  * @param {?} submessageCtor
- * @param {!string} suffix
+ * @param {string} suffix
  */
 function makeTests(msgInfo, submessageCtor, suffix) {
   /**
@@ -260,6 +265,39 @@ function makeTests(msgInfo, submessageCtor, suffix) {
       var decoded = msgInfo.deserializeBinary(serialized);
       checkMapFields(decoded);
     });
+    /**
+     * Tests deserialization of undefined map keys go to default values in binary format.
+     */
+    it('testMapDeserializationForUndefinedKeys', function() {
+      var testMessageOptionalKeys = new proto.jspb.test.TestMapFieldsOptionalKeys();
+      var mapEntryStringKey = new proto.jspb.test.MapEntryOptionalKeysStringKey();
+      mapEntryStringKey.setValue("a");
+      testMessageOptionalKeys.setMapStringString(mapEntryStringKey);
+      var mapEntryInt32Key = new proto.jspb.test.MapEntryOptionalKeysInt32Key();
+      mapEntryInt32Key.setValue("b");
+      testMessageOptionalKeys.setMapInt32String(mapEntryInt32Key);
+      var mapEntryInt64Key = new proto.jspb.test.MapEntryOptionalKeysInt64Key();
+      mapEntryInt64Key.setValue("c");
+      testMessageOptionalKeys.setMapInt64String(mapEntryInt64Key);
+      var mapEntryBoolKey = new proto.jspb.test.MapEntryOptionalKeysBoolKey();
+      mapEntryBoolKey.setValue("d");
+      testMessageOptionalKeys.setMapBoolString(mapEntryBoolKey);
+      var deserializedMessage = msgInfo.deserializeBinary(
+        testMessageOptionalKeys.serializeBinary()
+       );
+      checkMapEquals(deserializedMessage.getMapStringStringMap(), [
+        ['', 'a']
+      ]);
+      checkMapEquals(deserializedMessage.getMapInt32StringMap(), [
+        [0, 'b']
+      ]);
+      checkMapEquals(deserializedMessage.getMapInt64StringMap(), [
+        [0, 'c']
+      ]);
+      checkMapEquals(deserializedMessage.getMapBoolStringMap(), [
+        [false, 'd']
+      ]);
+    });
   }
 
 
diff --git a/js/testbinary.proto b/js/testbinary.proto
index 116f17fb..ed5bdfc6 100644
--- a/js/testbinary.proto
+++ b/js/testbinary.proto
@@ -201,6 +201,38 @@ message TestMapFields {
   map<string, TestMapFields> map_string_testmapfields = 12;
 }
 
+// These proto are 'mock map' entries to test the above map deserializing with
+// undefined keys. Make sure TestMapFieldsOptionalKeys is written to be
+// deserialized by TestMapFields
+message MapEntryOptionalKeysStringKey {
+  optional string key = 1;
+  optional string value = 2;
+}
+
+message MapEntryOptionalKeysInt32Key {
+  optional int32 key = 1;
+  optional string value = 2;
+}
+
+message MapEntryOptionalKeysInt64Key {
+  optional int64 key = 1;
+  optional string value = 2;
+}
+
+message MapEntryOptionalKeysBoolKey {
+  optional bool key = 1;
+  optional string value = 2;
+}
+
+message TestMapFieldsOptionalKeys {
+  optional MapEntryOptionalKeysStringKey map_string_string = 1;
+  optional MapEntryOptionalKeysInt32Key map_int32_string= 8;
+  optional MapEntryOptionalKeysInt64Key map_int64_string = 9;
+  optional MapEntryOptionalKeysBoolKey map_bool_string = 10;
+}
+
+// End mock-map entries
+
 enum MapValueEnum {
   MAP_VALUE_FOO = 0;
   MAP_VALUE_BAR = 1;
diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc
index d8282e40..b5771f26 100755
--- a/src/google/protobuf/compiler/js/js_generator.cc
+++ b/src/google/protobuf/compiler/js/js_generator.cc
@@ -2962,8 +2962,12 @@ void Generator::GenerateClassDeserializeBinaryField(
     if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) {
       printer->Print(", $messageType$.deserializeBinaryFromReader",
           "messageType", GetMessagePath(options, value_field->message_type()));
+    } else {
+      printer->Print(", null");
     }
-
+    printer->Print(", $defaultKey$",
+          "defaultKey", JSFieldDefault(key_field)
+    );
     printer->Print(");\n");
     printer->Print("         });\n");
   } else {