From 9b3858189615d3082b4f6c2cd1a67f6c1cf7ca80 Mon Sep 17 00:00:00 2001 From: Viraj Patel <144226112+VirajP1002@users.noreply.github.com> Date: Thu, 18 Jan 2024 09:38:56 +0000 Subject: [PATCH] Update validator to support floats in min/max and disallow string types (#230) * Ensure validator checks that min/max values are not strings * Add validation to ensure that numbers and floats in min_value or max_value are not declared as strings and create test cases * Update test case definition * Format python files * Fix test * Update validation logic * Add Number type to JSON definitions for Validator to accept floats * Format python tests * Update description * Update number validator to account for value sources and update types.py to check for the types of Metadata value sources * Format python files and fix broken tests * Reformat python to pass linting * Add boolean to types accepted for metadata values * Format python * Add additional boolean valid type for operators * Rename error message variable and add Decimal into values accepted for min/max * Format files * Update definitions to allow any_value type to be booleans and update tests * Parametrize tests, add additional test cases and refactor types.py and number_answer_validator.py * Refactor method in types.py * Refactor number_answer_validator and add type hints to types.py * Add metadata of types url/uuid in test_valid_metadata.json and add new test cases to reflect that these types are allowed * Format files * Update when rule to use a different source rather than answers --- .../answers/number_answer_validator.py | 24 ++++++ app/validators/routing/types.py | 82 ++++++++++++++----- app/validators/rules/rule_validator.py | 9 +- schemas/common_definitions.json | 4 +- schemas/rules/definitions.json | 4 +- .../rules/boolean/comparison/equal.json | 12 +++ tests/schemas/valid/test_valid_metadata.json | 27 ++++++ .../answers/test_number_answer_validator.py | 66 +++++++++++++++ tests/validators/routing/test_types.py | 22 +++++ .../routing/test_when_rule_validator.py | 10 ++- 10 files changed, 235 insertions(+), 25 deletions(-) diff --git a/app/validators/answers/number_answer_validator.py b/app/validators/answers/number_answer_validator.py index c99e9258..b618ad6a 100644 --- a/app/validators/answers/number_answer_validator.py +++ b/app/validators/answers/number_answer_validator.py @@ -1,4 +1,7 @@ +from decimal import Decimal + from app.validators.answers.answer_validator import AnswerValidator +from app.validators.routing.types import TYPE_NUMBER, resolve_value_source_json_type MAX_NUMBER = 999_999_999_999_999 MIN_NUMBER = -999_999_999_999_999 @@ -21,6 +24,9 @@ class NumberAnswerValidator(AnswerValidator): GREATER_DECIMALS_ON_ANSWER_REFERENCE = ( "The referenced answer has a greater number of decimal places than answer" ) + MIN_OR_MAX_IS_NOT_NUMERIC = ( + "The minimum or maximum value is not a float or an integer" + ) def __init__(self, schema_element, questionnaire_schema): super().__init__(schema_element, questionnaire_schema) @@ -29,6 +35,12 @@ def __init__(self, schema_element, questionnaire_schema): def validate(self): super().validate() + self.validate_min_max_is_number() + + # Prevent other validation methods that requires calculations running into errors due to types + if self.errors: + return self.errors + self.validate_decimal_places() self.validate_mandatory_has_no_default() self.validate_decimals() @@ -58,6 +70,18 @@ def validate_mandatory_has_no_default(self): if self.answer.get("mandatory") and self.answer.get("default") is not None: self.add_error(self.DEFAULT_ON_MANDATORY) + def validate_min_max_is_number(self): + for min_max in ["minimum", "maximum"]: + if value := self.answer.get(min_max, {}).get("value", 0): + if isinstance(value, dict): + if ( + resolve_value_source_json_type(value, self.questionnaire_schema) + != TYPE_NUMBER + ): + self.add_error(self.MIN_OR_MAX_IS_NOT_NUMERIC) + elif not isinstance(value, int | float | Decimal): + self.add_error(self.MIN_OR_MAX_IS_NOT_NUMERIC) + def validate_value_in_limits(self): min_value = self.answer.get("minimum", {}).get("value", 0) max_value = self.answer.get("maximum", {}).get("value", 0) diff --git a/app/validators/routing/types.py b/app/validators/routing/types.py index 6ae089b8..14a1bd9a 100644 --- a/app/validators/routing/types.py +++ b/app/validators/routing/types.py @@ -1,3 +1,5 @@ +from app.validators.questionnaire_schema import QuestionnaireSchema + TYPE_STRING = "string" TYPE_NUMBER = "number" TYPE_ARRAY = "array" @@ -42,36 +44,78 @@ "NoneType": TYPE_NULL, } +METADATA_TYPE_TO_JSON_TYPE = { + "string": TYPE_STRING, + "date": TYPE_STRING, + "boolean": TYPE_BOOLEAN, + "uuid": TYPE_STRING, + "url": TYPE_STRING, +} + + +def resolve_answer_source_json_type(answer_id: str, schema: QuestionnaireSchema) -> str: + answer_type = schema.answers_with_context[answer_id]["answer"]["type"] + return ANSWER_TYPE_TO_JSON_TYPE[answer_type] + + +def resolve_calculated_summary_source_json_type( + block_id: str, schema: QuestionnaireSchema +) -> str: + block = schema.get_block(block_id) + if block["calculation"].get("answers_to_calculate"): + answer_id = block["calculation"]["answers_to_calculate"][0] + else: + answer_value_source = block["calculation"]["operation"]["+"][0] + answer_id = answer_value_source["identifier"] + answer_type = schema.answers_with_context[answer_id]["answer"]["type"] + return ANSWER_TYPE_TO_JSON_TYPE[answer_type] + + +def resolve_grand_calculated_summary_source_json_type( + block_id: str, schema: QuestionnaireSchema +) -> str: + block = schema.get_block(block_id) + first_calculated_summary_source = block["calculation"]["operation"]["+"][0] + return resolve_value_source_json_type(first_calculated_summary_source, schema) -def resolve_value_source_json_type(value_source, schema): + +def resolve_metadata_source_json_type( + identifier: str | None, schema: QuestionnaireSchema +) -> str: + if identifier: + for values in schema.schema.get("metadata", []): + if values.get("name") == identifier: + return METADATA_TYPE_TO_JSON_TYPE[values.get("type")] + return TYPE_STRING + + +def resolve_list_source_json_type(selector: str | None) -> str: + return LIST_SELECTOR_TO_JSON_TYPE[selector] if selector else TYPE_ARRAY + + +def resolve_value_source_json_type( + value_source: dict[str, str], schema: QuestionnaireSchema +) -> str: source = value_source["source"] + identifier = value_source.get("identifier") + selector = value_source.get("selector") if source == "answers": - answer_id = value_source["identifier"] - answer_type = schema.answers_with_context[answer_id]["answer"]["type"] - return ANSWER_TYPE_TO_JSON_TYPE[answer_type] + return resolve_answer_source_json_type(identifier, schema) if source == "calculated_summary": - block = schema.get_block(value_source["identifier"]) - if block["calculation"].get("answers_to_calculate"): - answer_id = block["calculation"]["answers_to_calculate"][0] - else: - answer_value_source = block["calculation"]["operation"]["+"][0] - answer_id = answer_value_source["identifier"] - answer_type = schema.answers_with_context[answer_id]["answer"]["type"] - return ANSWER_TYPE_TO_JSON_TYPE[answer_type] + return resolve_calculated_summary_source_json_type(identifier, schema) if source == "grand_calculated_summary": - block = schema.get_block(value_source["identifier"]) - first_calculated_summary_source = block["calculation"]["operation"]["+"][0] - return resolve_value_source_json_type(first_calculated_summary_source, schema) + return resolve_grand_calculated_summary_source_json_type(identifier, schema) + + if source == "metadata": + return resolve_metadata_source_json_type(identifier, schema) if source == "list": - if selector := value_source.get("selector"): - return LIST_SELECTOR_TO_JSON_TYPE[selector] - return TYPE_ARRAY + return resolve_list_source_json_type(selector) return TYPE_STRING -def python_type_to_json_type(python_type): +def python_type_to_json_type(python_type: str) -> str: return PYTHON_TYPE_TO_JSON_TYPE[python_type] diff --git a/app/validators/rules/rule_validator.py b/app/validators/rules/rule_validator.py index 3d5b3b06..88fcdbf8 100644 --- a/app/validators/rules/rule_validator.py +++ b/app/validators/rules/rule_validator.py @@ -346,7 +346,14 @@ def _validate_comparison_operator_argument_types( @staticmethod def _get_valid_types_for_operator(operator_name, argument_position): if operator_name in [Operator.EQUAL, Operator.NOT_EQUAL]: - return [TYPE_DATE, TYPE_NUMBER, TYPE_STRING, TYPE_NULL, TYPE_ARRAY] + return [ + TYPE_DATE, + TYPE_NUMBER, + TYPE_STRING, + TYPE_NULL, + TYPE_ARRAY, + TYPE_BOOLEAN, + ] if operator_name in [ Operator.LESS_THAN, diff --git a/schemas/common_definitions.json b/schemas/common_definitions.json index c86f2f8c..8d435643 100755 --- a/schemas/common_definitions.json +++ b/schemas/common_definitions.json @@ -248,8 +248,8 @@ "value_reference": { "oneOf": [ { - "description": "A string or integer value", - "type": ["integer", "string"] + "description": "A string or number value", + "type": ["string", "number"] }, { "$ref": "value_sources.json#/answer_value_source" diff --git a/schemas/rules/definitions.json b/schemas/rules/definitions.json index 8821423a..ece68300 100644 --- a/schemas/rules/definitions.json +++ b/schemas/rules/definitions.json @@ -111,7 +111,7 @@ "description": "Any value", "oneOf": [ { - "type": ["number", "string", "array", "null"] + "type": ["number", "string", "array", "null", "boolean"] }, { "$ref": "../value_sources.json#/any_value_source" @@ -122,7 +122,7 @@ "description": "Any value", "oneOf": [ { - "type": ["number", "string", "null"] + "type": ["number", "string", "null", "boolean"] }, { "$ref": "../value_sources.json#/any_value_source" diff --git a/tests/schemas/rules/boolean/comparison/equal.json b/tests/schemas/rules/boolean/comparison/equal.json index b4569f4c..799db88c 100644 --- a/tests/schemas/rules/boolean/comparison/equal.json +++ b/tests/schemas/rules/boolean/comparison/equal.json @@ -34,5 +34,17 @@ null ] } + }, + { + "description": "Compare metadata to a boolean", + "rule": { + "==": [ + { + "source": "metadata", + "identifier": "test" + }, + true + ] + } } ] diff --git a/tests/schemas/valid/test_valid_metadata.json b/tests/schemas/valid/test_valid_metadata.json index 2566676c..fa499b99 100644 --- a/tests/schemas/valid/test_valid_metadata.json +++ b/tests/schemas/valid/test_valid_metadata.json @@ -26,6 +26,22 @@ { "name": "test_metadata", "type": "string" + }, + { + "name": "flag_1", + "type": "boolean" + }, + { + "name": "ref_p_start_date", + "type": "date" + }, + { + "name": "case_id", + "type": "uuid" + }, + { + "name": "test_url", + "type": "url" } ], "questionnaire_flow": { @@ -41,6 +57,17 @@ "id": "group3", "blocks": [ { + "skip_conditions": { + "when": { + "==": [ + { + "identifier": "flag_1", + "source": "metadata" + }, + true + ] + } + }, "id": "block4", "type": "Question", "question": { diff --git a/tests/validators/answers/test_number_answer_validator.py b/tests/validators/answers/test_number_answer_validator.py index d7185d5f..ba37ae18 100644 --- a/tests/validators/answers/test_number_answer_validator.py +++ b/tests/validators/answers/test_number_answer_validator.py @@ -1,3 +1,5 @@ +import pytest + from app.validators.answers import NumberAnswerValidator from app.validators.questionnaire_schema import ( MAX_NUMBER, @@ -80,6 +82,70 @@ def test_are_decimal_places_valid(): } +@pytest.mark.parametrize( + "bounds, error_count", + [ + ({"minimum": {"value": "100"}}, 1), + ({"maximum": {"value": "0"}}, 1), + ({"maximum": {"value": "100"}, "minimum": {"value": "0"}}, 2), + ], +) +def test_invalid_min_or_max_is_string(bounds, error_count): + answer = { + "calculated": True, + "description": "The total percentages should be 100%", + "id": "total-percentage", + "label": "Total", + "mandatory": False, + "q_code": "10002", + "type": "Percentage", + **bounds, + } + + validator = NumberAnswerValidator( + answer, get_mock_schema_with_data_version("0.0.3") + ) + validator.validate_min_max_is_number() + + assert validator.errors[0] == { + "message": validator.MIN_OR_MAX_IS_NOT_NUMERIC, + "answer_id": "total-percentage", + } + assert len(validator.errors) == error_count + + +def test_valid_minimum_value_is_float(): + answer = { + "id": "answer-2", + "mandatory": True, + "type": "Currency", + "label": "Money spent on vegetables", + "description": "Enter the full value", + "minimum": {"value": 0.00, "exclusive": True}, + } + + validator = NumberAnswerValidator( + answer, get_mock_schema_with_data_version("0.0.3") + ) + + validator.validate_min_max_is_number() + + assert len(validator.errors) == 0 + + +def test_valid_max_if_numeric_value_source(): + filename = "schemas/valid/test_calculated_summary.json" + schema = QuestionnaireSchema(_open_and_load_schema_file(filename)) + + answer = schema.get_answer("set-maximum-answer") + + validator = NumberAnswerValidator(answer, schema) + + validator.validate_min_max_is_number() + + assert len(validator.errors) == 0 + + def test_invalid_range(): answer = { "id": "answer-3", diff --git a/tests/validators/routing/test_types.py b/tests/validators/routing/test_types.py index 5c2375fd..ddbe44ed 100644 --- a/tests/validators/routing/test_types.py +++ b/tests/validators/routing/test_types.py @@ -98,6 +98,28 @@ def test_resolve_grand_calculated_summary_value_source_json_type(): ) +@pytest.mark.parametrize( + "metadata_value_source, json_type", + [ + ({"source": "metadata", "identifier": "ru_name"}, TYPE_STRING), + ({"identifier": "flag_1", "source": "metadata"}, TYPE_BOOLEAN), + ({"identifier": "ref_p_start_date", "source": "metadata"}, TYPE_STRING), + ({"identifier": "case_id", "source": "metadata"}, TYPE_STRING), + ({"identifier": "test_url", "source": "metadata"}, TYPE_STRING), + ], +) +def test_resolve_metadata_summary_value_source_json_type( + metadata_value_source, json_type +): + filename = "schemas/valid/test_valid_metadata.json" + questionnaire_schema = QuestionnaireSchema(_open_and_load_schema_file(filename)) + + assert ( + resolve_value_source_json_type(metadata_value_source, questionnaire_schema) + == json_type + ) + + @pytest.mark.parametrize( "source, selector, json_type", [ diff --git a/tests/validators/routing/test_when_rule_validator.py b/tests/validators/routing/test_when_rule_validator.py index 78c60b1f..e14c1dc7 100644 --- a/tests/validators/routing/test_when_rule_validator.py +++ b/tests/validators/routing/test_when_rule_validator.py @@ -2,6 +2,7 @@ from app.validators.routing.types import ( TYPE_ARRAY, + TYPE_BOOLEAN, TYPE_DATE, TYPE_NULL, TYPE_NUMBER, @@ -152,7 +153,14 @@ def test_comparison_operator_invalid_argument_types(operator_name): validator.validate() if operator_name in ["==", "!="]: - valid_types = [TYPE_DATE, TYPE_NUMBER, TYPE_STRING, TYPE_NULL, TYPE_ARRAY] + valid_types = [ + TYPE_DATE, + TYPE_NUMBER, + TYPE_STRING, + TYPE_NULL, + TYPE_ARRAY, + TYPE_BOOLEAN, + ] else: valid_types = [TYPE_DATE, TYPE_NUMBER]