From 0f4fa5539194f67f0c2b2dd2a343816f29a234ce Mon Sep 17 00:00:00 2001 From: martonvago <57952344+martonvago@users.noreply.github.com> Date: Tue, 21 Jan 2025 12:40:22 +0000 Subject: [PATCH] feat: :sparkles: add functions to check required and blank fields (#963) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR adds functions for checking that (Sprout-specific) required fields are present and not blank. This PR needs an in-depth review. ## Checklist - [x] Added or updated tests - [x] Updated documentation - [x] Ran `just run-all` --------- Co-authored-by: Luke W. Johnston Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com> --- .../sprout_checks/check_fields_not_blank.py | 39 +++++++++ .../sprout_checks/check_fields_present.py | 36 +++++++++ .../check_list_item_field_not_blank.py | 33 ++++++++ ...k_required_package_properties_not_blank.py | 32 ++++++++ .../test_check_fields_not_blank.py | 57 +++++++++++++ .../test_check_fields_present.py | 57 +++++++++++++ .../test_check_list_item_field_not_blank.py | 74 +++++++++++++++++ ...k_required_package_properties_not_blank.py | 80 +++++++++++++++++++ 8 files changed, 408 insertions(+) create mode 100644 seedcase_sprout/core/sprout_checks/check_fields_not_blank.py create mode 100644 seedcase_sprout/core/sprout_checks/check_fields_present.py create mode 100644 seedcase_sprout/core/sprout_checks/check_list_item_field_not_blank.py create mode 100644 seedcase_sprout/core/sprout_checks/check_required_package_properties_not_blank.py create mode 100644 tests/core/sprout_checks/test_check_fields_not_blank.py create mode 100644 tests/core/sprout_checks/test_check_fields_present.py create mode 100644 tests/core/sprout_checks/test_check_list_item_field_not_blank.py create mode 100644 tests/core/sprout_checks/test_check_required_package_properties_not_blank.py diff --git a/seedcase_sprout/core/sprout_checks/check_fields_not_blank.py b/seedcase_sprout/core/sprout_checks/check_fields_not_blank.py new file mode 100644 index 000000000..a86c4ea43 --- /dev/null +++ b/seedcase_sprout/core/sprout_checks/check_fields_not_blank.py @@ -0,0 +1,39 @@ +from seedcase_sprout.core.checks.check_error import CheckError +from seedcase_sprout.core.checks.required_fields import RequiredFieldType +from seedcase_sprout.core.sprout_checks.get_blank_value_for_type import ( + get_blank_value_for_type, +) +from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import ( + get_json_path_to_resource_field, +) + +SPROUT_BLANK_ERROR_MESSAGE = "The '{field_name}' field is blank, please fill it in." + + +def check_fields_not_blank( + properties: dict, fields: dict[str, RequiredFieldType], index: int | None = None +) -> list[CheckError]: + """Checks that fields in `fields` are not blank if they are present. + + Fields not present in `properties` are not checked. + + For resource properties, an index may be supplied, if the resource properties are + part of a set of package properties. + + Args: + properties: The properties where the fields are. + fields: A set of fields and their types. + index: The index of the resource properties. Defaults to None. + + Returns: + A list of errors. An empty list if no errors were found. + """ + return [ + CheckError( + message=SPROUT_BLANK_ERROR_MESSAGE.format(field_name=field), + json_path=get_json_path_to_resource_field(field, index), + validator="blank", + ) + for field, type in fields.items() + if properties.get(field) == get_blank_value_for_type(type) + ] diff --git a/seedcase_sprout/core/sprout_checks/check_fields_present.py b/seedcase_sprout/core/sprout_checks/check_fields_present.py new file mode 100644 index 000000000..cd3a9747f --- /dev/null +++ b/seedcase_sprout/core/sprout_checks/check_fields_present.py @@ -0,0 +1,36 @@ +from seedcase_sprout.core.checks.check_error import CheckError +from seedcase_sprout.core.checks.required_fields import RequiredFieldType +from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import ( + get_json_path_to_resource_field, +) + +CHECKS_REQUIRED_ERROR_MESSAGE = "'{field_name}' is a required property" + + +def check_fields_present( + properties: dict, + required_fields: dict[str, RequiredFieldType], + index: int | None = None, +) -> list[CheckError]: + """Checks that all fields in `required_fields` are present. + + For resource properties, an index may be supplied, if the resource properties are + part of a set of package properties. + + Args: + properties: The properties to check. + required_fields: The set of required fields and their types. + index: The index of the resource properties. Defaults to None. + + Returns: + A list of errors. An empty list if no errors were found. + """ + return [ + CheckError( + message=CHECKS_REQUIRED_ERROR_MESSAGE.format(field_name=field), + json_path=get_json_path_to_resource_field(field, index), + validator="required", + ) + for field in required_fields.keys() + if properties.get(field) is None + ] diff --git a/seedcase_sprout/core/sprout_checks/check_list_item_field_not_blank.py b/seedcase_sprout/core/sprout_checks/check_list_item_field_not_blank.py new file mode 100644 index 000000000..073a5c11c --- /dev/null +++ b/seedcase_sprout/core/sprout_checks/check_list_item_field_not_blank.py @@ -0,0 +1,33 @@ +from seedcase_sprout.core.checks.check_error import CheckError +from seedcase_sprout.core.checks.required_fields import RequiredFieldType +from seedcase_sprout.core.sprout_checks.check_fields_not_blank import ( + SPROUT_BLANK_ERROR_MESSAGE, +) +from seedcase_sprout.core.sprout_checks.get_blank_value_for_type import ( + get_blank_value_for_type, +) + + +def check_list_item_field_not_blank( + properties: dict, list_name: str, field_name: str, field_type=RequiredFieldType.str +) -> list[CheckError]: + """Checks that the specified field of items in a list is not blank. + + Args: + properties: The properties object containing the list. + list_name: The name of the list field. + field_name: The name of the item field. + field_type: The type of the item field. Defaults to str. + + Returns: + A list of errors. An empty list if no errors were found. + """ + return [ + CheckError( + message=SPROUT_BLANK_ERROR_MESSAGE.format(field_name=field_name), + json_path=f"$.{list_name}[{index}].{field_name}", + validator="blank", + ) + for index, item in enumerate(properties.get(list_name, [])) + if item.get(field_name) == get_blank_value_for_type(field_type) + ] diff --git a/seedcase_sprout/core/sprout_checks/check_required_package_properties_not_blank.py b/seedcase_sprout/core/sprout_checks/check_required_package_properties_not_blank.py new file mode 100644 index 000000000..5eb441d6c --- /dev/null +++ b/seedcase_sprout/core/sprout_checks/check_required_package_properties_not_blank.py @@ -0,0 +1,32 @@ +from seedcase_sprout.core.checks.check_error import CheckError +from seedcase_sprout.core.sprout_checks.check_fields_not_blank import ( + check_fields_not_blank, +) +from seedcase_sprout.core.sprout_checks.check_list_item_field_not_blank import ( + check_list_item_field_not_blank, +) +from seedcase_sprout.core.sprout_checks.required_fields import ( + PACKAGE_SPROUT_REQUIRED_FIELDS, +) + + +def check_required_package_properties_not_blank( + properties: dict, +) -> list[CheckError]: + """Checks that required package properties fields are not blank. + + Both Sprout-specific required fields and fields required by the Data Package + standard are checked. + + Args: + properties: The package properties. + + Returns: + A list of errors. An empty list if no errors were found. + """ + errors = check_fields_not_blank(properties, PACKAGE_SPROUT_REQUIRED_FIELDS) + errors += check_list_item_field_not_blank(properties, "contributors", "title") + errors += check_list_item_field_not_blank(properties, "sources", "title") + errors += check_list_item_field_not_blank(properties, "licenses", "name") + errors += check_list_item_field_not_blank(properties, "licenses", "path") + return errors diff --git a/tests/core/sprout_checks/test_check_fields_not_blank.py b/tests/core/sprout_checks/test_check_fields_not_blank.py new file mode 100644 index 000000000..14c2fb9de --- /dev/null +++ b/tests/core/sprout_checks/test_check_fields_not_blank.py @@ -0,0 +1,57 @@ +from pytest import mark + +from seedcase_sprout.core.checks.required_fields import RequiredFieldType +from seedcase_sprout.core.sprout_checks.check_fields_not_blank import ( + check_fields_not_blank, +) +from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import ( + get_json_path_to_resource_field, +) + +FIELDS = {"name": RequiredFieldType.str, "tags": RequiredFieldType.list} + + +@mark.parametrize("index", [None, 2]) +def test_no_error_found_in_properties_with_populated_fields(index): + """Should pass properties with fields populated.""" + properties = {"name": "My name", "tags": ["a", "b"]} + + assert check_fields_not_blank(properties, FIELDS, index) == [] + + +@mark.parametrize("index", [None, 2]) +def test_no_error_found_in_properties_with_fields_missing(index): + """Should pass properties without the specified fields.""" + assert check_fields_not_blank({}, FIELDS, index) == [] + + +@mark.parametrize("index", [None, 2]) +def test_error_found_if_properties_have_a_blank_field(index): + """Should find an error if properties contain a blank field.""" + properties = {"name": "My name", "tags": []} + + errors = check_fields_not_blank(properties, FIELDS, index) + + assert len(errors) == 1 + assert "blank" in errors[0].message + assert errors[0].json_path == get_json_path_to_resource_field("tags", index) + assert errors[0].validator == "blank" + + +@mark.parametrize("index", [None, 2]) +def test_error_found_if_properties_have_multiple_blank_fields(index): + """Should find an error if properties contain multiple blank fields.""" + properties = {"name": "", "tags": []} + + errors = check_fields_not_blank(properties, FIELDS, index) + + assert len(errors) == 2 + assert all(error.validator == "blank" for error in errors) + assert any( + error.json_path == get_json_path_to_resource_field("name", index) + for error in errors + ) + assert any( + error.json_path == get_json_path_to_resource_field("tags", index) + for error in errors + ) diff --git a/tests/core/sprout_checks/test_check_fields_present.py b/tests/core/sprout_checks/test_check_fields_present.py new file mode 100644 index 000000000..4b1637aa7 --- /dev/null +++ b/tests/core/sprout_checks/test_check_fields_present.py @@ -0,0 +1,57 @@ +from pytest import mark + +from seedcase_sprout.core.checks.required_fields import RequiredFieldType +from seedcase_sprout.core.sprout_checks.check_fields_present import ( + check_fields_present, +) +from seedcase_sprout.core.sprout_checks.get_json_path_to_resource_field import ( + get_json_path_to_resource_field, +) + +REQUIRED_FIELDS = {"name": RequiredFieldType.str, "tags": RequiredFieldType.list} + + +@mark.parametrize("index", [None, 2]) +def test_no_error_found_in_properties_with_required_fields(index): + """Should pass properties with required fields present and populated.""" + properties = {"name": "My name", "tags": ["a", "b"]} + + assert check_fields_present(properties, REQUIRED_FIELDS, index) == [] + + +@mark.parametrize("index", [None, 2]) +def test_no_error_found_in_properties_with_required_fields_blank(index): + """Should pass properties with required fields present but blank.""" + properties = {"name": "", "tags": []} + + assert check_fields_present(properties, REQUIRED_FIELDS, index) == [] + + +@mark.parametrize("index", [None, 2]) +def test_error_found_if_there_is_a_missing_required_field(index): + """Should find an error if there is a missing required field.""" + properties = {"name": "My name"} + + errors = check_fields_present(properties, REQUIRED_FIELDS, index) + + assert len(errors) == 1 + assert "required" in errors[0].message + assert errors[0].json_path == get_json_path_to_resource_field("tags", index) + assert errors[0].validator == "required" + + +@mark.parametrize("index", [None, 2]) +def test_error_found_if_there_are_multiple_missing_required_fields(index): + """Should find an error if there are multiple missing required fields.""" + errors = check_fields_present({}, REQUIRED_FIELDS, index) + + assert len(errors) == 2 + assert all(error.validator == "required" for error in errors) + assert any( + error.json_path == get_json_path_to_resource_field("name", index) + for error in errors + ) + assert any( + error.json_path == get_json_path_to_resource_field("tags", index) + for error in errors + ) diff --git a/tests/core/sprout_checks/test_check_list_item_field_not_blank.py b/tests/core/sprout_checks/test_check_list_item_field_not_blank.py new file mode 100644 index 000000000..273cdbe0c --- /dev/null +++ b/tests/core/sprout_checks/test_check_list_item_field_not_blank.py @@ -0,0 +1,74 @@ +from pytest import mark + +from seedcase_sprout.core.checks.required_fields import RequiredFieldType +from seedcase_sprout.core.sprout_checks.check_list_item_field_not_blank import ( + check_list_item_field_not_blank, +) +from seedcase_sprout.core.sprout_checks.get_blank_value_for_type import ( + get_blank_value_for_type, +) + + +def test_no_error_found_in_properties_without_list(): + """Should pass if the properties do not contain the specified list.""" + assert check_list_item_field_not_blank({}, "items", "field") == [] + + +@mark.parametrize("items", [[], [{}, {}], [{"a": 1}, {"a": 2}]]) +def test_no_error_found_when_list_does_not_contain_field(items): + """Should pass if list items do not contain the field.""" + properties = {"items": items} + + assert check_list_item_field_not_blank(properties, "items", "field") == [] + + +def test_no_error_found_when_fields_populated(): + """Should pass if all fields are populated.""" + properties = {"items": [{"field": "value"}, {"field": "value"}]} + + assert check_list_item_field_not_blank(properties, "items", "field") == [] + + +def test_no_error_found_when_fields_are_of_wrong_type(): + """Should pass if the fields are present but of the wrong type.""" + properties = {"items": [{"field": "value"}, {"field": ""}]} + + assert ( + check_list_item_field_not_blank( + properties, "items", "field", RequiredFieldType.list + ) + == [] + ) + + +@mark.parametrize( + "field_type,value", + [(RequiredFieldType.str, "value"), (RequiredFieldType.list, [1])], +) +def test_error_found_if_an_item_has_a_blank_field(field_type, value): + """Should find an error if there is an item with a blank field.""" + properties = { + "items": [{"field": value}, {"field": get_blank_value_for_type(field_type)}] + } + + errors = check_list_item_field_not_blank(properties, "items", "field", field_type) + + assert len(errors) == 1 + assert "blank" in errors[0].message + assert errors[0].json_path == "$.items[1].field" + assert errors[0].validator == "blank" + + +@mark.parametrize("field_type", RequiredFieldType) +def test_error_found_if_multiple_items_have_a_blank_field(field_type): + """Should find an error if there are multiple items with a blank field.""" + properties = {"items": [{"field": get_blank_value_for_type(field_type)}] * 2} + + errors = check_list_item_field_not_blank(properties, "items", "field", field_type) + + assert len(errors) == 2 + assert all( + "blank" in error.message and error.validator == "blank" for error in errors + ) + assert any(error.json_path == "$.items[0].field" for error in errors) + assert any(error.json_path == "$.items[1].field" for error in errors) diff --git a/tests/core/sprout_checks/test_check_required_package_properties_not_blank.py b/tests/core/sprout_checks/test_check_required_package_properties_not_blank.py new file mode 100644 index 000000000..3b727615a --- /dev/null +++ b/tests/core/sprout_checks/test_check_required_package_properties_not_blank.py @@ -0,0 +1,80 @@ +from pytest import fixture, mark + +from seedcase_sprout.core.sprout_checks.check_required_package_properties_not_blank import ( # noqa: E501 + check_required_package_properties_not_blank, +) +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 ( + PACKAGE_SPROUT_REQUIRED_FIELDS, +) + + +@fixture +def properties(): + return { + "name": "package-1", + "id": "abc1", + "title": "Package 1", + "description": "A package.", + "version": "1.0.0", + "created": "2024-05-14T05:00:01+00:00", + "licenses": [{"name": "a-license"}], + "contributors": [{"title": "a contributor"}], + "sources": [{"title": "a source"}], + } + + +def test_no_error_found_if_all_required_fields_populated(properties): + """Should pass if all required fields are present and populated.""" + assert check_required_package_properties_not_blank(properties) == [] + + +def test_no_error_found_if_all_required_fields_missing(): + """Should pass if all required fields are missing.""" + assert check_required_package_properties_not_blank({}) == [] + + +@mark.parametrize( + "field,value,json_path", + [ + *[ + (name, get_blank_value_for_type(type), f"$.{name}") + for name, type in PACKAGE_SPROUT_REQUIRED_FIELDS.items() + ], + ("contributors", [{"title": ""}], "$.contributors[0].title"), + ("sources", [{"title": ""}], "$.sources[0].title"), + ("licenses", [{"name": ""}], "$.licenses[0].name"), + ("licenses", [{"path": ""}], "$.licenses[0].path"), + ], +) +def test_error_found_if_required_field_is_blank(properties, field, value, json_path): + """Should find an error if a required field is present but blank.""" + properties[field] = value + + errors = check_required_package_properties_not_blank(properties) + + assert len(errors) == 1 + assert errors[0].json_path == json_path + assert errors[0].validator == "blank" + + +def test_error_found_if_all_required_fields_are_blank(): + """Should find an error if all required fields are present but blank.""" + properties = { + "name": "", + "id": "", + "title": "", + "description": "", + "version": "", + "created": "", + "licenses": [{"name": "", "path": ""}], + "contributors": [{"title": ""}], + "sources": [{"title": ""}], + } + + errors = check_required_package_properties_not_blank(properties) + + assert len(errors) == 10 + assert all(error.validator == "blank" for error in errors)