From 4909198e29b8e4147e80b0f761f062130626d36a Mon Sep 17 00:00:00 2001 From: Vivek Ratnavel Subramanian Date: Sun, 17 Oct 2021 12:18:03 -0700 Subject: [PATCH] ISSUE-670: Unable to add multiple new user in teams page (#820) --- .../openmetadata/catalog/util/JsonUtils.java | 39 +++++-- .../catalog/util/JsonUtilsTest.java | 101 ++++++++++++++++++ 2 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 catalog-rest-service/src/test/java/org/openmetadata/catalog/util/JsonUtilsTest.java diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java index e0afc9868a6..918a7860ac7 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/JsonUtils.java @@ -127,21 +127,46 @@ public final class JsonUtils { // {"op":"remove","path":"/tags/1"} // {"op":"remove","path":"/tags/2"} // Removes second array element in a 3 array field /tags/1 - // Then it fails to remove 3rd array element /tags/2. But the previous operation removed the second element and + // Then it fails to remove 3rd array element /tags/2. Because the previous operation removed the second element and // now array of length 2 and there is no third element to remove. The patch operation fails with "array index not // found error". // - // Reverse sorting the operations by "path" fields before applying the patch as a workaround. + // The same applies to add operation as well. Example, the following operation: + // {"op":"add","path":"/tags/2"} + // {"op":"add","path":"/tags/1"} + // It will try to add element in index 2 before adding element in index 1 and the patch operation fails with + // "contains no element for index 1" error. + // + // Reverse sorting the remove operations and sorting all the other operations including "add" by "path" fields + // before applying the patch as a workaround. // JsonArray array = patch.toJsonArray(); - List operations = new ArrayList<>(); - array.forEach(entry -> operations.add(entry.asJsonObject())); - operations.sort(Comparator.comparing(jsonObject -> jsonObject.getString("path"))); - Collections.reverse(operations); + List removeOperations = new ArrayList<>(); + List otherOperations = new ArrayList<>(); + + array.forEach(entry -> { + JsonObject jsonObject = entry.asJsonObject(); + if (jsonObject.getString("op").equals("remove")) { + removeOperations.add(jsonObject); + } else { + otherOperations.add(jsonObject); + } + }); + + // sort the operations by path + if (!otherOperations.isEmpty()) { + otherOperations.sort(Comparator.comparing(jsonObject -> jsonObject.getString("path"))); + } + if (!removeOperations.isEmpty()) { + removeOperations.sort(Comparator.comparing(jsonObject -> jsonObject.getString("path"))); + // reverse sort only the remove operations + Collections.reverse(removeOperations); + } // Build new sorted patch JsonArrayBuilder arrayBuilder = Json.createArrayBuilder(); - operations.forEach(arrayBuilder::add); + otherOperations.forEach(arrayBuilder::add); + removeOperations.forEach(arrayBuilder::add); JsonPatch sortedPatch = Json.createPatch(arrayBuilder.build()); // Apply sortedPatch diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/JsonUtilsTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/JsonUtilsTest.java new file mode 100644 index 00000000000..1eba766197e --- /dev/null +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/JsonUtilsTest.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.catalog.util; + +import org.openmetadata.catalog.entity.teams.Team; +import org.junit.jupiter.api.Test; + +import javax.json.Json; +import javax.json.JsonArrayBuilder; +import javax.json.JsonException; +import javax.json.JsonObject; +import javax.json.JsonObjectBuilder; +import javax.json.JsonPatchBuilder; +import java.io.IOException; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * This test provides examples of how to use applyPatch + */ +public class JsonUtilsTest { + /** + * Test apply patch method with different operations. + */ + @Test + public void applyPatch() throws IOException { + JsonObjectBuilder teamJson = Json.createObjectBuilder(); + JsonObjectBuilder user1 = Json.createObjectBuilder(); + JsonObjectBuilder user2 = Json.createObjectBuilder(); + String teamId = UUID.randomUUID().toString(); + user1.add("id", UUID.randomUUID().toString()).add("name", "alex_watts"); + user2.add("id", UUID.randomUUID().toString()).add("name", "amanda_dickson"); + JsonArrayBuilder users = Json.createArrayBuilder(); + users.add(user1); + users.add(user2); + teamJson.add("id", teamId).add("name", "finance") + .add("users", users); + + Team original = EntityUtil.validate(teamId, teamJson.build().toString(), Team.class); + JsonPatchBuilder patchBuilder = Json.createPatchBuilder(); + + // Add two users to the team + JsonObjectBuilder newUser1Builder = Json.createObjectBuilder(); + JsonObjectBuilder newUser2Builder = Json.createObjectBuilder(); + + String newUserId1 = UUID.randomUUID().toString(); + String newUserId2 = UUID.randomUUID().toString(); + newUser1Builder.add("id", newUserId1).add("type", "user").add("name", "alice_schmidt"); + newUser2Builder.add("id", newUserId2).add("type", "user").add("name", "adam_wong"); + JsonObject newUser1 = newUser1Builder.build(); + JsonObject newUser2 = newUser2Builder.build(); + patchBuilder.add("/users/3", newUser2); + patchBuilder.add("/users/2", newUser1); + Team updated = JsonUtils.applyPatch(original, patchBuilder.build(), Team.class); + + assertEquals(4, updated.getUsers().size()); + assertTrue(updated.getUsers().stream().anyMatch(entry -> + entry.getName().equals(newUser1.getString("name")) && entry.getId().toString().equals(newUserId1))); + assertTrue(updated.getUsers().stream().anyMatch(entry -> + entry.getName().equals(newUser2.getString("name")) && entry.getId().toString().equals(newUserId2))); + + // Add a user with an out of index path + final JsonPatchBuilder jsonPatchBuilder = Json.createPatchBuilder(); + jsonPatchBuilder.add("/users/4", newUser1); + JsonException jsonException = assertThrows(JsonException.class, + () -> JsonUtils.applyPatch(original, jsonPatchBuilder.build(), Team.class)); + assertTrue(jsonException.getMessage().contains("contains no element for index 4")); + + // Delete the two users from the team + patchBuilder = Json.createPatchBuilder(); + patchBuilder.remove("/users/0"); + patchBuilder.remove("/users/1"); + updated = JsonUtils.applyPatch(original, patchBuilder.build(), Team.class); + // Assert if both the users were removed + assertTrue(updated.getUsers().isEmpty()); + + // Delete a non-existent user + final JsonPatchBuilder jsonPatchBuilder2 = Json.createPatchBuilder(); + jsonPatchBuilder2.remove("/users/3"); + jsonException = assertThrows(JsonException.class, + () -> JsonUtils.applyPatch(original, jsonPatchBuilder2.build(), Team.class)); + assertTrue(jsonException.getMessage().contains("contains no element for index 3")); + } +}