fix(generator): address PR review comments for request-id centralization and clean up allowed private templates

Author: hebaalazzeh

packages/gapic-generator/gapic/generator/generator.py

@@ -37,6 +37,12 @@
 from google.protobuf.compiler.plugin_pb2 import CodeGeneratorResponse
 
 
+ALLOWED_PRIVATE_TEMPLATES = (
+    "__init__.py.j2",
+    "_compat.py.j2",
+)
+
+
 class Generator:
     """A protoc code generator for client libraries.
 
@@ -118,9 +124,9 @@ def get_response(self, api_schema: api.API, opts: Options) -> CodeGeneratorRespo
         # and instead of iterating over it/them, we iterate over samples
         # and plug those into the template.
         for template_name in client_templates:
-            # Quick check: Skip "private" templates.
+            # Quick check: Skip "private" templates except explicitly allowed ones.
             filename = template_name.split("/")[-1]
-            if filename.startswith("_") and filename not in ("__init__.py.j2", "_compat.py.j2"):
+            if filename.startswith("_") and filename not in ALLOWED_PRIVATE_TEMPLATES:
                 continue
 
             # Append to the output files dictionary.
@@ -266,9 +272,6 @@ def _render_template(
         if not opts.metadata and template_name.endswith("gapic_metadata.json.j2"):
             return answer
 
-        
-
-
         # Disables generation of an unversioned Python package for this client
         # library. This means that the module names will need to be versioned in
         # import statements. For example `import google.cloud.library_v2` instead

packages/gapic-generator/gapic/templates/%namespace/%name_%version/%sub/_compat.py.j2

@@ -142,7 +142,7 @@ except ImportError:
 
 
 try:
-    from google.api_core.gapic_v1.request import setup_request_id  # type: ignore
+    from google.api_core.gapic_v1.requests import setup_request_id  # type: ignore
 except ImportError:
     # TODO(https://github.com/googleapis/google-cloud-python/issues/17813): Remove this fallback when google-api-core >= 2.26.0 is the minimum required version.
     def setup_request_id(request, field_name: str, is_proto3_optional: bool):
@@ -153,10 +153,11 @@ except ImportError:
             field_name (str): The name of the field to populate.
             is_proto3_optional (bool): Whether the field is proto3 optional.
         """
-        request_id_val = str(uuid.uuid4())
         if request is None:
             return
 
+        request_id_val = str(uuid.uuid4())
+
         if isinstance(request, dict):
             if is_proto3_optional:
                 if field_name not in request or request[field_name] is None:
@@ -172,6 +173,13 @@ except ImportError:
                     setattr(request, field_name, request_id_val)
             except (AttributeError, ValueError):
                 # Proto-plus messages or other objects
+                if hasattr(request, "_pb"):
+                    try:
+                        if not request._pb.HasField(field_name):
+                            setattr(request, field_name, request_id_val)
+                        return
+                    except (AttributeError, ValueError):
+                        pass
                 if getattr(request, field_name, None) is None:
                     setattr(request, field_name, request_id_val)
         else:

packages/gapic-generator/noxfile.py

@@ -150,7 +150,9 @@ def __call__(self, frag):
                 constraints_path = str(
                     f"{tmp_dir}/testing/constraints-{self.session.python}.txt"
                 )
-                self.session.install(tmp_dir, "-e", ".", "-qqq", "-r", constraints_path)
+                self.session.install(tmp_dir, "-e", ".", "--no-build-isolation", "-qqq", "-r", constraints_path)
+
+            self.session.install("-e", "../google-api-core", "--no-build-isolation", "-qqq")
 
             # Run the fragment's generated unit tests.
             # Don't bother parallelizing them: we already parallelize
@@ -246,6 +248,9 @@ def showcase_library(
 
     # Install gapic-generator-python
     session.install("-e", ".")
+    # Install local editable google-api-core for monorepo development testing.
+    # On Python 3.10 below, google-api-core<2.28 is installed to validate
+    # fallback compatibility against released versions of google-api-core.
     session.install("-e", "../google-api-core")
 
     # Install grpcio-tools for protoc
@@ -375,12 +380,14 @@ def showcase_library(
                     )
                 extras = "[async_rest]"
 
-            session.install("-e", f"{tmp_dir}{extras}", "-r", constraints_path)
+            session.install("-e", f"{tmp_dir}{extras}", "--no-build-isolation", "-r", constraints_path)
         else:
             # The ads templates do not have constraints files.
             # See https://github.com/googleapis/gapic-generator-python/issues/1788
             # Install the library without a constraints file.
-            session.install("-e", tmp_dir)
+            session.install("-e", tmp_dir, "--no-build-isolation")
+
+        session.install("-e", "../google-api-core", "--no-build-isolation")
 
         yield tmp_dir
 

packages/gapic-generator/tests/integration/goldens/storagebatchoperations/google/cloud/storagebatchoperations_v1/_compat.py

@@ -17,7 +17,7 @@
 import uuid
 
 try:
-    from google.api_core.gapic_v1.request import setup_request_id  # type: ignore
+    from google.api_core.gapic_v1.requests import setup_request_id  # type: ignore
 except ImportError:
     # TODO(https://github.com/googleapis/google-cloud-python/issues/17813): Remove this fallback when google-api-core >= 2.26.0 is the minimum required version.
     def setup_request_id(request, field_name: str, is_proto3_optional: bool):
@@ -48,7 +48,14 @@ def setup_request_id(request, field_name: str, is_proto3_optional: bool):
                     setattr(request, field_name, request_id_val)
             except (AttributeError, ValueError):
                 # Proto-plus messages or other objects
-                if not getattr(request, field_name, None):
+                if hasattr(request, "_pb"):
+                    try:
+                        if not request._pb.HasField(field_name):
+                            setattr(request, field_name, request_id_val)
+                        return
+                    except (AttributeError, ValueError):
+                        pass
+                if getattr(request, field_name, None) is None:
                     setattr(request, field_name, request_id_val)
         else:
             if not getattr(request, field_name, None):

packages/gapic-generator/tests/integration/goldens/storagebatchoperations/tests/unit/gapic/storagebatchoperations_v1/test__compat.py

@@ -117,8 +117,6 @@ def test_get_response_ignores_private_files():
         list_templates.return_value = [
             "foo/bar/baz.py.j2",
             "foo/bar/_base.py.j2",
-            "foo/bar/__init__.py.j2",
-            "foo/bar/_compat.py.j2",
             "molluscs/squid/sample.py.j2",
         ]
         with mock.patch.object(jinja2.Environment, "get_template") as get_template:
@@ -130,13 +128,34 @@ def test_get_response_ignores_private_files():
             get_template.assert_has_calls(
                 [
                     mock.call("molluscs/squid/sample.py.j2"),
+                    mock.call("foo/bar/baz.py.j2"),
+                ]
+            )
+            assert len(cgr.file) == 1
+
+
+def test_get_response_renders_allowed_private_templates():
+    generator_obj = make_generator()
+    with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as list_templates:
+        list_templates.return_value = [
+            "foo/bar/__init__.py.j2",
+            "foo/bar/_compat.py.j2",
+            "foo/bar/_ignored.py.j2",
+        ]
+        with mock.patch.object(jinja2.Environment, "get_template") as get_template:
+            get_template.return_value = jinja2.Template("I am a template result.")
+            cgr = generator_obj.get_response(
+                api_schema=make_api(), opts=Options.build("")
+            )
+            list_templates.assert_called_once()
+            get_template.assert_has_calls(
+                [
                     mock.call("foo/bar/__init__.py.j2"),
                     mock.call("foo/bar/_compat.py.j2"),
-                    mock.call("foo/bar/baz.py.j2"),
                 ],
                 any_order=True,
             )
-            assert len(cgr.file) == 3
+            assert len(cgr.file) == 2
 
 
 def test_get_response_fails_invalid_file_paths():