Repository: googleapis/gapic-generator-python

Issue:
Map field message definitions do not consistently use module alias disambiguation
This is visible with Bigtable Admin. The generation produces output and generates no errors, but the generated unit tests fail.

```
    from .bigtable_instance_admin import (CreateInstanceRequest, GetInstanceRequest, ListInstancesRequest, ListInstancesResponse, PartialUpdateInstanceRequest, DeleteInstanceRequest, CreateClusterRequest, GetClusterRequest, ListClustersRequest, ListClustersResponse, DeleteClusterRequest, CreateInstanceMetadata, UpdateInstanceMetadata, CreateClusterMetadata, UpdateClusterMetadata, CreateAppProfileRequest, GetAppProfileRequest, ListAppProfilesRequest, ListAppProfilesResponse, UpdateAppProfileRequest, DeleteAppProfileRequest, UpdateAppProfileMetadata, )
google/cloud/bigtable_admin_v2/types/bigtable_instance_admin.py:55: in <module>
    class CreateInstanceRequest(proto.Message):
google/cloud/bigtable_admin_v2/types/bigtable_instance_admin.py:88: in CreateInstanceRequest
    message=instance.Cluster,
E   AttributeError: 'Field' object has no attribute 'Cluster
```

In the above stacktrace, `message=instance.Cluster` should be `message=gba_instance.Cluster`.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=h1) Report
> Merging [#654](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=desc) into [master](https://codecov.io/gh/googleapis/gapic-generator-python/commit/d7829efe207a8730321279e594429e3bdbf2e080?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654/graphs/tree.svg?width=650&height=150&src=pr&token=rvXPSpOilG)](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master      #654   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         1562      1563    +1     
  Branches       316       316           
=========================================
+ Hits          1562      1563    +1     
```


| [Impacted Files](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [gapic/schema/metadata.py](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654/diff?src=pr&el=tree#diff-Z2FwaWMvc2NoZW1hL21ldGFkYXRhLnB5) | `100.00% <ø> (ø)` | |
| [gapic/schema/wrappers.py](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654/diff?src=pr&el=tree#diff-Z2FwaWMvc2NoZW1hL3dyYXBwZXJzLnB5) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=footer). Last update [d7829ef...a221bb9](https://codecov.io/gh/googleapis/gapic-generator-python/pull/654?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/gapic/schema/metadata.py b/gapic/schema/metadata.py
index 9459bb5e..b801bb76 100644
--- a/gapic/schema/metadata.py
+++ b/gapic/schema/metadata.py
@@ -90,6 +90,20 @@ class Address:
         # Return the Python identifier.
         return '.'.join(self.parent + (self.name,))
 
+    def __repr__(self) -> str:
+        return "({})".format(
+            ", ".join(
+                (
+                    self.name,
+                    self.module,
+                    str(self.module_path),
+                    str(self.package),
+                    str(self.parent),
+                    str(self.api_naming),
+                )
+            )
+        )
+
     @property
     def module_alias(self) -> str:
         """Return an appropriate module alias if necessary.
diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py
index e68b397e..1b0db83e 100644
--- a/gapic/schema/wrappers.py
+++ b/gapic/schema/wrappers.py
@@ -222,7 +222,12 @@ class Field:
         raise TypeError(f'Unrecognized protobuf type: {self.field_pb.type}. '
                         'This code should not be reachable; please file a bug.')
 
-    def with_context(self, *, collisions: FrozenSet[str]) -> 'Field':
+    def with_context(
+            self,
+            *,
+            collisions: FrozenSet[str],
+            visited_messages: FrozenSet["MessageType"],
+    ) -> 'Field':
         """Return a derivative of this field with the provided context.
 
         This method is used to address naming collisions. The returned
@@ -233,7 +238,8 @@ class Field:
             self,
             message=self.message.with_context(
                 collisions=collisions,
-                skip_fields=True,
+                skip_fields=self.message in visited_messages,
+                visited_messages=visited_messages,
             ) if self.message else None,
             enum=self.enum.with_context(collisions=collisions)
             if self.enum else None,
@@ -406,7 +412,10 @@ class MessageType:
 
         # Base case: If this is the last field in the path, return it outright.
         if len(field_path) == 1:
-            return cursor.with_context(collisions=collisions)
+            return cursor.with_context(
+                collisions=collisions,
+                visited_messages=frozenset({self}),
+            )
 
         # Sanity check: If cursor is a repeated field, then raise an exception.
         # Repeated fields are only permitted in the terminal position.
@@ -433,6 +442,7 @@ class MessageType:
     def with_context(self, *,
                      collisions: FrozenSet[str],
                      skip_fields: bool = False,
+                     visited_messages: FrozenSet["MessageType"] = frozenset(),
                      ) -> 'MessageType':
         """Return a derivative of this message with the provided context.
 
@@ -444,10 +454,14 @@ class MessageType:
         underlying fields. This provides for an "exit" in the case of circular
         references.
         """
+        visited_messages = visited_messages | {self}
         return dataclasses.replace(
             self,
             fields=collections.OrderedDict(
-                (k, v.with_context(collisions=collisions))
+                (k, v.with_context(
+                    collisions=collisions,
+                    visited_messages=visited_messages
+                ))
                 for k, v in self.fields.items()
             ) if not skip_fields else self.fields,
             nested_enums=collections.OrderedDict(
@@ -457,7 +471,9 @@ class MessageType:
             nested_messages=collections.OrderedDict(
                 (k, v.with_context(
                     collisions=collisions,
-                    skip_fields=skip_fields,))
+                    skip_fields=skip_fields,
+                    visited_messages=visited_messages,
+                ))
                 for k, v in self.nested_messages.items()),
             meta=self.meta.with_context(collisions=collisions),
         )


Test patch:
diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py
index c52c2f32..e91a310e 100644
--- a/tests/unit/schema/test_api.py
+++ b/tests/unit/schema/test_api.py
@@ -1214,3 +1214,76 @@ def test_resources_referenced_but_not_typed(reference_attr="type"):
 
 def test_resources_referenced_but_not_typed_child_type():
     test_resources_referenced_but_not_typed("child_type")
+
+
+def test_map_field_name_disambiguation():
+    squid_file_pb = descriptor_pb2.FileDescriptorProto(
+        name="mollusc.proto",
+        package="animalia.mollusca.v2",
+        message_type=(
+            descriptor_pb2.DescriptorProto(
+                name="Mollusc",
+            ),
+        ),
+    )
+    method_types_file_pb = descriptor_pb2.FileDescriptorProto(
+        name="mollusc_service.proto",
+        package="animalia.mollusca.v2",
+        message_type=(
+            descriptor_pb2.DescriptorProto(
+                name="CreateMolluscRequest",
+                field=(
+                    descriptor_pb2.FieldDescriptorProto(
+                        name="mollusc",
+                        type="TYPE_MESSAGE",
+                        type_name=".animalia.mollusca.v2.Mollusc",
+                        number=1,
+                    ),
+                    descriptor_pb2.FieldDescriptorProto(
+                        name="molluscs_map",
+                        type="TYPE_MESSAGE",
+                        number=2,
+                        type_name=".animalia.mollusca.v2.CreateMolluscRequest.MolluscsMapEntry",
+                        label="LABEL_REPEATED",
+                    ),
+                ),
+                nested_type=(
+                    descriptor_pb2.DescriptorProto(
+                        name="MolluscsMapEntry",
+                        field=(
+                            descriptor_pb2.FieldDescriptorProto(
+                                name="key",
+                                type="TYPE_STRING",
+                                number=1,
+                            ),
+                            descriptor_pb2.FieldDescriptorProto(
+                                name="value",
+                                type="TYPE_MESSAGE",
+                                number=2,
+                                # We use the same type for the map value as for
+                                # the singleton above to better highlight the
+                                # problem raised in
+                                # https://github.com/googleapis/gapic-generator-python/issues/618.
+                                # The module _is_ disambiguated for singleton
+                                # fields but NOT for map fields.
+                                type_name=".animalia.mollusca.v2.Mollusc"
+                            ),
+                        ),
+                        options=descriptor_pb2.MessageOptions(map_entry=True),
+                    ),
+                ),
+            ),
+        ),
+    )
+    my_api = api.API.build(
+        file_descriptors=[squid_file_pb, method_types_file_pb],
+    )
+    create = my_api.messages['animalia.mollusca.v2.CreateMolluscRequest']
+    mollusc = create.fields['mollusc']
+    molluscs_map = create.fields['molluscs_map']
+    mollusc_ident = str(mollusc.type.ident)
+    mollusc_map_ident = str(molluscs_map.message.fields['value'].type.ident)
+
+    # The same module used in the same place should have the same import alias.
+    # Because there's a "mollusc" name used, the import should be disambiguated.
+    assert mollusc_ident == mollusc_map_ident == "am_mollusc.Mollusc"