Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

841 Validate properties are defined on cfn validate #892

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions src/rpdk/core/data_loaders.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901
read_only_properties_intersection,
)

verify_listed_properties_are_defined(resource_spec)

for handler in resource_spec.get("handlers", []):
for permission in resource_spec.get("handlers", [])[handler]["permissions"]:
if "*" in permission:
Expand Down Expand Up @@ -368,6 +370,54 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901

return inlined

def verify_listed_properties_are_defined(resource_spec):
LOG.debug("verify_listed_properties_are_defined")

schema_keys = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some newer JSON pointer property types

Suggested change
schema_keys = [
schema_keys = [
"nonPublicProperties",
"conditionalCreateOnlyProperties",

https://github.com/aws-cloudformation/cloudformation-resource-schema/blob/master/src/main/resources/schema/provider.definition.schema.v1.json

"readOnlyProperties",
"createOnlyProperties",
"primaryIdentifier",
"required",
"additionalIdentifiers",
"deprecatedProperties",
"writeOnlyProperties",
]
for schema_key in schema_keys:
verify_top_level_properties_are_defined(resource_spec, schema_key)

def verify_top_level_properties_are_defined(resource_spec, schema_key):
LOG.debug("verify_top_level_properties_are_defined: %s", schema_key)

properties = get_properties_from_schema_key(resource_spec, schema_key)

nested_properties = set(filter(lambda property: "/" in property, properties))
top_level_properties = properties - nested_properties

if len(top_level_properties):
defined_properties = get_defined_properties(resource_spec)
nondefined_top_level_properties = top_level_properties - defined_properties

if nondefined_top_level_properties:
raise SpecValidationError(
get_properties_not_defined_error_message(schema_key, nondefined_top_level_properties)
)

def get_defined_properties(resource_spec):
return set(resource_spec.get("properties", []))

def get_properties_from_schema_key(resource_spec, schema_key):
if schema_key == "additionalIdentifiers":
# additionalIdentifiers is a 2 dimensional list, we need to flatten it
properties = resource_spec.get(schema_key, [[]])
properties = {prop for identifier in properties for prop in identifier}
else:
properties = resource_spec.get(schema_key, [])

return set(map(lambda property: property.replace("/properties/", ""), properties))

def get_properties_not_defined_error_message(key, properties):
return f"The following properties are listed in '{key}' but not defined in 'properties': {', '.join(properties)}"


def load_hook_spec(hook_spec_file): # pylint: disable=R # noqa: C901
"""Load a hook definition from a file, and validate it."""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property1"
],
"additionalIdentifiers": [
[ "/properties/property2" ]
],
"additionalProperties": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property1"
],
"createOnlyProperties": [
"/properties/property2"
],
"additionalProperties": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property1"
],
"deprecatedProperties": [
"/properties/property2"
],
"additionalProperties": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property2"
],
"additionalProperties": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property1"
],
"readOnlyProperties": [
"/properties/property2"
],
"additionalProperties": false
}
16 changes: 16 additions & 0 deletions tests/data/schema/invalid/invalid_required_not_defined.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property1"
],
"required": [
"property2"
],
"additionalProperties": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"typeName": "AWS::Valid::TypeName",
"description": "a test schema",
"properties": {
"property1": {
"type": "string"
}
},
"primaryIdentifier": [
"/properties/property1"
],
"writeOnlyProperties": [
"/properties/property2"
],
"additionalProperties": false
}
46 changes: 46 additions & 0 deletions tests/test_data_loaders.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,52 @@ def test_load_resource_spec_invalid_snippets(example):
with pytest.raises(SpecValidationError):
load_resource_spec(f)

@pytest.mark.parametrize(
"test_props",
[
{
'schema': "invalid_read_only_properties_not_defined.json",
'schema_key': "readOnlyProperties",
'property': "properties/property2",
},
{
'schema': "invalid_create_only_properties_not_defined.json",
'schema_key': "createOnlyProperties",
'property': "properties/property2",
},
{
'schema': "invalid_primary_identifier_not_defined.json",
'schema_key': "primaryIdentifier",
'property': "properties/property2",
},
{
'schema': "invalid_required_not_defined.json",
'schema_key': "required",
'property': "property2",
},
{
'schema': "invalid_additional_identifiers_not_defined.json",
'schema_key': "additionalIdentifiers",
'property': "/properties/property2",
},
{
'schema': "invalid_deprecated_properties_not_defined.json",
'schema_key': "deprecatedProperties",
'property': "/properties/property2",
},
{
'schema': "invalid_write_only_properties_not_defined.json",
'schema_key': "writeOnlyProperties",
'property': "/properties/property2",
},
],
)
def test_load_resource_spec_property_not_defined(test_props):
schema = BASEDIR / "data" / "schema" / "invalid" / test_props["schema"]
with schema.open("r", encoding="utf-8") as f:
with pytest.raises(SpecValidationError) as excinfo:
load_resource_spec(f)
assert f"The following properties are listed in '{test_props['schema_key']}' but not defined in '{test_props['property']}': /properties/property2"in str(excinfo.value)

def test_load_resource_spec_remote_key_is_invalid():
schema = {
Expand Down