Fixes #986 Move ownership checks to base EntityResourceTest for all the entities

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

@@ -1,7 +1,6 @@
 package org.openmetadata.catalog.jdbi3;
 
 import org.jdbi.v3.sqlobject.transaction.Transaction;
-import org.openmetadata.catalog.Entity;
 import org.openmetadata.catalog.jdbi3.CollectionDAO.EntityVersionPair;
 import org.openmetadata.catalog.type.ChangeDescription;
 import org.openmetadata.catalog.type.EntityHistory;
@@ -251,12 +250,12 @@ public abstract class EntityRepository<T> {
       if (recordChange("owner", origOwner == null ? null : origOwner.getId(),
               updatedOwner == null ? null : updatedOwner.getId())) {
         EntityUtil.updateOwner(daoCollection.relationshipDAO(), origOwner,
-                updatedOwner, original.getId(), Entity.TABLE);
+                updatedOwner, original.getId(), entityName);
       }
     }
 
     private void updateTags() throws IOException {
-      // Remove current table tags in the database. It will be added back later from the merged tag list.
+      // Remove current entity tags in the database. It will be added back later from the merged tag list.
       List<TagLabel> origTags = original.getTags();
       List<TagLabel> updatedTags = updated.getTags();
       EntityUtil.removeTagsByPrefix(daoCollection.tagDAO(), original.getFullyQualifiedName());

catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java

@@ -193,7 +193,7 @@ public final class EntityUtil {
                               EntityReference owner) {
     // Add relationship owner --- owns ---> ownedEntity
     if (owner != null) {
-      LOG.info("Adding owner {}:{} for entity {}", owner.getType(), owner.getId(), ownedEntityId);
+      LOG.info("Adding owner {}:{} for entity {}:{}", owner.getType(), owner.getId(), ownedEntityType, ownedEntityId);
       dao.insert(owner.getId().toString(), ownedEntityId.toString(), owner.getType(), ownedEntityType,
               Relationship.OWNS.ordinal());
     }
@@ -465,12 +465,12 @@ public final class EntityUtil {
   public static String getLocalColumnName(String fqn) {
     // Return for fqn=service.database.table.c1 -> c1
     // Return for fqn=service.database.table.c1.c2 -> c1.c2 (note different from just the local name of the column c2)
-    String localColumnName = "";
+    StringBuilder localColumnName = new StringBuilder();
     String[] s = fqn.split("\\.");
     for (int i = 3; i < s.length -1 ; i++) {
-      localColumnName += s[i] + ".";
+      localColumnName.append(s[i]).append(".");
     }
-    localColumnName += s[s.length - 1];
+    localColumnName.append(s[s.length - 1]);
-    return localColumnName;
+    return localColumnName.toString();
   }
 }

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

@@ -211,27 +211,32 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
       // Ignore the test - User and team entities can't be owned like other data assets
       return;
     }
+    // Create an entity without owner
     Object request = createRequest(test, "description", "displayName", null);
     T entity = createAndCheckEntity(request, adminAuthHeaders());
     EntityInterface<T> entityInterface = getEntityInterface(entity);
 
-    // Add TEAM_OWNER1 as owner
+    // Set TEAM_OWNER1 as owner using PUT request
     request = createRequest(test, "description", "displayName", TEAM_OWNER1);
     ChangeDescription change = getChangeDescription(entityInterface.getVersion())
             .withFieldsAdded(Collections.singletonList("owner"));
     entity = updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change);
     entityInterface = getEntityInterface(entity);
+    checkOwnerOwns(TEAM_OWNER1, entityInterface.getId(), true);
 
-    // Change owner from TEAM_OWNER1 to USER_OWNER1
+    // Change owner from TEAM_OWNER1 to USER_OWNER1 using PUT request
     request = createRequest(test, "description", "displayName", USER_OWNER1);
     change = getChangeDescription(entityInterface.getVersion()).withFieldsUpdated(Collections.singletonList("owner"));
     entity = updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change);
     entityInterface = getEntityInterface(entity);
+    checkOwnerOwns(USER_OWNER1, entityInterface.getId(), true);
+    checkOwnerOwns(TEAM_OWNER1, entityInterface.getId(), false);
 
-    // Remove ownership
+    // Remove ownership (from USER_OWNER1) using PUT request
     request = createRequest(test, "description", "displayName", null);
     change = getChangeDescription(entityInterface.getVersion()).withFieldsDeleted(Collections.singletonList("owner"));
     updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change);
+    checkOwnerOwns(USER_OWNER1, entityInterface.getId(), false);
   }
 
   @Test
@@ -281,38 +286,38 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
   ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
   // Common entity functionality for tests
   ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-  public final WebTarget getCollection() {
+  protected final WebTarget getCollection() {
     return getResource(collectionName);
   }
 
-  public final WebTarget getResource(UUID id) {
+  protected final WebTarget getResource(UUID id) {
     return getResource(collectionName + "/" + id);
   }
 
-  public final T getEntity(UUID id, Map<String, String> authHeaders) throws HttpResponseException {
+  protected final T getEntity(UUID id, Map<String, String> authHeaders) throws HttpResponseException {
     WebTarget target = getResource(id);
     target = target.queryParam("fields", allFields);
     return TestUtils.get(target, entityClass, authHeaders);
   }
 
-  public final T createEntity(Object createRequest, Map<String, String> authHeaders)
+  protected final T createEntity(Object createRequest, Map<String, String> authHeaders)
           throws HttpResponseException {
     return TestUtils.post(getCollection(), createRequest, entityClass, authHeaders);
   }
 
-  public final T updateEntity(Object updateRequest, Status status, Map<String, String> authHeaders)
+  protected final T updateEntity(Object updateRequest, Status status, Map<String, String> authHeaders)
           throws HttpResponseException {
     return TestUtils.put(getCollection(), updateRequest, entityClass, status, authHeaders);
   }
 
-  public final T patchEntity(UUID id, String originalJson, T updated, Map<String, String> authHeaders)
+  protected final T patchEntity(UUID id, String originalJson, T updated, Map<String, String> authHeaders)
           throws JsonProcessingException, HttpResponseException {
     String updatedTableJson = JsonUtils.pojoToJson(updated);
     JsonPatch patch = JsonSchemaUtil.getJsonPatch(originalJson, updatedTableJson);
     return TestUtils.patch(getResource(id), patch, entityClass, authHeaders);
   }
 
-  public final T createAndCheckEntity(Object create, Map<String, String> authHeaders) throws HttpResponseException {
+  protected final T createAndCheckEntity(Object create, Map<String, String> authHeaders) throws HttpResponseException {
     // Validate an entity that is created has all the information set in create request
     String updatedBy = TestUtils.getPrincipal(authHeaders);
     T entity = createEntity(create, authHeaders);
@@ -330,7 +335,7 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
     return entity;
   }
 
-  public T updateAndCheckEntity(Object request, Status status, Map<String, String> authHeaders,
+  protected final T updateAndCheckEntity(Object request, Status status, Map<String, String> authHeaders,
                                 UpdateType updateType, ChangeDescription changeDescription)
           throws IOException {
     T updated = updateEntity(request, status, authHeaders);
@@ -369,7 +374,7 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
     return updated;
   }
 
-  public final T patchEntityAndCheck(T updated, String originalJson, Map<String, String> authHeaders,
+  protected final T patchEntityAndCheck(T updated, String originalJson, Map<String, String> authHeaders,
                                      UpdateType updateType, ChangeDescription expectedChange)
           throws JsonProcessingException, HttpResponseException {
     EntityInterface<T> entityInterface = getEntityInterface(updated);
@@ -386,7 +391,7 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
     return returned;
   }
 
-  public final void validateCommonEntityFields(EntityInterface<T> entity, String expectedDescription,
+  protected final void validateCommonEntityFields(EntityInterface<T> entity, String expectedDescription,
                                                String expectedUpdatedByUser, EntityReference expectedOwner) {
     assertNotNull(entity.getId());
     assertNotNull(entity.getHref());
@@ -396,7 +401,7 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
     assertOwner(expectedOwner, entity.getOwner());
   }
 
-  public final void validateChangeDescription(T updated, UpdateType updateType,
+  protected final void validateChangeDescription(T updated, UpdateType updateType,
                                               ChangeDescription expectedChange) {
     EntityInterface<T> updatedEntityInterface = getEntityInterface(updated);
     if (updateType == UpdateType.CREATED) {
@@ -414,29 +419,29 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
     }
   }
 
-  public EntityHistory getVersionList(UUID id, Map<String, String> authHeaders) throws HttpResponseException {
+  protected EntityHistory getVersionList(UUID id, Map<String, String> authHeaders) throws HttpResponseException {
     WebTarget target = getResource(collectionName + "/" + id + "/versions");
     return TestUtils.get(target, EntityHistory.class, authHeaders);
   }
 
-  public T getVersion(UUID id, Double version, Map<String, String> authHeaders) throws HttpResponseException {
+  protected T getVersion(UUID id, Double version, Map<String, String> authHeaders) throws HttpResponseException {
     WebTarget target = getResource(collectionName + "/" + id + "/versions/" + version.toString());
     return TestUtils.get(target, entityClass, authHeaders);
   }
 
-  public void assertFieldLists(List<String> expectedList, List<String> actualList) {
+  protected static void assertFieldLists(List<String> expectedList, List<String> actualList) {
     expectedList.sort(Comparator.comparing(String::toString));
     actualList.sort(Comparator.comparing(String::toString));
     assertIterableEquals(expectedList, actualList);
   }
 
-  public ChangeDescription getChangeDescription(Double previousVersion) {
+  protected ChangeDescription getChangeDescription(Double previousVersion) {
     return new ChangeDescription().withPreviousVersion(previousVersion)
             .withFieldsAdded(new ArrayList<>()).withFieldsUpdated(new ArrayList<>())
             .withFieldsDeleted(new ArrayList<>());
   }
 
-  public static void assertOwner(EntityReference expected, EntityReference actual) {
+  protected static void assertOwner(EntityReference expected, EntityReference actual) {
     if (expected != null) {
       TestUtils.validateEntityReference(actual);
       assertEquals(expected.getId(), actual.getId());
@@ -445,9 +450,36 @@ public abstract class EntityResourceTest<T> extends CatalogApplicationTest {
       assertNull(actual);
     }
   }
-  public static void assertService(EntityReference expected, EntityReference actual) {
+  protected static void assertService(EntityReference expected, EntityReference actual) {
     TestUtils.validateEntityReference(actual);
     assertEquals(expected.getId(), actual.getId());
     assertEquals(expected.getType(), actual.getType());
   }
+
+  protected static void checkOwnerOwns(EntityReference owner, UUID entityId, boolean expectedOwning)
+          throws HttpResponseException {
+    if (owner != null) {
+      UUID ownerId = owner.getId();
+      List<EntityReference> ownsList;
+      if (owner.getType().equals(Entity.USER)) {
+        User user = UserResourceTest.getUser(ownerId, "owns", adminAuthHeaders());
+        ownsList = user.getOwns();
+      } else if (owner.getType().equals(Entity.TEAM)) {
+        Team team = TeamResourceTest.getTeam(ownerId, "owns", adminAuthHeaders());
+        ownsList = team.getOwns();
+      } else {
+        throw new IllegalArgumentException("Invalid owner type " + owner.getType());
+      }
+
+      boolean owning = false;
+      for (EntityReference owns : ownsList) {
+        TestUtils.validateEntityReference(owns);
+        if (owns.getId().equals(entityId)) {
+          owning = true;
+          break;
+        }
+      }
+      assertEquals(expectedOwning, owning, "Ownership not correct in the owns list for " + owner.getType());
+    }
+  }
 }

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

@@ -31,7 +31,6 @@ import org.openmetadata.catalog.api.data.CreateTable;
 import org.openmetadata.catalog.entity.data.Database;
 import org.openmetadata.catalog.entity.data.Table;
 import org.openmetadata.catalog.entity.services.DatabaseService;
-import org.openmetadata.catalog.entity.teams.Team;
 import org.openmetadata.catalog.entity.teams.User;
 import org.openmetadata.catalog.exception.CatalogExceptionMessage;
 import org.openmetadata.catalog.jdbi3.TableRepository.TableEntityInterface;
@@ -39,7 +38,6 @@ import org.openmetadata.catalog.resources.EntityResourceTest;
 import org.openmetadata.catalog.resources.databases.TableResource.TableList;
 import org.openmetadata.catalog.resources.services.DatabaseServiceResourceTest;
 import org.openmetadata.catalog.resources.tags.TagResourceTest;
-import org.openmetadata.catalog.resources.teams.TeamResourceTest;
 import org.openmetadata.catalog.resources.teams.UserResourceTest;
 import org.openmetadata.catalog.type.ChangeDescription;
 import org.openmetadata.catalog.type.Column;
@@ -385,30 +383,6 @@ public class TableResourceTest extends EntityResourceTest<Table> {
       assertResponse(exception, FORBIDDEN, "Principal: CatalogPrincipal{name='test'} is not admin");
   }
 
-
-  @Test
-  public void put_tableOwnershipUpdate_200(TestInfo test) throws IOException {
-    CreateTable request = create(test).withOwner(USER_OWNER1).withDescription("description");
-    Table table = createAndCheckEntity(request, adminAuthHeaders());
-    checkOwnerOwns(USER_OWNER1, table.getId(), true);
-
-    // Change ownership from USER_OWNER1 to TEAM_OWNER1
-    ChangeDescription change = getChangeDescription(table.getVersion());
-    change.getFieldsUpdated().add("owner");
-    Table updatedTable = updateAndCheckEntity(request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders(),
-            MINOR_UPDATE, change);
-    checkOwnerOwns(USER_OWNER1, updatedTable.getId(), false);
-    checkOwnerOwns(TEAM_OWNER1, updatedTable.getId(), true);
-
-    // Remove ownership
-    change = getChangeDescription(updatedTable.getVersion());
-    change.getFieldsDeleted().add("owner");
-    updatedTable = updateAndCheckEntity(request.withOwner(null), OK, adminAuthHeaders(), MINOR_UPDATE,
-            change);
-    assertNull(updatedTable.getOwner());
-    checkOwnerOwns(TEAM_OWNER1, updatedTable.getId(), false);
-  }
-
   @Test
   public void put_tableTableConstraintUpdate_200(TestInfo test) throws IOException {
     // Create table without table constraints
@@ -1355,32 +1329,6 @@ public class TableResourceTest extends EntityResourceTest<Table> {
     return getTable;
   }
 
-  private static void checkOwnerOwns(EntityReference owner, UUID tableId, boolean expectedOwning)
-          throws HttpResponseException {
-    if (owner != null) {
-      UUID ownerId = owner.getId();
-      List<EntityReference> ownsList;
-      if (owner.getType().equals(Entity.USER)) {
-        User user = UserResourceTest.getUser(ownerId, "owns", adminAuthHeaders());
-        ownsList = user.getOwns();
-      } else if (owner.getType().equals(Entity.TEAM)) {
-        Team team = TeamResourceTest.getTeam(ownerId, "owns", adminAuthHeaders());
-        ownsList = team.getOwns();
-      } else {
-        throw new IllegalArgumentException("Invalid owner type " + owner.getType());
-      }
-
-      boolean owning = false;
-      for (EntityReference owns : ownsList) {
-        TestUtils.validateEntityReference(owns);
-        if (owns.getId().equals(tableId)) {
-          owning = true;
-          break;
-        }
-      }
-      assertEquals(expectedOwning, owning, "Ownership not correct in the owns list for " + owner.getType());
-    }
-  }
 
   private static int getTagUsageCount(String tagFQN, Map<String, String> authHeaders) throws HttpResponseException {
     return TagResourceTest.getTag(tagFQN, "usageCount", authHeaders).getUsageCount();