Skip to content

Feature/gr ndsp 655 add check for ensuring that mandatory entities have at least 1 valid record #163

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.

Sign up for GitHub

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

Open
georgeRobertson wants to merge 1 commit into
base: release_v010
Choose a base branch
from
Open

Feature/gr ndsp 655 add check for ensuring that mandatory entities have at least 1 valid record #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 11 additions & 4 deletions docs/advanced_guidance/json_schemas/entity_relationships.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
"type": "string"
}
},
"is_root_entity": {
"type": "boolean"
},
"mandatory": {
"type": "boolean"
},
Expand All @@ -31,12 +34,16 @@
},
"no_valid_records_error_message": {
"type": "string"
},
"empty_entity_error_code": {
"type": "string",
"minLength": 1
},
"empty_entity_error_message": {
"type": "string",
"minLength": 1
}
},
"required": [
"parent_entity",
"join_fields"
],
"additionalProperties": false
}
}
Expand Down
15 changes: 15 additions & 0 deletions src/dve/core_engine/configuration/v1/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ class _LinkageConfig(BaseModel):
"Records removed due to no valid parent record"
)
"""The error code to emit if the entity contains records that are orphaned by parent record rejections""" # pylint: disable=C0301
empty_entity_error_code: ErrorCode = "EmptyEntity"
"""The error code to emit if a mandatory entity has no valid remaining records"""
empty_entity_error_message: ErrorMessage = "no valid records remaining"
"""The error message to emit if a mandatory entity has no valid remaining records"""

@model_validator(mode="after")
def _check_root_no_parent_or_join_keys(self):
Expand All @@ -123,6 +127,17 @@ def _check_root_no_parent_or_join_keys(self):
)
return self

@model_validator(mode="after")
def _check_non_root_entities_have_a_defined_parent(self):
"""Check that non root entities have a parent defined."""
if not self.is_root_entity and self.parent_entity is None:
raise ValueError(
'Non-root entity has no defined parent entity. If you intend this to be a root ' \
'entity you must specify `"is_root_entity": true` for the entity. ' \
'Otherwise you must specify a `"parent_entity": "<EntityName>"` for this entity.'
)
return self

@model_validator(mode="after")
def _check_root_mandatory(self):
if self.is_root_entity and not self.mandatory:
Expand Down
46 changes: 44 additions & 2 deletions src/dve/core_engine/configuration/v1/hierarchy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import json
from typing import Any, Iterable, Optional, Union

from pydantic import BaseModel, Field
from pydantic import BaseModel, Field, model_validator

from dve.core_engine.configuration.v1 import V1EngineConfig, _LinkageConfig
from dve.core_engine.type_hints import EntityName, ErrorCode, ErrorMessage
Expand All @@ -26,6 +26,19 @@ class HierarchyNode(BaseModel):
missing_parent_id_error_message: Optional[ErrorMessage] = (
"Records removed due to no valid parent record"
)
empty_entity_error_code: ErrorCode = "EmptyEntity"
empty_entity_error_message: ErrorMessage = "no valid records remaining"

@model_validator(mode="after")
def validate_empty_error_details(self):
"""
Removes the default messaging for checking empty entities as not performed on
non mandatory nodes/entities
"""
if not self.mandatory:
self.empty_entity_error_code = None
self.empty_entity_error_message = None
return self

def get_descendents(self) -> list["HierarchyNode"]:
"""Recursively list all descendents of the node"""
Expand Down Expand Up @@ -129,12 +142,15 @@ def determine_trees(

for name, linkage_detail in entity_relationships.items():
for main_entity, parent_node in top_level_parents.items():
if linkage_detail.is_root_entity:
break

if (
linkage_detail.parent_entity == main_entity
or linkage_detail.parent_entity in parent_node.get_descendent_names()
):
parent_node.add_child_node(
linkage_detail.parent_entity,
linkage_detail.parent_entity, # type: ignore
HierarchyNode(entity_name=name, **linkage_detail.model_dump()),
)
break
Expand Down Expand Up @@ -166,3 +182,29 @@ def from_engine_config(cls, engine_config: V1EngineConfig):
entity_relationships=engine_config.entity_relationships,
)
)

def get_all_mandatory_nodes(
self,
node: Optional[HierarchyNode] = None,
mandatory_nodes: Optional[list[HierarchyNode]] = None,
nodes_visited: Optional[set[EntityName]] = None,
) -> list[HierarchyNode]:
"""Find and return all mandatory nodes"""
if mandatory_nodes is None:
mandatory_nodes = []

if nodes_visited is None:
nodes_visited = set()

if node is None:
for _node in self.entity_trees.values():
self.get_all_mandatory_nodes(_node, mandatory_nodes, nodes_visited)

if node:
if node.mandatory and node.entity_name not in nodes_visited:
nodes_visited.add(node.entity_name)
mandatory_nodes.append(node)
for child_node in node.children:
self.get_all_mandatory_nodes(child_node, mandatory_nodes, nodes_visited)

return mandatory_nodes
8 changes: 7 additions & 1 deletion src/dve/core_engine/type_hints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,13 @@
FieldValue = Optional[Any]
"""The value that caused the error."""
ErrorCategory = Literal[
"Blank", "Wrong format", "Bad value", "Bad file", "Parent Missing", "Children missing"
"Blank",
"Wrong format",
"Bad value",
"Bad file",
"Parent Missing",
"Children missing",
"Empty entity",
]
"""A string indicating the category of the error."""
RecordIndex = Optional[int]
Expand Down
43 changes: 43 additions & 0 deletions src/dve/pipeline/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import dve.reporting.excel_report as er
from dve.common.error_utils import (
BackgroundMessageWriter,
dump_feedback_errors,
dump_processing_errors,
get_feedback_errors_uri,
Expand Down Expand Up @@ -545,7 +546,45 @@

return processed_files, failed_processing

def check_mandatory_entities_have_records(
self,
working_directory: URI,
entities: EntityManager,
entity_hierarchy: EntityHierarchy,
key_fields: Optional[dict[str, list[str]]] = None,
) -> None:
"""
Check that mandatory entities have at least one record post business rules. Otherwise,
raise a submission rejection error message.
"""
with BackgroundMessageWriter(
working_directory=working_directory,
dve_stage="business_rules",
key_fields=key_fields,
logger=self._logger,
) as msg_writer:
_msgs = []
for node in entity_hierarchy.get_all_mandatory_nodes():
entity_name = node.entity_name
if node.mandatory and self.get_entity_count(entities[entity_name]) == 0:
self._logger.info(
f"Found 0 records in mandatory entity {entity_name} after applying all business rules" # pylint: disable=C0301
)
_msgs.append(
FeedbackMessage(
entity=entity_name,
record=None,
error_location=entity_name,
error_message=node.empty_entity_error_message,
failure_type="submission",
error_type="submission",
error_code=node.empty_entity_error_code,
category="Empty entity",
)
)
msg_writer.write_queue.put(_msgs)

def apply_business_rules( # pylint: disable=R0914,R0915

Check failure on line 587 in src/dve/pipeline/pipeline.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed. 


See more on https://sonarcloud.io/project/issues?id=NHSDigital_data-validation-engine&issues=AaDZZaKiz5PIa1mbuT2c&open=AaDZZaKiz5PIa1mbuT2c&pullRequest=163
self, submission_info: SubmissionInfo, submission_status: Optional[SubmissionStatus] = None
) -> tuple[SubmissionInfo, SubmissionStatus]:
"""Apply the business rules to a given submission, the submission may have failed at the
Expand Down Expand Up @@ -667,12 +706,12 @@

entity_issues: dict[EntityName, bool] = {
entity: any(
val
for val in (
orph_issues_1.get(entity, False),
grp_issues_1.get(entity, False),
orph_issues_2.get(entity, False),
)

Check warning on line 714 in src/dve/pipeline/pipeline.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace this comprehension with passing the iterable to the collection constructor call 


See more on https://sonarcloud.io/project/issues?id=NHSDigital_data-validation-engine&issues=AaDZZaKiz5PIa1mbuT2d&open=AaDZZaKiz5PIa1mbuT2d&pullRequest=163
)
for entity in orph_issues_1.keys()
}
Expand Down Expand Up @@ -725,6 +764,10 @@
)
)

self.check_mandatory_entities_have_records(
working_directory, entity_manager, entity_hierarchy
)

submission_status.number_of_records = self.get_entity_count(
entity=entity_manager.entities[f"""Original{rules.global_variables.get(
'entity',
Expand Down
39 changes: 24 additions & 15 deletions tests/features/flights.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,14 @@ Feature: Pipeline tests using the flights dataset
| record | CountryIdIsMissing | 1 |
When I run the business rules phase
Then there are errors with the following details and associated error_count from the business_rules phase
| ErrorType | ErrorCode | error_count |
| record | AirportHasNoCountry | 3 |
| record | StaffHasNoAirport | 15 |
| record | FlightHasNoAirport | 10 |
| record | PassengerHasNoFlight | 25 |
| ErrorType | ErrorCode | error_count |
| record | AirportHasNoCountry | 3 |
| record | StaffHasNoAirport | 15 |
| record | FlightHasNoAirport | 10 |
| record | PassengerHasNoFlight | 25 |
| submission | NoValidCountries | 1 |
| submission | NoValidAirports | 1 |
| submission | NoValidStaff | 1 |
And the final entities have the following row counts
| entity_name | row_count |
| country | 0 |
Expand All @@ -73,7 +76,7 @@ Feature: Pipeline tests using the flights dataset
And The statistics entry for the submission shows the following information
| parameter | value |
| record_count | 1 |
| number_submission_rejections | 0 |
| number_submission_rejections | 3 |
| number_record_rejections | 54 |
| number_warnings | 0 |

Expand Down Expand Up @@ -129,8 +132,11 @@ Feature: Pipeline tests using the flights dataset
And there are no record rejections from the data_contract phase
When I run the business rules phase
Then there are errors with the following details and associated error_count from the business_rules phase
| ErrorType | ErrorCode | error_count |
| record | CountryHasNoAirport | 1 |
| ErrorType | ErrorCode | error_count |
| record | CountryHasNoAirport | 1 |
| submission | NoValidCountries | 1 |
| submission | NoValidAirports | 1 |
| submission | NoValidStaff | 1 |
And the final entities have the following row counts
| entity_name | row_count |
| country | 0 |
Expand All @@ -143,7 +149,7 @@ Feature: Pipeline tests using the flights dataset
And The statistics entry for the submission shows the following information
| parameter | value |
| record_count | 1 |
| number_submission_rejections | 0 |
| number_submission_rejections | 3 |
| number_record_rejections | 1 |
| number_warnings | 0 |

Expand All @@ -163,11 +169,14 @@ Feature: Pipeline tests using the flights dataset
And there are no record rejections from the data_contract phase
When I run the business rules phase
Then there are errors with the following details and associated error_count from the business_rules phase
| ErrorType | Status | ErrorCode | error_count |
| record | error | InvalidFlightDestination | 2 |
| record | error | PassengerHasNoFlight | 4 |
| record | error | AirportHasNoStaff | 1 |
| record | error | CountryHasNoAirport | 1 |
| ErrorType | Status | ErrorCode | error_count |
| record | error | InvalidFlightDestination | 2 |
| record | error | PassengerHasNoFlight | 4 |
| record | error | AirportHasNoStaff | 1 |
| record | error | CountryHasNoAirport | 1 |
| submission | error | NoValidCountries | 1 |
| submission | error | NoValidAirports | 1 |
| submission | error | NoValidStaff | 1 |
And the final entities have the following row counts
| entity_name | row_count |
| country | 0 |
Expand All @@ -180,7 +189,7 @@ Feature: Pipeline tests using the flights dataset
And The statistics entry for the submission shows the following information
| parameter | value |
| record_count | 1 |
| number_submission_rejections | 0 |
| number_submission_rejections | 3 |
| number_record_rejections | 8 |
| number_warnings | 0 |

Expand Down
26 changes: 25 additions & 1 deletion tests/test_core_engine/test_hierarchy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "parent record removed as no valid child records",
"missing_parent_id_error_code": null,
"missing_parent_id_error_message": null,
"empty_entity_error_code": null,
"empty_entity_error_message": null,
"children": {
"ds_003": {
"parent_entity": "ds_001",
Expand All @@ -280,6 +282,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "parent record removed as no valid child records",
"missing_parent_id_error_code": "DS003NoParent",
"missing_parent_id_error_message": "record removed as no parent",
"empty_entity_error_code": null,
"empty_entity_error_message": null,
"children": {}
},
"ds_101": {
Expand All @@ -292,6 +296,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "{{ ds_001_id }} removed as no valid ds_101 records",
"missing_parent_id_error_code": "DS101NoParent",
"missing_parent_id_error_message": "record removed as no parent",
"empty_entity_error_code": null,
"empty_entity_error_message": null,
"children": {
"ds_201": {
"parent_entity": "ds_101",
Expand All @@ -303,6 +309,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "parent record removed as no valid child records",
"missing_parent_id_error_code": "DS201NoParent",
"missing_parent_id_error_message": "record removed as no parent",
"empty_entity_error_code": null,
"empty_entity_error_message": null,
"children": {
"ds_202": {
"parent_entity": "ds_201",
Expand All @@ -314,6 +322,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "parent record removed as no valid child records",
"missing_parent_id_error_code": "MissingParentRecord",
"missing_parent_id_error_message": "Records removed due to no valid parent record",
"empty_entity_error_code": "EmptyEntity",
"empty_entity_error_message": "no valid records remaining",
"children": {}
}
}
Expand Down Expand Up @@ -341,6 +351,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "{{ ds_001_id }} removed as no valid ds_101 records",
"missing_parent_id_error_code": "DS101NoParent",
"missing_parent_id_error_message": "record removed as no parent",
"empty_entity_error_code": null,
"empty_entity_error_message": null,
"children": {
"ds_201": {
"parent_entity": "ds_101",
Expand All @@ -352,6 +364,8 @@ def test_linkage_config_load():
"no_valid_records_error_message": "parent record removed as no valid child records",
"missing_parent_id_error_code": "DS201NoParent",
"missing_parent_id_error_message": "record removed as no parent",
"empty_entity_error_code": null,
"empty_entity_error_message": null,
"children": {
"ds_202": {
"parent_entity": "ds_201",
Expand All @@ -363,10 +377,20 @@ def test_linkage_config_load():
"no_valid_records_error_message": "parent record removed as no valid child records",
"missing_parent_id_error_code": "MissingParentRecord",
"missing_parent_id_error_message": "Records removed due to no valid parent record",
"empty_entity_error_code": "EmptyEntity",
"empty_entity_error_message": "no valid records remaining",
"children": {}
}
}
}
}
}""")



def test_get_all_mandatory_nodes():
with NamedTemporaryFile("w") as tmp:
tmp.write(CONFIG_WITH_LINKAGE)
tmp.flush()
hierarchy = EntityHierarchy.from_dischema(tmp.name)

assert len(hierarchy.get_all_mandatory_nodes()) == 1
Loading
Loading