Remove new code smells

Change-Id: Ifec818702cca925d7e687af155d2253703faf27a
Issue-ID: NONRTRIC-142
Signed-off-by: elinuxhenrik <henrik.b.andersson@est.tech>

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 3cc1b77..cee9327 100644 (file)
--- 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 b0c2caf..6d1603c 100644 (file)
--- 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<ResponseEntity<String>> handleAgentCallError(Throwable t, DmaapRequestMessage dmaapRequestMessage) {
+    private Mono<ResponseEntity<String>> 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<ResponseEntity<String>> 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<String> 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 d937785..22905f6 100644 (file)
--- 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 19036ef..9f88d5c 100644 (file)
--- 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 626a9b6..8d7062b 100644 (file)
--- 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<String> 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 1ea677c..b0f1e7f 100644 (file)
--- 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 cd4bcb6..153c4ec 100644 (file)
--- 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 5ba538e..d034d4d 100644 (file)
--- 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<String> 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);