Ensure schemas are not duplicated

It can happen (in Keystone it does for sure) that certain API method
decorators when applied together result in certain schemas being present
in multiple decorator processing iterations. This results in some
schemas being multiplied. It should be safe to just use `set` instead of
a `list` to deduplicate them.

While we were working on this Nova "again" changed some decorators
breaking us so fix that as well since generated code is broken.

Change-Id: I25b2506264d6027f9d605c74297e6f0cc6ab2767
Signed-off-by: Artem Goncharov <artem.goncharov@gmail.com>

codegenerator/openapi/base.py

@@ -16,7 +16,6 @@ import datetime
 import enum
 import importlib
 import inspect
-import itertools
 import jsonref
 import logging
 from pathlib import Path
@@ -48,6 +47,25 @@ class Unset:
         return False
 
 
+class QueryParamsSchema:
+    schema: dict[Any, Any] | None = None
+    min_version: str | None = None
+    max_version: str | None = None
+
+    def __init__(
+        self,
+        schema: dict[Any, Any] | None,
+        min_version: str | None,
+        max_version: str | None,
+    ):
+        self.schema = schema
+        self.min_version = min_version
+        self.max_version = max_version
+
+    def __hash(self):
+        return hash((self.min_version, self.max_version))
+
+
 UNSET: Unset = Unset()
 
 
@@ -464,6 +482,7 @@ class OpenStackServerSourceBase:
                 )
         elif action and hasattr(contr, action):
             # Normal REST operation without version bounds
+            (start_version, end_version) = (None, None)
             func = getattr(contr, action)
 
             # Get the path/op spec only when we have
@@ -477,6 +496,11 @@ class OpenStackServerSourceBase:
             operation_spec.tags.extend(operation_tags)
             operation_spec.tags = list(set(operation_spec.tags))
 
+            closure_vars = inspect.getclosurevars(func)
+            if closure_vars and closure_vars.nonlocals:
+                start_version = closure_vars.nonlocals.get("min_version")
+                end_version = closure_vars.nonlocals.get("max_version")
+
             self.process_operation(
                 func,
                 openapi_spec,
@@ -486,6 +510,8 @@ class OpenStackServerSourceBase:
                 operation_name=action,
                 method=method,
                 path=path,
+                start_version=start_version,
+                end_version=end_version,
             )
         elif action != "action" and action in controller_actions:
             # Normal REST operation without version bounds and present in
@@ -718,8 +744,8 @@ class OpenStackServerSourceBase:
             )
 
         action_name = None
-        query_params_versions = []
+        query_params_versions: set[QueryParamsSchema] = set()
-        body_schemas: list[str | None] | Unset = UNSET
+        body_schemas: set[str | None] | Unset = UNSET
         expected_errors = ["404"]
         response_code = None
         # Version bound on an operation are set only when it is not an
@@ -813,9 +839,9 @@ class OpenStackServerSourceBase:
                     schema_name, TypeSchema(**body_schema)
                 )
                 if body_schemas is UNSET:
-                    body_schemas = []
+                    body_schemas = set()
-                if isinstance(body_schemas, list):
+                if isinstance(body_schemas, set):
-                    body_schemas.append(f"#/components/schemas/{schema_name}")
+                    body_schemas.add(f"#/components/schemas/{schema_name}")
             rsp_spec = getattr(fdef, "return_type", None)
             if rsp_spec:
                 ser_schema = _convert_wsme_to_jsonschema(rsp_spec)
@@ -839,18 +865,20 @@ class OpenStackServerSourceBase:
             so = sorted(
                 query_params_versions,
                 key=lambda d: (
-                    tuple(map(int, d[1].split("."))) if d[1] else (0, 0)
+                    tuple(map(int, d.min_version.split(".")))
+                    if d.min_version
+                    else (0, 0)
                 ),
             )
-            for data, min_ver, max_ver in so:
+            for item in so:
-                if data:
+                if item.schema:
                     self.process_query_parameters(
                         openapi_spec,
                         operation_spec,
                         path_resource_names,
-                        data,
+                        item.schema,
-                        min_ver,
+                        item.min_version,
-                        max_ver,
+                        item.max_version,
                     )
         # if body_schemas or mode == "action":
         if method in ["PUT", "POST", "PATCH"]:
@@ -1070,7 +1098,7 @@ class OpenStackServerSourceBase:
                     schema_ref = f"#/components/schemas/{cont_schema_name}"
                 else:
                     # otherwise just use schema as body
-                    schema_ref = body_schemas[0]
+                    schema_ref = body_schemas.pop()
         elif body_schemas is not UNSET and len(body_schemas) > 1:
             # We may end up here multiple times if we have versioned operation. In this case merge to what we have already
             op_body = operation_spec.requestBody.setdefault("content", {})
@@ -1198,11 +1226,19 @@ class OpenStackServerSourceBase:
         ]:
             if not schema.openstack:
                 schema.openstack = {}
-            schema.openstack["min-ver"] = start_version.get_string()
+            schema.openstack["min-ver"] = (
+                start_version.get_string()
+                if hasattr(start_version, "get_string")
+                else start_version
+            )
         if end_version and self._api_ver_major(end_version) not in ["0", 0]:
             if not schema.openstack:
                 schema.openstack = {}
-            schema.openstack["max-ver"] = end_version.get_string()
+            schema.openstack["max-ver"] = (
+                end_version.get_string()
+                if hasattr(end_version, "get_string")
+                else end_version
+            )
         return schema
 
     def _get_param_ref(
@@ -1341,32 +1377,48 @@ class OpenStackServerSourceBase:
         end_version,
         action_name: str | None = None,
         operation_name: str | None = None,
-    ):
+    ) -> tuple[
+        set[QueryParamsSchema],
+        set[str | None] | Unset,
+        dict | Unset | None,
+        list[str],
+    ]:
         """Extract schemas from the decorated method."""
         # Unwrap operation decorators to access all properties
         expected_errors: list[str] = []
-        body_schemas: list[str | None] | Unset = UNSET
+        body_schemas: set[str | None] | Unset = UNSET
-        query_params_versions: list[tuple] = []
+        query_params_versions: set[QueryParamsSchema] = set()
         response_body_schema: dict | Unset | None = UNSET
 
         f = func
         while hasattr(f, "__wrapped__"):
             closure = inspect.getclosurevars(f)
             closure_locals = closure.nonlocals
-            min_ver = closure_locals.get("min_version", start_version)
+            min_ver = (
+                closure_locals.get("min_version", start_version)
+                or start_version
+            )
+
             if min_ver and not isinstance(min_ver, str):
                 min_ver = (
                     min_ver.get_string()
                     if hasattr(min_ver, "get_string")
                     else str(min_ver)
                 )
-            max_ver = closure_locals.get("max_version", end_version)
+            if min_ver and not start_version:
+                start_version = min_ver
+
+            max_ver = (
+                closure_locals.get("max_version", end_version) or end_version
+            )
             if max_ver and not isinstance(max_ver, str):
                 max_ver = (
                     max_ver.get_string()
                     if hasattr(max_ver, "get_string")
                     else str(max_ver)
                 )
+            if max_ver and not end_version:
+                end_version = max_ver
 
             if "errors" in closure_locals:
                 expected_errors = closure_locals["errors"]
@@ -1389,11 +1441,12 @@ class OpenStackServerSourceBase:
                 )
                 # body schemas are not UNSET anymore
                 if body_schemas is UNSET:
-                    body_schemas = []
+                    body_schemas = set()
                 if obj is not None:
                     if obj.get("type") in ["object", "array"]:
                         # We only allow object and array bodies
-                        # To prevent type name collision keep module name part of the name
+                        # To prevent type name collision keep module name
+                        # part of the name
                         typ_name = (
                             "".join([x.title() for x in path_resource_names])
                             + func.__name__.title()
@@ -1428,12 +1481,12 @@ class OpenStackServerSourceBase:
                             comp_schema.openstack["action-name"] = action_name
 
                         ref_name = f"#/components/schemas/{typ_name}"
-                        if isinstance(body_schemas, list):
+                        if isinstance(body_schemas, set):
-                            body_schemas.append(ref_name)
+                            body_schemas.add(ref_name)
                 else:
                     # register no-body
-                    if isinstance(body_schemas, list):
+                    if isinstance(body_schemas, set):
-                        body_schemas.append(None)
+                        body_schemas.add(None)
 
             if "response_body_schema" in closure_locals or hasattr(
                 f, "_response_body_schema"
@@ -1451,17 +1504,21 @@ class OpenStackServerSourceBase:
                     "query_params_schema",
                     getattr(f, "_request_query_schema", {}),
                 )
-                query_params_versions.append((obj, min_ver, max_ver))
+                query_params_versions.add(
+                    QueryParamsSchema(obj, min_ver, max_ver)
+                )
             elif "request_query_schema" in closure_locals:
                 # Nova altered the decorator
                 obj = closure_locals.get(
                     "request_query_schema",
                     getattr(f, "request_query_schema", {}),
                 )
-                query_params_versions.append((obj, min_ver, max_ver))
+                query_params_versions.add(
+                    QueryParamsSchema(obj, min_ver, max_ver)
+                )
             if "validators" in closure_locals:
                 validators = closure_locals.get("validators")
-                body_schemas = []
+                body_schemas = set()
                 if isinstance(validators, dict):
                     for k, v in validators.items():
                         sig = inspect.signature(v)
@@ -1498,8 +1555,9 @@ class OpenStackServerSourceBase:
                                 )
 
                                 ref_name = f"#/components/schemas/{typ_name}"
-                                if isinstance(body_schemas, list):
+
-                                    body_schemas.append(ref_name)
+                                if isinstance(body_schemas, set):
+                                    body_schemas.add(ref_name)
 
             f = f.__wrapped__
 

codegenerator/openapi/cinder.py

@@ -66,13 +66,22 @@ class CinderV3Generator(OpenStackServerSourceBase):
     ]
 
     def _api_ver_major(self, ver):
-        return ver._ver_major
+        if hasattr(ver, "_ver_major"):
+            return ver._ver_major
+        elif isinstance(ver, str) and "." in ver:
+            return ver.split(".")[0]
 
     def _api_ver_minor(self, ver):
-        return ver._ver_minor
+        if hasattr(ver, "_ver_minor"):
+            return ver._ver_minor
+        elif isinstance(ver, str) and "." in ver:
+            return ver.split(".")[1]
 
     def _api_ver(self, ver):
-        return (ver._ver_major, ver._ver_minor)
+        if hasattr(ver, "_ver_major") and hasattr(ver, "_ver_minor"):
+            return (ver._ver_major, ver._ver_minor)
+        elif isinstance(ver, str):
+            return tuple(int(x) for x in ver.split("."))
 
     def generate(self, target_dir, args):
         proc = Process(target=self._generate, args=[target_dir, args])

codegenerator/openapi/keystone.py

@@ -22,6 +22,8 @@ from codegenerator.common.schema import PathSchema
 from codegenerator.common.schema import SpecSchema
 from codegenerator.common.schema import TypeSchema
 from codegenerator.openapi.base import OpenStackServerSourceBase
+from codegenerator.openapi.base import QueryParamsSchema
+from codegenerator.openapi.base import Unset
 from codegenerator.openapi.keystone_schemas import application_credential
 from codegenerator.openapi.keystone_schemas import auth
 from codegenerator.openapi.keystone_schemas import common
@@ -378,13 +380,13 @@ class KeystoneGenerator(OpenStackServerSourceBase):
             doc = rst_to_md(doc)
             operation_spec.description = LiteralScalarString(doc)
 
-        query_params_versions = []
+        query_params_versions: set[QueryParamsSchema] = set()
-        body_schemas = []
+        body_schemas: set[str | None] | Unset | None = set()
         expected_errors = ["404"]
         response_code = None
         start_version = None
         end_version = None
-        ser_schema: dict | None = {}
+        ser_schema: dict | None | Unset = {}
 
         (query_params_versions, body_schemas, ser_schema, expected_errors) = (
             self._process_decorators(
@@ -403,18 +405,20 @@ class KeystoneGenerator(OpenStackServerSourceBase):
             so = sorted(
                 query_params_versions,
                 key=lambda d: (
-                    tuple(map(int, d[1].split("."))) if d[1] else (0, 0)
+                    tuple(map(int, d.min_version.split(".")))
+                    if d.min_version
+                    else (0, 0)
                 ),
             )
-            for data, min_ver, max_ver in so:
+            for item in so:
-                if data:
+                if item.schema:
                     self.process_query_parameters(
                         openapi_spec,
                         operation_spec,
                         path_resource_names,
-                        data,
+                        item.schema,
-                        min_ver,
+                        item.min_version,
-                        max_ver,
+                        item.max_version,
                     )
 
         if method in ["PUT", "POST", "PATCH"]:

codegenerator/openapi/nova.py

@@ -38,13 +38,22 @@ class NovaGenerator(OpenStackServerSourceBase):
     }
 
     def _api_ver_major(self, ver):
-        return ver.ver_major
+        if hasattr(ver, "ver_major"):
+            return ver.ver_major
+        elif isinstance(ver, str) and "." in ver:
+            return ver.split(".")[0]
 
     def _api_ver_minor(self, ver):
-        return ver.ver_minor
+        if hasattr(ver, "ver_minor"):
+            return ver.ver_minor
+        elif isinstance(ver, str) and "." in ver:
+            return ver.split(".")[1]
 
     def _api_ver(self, ver):
-        return (ver.ver_major, ver.ver_minor)
+        if hasattr(ver, "ver_major") and hasattr(ver, "ver_minor"):
+            return (ver.ver_major, ver.ver_minor)
+        elif isinstance(ver, str):
+            return tuple(int(x) for x in ver.split("."))
 
     def _generate(self, target_dir, args):
         from nova.api.openstack import api_version_request

codegenerator/openapi/octavia.py

@@ -20,10 +20,9 @@ import fixtures
 
 from codegenerator.common.schema import SpecSchema
 from codegenerator.common.schema import TypeSchema
-from codegenerator.openapi.base import (
+from codegenerator.openapi.base import _convert_wsme_to_jsonschema
-    OpenStackServerSourceBase,
+from codegenerator.openapi.base import OpenStackServerSourceBase
-    _convert_wsme_to_jsonschema,
+from codegenerator.openapi.base import QueryParamsSchema
-)
 from codegenerator.openapi.utils import merge_api_ref_doc
 
 from ruamel.yaml.scalarstring import LiteralScalarString
@@ -1177,7 +1176,7 @@ class OctaviaGenerator(OpenStackServerSourceBase):
             }
 
         if qp:
-            query_params_versions.append((qp, None, None))
+            query_params_versions.add(QueryParamsSchema(qp, None, None))
 
         return (
             query_params_versions,

zuul.d/project.yaml

@@ -12,6 +12,7 @@
         - codegenerator-openapi-container-infrastructure-management-tips-with-api-ref
         - codegenerator-openapi-dns-tips-with-api-ref
         - codegenerator-openapi-identity-tips-with-api-ref
+        - codegenerator-openapi-identity-tips
         - codegenerator-openapi-image-tips-with-api-ref
         - codegenerator-openapi-key-manager-tips
         - codegenerator-openapi-load-balancing-tips-with-api-ref