Update sds schema validation #1538

VirajP1002 · 2024-10-17T14:52:07Z

What is the context of this PR?

To align with SDS & Author we need to update the validation rules for supplementary data schema versions within Runner in order to support future automation of the releases of new SDS schema versions.

supplementary_data_parser previously used a list of known versions but this will become unmanageable with several versions. Instead, we compare the sds_schema_version set in a questionnaire schema to the schema_version found in the supplementary data payload. We pass sds_schema_version from session.py to supplementary_data_parser for it to be validated. The old logic has been removed and tests have been updated/added to show this change.

A new schema has been added called test_supplementary_data_with_sds_schema_version.json to include this new field.

How to review

Ensure changes make sense
Check unit, integration & functional tests pass
Ensure schema validation passes (using the validator PR)

Checklist

New static content marked up for translation
Newly defined schema content included in eq-translations repo

…order for it to be validated

…o if a schema doesn't include it, no error is raised

…t schema version in sds payload

berroar · 2024-10-21T14:05:27Z

app/utilities/supplementary_data_parser.py

@@ -80,12 +71,19 @@ def validate_dataset_and_survey_id(  # pylint: disable=unused-argument
                    "Supplementary data did not return the specified Survey ID"
                )

+            if self.context["sds_schema_version"]:


Can merge the nested if here. Also data["data"] is a bit weird but not sure there is a way around it

berroar · 2024-10-21T14:05:45Z

schemas/test/en/test_supplementary_data_with_sds_schema_version.json

+    "survey_id": "123",
+    "title": "Test Supplementary Data",
+    "theme": "default",
+    "description": "A questionnaire to demo using Supplementary data for placeholders, validation and routing in both repeating and non repeating sections.",


Would update the description here

berroar · 2024-10-21T14:07:54Z

schemas/test/en/test_supplementary_data_with_sds_schema_version.json

The new schema is not actually being used anywhere?

Added in an integration test 👍

Add discussed we decided to remove the schema and the integration test

app/routes/session.py

app/utilities/supplementary_data_parser.py

petechd · 2024-11-11T11:09:32Z

app/utilities/supplementary_data_parser.py

@@ -67,25 +58,33 @@ class SupplementaryDataMetadataSchema(Schema, StripWhitespaceMixin):

    @validates_schema()
    def validate_dataset_and_survey_id(  # pylint: disable=unused-argument


The function name should probably change since you also check "sds_schema_version" now.

Instead, I moved the sds_schema_version validation in it's own function underneath, just because it makes the name look smaller in addition to the method name making sense 😅 and validation still passes.

Good, that new function will require some unit testing (if it's not already tested).

V minor - No worries if this has already been discussed, so feel free to ignore me 😆 but we now have 2 similar functions that validate things in the payload as follows:

existing validate_dataset_and_survey_id() which validates 2 things in the payload

newly added validate_sds_schema_version() which validates 1 thing in the payload

would it be worth splitting validate_dataset_and_survey_id into separate validation functions, if we're making the validation as individual functions?

Personal preference, but I feel like it might make the most sense to combine all validation logic for this into a single function so it checks all 3 things (like it was originally in the PR I think?) and just rename it to something like validate_payload()?

app/utilities/supplementary_data_parser.py

tests/integration/session/test_login.py

petechd · 2024-11-11T12:06:06Z

tests/integration/session/test_login.py

+    @patch(
+        "app.questionnaire.questionnaire_store_updater.QuestionnaireStoreUpdaterBase.set_supplementary_data",
+    )
+    def test_login_with_sds_schema_version_valid(


You probably need to move your testing to test_supplementary_data_parser.py where the class and methods you changed are tested.

The purpose of this test was just to check if the schema opens, I initially did a functional test, but then we decided it would be better to add it as an integration test. So we chose to put it in test_login as these just check if schemas can open?

I'm not sure that this is testing anything at the moment though cause if you change the sds_schema_version in your schema the suite still passes

As discussed we agreed that the unit test did the same as the integration test and so I removed it

schemas/test/en/test_supplementary_data_with_sds_schema_version.json

…test structure

…y from unit test

petechd · 2024-12-03T12:42:47Z

app/services/supplementary_data.py

+    dataset_id: str,
+    identifier: str,
+    survey_id: str,
+    sds_schema_version: str | None = None,
 ) -> dict:


Little detail here but for the return value type hint we recommend in our guide to at least give types of the dict keys and values. https://github.com/ONSdigital/eq-questionnaire-runner/blob/main/doc/python-type-hinting.md#generic-types

petechd · 2024-12-03T12:54:41Z

tests/app/parser/test_supplementary_data_parser.py

+    )
+
+
+def test_valid_supplementary_dataset_version():


You can create some more readable code by just writing a test where valid dataset version does not raise an error by using some @contextmanager decorator like this one:

from contextlib import contextmanager @contextmanager def not_raises(exception): try: yield except exception: raise pytest.fail("DID RAISE {0}".format(exception))

...and then use it as with not_raises(ValidationError):
Then you can rename the test to align the wording with the first one.

VirajP1002 and others added 9 commits October 10, 2024 15:15

Get sds_schema_version from the schema and pass it as a parameter in …

a951b06

…order for it to be validated

Add the sds_schema_version as a parameter where the default is none s…

82db7a0

…o if a schema doesn't include it, no error is raised

Add tests to show the sds_schema_version with valid and invalid values

8755362

Remove older logic to validate sds schema versions

35e1f11

Add the sds_schema_version parameters for it to validated

3c28043

Add tests to show questionnaire schema_version being validated agains…

578a189

…t schema version in sds payload

Update test_supplementary.json to include new sds_schema_version field

42424a8

Merge branch 'main' into update-sds-schema-validation

06646f3

Add a new questionnaire schema with the new sds_schema version field

e2ae6cb

VirajP1002 marked this pull request as ready for review October 18, 2024 18:15

berroar reviewed Oct 21, 2024

View reviewed changes

liamtoozer reviewed Oct 22, 2024

View reviewed changes

app/routes/session.py Outdated Show resolved Hide resolved

liamtoozer reviewed Oct 22, 2024

View reviewed changes

app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved

VirajP1002 and others added 7 commits October 24, 2024 14:30

Merge branch 'main' into update-sds-schema-validation

c512778

Address review comments

0f97cba

Format

efe0d02

Merge branch 'main' into update-sds-schema-validation

2631959

Revert some changes to sds_dataset_id values and update test assertion

f16c0c6

Add mock to the new test

37ead17

Add functions to call mock and assert they're called

94e03c5

petechd reviewed Nov 11, 2024

View reviewed changes

app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved

petechd reviewed Nov 11, 2024

View reviewed changes

app/utilities/supplementary_data_parser.py Show resolved Hide resolved

petechd reviewed Nov 11, 2024

View reviewed changes

tests/integration/session/test_login.py Outdated Show resolved Hide resolved

petechd reviewed Nov 11, 2024

View reviewed changes

liamtoozer reviewed Nov 11, 2024

View reviewed changes

schemas/test/en/test_supplementary_data_with_sds_schema_version.json Outdated Show resolved Hide resolved

VirajP1002 and others added 4 commits November 14, 2024 11:02

Merge branch 'main' into update-sds-schema-validation

be808f2

Move sds_schema_version validation into separate function and update …

88d9781

…test structure

Remove integration test and schema as it would duplicate functionalit…

951f91b

…y from unit test

Remove content from test_login

5f66a3d

Merge branch 'main' into update-sds-schema-validation

f60402d

petechd reviewed Dec 3, 2024

View reviewed changes

Merge branch 'main' into update-sds-schema-validation

9f3add0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update sds schema validation #1538

Update sds schema validation #1538

VirajP1002 commented Oct 17, 2024 •

edited

Loading

berroar Oct 21, 2024

berroar Oct 21, 2024

berroar Oct 21, 2024

VirajP1002 Nov 8, 2024

VirajP1002 Nov 22, 2024

petechd Nov 11, 2024

VirajP1002 Nov 14, 2024

petechd Nov 15, 2024

liamtoozer Nov 29, 2024 •

edited

Loading

petechd Nov 11, 2024

VirajP1002 Nov 14, 2024

berroar Nov 14, 2024

VirajP1002 Nov 22, 2024

petechd Dec 3, 2024 •

edited

Loading

petechd Dec 3, 2024 •

edited

Loading

petechd Dec 3, 2024 •

edited

Loading

		@@ -67,25 +58,33 @@ class SupplementaryDataMetadataSchema(Schema, StripWhitespaceMixin):

		@validates_schema()
		def validate_dataset_and_survey_id( # pylint: disable=unused-argument

Update sds schema validation #1538

Are you sure you want to change the base?

Update sds schema validation #1538

Conversation

VirajP1002 commented Oct 17, 2024 • edited Loading

What is the context of this PR?

How to review

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamtoozer Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petechd Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

petechd Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

petechd Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

VirajP1002 commented Oct 17, 2024 •

edited

Loading

liamtoozer Nov 29, 2024 •

edited

Loading

petechd Dec 3, 2024 •

edited

Loading

petechd Dec 3, 2024 •

edited

Loading

petechd Dec 3, 2024 •

edited

Loading