From 278132f290f4132d40c459f738e53748cedd8f52 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 17 Mar 2026 10:42:35 +0000 Subject: [PATCH 1/3] Use new DVLA endpoints when sending letters This changes the `send_letter` method of the `DVLAClient` to send the letter in a different way - by uploading the file before making the POST request. Currently, sending a letter is done in a single request to the `/v1/print/jobs` endpoint that includes the file. The new way of sending involves making 3 requests: 1. A GET request to `/v1/print/files/upload-url` to get a presigned URL 2. A PUT request to upload the letter file to the URL returned by step 1 3. A POST request to `/v1/print/jobs` with the `uploadId` from step 1 included in the request data These three requests are all made consecutively from the same method, which means we don't need to store the URL and uploadId from step 1 - they are used immediately. Any of the stages failing with a retryable error will result in the whole `deliver_letter` task being retried. It is possible for a letter to be uploaded successfully at stage 2, then a failure at stage 3 cause the task to retry and it to be uploaded again to a new presigned URL before being sent. If a letter is uploaded but there is no corresponding print request for the uploadId associated with the presigned URL, the file will not be sent by the DVLA and will get deleted. Error handling at the new stages of sending a letter Stage 1 - getting a presigned URL requires authentication so has the same error handling as making the POST request. Stage 2 - we don't handle the case of a URL having expired when uploading a file to it - URLs are valid for 1 hour and the delay between getting a new URL and using it is seconds at most. We treat rate limit errors (503s) from this endpoint like any other 5xx status code and retry. We don't expect to see rate limiting here because we'd hit rate limiting in stage 1 first. --- app/clients/letter/dvla.py | 56 ++++- tests/app/clients/test_dvla.py | 418 ++++++++++++++++++++++++++++++--- 2 files changed, 436 insertions(+), 38 deletions(-) diff --git a/app/clients/letter/dvla.py b/app/clients/letter/dvla.py index 4e9036d449..42316b8c69 100644 --- a/app/clients/letter/dvla.py +++ b/app/clients/letter/dvla.py @@ -1,4 +1,3 @@ -import base64 import contextlib import secrets import string @@ -252,6 +251,43 @@ def _get_auth_headers(self): "X-API-Key": self.dvla_api_key.get(), } + def _get_upload_url(self): + """ + Calls the DVLA endpoint to get a presigned URL to use to upload a letter. + URLs are valid for 1 hour. + """ + + def _handle_http_errors(e: requests.HTTPError): + if e.response.status_code == 400: + raise DvlaNonRetryableException(e.response.json()["errors"][0]["detail"]) from e + elif e.response.status_code in {401, 403}: + # probably the api key is not valid + self.dvla_api_key.clear() + + raise DvlaUnauthorisedRequestException(e.response.json()["errors"][0]["detail"]) from e + + with _handle_common_dvla_errors(custom_httperror_exc_handler=_handle_http_errors): + response = self.session.get( + f"{self.base_url}/print-request/v1/print/files/upload-url", + headers=self._get_auth_headers(), + ) + response.raise_for_status() + return response.json() + + def _upload_file(self, *, upload_url: str, pdf_file: bytes): + """ + Uploads the letter to DVLA's S3 account using the given presigned URL. + + While the endpoint isn't a DVLA one, we still want the error handling behaviour of `_handle_common_dvla_errors`. + + Being rate limited when uploading the file will give a `503` status code instead of a `429` code. However, this + isn't handled separately because we should hit DVLA rate limits before being rate limited here, and 5xx status + codes are retried. + """ + with _handle_common_dvla_errors(): + response = self.session.put(upload_url, headers={"Content-Type": "application/pdf"}, data=pdf_file) + response.raise_for_status() + @record_request_duration(notification_type="letter", provider_name="dvla") def send_letter( self, @@ -268,6 +304,17 @@ def send_letter( """ Sends a letter to the DVLA for printing """ + url_response = self._get_upload_url() + upload_id = url_response["uploadId"] + upload_url = url_response["uploadUrl"] + + self._upload_file(upload_url=upload_url, pdf_file=pdf_file) + current_app.logger.info( + "Letter with notification id %s uploaded to DVLA presigned URL with upload_id %s", + notification_id, + upload_id, + extra={"notification_id": notification_id, "dvla_upload_id": upload_id}, + ) def _handle_http_errors(e: requests.HTTPError): if e.response.status_code == 400: @@ -291,7 +338,7 @@ def _handle_http_errors(e: requests.HTTPError): postage=postage, service_id=service_id, organisation_id=organisation_id, - pdf_file=pdf_file, + upload_id=upload_id, callback_url=callback_url, ), ) @@ -299,7 +346,7 @@ def _handle_http_errors(e: requests.HTTPError): return response.json() def _format_create_print_job_json( - self, *, notification_id, reference, address, postage, service_id, organisation_id, pdf_file, callback_url + self, *, notification_id, reference, address, postage, service_id, organisation_id, upload_id, callback_url ): # We shouldn't need to pass the postage in, as the address has a postage field. However, at this point we've # recorded the postage on the notification so we should respect that rather than introduce any possible @@ -319,7 +366,6 @@ def _format_create_print_job_json( "address": address_data, }, "customParams": [ - {"key": "pdfContent", "value": base64.b64encode(pdf_file).decode("utf-8")}, {"key": "organisationIdentifier", "value": organisation_id}, {"key": "serviceIdentifier", "value": service_id}, ], @@ -330,6 +376,8 @@ def _format_create_print_job_json( "retryParams": {"enabled": True, "maxRetryWindow": 10800}, } + json_payload["fileParams"] = [{"fileId": notification_id, "uploadId": upload_id}] + # `despatchMethod` should not be added for second class letters if postage == FIRST_CLASS: json_payload["standardParams"]["despatchMethod"] = "FIRST" diff --git a/tests/app/clients/test_dvla.py b/tests/app/clients/test_dvla.py index 297054f089..68e2d204d7 100644 --- a/tests/app/clients/test_dvla.py +++ b/tests/app/clients/test_dvla.py @@ -29,6 +29,25 @@ ) +@pytest.fixture +def mock_get_presigned_url(rmock): + return rmock.get( + f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url", + json={"uploadId": "upload_id", "uploadUrl": "https://s3bucketaddress/upload_id"}, + status_code=200, + ) + + +@pytest.fixture +def mock_upload_file(rmock): + return rmock.put("https://s3bucketaddress/upload_id", status_code=200) + + +@pytest.fixture +def mock_create_print_request(rmock): + return rmock.post(f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", status_code=202) + + @pytest.fixture def ssm(): with mock_aws(): @@ -419,7 +438,7 @@ def test_format_create_print_job_json_builds_json_body_to_create_print_job(dvla_ postage="second", service_id="my_service_id", organisation_id="my_organisation_id", - pdf_file=b"pdf_content", + upload_id="my_upload_id", callback_url="/my-callback", ) @@ -434,10 +453,10 @@ def test_format_create_print_job_json_builds_json_body_to_create_print_job(dvla_ }, "callbackParams": {"retryParams": {"enabled": True, "maxRetryWindow": 10800}, "target": "/my-callback"}, "customParams": [ - {"key": "pdfContent", "value": "cGRmX2NvbnRlbnQ="}, {"key": "organisationIdentifier", "value": "my_organisation_id"}, {"key": "serviceIdentifier", "value": "my_service_id"}, ], + "fileParams": [{"fileId": "my_notification_id", "uploadId": "my_upload_id"}], } @@ -449,7 +468,7 @@ def test_format_create_print_job_json_adds_callback_key_if_url_provided(dvla_cli postage="second", service_id="my_service_id", organisation_id="my_organisation_id", - pdf_file=b"pdf_content", + upload_id="my_upload_id", callback_url="https://www.example.com?token=1234", ) @@ -467,13 +486,28 @@ def test_format_create_print_job_json_adds_despatchMethod_key_for_first_class_po postage="first", service_id="my_service_id", organisation_id="my_organisation_id", - pdf_file=b"pdf_content", + upload_id="my_upload_id", callback_url="/my-callback", ) assert formatted_json["standardParams"]["despatchMethod"] == "FIRST" +def test_format_create_print_job_json_adds_despatchMethod_key_for_economy_class_post(dvla_client): + formatted_json = dvla_client._format_create_print_job_json( + notification_id="my_notification_id", + reference="ABCDEFGHIJKL", + address=PostalAddress("A. User\nThe road\nCity\nSW1 1AA"), + postage="economy", + service_id="my_service_id", + organisation_id="my_organisation_id", + upload_id="my_upload_id", + callback_url="/my-callback", + ) + + assert formatted_json["standardParams"]["despatchMethod"] == "ECONOMY" + + @pytest.mark.parametrize( "address, recipient, unstructured_address", [ @@ -508,7 +542,7 @@ def test_format_create_print_job_json_formats_address_lines(dvla_client, address postage="first", service_id="my_service_id", organisation_id="my_organisation_id", - pdf_file=b"pdf_content", + upload_id="my_upload_id", callback_url="/my-callback", ) @@ -527,7 +561,7 @@ def test_format_create_print_job_json_formats_international_address_lines(dvla_c postage="europe", service_id="my_service_id", organisation_id="my_organisation_id", - pdf_file=b"pdf_content", + upload_id="my_upload_id", callback_url="/my-callback", ) @@ -535,7 +569,7 @@ def test_format_create_print_job_json_formats_international_address_lines(dvla_c assert formatted_json["standardParams"]["address"]["internationalAddress"] == expected_address -def test_send_domestic_letter(dvla_client, dvla_authenticate, rmock): +def test_send_domestic_letter(dvla_client, dvla_authenticate, mock_get_presigned_url, mock_upload_file, rmock): print_mock = rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={"id": "noti_id"}, @@ -555,6 +589,15 @@ def test_send_domestic_letter(dvla_client, dvla_authenticate, rmock): assert response == {"id": "noti_id"} + presigned_url_headers = mock_get_presigned_url.last_request.headers + assert mock_get_presigned_url.called_once is True + assert presigned_url_headers["Accept"] == "application/json" + assert presigned_url_headers["X-API-Key"] == "some api key" + assert presigned_url_headers["Authorization"] + + assert mock_upload_file.last_request.body == b"pdf" + assert mock_upload_file.last_request.headers["Content-Type"] == "application/pdf" + assert print_mock.last_request.json() == { "id": "noti_id", "standardParams": { @@ -565,7 +608,6 @@ def test_send_domestic_letter(dvla_client, dvla_authenticate, rmock): "address": {"unstructuredAddress": {"line1": "city", "postcode": "postcode"}}, }, "customParams": [ - {"key": "pdfContent", "value": "cGRm"}, {"key": "organisationIdentifier", "value": "org_id"}, {"key": "serviceIdentifier", "value": "service_id"}, ], @@ -573,6 +615,7 @@ def test_send_domestic_letter(dvla_client, dvla_authenticate, rmock): "target": "https://www.example.com?token=1234", "retryParams": {"enabled": True, "maxRetryWindow": 10800}, }, + "fileParams": [{"fileId": "noti_id", "uploadId": "upload_id"}], } request_headers = print_mock.last_request.headers @@ -586,7 +629,9 @@ def test_send_domestic_letter(dvla_client, dvla_authenticate, rmock): @pytest.mark.parametrize( "postage, despatch_method", (("europe", "INTERNATIONAL_EU"), ("rest-of-world", "INTERNATIONAL_ROW")) ) -def test_send_international_letter(dvla_client, dvla_authenticate, postage, despatch_method, rmock): +def test_send_international_letter( + dvla_client, dvla_authenticate, postage, despatch_method, mock_get_presigned_url, mock_upload_file, rmock +): print_mock = rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={"id": "noti_id"}, @@ -606,6 +651,15 @@ def test_send_international_letter(dvla_client, dvla_authenticate, postage, desp assert response == {"id": "noti_id"} + presigned_url_headers = mock_get_presigned_url.last_request.headers + assert mock_get_presigned_url.called_once is True + assert presigned_url_headers["Accept"] == "application/json" + assert presigned_url_headers["X-API-Key"] == "some api key" + assert presigned_url_headers["Authorization"] + + assert mock_upload_file.last_request.body == b"pdf" + assert mock_upload_file.last_request.headers["Content-Type"] == "application/pdf" + assert print_mock.last_request.json() == { "id": "noti_id", "standardParams": { @@ -618,14 +672,14 @@ def test_send_international_letter(dvla_client, dvla_authenticate, postage, desp }, "callbackParams": {"retryParams": {"enabled": True, "maxRetryWindow": 10800}, "target": "/my-callback"}, "customParams": [ - {"key": "pdfContent", "value": "cGRm"}, {"key": "organisationIdentifier", "value": "org_id"}, {"key": "serviceIdentifier", "value": "service_id"}, ], + "fileParams": [{"fileId": "noti_id", "uploadId": "upload_id"}], } -def test_send_bfpo_letter(dvla_client, dvla_authenticate, rmock): +def test_send_bfpo_letter(dvla_client, dvla_authenticate, mock_get_presigned_url, mock_upload_file, rmock): print_mock = rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={"id": "noti_id"}, @@ -645,6 +699,15 @@ def test_send_bfpo_letter(dvla_client, dvla_authenticate, rmock): assert response == {"id": "noti_id"} + presigned_url_headers = mock_get_presigned_url.last_request.headers + assert mock_get_presigned_url.called_once is True + assert presigned_url_headers["Accept"] == "application/json" + assert presigned_url_headers["X-API-Key"] == "some api key" + assert presigned_url_headers["Authorization"] + + assert mock_upload_file.last_request.body == b"pdf" + assert mock_upload_file.last_request.headers["Content-Type"] == "application/pdf" + assert print_mock.last_request.json() == { "id": "noti_id", "standardParams": { @@ -656,14 +719,299 @@ def test_send_bfpo_letter(dvla_client, dvla_authenticate, rmock): }, "callbackParams": {"retryParams": {"enabled": True, "maxRetryWindow": 10800}, "target": "/my-callback"}, "customParams": [ - {"key": "pdfContent", "value": "cGRm"}, {"key": "organisationIdentifier", "value": "org_id"}, {"key": "serviceIdentifier", "value": "service_id"}, ], + "fileParams": [{"fileId": "noti_id", "uploadId": "upload_id"}], } -def test_send_letter_when_bad_request_error_is_raised(dvla_authenticate, dvla_client, rmock): +def test_send_letter_when_bad_request_error_is_raised_getting_upload_url( + dvla_authenticate, dvla_client, mock_upload_file, mock_create_print_request, rmock +): + rmock.get( + f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url", + json={ + "errors": [ + { + "status": "400 BAD_REQUEST", + "code": "NotNull", + "detail": "Bad request.", + }, + ] + }, + status_code=400, + ) + + with pytest.raises(DvlaNonRetryableException) as exc: + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + assert "Bad request." in str(exc.value) + assert mock_upload_file.called is False + assert mock_create_print_request.called is False + + +@pytest.mark.parametrize("status_code", [401, 403]) +def test_send_letter_when_auth_error_is_raised_getting_upload_url( + dvla_authenticate, dvla_client, mock_upload_file, mock_create_print_request, rmock, status_code +): + rmock.get( + f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url", + json={ + "errors": [ + { + "status": f"{status_code}", + "code": "Unauthorized", + "detail": "API Key or JWT is either not provided, expired or invalid", + } + ] + }, + status_code=status_code, + ) + + assert dvla_client.dvla_api_key.get() == "some api key" + + with pytest.raises(DvlaUnauthorisedRequestException) as exc: + dvla_client.send_letter( + notification_id="noti_id", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + + # make sure we clear down the api key + assert dvla_client.dvla_api_key._value is None + + assert "API Key or JWT is either not provided, expired or invalid" in str(exc.value) + assert mock_upload_file.called is False + assert mock_create_print_request.called is False + + +def test_send_letter_when_throttling_error_is_raised_getting_upload_url( + dvla_authenticate, dvla_client, mock_upload_file, mock_create_print_request, rmock +): + rmock.get( + f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url", + json={ + "errors": [ + { + "status": "429", + "title": "Too Many Requests", + "detail": "Too Many Requests", + } + ] + }, + status_code=429, + ) + + with pytest.raises(DvlaThrottlingException): + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + assert mock_upload_file.called is False + assert mock_create_print_request.called is False + + +def test_send_letter_when_5xx_status_code_is_returned_getting_upload_url( + dvla_authenticate, dvla_client, mock_upload_file, mock_create_print_request, rmock +): + url = f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url" + rmock.get( + url, + json={ + "errors": [ + { + "status": "500 INTERNAL_SERVER_ERROR", + "code": "2", + "detail": "An unexpected exception was raised by the service with Error Id: 16", + } + ] + }, + status_code=500, + ) + + with pytest.raises(DvlaRetryableException) as exc: + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + assert str(exc.value) == f"Received 500 from {url}" + assert mock_upload_file.called is False + assert mock_create_print_request.called is False + + +@pytest.mark.parametrize( + "exc_type", [ConnectionResetError, requests.exceptions.SSLError, requests.exceptions.ConnectTimeout] +) +def test_send_letter_when_connection_error_is_returned_getting_upload_url( + dvla_authenticate, + dvla_client, + mock_upload_file, + mock_create_print_request, + rmock, + exc_type, +): + rmock.get(f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url", exc=exc_type) + + with pytest.raises(DvlaRetryableException): + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + + assert mock_upload_file.called is False + assert mock_create_print_request.called is False + + +def test_send_letter_when_unknown_exception_is_raised_getting_upload_url( + dvla_authenticate, dvla_client, mock_upload_file, mock_create_print_request, rmock +): + rmock.get( + f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/files/upload-url", + status_code=418, + ) + + with pytest.raises(DvlaNonRetryableException): + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + + assert mock_upload_file.called is False + assert mock_create_print_request.called is False + + +def test_send_letter_when_429_error_is_raised_uploading_file_in_s3( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_create_print_request, rmock +): + # If rate limited by AWS, the status code should be a 503, which will be retried. However, since we're making use of + # error handling includes handling for 429s, this tests that logic anyway + rmock.put("https://s3bucketaddress/upload_id", status_code=429) + + with pytest.raises(DvlaThrottlingException): + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + + assert mock_create_print_request.called is False + + +def test_send_letter_when_5xx_status_code_is_returned_uploading_file_in_s3( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_create_print_request, rmock +): + url = "https://s3bucketaddress/upload_id" + rmock.put(url, status_code=503) + + with pytest.raises(DvlaRetryableException) as exc: + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + assert str(exc.value) == f"Received 503 from {url}" + assert mock_create_print_request.called is False + + +@pytest.mark.parametrize( + "exc_type", [ConnectionResetError, requests.exceptions.SSLError, requests.exceptions.ConnectTimeout] +) +def test_send_letter_when_connection_error_is_returned_uploading_file_in_s3( + dvla_authenticate, + dvla_client, + mock_get_presigned_url, + mock_create_print_request, + rmock, + exc_type, +): + rmock.put("https://s3bucketaddress/upload_id", exc=exc_type) + + with pytest.raises(DvlaRetryableException): + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + + assert mock_create_print_request.called is False + + +def test_send_letter_when_unknown_exception_is_raised_uploading_file_in_s3( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_create_print_request, rmock +): + rmock.put("https://s3bucketaddress/upload_id", status_code=418) + + with pytest.raises(DvlaNonRetryableException): + dvla_client.send_letter( + notification_id="1", + reference="ABCDEFGHIJKL", + address=PostalAddress("line\nline2\npostcode"), + postage="second", + service_id="s_id", + organisation_id="org_id", + pdf_file=b"pdf", + callback_url="/my-callback", + ) + + assert mock_create_print_request.called is False + + +def test_send_letter_when_bad_request_error_is_raised_posting_letter_data( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_upload_file, rmock +): rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={ @@ -701,7 +1049,9 @@ def test_send_letter_when_bad_request_error_is_raised(dvla_authenticate, dvla_cl @pytest.mark.parametrize("status_code", [401, 403]) -def test_send_letter_when_auth_error_is_raised(dvla_authenticate, dvla_client, rmock, status_code): +def test_send_letter_when_auth_error_is_raised_posting_letter_data( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_upload_file, rmock, status_code +): rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={ @@ -735,7 +1085,9 @@ def test_send_letter_when_auth_error_is_raised(dvla_authenticate, dvla_client, r assert "API Key or JWT is either not provided, expired or invalid" in str(exc.value) -def test_send_letter_when_conflict_error_is_raised(dvla_authenticate, dvla_client, rmock): +def test_send_letter_when_conflict_error_is_raised_posting_letter_data( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_upload_file, rmock +): rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={ @@ -768,7 +1120,9 @@ def test_send_letter_when_conflict_error_is_raised(dvla_authenticate, dvla_clien assert "The supplied identifier 1 conflicts with another print job" in str(exc.value) -def test_send_letter_when_throttling_error_is_raised(dvla_authenticate, dvla_client, rmock): +def test_send_letter_when_throttling_error_is_raised_posting_letter_data( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_upload_file, rmock +): rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", json={ @@ -796,7 +1150,9 @@ def test_send_letter_when_throttling_error_is_raised(dvla_authenticate, dvla_cli ) -def test_send_letter_when_5xx_status_code_is_returned(dvla_authenticate, dvla_client, rmock): +def test_send_letter_when_5xx_status_code_is_returned_posting_letter_data( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_upload_file, rmock +): url = f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs" rmock.post(url, status_code=500) @@ -817,7 +1173,14 @@ def test_send_letter_when_5xx_status_code_is_returned(dvla_authenticate, dvla_cl @pytest.mark.parametrize( "exc_type", [ConnectionResetError, requests.exceptions.SSLError, requests.exceptions.ConnectTimeout] ) -def test_send_letter_when_connection_error_is_returned(dvla_authenticate, dvla_client, rmock, exc_type): +def test_send_letter_when_connection_error_is_returned_posting_letter_data( + dvla_authenticate, + dvla_client, + mock_get_presigned_url, + mock_upload_file, + rmock, + exc_type, +): rmock.post(f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", exc=exc_type) with pytest.raises(DvlaRetryableException): @@ -833,7 +1196,9 @@ def test_send_letter_when_connection_error_is_returned(dvla_authenticate, dvla_c ) -def test_send_letter_when_unknown_exception_is_raised(dvla_authenticate, dvla_client, rmock): +def test_send_letter_when_unknown_exception_is_raised_posting_letter_data( + dvla_authenticate, dvla_client, mock_get_presigned_url, mock_upload_file, rmock +): rmock.post( f"{current_app.config['DVLA_API_BASE_URL']}/print-request/v1/print/jobs", status_code=418, @@ -1011,18 +1376,3 @@ def test_reject_cipher_not_accepted_by_server( ) assert "alert handshake failure" in str(e.value) - - -def test_format_create_print_job_json_adds_despatchMethod_key_for_economy_class_post(dvla_client): - formatted_json = dvla_client._format_create_print_job_json( - notification_id="my_notification_id", - reference="ABCDEFGHIJKL", - address=PostalAddress("A. User\nThe road\nCity\nSW1 1AA"), - postage="economy", - service_id="my_service_id", - organisation_id="my_organisation_id", - pdf_file=b"pdf_content", - callback_url="/my-callback", - ) - - assert formatted_json["standardParams"]["despatchMethod"] == "ECONOMY" From c83b4f6f8688101f196a4548729d52864c634e62 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 27 Mar 2026 10:14:01 +0000 Subject: [PATCH 2/3] Commit for testing - add Sentry This commit is for load testing only and will be removed when that is complete --- app/clients/letter/dvla.py | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/app/clients/letter/dvla.py b/app/clients/letter/dvla.py index 42316b8c69..c4907fc0f2 100644 --- a/app/clients/letter/dvla.py +++ b/app/clients/letter/dvla.py @@ -8,6 +8,7 @@ import boto3 import jwt import requests +import sentry_sdk from flask import current_app from notifications_utils.recipient_validation.postal_address import PostalAddress from requests.adapters import HTTPAdapter @@ -304,11 +305,13 @@ def send_letter( """ Sends a letter to the DVLA for printing """ - url_response = self._get_upload_url() + with sentry_sdk.start_span(op="http", description="DVLA GET upload URL"): + url_response = self._get_upload_url() upload_id = url_response["uploadId"] upload_url = url_response["uploadUrl"] - self._upload_file(upload_url=upload_url, pdf_file=pdf_file) + with sentry_sdk.start_span(op="http", description="DVLA PUT upload PDF"): + self._upload_file(upload_url=upload_url, pdf_file=pdf_file) current_app.logger.info( "Letter with notification id %s uploaded to DVLA presigned URL with upload_id %s", notification_id, @@ -328,21 +331,22 @@ def _handle_http_errors(e: requests.HTTPError): raise DvlaDuplicatePrintRequestException(e.response.json()["errors"][0]["detail"]) from e with _handle_common_dvla_errors(custom_httperror_exc_handler=_handle_http_errors): - response = self.session.post( - f"{self.base_url}/print-request/v1/print/jobs", - headers=self._get_auth_headers(), - json=self._format_create_print_job_json( - notification_id=notification_id, - reference=reference, - address=address, - postage=postage, - service_id=service_id, - organisation_id=organisation_id, - upload_id=upload_id, - callback_url=callback_url, - ), - ) - response.raise_for_status() + with sentry_sdk.start_span(op="http", description="DVLA POST create letter"): + response = self.session.post( + f"{self.base_url}/print-request/v1/print/jobs", + headers=self._get_auth_headers(), + json=self._format_create_print_job_json( + notification_id=notification_id, + reference=reference, + address=address, + postage=postage, + service_id=service_id, + organisation_id=organisation_id, + upload_id=upload_id, + callback_url=callback_url, + ), + ) + response.raise_for_status() return response.json() def _format_create_print_job_json( From 50456b49c3718166af4ff9d14383c4c28e461dd2 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 6 May 2026 11:40:30 +0100 Subject: [PATCH 3/3] wip --- app/clients/letter/dvla.py | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/app/clients/letter/dvla.py b/app/clients/letter/dvla.py index c4907fc0f2..45d518589b 100644 --- a/app/clients/letter/dvla.py +++ b/app/clients/letter/dvla.py @@ -272,8 +272,18 @@ def _handle_http_errors(e: requests.HTTPError): f"{self.base_url}/print-request/v1/print/files/upload-url", headers=self._get_auth_headers(), ) + try: + data = response.json() + except Exception: + current_app.logger.warning( + "Non-JSON response (status %s): %s", + response.status_code, + response.text, + ) + response.raise_for_status() + raise response.raise_for_status() - return response.json() + return data def _upload_file(self, *, upload_url: str, pdf_file: bytes): """ @@ -346,8 +356,20 @@ def _handle_http_errors(e: requests.HTTPError): callback_url=callback_url, ), ) + try: + data = response.json() + except Exception: + current_app.logger.warning( + "Non-JSON response for notification_id %s (status %s): %s", + notification_id, + response.status_code, + response.text, + ) response.raise_for_status() - return response.json() + raise + + response.raise_for_status() + return data def _format_create_print_job_json( self, *, notification_id, reference, address, postage, service_id, organisation_id, upload_id, callback_url