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 ae811597cb8..7f9fc540ded 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 @@ -6,6 +6,7 @@ import org.openmetadata.catalog.jdbi3.CollectionDAO.EntityVersionPair; import org.openmetadata.catalog.type.ChangeDescription; import org.openmetadata.catalog.type.EntityHistory; import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.type.TagLabel; import org.openmetadata.catalog.util.EntityInterface; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.EntityUtil.Fields; @@ -262,15 +263,16 @@ public abstract class EntityRepository { private void updateTags() throws IOException { // Remove current table 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()); if (!patchOperation) { // PUT operation merges tags in the request with what already exists - updated.setTags(EntityUtil.mergeTags(updated.getTags(), original.getTags())); + updated.setTags(EntityUtil.mergeTags(updatedTags, origTags)); } - recordChange("tags", original.getTags() == null ? 0 : original.getTags().size(), - updated.getTags() == null ? 0 : updated.getTags().size()); - EntityUtil.applyTags(daoCollection.tagDAO(), updated.getTags(), updated.getFullyQualifiedName()); + recordTagChange("tags", origTags, updatedTags); + EntityUtil.applyTags(daoCollection.tagDAO(), updatedTags, updated.getFullyQualifiedName()); } @@ -312,8 +314,22 @@ public abstract class EntityRepository { return false; } + public final boolean recordTagChange(String field, List origTags, List updatedTags) { + if (origTags == null || origTags.isEmpty()) { + origTags = null; + } else { + origTags.sort(Comparator.comparing(TagLabel::getTagFQN)); + } + if (updatedTags == null || updatedTags.isEmpty()) { + updatedTags = null; + } else { + updatedTags.sort(Comparator.comparing(TagLabel::getTagFQN)); + } + return recordChange(field,origTags, updatedTags); + } + private void storeOldVersion() throws IOException { - // TODO move this into a single palce + // TODO move this into a single place String extensionName = EntityUtil.getVersionExtension(entityName, original.getVersion()); daoCollection.entityExtensionDAO().insert(original.getId().toString(), extensionName, entityName, JsonUtils.pojoToJson(original.getEntity())); @@ -322,12 +338,10 @@ public abstract class EntityRepository { public final void store() throws IOException, ParseException { if (updateVersion(original.getVersion())) { // Store the old version - List versions = new ArrayList<>(); - versions.add(original.getEntity()); - EntityHistory history = new EntityHistory().withEntityType(entityName).withVersions(versions); storeOldVersion(); + // Store the new version + EntityRepository.this.store(updated.getEntity(), true); } - EntityRepository.this.store(updated.getEntity(), true); } } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java index a6b12820d6e..307abdd495e 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java @@ -639,8 +639,11 @@ public class TableRepository extends EntityRepository { @Override public void entitySpecificUpdate() throws IOException { - updateConstraints(original.getEntity(), updated.getEntity()); - updateColumns(original.getEntity().getColumns(), updated.getEntity().getColumns()); + Table origTable = original.getEntity(); + Table updatedTable = updated.getEntity(); + updateConstraints(origTable, updatedTable); + updateTableType(origTable, updatedTable); + updateColumns(origTable.getColumns(), updated.getEntity().getColumns()); } private void updateConstraints(Table origTable, Table updatedTable) { @@ -658,6 +661,10 @@ public class TableRepository extends EntityRepository
{ recordChange("tableConstraints", origConstraints, updatedConstraints); } + private void updateTableType(Table origTable, Table updatedTable) { + recordChange("tableType", origTable.getTableType(), updatedTable.getTableType()); + } + private void updateColumns(List origColumns, List updatedColumns) throws IOException { // Carry forward the user generated metadata from existing columns to new columns for (Column updated : updatedColumns) { @@ -721,10 +728,7 @@ public class TableRepository extends EntityRepository
{ // PUT operation merges tags in the request with what already exists updatedColumn.setTags(EntityUtil.mergeTags(updatedColumn.getTags(), origColumn.getTags())); } - - recordChange(getColumnField(origColumn, "tags"), - origColumn.getTags() == null ? 0 : origColumn.getTags().size(), - updatedColumn.getTags() == null ? 0 : updatedColumn.getTags().size()); + recordTagChange(getColumnField(origColumn, "tags"), origColumn.getTags(), updatedColumn.getTags()); EntityUtil.applyTags(dao.tagDAO(), updatedColumn.getTags(), updatedColumn.getFullyQualifiedName()); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityTestHelper.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityTestHelper.java new file mode 100644 index 00000000000..8219b347881 --- /dev/null +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityTestHelper.java @@ -0,0 +1,173 @@ +package org.openmetadata.catalog.resources; + +import com.fasterxml.jackson.core.JsonProcessingException; +import org.apache.http.client.HttpResponseException; +import org.openmetadata.catalog.CatalogApplicationTest; +import org.openmetadata.catalog.type.ChangeDescription; +import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.util.EntityInterface; +import org.openmetadata.catalog.util.JsonUtils; +import org.openmetadata.catalog.util.TestUtils; +import org.openmetadata.catalog.util.TestUtils.UpdateType; +import org.openmetadata.common.utils.JsonSchemaUtil; + +import javax.json.JsonPatch; +import javax.ws.rs.client.WebTarget; +import javax.ws.rs.core.Response.Status; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertIterableEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +public abstract class EntityTestHelper extends CatalogApplicationTest { + private final Class entityClass; + + public EntityTestHelper(Class entityClass) { + this.entityClass = entityClass; + } + + /** + * Methods to be overridden entity classes + */ + public abstract WebTarget getCollection(); + + public abstract WebTarget getResource(UUID id); + + public abstract void validateCreatedEntity(T createdEntity, Object request, Map authHeaders) + throws HttpResponseException; + + public abstract void validateUpdatedEntity(T updatedEntity, Object request, Map authHeaders) + throws HttpResponseException; + + public abstract void validatePatchedEntity(T expected, T updated, Map authHeaders) + throws HttpResponseException; + + + public abstract T getEntity(UUID id, Map authHeaders) throws HttpResponseException; + + public abstract EntityInterface getEntityInterface(T entity); + + public 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) + throws HttpResponseException { + return TestUtils.put(getCollection(), updateRequest, entityClass, status, authHeaders); + } + + public 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 { + // Validate an entity that is created has all the information set in create request + String updatedBy = TestUtils.getPrincipal(authHeaders); + T entity = createEntity(create, authHeaders); + EntityInterface entityInterface = getEntityInterface(entity); + + assertEquals(updatedBy, entityInterface.getUpdatedBy()); + assertEquals(0.1, entityInterface.getVersion()); // First version of the entity + validateCreatedEntity(entity, create, authHeaders); + + // GET the entity created and ensure it has all the information set in create request + T getEntity = getEntity(entityInterface.getId(), authHeaders); + assertEquals(0.1, entityInterface.getVersion()); // First version of the entity + validateCreatedEntity(getEntity, create, authHeaders); + + return entity; + } + + public T updateAndCheckEntity(Object request, Status status, Map authHeaders, + UpdateType updateType, ChangeDescription changeDescription) + throws HttpResponseException { + T updated = updateEntity(request, status, authHeaders); + validateUpdatedEntity(updated, request, authHeaders); + validateChangeDescription(updated, updateType, changeDescription); + + // GET the newly updated database and validate + EntityInterface entityInterface = getEntityInterface(updated); + T getEntity = getEntity(entityInterface.getId(), authHeaders); + validateUpdatedEntity(getEntity, request, authHeaders); + validateChangeDescription(getEntity, updateType, changeDescription); + return updated; + } + + public final T patchEntityAndCheck(T updated, String originalJson, Map authHeaders, + UpdateType updateType, ChangeDescription expectedChange) + throws JsonProcessingException, HttpResponseException { + String updatedBy = TestUtils.getPrincipal(authHeaders); + EntityInterface entityInterface = getEntityInterface(updated); + + // Validate information returned in patch response has the updates + T returned = patchEntity(entityInterface.getId(), originalJson, updated, authHeaders); + validatePatchedEntity(updated, returned, authHeaders); + validateChangeDescription(returned, updateType, expectedChange); + + // GET the entity and Validate information returned + T getEntity = getEntity(entityInterface.getId(), authHeaders); + validatePatchedEntity(updated, returned, authHeaders); + validateChangeDescription(getEntity, updateType, expectedChange); + return returned; + } + + public final void validateCommonEntityFields(EntityInterface entity, String expectedDescription, + String expectedUpdatedByUser, EntityReference expectedOwner) { + assertNotNull(entity.getId()); + assertNotNull(entity.getHref()); + assertNotNull(entity.getFullyQualifiedName()); + assertEquals(expectedDescription, entity.getDescription()); + assertEquals(expectedUpdatedByUser, entity.getUpdatedBy()); + assertOwner(expectedOwner, entity.getOwner()); + } + + public final void validateChangeDescription(T updated, UpdateType updateType, + ChangeDescription expectedChange) { + EntityInterface updatedEntityInterface = getEntityInterface(updated); + if (updateType == UpdateType.CREATED) { + return; // PUT operation was used to create an entity. No change description expected. + } + TestUtils.validateUpdate(expectedChange.getPreviousVersion(), updatedEntityInterface.getVersion(), updateType); + + if (updateType != UpdateType.NO_CHANGE) { + ChangeDescription change = updatedEntityInterface.getChangeDescription(); + assertEquals(expectedChange.getPreviousVersion(), change.getPreviousVersion()); + + assertFieldLists(expectedChange.getFieldsAdded(), change.getFieldsAdded()); + assertFieldLists(expectedChange.getFieldsUpdated(), change.getFieldsUpdated()); + assertFieldLists(expectedChange.getFieldsDeleted(), change.getFieldsDeleted()); + } + } + + public 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) { + return new ChangeDescription().withPreviousVersion(previousVersion) + .withFieldsAdded(new ArrayList<>()).withFieldsUpdated(new ArrayList<>()) + .withFieldsDeleted(new ArrayList<>()); + } + + public static void assertOwner(EntityReference expected, EntityReference actual) { + if (expected != null) { + TestUtils.validateEntityReference(actual); + assertEquals(expected.getId(), actual.getId()); + assertEquals(expected.getType(), actual.getType()); + } else { + assertNull(actual); + } + } +} diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java index ff5f4f0e4e2..948c5ec22f0 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java @@ -308,9 +308,10 @@ public class ChartResourceTest extends CatalogApplicationTest { CreateChart request = create(test).withService(SUPERSET_REFERENCE).withOwner(USER_OWNER1); // Create chart as admin Chart chart = createAndCheckChart(request, adminAuthHeaders()); - // Update chart as owner - but description to null is not updated - updateAndCheckChart(chart, request.withDescription(null), OK, authHeaders(USER1.getEmail()), NO_CHANGE); - // Update chart as owner - but description to null is not updated + // Update chart as user owner + chart = updateAndCheckChart(chart, request.withDescription("new1"), OK, authHeaders(USER1.getEmail()), + MINOR_UPDATE); + // Update chart ownership as owner updateAndCheckChart(chart, request.withOwner(TEAM_OWNER1), OK, authHeaders(USER1.getEmail()), MINOR_UPDATE); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/DatabaseResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/DatabaseResourceTest.java index b7a0e7b8850..f9f934ada85 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/DatabaseResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/DatabaseResourceTest.java @@ -328,7 +328,7 @@ public class DatabaseResourceTest extends CatalogApplicationTest { // Add database as admin Database database = createAndCheckDatabase(request, adminAuthHeaders()); // Update the table as Owner - updateAndCheckDatabase(database, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE); + updateAndCheckDatabase(database, request.withDescription("new"), OK, authHeaders(USER1.getEmail()), MINOR_UPDATE); } @Test 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 69479ebfda5..50c3ac66220 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 @@ -63,11 +63,9 @@ import org.openmetadata.catalog.util.JsonUtils; import org.openmetadata.catalog.util.RestUtil; import org.openmetadata.catalog.util.TestUtils; import org.openmetadata.catalog.util.TestUtils.UpdateType; -import org.openmetadata.common.utils.JsonSchemaUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import javax.json.JsonPatch; import javax.ws.rs.client.WebTarget; import javax.ws.rs.core.Response.Status; import java.text.ParseException; @@ -133,6 +131,10 @@ public class TableResourceTest extends EntityTestHelper
{ public static EntityReference TEAM_OWNER1; public static EntityReference SNOWFLAKE_REFERENCE; + public TableResourceTest() { + super(Table.class); + } + @BeforeAll public static void setup(TestInfo test) throws HttpResponseException { CreateDatabaseService createSnowflake = new CreateDatabaseService() @@ -290,18 +292,17 @@ public class TableResourceTest extends EntityTestHelper
{ Table table1 = createAndCheckEntity(create1, adminAuthHeaders()); // Test PUT operation - CreateTable create2 = create(test, 2).withColumns(Arrays.asList(c1, c2)); - Table table2= updateAndCheckEntity(null, create2.withName("put_complexColumnType"), Status.CREATED, - adminAuthHeaders(), NO_CHANGE, null); + CreateTable create2 = create(test, 2).withColumns(Arrays.asList(c1, c2)).withName("put_complexColumnType"); + Table table2= updateAndCheckEntity(create2, Status.CREATED, adminAuthHeaders(), UpdateType.CREATED, null); // Update without any change - updateAndCheckEntity(table2, create2.withName("put_complexColumnType"), Status.OK, adminAuthHeaders(), - NO_CHANGE, null); + ChangeDescription change = getChangeDescription(table2.getVersion()); + updateAndCheckEntity(create2, Status.OK, adminAuthHeaders(), NO_CHANGE, change); // // Update the complex columns // // c1 from array to array - Data type change means old c1 deleted, and new c1 added - ChangeDescription change = getChangeDescription(table2.getVersion()); + change = getChangeDescription(table2.getVersion()); c1.withArrayDataType(CHAR).withTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)).withDataTypeDisplay("array"); change.getFieldsDeleted().add("column:c1"); change.getFieldsAdded().add("column:c1"); @@ -330,7 +331,7 @@ public class TableResourceTest extends EntityTestHelper
{ // c2.b char --> SAME // c2.c struct> // c2.c.d int - updateAndCheckEntity(table2, create2.withName("put_complexColumnType"), Status.OK, + updateAndCheckEntity(create2.withName("put_complexColumnType"), Status.OK, adminAuthHeaders(), MAJOR_UPDATE, change); // @@ -355,7 +356,7 @@ public class TableResourceTest extends EntityTestHelper
{ c2_c_d = c2_c.getChildren().get(0); c2_c_d.setTags(singletonList(USER_BANK_ACCOUNT_TAG_LABEL)); // c2.c.d new tag added - table1 = patchTable(tableJson, table1, adminAuthHeaders()); + table1 = patchEntity(table1.getId(), tableJson, table1, adminAuthHeaders()); validateColumns(Arrays.asList(c1, c2), table1.getColumns()); } @@ -409,16 +410,17 @@ public class TableResourceTest extends EntityTestHelper
{ Table table = createAndCheckEntity(request, adminAuthHeaders()); // Update table two times successfully with PUT requests - updateAndCheckEntity(table, request, OK, adminAuthHeaders(), NO_CHANGE, null); - updateAndCheckEntity(table, request, OK, adminAuthHeaders(), NO_CHANGE, null); + ChangeDescription change = getChangeDescription(table.getVersion()); + updateAndCheckEntity(request, OK, adminAuthHeaders(), NO_CHANGE, change); + updateAndCheckEntity(request, OK, adminAuthHeaders(), NO_CHANGE, change); } @Test public void put_tableCreate_200(TestInfo test) throws HttpResponseException { // Create a new table with put CreateTable request = create(test).withOwner(USER_OWNER1); - updateAndCheckEntity(null, request.withName("newName").withDescription(null), CREATED, - adminAuthHeaders(), NO_CHANGE, null); + updateAndCheckEntity(request.withName("newName").withDescription(null), CREATED, + adminAuthHeaders(), UpdateType.CREATED, null); } @Test @@ -430,7 +432,7 @@ public class TableResourceTest extends EntityTestHelper
{ // Update null description with a new description ChangeDescription change = getChangeDescription(table.getVersion()); change.getFieldsAdded().add("description"); - updateAndCheckEntity(table, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE, + updateAndCheckEntity(request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE, change); } @@ -443,7 +445,7 @@ public class TableResourceTest extends EntityTestHelper
{ // Update empty description with a new description and expect minor version update ChangeDescription change = getChangeDescription(table.getVersion()); change.getFieldsAdded().add("description"); - updateAndCheckEntity(table, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE, + updateAndCheckEntity(request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE, change); } @@ -453,7 +455,7 @@ public class TableResourceTest extends EntityTestHelper
{ Table table = createAndCheckEntity(request, adminAuthHeaders()); // Updating non-empty description is ignored - Table updatedTable = updateTable(request.withDescription("newDescription"), OK, adminAuthHeaders()); + Table updatedTable = updateEntity(request.withDescription("newDescription"), OK, adminAuthHeaders()); assertEquals("description", updatedTable.getDescription()); assertEquals(table.getVersion(), updatedTable.getVersion()); // No version change since description remained the same } @@ -467,7 +469,7 @@ public class TableResourceTest extends EntityTestHelper
{ // Change ownership from USER_OWNER1 to TEAM_OWNER1 ChangeDescription change = getChangeDescription(table.getVersion()); change.getFieldsUpdated().add("owner"); - Table updatedTable = updateAndCheckEntity(table, request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders(), + Table updatedTable = updateAndCheckEntity(request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders(), MINOR_UPDATE, change); checkOwnerOwns(USER_OWNER1, updatedTable.getId(), false); checkOwnerOwns(TEAM_OWNER1, updatedTable.getId(), true); @@ -475,7 +477,7 @@ public class TableResourceTest extends EntityTestHelper
{ // Remove ownership change = getChangeDescription(updatedTable.getVersion()); change.getFieldsDeleted().add("owner"); - updatedTable = updateAndCheckEntity(updatedTable, request.withOwner(null), OK, adminAuthHeaders(), MINOR_UPDATE, + updatedTable = updateAndCheckEntity(request.withOwner(null), OK, adminAuthHeaders(), MINOR_UPDATE, change); assertNull(updatedTable.getOwner()); checkOwnerOwns(TEAM_OWNER1, updatedTable.getId(), false); @@ -494,23 +496,24 @@ public class TableResourceTest extends EntityTestHelper
{ .withColumns(List.of(COLUMNS.get(0).getName())); change.getFieldsAdded().add("tableConstraints"); request = request.withTableConstraints(List.of(constraint)); - Table updatedTable = updateAndCheckEntity(table, request, OK, adminAuthHeaders(), MINOR_UPDATE, change); + Table updatedTable = updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change); // Update again with no change. Version must not change - updatedTable = updateAndCheckEntity(updatedTable, request, OK, adminAuthHeaders(), NO_CHANGE, null); + change = getChangeDescription(updatedTable.getVersion()); + updatedTable = updateAndCheckEntity(request, OK, adminAuthHeaders(), NO_CHANGE, change); // Update the table with new constraints change = getChangeDescription(updatedTable.getVersion()); constraint = constraint.withConstraintType(ConstraintType.PRIMARY_KEY); request = request.withTableConstraints(List.of(constraint)); change.getFieldsUpdated().add("tableConstraints"); - updatedTable = updateAndCheckEntity(updatedTable, request, OK, adminAuthHeaders(), MINOR_UPDATE, change); + updatedTable = updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change); // Remove table constraint and ensure minor version changes change = getChangeDescription(updatedTable.getVersion()); request = request.withTableConstraints(null); change.getFieldsDeleted().add("tableConstraints"); - updateAndCheckEntity(updatedTable, request, OK, adminAuthHeaders(), MINOR_UPDATE, change); + updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change); } @Test @@ -529,7 +532,7 @@ public class TableResourceTest extends EntityTestHelper
{ request.getColumns().get(1).withConstraint(ColumnConstraint.PRIMARY_KEY); change.getFieldsUpdated().add("column:c2.constraint"); - Table updatedTable = updateAndCheckEntity(table, request, OK, adminAuthHeaders(), MINOR_UPDATE, change); + Table updatedTable = updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change); // Remove column constraints and expect minor version change change = getChangeDescription(updatedTable.getVersion()); @@ -538,7 +541,7 @@ public class TableResourceTest extends EntityTestHelper
{ request.getColumns().get(1).withConstraint(null); change.getFieldsDeleted().add("column:c2.constraint"); - updateAndCheckEntity(updatedTable, request, OK, adminAuthHeaders(), MINOR_UPDATE, change); + updateAndCheckEntity(request, OK, adminAuthHeaders(), MINOR_UPDATE, change); } @Test @@ -574,7 +577,7 @@ public class TableResourceTest extends EntityTestHelper
{ updatedColumns.add(getColumn("c1", BIGINT, null).withTags(tags)); ChangeDescription change = getChangeDescription(table.getVersion()); change.getFieldsUpdated().add("column:c1.tags"); - table = updateAndCheckEntity(table, request.withColumns(updatedColumns), OK, adminAuthHeaders(), MINOR_UPDATE, + table = updateAndCheckEntity(request.withColumns(updatedColumns), OK, adminAuthHeaders(), MINOR_UPDATE, change); // Ensure tag usage counts are updated @@ -589,7 +592,7 @@ public class TableResourceTest extends EntityTestHelper
{ updatedColumns.add(getColumn("c2", BINARY, null).withOrdinalPosition(2) .withDataLength(10).withTags(tags)); change.getFieldsAdded().add("column:c2"); - table = updateAndCheckEntity(table, request.withColumns(updatedColumns), OK, adminAuthHeaders(), MINOR_UPDATE, + table = updateAndCheckEntity(request.withColumns(updatedColumns), OK, adminAuthHeaders(), MINOR_UPDATE, change); // Ensure tag usage counts are updated - column c2 added both address and bank tags @@ -603,7 +606,7 @@ public class TableResourceTest extends EntityTestHelper
{ change = getChangeDescription(table.getVersion()); updatedColumns.remove(1); change.getFieldsDeleted().add("column:c2"); - table = updateAndCheckEntity(table, request.withColumns(updatedColumns), OK, adminAuthHeaders(), MAJOR_UPDATE, + table = updateAndCheckEntity(request.withColumns(updatedColumns), OK, adminAuthHeaders(), MAJOR_UPDATE, change); assertEquals(1, table.getColumns().size()); @@ -1091,30 +1094,47 @@ public class TableResourceTest extends EntityTestHelper
{ assertNull(table.getTableType()); assertNull(table.getTableConstraints()); - // Add description, table tags, tier, owner, tableType, and tableConstraints when previously they were null List tableConstraints = List.of(new TableConstraint().withConstraintType(ConstraintType.UNIQUE) .withColumns(List.of(COLUMNS.get(0).getName()))); List tableTags = singletonList(USER_ADDRESS_TAG_LABEL); - table = patchTableAttributesAndCheck(table, "description", TEAM_OWNER1, TableType.Regular, - tableConstraints, tableTags, adminAuthHeaders(), MINOR_UPDATE); - table.setOwner(TEAM_OWNER1); // Get rid of href and name returned in the response for owner + // + // Add description, table tags, tier, owner, tableType, and tableConstraints when previously they were null + // + String originalJson = JsonUtils.pojoToJson(table); + table.withDescription("description").withOwner(TEAM_OWNER1).withTableType(TableType.Regular) + .withTableConstraints(tableConstraints).withTags(tableTags); + ChangeDescription change = getChangeDescription(table.getVersion()) + .withFieldsAdded(Arrays.asList("description", "owner", "tableType", "tableConstraints", "tags")); + table = patchEntityAndCheck(table, originalJson, adminAuthHeaders(), MINOR_UPDATE, change); + + // // Replace description, tier, owner, tableType, tableConstraints + // tableConstraints = List.of(new TableConstraint().withConstraintType(ConstraintType.UNIQUE) .withColumns(List.of(COLUMNS.get(1).getName()))); tableTags = singletonList(USER_BANK_ACCOUNT_TAG_LABEL); - table = patchTableAttributesAndCheck(table, "description1", USER_OWNER1, TableType.External, - tableConstraints, tableTags, adminAuthHeaders(), MINOR_UPDATE); + table.getOwner().setHref(null); // Clear hrefs + originalJson = JsonUtils.pojoToJson(table); + table.withDescription("description1").withOwner(USER_OWNER1).withTableType(TableType.External) + .withTableConstraints(tableConstraints).withTags(tableTags); + change = getChangeDescription(table.getVersion()) + .withFieldsUpdated(Arrays.asList("description", "owner", "tableType", "tableConstraints", "tags")); + table = patchEntityAndCheck(table, originalJson, adminAuthHeaders(), MINOR_UPDATE, change); table.setOwner(USER_OWNER1); // Get rid of href and name returned in the response for owner // Remove description, tier, owner, tableType, tableConstraints - patchTableAttributesAndCheck(table, null, null, null, null, null, - adminAuthHeaders(), MINOR_UPDATE); + table.getOwner().setHref(null); // Clear hrefs + originalJson = JsonUtils.pojoToJson(table); + table.withDescription(null).withOwner(null).withTableType(null).withTableConstraints(null).withTags(null); + change = getChangeDescription(table.getVersion()) + .withFieldsDeleted(Arrays.asList("description", "owner", "tableType", "tableConstraints", "tags")); + patchEntityAndCheck(table, originalJson, adminAuthHeaders(), MINOR_UPDATE, change); } @Test public void patch_tableColumns_200_ok(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Create table without description, table tags, tier, owner, tableType, and tableConstraints + // Create table with the following columns List columns = new ArrayList<>(); columns.add(getColumn("c1", INT, USER_ADDRESS_TAG_LABEL)); columns.add(getColumn("c2", BIGINT, USER_ADDRESS_TAG_LABEL)); @@ -1123,12 +1143,22 @@ public class TableResourceTest extends EntityTestHelper
{ Table table = createEntity(create(test).withColumns(columns), adminAuthHeaders()); // Update the column tags and description + ChangeDescription change = getChangeDescription(table.getVersion()); columns.get(0).withDescription("new0") .withTags(List.of(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)); // Add a tag - columns.get(1).withDescription("new1").withTags(List.of(USER_ADDRESS_TAG_LABEL));// No change in tag - columns.get(2).withDescription("new3").withTags(new ArrayList<>()); // Remove tag + change.getFieldsUpdated().add("column:c1.description"); + change.getFieldsUpdated().add("column:c1.tags"); - table = patchTableColumnAttributesAndCheck(table, columns, adminAuthHeaders()); + columns.get(1).withDescription("new1").withTags(List.of(USER_ADDRESS_TAG_LABEL));// No change in tag + change.getFieldsUpdated().add("column:c2.description"); + + columns.get(2).withDescription("new3").withTags(new ArrayList<>()); // Remove tag + change.getFieldsUpdated().add("column:c3.description"); + change.getFieldsDeleted().add("column:c3.tags"); + + String originalJson = JsonUtils.pojoToJson(table); + table.setColumns(columns); + table = patchEntityAndCheck(table, originalJson, adminAuthHeaders(), MINOR_UPDATE, change); validateColumns(columns, table.getColumns()); } @@ -1167,45 +1197,6 @@ public class TableResourceTest extends EntityTestHelper
{ assertResponse(exception, NOT_FOUND, CatalogExceptionMessage.entityNotFound("User", NON_EXISTENT_ENTITY)); } - private Table patchTableAttributesAndCheck(Table before, String description, EntityReference owner, - TableType tableType, List tableConstraints, - List tags, Map authHeaders, - UpdateType updateType) - throws JsonProcessingException, HttpResponseException { - String updatedBy = TestUtils.getPrincipal(authHeaders); - String tableJson = JsonUtils.pojoToJson(before); - - // Update the table attributes - before.setDescription(description); - before.setOwner(owner); - before.setTableType(tableType); - before.setTableConstraints(tableConstraints); - before.setTags(tags); - - // Validate information returned in patch response has the updates - Table updatedTable = patchTable(tableJson, before, authHeaders); - validateTable(updatedTable, before.getDescription(), before.getColumns(), owner, null, tableType, - tableConstraints, tags, updatedBy); - TestUtils.validateUpdate(before.getVersion(), updatedTable.getVersion(), updateType); - - // GET the table and Validate information returned - Table getTable = getTable(before.getId(), "owner,tableConstraints,columns, tags", authHeaders); - validateTable(getTable, before.getDescription(), before.getColumns(), owner, null, tableType, - tableConstraints, tags, updatedBy); - return updatedTable; - } - - private Table patchTableColumnAttributesAndCheck(Table table, List columns, Map authHeaders) - throws JsonProcessingException, HttpResponseException { - String tableJson = JsonUtils.pojoToJson(table); - - // Update the table attributes - table.setColumns(columns); - - // Validate information returned in patch response has the updates - return patchTable(tableJson, table, authHeaders); - } - void assertFields(List
tableList, String fieldsParam) { tableList.forEach(t -> assertFields(t, fieldsParam)); } @@ -1273,40 +1264,6 @@ public class TableResourceTest extends EntityTestHelper
{ assertEquals(table.getDatabase().getName(), DATABASE.getFullyQualifiedName()); } - private static void validateTable(Table table, String expectedDescription, - List expectedColumns, EntityReference expectedOwner, - UUID expectedDatabaseId, TableType expectedTableType, - List expectedTableConstraints, List expectedTags, - String expectedUpdatedByUser) - throws HttpResponseException { - assertNotNull(table.getId()); - assertNotNull(table.getHref()); - assertNotNull(table.getFullyQualifiedName()); - assertEquals(expectedDescription, table.getDescription()); - assertEquals(expectedTableType, table.getTableType()); - assertEquals(expectedUpdatedByUser, table.getUpdatedBy()); - - validateColumns(expectedColumns, table.getColumns()); - - // Validate owner - if (expectedOwner != null) { - TestUtils.validateEntityReference(table.getOwner()); - assertEquals(expectedOwner.getId(), table.getOwner().getId()); - assertEquals(expectedOwner.getType(), table.getOwner().getType()); - assertNotNull(table.getOwner().getHref()); - } - - // Validate database - if (expectedDatabaseId != null) { - TestUtils.validateEntityReference(table.getDatabase()); - assertEquals(expectedDatabaseId, table.getDatabase().getId()); - } - - // Validate table constraints - assertEquals(expectedTableConstraints, table.getTableConstraints()); - TestUtils.validateTags(table.getFullyQualifiedName(), expectedTags, table.getTags()); - TestUtils.validateEntityReference(table.getFollowers()); - } private static void validateColumn(Column expectedColumn, Column actualColumn) throws HttpResponseException { assertNotNull(actualColumn.getFullyQualifiedName()); @@ -1395,12 +1352,6 @@ public class TableResourceTest extends EntityTestHelper
{ return createEntity(create, adminAuthHeaders()); } - public static Table updateTable(CreateTable create, Status status, Map authHeaders) - throws HttpResponseException { - return TestUtils.put(CatalogApplicationTest.getResource("tables"), create, Table.class, - status, authHeaders); - } - public static void putJoins(UUID tableId, TableJoins joins, Map authHeaders) throws HttpResponseException { WebTarget target = CatalogApplicationTest.getResource("tables/" + tableId + "/joins"); @@ -1428,19 +1379,6 @@ public class TableResourceTest extends EntityTestHelper
{ assertResponse(exception, NOT_FOUND, CatalogExceptionMessage.entityNotFound(Entity.TABLE, id)); } - private Table patchTable(UUID tableId, String originalJson, Table updatedTable, - Map authHeaders) throws JsonProcessingException, HttpResponseException { - String updateTableJson = JsonUtils.pojoToJson(updatedTable); - JsonPatch patch = JsonSchemaUtil.getJsonPatch(originalJson, updateTableJson); - return TestUtils.patch(CatalogApplicationTest.getResource("tables/" + tableId), - patch, Table.class, authHeaders); - } - - private Table patchTable(String originalJson, Table updatedTable, - Map authHeaders) throws JsonProcessingException, HttpResponseException { - return patchTable(updatedTable.getId(), originalJson, updatedTable, authHeaders); - } - public static void addAndCheckFollower(Table table, UUID userId, Status status, int totalFollowerCount, Map authHeaders) throws HttpResponseException { WebTarget target = CatalogApplicationTest.getResource(String.format("tables/%s/followers", table.getId())); @@ -1527,10 +1465,6 @@ public class TableResourceTest extends EntityTestHelper
{ return TagResourceTest.getCategory(name, "usageCount", authHeaders).getUsageCount(); } - public static String getTableName(TestInfo test) { - return String.format("table_%s", test.getDisplayName()); - } - public static String getTableName(TestInfo test, int index) { return String.format("table%d_%s", index, test.getDisplayName()); } @@ -1548,11 +1482,6 @@ public class TableResourceTest extends EntityTestHelper
{ } } - @Override - public Table createEntity(Object createRequest, Map authHeaders) throws HttpResponseException { - return TestUtils.post(CatalogApplicationTest.getResource("tables"), createRequest, Table.class, authHeaders); - } - @Override public Table getEntity(UUID id, Map authHeaders) throws HttpResponseException { WebTarget target = CatalogApplicationTest.getResource("tables/" + id); @@ -1561,33 +1490,45 @@ public class TableResourceTest extends EntityTestHelper
{ } @Override - public void validateCreatedEntity(Table table, Object request, Map authHeaders) + public WebTarget getCollection() { + return CatalogApplicationTest.getResource("tables"); + } + + @Override + public WebTarget getResource(UUID id) { + return CatalogApplicationTest.getResource("tables/" + id); + } + + @Override + public void validateCreatedEntity(Table createdEntity, Object request, Map authHeaders) throws HttpResponseException { CreateTable createRequest = (CreateTable) request; - validateCommonEntityFields(getEntityInterface(table), createRequest.getDescription(), + validateCommonEntityFields(getEntityInterface(createdEntity), createRequest.getDescription(), TestUtils.getPrincipal(authHeaders), createRequest.getOwner()); // Entity specific validation - assertEquals(createRequest.getTableType(), table.getTableType()); - validateColumns(createRequest.getColumns(), table.getColumns()); - validateDatabase(createRequest.getDatabase(), table.getDatabase()); + assertEquals(createRequest.getTableType(), createdEntity.getTableType()); + validateColumns(createRequest.getColumns(), createdEntity.getColumns()); + validateDatabase(createRequest.getDatabase(), createdEntity.getDatabase()); // Validate table constraints - assertEquals(createRequest.getTableConstraints(), table.getTableConstraints()); - TestUtils.validateTags(table.getFullyQualifiedName(), createRequest.getTags(), table.getTags()); - TestUtils.validateEntityReference(table.getFollowers()); + assertEquals(createRequest.getTableConstraints(), createdEntity.getTableConstraints()); + TestUtils.validateTags(createdEntity.getFullyQualifiedName(), createRequest.getTags(), createdEntity.getTags()); + TestUtils.validateEntityReference(createdEntity.getFollowers()); } @Override - public Table updateEntity(Object updateRequest, Status status, Map authHeaders) + public void validateUpdatedEntity(Table updated, Object request, Map authHeaders) throws HttpResponseException { - return TestUtils.put(CatalogApplicationTest.getResource("tables"), updateRequest, Table.class, - status, authHeaders); + validateCreatedEntity(updated, request, authHeaders); } @Override - public void validateUpdatedEntity(Table updated, Object request, Map authHeaders) throws HttpResponseException { - validateCreatedEntity(updated, request, authHeaders); + public void validatePatchedEntity(Table expected, Table patched, Map authHeaders) throws HttpResponseException { + assertEquals(expected.getDescription(), patched.getDescription()); + assertOwner(expected.getOwner(), patched.getOwner()); + assertEquals(expected.getTableType(), patched.getTableType()); + TestUtils.validateTags(expected.getFullyQualifiedName(), expected.getTags(), patched.getTags()); } private void validateDatabase(UUID expectedDatabaseId, EntityReference database) { diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/models/ModelResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/models/ModelResourceTest.java index 9662d056517..3e3aa3107fe 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/models/ModelResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/models/ModelResourceTest.java @@ -299,7 +299,7 @@ public class ModelResourceTest extends CatalogApplicationTest { // Add model as admin Model model = createAndCheckModel(request, adminAuthHeaders()); // Update the table as Owner - updateAndCheckModel(model, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE); + updateAndCheckModel(model, request.withDescription("new"), OK, authHeaders(USER1.getEmail()), MINOR_UPDATE); } @Test diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/pipelines/PipelineResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/pipelines/PipelineResourceTest.java index 1200961775a..f98e5a1d2fc 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/pipelines/PipelineResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/pipelines/PipelineResourceTest.java @@ -335,8 +335,8 @@ public class PipelineResourceTest extends CatalogApplicationTest { CreatePipeline request = create(test).withService(AIRFLOW_REFERENCE).withOwner(USER_OWNER1); // Add pipeline as admin Pipeline pipeline = createAndCheckPipeline(request, adminAuthHeaders()); - // Update the table as user owner - updateAndCheckPipeline(pipeline, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE); + // Update the pipeline as user owner + updateAndCheckPipeline(pipeline, request.withDescription("new"), OK, authHeaders(USER1.getEmail()), MINOR_UPDATE); } @Test diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/tasks/TaskResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/tasks/TaskResourceTest.java index 5767919750e..a3ee173c98c 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/tasks/TaskResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/tasks/TaskResourceTest.java @@ -319,8 +319,8 @@ public class TaskResourceTest extends CatalogApplicationTest { CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withOwner(USER_OWNER1); // Add task as admin Task task = createAndCheckTask(request, adminAuthHeaders()); - // Update the task Owner and see if it is allowed - updateAndCheckTask(task, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE); + // Update the task as Owner and see if it is allowed + updateAndCheckTask(task, request.withDescription("updated"), OK, authHeaders(USER1.getEmail()), MINOR_UPDATE); } @Test 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 ef589dcaace..b4047e08a01 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 @@ -329,7 +329,7 @@ public class TopicResourceTest extends CatalogApplicationTest { // Add as admin Topic topic = createAndCheckTopic(request, adminAuthHeaders()); // Update the topic as Owner - updateAndCheckTopic(topic, request.withDescription(null), OK, authHeaders(USER1.getEmail()), NO_CHANGE); + updateAndCheckTopic(topic, request.withDescription("new"), OK, authHeaders(USER1.getEmail()), MINOR_UPDATE); } @Test diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java index 49725437d1d..0f2f0b05993 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/util/TestUtils.java @@ -62,9 +62,10 @@ public final class TestUtils { public static URI PIPELINE_URL; public enum UpdateType { + CREATED, // No updated. The entity was created NO_CHANGE, // PUT/PATCH made no change MINOR_UPDATE, // PUT/PATCH made backward compatible minor version change - MAJOR_UPDATE // PUT/PATCH made backward incompatible minor version change + MAJOR_UPDATE // PUT/PATCH made backward incompatible minor version change } static { @@ -253,11 +254,7 @@ public final class TestUtils { updatedExpectedList = updatedExpectedList.stream().distinct().collect(Collectors.toList()); updatedExpectedList.sort(Comparator.comparing(TagLabel::getTagFQN)); actualList.sort(Comparator.comparing(TagLabel::getTagFQN)); - - assertEquals(updatedExpectedList.size(), actualList.size(), fqn); - for (int i = 0; i < actualList.size(); i++) { - assertEquals(updatedExpectedList.get(i), actualList.get(i)); - } + assertEquals(updatedExpectedList, actualList); } public static Map adminAuthHeaders() { @@ -294,7 +291,9 @@ public final class TestUtils { // TODO remove this public static void validateUpdate(Double previousVersion, Double newVersion, UpdateType updateType) { - if (updateType == UpdateType.NO_CHANGE) { + if (updateType == UpdateType.CREATED) { + assertEquals(0.1, newVersion); // New version of entity created + } else if (updateType == UpdateType.NO_CHANGE) { assertEquals(previousVersion, newVersion); // No change in the version } else if (updateType == UpdateType.MINOR_UPDATE) { assertEquals(Math.round((previousVersion + 0.1) * 10.0)/10.0, newVersion); // Minor version change