From 7a04afada993b46b5ef3c6a2bf503ea5c44fa0cf Mon Sep 17 00:00:00 2001 From: Trevor Date: Wed, 17 Apr 2024 11:14:11 -0700 Subject: [PATCH] Cleanup cloud query handler to prevent envelope mutation (#861) --- .github/workflows/testing.yml | 1 - .../udmi/service/core/CloudQueryHandler.java | 49 +++++++++++-------- .../udmi/service/core/ControlProcessor.java | 4 +- .../service/core/CloudQueryHandlerTest.java | 12 +++-- 4 files changed, 36 insertions(+), 30 deletions(-) diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index da272e14d4..3496360a5c 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -242,7 +242,6 @@ jobs: runlocal: name: UDMIS Local Setup runs-on: ubuntu-latest - needs: pretest timeout-minutes: 10 steps: - uses: actions/checkout@v4 diff --git a/udmis/src/main/java/com/google/bos/udmi/service/core/CloudQueryHandler.java b/udmis/src/main/java/com/google/bos/udmi/service/core/CloudQueryHandler.java index ee961faa49..ebaa7cd1af 100644 --- a/udmis/src/main/java/com/google/bos/udmi/service/core/CloudQueryHandler.java +++ b/udmis/src/main/java/com/google/bos/udmi/service/core/CloudQueryHandler.java @@ -1,9 +1,12 @@ package com.google.bos.udmi.service.core; +import static com.google.common.base.Preconditions.checkState; +import static com.google.udmi.util.GeneralUtils.deepCopy; import static com.google.udmi.util.GeneralUtils.ifTrueThen; import static com.google.udmi.util.GeneralUtils.isTrue; import static com.google.udmi.util.GeneralUtils.requireNull; import static com.google.udmi.util.GeneralUtils.toDate; +import static com.google.udmi.util.JsonUtil.stringifyTerse; import static java.util.Objects.requireNonNull; import com.google.bos.udmi.service.access.IotAccessBase; @@ -28,16 +31,26 @@ public class CloudQueryHandler { private final ControlProcessor controller; private final IotAccessBase iotAccess; private final TargetProcessor target; - private CloudQuery query; - private Envelope envelope; + private final CloudQuery query; + private final Envelope envelope; + private final String savedEnvelope; + private final String savedQuery; /** * Create a query handler for cloud queries. */ - public CloudQueryHandler(ControlProcessor controlProcessor) { + public CloudQueryHandler(ControlProcessor controlProcessor, CloudQuery cloudQuery) { controller = controlProcessor; iotAccess = controller.iotAccess; target = controller.targetProcessor; + query = cloudQuery; + envelope = controller.getContinuation(cloudQuery).getEnvelope(); + savedQuery = stringifyTerse(query); + savedEnvelope = stringifyTerse(envelope); + } + + public static void processQuery(ControlProcessor controlProcessor, CloudQuery query) { + new CloudQueryHandler(controlProcessor, query).process(); } @NotNull @@ -58,10 +71,9 @@ private void issueModifiedDevice(String deviceId) { } private void issueModifiedQuery(Consumer mutator) { - CloudQuery cloudQuery = new CloudQuery(); - cloudQuery.generation = query.generation; - mutator.accept(envelope); - controller.sideProcess(envelope, cloudQuery); + Envelope mutated = deepCopy(envelope); + mutator.accept(mutated); + controller.sideProcess(mutated, query); } private void issueModifiedRegistry(String registryId) { @@ -151,20 +163,15 @@ private boolean shouldTraverseRegistries() { /** * Process an individual cloud query. */ - public synchronized void process(CloudQuery newQuery) { - try { - query = newQuery; - envelope = controller.getContinuation(newQuery).getEnvelope(); - if (envelope.deviceRegistryId == null) { - queryAllRegistries(); - } else if (envelope.deviceId == null) { - queryRegistryDevices(); - } else { - queryDeviceDetails(); - } - } finally { - query = null; - envelope = null; + public synchronized void process() { + if (envelope.deviceRegistryId == null) { + queryAllRegistries(); + } else if (envelope.deviceId == null) { + queryRegistryDevices(); + } else { + queryDeviceDetails(); } + checkState(savedEnvelope.equals(stringifyTerse(envelope)), "mutated envelope"); + checkState(savedQuery.equals(stringifyTerse(query)), "mutated query"); } } diff --git a/udmis/src/main/java/com/google/bos/udmi/service/core/ControlProcessor.java b/udmis/src/main/java/com/google/bos/udmi/service/core/ControlProcessor.java index 042da2922b..5935d141e7 100644 --- a/udmis/src/main/java/com/google/bos/udmi/service/core/ControlProcessor.java +++ b/udmis/src/main/java/com/google/bos/udmi/service/core/ControlProcessor.java @@ -11,7 +11,6 @@ public class ControlProcessor extends ProcessorBase { TargetProcessor targetProcessor; - private CloudQueryHandler cloudQueryHandler; public ControlProcessor(EndpointConfiguration config) { super(config); @@ -21,7 +20,6 @@ public ControlProcessor(EndpointConfiguration config) { public void activate() { super.activate(); targetProcessor = UdmiServicePod.getComponent(TargetProcessor.class); - cloudQueryHandler = new CloudQueryHandler(this); } /** @@ -29,6 +27,6 @@ public void activate() { */ @MessageHandler public void cloudQueryHandler(CloudQuery query) { - cloudQueryHandler.process(query); + CloudQueryHandler.processQuery(this, query); } } diff --git a/udmis/src/test/java/com/google/bos/udmi/service/core/CloudQueryHandlerTest.java b/udmis/src/test/java/com/google/bos/udmi/service/core/CloudQueryHandlerTest.java index c4db377f1a..eb20647a37 100644 --- a/udmis/src/test/java/com/google/bos/udmi/service/core/CloudQueryHandlerTest.java +++ b/udmis/src/test/java/com/google/bos/udmi/service/core/CloudQueryHandlerTest.java @@ -30,12 +30,12 @@ class CloudQueryHandlerTest implements MessageContinuation { private static final Date QUERY_GENERATION = new Date(); private final ControlProcessor controlProcessor = mock(ControlProcessor.class); private final Envelope envelope = new Envelope(); - private CloudQueryHandler queryHandler; private final ArgumentCaptor targetCapture = ArgumentCaptor.forClass(Object.class); private final ArgumentCaptor controlCapture = ArgumentCaptor.forClass(Object.class); private final ArgumentCaptor envelopeCapture = ArgumentCaptor.forClass(Envelope.class); private final Set mockRegistries = ImmutableSet.of(TEST_REGISTRY); private final CloudQuery query = new CloudQuery(); + private CloudQueryHandler queryHandler; @Override public Envelope getEnvelope() { @@ -49,7 +49,7 @@ public void publish(Object message) { @Test public void queryAllRegistries() { - queryHandler.process(query); + queryHandler.process(); List targetMessages = targetCapture.getAllValues(); assertEquals(1, targetMessages.size(), "published messages"); @@ -72,6 +72,9 @@ public void queryAllRegistries() { @BeforeEach public void setupMock() { + query.generation = QUERY_GENERATION; + query.depth = Depth.ENTRIES; + doReturn(this).when(controlProcessor).getContinuation(eq(query)); doNothing().when(controlProcessor).publish(targetCapture.capture()); doNothing().when(controlProcessor) @@ -84,8 +87,7 @@ public void setupMock() { TargetProcessor targetProcessor = mock(TargetProcessor.class); controlProcessor.targetProcessor = targetProcessor; doReturn(LAST_SEEN).when(targetProcessor).getLastSeen(eq(TEST_REGISTRY)); - queryHandler = new CloudQueryHandler(controlProcessor); - query.generation = QUERY_GENERATION; - query.depth = Depth.ENTRIES; + + queryHandler = new CloudQueryHandler(controlProcessor, query); } }