fix: the common resources are not targets for lookup (#650)

The five common resources of Google Cloud are file-level options that are rendered separately from the rest of the APIs resources.
googleapis · Oct 9, 2020 · 8e1b384 · 8e1b384
1 parent 8abbc52
commit 8e1b384
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 21 deletions.
diff --git a/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 b/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
@@ -141,7 +141,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
         m = re.match(r"{{ message.path_regex_str }}", path)
         return m.groupdict() if m else {}
     {% endfor %}
-    {% for resource_msg in service.common_resources|sort(attribute="type_name") -%}
+    {% for resource_msg in service.common_resources.values()|sort(attribute="type_name") -%}
     @staticmethod
     def common_{{ resource_msg.message_type.resource_type|snake_case }}_path({% for arg in resource_msg.message_type.resource_path_args %}{{ arg }}: str, {%endfor %}) -> str:
         """Return a fully-qualified {{ resource_msg.message_type.resource_type|snake_case }} string."""

diff --git a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
@@ -738,7 +738,7 @@ def test_parse_{{ message.resource_type|snake_case }}_path():
     assert expected == actual
 
 {% endfor -%}
-{% for resource_msg in service.common_resources -%}
+{% for resource_msg in service.common_resources.values()|sort(attribute="type_name") -%}
 def test_common_{{ resource_msg.message_type.resource_type|snake_case }}_path():
     {% for arg in resource_msg.message_type.resource_path_args -%}
     {{ arg }} = "{{ molluscs.next() }}"

diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py
@@ -909,29 +909,29 @@ class Service:
         default_factory=metadata.Metadata,
     )
 
-    common_resources: ClassVar[Sequence[CommonResource]] = dataclasses.field(
-        default=(
-            CommonResource(
+    common_resources: ClassVar[Mapping[str, CommonResource]] = dataclasses.field(
+        default={
+            "cloudresourcemanager.googleapis.com/Project": CommonResource(
                 "cloudresourcemanager.googleapis.com/Project",
                 "projects/{project}",
             ),
-            CommonResource(
+            "cloudresourcemanager.googleapis.com/Organization": CommonResource(
                 "cloudresourcemanager.googleapis.com/Organization",
                 "organizations/{organization}",
             ),
-            CommonResource(
+            "cloudresourcemanager.googleapis.com/Folder": CommonResource(
                 "cloudresourcemanager.googleapis.com/Folder",
                 "folders/{folder}",
             ),
-            CommonResource(
+            "cloudbilling.googleapis.com/BillingAccount": CommonResource(
                 "cloudbilling.googleapis.com/BillingAccount",
                 "billingAccounts/{billing_account}",
             ),
-            CommonResource(
+            "locations.googleapis.com/Location": CommonResource(
                 "locations.googleapis.com/Location",
                 "projects/{project}/locations/{location}",
             ),
-        ),
+        },
         init=False,
         compare=False,
     )
@@ -1051,7 +1051,10 @@ def gen_indirect_resources_used(message):
                 resource = field.options.Extensions[
                     resource_pb2.resource_reference]
                 resource_type = resource.type or resource.child_type
-                if resource_type:
+                # The common resources are defined (and rendered) explicitly
+                # by separate logic, and the resource definitions are never
+                # visible in any of the APIs file descriptor protos.
+                if resource_type and resource_type not in self.common_resources:
                     yield self.visible_resources[resource_type]
 
         return frozenset(

diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2
@@ -42,7 +42,7 @@ class {{ service.async_client_name }}:
     {{ message.resource_type|snake_case }}_path = staticmethod({{ service.client_name }}.{{ message.resource_type|snake_case }}_path)
     parse_{{ message.resource_type|snake_case}}_path = staticmethod({{ service.client_name }}.parse_{{ message.resource_type|snake_case }}_path)
     {% endfor %}
-    {% for resource_msg in service.common_resources %}
+    {% for resource_msg in service.common_resources.values()|sort(attribute="type_name") %}
     common_{{ resource_msg.message_type.resource_type|snake_case }}_path = staticmethod({{ service.client_name }}.common_{{ resource_msg.message_type.resource_type|snake_case }}_path)
     parse_common_{{ resource_msg.message_type.resource_type|snake_case }}_path = staticmethod({{ service.client_name }}.parse_common_{{ resource_msg.message_type.resource_type|snake_case }}_path)
     {% endfor %}

diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
@@ -148,7 +148,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
         return m.groupdict() if m else {}
 
     {% endfor %} {# resources #}
-    {% for resource_msg in service.common_resources|sort(attribute="type_name") -%}
+    {% for resource_msg in service.common_resources.values()|sort(attribute="type_name") -%}
     @staticmethod
     def common_{{ resource_msg.message_type.resource_type|snake_case }}_path({% for arg in resource_msg.message_type.resource_path_args %}{{ arg }}: str, {%endfor %}) -> str:
         """Return a fully-qualified {{ resource_msg.message_type.resource_type|snake_case }} string."""

diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
@@ -1328,7 +1328,7 @@ def test_parse_{{ message.resource_type|snake_case }}_path():
     assert expected == actual
 
 {% endfor -%}
-{% for resource_msg in service.common_resources -%}
+{% for resource_msg in service.common_resources.values()|sort(attribute="type_name") -%}
 def test_common_{{ resource_msg.message_type.resource_type|snake_case }}_path():
     {% for arg in resource_msg.message_type.resource_path_args -%}
     {{ arg }} = "{{ molluscs.next() }}"

diff --git a/tests/unit/schema/wrappers/test_service.py b/tests/unit/schema/wrappers/test_service.py
@@ -311,28 +311,28 @@ def test_has_pagers():
 def test_default_common_resources():
     service = make_service(name="MolluscMaker")
 
-    assert service.common_resources == (
-        CommonResource(
+    assert service.common_resources == {
+        "cloudresourcemanager.googleapis.com/Project": CommonResource(
             "cloudresourcemanager.googleapis.com/Project",
             "projects/{project}",
         ),
-        CommonResource(
+        "cloudresourcemanager.googleapis.com/Organization": CommonResource(
             "cloudresourcemanager.googleapis.com/Organization",
             "organizations/{organization}",
         ),
-        CommonResource(
+        "cloudresourcemanager.googleapis.com/Folder": CommonResource(
             "cloudresourcemanager.googleapis.com/Folder",
             "folders/{folder}",
         ),
-        CommonResource(
+        "cloudbilling.googleapis.com/BillingAccount": CommonResource(
             "cloudbilling.googleapis.com/BillingAccount",
             "billingAccounts/{billing_account}",
         ),
-        CommonResource(
+        "locations.googleapis.com/Location": CommonResource(
             "locations.googleapis.com/Location",
             "projects/{project}/locations/{location}",
         ),
-    )
+    }
 
 
 def test_common_resource_patterns():