Duplicate samples generate an error (#207)

Note: distinct samples with duplicate IDs (provided or derived) are disambigutated.
Duplicate samples, i.e. those that are the result of a copy/paste, are the only ones that generate an error.

Fix for #201

Author: software-dov

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

@@ -21,7 +21,7 @@
 from collections import (OrderedDict, defaultdict)
 from gapic.samplegen_utils.utils import (
     coerce_response_name, is_valid_sample_cfg)
-from gapic.samplegen_utils.types import InvalidConfig
+from gapic.samplegen_utils.types import (InvalidConfig, DuplicateSample)
 from gapic.samplegen import (manifest, samplegen)
 from gapic.generator import options
 from gapic.generator import formatter
@@ -118,7 +118,12 @@ def _generate_samples_and_manifest(
         Returns:
             Dict[str, CodeGeneratorResponse.File]: A dict mapping filepath to rendered file.
         """
-        id_to_samples: DefaultDict[str, List[Any]] = defaultdict(list)
+        # The two-layer data structure lets us do two things:
+        # * detect duplicate samples, which is an error
+        # * detect distinct samples with the same ID, which are disambiguated
+        id_to_hash_to_spec: DefaultDict[str, Dict[str, Any]] = defaultdict(
+            dict)
+
         for config_fpath in self._sample_configs:
             with open(config_fpath) as f:
                 configs = yaml.safe_load_all(f.read())
@@ -137,23 +142,27 @@ def _generate_samples_and_manifest(
                 #
                 # Ideally the sample author should pick a descriptive, unique ID,
                 # but this may be impractical and can be error-prone.
+                spec_hash = sha256(str(spec).encode('utf8')).hexdigest()[:8]
                 sample_id = (spec.get("id")
                              or spec.get("region_tag")
-                             or sha256(str(spec).encode('utf8')).hexdigest()[:8])
-
+                             or spec_hash)
                 spec["id"] = sample_id
-                id_to_samples[sample_id].append(spec)
+
+                hash_to_spec = id_to_hash_to_spec[sample_id]
+                if spec_hash in hash_to_spec:
+                    raise DuplicateSample(
+                        f"Duplicate samplegen spec found: {spec}")
+
+                hash_to_spec[spec_hash] = spec
 
         out_dir = "samples"
         fpath_to_spec_and_rendered = {}
-        for samples in id_to_samples.values():
-            for spec in samples:
-                id_is_unique = len(samples) == 1
+        for hash_to_spec in id_to_hash_to_spec.values():
+            for spec_hash, spec in hash_to_spec.items():
+                id_is_unique = len(hash_to_spec) == 1
                 # The ID is used to generate the file name and by sample tester
                 # to link filenames to invoked samples. It must be globally unique.
                 if not id_is_unique:
-                    spec_hash = sha256(
-                        str(spec).encode('utf8')).hexdigest()[:8]
                     spec["id"] += f"_{spec_hash}"
 
                 sample = samplegen.generate_sample(

packages/gapic-generator/gapic/samplegen_utils/types.py

@@ -80,6 +80,10 @@ class InvalidSampleFpath(SampleError):
     pass
 
 
+class DuplicateSample(SampleError):
+    pass
+
+
 class CallingForm(Enum):
     Request = auto()
     RequestPaged = auto()

packages/gapic-generator/gapic/samplegen_utils/utils.py

@@ -67,7 +67,7 @@ def parse_version(version_str: str) -> Tuple[int, ...]:
         # Yaml may return a dict, a list, or a str
         isinstance(doc, dict)
         and doc.get("type") == VALID_CONFIG_TYPE
-        and parse_version(doc.get(version_token, "")) >= MIN_SCHEMA_VERSION
+        and parse_version(doc.get(version_token, "")) >= min_version
         and doc.get("samples")
     )
 

packages/gapic-generator/tests/unit/generator/test_generator.py

@@ -433,7 +433,7 @@ def test_samplegen_id_disambiguation(mock_gmtime, mock_generate_sample, fs):
                 content="\n",
             ),
             CodeGeneratorResponse.File(
-                name="samples/squid_sample_c8014108.py",
+                name="samples/squid_sample_55051b38.py",
                 content="\n",
             ),
             CodeGeneratorResponse.File(
@@ -457,8 +457,8 @@ def test_samplegen_id_disambiguation(mock_gmtime, mock_generate_sample, fs):
                   path: '{base_path}/squid_sample_91a465c6.py'
                   region_tag: humboldt_tag
                 - <<: *python
-                  sample: squid_sample_c8014108
-                  path: '{base_path}/squid_sample_c8014108.py'
+                  sample: squid_sample_55051b38
+                  path: '{base_path}/squid_sample_55051b38.py'
                   region_tag: squid_sample
                 - <<: *python
                   sample: 157884ee
@@ -471,6 +471,34 @@ def test_samplegen_id_disambiguation(mock_gmtime, mock_generate_sample, fs):
     assert actual_response == expected_response
 
 
+def test_generator_duplicate_samples(fs):
+    config_fpath = "samples.yaml"
+    fs.create_file(
+        config_fpath,
+        contents=dedent(
+            '''
+            # Note: the samples are duplicates.
+            type: com.google.api.codegen.SampleConfigProto
+            schema_version: 1.2.0
+            samples:
+            - id: squid_sample
+              region_tag: humboldt_tag
+              rpc: get_squid
+            - id: squid_sample
+              region_tag: humboldt_tag
+              rpc: get_squid
+            '''
+        )
+    )
+
+    generator = make_generator('samples=samples.yaml')
+    generator._env.loader = jinja2.DictLoader({'sample.py.j2': ''})
+    api_schema = make_api(naming=naming.Naming(name='Mollusc', version='v6'))
+
+    with pytest.raises(types.DuplicateSample):
+        generator.get_response(api_schema=api_schema)
+
+
 def make_generator(opts_str: str = '') -> generator.Generator:
     return generator.Generator(options.Options.build(opts_str))
 

packages/gapic-generator/tests/unit/samplegen/test_integration.py

@@ -273,6 +273,7 @@ def test_generate_sample_config_partial_config(fs):
         contents=dedent(
             '''
             ---
+            # Note: not a valid config because of the type.
             type: com.google.api.codegen.SampleConfigPronto
             schema_version: 1.2.0
             samples: