diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java index ef86deb498c..4aca7669511 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java @@ -65,6 +65,7 @@ public class TeamRepository extends EntityRepository { public void validateUsers(List users) throws IOException { if (users != null) { + users.sort(EntityUtil.compareEntityReference); for (EntityReference user : users) { EntityReference ref = daoCollection.userDAO().findEntityReferenceById(user.getId()); user.withType(ref.getType()).withName(ref.getName()).withDisplayName(ref.getDisplayName()); 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 384f3cedb15..3c4b686c553 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 @@ -112,6 +112,9 @@ public final class JsonUtils { JsonStructure targetJson = JsonUtils.getJsonStructure(original); // + // ----------------------------------------------------------- + // JSON patch modification 1 - Reorder the operations + // ----------------------------------------------------------- // JsonPatch array operations are not handled correctly by johnzon libraries. Example, the following operation: // {"op":"replace","path":"/tags/0/tagFQN","value":"User.BankAccount"} // {"op":"replace","path":"/tags/0/labelType","value":"MANUAL"} @@ -131,6 +134,13 @@ public final class JsonUtils { // Reverse sorting the remove operations and sorting all the other operations including "add" by "path" fields // before applying the patch as a workaround. // + // --------------------------------------------------------------------- + // JSON patch modification 2 - Ignore operations related to href patch + // --------------------------------------------------------------------- + // Another important modification to patch operation: + // Ignore all the patch operations related to the href path as href path is read only and is auto generated + // by removing those operations from patch operation array + // JsonArray array = patch.toJsonArray(); List removeOperations = new ArrayList<>(); List otherOperations = new ArrayList<>(); @@ -138,6 +148,10 @@ public final class JsonUtils { array.forEach( entry -> { JsonObject jsonObject = entry.asJsonObject(); + if (jsonObject.getString("path").endsWith("href")) { + // Ignore patch operations related to href path + return; + } if (jsonObject.getString("op").equals("remove")) { removeOperations.add(jsonObject); } else { diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java index ebef46518df..2d1c404ead7 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java @@ -1832,6 +1832,15 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { } } + protected void assertEntityReferencesFieldChange( + List expectedList, List actualList) { + for (EntityReference expected : expectedList) { + EntityReference actual = + actualList.stream().filter(a -> EntityUtil.entityReferenceMatch.test(a, expected)).findAny().orElse(null); + assertNotNull(actual, "Expected entity " + expected.getId() + " not found"); + } + } + public final String getEntityName(TestInfo test) { return String.format("%s_%s", entityType, test.getDisplayName()); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java index 692ca292b75..6daa66d3eff 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java @@ -278,7 +278,7 @@ public class DashboardResourceTest extends EntityResourceTest expectedRefs = (List) expected; List actualRefs = JsonUtils.readObjects(actual.toString(), EntityReference.class); - assertEquals(expectedRefs, actualRefs); + assertEntityReferencesFieldChange(expectedRefs, actualRefs); } else { assertCommonFieldChange(fieldName, expected, actual); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java index 28a593b81a4..0db0e497aa6 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java @@ -35,6 +35,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Random; import java.util.UUID; import lombok.extern.slf4j.Slf4j; import org.apache.http.client.HttpResponseException; @@ -49,13 +50,16 @@ import org.openmetadata.catalog.jdbi3.TeamRepository.TeamEntityInterface; import org.openmetadata.catalog.resources.EntityResourceTest; import org.openmetadata.catalog.resources.locations.LocationResourceTest; import org.openmetadata.catalog.resources.teams.TeamResource.TeamList; +import org.openmetadata.catalog.type.ChangeDescription; import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.type.FieldChange; import org.openmetadata.catalog.type.ImageList; import org.openmetadata.catalog.type.Profile; import org.openmetadata.catalog.util.EntityInterface; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.JsonUtils; import org.openmetadata.catalog.util.TestUtils; +import org.openmetadata.catalog.util.TestUtils.UpdateType; @Slf4j public class TeamResourceTest extends EntityResourceTest { @@ -154,6 +158,29 @@ public class TeamResourceTest extends EntityResourceTest { assertResponse(exception, FORBIDDEN, "Principal: CatalogPrincipal{name='test'} is not admin"); } + @Test + void patch_deleteUserFromTeam_200(TestInfo test) throws IOException { + UserResourceTest userResourceTest = new UserResourceTest(); + final int totalUsers = 20; + ArrayList users = new ArrayList<>(); + for (int i = 0; i < totalUsers; i++) { + User user = userResourceTest.createEntity(userResourceTest.createRequest(test, i), ADMIN_AUTH_HEADERS); + users.add(user.getId()); + } + CreateTeam create = + createRequest(getEntityName(test), "description", "displayName", null).withProfile(PROFILE).withUsers(users); + Team team = createAndCheckEntity(create, ADMIN_AUTH_HEADERS); + + // Remove a user from the team list using patch request + String json = JsonUtils.pojoToJson(team); + int removeUserIndex = new Random().nextInt(totalUsers); + EntityReference deletedUser = team.getUsers().get(removeUserIndex).withHref(null); + team.getUsers().remove(removeUserIndex); + ChangeDescription change = getChangeDescription(team.getVersion()); + change.getFieldsDeleted().add(new FieldChange().withName("users").withOldValue(Arrays.asList(deletedUser))); + patchEntityAndCheck(team, json, ADMIN_AUTH_HEADERS, UpdateType.MINOR_UPDATE, change); + } + private static void validateTeam( Team team, String expectedDescription, @@ -237,6 +264,8 @@ public class TeamResourceTest extends EntityResourceTest { expectedUsers.add(new EntityReference().withId(teamId).withType(Entity.USER)); } List actualUsers = Optional.ofNullable(team.getUsers()).orElse(Collections.emptyList()); + actualUsers.forEach(actual -> TestUtils.validateEntityReference(actual)); + if (!expectedUsers.isEmpty()) { assertEquals(expectedUsers.size(), actualUsers.size()); for (EntityReference user : expectedUsers) { @@ -280,8 +309,6 @@ public class TeamResourceTest extends EntityResourceTest { List expectedUsers = Optional.ofNullable(expected.getUsers()).orElse(Collections.emptyList()); List actualUsers = Optional.ofNullable(updated.getUsers()).orElse(Collections.emptyList()); actualUsers.forEach(TestUtils::validateEntityReference); - actualUsers.forEach(user -> user.setHref(null)); - expectedUsers.forEach(user -> user.setHref(null)); actualUsers.sort(EntityUtil.compareEntityReference); expectedUsers.sort(EntityUtil.compareEntityReference); @@ -296,6 +323,16 @@ public class TeamResourceTest extends EntityResourceTest { @Override public void assertFieldChange(String fieldName, Object expected, Object actual) throws IOException { - assertCommonFieldChange(fieldName, expected, actual); + if (expected == actual) { + return; + } + if (fieldName.equals("users")) { + @SuppressWarnings("unchecked") + List expectedUsers = (List) expected; + List actualUsers = JsonUtils.readObjects(actual.toString(), EntityReference.class); + assertEntityReferencesFieldChange(expectedUsers, actualUsers); + } else { + assertCommonFieldChange(fieldName, expected, actual); + } } } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java index 94390dee02e..4471a5f0db0 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java @@ -342,18 +342,15 @@ public class UserResourceTest extends EntityResourceTest { EntityReference team1 = new TeamEntityInterface( teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS)) - .getEntityReference() - .withHref(null); + .getEntityReference(); EntityReference team2 = new TeamEntityInterface( teamResourceTest.createEntity(teamResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS)) - .getEntityReference() - .withHref(null); + .getEntityReference(); EntityReference team3 = new TeamEntityInterface( teamResourceTest.createEntity(teamResourceTest.createRequest(test, 3), ADMIN_AUTH_HEADERS)) - .getEntityReference() - .withHref(null); + .getEntityReference(); List teams = Arrays.asList(team1, team2); Profile profile = new Profile().withImages(new ImageList().withImage(URI.create("http://image.com"))); @@ -361,8 +358,7 @@ public class UserResourceTest extends EntityResourceTest { EntityReference role1 = new RoleEntityInterface( roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS)) - .getEntityReference() - .withHref(null); + .getEntityReference(); List roles1 = Arrays.asList(role1); // @@ -398,8 +394,7 @@ public class UserResourceTest extends EntityResourceTest { EntityReference role2 = new RoleEntityInterface( roleResourceTest.createEntity(roleResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS)) - .getEntityReference() - .withHref(null); + .getEntityReference(); List roles2 = Arrays.asList(role2); origJson = JsonUtils.pojoToJson(user); @@ -626,8 +621,6 @@ public class UserResourceTest extends EntityResourceTest { updatedList.forEach(TestUtils::validateEntityReference); expectedList.sort(EntityUtil.compareEntityReference); updatedList.sort(EntityUtil.compareEntityReference); - updatedList.forEach(t -> t.setHref(null)); - expectedList.forEach(t -> t.setHref(null)); assertEquals(expectedList, updatedList); } @@ -649,7 +642,7 @@ public class UserResourceTest extends EntityResourceTest { @SuppressWarnings("unchecked") List expectedList = (List) expected; List actualList = JsonUtils.readObjects(actual.toString(), EntityReference.class); - assertEquals(expectedList, actualList); + assertEntityReferencesFieldChange(expectedList, actualList); } else { assertCommonFieldChange(fieldName, expected, actual); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/topics/TopicResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/topics/TopicResourceTest.java index a1a8a78acd5..0c8835e1d85 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/topics/TopicResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/topics/TopicResourceTest.java @@ -181,8 +181,6 @@ public class TopicResourceTest extends EntityResourceTest { // Patch and update the topic Topic topic = createEntity(createTopic, ADMIN_AUTH_HEADERS); - topic.setHref(null); - topic.getOwner().withHref(null); String origJson = JsonUtils.pojoToJson(topic); topic