Fixes #829 - Update Database version during PUT and POST operations

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<Database> createOrUpdate(Database updatedDB, EntityReference service, EntityReference newOwner)
+  public PutResponse<Database> createOrUpdate(Database updated, EntityReference service, EntityReference newOwner)
           throws IOException {
     getService(service); // Validate service
 
-    String fqn = getFQN(service, updatedDB);
+    String fqn = getFQN(service, updated);
-    Database storedDB = JsonUtils.readValue(databaseDAO().findByFQN(fqn), Database.class);
+    Database stored = JsonUtils.readValue(databaseDAO().findByFQN(fqn), Database.class);
-    if (storedDB == null) {  // Database does not exist. Create a new one
+    if (stored == null) {  // Database does not exist. Create a new one
-      return new PutResponse<>(Status.CREATED, createInternal(updatedDB, service, newOwner));
+      return new PutResponse<>(Status.CREATED, createInternal(updated, service, newOwner));
     }
-    // Update the existing database
+    setFields(stored, DATABASE_UPDATE_FIELDS);
-    EntityUtil.populateOwner(userDAO(), teamDAO(), newOwner); // Validate new owner
+    updated.setId(stored.getId());
-    if (storedDB.getDescription() == null || storedDB.getDescription().isEmpty()) {
+    validateRelationships(updated, service, newOwner);
-      storedDB.withDescription(updatedDB.getDescription());
-    }
-    databaseDAO().update(storedDB.getId().toString(), JsonUtils.pojoToJson(storedDB));
 
-    // Update owner relationship
+    DatabaseUpdater databaseUpdater = new DatabaseUpdater(stored, updated, false);
-    setFields(storedDB, DATABASE_UPDATE_FIELDS); // First get the ownership information
+    databaseUpdater.updateAll();
-    updateOwner(storedDB, storedDB.getOwner(), newOwner);
+    databaseUpdater.store();
-
+    return new PutResponse<>(Status.OK, updated);
-    // 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);
   }
 
   @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));
+    validateRelationships(database, service, owner);
-    EntityUtil.populateOwner(userDAO(), teamDAO(), owner); // Validate owner
+    storeDatabase(database, false);
-
+    addRelationships(database);
-    // Query 1 - insert database into database_entity table
-    databaseDAO().insert(JsonUtils.pojoToJson(database));
-    setService(database, service);
-    setOwner(database, owner);
     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();
+    // Patch can't make changes to following fields. Ignore the changes
-    if (!original.getId().equals(updated.getId())) {
+    updated.withFullyQualifiedName(original.getFullyQualifiedName()).withName(original.getName())
-      throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.DATABASE, "id"));
+            .withService(original.getService()).withId(original.getId());
-    }
+    validateRelationships(updated, updated.getService(), updated.getOwner());
-    if (!original.getName().equals(updated.getName())) {
+    DatabaseUpdater databaseUpdater = new DatabaseUpdater(original, updated, true);
-      throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.DATABASE, "name"));
+    databaseUpdater.updateAll();
-    }
+    databaseUpdater.store();
-    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);
   }
 
   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<Table> 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<TagLabel> 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<TagLabel> 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);
+    }
+  }
 }

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());
+    database = updateAndCheckDatabase(database, request, OK, adminAuthHeaders(), NO_CHANGE);
-    updateAndCheckDatabase(request, OK, adminAuthHeaders());
+    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
+    // Add database as admin
-    createAndCheckDatabase(request, adminAuthHeaders());
+    Database database = createAndCheckDatabase(request, adminAuthHeaders());
-    //Update the table as Owner
+    // Update the table as Owner
-    updateAndCheckDatabase(request.withName(test.getDisplayName()).withDescription(null),
+    updateAndCheckDatabase(database, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE);
-            CREATED, authHeaders(USER1.getEmail()));
-
   }
 
   @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());
+    updateAndCheckDatabase(database, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE);
-    assertEquals("newDescription", db.getDescription());
   }
 
   @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());
+    updateAndCheckDatabase(database, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE);
-    assertEquals("newDescription", db.getDescription());
   }
 
   @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());
+    database = updateAndCheckDatabase(database, request.withOwner(null), OK, adminAuthHeaders(), MINOR_UPDATE);
-    assertNull(db.getOwner());
+    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());
+    patchDatabaseAttributesAndCheck(database, null, null, adminAuthHeaders(), MINOR_UPDATE);
-  }
-
-  @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"));
   }
 
   // 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,
+  public static Database updateAndCheckDatabase(Database before, CreateDatabase create, Status status,
-                                                Status status,
+                                                Map<String, String> authHeaders, UpdateType updateType)
-                                                Map<String, String> authHeaders) throws HttpResponseException {
+          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,
+  private Database patchDatabaseAttributesAndCheck(Database before, String newDescription, EntityReference newOwner,
-                                                EntityReference newOwner, Map<String, String> authHeaders)
+                                                   Map<String, String> 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);
+    before.setDescription(newDescription);
-    database.setOwner(newOwner);
+    before.setOwner(newOwner);
 
     // Validate information returned in patch response has the updates
-    Database updatedDatabase = patchDatabase(databaseJson, database, authHeaders);
+    Database updatedDatabase = patchDatabase(databaseJson, before, authHeaders);
-    validateDatabase(updatedDatabase, database.getDescription(), newOwner, null, updatedBy);
+    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);
+    Database getDatabase = getDatabase(before.getId(), "service,owner", authHeaders);
-    validateDatabase(getDatabase, database.getDescription(), newOwner, null, updatedBy);
+    validateDatabase(getDatabase, before.getDescription(), newOwner, null, updatedBy);
     return updatedDatabase;
   }