diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java index d209fc6d477..e8b803a76c0 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java +++ b/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 { 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 origTags = original.getTags(); List updatedTags = updated.getTags(); EntityUtil.removeTagsByPrefix(daoCollection.tagDAO(), original.getFullyQualifiedName()); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java index 2a3cf044e1d..4b36570a493 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java +++ b/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]; - return localColumnName; + localColumnName.append(s[s.length - 1]); + return localColumnName.toString(); } } 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 bceeaee21e5..b39ce3769fe 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 @@ -211,27 +211,32 @@ public abstract class EntityResourceTest 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 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 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 authHeaders) throws HttpResponseException { + protected final T getEntity(UUID id, Map 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 authHeaders) + protected final T createEntity(Object createRequest, Map authHeaders) throws HttpResponseException { return TestUtils.post(getCollection(), createRequest, entityClass, authHeaders); } - public final T updateEntity(Object updateRequest, Status status, Map authHeaders) + protected final T updateEntity(Object updateRequest, Status status, Map authHeaders) throws HttpResponseException { return TestUtils.put(getCollection(), updateRequest, entityClass, status, authHeaders); } - public final T patchEntity(UUID id, String originalJson, T updated, Map authHeaders) + protected final T patchEntity(UUID id, String originalJson, T updated, Map 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 authHeaders) throws HttpResponseException { + protected final T createAndCheckEntity(Object create, Map 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 extends CatalogApplicationTest { return entity; } - public T updateAndCheckEntity(Object request, Status status, Map authHeaders, + protected final T updateAndCheckEntity(Object request, Status status, Map authHeaders, UpdateType updateType, ChangeDescription changeDescription) throws IOException { T updated = updateEntity(request, status, authHeaders); @@ -369,7 +374,7 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { return updated; } - public final T patchEntityAndCheck(T updated, String originalJson, Map authHeaders, + protected final T patchEntityAndCheck(T updated, String originalJson, Map authHeaders, UpdateType updateType, ChangeDescription expectedChange) throws JsonProcessingException, HttpResponseException { EntityInterface entityInterface = getEntityInterface(updated); @@ -386,7 +391,7 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { return returned; } - public final void validateCommonEntityFields(EntityInterface entity, String expectedDescription, + protected final void validateCommonEntityFields(EntityInterface entity, String expectedDescription, String expectedUpdatedByUser, EntityReference expectedOwner) { assertNotNull(entity.getId()); assertNotNull(entity.getHref()); @@ -396,7 +401,7 @@ public abstract class EntityResourceTest 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 updatedEntityInterface = getEntityInterface(updated); if (updateType == UpdateType.CREATED) { @@ -414,29 +419,29 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { } } - public EntityHistory getVersionList(UUID id, Map authHeaders) throws HttpResponseException { + protected EntityHistory getVersionList(UUID id, Map authHeaders) throws HttpResponseException { WebTarget target = getResource(collectionName + "/" + id + "/versions"); return TestUtils.get(target, EntityHistory.class, authHeaders); } - public T getVersion(UUID id, Double version, Map authHeaders) throws HttpResponseException { + protected T getVersion(UUID id, Double version, Map authHeaders) throws HttpResponseException { WebTarget target = getResource(collectionName + "/" + id + "/versions/" + version.toString()); return TestUtils.get(target, entityClass, authHeaders); } - public void assertFieldLists(List expectedList, List actualList) { + protected static void assertFieldLists(List expectedList, List 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 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 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()); + } + } } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java index be87a073f6f..0320f09f866 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java +++ b/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 { 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
{ return getTable; } - private static void checkOwnerOwns(EntityReference owner, UUID tableId, boolean expectedOwning) - throws HttpResponseException { - if (owner != null) { - UUID ownerId = owner.getId(); - List 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 authHeaders) throws HttpResponseException { return TagResourceTest.getTag(tagFQN, "usageCount", authHeaders).getUsageCount();