Fixes #817 - Update task version during PUT and POST operations

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

@@ -16,11 +16,16 @@
 
 package org.openmetadata.catalog.jdbi3;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
 import org.openmetadata.catalog.Entity;
 import org.openmetadata.catalog.entity.data.Task;
+import org.openmetadata.catalog.entity.data.Task;
+import org.openmetadata.catalog.entity.data.Task;
 import org.openmetadata.catalog.entity.services.PipelineService;
 import org.openmetadata.catalog.exception.CatalogExceptionMessage;
 import org.openmetadata.catalog.exception.EntityNotFoundException;
+import org.openmetadata.catalog.jdbi3.ChartRepository.ChartUpdater;
+import org.openmetadata.catalog.jdbi3.TaskRepository.TaskEntityInterface;
 import org.openmetadata.catalog.jdbi3.PipelineServiceRepository.PipelineServiceDAO;
 import org.openmetadata.catalog.jdbi3.TeamRepository.TeamDAO;
 import org.openmetadata.catalog.jdbi3.UserRepository.UserDAO;
@@ -28,6 +33,8 @@ import org.openmetadata.catalog.resources.tasks.TaskResource;
 import org.openmetadata.catalog.resources.tasks.TaskResource.TaskList;
 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;
@@ -49,6 +56,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;
 
@@ -152,37 +160,23 @@ public abstract class TaskRepository {
   }
 
   @Transaction
-  public PutResponse<Task> createOrUpdate(Task updatedTask, EntityReference service, EntityReference newOwner)
+  public PutResponse<Task> createOrUpdate(Task updated, EntityReference service, EntityReference newOwner)
           throws IOException {
     getService(service); // Validate service
 
-    String fqn = getFQN(service, updatedTask);
+    String fqn = getFQN(service, updated);
-    Task storedDB = JsonUtils.readValue(taskDAO().findByFQN(fqn), Task.class);
+    Task stored = JsonUtils.readValue(taskDAO().findByFQN(fqn), Task.class);
-    if (storedDB == null) {  // Task does not exist. Create a new one
+    if (stored == null) {  // Task does not exist. Create a new one
-      return new PutResponse<>(Status.CREATED, createInternal(updatedTask, service, newOwner));
+      return new PutResponse<>(Status.CREATED, createInternal(updated, service, newOwner));
-    }
-    // Update the existing Task
-    EntityUtil.populateOwner(userDAO(), teamDAO(), newOwner); // Validate new owner
-    if (storedDB.getDescription() == null || storedDB.getDescription().isEmpty()) {
-      storedDB.withDescription(updatedTask.getDescription());
     }
+    setFields(stored, TASK_UPDATE_FIELDS);
+    updated.setId(stored.getId());
+    validateRelationships(updated, service, newOwner);
 
-    //update the display name from source
+    TaskUpdater taskUpdater = new TaskUpdater(stored, updated, false);
-    if (updatedTask.getDisplayName() != null && !updatedTask.getDisplayName().isEmpty()) {
+    taskUpdater.updateAll();
-      storedDB.withDisplayName(updatedTask.getDisplayName());
+    taskUpdater.store();
-    }
+    return new PutResponse<>(Status.OK, updated);
-    taskDAO().update(storedDB.getId().toString(), JsonUtils.pojoToJson(storedDB));
-
-    // Update owner relationship
-    setFields(storedDB, TASK_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 task under the new service
-    storedDB.setService(service);
-    applyTags(updatedTask);
-
-    return new PutResponse<>(Status.OK, storedDB);
   }
 
   @Transaction
@@ -195,17 +189,45 @@ public abstract class TaskRepository {
   }
 
   public Task createInternal(Task task, EntityReference service, EntityReference owner) throws IOException {
-    task.setFullyQualifiedName(getFQN(service, task));
+    validateRelationships(task, service, owner);
-    EntityUtil.populateOwner(userDAO(), teamDAO(), owner); // Validate owner
+    storeTask(task, false);
-
+    addRelationships(task);
-    // Query 1 - insert task into task_entity table
-    taskDAO().insert(JsonUtils.pojoToJson(task));
-    setService(task, service);
-    setOwner(task, owner);
-    applyTags(task);
     return task;
   }
 
+  private void validateRelationships(Task task, EntityReference service, EntityReference owner) throws IOException {
+    task.setFullyQualifiedName(getFQN(service, task));
+    EntityUtil.populateOwner(userDAO(), teamDAO(), owner); // Validate owner
+    getService(service);
+    task.setTags(EntityUtil.addDerivedTags(tagDAO(), task.getTags()));
+  }
+
+  private void addRelationships(Task task) throws IOException {
+    setService(task, task.getService());
+    setOwner(task, task.getOwner());
+    applyTags(task);
+  }
+
+  private void storeTask(Task task, boolean update) throws JsonProcessingException {
+    // Relationships and fields such as href are derived and not stored as part of json
+    EntityReference owner = task.getOwner();
+    List<TagLabel> tags = task.getTags();
+    EntityReference service = task.getService();
+
+    // Don't store owner, database, href and tags as JSON. Build it on the fly based on relationships
+    task.withOwner(null).withService(null).withHref(null).withTags(null);
+
+    if (update) {
+      taskDAO().update(task.getId().toString(), JsonUtils.pojoToJson(task));
+    } else {
+      taskDAO().insert(JsonUtils.pojoToJson(task));
+    }
+
+    // Restore the relationships
+    task.withOwner(owner).withService(service).withTags(tags);
+  }
+
+
   private void applyTags(Task task) throws IOException {
     // Add task level tags by adding tag to task relationship
     EntityUtil.applyTags(tagDAO(), task.getTags(), task.getFullyQualifiedName());
@@ -213,29 +235,13 @@ public abstract class TaskRepository {
   }
 
   private void patch(Task original, Task updated) throws IOException {
-    String taskId = 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.TASK, "id"));
+            .withService(original.getService()).withId(original.getId());
-    }
+    validateRelationships(updated, updated.getService(), updated.getOwner());
-    if (!original.getName().equals(updated.getName())) {
+    TaskUpdater taskUpdater = new TaskUpdater(original, updated, true);
-      throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.TASK, "name"));
+    taskUpdater.updateAll();
-    }
+    taskUpdater.store();
-    if (updated.getService() == null || !original.getService().getId().equals(updated.getService().getId())) {
-      throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.TASK, "service"));
-    }
-    // Validate new owner
-    EntityReference newOwner = EntityUtil.populateOwner(userDAO(), teamDAO(), updated.getOwner());
-
-    EntityReference newService = updated.getService();
-    // Remove previous tags. Merge tags from the update and the existing tags
-    EntityUtil.removeTags(tagDAO(), original.getFullyQualifiedName());
-    updated.setHref(null);
-    updated.setOwner(null);
-    updated.setService(null);
-    taskDAO().update(taskId, JsonUtils.pojoToJson(updated));
-    updateOwner(updated, original.getOwner(), newOwner);
-    updated.setService(newService);
-    applyTags(updated);
   }
 
   public EntityReference getOwner(Task task) throws IOException {
@@ -338,4 +344,81 @@ public abstract class TaskRepository {
     @SqlUpdate("DELETE FROM task_entity WHERE id = :id")
     int delete(@Bind("id") String id);
   }
+
+  static class TaskEntityInterface implements EntityInterface {
+    private final Task task;
+
+    TaskEntityInterface(Task Task) {
+      this.task = Task;
+    }
+
+    @Override
+    public UUID getId() {
+      return task.getId();
+    }
+
+    @Override
+    public String getDescription() {
+      return task.getDescription();
+    }
+
+    @Override
+    public String getDisplayName() {
+      return task.getDisplayName();
+    }
+
+    @Override
+    public EntityReference getOwner() {
+      return task.getOwner();
+    }
+
+    @Override
+    public String getFullyQualifiedName() {
+      return task.getFullyQualifiedName();
+    }
+
+    @Override
+    public List<TagLabel> getTags() {
+      return task.getTags();
+    }
+
+    @Override
+    public void setDescription(String description) {
+      task.setDescription(description);
+    }
+
+    @Override
+    public void setDisplayName(String displayName) {
+      task.setDisplayName(displayName);
+    }
+
+    @Override
+    public void setTags(List<TagLabel> tags) {
+      task.setTags(tags);
+    }
+  }
+
+  /**
+   * Handles entity updated from PUT and POST operation.
+   */
+  public class TaskUpdater extends EntityUpdater {
+    final Task orig;
+    final Task updated;
+
+    public TaskUpdater(Task orig, Task updated, boolean patchOperation) {
+      super(new TaskRepository.TaskEntityInterface(orig), new TaskRepository.TaskEntityInterface(updated), patchOperation, relationshipDAO(),
+              tagDAO());
+      this.orig = orig;
+      this.updated = updated;
+    }
+
+    public void updateAll() throws IOException {
+      super.updateAll();
+    }
+
+    public void store() throws IOException {
+      updated.setVersion(getNewVersion(orig.getVersion()));
+      storeTask(updated, true);
+    }
+  }
 }

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

@@ -39,6 +39,7 @@ import org.openmetadata.catalog.type.TagLabel;
 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;
@@ -63,9 +64,10 @@ 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.LONG_ENTITY_NAME;
 import static org.openmetadata.catalog.util.TestUtils.NON_EXISTENT_ENTITY;
+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,53 +294,49 @@ public class TaskResourceTest extends CatalogApplicationTest {
   public void put_taskUpdateWithNoChange_200(TestInfo test) throws HttpResponseException, URISyntaxException {
     // Create a task with POST
     CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withOwner(USER_OWNER1);
-    createAndCheckTask(request, adminAuthHeaders());
+    Task task = createAndCheckTask(request, adminAuthHeaders());
 
     // Update task two times successfully with PUT requests
-    updateAndCheckTask(request, OK, adminAuthHeaders());
+    task = updateAndCheckTask(task, request, OK, adminAuthHeaders(), NO_CHANGE);
-    updateAndCheckTask(request, OK, adminAuthHeaders());
+    updateAndCheckTask(task, request, OK, adminAuthHeaders(), NO_CHANGE);
   }
 
   @Test
   public void put_taskCreate_200(TestInfo test) throws HttpResponseException, URISyntaxException {
-    // Create a new task with put
+    // Create a new task with PUT
     CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withOwner(USER_OWNER1);
-    updateAndCheckTask(request.withName(test.getDisplayName()).withDescription(null), CREATED, adminAuthHeaders());
+    updateAndCheckTask(null, request, CREATED, adminAuthHeaders(), NO_CHANGE);
   }
 
   @Test
   public void put_taskCreate_as_owner_200(TestInfo test) throws HttpResponseException, URISyntaxException {
     // Create a new task with put
     CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withOwner(USER_OWNER1);
-    // Add Owner as admin
+    // Add task as admin
-    createAndCheckTask(request, adminAuthHeaders());
+    Task task = createAndCheckTask(request, adminAuthHeaders());
-    //Update the task Owner
+    // Update the task Owner and see if it is allowed
-    updateAndCheckTask(request.withName(test.getDisplayName()).withDescription(null),
+    updateAndCheckTask(task, request, OK, authHeaders(USER1.getEmail()), NO_CHANGE);
-            CREATED, authHeaders(USER1.getEmail()));
-
   }
 
   @Test
   public void put_taskNullDescriptionUpdate_200(TestInfo test) throws HttpResponseException, URISyntaxException {
     CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withDescription(null);
-    createAndCheckTask(request, adminAuthHeaders());
+    Task task = createAndCheckTask(request, adminAuthHeaders());
 
     // Update null description with a new description
-    Task task = updateAndCheckTask(request.withDescription("newDescription").withDisplayName("newTask"), OK,
+    task = updateAndCheckTask(task, request.withDescription("newDescription").withDisplayName("newTask"), OK,
-            adminAuthHeaders());
+            adminAuthHeaders(), MINOR_UPDATE);
-    assertEquals("newDescription", task.getDescription());
+    assertEquals("newTask", task.getDisplayName()); // TODO move this to validate
-    assertEquals("newTask", task.getDisplayName());
   }
 
   @Test
   public void put_taskEmptyDescriptionUpdate_200(TestInfo test) throws HttpResponseException, URISyntaxException {
     // Create task with empty description
     CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withDescription("");
-    createAndCheckTask(request, adminAuthHeaders());
+    Task task = createAndCheckTask(request, adminAuthHeaders());
 
     // Update empty description with a new description
-    Task task = updateAndCheckTask(request.withDescription("newDescription"), OK, adminAuthHeaders());
+    updateAndCheckTask(task, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE);
-    assertEquals("newDescription", task.getDescription());
   }
 
   @Test
@@ -354,13 +352,13 @@ public class TaskResourceTest extends CatalogApplicationTest {
   @Test
   public void put_taskUpdateOwner_200(TestInfo test) throws HttpResponseException, URISyntaxException {
     CreateTask request = create(test).withService(AIRFLOW_REFERENCE).withDescription("");
-    createAndCheckTask(request, adminAuthHeaders());
+    Task task = createAndCheckTask(request, adminAuthHeaders());
 
     // Change ownership from USER_OWNER1 to TEAM_OWNER1
-    updateAndCheckTask(request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders());
+    task = updateAndCheckTask(task, request.withOwner(TEAM_OWNER1), OK, adminAuthHeaders(), MINOR_UPDATE);
 
     // Remove ownership
-    Task task = updateAndCheckTask(request.withOwner(null), OK, adminAuthHeaders());
+    task = updateAndCheckTask(task, request.withOwner(null), OK, adminAuthHeaders(), MINOR_UPDATE);
     assertNull(task.getOwner());
   }
 
@@ -402,75 +400,21 @@ public class TaskResourceTest extends CatalogApplicationTest {
     task.getService().setHref(null); // href is readonly and not patchable
 
     // Add description, owner when previously they were null
-    task = patchTaskAttributesAndCheck(task, "description", TEAM_OWNER1, taskTags, adminAuthHeaders());
+    task = patchTaskAttributesAndCheck(task, "description", TEAM_OWNER1, taskTags,
+            adminAuthHeaders(), MINOR_UPDATE);
     task.setOwner(TEAM_OWNER1); // Get rid of href and name returned in the response for owner
     task.setService(AIRFLOW_REFERENCE); // Get rid of href and name returned in the response for service
     taskTags = List.of(USER_ADDRESS_TAG_LABEL, TIER_1);
+
     // Replace description, tier, owner
     task = patchTaskAttributesAndCheck(task, "description1", USER_OWNER1, taskTags,
-            adminAuthHeaders());
+            adminAuthHeaders(), MINOR_UPDATE);
     task.setOwner(USER_OWNER1); // Get rid of href and name returned in the response for owner
     task.setService(AIRFLOW_REFERENCE); // Get rid of href and name returned in the response for service
     taskTags = List.of(TIER_1);
+
     // Remove description, tier, owner
-    patchTaskAttributesAndCheck(task, null, null, taskTags, adminAuthHeaders());
+    patchTaskAttributesAndCheck(task, null, null, taskTags, adminAuthHeaders(), MINOR_UPDATE);
-  }
-
-  @Test
-  public void patch_taskIDChange_400(TestInfo test) throws HttpResponseException,
-          JsonProcessingException, URISyntaxException {
-    // Ensure task ID can't be changed using patch
-    Task task = createTask(create(test), adminAuthHeaders());
-    UUID taskId = task.getId();
-    String taskJson = JsonUtils.pojoToJson(task);
-    task.setId(UUID.randomUUID());
-    HttpResponseException exception = assertThrows(HttpResponseException.class, () ->
-            patchTask(taskId, taskJson, task, adminAuthHeaders()));
-    assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.TASK, "id"));
-
-    // ID can't be deleted
-    task.setId(null);
-    exception = assertThrows(HttpResponseException.class, () ->
-            patchTask(taskId, taskJson, task, adminAuthHeaders()));
-    assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.TASK, "id"));
-  }
-
-  @Test
-  public void patch_taskNameChange_400(TestInfo test) throws HttpResponseException,
-          JsonProcessingException, URISyntaxException {
-    // Ensure task name can't be changed using patch
-    Task task = createTask(create(test), adminAuthHeaders());
-    String taskJson = JsonUtils.pojoToJson(task);
-    task.setName("newName");
-    HttpResponseException exception = assertThrows(HttpResponseException.class, () ->
-            patchTask(taskJson, task, adminAuthHeaders()));
-    assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.TASK, "name"));
-
-    // Name can't be removed
-    task.setName(null);
-    exception = assertThrows(HttpResponseException.class, () ->
-            patchTask(taskJson, task, adminAuthHeaders()));
-    assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.TASK, "name"));
-  }
-
-  @Test
-  public void patch_taskRemoveService_400(TestInfo test) throws HttpResponseException,
-          JsonProcessingException, URISyntaxException {
-    // Ensure service corresponding to task can't be changed by patch operation
-    Task task = createTask(create(test), adminAuthHeaders());
-    task.getService().setHref(null); // Remove href from returned response as it is read-only field
-
-    String taskJson = JsonUtils.pojoToJson(task);
-    task.setService(PREFECT_REFERENCE);
-    HttpResponseException exception = assertThrows(HttpResponseException.class, () ->
-            patchTask(taskJson, task, adminAuthHeaders()));
-    assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.TASK, "service"));
-
-    // Service relationship can't be removed
-    task.setService(null);
-    exception = assertThrows(HttpResponseException.class, () ->
-            patchTask(taskJson, task, adminAuthHeaders()));
-    assertResponse(exception, BAD_REQUEST, readOnlyAttribute(Entity.TASK, "service"));
   }
 
   @Test
@@ -496,13 +440,18 @@ public class TaskResourceTest extends CatalogApplicationTest {
     return getAndValidate(task.getId(), create, authHeaders, updatedBy);
   }
 
-  public static Task updateAndCheckTask(CreateTask create,
+  public static Task updateAndCheckTask(Task before, CreateTask create, Status status,
-                                        Status status,
+                                        Map<String, String> authHeaders, UpdateType updateType)
-                                        Map<String, String> authHeaders) throws HttpResponseException {
+          throws HttpResponseException {
     String updatedBy = TestUtils.getPrincipal(authHeaders);
     Task updatedTask = updateTask(create, status, authHeaders);
     validateTask(updatedTask, create.getDescription(), create.getOwner(), create.getService(), create.getTags(),
             updatedBy);
+    if (before == null) {
+      assertEquals(0.1, updatedTask.getVersion()); // First version created
+    } else {
+      TestUtils.validateUpdate(before.getVersion(), updatedTask.getVersion(), updateType);
+    }
 
     // GET the newly updated task and validate
     return getAndValidate(updatedTask.getId(), create, authHeaders, updatedBy);
@@ -599,26 +548,25 @@ public class TaskResourceTest extends CatalogApplicationTest {
     return task;
   }
 
-  private Task patchTaskAttributesAndCheck(Task task, String newDescription,
+  private Task patchTaskAttributesAndCheck(Task before, String newDescription, EntityReference newOwner,
-                                             EntityReference newOwner,
+                                           List<TagLabel> tags, Map<String, String> authHeaders, UpdateType updateType)
-                                             List<TagLabel> tags,
-                                             Map<String, String> authHeaders)
           throws JsonProcessingException, HttpResponseException {
     String updatedBy = TestUtils.getPrincipal(authHeaders);
-    String taskJson = JsonUtils.pojoToJson(task);
+    String taskJson = JsonUtils.pojoToJson(before);
 
     // Update the task attributes
-    task.setDescription(newDescription);
+    before.setDescription(newDescription);
-    task.setOwner(newOwner);
+    before.setOwner(newOwner);
-    task.setTags(tags);
+    before.setTags(tags);
 
     // Validate information returned in patch response has the updates
-    Task updateTask = patchTask(taskJson, task, authHeaders);
+    Task updateTask = patchTask(taskJson, before, authHeaders);
-    validateTask(updateTask, task.getDescription(), newOwner, null, tags, updatedBy);
+    validateTask(updateTask, before.getDescription(), newOwner, null, tags, updatedBy);
+    TestUtils.validateUpdate(before.getVersion(), updateTask.getVersion(), updateType);
 
     // GET the task and Validate information returned
-    Task getTask = getTask(task.getId(), "service,owner,tags", authHeaders);
+    Task getTask = getTask(before.getId(), "service,owner,tags", authHeaders);
-    validateTask(getTask, task.getDescription(), newOwner, null, tags, updatedBy);
+    validateTask(getTask, before.getDescription(), newOwner, null, tags, updatedBy);
     return updateTask;
   }