From add81106f192c4286dbb57451c4be49c55881559 Mon Sep 17 00:00:00 2001 From: Graham Lee Date: Thu, 28 Jul 2022 10:40:24 +0100 Subject: [PATCH] Add robust validation for list-type fields #2714 --- .../data_service/day_zero_fields.json | 15 +++++++++++++++ .../data_service/model/document.py | 16 +++++++++++----- .../data_service/model/field.py | 12 +++++++++--- .../tests/data/case.excluded.json | 3 ++- .../tests/data/case.minimal.json | 3 ++- .../tests/data/case.with_location.json | 4 +++- .../tests/test_case_model.py | 16 ++++++++++++++++ 7 files changed, 58 insertions(+), 11 deletions(-) diff --git a/data-serving/reusable-data-service/data_service/day_zero_fields.json b/data-serving/reusable-data-service/data_service/day_zero_fields.json index f10a1152e..e095ccc7b 100644 --- a/data-serving/reusable-data-service/data_service/day_zero_fields.json +++ b/data-serving/reusable-data-service/data_service/day_zero_fields.json @@ -53,6 +53,21 @@ "other" ] }, + { + "key": "gender", + "type": "string", + "data_dictionary_text": "Gender identity of the individual; can select multiple options.", + "required": true, + "values": [ + "man", + "woman", + "transgender", + "non-binary", + "other", + "unknown" + ], + "is_list": true + }, { "key": "confirmationDate", "type": "date", diff --git a/data-serving/reusable-data-service/data_service/model/document.py b/data-serving/reusable-data-service/data_service/model/document.py index d9b7eb32e..bd4340ba1 100644 --- a/data-serving/reusable-data-service/data_service/model/document.py +++ b/data-serving/reusable-data-service/data_service/model/document.py @@ -127,7 +127,7 @@ def field_values(self) -> List[str]: fields += value.custom_field_values() else: fields += f.type.custom_none_field_values() - elif issubclass(f.type, list): + elif isinstance(value, list): fields.append(",".join(value)) else: fields.append(str(value) if value is not None else "") @@ -203,11 +203,17 @@ def validate(self): raise ValidationError(f"{field.key} must have a value") if field.key in self.document_fields() and value is not None: getter(self).validate() + if field.is_list: + for element in value: + if not isinstance(element, field.element_type()): + raise ValidationError(f"{field.key} member {element} is of the wrong type") if field.values is not None: - if value is not None and value not in field.values: - raise ValidationError( - f"{field.key} value {value} not in permissible values {field.values}" - ) + test_collection = value if field.is_list is True else [value] + for a_value in test_collection: + if a_value is not None and a_value not in field.values: + raise ValidationError( + f"{field.key} value {a_value} not in permissible values {field.values}" + ) def _internal_set_value(self, key, value): self._internal_ensure_containers_exist(key) diff --git a/data-serving/reusable-data-service/data_service/model/field.py b/data-serving/reusable-data-service/data_service/model/field.py index 39d251703..746f6a765 100644 --- a/data-serving/reusable-data-service/data_service/model/field.py +++ b/data-serving/reusable-data-service/data_service/model/field.py @@ -12,13 +12,16 @@ @dataclasses.dataclass class Field(Document): - """Represents a custom field in a Document object.""" + """Represents a custom field in a Document object. + Note for future architects: I learned today that dataclasses fields can carry + custom metadata. It might make sense to reorganise things so that information + currently in the Field class is carried in that metadata instead.""" key: str = dataclasses.field(init=True, default=None) type: str = dataclasses.field(init=True, default=None) data_dictionary_text: str = dataclasses.field(init=True, default=None) required: bool = dataclasses.field(init=True, default=False) - default: Optional[Union[bool, str, int, date]] = dataclasses.field( + default: Optional[Union[bool, str, int, date, Feature, AgeRange, CaseReference, CaseExclusionMetadata]] = dataclasses.field( init=True, default=None ) values: Optional[List[Any]] = dataclasses.field(init=True, default=None) @@ -63,7 +66,10 @@ def python_type(self) -> type: if self.is_list: return list else: - return self.model_type(self.type) + return self.element_type() + + def element_type(self) -> type: + return self.model_type(self.type) def dataclasses_tuples(self): # Note that the default value here is always None, even if I have a default value! diff --git a/data-serving/reusable-data-service/tests/data/case.excluded.json b/data-serving/reusable-data-service/tests/data/case.excluded.json index 128010f18..baba5a6c1 100644 --- a/data-serving/reusable-data-service/tests/data/case.excluded.json +++ b/data-serving/reusable-data-service/tests/data/case.excluded.json @@ -9,5 +9,6 @@ }, "caseStatus": "omit_error", "pathogenStatus": "endemic", - "sexAtBirth": "female" + "sexAtBirth": "female", + "gender": ["woman"] } \ No newline at end of file diff --git a/data-serving/reusable-data-service/tests/data/case.minimal.json b/data-serving/reusable-data-service/tests/data/case.minimal.json index f80913996..c440e6377 100644 --- a/data-serving/reusable-data-service/tests/data/case.minimal.json +++ b/data-serving/reusable-data-service/tests/data/case.minimal.json @@ -5,5 +5,6 @@ }, "caseStatus": "probable", "pathogenStatus": "emerging", - "sexAtBirth": "female" + "sexAtBirth": "female", + "gender": [] } \ No newline at end of file diff --git a/data-serving/reusable-data-service/tests/data/case.with_location.json b/data-serving/reusable-data-service/tests/data/case.with_location.json index 9e74cb35d..2735b6263 100644 --- a/data-serving/reusable-data-service/tests/data/case.with_location.json +++ b/data-serving/reusable-data-service/tests/data/case.with_location.json @@ -18,5 +18,7 @@ }, "caseStatus": "probable", "pathogenStatus": "unknown", - "sexAtBirth": "female" + "sexAtBirth": "female", + "gender": ["non-binary", "other"], + "gender_other": "womxn" } \ No newline at end of file diff --git a/data-serving/reusable-data-service/tests/test_case_model.py b/data-serving/reusable-data-service/tests/test_case_model.py index b886740d0..2985714dc 100644 --- a/data-serving/reusable-data-service/tests/test_case_model.py +++ b/data-serving/reusable-data-service/tests/test_case_model.py @@ -86,3 +86,19 @@ def test_apply_update_that_unsets_value(): update = DocumentUpdate.from_dict({"confirmationDate": None}) case.apply_update(update) assert case.confirmationDate is None + + +def test_cannot_put_wrong_type_in_list(): + with open("./tests/data/case.minimal.json", "r") as minimal_file: + case = Case.from_json(minimal_file.read()) + case.gender = ["man", True] + with pytest.raises(ValidationError): + case.validate() + + +def test_list_elements_must_come_from_acceptable_values(): + with open("./tests/data/case.minimal.json", "r") as minimal_file: + case = Case.from_json(minimal_file.read()) + case.gender = ["woman", "dalek"] + with pytest.raises(ValidationError): + case.validate()