From 55c2ad02bfd86fba484f197d3d77c8f8486505dd Mon Sep 17 00:00:00 2001 From: Anton Mokhovikov Date: Fri, 20 Nov 2020 13:57:44 -0800 Subject: [PATCH] Multi Ref Support (#619) --- requirements.txt | 2 + setup.cfg | 2 +- src/rpdk/core/jsonutils/flattener.py | 36 +++-- src/rpdk/core/jsonutils/utils.py | 93 +++++++++-- src/rpdk/core/project.py | 16 +- tests/data/schema/target_output/multiref.md | 126 +++++++++++++++ .../schema/valid/valid_multiref_property.json | 153 ++++++++++++++++++ tests/jsonutils/test_flattener.py | 8 +- tests/test_project.py | 36 +++++ 9 files changed, 435 insertions(+), 37 deletions(-) create mode 100644 tests/data/schema/target_output/multiref.md create mode 100644 tests/data/schema/valid/valid_multiref_property.json diff --git a/requirements.txt b/requirements.txt index 25a56c0f..d3007d33 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,6 +11,8 @@ pytest-random-order>=1.0.4 hypothesis>=4.32.3 pytest-localserver>=0.5.0 +ordered-set>=4.0.2 + # commit hooks pre-commit>=1.18.1 diff --git a/setup.cfg b/setup.cfg index ef357424..c3d76d28 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,7 +29,7 @@ include_trailing_comma = true combine_as_imports = True force_grid_wrap = 0 known_first_party = rpdk -known_third_party = boto3,botocore,colorama,docker,hypothesis,jinja2,jsonschema,pkg_resources,pytest,pytest_localserver,setuptools,yaml +known_third_party = boto3,botocore,colorama,docker,hypothesis,jinja2,jsonschema,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml [tool:pytest] # can't do anything about 3rd part modules, so don't spam us diff --git a/src/rpdk/core/jsonutils/flattener.py b/src/rpdk/core/jsonutils/flattener.py index b8765b47..55963fe3 100644 --- a/src/rpdk/core/jsonutils/flattener.py +++ b/src/rpdk/core/jsonutils/flattener.py @@ -1,8 +1,10 @@ # pylint: disable=too-few-public-methods,raising-format-tuple import logging +from ordered_set import OrderedSet + from .pointer import fragment_decode -from .utils import ConstraintError, FlatteningError, schema_merge, traverse +from .utils import TYPE, ConstraintError, FlatteningError, schema_merge, traverse LOG = logging.getLogger(__name__) COMBINERS = ("oneOf", "anyOf", "allOf") @@ -50,7 +52,6 @@ def _walk(self, sub_schema, property_path): except KeyError: # schemas without type are assumed to be objects json_type = sub_schema.get("type", "object") - if json_type == "array": sub_schema = self._flatten_array_type(sub_schema, property_path) @@ -157,19 +158,24 @@ def _flatten_combiners(self, sub_schema, path): try: schema_array = sub_schema.pop(arr_key) except KeyError: - continue - for i, nested_schema in enumerate(schema_array): - ref_path = path + (arr_key, i) - ref_path_is_used = ref_path in self._schema_map - walked_schema = self._walk(nested_schema, ref_path) - - # if no other schema is referencing the ref_path, - # we no longer need the refkey since the properties will be squashed - if ref_path_is_used: - resolved_schema = self._schema_map.get(ref_path) - else: - resolved_schema = self._schema_map.pop(ref_path, walked_schema) - schema_merge(sub_schema, resolved_schema, path) + pass + else: + for i, nested_schema in enumerate(schema_array): + + ref_path = path + (arr_key, i) + ref_path_is_used = ref_path in self._schema_map + walked_schema = self._walk(nested_schema, ref_path) + + # if no other schema is referencing the ref_path, + # we no longer need the refkey since the properties will be squashed + if ref_path_is_used: + resolved_schema = self._schema_map.get(ref_path) + else: + resolved_schema = self._schema_map.pop(ref_path, walked_schema) + schema_merge(sub_schema, resolved_schema, path) + + if isinstance(sub_schema.get(TYPE), OrderedSet): + sub_schema[TYPE] = list(sub_schema[TYPE]) return sub_schema def _find_subschema_by_ref(self, ref_path): diff --git a/src/rpdk/core/jsonutils/utils.py b/src/rpdk/core/jsonutils/utils.py index 2c6461ee..3b2acca9 100644 --- a/src/rpdk/core/jsonutils/utils.py +++ b/src/rpdk/core/jsonutils/utils.py @@ -1,14 +1,27 @@ from collections.abc import Mapping, Sequence +from typing import Any + +from ordered_set import OrderedSet from .pointer import fragment_encode -NON_MERGABLE_KEYS = ("$ref", "uniqueItems", "insertionOrder") +NON_MERGABLE_KEYS = ("uniqueItems", "insertionOrder") +TYPE = "type" +REF = "$ref" class FlatteningError(Exception): pass +def to_set(value: Any) -> OrderedSet: + return ( + OrderedSet(value) + if isinstance(value, (list, OrderedSet)) + else OrderedSet([value]) + ) + + class ConstraintError(FlatteningError, ValueError): def __init__(self, message, path, *args): self.path = fragment_encode(path) @@ -98,7 +111,7 @@ def traverse(document, path_parts): return document, tuple(path), parent -def schema_merge(target, src, path): # noqa: C901 +def schema_merge(target, src, path): # noqa: C901 # pylint: disable=R0912 """Merges the src schema into the target schema in place. If there are duplicate keys, src will overwrite target. @@ -110,27 +123,66 @@ def schema_merge(target, src, path): # noqa: C901 {} >>> schema_merge({'foo': 'a'}, {}, ()) {'foo': 'a'} + >>> schema_merge({}, {'foo': 'a'}, ()) {'foo': 'a'} + >>> schema_merge({'foo': 'a'}, {'foo': 'b'}, ()) {'foo': 'b'} + >>> schema_merge({'required': 'a'}, {'required': 'b'}, ()) {'required': ['a', 'b']} + + >>> a, b = {'$ref': 'a'}, {'foo': 'b'} + >>> schema_merge(a, b, ('foo',)) + {'$ref': 'a', 'foo': 'b'} + >>> a, b = {'$ref': 'a'}, {'type': 'b'} >>> schema_merge(a, b, ('foo',)) - {'$ref': 'a', 'type': 'b'} + {'type': OrderedSet(['a', 'b'])} + + >>> a, b = {'$ref': 'a'}, {'$ref': 'b'} + >>> schema_merge(a, b, ('foo',)) + {'type': OrderedSet(['a', 'b'])} + + >>> a, b = {'$ref': 'a'}, {'type': ['b', 'c']} + >>> schema_merge(a, b, ('foo',)) + {'type': OrderedSet(['a', 'b', 'c'])} + + >>> a, b = {'$ref': 'a'}, {'type': OrderedSet(['b', 'c'])} + >>> schema_merge(a, b, ('foo',)) + {'type': OrderedSet(['a', 'b', 'c'])} + + >>> a, b = {'type': ['a', 'b']}, {'$ref': 'c'} + >>> schema_merge(a, b, ('foo',)) + {'type': OrderedSet(['a', 'b', 'c'])} + + >>> a, b = {'type': OrderedSet(['a', 'b'])}, {'$ref': 'c'} + >>> schema_merge(a, b, ('foo',)) + {'type': OrderedSet(['a', 'b', 'c'])} + >>> a, b = {'Foo': {'$ref': 'a'}}, {'Foo': {'type': 'b'}} >>> schema_merge(a, b, ('foo',)) - {'Foo': {'$ref': 'a', 'type': 'b'}} + {'Foo': {'type': OrderedSet(['a', 'b'])}} + >>> schema_merge({'type': 'a'}, {'type': 'b'}, ()) # doctest: +NORMALIZE_WHITESPACE - {'type': ['a', 'b']} + {'type': OrderedSet(['a', 'b'])} + + >>> schema_merge({'type': 'string'}, {'type': 'integer'}, ()) + {'type': OrderedSet(['string', 'integer'])} """ if not (isinstance(target, Mapping) and isinstance(src, Mapping)): raise TypeError("Both schemas must be dictionaries") for key, src_schema in src.items(): try: - target_schema = target[key] + if key in ( + REF, + TYPE, + ): # $ref and type are treated similarly and unified + target_schema = target.get(key) or target.get(TYPE) or target[REF] + else: + target_schema = target[key] # carry over existing properties except KeyError: target[key] = src_schema else: @@ -138,11 +190,30 @@ def schema_merge(target, src, path): # noqa: C901 try: target[key] = schema_merge(target_schema, src_schema, next_path) except TypeError: - if key == "type": - if isinstance(target_schema, list): - target_schema.append(src_schema) - continue - target[key] = [target_schema, src_schema] + if key in (TYPE, REF): # combining multiple $ref and types + src_set = to_set(src_schema) + + try: + target[TYPE] = to_set( + target[TYPE] + ) # casting to ordered set as lib + # implicitly converts strings to sets + target[TYPE] |= src_set + except (TypeError, KeyError): + target_set = to_set(target_schema) + target[TYPE] = target_set | src_set + + try: + # check if there are conflicting $ref and type + # at the same sub schema. Conflicting $ref could only + # happen on combiners because method merges two json + # objects without losing any previous info: + # e.g. "oneOf": [{"$ref": "..#1.."},{"$ref": "..#2.."}] -> + # { "ref": "..#1..", "type": [{},{}] } + target.pop(REF) + except KeyError: + pass + elif key == "required": target[key] = sorted(set(target_schema) | set(src_schema)) else: diff --git a/src/rpdk/core/project.py b/src/rpdk/core/project.py index 05517eb5..84f00aed 100644 --- a/src/rpdk/core/project.py +++ b/src/rpdk/core/project.py @@ -646,7 +646,6 @@ def __set_property_type(prop_type, single_type=True): prop[jsontype] = __join(prop.get(jsontype), type_json) prop[yamltype] = __join(prop.get(yamltype), type_yaml) prop[longformtype] = __join(prop.get(longformtype), type_longform) - if "enum" in prop: prop["allowedvalues"] = prop["enum"] @@ -657,12 +656,19 @@ def __set_property_type(prop_type, single_type=True): prop_type = [prop_type] single_item = True - visited = set() for prop_item in prop_type: - if prop_item not in visited: - visited.add(prop_item) + if isinstance(prop_item, tuple): # if tuple, then it's a ref + # using doc method to generate the mdo and reassign the ref + resolved = self._set_docs_properties( + propname, {"$ref": prop_item}, proppath + ) + prop[jsontype] = __join(prop.get(jsontype), resolved[jsontype]) + prop[yamltype] = __join(prop.get(yamltype), resolved[yamltype]) + prop[longformtype] = __join( + prop.get(longformtype), resolved[longformtype] + ) + else: __set_property_type(prop_item, single_type=single_item) - return prop def _upload( diff --git a/tests/data/schema/target_output/multiref.md b/tests/data/schema/target_output/multiref.md new file mode 100644 index 00000000..c313a31a --- /dev/null +++ b/tests/data/schema/target_output/multiref.md @@ -0,0 +1,126 @@ +# AWS::Color::Red + +a test schema + +## Syntax + +To declare this entity in your AWS CloudFormation template, use the following syntax: + +### JSON + +
+{
+    "Type" : "AWS::Color::Red",
+    "Properties" : {
+        "primaryID" : String,
+        "PropertyWithMultipleConstraints" : String,
+        "PropertyWithMultipleMultiples" : String, Map, Integer, Boolean,
+        "PropertyWithMultiplePrimitives" : Integer, String, Map,
+        "PropertyWithTwoComplexTypes" : ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives,
+        "PropertyWithMultipleComplexTypes" : ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives, ComplexTypeWithCircularRef,
+        "PropertyWithMultipleComplexTypesAndOnePrimitive" : ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives, ComplexTypeWithCircularRef, Map,
+        "PropertyWithComplexTypeAndPrimitive" : ComplexTypeWithOnePrimitive, Map,
+        "MultiProperty3" : Integer, Map
+    }
+}
+
+ +### YAML + +
+Type: AWS::Color::Red
+Properties:
+    primaryID: String
+    PropertyWithMultipleConstraints: String
+    PropertyWithMultipleMultiples: String, Map, Integer, Boolean
+    PropertyWithMultiplePrimitives: Integer, String, Map
+    PropertyWithTwoComplexTypes: ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives
+    PropertyWithMultipleComplexTypes: ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives, ComplexTypeWithCircularRef
+    PropertyWithMultipleComplexTypesAndOnePrimitive: ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives, ComplexTypeWithCircularRef, Map
+    PropertyWithComplexTypeAndPrimitive: ComplexTypeWithOnePrimitive, Map
+    MultiProperty3: Integer, Map
+
+ +## Properties + +#### primaryID + +_Required_: No + +_Type_: String + +_Update requires_: [Replacement](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-replacement) + +#### PropertyWithMultipleConstraints + +_Required_: No + +_Type_: String + +_Minimum_: 13 + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### PropertyWithMultipleMultiples + +_Required_: No + +_Type_: String, Map, Integer, Boolean + +_Minimum_: 13 + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### PropertyWithMultiplePrimitives + +_Required_: No + +_Type_: Integer, String, Map + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### PropertyWithTwoComplexTypes + +_Required_: No + +_Type_: ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### PropertyWithMultipleComplexTypes + +_Required_: No + +_Type_: ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives, ComplexTypeWithCircularRef + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### PropertyWithMultipleComplexTypesAndOnePrimitive + +_Required_: No + +_Type_: ComplexTypeWithOnePrimitive, ComplexTypeWithMultiplePrimitives, ComplexTypeWithCircularRef, Map + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### PropertyWithComplexTypeAndPrimitive + +_Required_: No + +_Type_: ComplexTypeWithOnePrimitive, Map + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +#### MultiProperty3 + +_Required_: No + +_Type_: Integer, Map + +_Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt) + +## Return Values + +### Ref + +When you pass the logical ID of this resource to the intrinsic `Ref` function, Ref returns the primaryID. diff --git a/tests/data/schema/valid/valid_multiref_property.json b/tests/data/schema/valid/valid_multiref_property.json new file mode 100644 index 00000000..4d1b8843 --- /dev/null +++ b/tests/data/schema/valid/valid_multiref_property.json @@ -0,0 +1,153 @@ +{ + "typeName": "AWS::Valid::TypeName", + "description": "a test schema", + "definitions": { + "ComplexTypeWithOnePrimitive": { + "type": "object", + "properties": { + "PrimitiveProperty": { + "type": "string" + } + } + }, + "ComplexTypeWithMultiplePrimitives": { + "type": "object", + "properties": { + "PrimitiveProperty1": { + "type": "string" + }, + "PrimitiveProperty2": { + "type": "string" + } + } + }, + "ComplexTypeWithCircularRef": { + "type": "object", + "properties": { + "PrimitiveProperty": { + "type": "string" + }, + "CircularProperty": { + "$ref": "#/definitions/ComplexTypeWithCircularRef" + } + } + } + }, + "properties": { + "primaryID": { + "type": "string" + }, + "PropertyWithMultipleConstraints": { + "oneOf": [ + { + "type": "string", + "minLength": 3 + }, + { + "type": "string", + "minLength": 13 + } + ] + }, + "PropertyWithMultipleMultiples": { + "oneOf": [ + { + "type": "string", + "minLength": 3 + }, + { + "type": "string", + "minLength": 13 + }, + { + "type": "object" + }, + { + "type": [ + "integer", + "boolean" + ] + } + ] + }, + "PropertyWithMultiplePrimitives": { + "oneOf": [ + { + "type": "integer" + }, + { + "type": "string" + }, + { + "type": "object" + } + ] + }, + "PropertyWithTwoComplexTypes": { + "oneOf": [ + { + "$ref": "#/definitions/ComplexTypeWithOnePrimitive" + }, + { + "$ref": "#/definitions/ComplexTypeWithMultiplePrimitives" + } + ] + }, + "PropertyWithMultipleComplexTypes": { + "oneOf": [ + { + "$ref": "#/definitions/ComplexTypeWithOnePrimitive" + }, + { + "$ref": "#/definitions/ComplexTypeWithMultiplePrimitives" + }, + { + "$ref": "#/definitions/ComplexTypeWithCircularRef" + } + ] + }, + "PropertyWithMultipleComplexTypesAndOnePrimitive": { + "oneOf": [ + { + "$ref": "#/definitions/ComplexTypeWithOnePrimitive" + }, + { + "$ref": "#/definitions/ComplexTypeWithMultiplePrimitives" + }, + { + "$ref": "#/definitions/ComplexTypeWithCircularRef" + }, + { + "type": "object" + } + ] + }, + "PropertyWithComplexTypeAndPrimitive": { + "oneOf": [ + { + "$ref": "#/definitions/ComplexTypeWithOnePrimitive" + }, + { + "type": "object" + } + ] + }, + "MultiProperty3": { + "oneOf": [ + { + "type": "integer" + }, + { + "type": "object" + } + ] + } + }, + "additionalProperties": false, + "createOnlyProperties": [ + "/properties/primaryID" + ], + "primaryIdentifier": [ + "/properties/primaryID" + ] +} diff --git a/tests/jsonutils/test_flattener.py b/tests/jsonutils/test_flattener.py index ad82c17f..f123a5e7 100644 --- a/tests/jsonutils/test_flattener.py +++ b/tests/jsonutils/test_flattener.py @@ -306,7 +306,7 @@ def test_flatten_combiners_resolve_types_nested_should_fail(combiner): @pytest.mark.parametrize("combiner", COMBINERS) -def test_flatten_combiners_flattened_before_merge_failed_but_should_not(combiner): +def test_flatten_combiners_flattened_before_merge(combiner): # this should not fail, since the refs are actually compatible with each other # https://github.com/aws-cloudformation/aws-cloudformation-rpdk/issues/333 ref = ("definitions", "obj") @@ -327,10 +327,8 @@ def test_flatten_combiners_flattened_before_merge_failed_but_should_not(combiner }, } - flattener = JsonSchemaFlattener(test_schema) - with pytest.raises(ConstraintError) as excinfo: - flattener.flatten_schema() - assert "declared multiple values for '$ref'" in str(excinfo.value) + flattened_schema = JsonSchemaFlattener(test_schema).flatten_schema() + assert isinstance(flattened_schema[()]["properties"]["p"]["type"], list) def test_contraint_array_additional_items_valid(): diff --git a/tests/test_project.py b/tests/test_project.py index f4c51412..c4f5029c 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -283,6 +283,42 @@ def test_generate_docs_with_multityped_property(project, tmp_path_factory): assert read_me_stripped == read_me_target_stripped +def test_generate_docs_with_multiref_property(project, tmp_path_factory): + project.schema = resource_json( + __name__, "data/schema/valid/valid_multiref_property.json" + ) + + project.type_name = "AWS::Color::Red" + # tmpdir conflicts with other tests, make a unique one + project.root = tmp_path_factory.mktemp("generate_with_docs_type_complex") + mock_plugin = MagicMock(spec=["generate"]) + with patch.object(project, "_plugin", mock_plugin): + project.generate() + project.generate_docs() + mock_plugin.generate.assert_called_once_with(project) + + docs_dir = project.root / "docs" + readme_file = project.root / "docs" / "README.md" + + assert docs_dir.is_dir() + assert readme_file.is_file() + with patch.object(project, "_plugin", mock_plugin): + project.generate() + readme_contents = readme_file.read_text(encoding="utf-8") + readme_contents_target = resource_stream( + __name__, "data/schema/target_output/multiref.md" + ) + + read_me_stripped = readme_contents.strip().replace(" ", "") + read_me_target_stripped = readme_contents_target.read().strip().replace(" ", "") + + LOG.debug("read_me_stripped %s", read_me_stripped) + LOG.debug("read_me_target_stripped %s", read_me_target_stripped) + + assert project.type_name in readme_contents + assert read_me_stripped == read_me_target_stripped + + def test_generate_with_docs_invalid_property_type(project, tmp_path_factory): project.schema = resource_json( __name__, "data/schema/invalid/invalid_property_type_invalid.json"