diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DatabaseRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DatabaseRepository.java index 390999a4eac..8ca30ade58b 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DatabaseRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DatabaseRepository.java @@ -16,11 +16,11 @@ package org.openmetadata.catalog.jdbi3; +import com.fasterxml.jackson.core.JsonProcessingException; import org.openmetadata.catalog.Entity; 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.exception.CatalogExceptionMessage; import org.openmetadata.catalog.exception.EntityNotFoundException; import org.openmetadata.catalog.jdbi3.DatabaseServiceRepository.DatabaseServiceDAO; import org.openmetadata.catalog.jdbi3.TableRepository.TableDAO; @@ -30,6 +30,9 @@ import org.openmetadata.catalog.jdbi3.UserRepository.UserDAO; import org.openmetadata.catalog.resources.databases.DatabaseResource; import org.openmetadata.catalog.resources.databases.DatabaseResource.DatabaseList; import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.type.TagLabel; +import org.openmetadata.catalog.util.EntityInterface; +import org.openmetadata.catalog.util.EntityUpdater; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.EntityUtil.Fields; import org.openmetadata.catalog.util.JsonUtils; @@ -44,7 +47,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.json.JsonPatch; -import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; import java.io.IOException; import java.security.GeneralSecurityException; @@ -52,6 +54,7 @@ import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.Objects; +import java.util.UUID; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.entityNotFound; @@ -149,7 +152,6 @@ public abstract class DatabaseRepository { @Transaction public Database create(Database database, EntityReference service, EntityReference owner) throws IOException { - getService(service); // Validate service return createInternal(database, service, owner); } @@ -165,31 +167,23 @@ public abstract class DatabaseRepository { } @Transaction - public PutResponse createOrUpdate(Database updatedDB, EntityReference service, EntityReference newOwner) + public PutResponse createOrUpdate(Database updated, EntityReference service, EntityReference newOwner) throws IOException { getService(service); // Validate service - String fqn = getFQN(service, updatedDB); - Database storedDB = JsonUtils.readValue(databaseDAO().findByFQN(fqn), Database.class); - if (storedDB == null) { // Database does not exist. Create a new one - return new PutResponse<>(Status.CREATED, createInternal(updatedDB, service, newOwner)); + String fqn = getFQN(service, updated); + Database stored = JsonUtils.readValue(databaseDAO().findByFQN(fqn), Database.class); + if (stored == null) { // Database does not exist. Create a new one + return new PutResponse<>(Status.CREATED, createInternal(updated, service, newOwner)); } - // Update the existing database - EntityUtil.populateOwner(userDAO(), teamDAO(), newOwner); // Validate new owner - if (storedDB.getDescription() == null || storedDB.getDescription().isEmpty()) { - storedDB.withDescription(updatedDB.getDescription()); - } - databaseDAO().update(storedDB.getId().toString(), JsonUtils.pojoToJson(storedDB)); + setFields(stored, DATABASE_UPDATE_FIELDS); + updated.setId(stored.getId()); + validateRelationships(updated, service, newOwner); - // Update owner relationship - setFields(storedDB, DATABASE_UPDATE_FIELDS); // First get the ownership information - updateOwner(storedDB, storedDB.getOwner(), newOwner); - - // Service can't be changed in update since service name is part of FQN and - // change to a different service will result in a different FQN and creation of a new database under the new service - storedDB.setService(service); - - return new PutResponse<>(Response.Status.OK, storedDB); + DatabaseUpdater databaseUpdater = new DatabaseUpdater(stored, updated, false); + databaseUpdater.updateAll(); + databaseUpdater.store(); + return new PutResponse<>(Status.OK, updated); } @Transaction @@ -205,44 +199,49 @@ public abstract class DatabaseRepository { } public Database createInternal(Database database, EntityReference service, EntityReference owner) throws IOException { - database.setFullyQualifiedName(getFQN(service, database)); - EntityUtil.populateOwner(userDAO(), teamDAO(), owner); // Validate owner - - // Query 1 - insert database into database_entity table - databaseDAO().insert(JsonUtils.pojoToJson(database)); - setService(database, service); - setOwner(database, owner); + validateRelationships(database, service, owner); + storeDatabase(database, false); + addRelationships(database); return database; } + private void validateRelationships(Database database, EntityReference service, EntityReference owner) throws IOException { + database.setFullyQualifiedName(getFQN(service, database)); + database.setOwner(EntityUtil.populateOwner(userDAO(), teamDAO(), owner)); // Validate owner + database.setService(getService(service)); + } + + private void addRelationships(Database database) throws IOException { + setService(database, database.getService()); + setOwner(database, database.getOwner()); + } + + private void storeDatabase(Database database, boolean update) throws JsonProcessingException { + // Relationships and fields such as href are derived and not stored as part of json + EntityReference owner = database.getOwner(); + EntityReference service = database.getService(); + + // Don't store owner, database, href and tags as JSON. Build it on the fly based on relationships + database.withOwner(null).withService(null).withHref(null); + + if (update) { + databaseDAO().update(database.getId().toString(), JsonUtils.pojoToJson(database)); + } else { + databaseDAO().insert(JsonUtils.pojoToJson(database)); + } + + // Restore the relationships + database.withOwner(owner).withService(service); + } + private void patch(Database original, Database updated) throws IOException { - String databaseId = original.getId().toString(); - if (!original.getId().equals(updated.getId())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.DATABASE, "id")); - } - if (!original.getName().equals(updated.getName())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.DATABASE, "name")); - } - if (updated.getService() == null || !original.getService().getId().equals(updated.getService().getId())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.DATABASE, "service")); - } - LOG.info("updated summary {}", updated.getUsageSummary()); - if (updated.getUsageSummary() == null || !original.getUsageSummary().equals(updated.getUsageSummary())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.DATABASE, - "usageSummary")); - } - // Validate new owner - EntityReference newOwner = EntityUtil.populateOwner(userDAO(), teamDAO(), updated.getOwner()); - - EntityReference newService = updated.getService(); - - // TODO tables can't be changed - updated.setHref(null); - updated.setOwner(null); - updated.setService(null); - databaseDAO().update(databaseId, JsonUtils.pojoToJson(updated)); - updateOwner(updated, original.getOwner(), newOwner); - updated.setService(newService); + // Patch can't make changes to following fields. Ignore the changes + updated.withFullyQualifiedName(original.getFullyQualifiedName()).withName(original.getName()) + .withService(original.getService()).withId(original.getId()); + validateRelationships(updated, updated.getService(), updated.getOwner()); + DatabaseUpdater databaseUpdater = new DatabaseUpdater(original, updated, true); + databaseUpdater.updateAll(); + databaseUpdater.store(); } public EntityReference getOwner(Database database) throws IOException { @@ -255,11 +254,6 @@ public abstract class DatabaseRepository { database.setOwner(owner); } - private void updateOwner(Database database, EntityReference origOwner, EntityReference newOwner) { - EntityUtil.updateOwner(relationshipDAO(), origOwner, newOwner, database.getId(), Entity.DATABASE); - database.setOwner(newOwner); - } - private List getTables(Database database) throws IOException { if (database == null) { return null; @@ -356,4 +350,79 @@ public abstract class DatabaseRepository { @SqlUpdate("DELETE FROM database_entity WHERE id = :id") int delete(@Bind("id") String id); } + + static class DatabaseEntityInterface implements EntityInterface { + private final Database database; + + DatabaseEntityInterface(Database Database) { + this.database = Database; + } + + @Override + public UUID getId() { + return database.getId(); + } + + @Override + public String getDescription() { + return database.getDescription(); + } + + @Override + public String getDisplayName() { + return database.getDisplayName(); + } + + @Override + public EntityReference getOwner() { + return database.getOwner(); + } + + @Override + public String getFullyQualifiedName() { + return database.getFullyQualifiedName(); + } + + @Override + public List getTags() { return null; } + + @Override + public void setDescription(String description) { + database.setDescription(description); + } + + @Override + public void setDisplayName(String displayName) { + database.setDisplayName(displayName); + } + + @Override + public void setTags(List tags) { } + } + + /** + * Handles entity updated from PUT and POST operation. + */ + public class DatabaseUpdater extends EntityUpdater { + final Database orig; + final Database updated; + + public DatabaseUpdater(Database orig, Database updated, boolean patchOperation) { + super(new DatabaseEntityInterface(orig), + new DatabaseEntityInterface(updated), patchOperation, relationshipDAO(), null); + + this.orig = orig; + this.updated = updated; + } + + public void updateAll() throws IOException { + super.updateAll(); +// updateService(); + } + + public void store() throws IOException { + updated.setVersion(getNewVersion(orig.getVersion())); + storeDatabase(updated, true); + } + } } 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 c275c498061..3eea3c57eb4 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 @@ -39,6 +39,7 @@ import org.openmetadata.catalog.type.EntityReference; 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; import org.openmetadata.common.utils.JsonSchemaUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -60,7 +61,8 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.entityNotFound; -import static org.openmetadata.catalog.exception.CatalogExceptionMessage.readOnlyAttribute; +import static org.openmetadata.catalog.util.TestUtils.UpdateType.MINOR_UPDATE; +import static org.openmetadata.catalog.util.TestUtils.UpdateType.NO_CHANGE; import static org.openmetadata.catalog.util.TestUtils.adminAuthHeaders; import static org.openmetadata.catalog.util.TestUtils.assertEntityPagination; import static org.openmetadata.catalog.util.TestUtils.assertResponse; @@ -292,51 +294,48 @@ public class DatabaseResourceTest extends CatalogApplicationTest { public void put_databaseUpdateWithNoChange_200(TestInfo test) throws HttpResponseException { // Create a database with POST CreateDatabase request = create(test).withService(SNOWFLAKE_REFERENCE).withOwner(USER_OWNER1); - createAndCheckDatabase(request, adminAuthHeaders()); + Database database = createAndCheckDatabase(request, adminAuthHeaders()); // Update database two times successfully with PUT requests - updateAndCheckDatabase(request, OK, adminAuthHeaders()); - updateAndCheckDatabase(request, OK, adminAuthHeaders()); + database = updateAndCheckDatabase(database, request, OK, adminAuthHeaders(), NO_CHANGE); + updateAndCheckDatabase(database, request, OK, adminAuthHeaders(), NO_CHANGE); } @Test public void put_databaseCreate_200(TestInfo test) throws HttpResponseException { - // Create a new database with put + // Create a new database with PUT CreateDatabase request = create(test).withService(SNOWFLAKE_REFERENCE).withOwner(USER_OWNER1); - updateAndCheckDatabase(request.withName(test.getDisplayName()).withDescription(null), CREATED, adminAuthHeaders()); + updateAndCheckDatabase(null, request.withName(test.getDisplayName()).withDescription(null), CREATED, + adminAuthHeaders(), NO_CHANGE); } @Test public void put_databaseCreate_as_owner_200(TestInfo test) throws HttpResponseException { // Create a new database with put CreateDatabase request = create(test).withService(SNOWFLAKE_REFERENCE).withOwner(USER_OWNER1); - // Add Owner as admin - createAndCheckDatabase(request, adminAuthHeaders()); - //Update the table as Owner - updateAndCheckDatabase(request.withName(test.getDisplayName()).withDescription(null), - CREATED, authHeaders(USER1.getEmail())); - + // Add database as admin + Database database = createAndCheckDatabase(request, adminAuthHeaders()); + // Update the table as Owner + updateAndCheckDatabase(database, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE); } @Test public void put_databaseNullDescriptionUpdate_200(TestInfo test) throws HttpResponseException { CreateDatabase request = create(test).withService(SNOWFLAKE_REFERENCE).withDescription(null); - createAndCheckDatabase(request, adminAuthHeaders()); + Database database = createAndCheckDatabase(request, adminAuthHeaders()); // Update null description with a new description - Database db = updateAndCheckDatabase(request.withDescription("newDescription"), OK, adminAuthHeaders()); - assertEquals("newDescription", db.getDescription()); + updateAndCheckDatabase(database, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE); } @Test public void put_databaseEmptyDescriptionUpdate_200(TestInfo test) throws HttpResponseException { // Create table with empty description CreateDatabase request = create(test).withService(SNOWFLAKE_REFERENCE).withDescription(""); - createAndCheckDatabase(request, adminAuthHeaders()); + Database database = createAndCheckDatabase(request, adminAuthHeaders()); // Update empty description with a new description - Database db = updateAndCheckDatabase(request.withDescription("newDescription"), OK, adminAuthHeaders()); - assertEquals("newDescription", db.getDescription()); + updateAndCheckDatabase(database, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE); } @Test @@ -352,14 +351,14 @@ public class DatabaseResourceTest extends CatalogApplicationTest { @Test public void put_databaseUpdateOwner_200(TestInfo test) throws HttpResponseException { CreateDatabase request = create(test).withService(SNOWFLAKE_REFERENCE).withDescription(""); - createAndCheckDatabase(request, adminAuthHeaders()); + Database database = createAndCheckDatabase(request, adminAuthHeaders()); // Change ownership from USER_OWNER1 to TEAM_OWNER1 - updateAndCheckDatabase(request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders()); + database = updateAndCheckDatabase(database, request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders(), MINOR_UPDATE); // Remove ownership - Database db = updateAndCheckDatabase(request.withOwner(null), OK, adminAuthHeaders()); - assertNull(db.getOwner()); + database = updateAndCheckDatabase(database, request.withOwner(null), OK, adminAuthHeaders(), MINOR_UPDATE); + assertNull(database.getOwner()); } @Test @@ -398,71 +397,19 @@ public class DatabaseResourceTest extends CatalogApplicationTest { database.getService().setHref(null); // href is readonly and not patchable // Add description, owner when previously they were null - database = patchDatabaseAttributesAndCheck(database, "description", TEAM_OWNER1, adminAuthHeaders()); + database = patchDatabaseAttributesAndCheck(database, "description", TEAM_OWNER1, + adminAuthHeaders(), MINOR_UPDATE); database.setOwner(TEAM_OWNER1); // Get rid of href and name returned in the response for owner database.setService(MYSQL_REFERENCE); // Get rid of href and name returned in the response for service // Replace description, tier, owner - database = patchDatabaseAttributesAndCheck(database, "description1", USER_OWNER1, adminAuthHeaders()); + database = patchDatabaseAttributesAndCheck(database, "description1", USER_OWNER1, + adminAuthHeaders(), MINOR_UPDATE); database.setOwner(USER_OWNER1); // Get rid of href and name returned in the response for owner database.setService(REDSHIFT_REFERENCE); // Get rid of href and name returned in the response for service // Remove description, tier, owner - patchDatabaseAttributesAndCheck(database, null, null, adminAuthHeaders()); - } - - @Test - public void patch_databaseIDChange_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure database ID can't be changed using patch - Database database = createDatabase(create(test), adminAuthHeaders()); - UUID databaseId = database.getId(); - String databaseJson = JsonUtils.pojoToJson(database); - database.setId(UUID.randomUUID()); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchDatabase(databaseId, databaseJson, database, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.DATABASE, "id")); - - // ID can't be deleted - database.setId(null); - exception = assertThrows(HttpResponseException.class, () -> - patchDatabase(databaseId, databaseJson, database, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.DATABASE, "id")); - } - - @Test - public void patch_databaseNameChange_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure database name can't be changed using patch - Database database = createDatabase(create(test), adminAuthHeaders()); - String databaseJson = JsonUtils.pojoToJson(database); - database.setName("newName"); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchDatabase(databaseJson, database, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.DATABASE, "name")); - - // Name can't be removed - database.setName(null); - exception = assertThrows(HttpResponseException.class, () -> - patchDatabase(databaseJson, database, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.DATABASE, "name")); - } - - @Test - public void patch_databaseRemoveService_400(TestInfo test) throws HttpResponseException, JsonProcessingException { - // Ensure service corresponding to database can't be changed by patch operation - Database database = createDatabase(create(test), adminAuthHeaders()); - database.getService().setHref(null); // Remove href from returned response as it is read-only field - - String databaseJson = JsonUtils.pojoToJson(database); - database.setService(MYSQL_REFERENCE); - HttpResponseException exception = assertThrows(HttpResponseException.class, () -> - patchDatabase(databaseJson, database, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.DATABASE, "service")); - - // Service relationship can't be removed - database.setService(null); - exception = assertThrows(HttpResponseException.class, () -> - patchDatabase(databaseJson, database, adminAuthHeaders())); - assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.DATABASE, "service")); + patchDatabaseAttributesAndCheck(database, null, null, adminAuthHeaders(), MINOR_UPDATE); } // TODO listing tables test:1 @@ -495,12 +442,17 @@ public class DatabaseResourceTest extends CatalogApplicationTest { return getAndValidate(database.getId(), create, authHeaders, updatedBy); } - public static Database updateAndCheckDatabase(CreateDatabase create, - Status status, - Map authHeaders) throws HttpResponseException { + public static Database updateAndCheckDatabase(Database before, CreateDatabase create, Status status, + Map authHeaders, UpdateType updateType) + throws HttpResponseException { String updatedBy = TestUtils.getPrincipal(authHeaders); Database updatedDb = updateDatabase(create, status, authHeaders); validateDatabase(updatedDb, create.getDescription(), create.getOwner(), create.getService(), updatedBy); + if (before == null) { + assertEquals(0.1, updatedDb.getVersion()); // First version created + } else { + TestUtils.validateUpdate(before.getVersion(), updatedDb.getVersion(), updateType); + } // GET the newly updated database and validate return getAndValidate(updatedDb.getId(), create, authHeaders, updatedBy); @@ -588,23 +540,24 @@ public class DatabaseResourceTest extends CatalogApplicationTest { return database; } - private Database patchDatabaseAttributesAndCheck(Database database, String newDescription, - EntityReference newOwner, Map authHeaders) + private Database patchDatabaseAttributesAndCheck(Database before, String newDescription, EntityReference newOwner, + Map authHeaders, UpdateType updateType) throws JsonProcessingException, HttpResponseException { String updatedBy = TestUtils.getPrincipal(authHeaders); - String databaseJson = JsonUtils.pojoToJson(database); + String databaseJson = JsonUtils.pojoToJson(before); // Update the table attributes - database.setDescription(newDescription); - database.setOwner(newOwner); + before.setDescription(newDescription); + before.setOwner(newOwner); // Validate information returned in patch response has the updates - Database updatedDatabase = patchDatabase(databaseJson, database, authHeaders); - validateDatabase(updatedDatabase, database.getDescription(), newOwner, null, updatedBy); + Database updatedDatabase = patchDatabase(databaseJson, before, authHeaders); + validateDatabase(updatedDatabase, before.getDescription(), newOwner, null, updatedBy); + TestUtils.validateUpdate(before.getVersion(), updatedDatabase.getVersion(), updateType); // GET the table and Validate information returned - Database getDatabase = getDatabase(database.getId(), "service,owner", authHeaders); - validateDatabase(getDatabase, database.getDescription(), newOwner, null, updatedBy); + Database getDatabase = getDatabase(before.getId(), "service,owner", authHeaders); + validateDatabase(getDatabase, before.getDescription(), newOwner, null, updatedBy); return updatedDatabase; }