From b4574ec2bcf7e36e02127b91138d2563639e8352 Mon Sep 17 00:00:00 2001 From: Artem Goncharov 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 %}