diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TaskRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TaskRepository.java index a0ccaec027d..0e2a5e38cb1 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TaskRepository.java +++ b/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 createOrUpdate(Task updatedTask, EntityReference service, EntityReference newOwner) + public PutResponse createOrUpdate(Task updated, EntityReference service, EntityReference newOwner) throws IOException { getService(service); // Validate service - String fqn = getFQN(service, updatedTask); - Task storedDB = JsonUtils.readValue(taskDAO().findByFQN(fqn), Task.class); - if (storedDB == null) { // Task does not exist. Create a new one - return new PutResponse<>(Status.CREATED, createInternal(updatedTask, 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()); + String fqn = getFQN(service, updated); + Task stored = JsonUtils.readValue(taskDAO().findByFQN(fqn), Task.class); + if (stored == null) { // Task does not exist. Create a new one + return new PutResponse<>(Status.CREATED, createInternal(updated, service, newOwner)); } + setFields(stored, TASK_UPDATE_FIELDS); + updated.setId(stored.getId()); + validateRelationships(updated, service, newOwner); - //update the display name from source - if (updatedTask.getDisplayName() != null && !updatedTask.getDisplayName().isEmpty()) { - storedDB.withDisplayName(updatedTask.getDisplayName()); - } - 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); + TaskUpdater taskUpdater = new TaskUpdater(stored, updated, false); + taskUpdater.updateAll(); + taskUpdater.store(); + return new PutResponse<>(Status.OK, updated); } @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)); - EntityUtil.populateOwner(userDAO(), teamDAO(), owner); // Validate owner - - // Query 1 - insert task into task_entity table - taskDAO().insert(JsonUtils.pojoToJson(task)); - setService(task, service); - setOwner(task, owner); - applyTags(task); + validateRelationships(task, service, owner); + storeTask(task, false); + addRelationships(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 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(); - if (!original.getId().equals(updated.getId())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.TASK, "id")); - } - if (!original.getName().equals(updated.getName())) { - throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(Entity.TASK, "name")); - } - 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); + // 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()); + TaskUpdater taskUpdater = new TaskUpdater(original, updated, true); + taskUpdater.updateAll(); + taskUpdater.store(); } 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 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 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); + } + } } 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 061078746f9..dacd8927cb7 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 @@ -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()); - updateAndCheckTask(request, OK, adminAuthHeaders()); + task = updateAndCheckTask(task, request, OK, adminAuthHeaders(), NO_CHANGE); + 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 - createAndCheckTask(request, adminAuthHeaders()); - //Update the task Owner - updateAndCheckTask(request.withName(test.getDisplayName()).withDescription(null), - CREATED, authHeaders(USER1.getEmail())); - + // 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); } @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, - adminAuthHeaders()); - assertEquals("newDescription", task.getDescription()); - assertEquals("newTask", task.getDisplayName()); + task = updateAndCheckTask(task, request.withDescription("newDescription").withDisplayName("newTask"), OK, + adminAuthHeaders(), MINOR_UPDATE); + assertEquals("newTask", task.getDisplayName()); // TODO move this to validate } @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()); - assertEquals("newDescription", task.getDescription()); + updateAndCheckTask(task, request.withDescription("newDescription"), OK, adminAuthHeaders(), MINOR_UPDATE); } @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()); - } - - @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")); + patchTaskAttributesAndCheck(task, null, null, taskTags, adminAuthHeaders(), MINOR_UPDATE); } @Test @@ -496,13 +440,18 @@ public class TaskResourceTest extends CatalogApplicationTest { return getAndValidate(task.getId(), create, authHeaders, updatedBy); } - public static Task updateAndCheckTask(CreateTask create, - Status status, - Map authHeaders) throws HttpResponseException { + public static Task updateAndCheckTask(Task before, CreateTask create, Status status, + Map authHeaders, UpdateType updateType) + 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, - EntityReference newOwner, - List tags, - Map authHeaders) + private Task patchTaskAttributesAndCheck(Task before, String newDescription, EntityReference newOwner, + List tags, Map authHeaders, UpdateType updateType) 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); - task.setOwner(newOwner); - task.setTags(tags); + before.setDescription(newDescription); + before.setOwner(newOwner); + before.setTags(tags); // Validate information returned in patch response has the updates - Task updateTask = patchTask(taskJson, task, authHeaders); - validateTask(updateTask, task.getDescription(), newOwner, null, tags, updatedBy); + Task updateTask = patchTask(taskJson, before, authHeaders); + 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); - validateTask(getTask, task.getDescription(), newOwner, null, tags, updatedBy); + Task getTask = getTask(before.getId(), "service,owner,tags", authHeaders); + validateTask(getTask, before.getDescription(), newOwner, null, tags, updatedBy); return updateTask; }