From 56bc6f68baeb376b99e0dbe4de4790f494dbe3c2 Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Mon, 23 Mar 2020 15:04:46 -0400 Subject: New Guard actor request structure is incorrect Missing various fields within the request structure (e.g., ONAPName). Fixed. In the process, also modified makeGuardPayload() so that it only constructs the inner "guard" JSON object, making it easier for invoking code to modify it. Issue-ID: POLICY-2434 Signed-off-by: Jim Hahn Change-Id: I7d34a279845bb98425caf66565eab7513d310815 --- .../controlloop/actor/guard/GuardConfig.java | 31 ++++--------- .../controlloop/actor/guard/GuardOperation.java | 14 +++--- .../controlloop/actor/guard/GuardParams.java | 11 +++-- .../controlloop/actor/guard/GuardConfigTest.java | 5 +-- .../actor/guard/GuardOperationTest.java | 52 ++++++++-------------- .../controlloop/actor/guard/GuardParamsTest.java | 9 ++-- .../actor.guard/src/test/resources/makeReq.json | 10 ----- .../src/test/resources/makeReqDefault.json | 8 +++- .../actor.guard/src/test/resources/makeReqStd.json | 12 ++--- .../actor.guard/src/test/resources/service.yaml | 4 ++ .../controlloop/actor/so/VfModuleCreateTest.java | 10 +---- .../controlloop/actor/so/VfModuleDeleteTest.java | 10 +---- .../impl/OperationPartial.java | 11 +---- .../impl/OperationPartialTest.java | 12 ++--- 14 files changed, 71 insertions(+), 128 deletions(-) delete mode 100644 models-interactions/model-actors/actor.guard/src/test/resources/makeReq.json diff --git a/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardConfig.java b/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardConfig.java index 0e711d1be..7dca3bf37 100644 --- a/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardConfig.java +++ b/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardConfig.java @@ -20,19 +20,18 @@ package org.onap.policy.controlloop.actor.guard; -import java.util.LinkedHashMap; -import java.util.Map; import java.util.concurrent.Executor; import lombok.Getter; import org.onap.policy.common.endpoints.http.client.HttpClient; import org.onap.policy.common.endpoints.http.client.HttpClientFactory; import org.onap.policy.controlloop.actorserviceprovider.parameters.HttpConfig; +import org.onap.policy.models.decisions.concepts.DecisionRequest; /** * Configuration for Guard Operators. */ public class GuardConfig extends HttpConfig { - private final Map defaultRequest = new LinkedHashMap<>(); + private final DecisionRequest defaultRequest = new DecisionRequest(); /** * {@code True} if the associated guard operation is disabled. @@ -50,32 +49,20 @@ public class GuardConfig extends HttpConfig { public GuardConfig(Executor blockingExecutor, GuardParams params, HttpClientFactory clientFactory) { super(blockingExecutor, params, clientFactory); - addProperty("ONAPComponent", params.getOnapComponent()); - addProperty("ONAPInstance", params.getOnapInstance()); - addProperty("ONAPName", params.getOnapName()); - addProperty("action", params.getAction()); + defaultRequest.setOnapComponent(params.getOnapComponent()); + defaultRequest.setOnapInstance(params.getOnapInstance()); + defaultRequest.setOnapName(params.getOnapName()); + defaultRequest.setAction(params.getAction()); this.disabled = params.isDisabled(); } - /** - * Adds a property to the default request, if the value is not {@code null}. - * - * @param key property key - * @param value property value, or {@code null} - */ - private void addProperty(String key, String value) { - if (value != null) { - defaultRequest.put(key, value); - } - } - /** * Creates a new request, with the default values. * - * @return a new request map + * @return a new request */ - public Map makeRequest() { - return new LinkedHashMap<>(defaultRequest); + public DecisionRequest makeRequest() { + return new DecisionRequest(defaultRequest); } } diff --git a/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardOperation.java b/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardOperation.java index b9dfb9eba..edd2e230d 100644 --- a/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardOperation.java +++ b/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardOperation.java @@ -31,7 +31,6 @@ import org.onap.policy.common.endpoints.event.comm.Topic.CommInfrastructure; import org.onap.policy.common.endpoints.utils.NetLoggerUtil.EventType; import org.onap.policy.controlloop.actorserviceprovider.CallbackManager; import org.onap.policy.controlloop.actorserviceprovider.OperationOutcome; -import org.onap.policy.controlloop.actorserviceprovider.Util; import org.onap.policy.controlloop.actorserviceprovider.impl.HttpOperation; import org.onap.policy.controlloop.actorserviceprovider.impl.OperationPartial; import org.onap.policy.controlloop.actorserviceprovider.parameters.ControlLoopOperationParams; @@ -109,8 +108,7 @@ public class GuardOperation extends HttpOperation { protected CompletableFuture startOperationAsync(int attempt, OperationOutcome outcome) { outcome.setSubRequestId(String.valueOf(attempt)); - DecisionRequest request = Util.translate(getName(), makeRequest(), DecisionRequest.class); - + DecisionRequest request = makeRequest(); Entity entity = Entity.entity(request, MediaType.APPLICATION_JSON); Map headers = makeHeaders(); @@ -129,16 +127,16 @@ public class GuardOperation extends HttpOperation { /** * Makes a request from the payload. * - * @return a new request map + * @return a new request */ - protected Map makeRequest() { + protected DecisionRequest makeRequest() { if (params.getPayload() == null) { throw new IllegalArgumentException("missing payload"); } - Map req = config.makeRequest(); - req.putAll(params.getPayload()); - req.computeIfAbsent("requestId", key -> UUID.randomUUID().toString()); + DecisionRequest req = config.makeRequest(); + req.setRequestId(UUID.randomUUID().toString()); + req.setResource(Map.of("guard", params.getPayload())); return req; } diff --git a/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardParams.java b/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardParams.java index 5f8360377..959b4d3ef 100644 --- a/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardParams.java +++ b/models-interactions/model-actors/actor.guard/src/main/java/org/onap/policy/controlloop/actor/guard/GuardParams.java @@ -23,20 +23,19 @@ package org.onap.policy.controlloop.actor.guard; import lombok.Data; import lombok.EqualsAndHashCode; import lombok.experimental.SuperBuilder; +import org.onap.policy.common.parameters.annotations.NotBlank; +import org.onap.policy.common.parameters.annotations.NotNull; import org.onap.policy.controlloop.actorserviceprovider.parameters.HttpParams; /** - * Default values to be included if not specified in the payload. + * Guard parameters. */ +@NotBlank +@NotNull @Data @EqualsAndHashCode(callSuper = true) @SuperBuilder(toBuilder = true) public class GuardParams extends HttpParams { - - /* - * Optional, default values that are used if missing from the payload. - */ - private String onapName; private String onapComponent; private String onapInstance; diff --git a/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardConfigTest.java b/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardConfigTest.java index 49c1c916a..d30c71172 100644 --- a/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardConfigTest.java +++ b/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardConfigTest.java @@ -33,7 +33,6 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.onap.policy.common.endpoints.http.client.HttpClient; import org.onap.policy.common.endpoints.http.client.HttpClientFactory; -import org.onap.policy.controlloop.actorserviceprovider.Util; import org.onap.policy.models.decisions.concepts.DecisionRequest; public class GuardConfigTest { @@ -77,7 +76,7 @@ public class GuardConfigTest { expected.setOnapName(ONAP_NAME); expected.setAction(MY_ACTION); - DecisionRequest actual = Util.translate("", config.makeRequest(), DecisionRequest.class); + DecisionRequest actual = config.makeRequest(); assertEquals(expected, actual); // check value from superclass @@ -89,7 +88,7 @@ public class GuardConfigTest { config = new GuardConfig(executor, params, factory); assertFalse(config.isDisabled()); - actual = Util.translate("", config.makeRequest(), DecisionRequest.class); + actual = config.makeRequest(); assertEquals(new DecisionRequest(), actual); // try with disabled=true diff --git a/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardOperationTest.java b/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardOperationTest.java index be40ef3b4..13a241124 100644 --- a/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardOperationTest.java +++ b/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardOperationTest.java @@ -23,6 +23,7 @@ package org.onap.policy.controlloop.actor.guard; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -63,7 +64,14 @@ public class GuardOperationTest extends BasicHttpOperation { super.setUpBasic(); guardConfig = mock(GuardConfig.class); - when(guardConfig.makeRequest()).thenAnswer(args -> new TreeMap<>(Map.of("action", "guard"))); + when(guardConfig.makeRequest()).thenAnswer(args -> { + DecisionRequest req = new DecisionRequest(); + req.setAction("guard"); + req.setOnapComponent("my-onap-component"); + req.setOnapInstance("my-onap-instance"); + req.setOnapName("my-onap-name"); + return req; + }); config = guardConfig; initConfig(); @@ -128,19 +136,6 @@ public class GuardOperationTest extends BasicHttpOperation { verifyPayload("makeReqStd.json", makePayload()); verifyPayload("makeReqDefault.json", new TreeMap<>()); - Map payload = new TreeMap<>(); - payload.put("action", "some action"); - payload.put("hello", "world"); - payload.put("r u there?", "yes"); - payload.put("requestId", "some request id"); - - Map resource = new TreeMap<>(); - payload.put("resource", resource); - resource.put("abc", "def"); - resource.put("ghi", "jkl"); - - verifyPayload("makeReq.json", payload); - // null payload - start with fresh parameters and operation params = params.toBuilder().payload(null).build(); oper = new GuardOperation(params, config); @@ -151,9 +146,16 @@ public class GuardOperationTest extends BasicHttpOperation { params.getPayload().clear(); params.getPayload().putAll(payload); - Map requestMap = oper.makeRequest(); + DecisionRequest request = oper.makeRequest(); + + assertEquals("guard", request.getAction()); + assertEquals("my-onap-component", request.getOnapComponent()); + assertEquals("my-onap-instance", request.getOnapInstance()); + assertEquals("my-onap-name", request.getOnapName()); + assertNotNull(request.getRequestId()); + assertEquals(Map.of("guard", payload), request.getResource()); - verifyRequest(expectedJsonFile, requestMap, "requestId"); + verifyRequest(expectedJsonFile, request, "requestId"); } @Test @@ -189,22 +191,6 @@ public class GuardOperationTest extends BasicHttpOperation { @Override protected Map makePayload() { - DecisionRequest req = new DecisionRequest(); - req.setAction("my-action"); - req.setOnapComponent("my-onap-component"); - req.setOnapInstance("my-onap-instance"); - req.setOnapName("my-onap-name"); - req.setRequestId("my-request-id"); - - // add resources - Map resource = new TreeMap<>(); - req.setResource(resource); - resource.put("actor", "resource-actor"); - resource.put("operation", "resource-operation"); - - @SuppressWarnings("unchecked") - Map map = Util.translate("", req, TreeMap.class); - - return map; + return new TreeMap<>(Map.of("hello", "world", "abc", "123")); } } diff --git a/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardParamsTest.java b/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardParamsTest.java index 172368349..102dcbb08 100644 --- a/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardParamsTest.java +++ b/models-interactions/model-actors/actor.guard/src/test/java/org/onap/policy/controlloop/actor/guard/GuardParamsTest.java @@ -60,12 +60,13 @@ public class GuardParamsTest { public void testValidate() { assertTrue(params.validate(CONTAINER).isValid()); + testValidateField("onapName", "null", bldr -> bldr.onapName(null)); + testValidateField("onapComponent", "null", bldr -> bldr.onapComponent(null)); + testValidateField("onapInstance", "null", bldr -> bldr.onapInstance(null)); + testValidateField("action", "null", bldr -> bldr.action(null)); + // validate one of the superclass fields testValidateField("clientName", "null", bldr -> bldr.clientName(null)); - - // validate with mostly empty params - params = GuardParams.builder().clientName(CLIENT).path(PATH).timeoutSec(TIMEOUT).build(); - assertTrue(params.validate(CONTAINER).isValid()); } @Test diff --git a/models-interactions/model-actors/actor.guard/src/test/resources/makeReq.json b/models-interactions/model-actors/actor.guard/src/test/resources/makeReq.json deleted file mode 100644 index 2716d030e..000000000 --- a/models-interactions/model-actors/actor.guard/src/test/resources/makeReq.json +++ /dev/null @@ -1,10 +0,0 @@ -{ - "action": "some action", - "hello": "world", - "r u there?": "yes", - "requestId": "abcdefghi", - "resource": { - "abc": "def", - "ghi": "jkl" - } -} diff --git a/models-interactions/model-actors/actor.guard/src/test/resources/makeReqDefault.json b/models-interactions/model-actors/actor.guard/src/test/resources/makeReqDefault.json index d757eb993..a08f13f43 100644 --- a/models-interactions/model-actors/actor.guard/src/test/resources/makeReqDefault.json +++ b/models-interactions/model-actors/actor.guard/src/test/resources/makeReqDefault.json @@ -1,4 +1,10 @@ { + "ONAPName": "my-onap-name", + "ONAPComponent": "my-onap-component", + "ONAPInstance": "my-onap-instance", + "requestId": "abcdefghi", "action": "guard", - "requestId": "abcdefghi" + "resource": { + "guard": {} + } } diff --git a/models-interactions/model-actors/actor.guard/src/test/resources/makeReqStd.json b/models-interactions/model-actors/actor.guard/src/test/resources/makeReqStd.json index 6ae886726..fd1f4c716 100644 --- a/models-interactions/model-actors/actor.guard/src/test/resources/makeReqStd.json +++ b/models-interactions/model-actors/actor.guard/src/test/resources/makeReqStd.json @@ -1,11 +1,13 @@ { + "ONAPName": "my-onap-name", "ONAPComponent": "my-onap-component", "ONAPInstance": "my-onap-instance", - "ONAPName": "my-onap-name", - "action": "my-action", "requestId": "abcdefghi", + "action": "guard", "resource": { - "actor": "resource-actor", - "operation": "resource-operation" + "guard": { + "abc": "123", + "hello": "world" + } } -} \ No newline at end of file +} diff --git a/models-interactions/model-actors/actor.guard/src/test/resources/service.yaml b/models-interactions/model-actors/actor.guard/src/test/resources/service.yaml index 131cf785b..998928d44 100644 --- a/models-interactions/model-actors/actor.guard/src/test/resources/service.yaml +++ b/models-interactions/model-actors/actor.guard/src/test/resources/service.yaml @@ -26,6 +26,10 @@ httpClients: actors: GUARD: clientName: my-client + onapName: my-onap-name + onapComponent: my-onap-component + onapInstance: my-onap-instance + action: guard operations: Decision: path: decide \ No newline at end of file diff --git a/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleCreateTest.java b/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleCreateTest.java index 8c084b8dc..1d5d44cc8 100644 --- a/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleCreateTest.java +++ b/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleCreateTest.java @@ -132,15 +132,7 @@ public class VfModuleCreateTest extends BasicSoOperation { Map payload = captor.getValue().getPayload(); assertNotNull(payload); - @SuppressWarnings("unchecked") - Map resource = (Map) payload.get("resource"); - assertNotNull(resource); - - @SuppressWarnings("unchecked") - Map guard = (Map) resource.get("guard"); - assertNotNull(guard); - - Integer newCount = (Integer) guard.get(VfModuleCreate.PAYLOAD_KEY_VF_COUNT); + Integer newCount = (Integer) payload.get(VfModuleCreate.PAYLOAD_KEY_VF_COUNT); assertNotNull(newCount); assertEquals(origCount + 1, newCount.intValue()); } diff --git a/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleDeleteTest.java b/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleDeleteTest.java index c08fa37d1..cb2bceffd 100644 --- a/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleDeleteTest.java +++ b/models-interactions/model-actors/actor.so/src/test/java/org/onap/policy/controlloop/actor/so/VfModuleDeleteTest.java @@ -166,15 +166,7 @@ public class VfModuleDeleteTest extends BasicSoOperation { Map payload = captor.getValue().getPayload(); assertNotNull(payload); - @SuppressWarnings("unchecked") - Map resource = (Map) payload.get("resource"); - assertNotNull(resource); - - @SuppressWarnings("unchecked") - Map guard = (Map) resource.get("guard"); - assertNotNull(guard); - - Integer newCount = (Integer) guard.get(VfModuleDelete.PAYLOAD_KEY_VF_COUNT); + Integer newCount = (Integer) payload.get(VfModuleDelete.PAYLOAD_KEY_VF_COUNT); assertNotNull(newCount); assertEquals(origCount - 1, newCount.intValue()); } diff --git a/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java b/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java index cdbdc2af5..a9d7f4e58 100644 --- a/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java +++ b/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java @@ -230,14 +230,7 @@ public abstract class OperationPartial implements Operation { */ protected CompletableFuture startGuardAsync() { // get the guard payload - Map guardPayload = makeGuardPayload(); - - // wrap it in a "resource" - Map resource = new LinkedHashMap<>(); - resource.put("guard", guardPayload); - - Map payload = new LinkedHashMap<>(); - payload.put("resource", resource); + Map payload = makeGuardPayload(); /* * Note: can't use constants from actor.guard, because that would create a @@ -255,7 +248,7 @@ public abstract class OperationPartial implements Operation { protected Map makeGuardPayload() { Map guard = new LinkedHashMap<>(); guard.put("actor", params.getActor()); - guard.put("recipe", params.getOperation()); + guard.put("operation", params.getOperation()); guard.put("target", params.getTargetEntity()); guard.put("requestId", params.getRequestId()); diff --git a/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java b/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java index b73a3a068..7b8eed59e 100644 --- a/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java +++ b/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java @@ -324,13 +324,7 @@ public class OperationPartialTest { Map payload = params.getPayload(); assertNotNull(payload); - @SuppressWarnings("unchecked") - Map resource = (Map) payload.get("resource"); - assertNotNull(resource); - - @SuppressWarnings("unchecked") - Map guard = (Map) resource.get("guard"); - assertEquals(oper.makeGuardPayload(), guard); + assertEquals(oper.makeGuardPayload(), payload); } @Test @@ -341,13 +335,13 @@ public class OperationPartialTest { // request id changes, so remove it payload.remove("requestId"); - assertEquals("{actor=my-actor, recipe=my-operation, target=my-entity}", payload.toString()); + assertEquals("{actor=my-actor, operation=my-operation, target=my-entity}", payload.toString()); // repeat, but with closed loop name event.setClosedLoopControlName("my-loop"); payload = oper.makeGuardPayload(); payload.remove("requestId"); - assertEquals("{actor=my-actor, recipe=my-operation, target=my-entity, clname=my-loop}", payload.toString()); + assertEquals("{actor=my-actor, operation=my-operation, target=my-entity, clname=my-loop}", payload.toString()); } @Test -- cgit 1.2.3-korg