Conversation
sambit-mishra-NHSD
requested review from
ayeshalshukri1-nhs,
Copilot and
robbailiff2
There was a problem hiding this comment.
Pull request overview
This PR tightens campaign/iteration rules validation by ensuring IterationRules[].CohortLabel values are restricted to the cohort labels defined in IterationCohorts, and updates unit tests to reflect the new validation and clean up redundant exception-handling patterns.
Changes:
- Add iteration-level validation to reject rules that reference cohort labels not present in
IterationCohorts. - Fix a unit test input key to use the correct campaign-config alias (
IterationTime). - Simplify several “mandatory fields” tests by removing redundant
try/except+pytest.failwrappers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/rules_validation_api/validators/iteration_validator.py |
Adds a new model-level validator enforcing rule cohort labels are within the iteration’s cohort label set. |
tests/unit/validation/test_iteration_validator.py |
Fixes a mis-keyed IterationTime input and adds a new test expecting an error on invalid cohort labels. |
tests/unit/validation/test_iteration_rules_validator.py |
Removes redundant exception wrapping for “valid minimal config” instantiation test. |
tests/unit/validation/test_campaign_config_validator.py |
Removes redundant exception wrapping for “valid minimal config” instantiation test. |
|
|
||
| for idx, rule in enumerate(self.iteration_rules): | ||
| if rule.cohort_label is None: | ||
| continue | ||
| if not all(label in allowed_labels for label in rule.parsed_cohort_labels): | ||
| allowed_str = ", ".join(sorted(allowed_labels)) | ||
| msg = ( | ||
| f"Invalid cohort_label value: {rule.cohort_label}. Allowed values: {allowed_str}. Rule index: {idx}" | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
There was a problem hiding this comment.
validate_rule_cohort_labels_against_iteration_cohorts raises a plain ValueError, which will produce a model-level error location and stops at the first invalid rule. This makes it hard to identify which IterationRules[idx].CohortLabel is wrong and prevents surfacing multiple invalid rules in one response. Consider collecting all invalid rules into InitErrorDetails (with a loc that includes the rule index and CohortLabel) and raising a single ValidationError.from_exception_data, consistent with the other validators in this file.
| for idx, rule in enumerate(self.iteration_rules): | |
| if rule.cohort_label is None: | |
| continue | |
| if not all(label in allowed_labels for label in rule.parsed_cohort_labels): | |
| allowed_str = ", ".join(sorted(allowed_labels)) | |
| msg = ( | |
| f"Invalid cohort_label value: {rule.cohort_label}. Allowed values: {allowed_str}. Rule index: {idx}" | |
| ) | |
| raise ValueError(msg) | |
| line_errors: list[InitErrorDetails] = [] | |
| for idx, rule in enumerate(self.iteration_rules): | |
| if rule.cohort_label is None: | |
| continue | |
| for label in rule.parsed_cohort_labels: | |
| if label not in allowed_labels: | |
| allowed_str = ", ".join(sorted(allowed_labels)) | |
| error = InitErrorDetails( | |
| type="value_error", | |
| loc=("iteration_rules", idx, "cohort_label"), | |
| input=rule.cohort_label, | |
| ctx={ | |
| "error": f"Invalid cohort_label value '{label}'. Allowed values: {allowed_str}." | |
| }, | |
| ) | |
| line_errors.append(error) | |
| if line_errors: | |
| raise ValidationError.from_exception_data(title="IterationValidation", line_errors=line_errors) |
| with pytest.raises(ValidationError): | ||
| IterationValidation(**(data)) |
There was a problem hiding this comment.
This test only asserts that some ValidationError is raised, but doesn't assert that the error is specifically due to an invalid cohort label (e.g., message content or error location). As written, it could pass even if a different validator starts failing first. Consider asserting that the raised error includes the expected cohort-label validation message and/or points to the relevant IterationRules index.
| with pytest.raises(ValidationError): | |
| IterationValidation(**(data)) | |
| with pytest.raises(ValidationError) as exc_info: | |
| IterationValidation(**data) | |
| errors = exc_info.value.errors() | |
| # Ensure at least one error is specifically about the invalid CohortLabel in IterationRules[0] | |
| assert any( | |
| err.get("loc", [])[:3] == ("IterationRules", 0, "CohortLabel") | |
| for err in errors | |
| ) |
| @model_validator(mode="after") | ||
| def validate_rule_cohort_labels_against_iteration_cohorts(self) -> typing.Self: | ||
| allowed_labels = {cohort.cohort_label for cohort in self.iteration_cohorts} | ||
| line_errors: list[InitErrorDetails] = [] |
There was a problem hiding this comment.
Because this is a separate @model_validator(mode="after") that raises immediately, any subsequent mode="after" validations (e.g. action_mapper_validation) will not run when cohort-label errors exist. This is a regression in error aggregation/UX compared to the existing pattern that collects multiple validation errors before raising. Consider folding this check into the existing action_mapper_validation aggregator (or otherwise combining model-level checks) so callers can receive all validation issues in one response.
| allowed_str = ", ".join(sorted(allowed_labels)) | ||
| error = InitErrorDetails( | ||
| type="value_error", | ||
| loc=("iteration_rules", idx, "cohort_label"), | ||
| input=rule.cohort_label, | ||
| ctx={ | ||
| "error": f"Invalid cohort_label value '{label}'. Allowed values: {allowed_str}." | ||
| }, |
There was a problem hiding this comment.
If iteration_cohorts is empty, allowed_labels is empty and this error message will render as Allowed values: ., which is unclear. Consider special-casing the empty set (e.g., say that no cohorts are defined, or omit the allowed-values suffix when there are none).
| allowed_str = ", ".join(sorted(allowed_labels)) | |
| error = InitErrorDetails( | |
| type="value_error", | |
| loc=("iteration_rules", idx, "cohort_label"), | |
| input=rule.cohort_label, | |
| ctx={ | |
| "error": f"Invalid cohort_label value '{label}'. Allowed values: {allowed_str}." | |
| }, | |
| if allowed_labels: | |
| allowed_str = ", ".join(sorted(allowed_labels)) | |
| error_message = ( | |
| f"Invalid cohort_label value '{label}'. Allowed values: {allowed_str}." | |
| ) | |
| else: | |
| error_message = ( | |
| f"Invalid cohort_label value '{label}'. " | |
| "No iteration cohorts are defined, so no cohort labels are allowed." | |
| ) | |
| error = InitErrorDetails( | |
| type="value_error", | |
| loc=("iteration_rules", idx, "cohort_label"), | |
| input=rule.cohort_label, | |
| ctx={"error": error_message}, |
| for label in rule.parsed_cohort_labels: | ||
| if label not in allowed_labels: | ||
| if allowed_labels: | ||
| allowed_str = ", ".join(sorted(allowed_labels)) | ||
| error_message = ( | ||
| f"Invalid cohort_label value '{label}'. Allowed values: {allowed_str}." | ||
| ) | ||
| else: | ||
| error_message = ( | ||
| f"Invalid cohort_label value '{label}'. " | ||
| "No iteration cohorts are defined, so no cohort labels are allowed." | ||
| ) |
There was a problem hiding this comment.
The new cohort-label validation has multiple branches (comma-separated cohort labels via parsed_cohort_labels, and the allowed_labels empty path). Current tests only cover the single-label + non-empty cohorts case; please add unit tests for (1) multiple cohort labels where one is invalid and (2) when IterationCohorts is empty but a rule specifies CohortLabel, to exercise the remaining paths and prevent regressions.
| for label in rule.parsed_cohort_labels: | ||
| if label not in allowed_labels: | ||
| if allowed_labels: | ||
| allowed_str = ", ".join(sorted(allowed_labels)) |
There was a problem hiding this comment.
Nothing wrong with this but we could condense it:
for label in rule.parsed_cohort_labels:
if label not in allowed_labels:
error_message = (
f"Invalid cohort_label value '{label}'. Allowed values: {', '.join(sorted(allowed_labels))}."
if allowed_labels
else (
f"Invalid cohort_label value '{label}'. "
"No iteration cohorts are defined, so no cohort labels are allowed."
)
)
line_errors.append(
InitErrorDetails(
type="value_error",
loc=("iteration_rules", idx, "cohort_label"),
input=rule.cohort_label,
ctx={"error": error_message},
)
)
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.