VED-909: Uplift Schema Expression Rule #982

Akol125 · 2025-11-11T21:05:31Z

Summary

Routine Change
❗ Breaking Change
🤖 Operational or Infrastructure Change
✨ New Feature
⚠️ Potential issues that might be caused by this change

Add any other relevant notes or explanations here. Remove this line if you have nothing to add.

Reviews Required

Dev
Test
Tech Author
Product Owner

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

I have reviewed the changes in this PR and they fill all of the acceptance criteria of the ticket.
If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
If there were changes that are outside of the regular release processes e.g. account infrastructure to setup, manual setup for external API integrations, secrets to set, then I have checked that the developer has flagged this to the Tech Lead as release steps.
I have checked that no Personal Identifiable Data (PID) is logged as part of the changes.

github-actions · 2025-11-11T21:05:46Z

This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket:

VED-909

sonarqubecloud · 2025-11-14T17:14:03Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

JamesW1-NHS

just a few issues.

NB: though the operation of the validator is dependent on fields in the schema, we're not testing explicitly for an error when checking a field that we know is not present in the schema. test_postcode_string_rule_valid_and_invalid() sort of does it (with field_path = POST_CODE) but it's by accident rather than on purpose.

I assume that we expect the FHIR validator to deal with those?

JamesW1-NHS · 2025-11-17T15:13:06Z

lambdas/shared/src/common/validator/config_schema/dev_schema.json

Where are these schema files used?

these files would be used as schema files per environment, not populated yet as it is meant to be used when we have a fully functional schema that we can use per environment,

JamesW1-NHS · 2025-11-17T15:13:23Z

lambdas/shared/src/common/validator/constants/config.json

Where is this file used?

JamesW1-NHS · 2025-11-17T15:20:27Z

lambdas/shared/src/common/validator/validator.py

            expression_values = data_parser.extract_field_values(expression_fieldname)
+            print(f"Validating Expression ID {expression_fieldname} with values: {expression_values}")
        except Exception as e:
            message = f"Data get values Unexpected exception [{e.__class__.__name__}]: {e}"


I'd make the message "Extract Field Values: Unexpected exception ..." - it's better English and more consistent with the function naming

for testing but would ultimatelty remove it eventually

JamesW1-NHS · 2025-11-17T15:20:29Z

lambdas/shared/src/common/validator/validator.py

-        row += 1
-        return row
+        except Exception as e:
+            message = f"Expression Validation Unexpected exception [{e.__class__.__name__}]: {e}"


I'd make the message "Validate Expression: Unexpected exception ..." (see above)

JamesW1-NHS · 2025-11-17T15:21:37Z

lambdas/shared/src/common/validator/validator.py

+        except Exception as e:
+            message = f"Expression Validation Unexpected exception [{e.__class__.__name__}]: {e}"
+            error_record = ErrorReport(code=ExceptionLevels.UNEXPECTED_EXCEPTION, message=message)
+            error_record = ErrorReport(code=ExceptionLevels.PARSING_ERROR, message=message)


We're overwriting the error_record written in the previous line.
Do we want to add both of these in add_error_record()?

JamesW1-NHS · 2025-11-17T15:23:43Z

lambdas/shared/tests/test_common/validator/test_schemas/test_schema.json

Question: should backend (and later recordprocessor) unit tests also use this schema when creating a Validator?

JamesW1-NHS · 2025-11-17T15:27:02Z

lambdas/shared/tests/test_common/validator/test_expression_checker.py

+                "9876543210",
+            )
+        )
+        self.assertIsNone(


This is a duplicate of lines 28-35

JamesW1-NHS · 2025-11-17T15:37:03Z

lambdas/shared/tests/test_common/validator/test_expression_checker.py

+        # With empty rule, generic string constraints apply: non-empty and no spaces
+        self.assertIsNone(checker.validate_expression("STRING", "", field_path, "SW1A 1AA"))
+        # Real-world postcode with a space should fail as spaces are not allowed without a rule override
+        field_path = "POST_CODE"


I can't see POST_CODE in the test schema anywhere, did you mean PERSON_POSTCODE?

JamesW1-NHS · 2025-11-17T15:38:38Z

lambdas/shared/tests/test_common/validator/test_expression_checker.py

+        field_path = "contained|#:Patient|address|#:postalCode|postalCode"
+        # With empty rule, generic string constraints apply: non-empty and no spaces
+        self.assertIsNone(checker.validate_expression("STRING", "", field_path, "SW1A 1AA"))
+        # Real-world postcode with a space should fail as spaces are not allowed without a rule override


should we then add a test for that e.g.
self.assertIsInstance(
checker.validate_expression("STRING", "PERSON_POSTCODE", field_path, "SW1A 1AA"),
ErrorReport,
)

dlzhry2nhs · 2025-12-09T13:57:25Z

Work has been paused on this approach. Worth considering closing the PR. Especially if we get a clear outcome from VED-947 which results in us redesigning the approach.

start uplift schema ticket2

17a4874

Akol125 temporarily deployed to dev November 11, 2025 21:05 — with GitHub Actions Inactive

Akol125 temporarily deployed to dev November 11, 2025 21:06 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 11, 2025 21:12 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 11, 2025 21:12 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 11, 2025 21:18 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 11, 2025 21:18 — with GitHub Actions Inactive

remove unused files

2493245

Akol125 temporarily deployed to dev November 12, 2025 14:14 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 12, 2025 14:19 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 12, 2025 14:19 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 12, 2025 14:19 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 12, 2025 14:22 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 12, 2025 14:26 — with GitHub Actions Inactive

uplift validation rules and create utility used in expression checker

9b50fcd

Akol125 temporarily deployed to dev November 12, 2025 17:29 — with GitHub Actions Inactive

Akol125 temporarily deployed to dev November 12, 2025 17:30 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 12, 2025 17:34 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 12, 2025 17:34 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 12, 2025 17:34 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 12, 2025 17:38 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 12, 2025 17:40 — with GitHub Actions Inactive

add expressionType for all 34 fields

69d8e24

Akol125 temporarily deployed to dev November 12, 2025 22:09 — with GitHub Actions Inactive

Akol125 temporarily deployed to dev November 12, 2025 22:10 — with GitHub Actions Inactive

Akol125 changed the title ~~start uplift schema ticket2~~ VED-909-Uplift-Schema-Expression-Rule Nov 12, 2025

Akol125 changed the title ~~VED-909-Uplift-Schema-Expression-Rule~~ VED-909: Uplift Schema Expression Rule Nov 12, 2025

Akol125 temporarily deployed to dev November 14, 2025 14:46 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 14:49 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 14:49 — with GitHub Actions Inactive

exclude backend

b5ae016

Akol125 temporarily deployed to dev November 14, 2025 14:49 — with GitHub Actions Inactive

Akol125 temporarily deployed to dev November 14, 2025 14:50 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 14:52 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 14:52 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 14:55 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 14:57 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 15:00 — with GitHub Actions Inactive

resolve sonar issues

a705b80

Akol125 temporarily deployed to dev November 14, 2025 16:52 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 16:56 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 16:56 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 16:56 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 16:59 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 17:02 — with GitHub Actions Inactive

resolve sonar issues 2

56b813d

Akol125 temporarily deployed to dev November 14, 2025 17:10 — with GitHub Actions Inactive

Akol125 temporarily deployed to dev November 14, 2025 17:11 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 17:14 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 17:14 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 17:14 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev-sandbox November 14, 2025 17:18 — with GitHub Actions Inactive

Akol125 temporarily deployed to internal-dev November 14, 2025 17:20 — with GitHub Actions Inactive

JamesW1-NHS requested changes Nov 17, 2025

View reviewed changes

VED-909: Uplift Schema Expression Rule #982

Are you sure you want to change the base?

VED-909: Uplift Schema Expression Rule #982

Uh oh!

Conversation

Akol125 commented Nov 11, 2025

Summary

Reviews Required

Review Checklist

Uh oh!

github-actions bot commented Nov 11, 2025

Uh oh!

sonarqubecloud bot commented Nov 14, 2025

Quality Gate passed

Uh oh!

JamesW1-NHS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlzhry2nhs commented Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants