From 5e8fe3d5256872bda413aabb66cf217e6ef30cef Mon Sep 17 00:00:00 2001 From: elinuxhenrik Date: Mon, 9 Mar 2020 15:58:27 +0100 Subject: [PATCH] Remove new code smells Change-Id: Ifec818702cca925d7e687af155d2253703faf27a Issue-ID: NONRTRIC-142 Signed-off-by: elinuxhenrik --- .../policyagent/dmaap/DmaapMessageConsumer.java | 12 +++--------- .../policyagent/dmaap/DmaapMessageHandler.java | 22 +++++++++++++++++----- .../policyagent/tasks/RepositorySupervision.java | 2 +- .../policyagent/tasks/RicSynchronizationTask.java | 8 ++++---- .../policyagent/tasks/ServiceSupervision.java | 4 ++-- .../org/oransc/policyagent/MockPolicyAgent.java | 2 ++ .../dmaap/DmaapMessageConsumerTest.java | 8 ++++---- .../policyagent/dmaap/DmaapMessageHandlerTest.java | 4 ++-- 8 files changed, 35 insertions(+), 27 deletions(-) diff --git a/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageConsumer.java b/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageConsumer.java index 3cc1b77d..cee93274 100644 --- a/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageConsumer.java +++ b/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageConsumer.java @@ -22,7 +22,6 @@ package org.oransc.policyagent.dmaap; import com.google.common.collect.Iterables; -import java.io.FileNotFoundException; import java.io.IOException; import java.time.Duration; import java.util.Properties; @@ -49,7 +48,7 @@ public class DmaapMessageConsumer implements Runnable { private static final Logger logger = LoggerFactory.getLogger(DmaapMessageConsumer.class); - private final static Duration TIME_BETWEEN_DMAAP_POLLS = Duration.ofSeconds(10); + private static final Duration TIME_BETWEEN_DMAAP_POLLS = Duration.ofSeconds(10); private final ApplicationConfig applicationConfig; @@ -64,10 +63,6 @@ public class DmaapMessageConsumer implements Runnable { thread.start(); } - DmaapMessageConsumer(ApplicationConfig applicationConfig, boolean start) { - this.applicationConfig = applicationConfig; - } - private boolean isDmaapConfigured() { Properties consumerCfg = applicationConfig.getDmaapConsumerConfig(); Properties producerCfg = applicationConfig.getDmaapPublisherConfig(); @@ -133,7 +128,7 @@ public class DmaapMessageConsumer implements Runnable { } } - MRConsumer getMessageRouterConsumer(Properties dmaapConsumerProperties) throws FileNotFoundException, IOException { + MRConsumer getMessageRouterConsumer(Properties dmaapConsumerProperties) throws IOException { return MRClientFactory.createConsumer(dmaapConsumerProperties); } @@ -145,8 +140,7 @@ public class DmaapMessageConsumer implements Runnable { return new AsyncRestClient(agentBaseUrl); } - MRBatchingPublisher getMessageRouterPublisher(Properties dmaapPublisherProperties) - throws FileNotFoundException, IOException { + MRBatchingPublisher getMessageRouterPublisher(Properties dmaapPublisherProperties) throws IOException { return MRClientFactory.createBatchingPublisher(dmaapPublisherProperties); } } diff --git a/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageHandler.java b/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageHandler.java index b0c2cafa..6d1603c0 100644 --- a/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageHandler.java +++ b/policy-agent/src/main/java/org/oransc/policyagent/dmaap/DmaapMessageHandler.java @@ -66,9 +66,8 @@ public class DmaapMessageHandler { try { DmaapRequestMessage dmaapRequestMessage = gson.fromJson(msg, ImmutableDmaapRequestMessage.class); return this.invokePolicyAgent(dmaapRequestMessage) // - .onErrorResume(t -> handleAgentCallError(t, dmaapRequestMessage)) // + .onErrorResume(t -> handleAgentCallError(t, msg, dmaapRequestMessage)) // .flatMap( - response -> sendDmaapResponse(response.getBody(), dmaapRequestMessage, response.getStatusCode())); } catch (Exception e) { logger.warn("Received unparsable message from DMAAP: {}", msg); @@ -76,7 +75,8 @@ public class DmaapMessageHandler { } } - private Mono> handleAgentCallError(Throwable t, DmaapRequestMessage dmaapRequestMessage) { + private Mono> handleAgentCallError(Throwable t, String originalMessage, + DmaapRequestMessage dmaapRequestMessage) { logger.debug("Agent call failed: {}", t.getMessage()); HttpStatus status = HttpStatus.NOT_FOUND; String errorMessage = t.getMessage(); @@ -84,15 +84,27 @@ public class DmaapMessageHandler { WebClientResponseException exception = (WebClientResponseException) t; status = exception.getStatusCode(); errorMessage = exception.getResponseBodyAsString(); + } else if (t instanceof ServiceException) { + status = HttpStatus.BAD_REQUEST; + errorMessage = prepareBadOperationErrorMessage(t, originalMessage); + } return sendDmaapResponse(errorMessage, dmaapRequestMessage, status) // .flatMap(notUsed -> Mono.empty()); } + private String prepareBadOperationErrorMessage(Throwable t, String originalMessage) { + String operationParameterStart = "operation\":\""; + int indexOfOperationStart = originalMessage.indexOf(operationParameterStart) + operationParameterStart.length(); + int indexOfOperationEnd = originalMessage.indexOf("\",\"", indexOfOperationStart); + String badOperation = originalMessage.substring(indexOfOperationStart, indexOfOperationEnd); + return t.getMessage().replace("null", badOperation); + } + private Mono> invokePolicyAgent(DmaapRequestMessage dmaapRequestMessage) { DmaapRequestMessage.Operation operation = dmaapRequestMessage.operation(); - String uri = dmaapRequestMessage.url(); + if (operation == Operation.DELETE) { return agentClient.deleteForEntity(uri); } else if (operation == Operation.GET) { @@ -136,7 +148,7 @@ public class DmaapMessageHandler { } private Mono handleResponseCallError(Throwable t) { - logger.debug("Failed to respond: {}", t.getMessage()); + logger.debug("Failed to send response to DMaaP: {}", t.getMessage()); return Mono.empty(); } diff --git a/policy-agent/src/main/java/org/oransc/policyagent/tasks/RepositorySupervision.java b/policy-agent/src/main/java/org/oransc/policyagent/tasks/RepositorySupervision.java index d9377850..22905f6c 100644 --- a/policy-agent/src/main/java/org/oransc/policyagent/tasks/RepositorySupervision.java +++ b/policy-agent/src/main/java/org/oransc/policyagent/tasks/RepositorySupervision.java @@ -163,7 +163,7 @@ public class RepositorySupervision { return Mono.error(new Exception("Syncronization started")); } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally private void onRicChecked(RicData ric) { logger.debug("Ric: {} checked", ric.ric.name()); } diff --git a/policy-agent/src/main/java/org/oransc/policyagent/tasks/RicSynchronizationTask.java b/policy-agent/src/main/java/org/oransc/policyagent/tasks/RicSynchronizationTask.java index 19036efb..9f88d5cf 100644 --- a/policy-agent/src/main/java/org/oransc/policyagent/tasks/RicSynchronizationTask.java +++ b/policy-agent/src/main/java/org/oransc/policyagent/tasks/RicSynchronizationTask.java @@ -72,7 +72,7 @@ public class RicSynchronizationTask { this.services = services; } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally public void run(Ric ric) { logger.debug("Handling ric: {}", ric.getConfig().name()); @@ -101,7 +101,7 @@ public class RicSynchronizationTask { return Flux.concat(recoverTypes, policiesDeletedInRic, policiesRecreatedInRic); } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally private void onSynchronizationComplete(Ric ric) { logger.debug("Synchronization completed for: {}", ric.name()); ric.setState(RicState.IDLE); @@ -124,7 +124,7 @@ public class RicSynchronizationTask { } } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally private void onSynchronizationError(Ric ric, Throwable t) { logger.warn("Synchronization failed for ric: {}, reason: {}", ric.name(), t.getMessage()); // If synchronization fails, try to remove all instances @@ -142,7 +142,7 @@ public class RicSynchronizationTask { () -> onSynchronizationComplete(ric)); } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally private void onRecoveryError(Ric ric, Throwable t) { logger.warn("Synchronization failure recovery failed for ric: {}, reason: {}", ric.name(), t.getMessage()); ric.setState(RicState.UNDEFINED); diff --git a/policy-agent/src/main/java/org/oransc/policyagent/tasks/ServiceSupervision.java b/policy-agent/src/main/java/org/oransc/policyagent/tasks/ServiceSupervision.java index 626a9b69..8d7062b1 100644 --- a/policy-agent/src/main/java/org/oransc/policyagent/tasks/ServiceSupervision.java +++ b/policy-agent/src/main/java/org/oransc/policyagent/tasks/ServiceSupervision.java @@ -61,7 +61,7 @@ public class ServiceSupervision { createTask().subscribe(this::onPolicyDeleted, null, this::onComplete); } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally private void onPolicyDeleted(Policy policy) { logger.debug("Policy deleted due to inactivity: {}, service: {}", policy.id(), policy.ownerServiceName()); } @@ -95,7 +95,7 @@ public class ServiceSupervision { .map(nothing -> policy)); } - @SuppressWarnings("squid:S2629") + @SuppressWarnings("squid:S2629") // Invoke method(s) only conditionally private Mono handleDeleteFromRicFailure(Policy policy, Throwable e) { logger.warn("Could not delete policy: {} from ric: {}", policy.id(), policy.ric().name(), e); return Mono.empty(); diff --git a/policy-agent/src/test/java/org/oransc/policyagent/MockPolicyAgent.java b/policy-agent/src/test/java/org/oransc/policyagent/MockPolicyAgent.java index 1ea677ca..b0f1e7f4 100644 --- a/policy-agent/src/test/java/org/oransc/policyagent/MockPolicyAgent.java +++ b/policy-agent/src/test/java/org/oransc/policyagent/MockPolicyAgent.java @@ -144,6 +144,8 @@ public class MockPolicyAgent { } @Test + @SuppressWarnings("squid:S2699") // Tests should include assertions. This test is only for keeping the server alive, + // so it will only be confusing to add an assertion. public void runMock() throws Exception { keepServerAlive(); } diff --git a/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageConsumerTest.java b/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageConsumerTest.java index cd4bcb65..153c4ecc 100644 --- a/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageConsumerTest.java +++ b/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageConsumerTest.java @@ -68,7 +68,7 @@ public class DmaapMessageConsumerTest { @Test public void dmaapNotConfigured_thenDoNothing() { - messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock, false)); + messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock)); doReturn(true).when(messageConsumerUnderTest).sleep(any(Duration.class)); @@ -82,7 +82,7 @@ public class DmaapMessageConsumerTest { @Test public void dmaapConfiguredAndNoMessages_thenPollOnce() throws Exception { - messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock, false)); + messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock)); doReturn(true, false).when(messageConsumerUnderTest).sleep(any(Duration.class)); @@ -113,7 +113,7 @@ public class DmaapMessageConsumerTest { @Test public void dmaapConfiguredAndErrorGettingMessages_thenLogWarning() throws Exception { - messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock, false)); + messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock)); doReturn(true, false).when(messageConsumerUnderTest).sleep(any(Duration.class)); @@ -141,7 +141,7 @@ public class DmaapMessageConsumerTest { @Test public void dmaapConfiguredAndOneMessage_thenPollOnceAndProcessMessage() throws Exception { - messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock, false)); + messageConsumerUnderTest = spy(new DmaapMessageConsumer(applicationConfigMock)); doReturn(true, false).when(messageConsumerUnderTest).sleep(any(Duration.class)); diff --git a/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageHandlerTest.java b/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageHandlerTest.java index 5ba538e8..d034d4d0 100644 --- a/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageHandlerTest.java +++ b/policy-agent/src/test/java/org/oransc/policyagent/dmaap/DmaapMessageHandlerTest.java @@ -251,8 +251,8 @@ public class DmaapMessageHandlerTest { ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(dmaapClient).send(captor.capture()); String actualMessage = captor.getValue(); - assertThat(actualMessage.contains(HttpStatus.NOT_FOUND + "\",\"message\":\"Not implemented operation:")) - .isTrue(); + assertThat(actualMessage + .contains(HttpStatus.BAD_REQUEST + "\",\"message\":\"Not implemented operation: " + badOperation)).isTrue(); verify(dmaapClient).sendBatchWithResponse(); verifyNoMoreInteractions(dmaapClient); -- 2.16.6