From b4574ec2bcf7e36e02127b91138d2563639e8352 Mon Sep 17 00:00:00 2001
From: Artem Goncharov <artem.goncharov@gmail.com>
Date: Thu, 28 Nov 2024 14:39:34 +0100
Subject: [PATCH] Parse schema for validation
neither jsonschema not openapi validation are enough. In order to
improve the situation try to apply our own schema parser (ADT convertor)
to identify potential issues.
Change-Id: I607120aafe6eb3de56270e8d26ffbb0a3f99c101
---
codegenerator/metadata.py | 95 ++++++++++++++++-------------
codegenerator/openapi/base.py | 98 +++++++++++++++++++++++++++---
codegenerator/openapi/cinder.py | 4 +-
codegenerator/openapi/designate.py | 2 +-
codegenerator/openapi/glance.py | 2 +-
codegenerator/openapi/ironic.py | 4 +-
codegenerator/openapi/keystone.py | 4 +-
codegenerator/openapi/manila.py | 4 +-
codegenerator/openapi/neutron.py | 8 ++-
codegenerator/openapi/nova.py | 2 +-
codegenerator/openapi/octavia.py | 4 +-
codegenerator/openapi/placement.py | 2 +-
playbooks/openapi/run.yaml | 1 +
13 files changed, 167 insertions(+), 63 deletions(-)
diff --git a/codegenerator/metadata.py b/codegenerator/metadata.py
index 6a77f71..8047f53 100644
--- a/codegenerator/metadata.py
+++ b/codegenerator/metadata.py
@@ -61,6 +61,27 @@ class MetadataGenerator(BaseGenerator):
schema = self.load_openapi(spec_path)
openapi_spec = common.get_openapi_spec(spec_path)
+ metadata = self.build_metadata(
+ schema, openapi_spec, args.service_type, spec_path.as_posix()
+ )
+
+ yaml = YAML()
+ yaml.preserve_quotes = True
+ yaml.default_flow_style = False
+ yaml.indent(mapping=2, sequence=4, offset=2)
+ metadata_path.parent.mkdir(exist_ok=True, parents=True)
+ with open(metadata_path, "w") as fp:
+ yaml.dump(
+ metadata.model_dump(
+ exclude_none=True, exclude_defaults=True, by_alias=True
+ ),
+ fp,
+ )
+
+ @staticmethod
+ def build_metadata(
+ schema, openapi_spec, service_type: str, spec_path: str
+ ) -> Metadata:
metadata = Metadata(resources={})
api_ver = "v" + schema.info["version"].split(".")[0]
for path, spec in schema.paths.items():
@@ -68,7 +89,7 @@ class MetadataGenerator(BaseGenerator):
resource_name = "/".join(
list(common.get_resource_names_from_url(path))
)
- if args.service_type == "object-store":
+ if service_type == "object-store":
if path == "/v1/{account}":
resource_name = "account"
elif path == "/v1/{account}/{container}":
@@ -76,7 +97,7 @@ class MetadataGenerator(BaseGenerator):
if path == "/v1/{account}/{object}":
resource_name = "object"
- if args.service_type == "compute" and resource_name in [
+ if service_type == "compute" and resource_name in [
"agent",
"baremetal_node",
"cell",
@@ -111,11 +132,9 @@ class MetadataGenerator(BaseGenerator):
# We do not need to produce anything for deprecated APIs
continue
resource_model = metadata.resources.setdefault(
- f"{args.service_type}.{resource_name}",
+ f"{service_type}.{resource_name}",
ResourceModel(
- api_version=api_ver,
- spec_file=spec_path.as_posix(),
- operations={},
+ api_version=api_ver, spec_file=spec_path, operations={}
),
)
for method in [
@@ -187,44 +206,44 @@ class MetadataGenerator(BaseGenerator):
if method == "post":
operation_key = "update"
elif (
- args.service_type == "compute"
+ service_type == "compute"
and resource_name == "flavor/flavor_access"
and method == "get"
):
operation_key = "list"
elif (
- args.service_type == "compute"
+ service_type == "compute"
and resource_name == "aggregate/image"
and method == "post"
):
operation_key = "action"
elif (
- args.service_type == "compute"
+ service_type == "compute"
and resource_name == "server/security_group"
and method == "get"
):
operation_key = "list"
elif (
- args.service_type == "compute"
+ service_type == "compute"
and resource_name == "server/topology"
and method == "get"
):
operation_key = "list"
elif (
- args.service_type == "compute"
+ service_type == "compute"
and resource_name == "quota_set"
and path.endswith("defaults")
):
operation_key = "defaults"
elif (
- args.service_type == "compute"
+ service_type == "compute"
and resource_name == "quota_set"
and path.endswith("detail")
):
# normalize "details" name
operation_key = "details"
elif (
- args.service_type == "load-balancer"
+ service_type == "load-balancer"
and len(path_elements) > 1
and path_elements[-1]
in ["stats", "status", "failover", "config"]
@@ -249,22 +268,22 @@ class MetadataGenerator(BaseGenerator):
elif path.endswith("/action"):
# Action
operation_key = "action"
- elif args.service_type == "image" and path.endswith(
+ elif service_type == "image" and path.endswith(
"/actions/deactivate"
):
operation_key = "deactivate"
- elif args.service_type == "image" and path.endswith(
+ elif service_type == "image" and path.endswith(
"/actions/reactivate"
):
operation_key = "reactivate"
elif (
- args.service_type == "block-storage"
+ service_type == "block-storage"
and "volume-transfer" in path
and path.endswith("/accept")
):
operation_key = "accept"
elif (
- args.service_type == "block-storage"
+ service_type == "block-storage"
and "qos-specs" in path
and path_elements[-1]
in [
@@ -276,26 +295,26 @@ class MetadataGenerator(BaseGenerator):
):
operation_key = path_elements[-1]
elif (
- args.service_type == "network"
+ service_type == "network"
and "quota" in path
and path.endswith("/default")
):
# normalize "defaults" name
operation_key = "defaults"
elif (
- args.service_type == "network"
+ service_type == "network"
and "quota" in path
and path.endswith("/details")
):
operation_key = "details"
elif (
- args.service_type == "placement"
+ service_type == "placement"
and resource_name == "allocation_candidate"
and method == "get"
):
operation_key = "list"
elif (
- args.service_type == "placement"
+ service_type == "placement"
and resource_name
in [
"resource_provider/aggregate",
@@ -357,7 +376,7 @@ class MetadataGenerator(BaseGenerator):
)
# Next hacks
- if args.service_type == "identity" and resource_name in [
+ if service_type == "identity" and resource_name in [
"OS_FEDERATION/identity_provider",
"OS_FEDERATION/identity_provider/protocol",
"OS_FEDERATION/mapping",
@@ -368,7 +387,7 @@ class MetadataGenerator(BaseGenerator):
elif method == "patch":
operation_key = "update"
if (
- args.service_type == "identity"
+ service_type == "identity"
and resource_name
in [
"domain/config",
@@ -381,7 +400,7 @@ class MetadataGenerator(BaseGenerator):
operation_key = "default"
if (
- args.service_type == "identity"
+ service_type == "identity"
and resource_name
in [
"domain/config",
@@ -393,7 +412,7 @@ class MetadataGenerator(BaseGenerator):
):
# No need in HEAD defaults
continue
- if args.service_type == "object-store":
+ if service_type == "object-store":
if resource_name == "object":
mapping_obj: dict[str, str] = {
"head": "head",
@@ -421,7 +440,7 @@ class MetadataGenerator(BaseGenerator):
"post": "update",
}
operation_key = mapping_account[method]
- if args.service_type == "dns":
+ if service_type == "dns":
if resource_name == "zone/task":
if path == "/v2/zones/{zone_id}/tasks/xfr":
operation_key = "xfr"
@@ -438,9 +457,10 @@ class MetadataGenerator(BaseGenerator):
if operation_key in resource_model:
raise RuntimeError("Operation name conflict")
else:
- if operation_key == "action" and args.service_type in [
+ if operation_key == "action" and service_type in [
"compute",
"block-storage",
+ "shared-file-system",
]:
# For action we actually have multiple independent operations
try:
@@ -522,7 +542,7 @@ class MetadataGenerator(BaseGenerator):
)
op_model = post_process_operation(
- args.service_type,
+ service_type,
resource_name,
operation_name,
op_model,
@@ -553,13 +573,13 @@ class MetadataGenerator(BaseGenerator):
op_model.targets["rust-sdk"] = rust_sdk_params
if rust_cli_params and not (
- args.service_type == "identity"
+ service_type == "identity"
and operation_key == "check"
):
op_model.targets["rust-cli"] = rust_cli_params
op_model = post_process_operation(
- args.service_type,
+ service_type,
resource_name,
operation_key,
op_model,
@@ -587,7 +607,7 @@ class MetadataGenerator(BaseGenerator):
openapi_spec, show_op.operation_id
)
mod_path = common.get_rust_sdk_mod_path(
- args.service_type, res_data.api_version or "", path
+ service_type, res_data.api_version or "", path
)
response_schema = None
for code, rspec in spec.get("responses", {}).items():
@@ -662,18 +682,7 @@ class MetadataGenerator(BaseGenerator):
if target_name in ["rust-cli"]:
target_params.find_implemented_by_sdk = True
- yaml = YAML()
- yaml.preserve_quotes = True
- yaml.default_flow_style = False
- yaml.indent(mapping=2, sequence=4, offset=2)
- metadata_path.parent.mkdir(exist_ok=True, parents=True)
- with open(metadata_path, "w") as fp:
- yaml.dump(
- metadata.model_dump(
- exclude_none=True, exclude_defaults=True, by_alias=True
- ),
- fp,
- )
+ return metadata
def get_operation_type_by_key(operation_key):
diff --git a/codegenerator/openapi/base.py b/codegenerator/openapi/base.py
index e83883d..7c2e336 100644
--- a/codegenerator/openapi/base.py
+++ b/codegenerator/openapi/base.py
@@ -16,17 +16,23 @@ import datetime
import enum
import importlib
import inspect
+import itertools
+import jsonref
import logging
from pathlib import Path
from typing import Any, Callable, Literal
import re
+from codegenerator import common
from codegenerator.common.schema import ParameterSchema
from codegenerator.common.schema import PathSchema
from codegenerator.common.schema import SpecSchema
from codegenerator.common.schema import TypeSchema
+from codegenerator.metadata import MetadataGenerator
+from codegenerator import model
from codegenerator.openapi.utils import rst_to_md
from openapi_core import Spec
+from openapi_spec_validator import validate
from ruamel.yaml.scalarstring import LiteralScalarString
from ruamel.yaml import YAML
from wsme import types as wtypes
@@ -116,10 +122,10 @@ class OpenStackServerSourceBase:
return SpecSchema(**spec)
- def dump_openapi(self, spec, path, validate=False):
+ def dump_openapi(self, spec, path, validate: bool, service_type: str):
"""Dump OpenAPI spec into the file"""
if validate:
- self.validate_spec(spec)
+ self.validate_spec(spec, service_type)
yaml = YAML()
yaml.preserve_quotes = True
yaml.indent(mapping=2, sequence=4, offset=2)
@@ -131,12 +137,90 @@ class OpenStackServerSourceBase:
fp,
)
- def validate_spec(self, openapi_spec):
- Spec.from_dict(
- openapi_spec.model_dump(
- exclude_none=True, exclude_defaults=True, by_alias=True
- )
+ def validate_spec(self, openapi_spec, service_type: str):
+ # Perform openapi validation
+ model_data = openapi_spec.model_dump(
+ exclude_none=True, exclude_defaults=True, by_alias=True
)
+ Spec.from_dict(model_data)
+ validate(model_data)
+
+ openapi_spec = Spec.from_dict(
+ jsonref.replace_refs(model_data, proxies=False)
+ )
+
+ # Build the metadata as if we would do this normally
+ metadata = MetadataGenerator.build_metadata(
+ SpecSchema(**jsonref.replace_refs(model_data, proxies=False)),
+ openapi_spec,
+ service_type,
+ "",
+ )
+ # Try to parse schema as if we would be doing for generating the code
+ openapi_parser = model.OpenAPISchemaParser()
+ for res, res_spec in metadata.resources.items():
+ for operation_name, operation_spec in res_spec.operations.items():
+ operation_id = operation_spec.operation_id
+
+ (path, method, spec) = common.find_openapi_operation(
+ openapi_spec, operation_id
+ )
+ resource_name = common.get_resource_names_from_url(path)[-1]
+
+ # Parse params
+ for param in openapi_spec["paths"][path].get("parameters", []):
+ openapi_parser.parse_parameter(param)
+
+ op_name: str | None = None
+ response_key: str | None = None
+ sdk_target = operation_spec.targets.get("rust-sdk")
+ if sdk_target:
+ op_name = sdk_target.operation_name
+ response_key = sdk_target.response_key
+
+ operation_variants = common.get_operation_variants(
+ spec, op_name or operation_name
+ )
+
+ for operation_variant in operation_variants:
+ operation_body = operation_variant.get("body")
+ if operation_body:
+ openapi_parser.parse(
+ operation_body, ignore_read_only=True
+ )
+
+ if method.upper() != "HEAD":
+ response = common.find_response_schema(
+ spec["responses"],
+ response_key or resource_name,
+ (
+ operation_name
+ if operation_spec.operation_type == "action"
+ else None
+ ),
+ )
+
+ if response:
+ if response_key:
+ response_key = (
+ response_key
+ if response_key != "null"
+ else None
+ )
+ else:
+ response_key = resource_name
+ response_def, _ = common.find_resource_schema(
+ response, None, response_key
+ )
+
+ if response_def:
+ if response_def.get(
+ "type", "object"
+ ) == "object" or (
+ isinstance(response_def.get("type"), list)
+ and "object" in response_def["type"]
+ ):
+ openapi_parser.parse(response_def)
def _sanitize_param_ver_info(self, openapi_spec, min_api_version):
# Remove min_version of params if it matches to min_api_version
diff --git a/codegenerator/openapi/cinder.py b/codegenerator/openapi/cinder.py
index 5716ed2..bad289e 100644
--- a/codegenerator/openapi/cinder.py
+++ b/codegenerator/openapi/cinder.py
@@ -173,7 +173,9 @@ class CinderV3Generator(OpenStackServerSourceBase):
if args.api_ref_src:
merge_api_ref_doc(openapi_spec, args.api_ref_src)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(
+ openapi_spec, impl_path, args.validate, "block-storage"
+ )
lnk = Path(impl_path.parent, "v3.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/designate.py b/codegenerator/openapi/designate.py
index 69e88a6..1f97a0f 100644
--- a/codegenerator/openapi/designate.py
+++ b/codegenerator/openapi/designate.py
@@ -250,7 +250,7 @@ class DesignateGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, Path(impl_path), args.validate)
+ self.dump_openapi(openapi_spec, Path(impl_path), args.validate, "dns")
lnk = Path(impl_path.parent, "v2.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/glance.py b/codegenerator/openapi/glance.py
index fb4903d..873d77c 100644
--- a/codegenerator/openapi/glance.py
+++ b/codegenerator/openapi/glance.py
@@ -332,7 +332,7 @@ class GlanceGenerator(OpenStackServerSourceBase):
if args.api_ref_src:
merge_api_ref_doc(openapi_spec, args.api_ref_src)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(openapi_spec, impl_path, args.validate, "image")
lnk = Path(impl_path.parent, "v2.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/ironic.py b/codegenerator/openapi/ironic.py
index 1294fd8..8649d16 100644
--- a/codegenerator/openapi/ironic.py
+++ b/codegenerator/openapi/ironic.py
@@ -192,7 +192,9 @@ class IronicGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, Path(impl_path), args.validate)
+ self.dump_openapi(
+ openapi_spec, Path(impl_path), args.validate, "baremetal"
+ )
lnk = Path(impl_path.parent, "v1.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/keystone.py b/codegenerator/openapi/keystone.py
index f4bcf4b..183d6b9 100644
--- a/codegenerator/openapi/keystone.py
+++ b/codegenerator/openapi/keystone.py
@@ -160,7 +160,7 @@ class KeystoneGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(openapi_spec, impl_path, args.validate, "image")
lnk = Path(impl_path.parent, "v3.yaml")
lnk.unlink(missing_ok=True)
@@ -500,7 +500,7 @@ class KeystoneGenerator(OpenStackServerSourceBase):
# Ensure operation tags are existing
for tag in operation_spec.tags:
if tag not in [x["name"] for x in openapi_spec.tags]:
- openapi_spec.tags.append({"name": tag, "description": None})
+ openapi_spec.tags.append({"name": tag})
self._post_process_operation_hook(
openapi_spec, operation_spec, path=path
diff --git a/codegenerator/openapi/manila.py b/codegenerator/openapi/manila.py
index 3059a45..64da8f8 100644
--- a/codegenerator/openapi/manila.py
+++ b/codegenerator/openapi/manila.py
@@ -122,7 +122,9 @@ class ManilaGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(
+ openapi_spec, impl_path, args.validate, "shared-file-system"
+ )
lnk = Path(impl_path.parent, "v2.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/neutron.py b/codegenerator/openapi/neutron.py
index deb59b0..4d24692 100644
--- a/codegenerator/openapi/neutron.py
+++ b/codegenerator/openapi/neutron.py
@@ -180,7 +180,7 @@ class NeutronGenerator(OpenStackServerSourceBase):
# Add base resource routes exposed as a pecan app
self._process_base_resource_routes(openapi_spec, processed_routes)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(openapi_spec, impl_path, args.validate, "network")
def process_neutron_with_vpnaas(self, work_dir, processed_routes, args):
"""Setup base Neutron with enabled vpnaas"""
@@ -240,7 +240,7 @@ class NeutronGenerator(OpenStackServerSourceBase):
(impl_path, openapi_spec) = self._read_spec(work_dir)
self._process_router(router, openapi_spec, processed_routes)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(openapi_spec, impl_path, args.validate, "network")
def _read_spec(self, work_dir):
"""Read the spec from file or create an empty one"""
@@ -373,7 +373,9 @@ class NeutronGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, Path(impl_path), args.validate)
+ self.dump_openapi(
+ openapi_spec, Path(impl_path), args.validate, "network"
+ )
return impl_path
diff --git a/codegenerator/openapi/nova.py b/codegenerator/openapi/nova.py
index 3e041c2..b0e50d4 100644
--- a/codegenerator/openapi/nova.py
+++ b/codegenerator/openapi/nova.py
@@ -103,7 +103,7 @@ class NovaGenerator(OpenStackServerSourceBase):
doc_url_prefix="/v2.1",
)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(openapi_spec, impl_path, args.validate, "compute")
lnk = Path(impl_path.parent, "v2.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/octavia.py b/codegenerator/openapi/octavia.py
index 4924a47..3130ce4 100644
--- a/codegenerator/openapi/octavia.py
+++ b/codegenerator/openapi/octavia.py
@@ -946,7 +946,9 @@ class OctaviaGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, Path(impl_path), args.validate)
+ self.dump_openapi(
+ openapi_spec, Path(impl_path), args.validate, "load-balancer"
+ )
lnk = Path(impl_path.parent, "v2.yaml")
lnk.unlink(missing_ok=True)
diff --git a/codegenerator/openapi/placement.py b/codegenerator/openapi/placement.py
index 12920a6..0e0ca38 100644
--- a/codegenerator/openapi/placement.py
+++ b/codegenerator/openapi/placement.py
@@ -121,7 +121,7 @@ class PlacementGenerator(OpenStackServerSourceBase):
openapi_spec, args.api_ref_src, allow_strip_version=False
)
- self.dump_openapi(openapi_spec, impl_path, args.validate)
+ self.dump_openapi(openapi_spec, impl_path, args.validate, "placement")
lnk = Path(impl_path.parent, "v1.yaml")
lnk.unlink(missing_ok=True)
diff --git a/playbooks/openapi/run.yaml b/playbooks/openapi/run.yaml
index 8ab4f5e..3c1b76c 100644
--- a/playbooks/openapi/run.yaml
+++ b/playbooks/openapi/run.yaml
@@ -17,6 +17,7 @@
--work-dir {{ ansible_user_dir }}/{{ codegenerator_work_dir }}
--target openapi-spec
--service-type {{ openapi_service }}
+ --validate
{%- if codegenerator_api_ref is defined and codegenerator_api_ref is mapping %}
--api-ref-src {{ ansible_user_dir }}/{{ zuul.projects[codegenerator_api_ref.project].src_dir }}/{{ codegenerator_api_ref.path | default("/api-ref/build/html/index.html") }}
{% endif %}