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

feat: ✨ add check_resource_properties [5/7] #965

Open
wants to merge 14 commits into
base: feat/sprout-checks-4-sprout-specific-package-and-resource-errors
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 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
4 changes: 2 additions & 2 deletions ruff.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Support Python 3.10+
target-version = "py310"
# Support Python 3.12+
target-version = "py312"

# In addition to the default formatters/linters, add these as well.
[lint]
Expand Down
48 changes: 48 additions & 0 deletions seedcase_sprout/core/sprout_checks/check_resource_properties.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is very similar to the other two top-level functions check_package_properties and check_properties (in other PRs). We can commonise some bits later if we want.

Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
from seedcase_sprout.core import checks
from seedcase_sprout.core.sprout_checks.exclude_non_sprout_resource_errors import (
exclude_non_sprout_resource_errors,
)
from seedcase_sprout.core.sprout_checks.get_sprout_resource_errors import (
get_sprout_resource_errors,
)


def check_resource_properties(properties: dict, check_required: bool = True) -> dict:
"""Checks that resource `properties` matches requirements in Sprout.

`properties` is checked against the Data Package standard and the following
Sprout-specific requirements:
- Sprout-specific required fields are present
- Required fields are not blank
- `path` is of type string
- `path` includes resource ID
- `data` is not set

Only resource properties are checked.

Args:
properties: The resource properties to check.
signekb marked this conversation as resolved.
Show resolved Hide resolved
check_required: Whether the function should enforce the presence of required
fields. Defaults to True.

Returns:
`properties`, if all checks passed.

Raises:
ExceptionGroup: A group of `CheckError`s, one for each check that failed.
"""
errors = checks.check_resource_properties(properties)
errors = exclude_non_sprout_resource_errors(errors)

if not check_required:
errors = [error for error in errors if error.validator != "required"]

errors += get_sprout_resource_errors(properties, check_required)
errors = sorted(set(errors))

if errors:
raise ExceptionGroup(
f"Resource properties check failed on properties\n{properties}", errors
martonvago marked this conversation as resolved.
Show resolved Hide resolved
)

return properties
168 changes: 168 additions & 0 deletions tests/core/sprout_checks/test_check_resource_properties_sprout.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
from pathlib import Path

from pytest import fixture, mark, raises

from seedcase_sprout.core.checks.check_error import CheckError
from seedcase_sprout.core.properties import ResourceProperties
from seedcase_sprout.core.sprout_checks.check_resource_properties import (
check_resource_properties,
)
from seedcase_sprout.core.sprout_checks.get_blank_value_for_type import (
get_blank_value_for_type,
)
from seedcase_sprout.core.sprout_checks.required_fields import (
RESOURCE_SPROUT_REQUIRED_FIELDS,
)


@fixture
def properties():
return ResourceProperties(
name="resource-1",
path=str(Path("resources", "1", "data.parquet")),
title="Resource 1",
description="A resource.",
).compact_dict


@mark.parametrize("check_required", [True, False])
def test_passes_full_resource_properties(properties, check_required):
"""Should pass if all required fields are present and correct."""
assert (
check_resource_properties(properties, check_required=check_required)
== properties
)


@mark.parametrize(
"field",
RESOURCE_SPROUT_REQUIRED_FIELDS.keys(),
)
def test_error_raised_if_required_field_is_missing(properties, field):
"""Should raise an error if a required field is missing."""
del properties[field]

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=True)

errors = error_info.value.exceptions
assert len(errors) == 1
assert isinstance(errors[0], CheckError)
assert errors[0].json_path == f"$.{field}"
assert errors[0].validator == "required"


@mark.parametrize("check_required", [True, False])
@mark.parametrize("path", ["", [], str(Path("resources", "1")), "/bad/path/data.csv"])
def test_error_raised_if_data_path_is_incorrect_(properties, path, check_required):
"""Should raise an error if `path` contains no resource ID, is not a string, or is
otherwise malformed."""
properties["path"] = path

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

errors = error_info.value.exceptions
assert len(errors) >= 1
assert all(
isinstance(error, CheckError) and error.json_path.endswith("path")
for error in errors
)


@mark.parametrize("check_required", [True, False])
def test_error_raised_if_inline_data_is_set(properties, check_required):
"""Should raise an error if inline data is set."""
properties["data"] = "some data"

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

errors = error_info.value.exceptions
assert len(errors) == 1
assert isinstance(errors[0], CheckError)
assert errors[0].json_path == "$.data"
assert errors[0].validator == "inline-data"


@mark.parametrize("check_required", [True, False])
@mark.parametrize("name,type", RESOURCE_SPROUT_REQUIRED_FIELDS.items())
def test_error_raised_if_fields_are_blank(properties, name, type, check_required):
"""Should raise an error if there is one required field that is present but
blank."""
properties[name] = get_blank_value_for_type(type)

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

blank_errors = [
error for error in error_info.value.exceptions if error.validator == "blank"
]

assert len(blank_errors) == 1
assert isinstance(blank_errors[0], CheckError)
assert blank_errors[0].json_path == f"$.{name}"


@mark.parametrize("check_required", [True, False])
def test_error_raised_for_mismatched_pattern(properties, check_required):
"""Should raise an error if `name` violates the pattern."""
properties["name"] = "a name with spaces"

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

errors = error_info.value.exceptions
assert len(errors) == 1
assert isinstance(errors[0], CheckError)
assert errors[0].json_path == "$.name"
assert errors[0].validator == "pattern"


@mark.parametrize("check_required", [True, False])
def test_error_raised_for_mismatched_format(properties, check_required):
"""Should raise an error if `homepage` violates the format."""
properties["homepage"] = "not a URL"

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

errors = error_info.value.exceptions
assert len(errors) == 1
assert isinstance(errors[0], CheckError)
assert errors[0].json_path == "$.homepage"
assert errors[0].validator == "format"


@mark.parametrize("check_required", [True, False])
def test_error_raised_for_mismatched_type(properties, check_required):
"""Should raise an error if `name` violates the type constraint."""
properties["name"] = 123

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

errors = error_info.value.exceptions
assert len(errors) == 1
assert isinstance(errors[0], CheckError)
assert errors[0].json_path == "$.name"
assert errors[0].validator == "type"


@mark.parametrize("check_required", [True, False])
def test_error_raised_for_only_sprout_specific_errors(properties, check_required):
"""Errors should be triggered by only those Data Package standard violations that
are relevant for Sprout."""
properties["path"] = 123

with raises(ExceptionGroup) as error_info:
check_resource_properties(properties, check_required=check_required)

errors = error_info.value.exceptions
assert errors == (CheckError("123 is not of type 'string'", "$.path", "type"),)
martonvago marked this conversation as resolved.
Show resolved Hide resolved


def test_passes_partial_resource_properties_without_required_check():
"""Should pass properties with missing required fields when these are not
enforced."""
assert check_resource_properties({}, check_required=False) == {}