fix: clear state only on null (#193)

aws-greengrass · Sep 26, 2023 · 1442e7e · 1442e7e
1 parent 7c2a92a
commit 1442e7e
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 33 deletions.
diff --git a/src/integrationtests/java/com/aws/greengrass/integrationtests/SyncTest.java b/src/integrationtests/java/com/aws/greengrass/integrationtests/SyncTest.java
@@ -1429,8 +1429,7 @@ void GIVEN_local_shadow_state_empty_WHEN_shadow_manager_syncs_THEN_cloud_shadow_
         String initialCloudState = "{\"version\":1,\"state\":{\"desired\":{\"SomeKey\":\"foo\"}}}";
         String initialLocalState = "{\"state\":{\"desired\":{\"SomeKey\":\"foo\"}}}";
         String localUpdate1 = "{\"state\":{}}}";
-        String localUpdate2 = "{\"state\":{\"desired\":{\"SomeKey\":\"foo\"}}}";
-        String localUpdate3 = "{\"state\":null}}";
+        String localUpdate2 = "{\"state\":null}";
         String finalLocalState = "{\"state\":{}}";
 
         UpdateThingShadowRequest updateRequest1 = new UpdateThingShadowRequest();
@@ -1443,15 +1442,9 @@ void GIVEN_local_shadow_state_empty_WHEN_shadow_manager_syncs_THEN_cloud_shadow_
         updateRequest2.setShadowName(CLASSIC_SHADOW);
         updateRequest2.setPayload(localUpdate2.getBytes(UTF_8));
 
-        UpdateThingShadowRequest updateRequest3 = new UpdateThingShadowRequest();
-        updateRequest3.setThingName(MOCK_THING_NAME_1);
-        updateRequest3.setShadowName(CLASSIC_SHADOW);
-        updateRequest3.setPayload(localUpdate3.getBytes(UTF_8));
-
         when(mockUpdateThingShadowResponse.payload())
                 .thenReturn(SdkBytes.fromString("{\"version\": 2}", UTF_8))
-                .thenReturn(SdkBytes.fromString("{\"version\": 3}", UTF_8))
-                .thenReturn(SdkBytes.fromString("{\"version\": 4}", UTF_8));
+                .thenReturn(SdkBytes.fromString("{\"version\": 3}", UTF_8));
         when(iotDataPlaneClientFactory.getIotDataPlaneClient().updateThingShadow(cloudUpdateThingShadowRequestCaptor.capture()))
                 .thenReturn(mockUpdateThingShadowResponse);
 
@@ -1479,24 +1472,15 @@ void GIVEN_local_shadow_state_empty_WHEN_shadow_manager_syncs_THEN_cloud_shadow_
         updateHandler.handleRequest(updateRequest1, "DoAll");
         assertEmptySyncQueue(clazz);
         assertThat("sync info exists", () -> syncInfo.get().isPresent(), eventuallyEval(is(true)));
-        assertThat("cloud version", () -> syncInfo.get().get().getCloudVersion(), eventuallyEval(is(2L)));
+        assertThat("cloud version", () -> syncInfo.get().get().getCloudVersion(), eventuallyEval(is(1L)));
         assertThat("local version", () -> syncInfo.get().get().getLocalVersion(), eventuallyEval(is(2L)));
-        assertLocalShadowEquals(localUpdate1);
-        assertCloudUpdateEquals(localUpdate1);
+        assertLocalShadowEquals(initialLocalState);
 
         updateHandler.handleRequest(updateRequest2, "DoAll");
         assertEmptySyncQueue(clazz);
         assertThat("sync info exists", () -> syncInfo.get().isPresent(), eventuallyEval(is(true)));
-        assertThat("cloud version", () -> syncInfo.get().get().getCloudVersion(), eventuallyEval(is(3L)));
+        assertThat("cloud version", () -> syncInfo.get().get().getCloudVersion(), eventuallyEval(is(2L)));
         assertThat("local version", () -> syncInfo.get().get().getLocalVersion(), eventuallyEval(is(3L)));
-        assertLocalShadowEquals(localUpdate2);
-        assertCloudUpdateEquals(localUpdate2);
-
-        updateHandler.handleRequest(updateRequest3, "DoAll");
-        assertEmptySyncQueue(clazz);
-        assertThat("sync info exists", () -> syncInfo.get().isPresent(), eventuallyEval(is(true)));
-        assertThat("cloud version", () -> syncInfo.get().get().getCloudVersion(), eventuallyEval(is(4L)));
-        assertThat("local version", () -> syncInfo.get().get().getLocalVersion(), eventuallyEval(is(4L)));
         assertLocalShadowEquals(finalLocalState);
         assertCloudUpdateEquals(finalLocalState);
     }

diff --git a/src/main/java/com/aws/greengrass/shadowmanager/model/ShadowState.java b/src/main/java/com/aws/greengrass/shadowmanager/model/ShadowState.java
@@ -17,7 +17,9 @@
 import static com.aws.greengrass.shadowmanager.model.Constants.SHADOW_DOCUMENT_STATE_DELTA;
 import static com.aws.greengrass.shadowmanager.model.Constants.SHADOW_DOCUMENT_STATE_DESIRED;
 import static com.aws.greengrass.shadowmanager.model.Constants.SHADOW_DOCUMENT_STATE_REPORTED;
+import static com.aws.greengrass.shadowmanager.util.JsonUtil.isEmptyStateDocument;
 import static com.aws.greengrass.shadowmanager.util.JsonUtil.isNullOrMissing;
+import static com.aws.greengrass.shadowmanager.util.JsonUtil.isNullStateDocument;
 import static com.aws.greengrass.shadowmanager.util.JsonUtil.nullIfEmpty;
 
 /**
@@ -58,11 +60,14 @@ public ShadowState deepCopy() {
      */
     @SuppressWarnings({"PMD.ForLoopCanBeForeach", "PMD.NullAssignment"})
     public void update(JsonNode updatedStateNode) {
-        if (isNullOrMissing(updatedStateNode)) {
+        if (isNullStateDocument(updatedStateNode)) {
             this.desired = null;
             this.reported = null;
             return;
         }
+        if (isEmptyStateDocument(updatedStateNode)) {
+            return;
+        }
         for (final Iterator<String> i = updatedStateNode.fieldNames(); i.hasNext(); ) {
             final String field = i.next();
             final JsonNode value = updatedStateNode.get(field);

diff --git a/src/main/java/com/aws/greengrass/shadowmanager/util/JsonMerger.java b/src/main/java/com/aws/greengrass/shadowmanager/util/JsonMerger.java
@@ -35,11 +35,15 @@ private JsonMerger() {
      */
     @SuppressFBWarnings(value = "BC_UNCONFIRMED_CAST", justification = "We do check the type before cast.")
     public static void merge(JsonNode source, final JsonNode patch) throws InvalidRequestParametersException {
-        if (JsonUtil.isEmptyStateDocument(patch)) {
+        if (JsonUtil.isNullStateDocument(patch)) {
             clearState(source);
             return;
         }
 
+        if (JsonUtil.isEmptyStateDocument(patch)) {
+            return;
+        }
+
         // If both nodes are objects then do a recursive patch
         if (source.isObject() && patch.isObject()) {
             merge((ObjectNode) source, (ObjectNode) patch);

diff --git a/src/main/java/com/aws/greengrass/shadowmanager/util/JsonUtil.java b/src/main/java/com/aws/greengrass/shadowmanager/util/JsonUtil.java
@@ -110,16 +110,27 @@ public static boolean isMissing(JsonNode node) {
     }
 
     /**
-     * Determine if a node represents an empty state document.
+     * Determine if a node represents an empty state document, {} or {"state": {}}.
      *
      * @param node node
      * @return true if node is null, empty, or has a null or empty state node
      */
     public static boolean isEmptyStateDocument(JsonNode node) {
-        return isNullOrMissing(node)
-                || node.isObject()
-                && node.get(SHADOW_DOCUMENT_STATE) != null
-                && isMissing(node.get(SHADOW_DOCUMENT_STATE));
+        return node != null
+                && (node.isObject() && node.isEmpty() && !node.isNull()
+                || isEmptyStateDocument(node.get(SHADOW_DOCUMENT_STATE)));
+    }
+
+    /**
+     * Determine if a node represents an null state document, null or {"state": null}.
+     *
+     * @param node node
+     * @return true if node is null or has a null state node
+     */
+    public static boolean isNullStateDocument(JsonNode node) {
+        return node != null
+                && (node.isNull()
+                || node.isObject() && isNullStateDocument(node.get(SHADOW_DOCUMENT_STATE)));
     }
 
     /**

diff --git a/src/test/java/com/aws/greengrass/shadowmanager/util/JsonMergerTest.java b/src/test/java/com/aws/greengrass/shadowmanager/util/JsonMergerTest.java
@@ -28,6 +28,7 @@
 @ExtendWith({MockitoExtension.class, GGExtension.class})
 class JsonMergerTest {
     private final static String SOURCE_NODE_STRING = "{\"id\": 100, \"SomeKey\": \"SomeValue\",\"SomeObjectKey\": {\"foo\": \"bar\"}}";
+    private final static String SOURCE_STATE_NODE_STRING = "{\"state\":{\"id\": 100, \"SomeKey\": \"SomeValue\",\"SomeObjectKey\": {\"foo\": \"bar\"}}}";
     private final static String SOURCE_NODE_WITH_ARRAY_STRING = "{\"id\": 100, \"SomeArrayKey\": [\"SomeValue1\", \"SomeValue2\"]}";
     private final static String SOURCE_NODE_WITH_ARRAY_NODE_STRING = "[\"SomeValue1\", \"SomeValue2\"]";
     private final static String PATCH_NODE_WITH_NEW_FIELD_STRING = "{\"NewKey\": true, \"NewNullParent\": {\"NewNullChild1\": null, \"NewNullChild2\": {\"NewNullChild3\": null}}, \"NewChildLevel1\": {\"NewChildLevel2\": {\"NewChildLevel3\":\"NewChildValue\"}}}";
@@ -37,7 +38,9 @@ class JsonMergerTest {
     private final static String MERGED_NODE_WITH_UPDATED_ARRAY_NODE_STRING = "[\"SomeValue3\", \"SomeValue4\"]";
     private final static String PATCH_NODE_WITH_ARRAY_VALUE_NODE_STRING = "[\"SomeValue3\", \"SomeValue4\"]";
     private final static String EMPTY_DOCUMENT = "{}";
+    private final static String EMPTY_STATE_DOCUMENT = "{\"state\": {}}";
     private final static String NULL_DOCUMENT = "null";
+    private final static String NULL_STATE_DOCUMENT = "{\"state\": null}";
     private static JsonNode sourceNodeWithArray;
 
     @SuppressWarnings("PMD.UnusedPrivateMethod")
@@ -46,9 +49,13 @@ private static Stream<Arguments> mergeTestInput() {
                 Arguments.of("GIVEN patch with new node, THEN add new field in source node", SOURCE_NODE_STRING, PATCH_NODE_WITH_NEW_FIELD_STRING, MERGED_NODE_WITH_NEW_FIELD_STRING),
                 Arguments.of("GIVEN patch with null nodes, THEN removes all null nodes in source node", SOURCE_NODE_STRING, PATCH_NODE_WITH_NULL_FIELD_STRING, MERGED_NODE_WITHOUT_NULL_FIELD_STRING),
                 Arguments.of("GIVEN patch with new nodes in an array node, THEN replaces all the elements in the source with the elements in the patch", SOURCE_NODE_WITH_ARRAY_NODE_STRING, PATCH_NODE_WITH_ARRAY_VALUE_NODE_STRING, MERGED_NODE_WITH_UPDATED_ARRAY_NODE_STRING),
-                Arguments.of("GIVEN patch with empty state, THEN source node is cleared", SOURCE_NODE_STRING, EMPTY_DOCUMENT, EMPTY_DOCUMENT),
-                Arguments.of("GIVEN patch with null state, THEN source node is cleared", SOURCE_NODE_STRING, NULL_DOCUMENT, EMPTY_DOCUMENT),
-                Arguments.of("GIVEN empty source and non-empty patch, THEN patch is the result", EMPTY_DOCUMENT, SOURCE_NODE_STRING, SOURCE_NODE_STRING));
+                Arguments.of("GIVEN empty patch, THEN source node is unaffected", SOURCE_NODE_STRING, EMPTY_DOCUMENT, SOURCE_NODE_STRING),
+                Arguments.of("GIVEN null patch, THEN source node is cleared", SOURCE_NODE_STRING, NULL_DOCUMENT, EMPTY_DOCUMENT),
+                Arguments.of("GIVEN empty source and non-empty patch, THEN patch is the result", EMPTY_DOCUMENT, SOURCE_NODE_STRING, SOURCE_NODE_STRING),
+                Arguments.of("GIVEN empty state patch, THEN source node is unaffected", SOURCE_STATE_NODE_STRING, EMPTY_STATE_DOCUMENT, SOURCE_STATE_NODE_STRING),
+                Arguments.of("GIVEN null state patch, THEN source node is cleared", SOURCE_STATE_NODE_STRING, NULL_STATE_DOCUMENT, EMPTY_STATE_DOCUMENT),
+                Arguments.of("GIVEN empty source and non-empty state patch, THEN patch is the result", EMPTY_STATE_DOCUMENT, SOURCE_STATE_NODE_STRING, SOURCE_STATE_NODE_STRING),
+                Arguments.of("GIVEN null source and non-empty state patch, THEN patch is the result", NULL_STATE_DOCUMENT, SOURCE_STATE_NODE_STRING, SOURCE_STATE_NODE_STRING));
     }
 
 

diff --git a/src/test/java/com/aws/greengrass/shadowmanager/util/JsonUtilTest.java b/src/test/java/com/aws/greengrass/shadowmanager/util/JsonUtilTest.java
@@ -136,10 +136,10 @@ void GIVEN_state_with_7_levels_WHEN_validatePayload_THEN_throws_invalid_request_
     @SuppressWarnings("PMD.UnusedPrivateMethod")
     private static Stream<Arguments> emptyStateDocuments() {
         return Stream.of(
-                Arguments.of("{\"state\": null}", true),
+                Arguments.of("{\"state\": null}", false),
                 Arguments.of("{\"state\": {}}", true),
                 Arguments.of("{}", true),
-                Arguments.of("null", true),
+                Arguments.of("null", false),
                 Arguments.of("{\"state\": {\"field\":1}}", false),
                 Arguments.of("{\"field\":1}", false)
         );
@@ -152,4 +152,24 @@ void GIVEN_empty_state_document_WHEN_check_is_empty_state_document_THEN_return_t
                 JsonUtil.isEmptyStateDocument(getPayloadJson(json.getBytes(StandardCharsets.UTF_8)).get()),
                 String.format("%s %sexpected to be empty", json, emptyDocumentExpected ? "" : "not "));
     }
+
+    @SuppressWarnings("PMD.UnusedPrivateMethod")
+    private static Stream<Arguments> nullStateDocuments() {
+        return Stream.of(
+                Arguments.of("{\"state\": null}", true),
+                Arguments.of("{\"state\": {}}", false),
+                Arguments.of("{}", false),
+                Arguments.of("null", true),
+                Arguments.of("{\"state\": {\"field\":1}}", false),
+                Arguments.of("{\"field\":1}", false)
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("nullStateDocuments")
+    void GIVEN_null_state_document_WHEN_check_is_null_state_document_THEN_return_true(String json, boolean nullDocumentExpected) throws IOException {
+        assertEquals(nullDocumentExpected,
+                JsonUtil.isNullStateDocument(getPayloadJson(json.getBytes(StandardCharsets.UTF_8)).get()),
+                String.format("%s %sexpected to be null", json, nullDocumentExpected ? "" : "not "));
+    }
 }