Fixes #2528 - Getting contains no value for name href error from backend for teams PATCH API (#2666)

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java

@@ -65,6 +65,7 @@ public class TeamRepository extends EntityRepository<Team> {
 
   public void validateUsers(List<EntityReference> 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());

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<JsonObject> removeOperations = new ArrayList<>();
     List<JsonObject> 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 {

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java

@@ -1832,6 +1832,15 @@ public abstract class EntityResourceTest<T, K> extends CatalogApplicationTest {
     }
   }
 
+  protected void assertEntityReferencesFieldChange(
+      List<EntityReference> expectedList, List<EntityReference> 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());
   }

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java

@@ -278,7 +278,7 @@ public class DashboardResourceTest extends EntityResourceTest<Dashboard, CreateD
       @SuppressWarnings("unchecked")
       List<EntityReference> expectedRefs = (List<EntityReference>) expected;
       List<EntityReference> actualRefs = JsonUtils.readObjects(actual.toString(), EntityReference.class);
-      assertEquals(expectedRefs, actualRefs);
+      assertEntityReferencesFieldChange(expectedRefs, actualRefs);
     } else {
       assertCommonFieldChange(fieldName, expected, actual);
     }

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<Team, CreateTeam> {
@@ -154,6 +158,29 @@ public class TeamResourceTest extends EntityResourceTest<Team, CreateTeam> {
     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<UUID> 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<Team, CreateTeam> {
       expectedUsers.add(new EntityReference().withId(teamId).withType(Entity.USER));
     }
     List<EntityReference> 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<Team, CreateTeam> {
     List<EntityReference> expectedUsers = Optional.ofNullable(expected.getUsers()).orElse(Collections.emptyList());
     List<EntityReference> 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<Team, CreateTeam> {
 
   @Override
   public void assertFieldChange(String fieldName, Object expected, Object actual) throws IOException {
+    if (expected == actual) {
+      return;
+    }
+    if (fieldName.equals("users")) {
+      @SuppressWarnings("unchecked")
+      List<EntityReference> expectedUsers = (List<EntityReference>) expected;
+      List<EntityReference> actualUsers = JsonUtils.readObjects(actual.toString(), EntityReference.class);
+      assertEntityReferencesFieldChange(expectedUsers, actualUsers);
+    } else {
       assertCommonFieldChange(fieldName, expected, actual);
     }
   }
+}

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java

@@ -342,18 +342,15 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
     EntityReference team1 =
         new TeamEntityInterface(
                 teamResourceTest.createEntity(teamResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS))
-            .getEntityReference()
+            .getEntityReference();
-            .withHref(null);
     EntityReference team2 =
         new TeamEntityInterface(
                 teamResourceTest.createEntity(teamResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS))
-            .getEntityReference()
+            .getEntityReference();
-            .withHref(null);
     EntityReference team3 =
         new TeamEntityInterface(
                 teamResourceTest.createEntity(teamResourceTest.createRequest(test, 3), ADMIN_AUTH_HEADERS))
-            .getEntityReference()
+            .getEntityReference();
-            .withHref(null);
     List<EntityReference> 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<User, CreateUser> {
     EntityReference role1 =
         new RoleEntityInterface(
                 roleResourceTest.createEntity(roleResourceTest.createRequest(test, 1), ADMIN_AUTH_HEADERS))
-            .getEntityReference()
+            .getEntityReference();
-            .withHref(null);
     List<EntityReference> roles1 = Arrays.asList(role1);
 
     //
@@ -398,8 +394,7 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
     EntityReference role2 =
         new RoleEntityInterface(
                 roleResourceTest.createEntity(roleResourceTest.createRequest(test, 2), ADMIN_AUTH_HEADERS))
-            .getEntityReference()
+            .getEntityReference();
-            .withHref(null);
     List<EntityReference> roles2 = Arrays.asList(role2);
 
     origJson = JsonUtils.pojoToJson(user);
@@ -626,8 +621,6 @@ public class UserResourceTest extends EntityResourceTest<User, CreateUser> {
     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<User, CreateUser> {
       @SuppressWarnings("unchecked")
       List<EntityReference> expectedList = (List<EntityReference>) expected;
       List<EntityReference> actualList = JsonUtils.readObjects(actual.toString(), EntityReference.class);
-      assertEquals(expectedList, actualList);
+      assertEntityReferencesFieldChange(expectedList, actualList);
     } else {
       assertCommonFieldChange(fieldName, expected, actual);
     }

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/topics/TopicResourceTest.java

@@ -181,8 +181,6 @@ public class TopicResourceTest extends EntityResourceTest<Topic, CreateTopic> {
 
     // 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