Merge pull request #2193 from acozzette/common-js-fix

Fixed references to foreign nested messages with CommonJS-style imports

js/message_test.js

@@ -1040,4 +1040,18 @@ describe('Message test suite', function() {
     assertNan(message.getDefaultDoubleField());
   });
 
+  // Verify that we can successfully use a field referring to a nested message
+  // from a different .proto file.
+  it('testForeignNestedMessage', function() {
+    var msg = new proto.jspb.test.ForeignNestedFieldMessage();
+    var nested = new proto.jspb.test.Deeply.Nested.Message();
+    nested.setCount(5);
+    msg.setDeeplyNestedMessage(nested);
+
+    // After a serialization-deserialization round trip we should get back the
+    // same data we started with.
+    var serialized = msg.serializeBinary();
+    var deserialized = proto.jspb.test.ForeignNestedFieldMessage.deserializeBinary(serialized);
+    assertEquals(5, deserialized.getDeeplyNestedMessage().getCount());
+  });
 });

js/test.proto

@@ -260,3 +260,11 @@ enum MapValueEnumNoBinary {
 message MapValueMessageNoBinary {
   optional int32 foo = 1;
 }
+
+message Deeply {
+  message Nested {
+    message Message {
+      optional int32 count = 1;
+    }
+  }
+}

js/test2.proto

@@ -35,6 +35,8 @@ option java_multiple_files = true;
 
 package jspb.test;
 
+import "test.proto";
+
 message TestExtensionsMessage {
   optional int32 intfield = 1;
   extensions 100 to max;
@@ -52,3 +54,7 @@ extend TestExtensionsMessage {
   optional ExtensionMessage floating_msg_field = 101;
   optional string floating_str_field = 102;
 }
+
+message ForeignNestedFieldMessage {
+  optional Deeply.Nested.Message deeply_nested_message = 1;
+}

src/google/protobuf/compiler/js/js_generator.cc

@@ -208,28 +208,28 @@ string GetPath(const GeneratorOptions& options,
   }
 }
 
-// Forward declare, so that GetPrefix can call this method,
-// which in turn, calls GetPrefix.
-string GetPath(const GeneratorOptions& options,
-               const Descriptor* descriptor);
+// Returns the name of the message with a leading dot and taking into account
+// nesting, for example ".OuterMessage.InnerMessage", or returns empty if
+// descriptor is null. This function does not handle namespacing, only message
+// nesting.
+string GetNestedMessageName(const Descriptor* descriptor) {
+  if (descriptor == NULL) {
+    return "";
+  }
+  return StripPrefixString(descriptor->full_name(),
+                           descriptor->file()->package());
+}
 
 // Returns the path prefix for a message or enumeration that
 // lives under the given file and containing type.
 string GetPrefix(const GeneratorOptions& options,
                  const FileDescriptor* file_descriptor,
                  const Descriptor* containing_type) {
-  string prefix = "";
-
-  if (containing_type == NULL) {
-    prefix = GetPath(options, file_descriptor);
-  } else {
-    prefix = GetPath(options, containing_type);
-  }
-
+  string prefix = GetPath(options, file_descriptor) +
+      GetNestedMessageName(containing_type);
   if (!prefix.empty()) {
     prefix += ".";
   }
-
   return prefix;
 }
 
@@ -277,7 +277,9 @@ string MaybeCrossFileRef(const GeneratorOptions& options,
       from_file != to_message->file()) {
     // Cross-file ref in CommonJS needs to use the module alias instead of
     // the global name.
-    return ModuleAlias(to_message->file()->name()) + "." + to_message->name();
+    return ModuleAlias(to_message->file()->name()) +
+        GetNestedMessageName(to_message->containing_type()) +
+        "." + to_message->name();
   } else {
     // Within a single file we use a full name.
     return GetPath(options, to_message);