From f72ab81a14a752373b614788d27b66dba6e9e2d1 Mon Sep 17 00:00:00 2001 From: Derek Fitchett <135860892+dfitchett@users.noreply.github.com> Date: Tue, 13 Feb 2024 11:42:17 -0800 Subject: [PATCH] Handle 204 from BIP (#2601) 204 handling to ep-merge, svc-bip-api, and mock-bip-claims-api, and catch all errors on dd submit metrics/distribution calls so jobs aren't interrupted. --- .../integration/test_merge_request.py | 14 +++++++++++ .../python_src/service/ep_merge_machine.py | 10 ++++---- .../src/python_src/util/metric_logger.py | 14 +++++++---- .../tests/responses/204_response.json | 4 ++++ .../ee-ep-merge-app/tests/service/conftest.py | 2 ++ .../tests/service/test_ep_merge_process.py | 24 +++++++++++++++++++ .../controller/BaseController.java | 4 ++++ .../controller/ContentionsController.java | 3 +++ .../src/main/resources/mock-claims.json | 11 +++++++++ .../gov/va/vro/bip/service/BipApiService.java | 8 ++++++- .../va/vro/bip/service/BipApiServiceTest.java | 22 +++++++++++++---- 11 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 domain-ee/ee-ep-merge-app/tests/responses/204_response.json diff --git a/domain-ee/ee-ep-merge-app/integration/test_merge_request.py b/domain-ee/ee-ep-merge-app/integration/test_merge_request.py index 9f2b055e06..12ccb04de0 100644 --- a/domain-ee/ee-ep-merge-app/integration/test_merge_request.py +++ b/domain-ee/ee-ep-merge-app/integration/test_merge_request.py @@ -7,6 +7,7 @@ RESPONSE_DIR = './tests/responses' response_200 = f'{RESPONSE_DIR}/200_response.json' response_201 = f'{RESPONSE_DIR}/201_response.json' +response_204 = f'{RESPONSE_DIR}/204_response.json' response_404 = f'{RESPONSE_DIR}/404_response.json' response_400 = f'{RESPONSE_DIR}/400_response.json' response_500 = f'{RESPONSE_DIR}/500_response.json' @@ -92,6 +93,19 @@ async def test_completed_success_with_duplicate_contention( response = await submit_request_and_process(client) assert_successful_response(response) + @pytest.mark.asyncio(scope="session") + async def test_completed_no_ep400_contentions( + self, get_claim_endpoint: MqEndpoint, get_claim_contentions_endpoint: MqEndpoint, put_tsoj_endpoint: MqEndpoint, cancel_claim_endpoint: MqEndpoint + ): + get_claim_endpoint.set_responses([pending_claim_200]) + get_claim_contentions_endpoint.set_responses([pending_contentions_200, response_204]) + put_tsoj_endpoint.set_responses([response_200]) + cancel_claim_endpoint.set_responses([response_200]) + + async with AsyncClient(app=app, base_url="http://test") as client: + response = await submit_request_and_process(client) + assert_successful_response(response) + class TestErrorAtGetPendingClaim(TestMergeRequestBase): diff --git a/domain-ee/ee-ep-merge-app/src/python_src/service/ep_merge_machine.py b/domain-ee/ee-ep-merge-app/src/python_src/service/ep_merge_machine.py index 0e9e03273f..af3d2dbac7 100644 --- a/domain-ee/ee-ep-merge-app/src/python_src/service/ep_merge_machine.py +++ b/domain-ee/ee-ep-merge-app/src/python_src/service/ep_merge_machine.py @@ -168,7 +168,7 @@ def on_get_pending_contentions(self, event): @running_get_ep400_contentions.enter def on_get_ep400_contentions(self, event, pending_contentions_response=None): request = get_contentions.Request(claim_id=self.job.ep400_claim_id) - response = self.make_request(request=request, hoppy_client=HOPPY.get_client(ClientName.GET_CLAIM_CONTENTIONS), response_type=get_contentions.Response) + response = self.make_request(request=request, hoppy_client=HOPPY.get_client(ClientName.GET_CLAIM_CONTENTIONS), response_type=get_contentions.Response, expected_statuses=[200, 204]) self.send(event=event, pending_contentions_response=pending_contentions_response, ep400_contentions_response=response) @running_set_temp_station_of_jurisdiction.enter @@ -191,7 +191,7 @@ def on_merge_contentions(self, event, pending_contentions_response=None, ep400_c def on_move_contentions_to_pending_claim(self, event, new_contentions=None, ep400_contentions_response=None): request = create_contentions.Request(claim_id=self.job.pending_claim_id, create_contentions=new_contentions) self.make_request( - request=request, hoppy_client=HOPPY.get_client(ClientName.CREATE_CLAIM_CONTENTIONS), response_type=create_contentions.Response, expected_status=201 + request=request, hoppy_client=HOPPY.get_client(ClientName.CREATE_CLAIM_CONTENTIONS), response_type=create_contentions.Response, expected_statuses=201 ) self.send(event=event, ep400_contentions_response=ep400_contentions_response) @@ -291,13 +291,15 @@ def log_metrics(self, job_duration): if self.skipped_merge: increment(JOB_SKIPPED_MERGE_METRIC) - def make_request(self, request: GeneralRequest, hoppy_client: AsyncHoppyClient, response_type: Type[GeneralResponse], expected_status: int = 200): + def make_request(self, request: GeneralRequest, hoppy_client: AsyncHoppyClient, response_type: Type[GeneralResponse], expected_statuses: list[int] | int = 200): + if not isinstance(expected_statuses, list): + expected_statuses = [expected_statuses] try: loop = asyncio.new_event_loop() req = hoppy_client.make_request(self.job.job_id, request.model_dump(by_alias=True)) response = loop.run_until_complete(req) model = response_type.model_validate(response) - if model.status_code != expected_status: + if model.status_code not in expected_statuses: self.add_error( model.messages if model.messages diff --git a/domain-ee/ee-ep-merge-app/src/python_src/util/metric_logger.py b/domain-ee/ee-ep-merge-app/src/python_src/util/metric_logger.py index debf524c03..fc04f93184 100644 --- a/domain-ee/ee-ep-merge-app/src/python_src/util/metric_logger.py +++ b/domain-ee/ee-ep-merge-app/src/python_src/util/metric_logger.py @@ -39,11 +39,12 @@ def increment(metric: str, value: float = 1): :param metric: string containing the metric name :param value: value to increment by """ + full_metric = f'{APP_PREFIX}.{metric.strip(".").lower()}' body = MetricPayload( series=[ MetricSeries( - metric=f'{APP_PREFIX}.{metric.strip(".").lower()}', + metric=full_metric, type=MetricIntakeType.COUNT, points=[ MetricPoint( @@ -59,7 +60,9 @@ def increment(metric: str, value: float = 1): try: count_metrics_api.submit_metrics(body=body) except ApiException as e: - logging.warning(f'event=logMetricFailed status={e.status} reason={e.reason} body={e.body}') + logging.warning(f'event=logMetricFailed metric={full_metric} type=count value={value} status={e.status} reason={e.reason} body={e.body}') + except Exception as e: + logging.warning(f'event=logMetricFailed metric={full_metric} type=count value={value} type={type(e)} error="{e}"') def distribution(metric: str, value: float): @@ -68,11 +71,12 @@ def distribution(metric: str, value: float): :param metric: string containing the metric name :param value: value to increment by """ + full_metric = f'{APP_PREFIX}.{metric.strip(".").lower()}.distribution' body = DistributionPointsPayload( series=[ DistributionPointsSeries( - metric=f'{APP_PREFIX}.{metric.strip(".").lower()}.distribution', + metric=full_metric, points=[ DistributionPoint( [ @@ -89,4 +93,6 @@ def distribution(metric: str, value: float): try: distribution_metrics_api.submit_distribution_points(body=body) except ApiException as e: - logging.warning(f'event=logMetricFailed status={e.status} reason={e.reason} body={e.body}') + logging.warning(f'event=logMetricFailed metric={full_metric} type=distribution value={value} status={e.status} reason={e.reason} body={e.body}') + except Exception as e: + logging.warning(f'event=logMetricFailed metric={full_metric} type=distribution value={value} type={type(e)} error="{e}"') diff --git a/domain-ee/ee-ep-merge-app/tests/responses/204_response.json b/domain-ee/ee-ep-merge-app/tests/responses/204_response.json new file mode 100644 index 0000000000..7ce5eb82ac --- /dev/null +++ b/domain-ee/ee-ep-merge-app/tests/responses/204_response.json @@ -0,0 +1,4 @@ +{ + "statusCode": 204, + "statusMessage": "NO CONTENT" +} diff --git a/domain-ee/ee-ep-merge-app/tests/service/conftest.py b/domain-ee/ee-ep-merge-app/tests/service/conftest.py index ceddcf2c49..54dfc43a25 100644 --- a/domain-ee/ee-ep-merge-app/tests/service/conftest.py +++ b/domain-ee/ee-ep-merge-app/tests/service/conftest.py @@ -36,6 +36,7 @@ RESPONSE_DIR = os.path.abspath('./tests/responses') response_200 = f'{RESPONSE_DIR}/200_response.json' response_201 = f'{RESPONSE_DIR}/201_response.json' +response_204 = f'{RESPONSE_DIR}/204_response.json' response_404 = f'{RESPONSE_DIR}/404_response.json' response_400 = f'{RESPONSE_DIR}/400_response.json' response_500 = f'{RESPONSE_DIR}/500_response.json' @@ -63,6 +64,7 @@ def load_response(file, response_type): get_pending_contentions_increase_tinnitus_200 = load_response(pending_contentions_increase_tinnitus_200, get_contentions.Response) get_ep400_contentions_req = get_contentions.Request(claim_id=EP400_CLAIM_ID).model_dump(by_alias=True) get_ep400_contentions_200 = load_response(ep400_contentions_increase_tinnitus_200, get_contentions.Response) +get_ep400_contentions_204 = load_response(response_204, get_contentions.Response) get_ep400_contentions_without_special_issues_200 = load_response(ep400_contentions_increase_tinnitus_without_special_issues_200, get_contentions.Response) update_temporary_station_of_jurisdiction_req = tsoj.Request(claim_id=EP400_CLAIM_ID, temp_station_of_jurisdiction="398").model_dump(by_alias=True) revert_temporary_station_of_jurisdiction_req = tsoj.Request(claim_id=EP400_CLAIM_ID, temp_station_of_jurisdiction="111").model_dump(by_alias=True) diff --git a/domain-ee/ee-ep-merge-app/tests/service/test_ep_merge_process.py b/domain-ee/ee-ep-merge-app/tests/service/test_ep_merge_process.py index 626921c9d9..ade6dc2825 100644 --- a/domain-ee/ee-ep-merge-app/tests/service/test_ep_merge_process.py +++ b/domain-ee/ee-ep-merge-app/tests/service/test_ep_merge_process.py @@ -16,6 +16,7 @@ ep400_contentions_increase_tinnitus_200, ep400_contentions_new_tinnitus_200, get_ep400_contentions_200, + get_ep400_contentions_204, get_ep400_contentions_req, get_pending_claim_200, get_pending_claim_req, @@ -733,3 +734,26 @@ def test_process_succeeds_with_duplicate_contention(self, machine, mock_hoppy_as ] ) assert_metrics_called(metric_logger_distribution, metric_logger_increment, JobState.COMPLETED_SUCCESS, None, 0, True) + + def test_process_succeeds_with_no_ep400_contentions(self, machine, mock_hoppy_async_client, metric_logger_distribution, metric_logger_increment): + mock_async_responses( + mock_hoppy_async_client, + [ + get_pending_claim_200, + get_pending_contentions_increase_tinnitus_200, + get_ep400_contentions_204, + update_temporary_station_of_jurisdiction_200, + cancel_claim_200, + add_claim_note_200, + ], + ) + process_and_assert(machine, JobState.COMPLETED_SUCCESS) + mock_hoppy_async_client.make_request.assert_has_calls( + [ + call(machine.job.job_id, get_pending_contentions_req), + call(machine.job.job_id, get_ep400_contentions_req), + call(machine.job.job_id, update_temporary_station_of_jurisdiction_req), + call(machine.job.job_id, cancel_ep400_claim_req), + ] + ) + assert_metrics_called(metric_logger_distribution, metric_logger_increment, JobState.COMPLETED_SUCCESS, None, 0, True) diff --git a/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/BaseController.java b/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/BaseController.java index f5d2728271..f379742cba 100644 --- a/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/BaseController.java +++ b/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/BaseController.java @@ -17,6 +17,10 @@ protected ResponseEntity create201(T response) { return new ResponseEntity<>(response, HttpStatus.CREATED); } + protected ResponseEntity create204() { + return ResponseEntity.noContent().build(); + } + protected ResponseEntity create500(T response) { response.addMessagesItem(createInternalServerMessage()); return new ResponseEntity<>(response, HttpStatus.INTERNAL_SERVER_ERROR); diff --git a/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/ContentionsController.java b/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/ContentionsController.java index bbc848031e..2dd6ca0072 100644 --- a/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/ContentionsController.java +++ b/mocks/mock-bip-claims-api/src/main/java/gov/va/vro/mockbipclaims/controller/ContentionsController.java @@ -84,6 +84,9 @@ public ResponseEntity getContentionsForClaim(Long c } List contentions = item.getContentions(); + if (contentions.isEmpty()) { + return create204(); + } response.setContentions(contentions); diff --git a/mocks/mock-bip-claims-api/src/main/resources/mock-claims.json b/mocks/mock-bip-claims-api/src/main/resources/mock-claims.json index bafc48004b..4ab0898123 100644 --- a/mocks/mock-bip-claims-api/src/main/resources/mock-claims.json +++ b/mocks/mock-bip-claims-api/src/main/resources/mock-claims.json @@ -962,6 +962,17 @@ } ] }, + { + "description": [ + "Employee Experience EP Merge End 2 End Testing - EP 400 w/ no contentions" + ], + "claimDetail": { + "claimId": 10006, + "phase": "Claim Received", + "endProductCode": "400", + "claimLifecycleStatus": "Open" + } + }, { "description": [ "Employee Experience EP Merge End 2 End Testing - Fails at get claim details" diff --git a/svc-bip-api/src/main/java/gov/va/vro/bip/service/BipApiService.java b/svc-bip-api/src/main/java/gov/va/vro/bip/service/BipApiService.java index 7c9c9cc068..e699a49c96 100644 --- a/svc-bip-api/src/main/java/gov/va/vro/bip/service/BipApiService.java +++ b/svc-bip-api/src/main/java/gov/va/vro/bip/service/BipApiService.java @@ -146,8 +146,14 @@ private T makeRequest( url, method, bipResponse.getStatusCode().value()); + BipPayloadResponse.BipPayloadResponseBuilder responseBuilder; + if (bipResponse.hasBody()) { + responseBuilder = mapper.readValue(bipResponse.getBody(), expectedResponse).toBuilder(); + } else { + responseBuilder = mapper.readValue("{}", expectedResponse).toBuilder(); + } return (T) - mapper.readValue(bipResponse.getBody(), expectedResponse).toBuilder() + responseBuilder .statusCode(bipResponse.getStatusCode().value()) .statusMessage(HttpStatus.valueOf(bipResponse.getStatusCode().value()).name()) .build(); diff --git a/svc-bip-api/src/test/java/gov/va/vro/bip/service/BipApiServiceTest.java b/svc-bip-api/src/test/java/gov/va/vro/bip/service/BipApiServiceTest.java index 7673852fd0..c1dccdf70b 100644 --- a/svc-bip-api/src/test/java/gov/va/vro/bip/service/BipApiServiceTest.java +++ b/svc-bip-api/src/test/java/gov/va/vro/bip/service/BipApiServiceTest.java @@ -204,6 +204,18 @@ public void testGetClaimContention_200() throws Exception { assertEquals(1, result.getContentions().size()); } + @Test + public void testGetClaimContention_204() throws Exception { + ResponseEntity resp204 = ResponseEntity.noContent().build(); + + mockResponseForUrl( + Mockito.doReturn(resp204), formatClaimUrl(CONTENTION, GOOD_CLAIM_ID), HttpMethod.GET); + + GetClaimContentionsResponse result = service.getClaimContentions(GOOD_CLAIM_ID); + assertResponseIsSuccess(result, HttpStatus.NO_CONTENT); + assertNull(result.getContentions()); + } + @Test public void testGetClaimContentions_404() throws Exception { String resp404Body = getTestData(CLAIM_RESPONSE_404); @@ -650,12 +662,12 @@ public enum TestCase { this.ex = status == HttpStatus.INTERNAL_SERVER_ERROR ? new HttpServerErrorException( - status, status.name(), getTestData(dataFile).getBytes(), Charset.defaultCharset()) + status, status.name(), getTestData(dataFile).getBytes(), Charset.defaultCharset()) : new HttpClientErrorException( - status, - status.name(), - getTestData(dataFile).getBytes(), - Charset.defaultCharset()); + status, + status.name(), + getTestData(dataFile).getBytes(), + Charset.defaultCharset()); } } }