From eccf0d5f85d973d72942e987074f6fc66aae953d Mon Sep 17 00:00:00 2001 From: Anton Mokhovikov Date: Wed, 8 Jul 2020 00:59:30 -0700 Subject: [PATCH 1/2] added primary id validation --- src/rpdk/core/data_loaders.py | 17 +++++++++++++++++ .../invalid/invalid_additional_items.json | 3 +++ .../invalid/invalid_additional_properties.json | 6 ++++++ .../invalid_additional_properties_true.json | 6 ++++++ .../invalid/invalid_boolean_property.json | 6 ++++++ .../invalid_definition_name_invalid.json | 6 ++++++ .../invalid_definition_reference_invalid.json | 3 +++ .../invalid_extraneous_top_level_keys.json | 3 +++ .../invalid/invalid_handler_definitions.json | 3 +++ .../data/schema/invalid/invalid_items_list.json | 3 +++ .../schema/invalid/invalid_no_description.json | 3 +++ .../invalid/invalid_no_primary_identifier.json | 3 --- .../invalid/invalid_no_properties_defined.json | 3 +++ .../schema/invalid/invalid_no_type_const.json | 3 +++ .../schema/invalid/invalid_no_type_enum.json | 3 +++ .../data/schema/invalid/invalid_primary_id.json | 13 +++++++++++++ ...nvalid_properties_and_patternproperties.json | 3 +++ .../invalid/invalid_property_name_invalid.json | 3 +++ .../invalid/invalid_property_not_an_object.json | 3 +++ .../invalid/invalid_property_type_invalid.json | 3 +++ .../invalid/invalid_remote_reserved_word.json | 3 +++ tests/data/schema/invalid/invalid_typename.json | 3 +++ .../data/schema/valid/valid_array_no_items.json | 3 +++ .../valid/valid_nested_property_object.json | 3 +++ .../data/schema/valid/valid_no_properties.json | 3 ++- tests/data/schema/valid/valid_no_type.json | 3 +++ .../schema/valid/valid_pattern_properties.json | 3 ++- tests/data/schema/valid/valid_type_complex.json | 3 ++- ...valid_type_composite_primary_identifier.json | 4 ++++ tests/data/schema/valid/valid_type_const.json | 3 +++ tests/data/schema/valid/valid_type_enum.json | 3 +++ .../data/schema/valid/valid_with_handlers.json | 3 +++ tests/test_data_loaders.py | 2 ++ 33 files changed, 129 insertions(+), 6 deletions(-) create mode 100644 tests/data/schema/invalid/invalid_primary_id.json diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 0bfcf505..9e97ee24 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -91,6 +91,13 @@ def get_file_base_uri(file): return path.resolve().as_uri() +def _is_in(schema, key): + def contains(values): + return set(values).issubset(set(schema.get(key, []))) + + return contains + + def load_resource_spec(resource_spec_file): # noqa: C901 """Load a resource provider definition from a file, and validate it.""" try: @@ -106,6 +113,16 @@ def load_resource_spec(resource_spec_file): # noqa: C901 LOG.debug("Resource spec validation failed", exc_info=True) raise SpecValidationError(str(e)) from e + in_readonly = _is_in(resource_spec, "readOnlyProperties") + in_createonly = _is_in(resource_spec, "createOnlyProperties") + + primary_id = resource_spec["primaryIdentifier"] + if not in_readonly(primary_id) and not in_createonly(primary_id): + raise SpecValidationError( + "Property 'primaryIdentifier' must be specified \ +as either readOnly or createOnly" + ) + # TODO: more general validation framework if "remote" in resource_spec: raise SpecValidationError( diff --git a/tests/data/schema/invalid/invalid_additional_items.json b/tests/data/schema/invalid/invalid_additional_items.json index 20e4a180..7a3870bd 100644 --- a/tests/data/schema/invalid/invalid_additional_items.json +++ b/tests/data/schema/invalid/invalid_additional_items.json @@ -10,5 +10,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_additional_properties.json b/tests/data/schema/invalid/invalid_additional_properties.json index b9587eee..8df6a45c 100644 --- a/tests/data/schema/invalid/invalid_additional_properties.json +++ b/tests/data/schema/invalid/invalid_additional_properties.json @@ -9,5 +9,11 @@ } } }, + "primaryIdentifier": [ + "/properties/property1" + ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_additional_properties_true.json b/tests/data/schema/invalid/invalid_additional_properties_true.json index b33d8145..0fbf938f 100644 --- a/tests/data/schema/invalid/invalid_additional_properties_true.json +++ b/tests/data/schema/invalid/invalid_additional_properties_true.json @@ -7,5 +7,11 @@ "additionalProperties": true } }, + "primaryIdentifier": [ + "/properties/property1" + ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_boolean_property.json b/tests/data/schema/invalid/invalid_boolean_property.json index ed317401..163064b3 100644 --- a/tests/data/schema/invalid/invalid_boolean_property.json +++ b/tests/data/schema/invalid/invalid_boolean_property.json @@ -4,5 +4,11 @@ "properties": { "property1": true }, + "primaryIdentifier": [ + "/properties/property1" + ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_definition_name_invalid.json b/tests/data/schema/invalid/invalid_definition_name_invalid.json index 74779e59..5af61664 100644 --- a/tests/data/schema/invalid/invalid_definition_name_invalid.json +++ b/tests/data/schema/invalid/invalid_definition_name_invalid.json @@ -11,5 +11,11 @@ "type": "integer" } }, + "primaryIdentifier": [ + "/properties/property1" + ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_definition_reference_invalid.json b/tests/data/schema/invalid/invalid_definition_reference_invalid.json index 4fcdc162..d5f508bd 100644 --- a/tests/data/schema/invalid/invalid_definition_reference_invalid.json +++ b/tests/data/schema/invalid/invalid_definition_reference_invalid.json @@ -10,5 +10,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_extraneous_top_level_keys.json b/tests/data/schema/invalid/invalid_extraneous_top_level_keys.json index 442b87c2..6f6a46db 100644 --- a/tests/data/schema/invalid/invalid_extraneous_top_level_keys.json +++ b/tests/data/schema/invalid/invalid_extraneous_top_level_keys.json @@ -14,6 +14,9 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false, "unsupportedKey": 1 } diff --git a/tests/data/schema/invalid/invalid_handler_definitions.json b/tests/data/schema/invalid/invalid_handler_definitions.json index 7c15448b..03140048 100644 --- a/tests/data/schema/invalid/invalid_handler_definitions.json +++ b/tests/data/schema/invalid/invalid_handler_definitions.json @@ -19,5 +19,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_items_list.json b/tests/data/schema/invalid/invalid_items_list.json index 8b58de89..183babfa 100644 --- a/tests/data/schema/invalid/invalid_items_list.json +++ b/tests/data/schema/invalid/invalid_items_list.json @@ -14,5 +14,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_no_description.json b/tests/data/schema/invalid/invalid_no_description.json index 20e4a180..7a3870bd 100644 --- a/tests/data/schema/invalid/invalid_no_description.json +++ b/tests/data/schema/invalid/invalid_no_description.json @@ -10,5 +10,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_no_primary_identifier.json b/tests/data/schema/invalid/invalid_no_primary_identifier.json index 20e4a180..93995c8a 100644 --- a/tests/data/schema/invalid/invalid_no_primary_identifier.json +++ b/tests/data/schema/invalid/invalid_no_primary_identifier.json @@ -7,8 +7,5 @@ "additionalItems": true } }, - "primaryIdentifier": [ - "/properties/property1" - ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_no_properties_defined.json b/tests/data/schema/invalid/invalid_no_properties_defined.json index 73fcb87a..7af40f85 100644 --- a/tests/data/schema/invalid/invalid_no_properties_defined.json +++ b/tests/data/schema/invalid/invalid_no_properties_defined.json @@ -5,5 +5,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_no_type_const.json b/tests/data/schema/invalid/invalid_no_type_const.json index 6b7fb7d9..78b5b51d 100644 --- a/tests/data/schema/invalid/invalid_no_type_const.json +++ b/tests/data/schema/invalid/invalid_no_type_const.json @@ -9,5 +9,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_no_type_enum.json b/tests/data/schema/invalid/invalid_no_type_enum.json index b4b00dfb..b0806671 100644 --- a/tests/data/schema/invalid/invalid_no_type_enum.json +++ b/tests/data/schema/invalid/invalid_no_type_enum.json @@ -12,5 +12,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_primary_id.json b/tests/data/schema/invalid/invalid_primary_id.json new file mode 100644 index 00000000..a51bb381 --- /dev/null +++ b/tests/data/schema/invalid/invalid_primary_id.json @@ -0,0 +1,13 @@ +{ + "typeName": "AWS::Valid::TypeName", + "description": "a test schema", + "properties": { + "property1": { + "type": "array" + } + }, + "primaryIdentifier": [ + "/properties/property1" + ], + "additionalProperties": false +} diff --git a/tests/data/schema/invalid/invalid_properties_and_patternproperties.json b/tests/data/schema/invalid/invalid_properties_and_patternproperties.json index af5dded4..70d966cc 100644 --- a/tests/data/schema/invalid/invalid_properties_and_patternproperties.json +++ b/tests/data/schema/invalid/invalid_properties_and_patternproperties.json @@ -22,5 +22,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_property_name_invalid.json b/tests/data/schema/invalid/invalid_property_name_invalid.json index 544e97fb..8e26a804 100644 --- a/tests/data/schema/invalid/invalid_property_name_invalid.json +++ b/tests/data/schema/invalid/invalid_property_name_invalid.json @@ -8,5 +8,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_property_not_an_object.json b/tests/data/schema/invalid/invalid_property_not_an_object.json index e09f9a6a..1d59457e 100644 --- a/tests/data/schema/invalid/invalid_property_not_an_object.json +++ b/tests/data/schema/invalid/invalid_property_not_an_object.json @@ -6,5 +6,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_property_type_invalid.json b/tests/data/schema/invalid/invalid_property_type_invalid.json index 88e9c513..b1b6ffc9 100644 --- a/tests/data/schema/invalid/invalid_property_type_invalid.json +++ b/tests/data/schema/invalid/invalid_property_type_invalid.json @@ -9,5 +9,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_remote_reserved_word.json b/tests/data/schema/invalid/invalid_remote_reserved_word.json index 26bf9270..264bba29 100644 --- a/tests/data/schema/invalid/invalid_remote_reserved_word.json +++ b/tests/data/schema/invalid/invalid_remote_reserved_word.json @@ -12,5 +12,8 @@ "remote": { "schema01": {} }, + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/invalid/invalid_typename.json b/tests/data/schema/invalid/invalid_typename.json index 75699682..c84aedc1 100644 --- a/tests/data/schema/invalid/invalid_typename.json +++ b/tests/data/schema/invalid/invalid_typename.json @@ -9,5 +9,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_array_no_items.json b/tests/data/schema/valid/valid_array_no_items.json index a51bb381..6252e093 100644 --- a/tests/data/schema/valid/valid_array_no_items.json +++ b/tests/data/schema/valid/valid_array_no_items.json @@ -9,5 +9,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_nested_property_object.json b/tests/data/schema/valid/valid_nested_property_object.json index 7cee2afd..72b7a177 100644 --- a/tests/data/schema/valid/valid_nested_property_object.json +++ b/tests/data/schema/valid/valid_nested_property_object.json @@ -14,5 +14,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_no_properties.json b/tests/data/schema/valid/valid_no_properties.json index 1a0629f2..2c950ade 100644 --- a/tests/data/schema/valid/valid_no_properties.json +++ b/tests/data/schema/valid/valid_no_properties.json @@ -11,7 +11,8 @@ "/properties/enum1" ], "readOnlyProperties": [ - "/properties/str3" + "/properties/str3", + "/properties/enum1" ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_no_type.json b/tests/data/schema/valid/valid_no_type.json index f53fce8d..961d8a5c 100644 --- a/tests/data/schema/valid/valid_no_type.json +++ b/tests/data/schema/valid/valid_no_type.json @@ -19,5 +19,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_pattern_properties.json b/tests/data/schema/valid/valid_pattern_properties.json index 637d3896..3222f8aa 100644 --- a/tests/data/schema/valid/valid_pattern_properties.json +++ b/tests/data/schema/valid/valid_pattern_properties.json @@ -66,7 +66,8 @@ "/properties/enum1" ], "readOnlyProperties": [ - "/properties/str3" + "/properties/str3", + "/properties/enum1" ], "additionalIdentifiers": [ [ diff --git a/tests/data/schema/valid/valid_type_complex.json b/tests/data/schema/valid/valid_type_complex.json index 4d2f1b3d..c890bad5 100644 --- a/tests/data/schema/valid/valid_type_complex.json +++ b/tests/data/schema/valid/valid_type_complex.json @@ -46,7 +46,8 @@ "/properties/enum1" ], "readOnlyProperties": [ - "/properties/str3" + "/properties/str3", + "/properties/enum1" ], "additionalIdentifiers": [ [ diff --git a/tests/data/schema/valid/valid_type_composite_primary_identifier.json b/tests/data/schema/valid/valid_type_composite_primary_identifier.json index beb4f49a..17ced9c9 100644 --- a/tests/data/schema/valid/valid_type_composite_primary_identifier.json +++ b/tests/data/schema/valid/valid_type_composite_primary_identifier.json @@ -13,5 +13,9 @@ "/properties/property1", "/properties/property2" ], + "readOnlyProperties": [ + "/properties/property1", + "/properties/property2" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_type_const.json b/tests/data/schema/valid/valid_type_const.json index 064f8e1f..7831470e 100644 --- a/tests/data/schema/valid/valid_type_const.json +++ b/tests/data/schema/valid/valid_type_const.json @@ -10,5 +10,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_type_enum.json b/tests/data/schema/valid/valid_type_enum.json index 177ff67b..ce961ed0 100644 --- a/tests/data/schema/valid/valid_type_enum.json +++ b/tests/data/schema/valid/valid_type_enum.json @@ -13,5 +13,8 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false } diff --git a/tests/data/schema/valid/valid_with_handlers.json b/tests/data/schema/valid/valid_with_handlers.json index 0df96e90..d7b48b56 100644 --- a/tests/data/schema/valid/valid_with_handlers.json +++ b/tests/data/schema/valid/valid_with_handlers.json @@ -9,6 +9,9 @@ "primaryIdentifier": [ "/properties/property1" ], + "readOnlyProperties": [ + "/properties/property1" + ], "additionalProperties": false, "handlers": { "create": { diff --git a/tests/test_data_loaders.py b/tests/test_data_loaders.py index f7f60f1a..16de29ac 100644 --- a/tests/test_data_loaders.py +++ b/tests/test_data_loaders.py @@ -36,6 +36,7 @@ "description": "test schema", "properties": {"foo": {"type": "string"}}, "primaryIdentifier": ["/properties/foo"], + "readOnlyProperties": ["/properties/foo"], "additionalProperties": False, } @@ -112,6 +113,7 @@ def test_load_resource_spec_remote_key_is_invalid(): "description": "test schema", "properties": {"foo": {"type": "string"}}, "primaryIdentifier": ["/properties/foo"], + "readOnlyProperties": ["/properties/foo"], "remote": {}, } with pytest.raises(SpecValidationError) as excinfo: From cf31c37a68df4d87d189ff4d08a73d18874a09b9 Mon Sep 17 00:00:00 2001 From: Anton Mokhovikov Date: Fri, 17 Jul 2020 15:57:46 -0700 Subject: [PATCH 2/2] changed from exception to warning --- src/rpdk/core/data_loaders.py | 2 +- src/rpdk/core/project.py | 2 +- tests/data/schema/{invalid => valid}/invalid_primary_id.json | 0 tests/test_project.py | 4 +++- 4 files changed, 5 insertions(+), 3 deletions(-) rename tests/data/schema/{invalid => valid}/invalid_primary_id.json (100%) diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 9e97ee24..7a279ff4 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -118,7 +118,7 @@ def load_resource_spec(resource_spec_file): # noqa: C901 primary_id = resource_spec["primaryIdentifier"] if not in_readonly(primary_id) and not in_createonly(primary_id): - raise SpecValidationError( + LOG.warning( "Property 'primaryIdentifier' must be specified \ as either readOnly or createOnly" ) diff --git a/src/rpdk/core/project.py b/src/rpdk/core/project.py index 608729a7..9296fbd8 100644 --- a/src/rpdk/core/project.py +++ b/src/rpdk/core/project.py @@ -249,7 +249,7 @@ def safewrite(self, path, contents): else: f.write(contents) except FileExistsError: - LOG.warning("File already exists, not overwriting '%s'", path) + LOG.info("File already exists, not overwriting '%s'", path) def generate(self): # generate template for IAM role assumed by cloudformation diff --git a/tests/data/schema/invalid/invalid_primary_id.json b/tests/data/schema/valid/invalid_primary_id.json similarity index 100% rename from tests/data/schema/invalid/invalid_primary_id.json rename to tests/data/schema/valid/invalid_primary_id.json diff --git a/tests/test_project.py b/tests/test_project.py index 9cbff737..ccc6e662 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -1,6 +1,7 @@ # fixture and parameter have the same name # pylint: disable=redefined-outer-name,useless-super-delegation,protected-access import json +import logging import os import random import string @@ -174,6 +175,7 @@ def test_safewrite_doesnt_exist(project, tmpdir): def test_safewrite_exists(project, tmpdir, caplog): + caplog.set_level(logging.INFO) path = Path(tmpdir.join("test")).resolve() with path.open("w", encoding="utf-8") as f: @@ -183,7 +185,7 @@ def test_safewrite_exists(project, tmpdir, caplog): project.safewrite(path, CONTENTS_UTF8) last_record = caplog.records[-1] - assert last_record.levelname == "WARNING" + assert last_record.levelname == "INFO" assert str(path) in last_record.message