From 7634c2e77253d0fd8b8f0393b05204e6917b687b Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 15 Dec 2016 16:35:27 +0000 Subject: [PATCH 1/3] Add robustness to our v2 schema error messages: * Ensure we dont raise exception if e.cause does not contain a message * Ensure we handle case where e.path may be empty * Refactor existing tests to conform to new format --- app/schema_validation/__init__.py | 23 ++++++++++++++++++- tests/app/delivery/test_send_to_providers.py | 1 - .../notifications/test_get_notifications.py | 4 ++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index fe643ad61..593e1b41a 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -42,4 +42,25 @@ def build_error_message(errors): def __format_message(e): - return e.message.replace("'", "") if not e.cause else "{} {}".format(e.path[0], e.cause.message) + def get_path(e): + error_path = None + try: + error_path = e.path[0] + finally: + return error_path + + def get_error_message(e): + error_message = None + try: + error_message = e.cause.message + except AttributeError: + error_message = e.message + finally: + return error_message.replace("'", '') + + path = get_path(e) + message = get_error_message(e) + if path: + return "{} {}".format(path, message) + else: + return "{}".format(message) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index dfddbb49b..2f37d1eed 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -109,7 +109,6 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist assert ' Date: Thu, 15 Dec 2016 16:37:40 +0000 Subject: [PATCH 2/3] Add uri validation to response schemas: * Add to root uri and template.uri in responses * Add tests to validate invalid or missing uri * General refactoring for clarity --- app/v2/notifications/notification_schemas.py | 12 +- .../test_notification_schemas.py | 208 ++++++++++++------ 2 files changed, 143 insertions(+), 77 deletions(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index eadcc2aa7..2e3267223 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -10,7 +10,7 @@ template = { "properties": { "id": uuid, "version": {"type": "integer"}, - "uri": {"type": "string"} + "uri": {"type": "string", "format": "uri"} }, "required": ["id", "version", "uri"] } @@ -161,7 +161,7 @@ post_sms_response = { "id": uuid, "reference": {"type": ["string", "null"]}, "content": sms_content, - "uri": {"type": "string"}, + "uri": {"type": "string", "format": "uri"}, "template": template }, "required": ["id", "content", "uri", "template"] @@ -204,7 +204,7 @@ post_email_response = { "id": uuid, "reference": {"type": ["string", "null"]}, "content": email_content, - "uri": {"type": "string"}, + "uri": {"type": "string", "format": "uri"}, "template": template }, "required": ["id", "content", "uri", "template"] @@ -216,7 +216,7 @@ def create_post_sms_response_from_notification(notification, body, from_number, "reference": notification.client_reference, "content": {'body': body, 'from_number': from_number}, - "uri": "{}/v2/notifications/{}".format(url_root, str(notification.id)), + "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), "template": __create_template_from_notification(notification=notification, url_root=url_root) } @@ -230,7 +230,7 @@ def create_post_email_response_from_notification(notification, content, subject, "body": content, "subject": subject }, - "uri": "{}/v2/notifications/{}".format(url_root, str(notification.id)), + "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), "template": __create_template_from_notification(notification=notification, url_root=url_root) } @@ -239,5 +239,5 @@ def __create_template_from_notification(notification, url_root): return { "id": notification.template_id, "version": notification.template_version, - "uri": "{}/v2/templates/{}".format(url_root, str(notification.template_id)) + "uri": "{}v2/templates/{}".format(url_root, str(notification.template_id)) } diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index bd54d3e8f..95c977d9e 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -6,7 +6,10 @@ from jsonschema import ValidationError from app.v2.notifications.notification_schemas import ( get_notifications_request, - post_sms_request, post_sms_response, post_email_request, post_email_response + post_sms_request as post_sms_request_schema, + post_sms_response as post_sms_response_schema, + post_email_request as post_email_request_schema, + post_email_response as post_email_response_schema ) from app.schema_validation import validate @@ -23,7 +26,8 @@ def test_get_notifications_request_invalid_statuses( invalid_statuses, valid_statuses ): partial_error_status = "is not one of " \ - "[created, sending, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" + "[created, sending, delivered, pending, failed, " \ + "technical-failure, temporary-failure, permanent-failure]" with pytest.raises(ValidationError) as e: validate({'status': invalid_statuses + valid_statuses}, get_notifications_request) @@ -31,7 +35,7 @@ def test_get_notifications_request_invalid_statuses( errors = json.loads(e.value.message).get('errors') assert len(errors) == len(invalid_statuses) for index, value in enumerate(invalid_statuses): - assert errors[index]['message'] == "{} {}".format(value, partial_error_status) + assert errors[index]['message'] == "status {} {}".format(value, partial_error_status) @pytest.mark.parametrize('invalid_template_types, valid_template_types', [ @@ -53,7 +57,7 @@ def test_get_notifications_request_invalid_template_types( errors = json.loads(e.value.message).get('errors') assert len(errors) == len(invalid_template_types) for index, value in enumerate(invalid_template_types): - assert errors[index]['message'] == "{} {}".format(value, partial_error_template_type) + assert errors[index]['message'] == "template_type {} {}".format(value, partial_error_template_type) def test_get_notifications_request_invalid_statuses_and_template_types(): @@ -68,15 +72,15 @@ def test_get_notifications_request_invalid_statuses_and_template_types(): assert len(errors) == 4 error_messages = [error['message'] for error in errors] - for invalid_status in ["elephant", "giraffe"]: - assert "{} is not one of [created, sending, delivered, " \ + assert "status {} is not one of [created, sending, delivered, " \ "pending, failed, technical-failure, temporary-failure, permanent-failure]".format( invalid_status ) in error_messages for invalid_template_type in ["orange", "avocado"]: - assert "{} is not one of [sms, email, letter]".format(invalid_template_type) in error_messages + assert "template_type {} is not one of [sms, email, letter]" \ + .format(invalid_template_type) in error_messages valid_json = {"phone_number": "07515111111", @@ -92,13 +96,13 @@ valid_json_with_optionals = { @pytest.mark.parametrize("input", [valid_json, valid_json_with_optionals]) def test_post_sms_schema_is_valid(input): - assert validate(input, post_sms_request) == input + assert validate(input, post_sms_request_schema) == input def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): j = {"template_id": "notUUID"} with pytest.raises(ValidationError) as e: - validate(j, post_sms_request) + validate(j, post_sms_request_schema) error = json.loads(e.value.message) assert len(error.keys()) == 2 assert error.get('status_code') == 400 @@ -117,7 +121,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): "personalisation": "not_a_dict" } with pytest.raises(ValidationError) as e: - validate(j, post_sms_request) + validate(j, post_sms_request_schema) error = json.loads(e.value.message) assert len(error.get('errors')) == 1 assert error['errors'] == [{'error': 'ValidationError', @@ -130,64 +134,60 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): [('08515111111', 'phone_number Not a UK mobile number'), ('07515111*11', 'phone_number Must not contain letters or symbols'), ('notaphoneumber', 'phone_number Must not contain letters or symbols')]) -def test_post_sms_request_invalid_phone_number(invalid_phone_number, err_msg): +def test_post_sms_request_schema_invalid_phone_number(invalid_phone_number, err_msg): j = {"phone_number": invalid_phone_number, "template_id": str(uuid.uuid4()) } with pytest.raises(ValidationError) as e: - validate(j, post_sms_request) + validate(j, post_sms_request_schema) errors = json.loads(e.value.message).get('errors') assert len(errors) == 1 assert {"error": "ValidationError", "message": err_msg} == errors[0] -def test_post_sms_request_invalid_phone_number_and_missing_template(): +def test_post_sms_request_schema_invalid_phone_number_and_missing_template(): j = {"phone_number": '08515111111', } with pytest.raises(ValidationError) as e: - validate(j, post_sms_request) + validate(j, post_sms_request_schema) errors = json.loads(e.value.message).get('errors') assert len(errors) == 2 assert {"error": "ValidationError", "message": "phone_number Not a UK mobile number"} in errors assert {"error": "ValidationError", "message": "template_id is a required property"} in errors -valid_response = { - "id": str(uuid.uuid4()), - "content": {"body": "contents of message", - "from_number": "46045"}, - "uri": "/v2/notifications/id", - "template": {"id": str(uuid.uuid4()), - "version": 1, - "uri": "/v2/template/id"} -} - -valid_response_with_optionals = { - "id": str(uuid.uuid4()), - "reference": "reference_from_service", - "content": {"body": "contents of message", - "from_number": "46045"}, - "uri": "/v2/notifications/id", - "template": {"id": str(uuid.uuid4()), - "version": 1, - "uri": "/v2/template/id"} -} +def valid_sms_response(): + return { + "id": str(uuid.uuid4()), + "content": {"body": "contents of message", + "from_number": "46045"}, + "uri": "http://notify.api/v2/notifications/id", + "template": { + "id": str(uuid.uuid4()), + "version": 1, + "uri": "http://notify.api/v2/template/id" + } + } -@pytest.mark.parametrize('input', [valid_response]) -def test_post_sms_response_schema_is_valid(input): - assert validate(input, post_sms_response) == input +def valid_sms_response_with_optionals(): + return { + "id": str(uuid.uuid4()), + "reference": "reference_from_service", + "content": {"body": "contents of message", + "from_number": "46045"}, + "uri": "http://notify.api/v2/notifications/id", + "template": { + "id": str(uuid.uuid4()), + "version": 1, + "uri": "http://notify.api/v2/template/id" + } + } -def test_post_sms_response_schema_missing_uri(): - j = valid_response - del j["uri"] - with pytest.raises(ValidationError) as e: - validate(j, post_sms_response) - error = json.loads(e.value.message) - assert error['status_code'] == 400 - assert error['errors'] == [{'error': 'ValidationError', - 'message': "uri is a required property"}] +@pytest.mark.parametrize('input', [valid_sms_response()]) +def test_post_sms_response_schema_schema_is_valid(input): + assert validate(input, post_sms_response_schema) == input valid_post_email_json = {"email_address": "test@example.gov.uk", @@ -203,43 +203,109 @@ valid_post_email_json_with_optionals = { @pytest.mark.parametrize("input", [valid_post_email_json, valid_post_email_json_with_optionals]) def test_post_email_schema_is_valid(input): - assert validate(input, post_email_request) == input + assert validate(input, post_email_request_schema) == input def test_post_email_schema_bad_uuid_and_missing_email_address(): j = {"template_id": "bad_template"} with pytest.raises(ValidationError): - validate(j, post_email_request) + validate(j, post_email_request_schema) def test_post_email_schema_invalid_email_address(): j = {"template_id": str(uuid.uuid4()), "email_address": "notavalidemail@address"} with pytest.raises(ValidationError): - validate(j, post_email_request) + validate(j, post_email_request_schema) -valid_email_response = {"id": str(uuid.uuid4()), - "content": {"body": "the body of the message", - "subject": "subject of the message", - "from_email": "service@dig.gov.uk"}, - "uri": "/v2/notifications/id", - "template": {"id": str(uuid.uuid4()), - "version": 1, - "uri": "/v2/template/id"} - } -valid_email_response_with_optionals = {"id": str(uuid.uuid4()), - "reference": "some reference", - "content": {"body": "the body of the message", - "subject": "subject of the message", - "from_email": "service@dig.gov.uk"}, - "uri": "/v2/notifications/id", - "template": {"id": str(uuid.uuid4()), - "version": 1, - "uri": "/v2/template/id"} - } +def valid_email_response(): + return { + "id": str(uuid.uuid4()), + "content": {"body": "the body of the message", + "subject": "subject of the message", + "from_email": "service@dig.gov.uk"}, + "uri": "http://notify.api/v2/notifications/id", + "template": { + "id": str(uuid.uuid4()), + "version": 1, + "uri": "http://notify.api/v2/template/id" + } + } -@pytest.mark.parametrize("input", [valid_email_response, valid_email_response_with_optionals]) -def test_post_email_response(input): - assert validate(input, post_email_response) == input +def valid_email_response_with_optionals(): + return { + "id": str(uuid.uuid4()), + "reference": "some reference", + "content": {"body": "the body of the message", + "subject": "subject of the message", + "from_email": "service@dig.gov.uk"}, + "uri": "http://notify.api/v2/notifications/id", + "template": { + "id": str(uuid.uuid4()), + "version": 1, + "uri": "http://notify.api/v2/template/id" + } + } + + +@pytest.mark.parametrize("input", [valid_email_response(), valid_email_response_with_optionals()]) +def test_post_email_response_schema(input): + assert validate(input, post_email_response_schema) == input + + +@pytest.mark.parametrize('response, schema', [ + (valid_email_response(), post_email_response_schema), + (valid_sms_response(), post_sms_response_schema) +]) +def test_post_sms_response_schema_missing_uri_raises_validation_error(response, schema): + del response['uri'] + with pytest.raises(ValidationError) as e: + validate(response, schema) + error = json.loads(e.value.message) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "uri is a required property"}] + + +@pytest.mark.parametrize('response, schema', [ + (valid_email_response(), post_email_response_schema), + (valid_sms_response(), post_sms_response_schema) +]) +def test_post_sms_response_schema_invalid_uri_raises_validation_error(response, schema): + response['uri'] = 'invalid-uri' + with pytest.raises(ValidationError) as e: + validate(response, schema) + error = json.loads(e.value.message) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "uri invalid-uri is not a uri"}] + + +@pytest.mark.parametrize('response, schema', [ + (valid_email_response(), post_email_response_schema), + (valid_sms_response(), post_sms_response_schema) +]) +def test_post_sms_response_schema_missing_template_uri_raises_validation_error(response, schema): + del response['template']['uri'] + with pytest.raises(ValidationError) as e: + validate(response, schema) + error = json.loads(e.value.message) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "template uri is a required property"}] + + +@pytest.mark.parametrize('response, schema', [ + (valid_email_response(), post_email_response_schema), + (valid_sms_response(), post_sms_response_schema) +]) +def test_post_sms_response_schema_invalid_template_uri_raises_validation_error(response, schema): + response['template']['uri'] = 'invalid-uri' + with pytest.raises(ValidationError) as e: + validate(response, schema) + error = json.loads(e.value.message) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "template invalid-uri is not a uri"}] From 27952e96e05cea4ce0f665c3ac78a49ced6defe9 Mon Sep 17 00:00:00 2001 From: imdadahad Date: Fri, 16 Dec 2016 12:41:19 +0000 Subject: [PATCH 3/3] Add comment for e.path schema validation fix Add comment for e.path in schema validation and use popleft() --- app/schema_validation/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 593e1b41a..bd94755d4 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -45,7 +45,9 @@ def __format_message(e): def get_path(e): error_path = None try: - error_path = e.path[0] + error_path = e.path.popleft() + # no need to catch IndexError exception explicity as + # error_path is None if e.path has no items finally: return error_path