From f45d82484dd68e58f555c132355ff4fa8a22e6df Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Tue, 19 Sep 2023 18:30:20 -0700 Subject: [PATCH] Fixes #3090 Glossary Term approval workflow (#13269) --- .../openmetadata/common/utils/CommonUtil.java | 4 +- .../exception/CatalogExceptionMessage.java | 4 + .../jdbi3/ClassificationRepository.java | 8 +- .../service/jdbi3/CollectionDAO.java | 14 ++ .../service/jdbi3/EntityRepository.java | 7 +- .../service/jdbi3/FeedRepository.java | 33 +++- .../service/jdbi3/GlossaryTermRepository.java | 145 +++++++++++++++++- .../service/jdbi3/TestCaseRepository.java | 2 +- .../service/security/DefaultAuthorizer.java | 15 ++ .../policyevaluator/ResourceContext.java | 3 + .../openmetadata/service/util/EntityUtil.java | 4 + .../glossary/GlossaryResourceTest.java | 13 +- .../glossary/GlossaryTermResourceTest.java | 136 ++++++++++++++-- .../json/schema/entity/data/glossaryTerm.json | 3 +- .../json/schema/entity/feed/thread.json | 4 + 15 files changed, 354 insertions(+), 41 deletions(-) diff --git a/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java b/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java index 25121a84e28..fbdf41fc7c7 100644 --- a/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java +++ b/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java @@ -72,7 +72,7 @@ public final class CommonUtil { String fileName = e.nextElement().getName(); if (pattern.matcher(fileName).matches()) { retval.add(fileName); - LOG.info("Adding file from jar {}", fileName); + LOG.debug("Adding file from jar {}", fileName); } } } catch (Exception ignored) { @@ -90,7 +90,7 @@ public final class CommonUtil { .map( path -> { String relativePath = root.relativize(path).toString(); - LOG.info("Adding directory file {}", relativePath); + LOG.debug("Adding directory file {}", relativePath); return relativePath; }) .collect(Collectors.toSet()); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java b/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java index c55758d9dd0..2bc18598bf4 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java @@ -136,6 +136,10 @@ public final class CatalogExceptionMessage { return String.format("Principal: CatalogPrincipal{name='%s'} is not admin", name); } + public static String notReviewer(String name) { + return String.format("User '%s' is not a reviewer", name); + } + public static String permissionDenied( String user, MetadataOperation operation, String roleName, String policyName, String ruleName) { if (roleName != null) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ClassificationRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ClassificationRepository.java index ab94b97f59c..43a74b5b81f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ClassificationRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ClassificationRepository.java @@ -107,13 +107,13 @@ public class ClassificationRepository extends EntityRepository { @Override @SuppressWarnings("unused") - public void postUpdate(Classification entity) { + public void postUpdate(Classification original, Classification updated) { String scriptTxt = "for (k in params.keySet()) { ctx._source.put(k, params.get(k)) }"; - if (entity.getDisabled() != null) { - scriptTxt = "ctx._source.disabled=" + entity.getDisabled(); + if (updated.getDisabled() != null) { + scriptTxt = "ctx._source.disabled=" + updated.getDisabled(); } searchClient.updateSearchEntityUpdated( - JsonUtils.deepCopy(entity, Classification.class), scriptTxt, "classification.fullyQualifiedName"); + JsonUtils.deepCopy(updated, Classification.class), scriptTxt, "classification.fullyQualifiedName"); } @Override diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java index b67b960619f..3283ca0cec3 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java @@ -1276,6 +1276,13 @@ public interface CollectionDAO { @Bind("toType") String toType, @Bind("relation") int relation); + @SqlQuery( + "SELECT fromFQN, fromType, json FROM field_relationship WHERE " + + "toFQNHash = :toFQNHash AND toType = :toType AND relation = :relation") + @RegisterRowMapper(FromFieldMapper.class) + List> findFrom( + @BindFQN("toFQNHash") String toFQNHash, @Bind("toType") String toType, @Bind("relation") int relation); + @SqlQuery( "SELECT fromFQN, toFQN, json FROM field_relationship WHERE " + "fromFQNHash LIKE CONCAT(:fqnPrefixHash, '%') AND fromType = :fromType AND toType = :toType " @@ -1349,6 +1356,13 @@ public interface CollectionDAO { @Bind("toType") String toType, @Bind("relation") int relation); + class FromFieldMapper implements RowMapper> { + @Override + public Triple map(ResultSet rs, StatementContext ctx) throws SQLException { + return Triple.of(rs.getString("fromFQN"), rs.getString("fromType"), rs.getString("json")); + } + } + class ToFieldMapper implements RowMapper> { @Override public Triple map(ResultSet rs, StatementContext ctx) throws SQLException { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java index 4bc152a3d30..7e3b2dac53f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java @@ -701,10 +701,10 @@ public abstract class EntityRepository { } @SuppressWarnings("unused") - public void postUpdate(T entity) { + protected void postUpdate(T original, T updated) { if (supportsSearchIndex) { String scriptTxt = "for (k in params.keySet()) { ctx._source.put(k, params.get(k)) }"; - searchClient.updateSearchEntityUpdated(JsonUtils.deepCopy(entity, entityClass), scriptTxt, ""); + searchClient.updateSearchEntityUpdated(JsonUtils.deepCopy(updated, entityClass), scriptTxt, ""); } } @@ -775,7 +775,7 @@ public abstract class EntityRepository { .withCurrentVersion(entity.getVersion()) .withPreviousVersion(change.getPreviousVersion()); entity.setChangeDescription(change); - postUpdate(entity); + postUpdate(entity, entity); return new PutResponse<>(Status.OK, changeEvent, RestUtil.ENTITY_FIELDS_CHANGED); } @@ -1754,6 +1754,7 @@ public abstract class EntityRepository { // Store the updated entity storeUpdate(); + postUpdate(original, updated); } public void entitySpecificUpdate() { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java index 653856d3e4e..897ee7809e3 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java @@ -49,6 +49,7 @@ import lombok.Getter; import lombok.Setter; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.tuple.Triple; import org.json.JSONObject; import org.openmetadata.schema.EntityInterface; import org.openmetadata.schema.api.feed.CloseTask; @@ -127,11 +128,11 @@ public class FeedRepository { return dao.feedDAO().getTaskId(); } - public class ThreadContext { + public static class ThreadContext { @Getter protected final Thread thread; - @Getter @Setter protected final EntityLink about; + @Getter protected final EntityLink about; @Getter @Setter protected EntityInterface aboutEntity; - @Getter private EntityReference createdBy; + @Getter private final EntityReference createdBy; ThreadContext(Thread thread) { this.thread = thread; @@ -251,6 +252,23 @@ public class FeedRepository { storeMentions(thread, thread.getMessage()); } + public Thread getTask(EntityLink about, TaskType taskType) { + List> tasks = + dao.fieldRelationshipDAO() + .findFrom(about.getFullyQualifiedFieldValue(), about.getFullyQualifiedFieldType(), IS_ABOUT.ordinal()); + for (Triple task : tasks) { + if (task.getMiddle().equals(Entity.THREAD)) { + String threadId = task.getLeft(); + Thread thread = EntityUtil.validate(threadId, dao.feedDAO().findById(threadId), Thread.class); + if (thread.getTask() != null && thread.getTask().getType() == taskType) { + return thread; + } + } + } + throw new EntityNotFoundException( + String.format("Task for entity %s of type %s was not found", about.getEntityType(), taskType)); + } + private Thread createThread(ThreadContext threadContext) { Thread thread = threadContext.getThread(); if (thread.getType() == ThreadType.Task) { @@ -279,8 +297,7 @@ public class FeedRepository { public PatchResponse closeTask(UriInfo uriInfo, Thread thread, String user, CloseTask closeTask) { // Update the attributes - ThreadContext threadContext = getThreadContext(thread); - closeTask(threadContext, user, closeTask); + closeTask(thread, user, closeTask); Thread updatedHref = FeedResource.addHref(uriInfo, thread); return new PatchResponse<>(Status.OK, updatedHref, RestUtil.ENTITY_UPDATED); } @@ -305,7 +322,7 @@ public class FeedRepository { // Update the attributes threadContext.getThread().getTask().withNewValue(resolveTask.getNewValue()); - closeTask(threadContext, user, new CloseTask()); + closeTask(threadContext.getThread(), user, new CloseTask()); } private static String getTagFQNs(List tags) { @@ -340,8 +357,8 @@ public class FeedRepository { addPostToThread(thread.getId().toString(), post, user); } - private void closeTask(ThreadContext threadContext, String user, CloseTask closeTask) { - Thread thread = threadContext.getThread(); + public void closeTask(Thread thread, String user, CloseTask closeTask) { + ThreadContext threadContext = getThreadContext(thread); TaskDetails task = thread.getTask(); if (task.getStatus() != Open) { return; diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java index 95848bb6ceb..344a0cf517e 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java @@ -17,11 +17,13 @@ package org.openmetadata.service.jdbi3; import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; import static org.openmetadata.schema.type.Include.ALL; import static org.openmetadata.service.Entity.FIELD_REVIEWERS; import static org.openmetadata.service.Entity.GLOSSARY; import static org.openmetadata.service.Entity.GLOSSARY_TERM; import static org.openmetadata.service.exception.CatalogExceptionMessage.invalidGlossaryTermMove; +import static org.openmetadata.service.exception.CatalogExceptionMessage.notReviewer; import static org.openmetadata.service.resources.EntityResource.searchClient; import static org.openmetadata.service.util.EntityUtil.entityReferenceMatch; import static org.openmetadata.service.util.EntityUtil.getId; @@ -32,22 +34,36 @@ import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.UUID; +import javax.json.JsonPatch; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.tuple.ImmutablePair; import org.openmetadata.schema.EntityInterface; import org.openmetadata.schema.api.data.TermReference; +import org.openmetadata.schema.api.feed.CloseTask; +import org.openmetadata.schema.api.feed.ResolveTask; import org.openmetadata.schema.entity.data.Glossary; import org.openmetadata.schema.entity.data.GlossaryTerm; +import org.openmetadata.schema.entity.data.GlossaryTerm.Status; +import org.openmetadata.schema.entity.feed.Thread; import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.Include; import org.openmetadata.schema.type.ProviderType; import org.openmetadata.schema.type.Relationship; import org.openmetadata.schema.type.TagLabel; import org.openmetadata.schema.type.TagLabel.TagSource; +import org.openmetadata.schema.type.TaskDetails; +import org.openmetadata.schema.type.TaskStatus; +import org.openmetadata.schema.type.TaskType; +import org.openmetadata.schema.type.ThreadType; import org.openmetadata.service.Entity; import org.openmetadata.service.exception.CatalogExceptionMessage; import org.openmetadata.service.jdbi3.CollectionDAO.EntityRelationshipRecord; +import org.openmetadata.service.jdbi3.FeedRepository.TaskWorkflow; +import org.openmetadata.service.jdbi3.FeedRepository.ThreadContext; +import org.openmetadata.service.resources.feeds.FeedResource; +import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; import org.openmetadata.service.resources.glossary.GlossaryTermResource; +import org.openmetadata.service.security.AuthorizationException; import org.openmetadata.service.util.EntityUtil; import org.openmetadata.service.util.EntityUtil.Fields; import org.openmetadata.service.util.FullyQualifiedName; @@ -108,18 +124,21 @@ public class GlossaryTermRepository extends EntityRepository { @Override public void prepare(GlossaryTerm entity, boolean update) { + List parentReviewers = null; // Validate parent term GlossaryTerm parentTerm = entity.getParent() != null - ? getByName(null, entity.getParent().getFullyQualifiedName(), getFields("owner")) + ? Entity.getEntity(entity.getParent(), "owner,reviewers", Include.NON_DELETED) : null; if (parentTerm != null) { + parentReviewers = parentTerm.getReviewers(); entity.setParent(parentTerm.getEntityReference()); } // Validate glossary - Glossary glossary = Entity.getEntity(entity.getGlossary(), "", Include.NON_DELETED); + Glossary glossary = Entity.getEntity(entity.getGlossary(), "reviewers", Include.NON_DELETED); entity.setGlossary(glossary.getEntityReference()); + parentReviewers = parentReviewers != null ? parentReviewers : glossary.getReviewers(); validateHierarchy(entity); @@ -128,6 +147,11 @@ public class GlossaryTermRepository extends EntityRepository { // Validate reviewers EntityUtil.populateEntityReferences(entity.getReviewers()); + + if (!update || entity.getStatus() == null) { + // If parentTerm or glossary has reviewers set, the glossary term can only be created in `Draft` mode + entity.setStatus(!nullOrEmpty(parentReviewers) ? Status.DRAFT : Status.APPROVED); + } } @Override @@ -191,6 +215,32 @@ public class GlossaryTermRepository extends EntityRepository { return new GlossaryTermUpdater(original, updated, operation); } + protected void postCreate(GlossaryTerm entity) { + if (entity.getStatus() == Status.DRAFT) { + // Create an approval task for glossary term in draft mode + createApprovalTask(entity, entity.getReviewers()); + } + } + + @Override + public void postUpdate(GlossaryTerm original, GlossaryTerm updated) { + if (original.getStatus() == Status.DRAFT) { + if (updated.getStatus() == Status.APPROVED) { + closeApprovalTask(updated, "Approved the glossary term"); + } else if (updated.getStatus() == Status.REJECTED) { + closeApprovalTask(updated, "Rejected the glossary term"); + } + } + } + + @Override + protected void preDelete(GlossaryTerm entity, String deletedBy) { + // A glossary term in `Draft` state can only be deleted by the reviewers + if (Status.DRAFT.equals(entity.getStatus())) { + checkUpdatedByReviewer(entity, deletedBy); + } + } + @Override protected void postDelete(GlossaryTerm entity) { // Cleanup all the tag labels using this glossary term @@ -212,6 +262,42 @@ public class GlossaryTermRepository extends EntityRepository { } } + public TaskWorkflow getTaskWorkflow(ThreadContext threadContext) { + validateTaskThread(threadContext); + TaskType taskType = threadContext.getThread().getTask().getType(); + if (EntityUtil.isApprovalTask(taskType)) { + return new ApprovalTaskWorkflow(threadContext); + } + return super.getTaskWorkflow(threadContext); + } + + public static class ApprovalTaskWorkflow extends TaskWorkflow { + ApprovalTaskWorkflow(ThreadContext threadContext) { + super(threadContext); + } + + @Override + public EntityInterface performTask(String user, ResolveTask resolveTask) { + GlossaryTerm glossaryTerm = (GlossaryTerm) threadContext.getAboutEntity(); + glossaryTerm.setStatus(Status.APPROVED); + return glossaryTerm; + } + + @Override + protected void closeTask(String user, CloseTask closeTask) { + // Closing task results in glossary term going from `Draft` to `Rejected` + GlossaryTerm term = (GlossaryTerm) threadContext.getAboutEntity(); + if (term.getStatus() == Status.DRAFT) { + String origJson = JsonUtils.pojoToJson(term); + term.setStatus(Status.REJECTED); + String updatedJson = JsonUtils.pojoToJson(term); + JsonPatch patch = JsonUtils.getJsonPatch(origJson, updatedJson); + EntityRepository repository = threadContext.getEntityRepository(); + repository.patch(null, term.getId(), user, patch); + } + } + } + @Override public void restoreFromSearch(GlossaryTerm entity) { if (supportsSearchIndex) { @@ -245,6 +331,52 @@ public class GlossaryTermRepository extends EntityRepository { } } + private void checkUpdatedByReviewer(GlossaryTerm term, String updatedBy) { + // Only list of allowed reviewers can change the status from DRAFT to APPROVED + List reviewers = term.getReviewers(); + if (!nullOrEmpty(reviewers)) { + // Updating user must be one of the reviewers + boolean isReviewer = + reviewers.stream() + .anyMatch(e -> e.getName().equals(updatedBy) || e.getFullyQualifiedName().equals(updatedBy)); + if (!isReviewer) { + throw new AuthorizationException(notReviewer(updatedBy)); + } + } + } + + private void createApprovalTask(GlossaryTerm entity, List parentReviewers) { + TaskDetails taskDetails = + new TaskDetails() + .withAssignees(FeedResource.formatAssignees(parentReviewers)) + .withType(TaskType.RequestApproval) + .withStatus(TaskStatus.Open); + + EntityLink about = new EntityLink(entityType, entity.getFullyQualifiedName()); + Thread thread = + new Thread() + .withId(UUID.randomUUID()) + .withThreadTs(System.currentTimeMillis()) + .withMessage("Approval required for ") // TODO fix this + .withCreatedBy(entity.getUpdatedBy()) + .withAbout(about.getLinkString()) + .withType(ThreadType.Task) + .withTask(taskDetails) + .withUpdatedBy(entity.getUpdatedBy()) + .withUpdatedAt(System.currentTimeMillis()); + FeedRepository feedRepository = Entity.getFeedRepository(); + feedRepository.create(thread); + } + + private void closeApprovalTask(GlossaryTerm entity, String comment) { + EntityLink about = new EntityLink(GLOSSARY_TERM, entity.getFullyQualifiedName()); + FeedRepository feedRepository = Entity.getFeedRepository(); + Thread taskThread = feedRepository.getTask(about, TaskType.RequestApproval); + if (TaskStatus.Open.equals(taskThread.getTask().getStatus())) { + feedRepository.closeTask(taskThread, entity.getUpdatedBy(), new CloseTask().withComment(comment)); + } + } + /** Handles entity updated from PUT and POST operation. */ public class GlossaryTermUpdater extends EntityUpdater { public GlossaryTermUpdater(GlossaryTerm original, GlossaryTerm updated, Operation operation) { @@ -273,7 +405,14 @@ public class GlossaryTermRepository extends EntityRepository { } private void updateStatus(GlossaryTerm origTerm, GlossaryTerm updatedTerm) { - // TODO Only list of allowed reviewers can change the status from DRAFT to APPROVED + if (origTerm.getStatus() == updatedTerm.getStatus()) { + return; + } + // Only reviewers can change from DRAFT status to APPROVED/REJECTED status + if (origTerm.getStatus() == Status.DRAFT + && (updatedTerm.getStatus() == Status.APPROVED || updatedTerm.getStatus() == Status.REJECTED)) { + checkUpdatedByReviewer(origTerm, updatedTerm.getUpdatedBy()); + } recordChange("status", origTerm.getStatus(), updatedTerm.getStatus()); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java index 701b303d160..22facc98018 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java @@ -418,7 +418,7 @@ public class TestCaseRepository extends EntityRepository { List resultSummaries = listOrEmpty(testSuite.getTestCaseResultSummary()); for (UUID testCaseId : testCaseIds) { TestCase testCase = Entity.getEntity(Entity.TEST_CASE, testCaseId, "*", Include.ALL); - postUpdate(testCase); + postUpdate(testCase, testCase); // Get the latest result to set the testSuite summary field String result = daoCollection diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/DefaultAuthorizer.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/DefaultAuthorizer.java index 16cc1067d10..acc840767dd 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/DefaultAuthorizer.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/DefaultAuthorizer.java @@ -13,6 +13,7 @@ package org.openmetadata.service.security; +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; import static org.openmetadata.schema.type.Permission.Access.ALLOW; import static org.openmetadata.service.exception.CatalogExceptionMessage.notAdmin; @@ -71,6 +72,9 @@ public class DefaultAuthorizer implements Authorizer { if (subjectContext.isAdmin()) { return; } + if (isReviewer(resourceContext, subjectContext)) { + return; // Reviewer of a resource gets admin level privilege on the resource + } PolicyEvaluator.hasPermission(subjectContext, resourceContext, operationContext); } @@ -123,4 +127,15 @@ public class DefaultAuthorizer implements Authorizer { } return loggedInUser; } + + private boolean isReviewer(ResourceContextInterface resourceContext, SubjectContext subjectContext) { + if (resourceContext.getEntity() == null) { + return false; + } + String updatedBy = subjectContext.getUser().getName(); + List reviewers = resourceContext.getEntity().getReviewers(); + return !nullOrEmpty(reviewers) + ? reviewers.stream().anyMatch(e -> e.getName().equals(updatedBy) || e.getFullyQualifiedName().equals(updatedBy)) + : false; + } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java index 6517232b1f6..5edb9d3aa3a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java @@ -86,6 +86,9 @@ public class ResourceContext implements ResourceConte if (entityRepository.isSupportsDomain()) { fields = EntityUtil.addField(fields, Entity.FIELD_DOMAIN); } + if (entityRepository.isSupportsReviewers()) { + fields = EntityUtil.addField(fields, Entity.FIELD_REVIEWERS); + } Fields fieldList = entityRepository.getFields(fields); try { if (id != null) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityUtil.java index d6e53e131bf..b9b1b17fd18 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityUtil.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityUtil.java @@ -545,6 +545,10 @@ public final class EntityUtil { return taskType == TaskType.RequestTag || taskType == TaskType.UpdateTag; } + public static boolean isApprovalTask(TaskType taskType) { + return taskType == TaskType.RequestApproval; + } + public static Column findColumn(List columns, String columnName) { return columns.stream() .filter(c -> c.getName().equals(columnName)) diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java index b310d98929a..2de47b70953 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java @@ -371,7 +371,7 @@ public class GlossaryResourceTest extends EntityResourceTest updateRecords = @@ -392,12 +392,11 @@ public class GlossaryResourceTest extends EntityResourceTest newRecords = listOf(",g3,dsp0,dsc0,h1;h2;h3,,term0;http://term0,Tier.Tier3,,,Draft"); + List newRecords = listOf(",g3,dsp0,dsc0,h1;h2;h3,,term0;http://term0,Tier.Tier3,,,Approved"); testImportExport(glossary.getName(), GlossaryCsv.HEADERS, createRecords, updateRecords, newRecords); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java index 7022419b378..a1078ed67d1 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java @@ -17,6 +17,7 @@ package org.openmetadata.service.resources.glossary; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; +import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static org.junit.jupiter.api.Assertions.*; import static org.openmetadata.common.utils.CommonUtil.listOf; import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; @@ -26,7 +27,9 @@ import static org.openmetadata.service.Entity.GLOSSARY; import static org.openmetadata.service.Entity.GLOSSARY_TERM; import static org.openmetadata.service.exception.CatalogExceptionMessage.entityIsNotEmpty; import static org.openmetadata.service.exception.CatalogExceptionMessage.glossaryTermMismatch; +import static org.openmetadata.service.exception.CatalogExceptionMessage.notReviewer; import static org.openmetadata.service.resources.databases.TableResourceTest.getColumn; +import static org.openmetadata.service.security.SecurityUtil.authHeaders; import static org.openmetadata.service.util.EntityUtil.fieldAdded; import static org.openmetadata.service.util.EntityUtil.fieldDeleted; import static org.openmetadata.service.util.EntityUtil.fieldUpdated; @@ -37,6 +40,11 @@ import static org.openmetadata.service.util.EntityUtil.toTagLabels; import static org.openmetadata.service.util.TestUtils.*; import static org.openmetadata.service.util.TestUtils.UpdateType.MINOR_UPDATE; import static org.openmetadata.service.util.TestUtils.UpdateType.NO_CHANGE; +import static org.openmetadata.service.util.TestUtils.assertEntityReferenceNames; +import static org.openmetadata.service.util.TestUtils.assertListNotEmpty; +import static org.openmetadata.service.util.TestUtils.assertListNotNull; +import static org.openmetadata.service.util.TestUtils.assertListNull; +import static org.openmetadata.service.util.TestUtils.assertResponse; import java.io.IOException; import java.net.URI; @@ -57,18 +65,25 @@ import org.openmetadata.schema.api.data.CreateGlossary; import org.openmetadata.schema.api.data.CreateGlossaryTerm; import org.openmetadata.schema.api.data.CreateTable; import org.openmetadata.schema.api.data.TermReference; +import org.openmetadata.schema.api.feed.ResolveTask; import org.openmetadata.schema.entity.data.Glossary; import org.openmetadata.schema.entity.data.GlossaryTerm; import org.openmetadata.schema.entity.data.GlossaryTerm.Status; import org.openmetadata.schema.entity.data.Table; +import org.openmetadata.schema.entity.feed.Thread; import org.openmetadata.schema.entity.type.Style; import org.openmetadata.schema.type.ChangeDescription; import org.openmetadata.schema.type.Column; import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.TagLabel; +import org.openmetadata.schema.type.TaskDetails; +import org.openmetadata.schema.type.TaskStatus; import org.openmetadata.service.Entity; import org.openmetadata.service.resources.EntityResourceTest; import org.openmetadata.service.resources.databases.TableResourceTest; +import org.openmetadata.service.resources.feeds.FeedResource.ThreadList; +import org.openmetadata.service.resources.feeds.FeedResourceTest; +import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; import org.openmetadata.service.util.EntityUtil; import org.openmetadata.service.util.FullyQualifiedName; import org.openmetadata.service.util.JsonUtils; @@ -79,6 +94,7 @@ import org.openmetadata.service.util.TestUtils.UpdateType; @TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class GlossaryTermResourceTest extends EntityResourceTest { private final GlossaryResourceTest glossaryTest = new GlossaryResourceTest(); + private final FeedResourceTest taskTest = new FeedResourceTest(); public GlossaryTermResourceTest() { super( @@ -254,16 +270,116 @@ public class GlossaryTermResourceTest extends EntityResourceTest patchEntity(g2t1.getId(), json, g2t1, ADMIN_AUTH_HEADERS), FORBIDDEN, notReviewer("admin")); + + // A reviewer can change the `Draft` to `Approved` status using PATCH or PUT + GlossaryTerm g2t1Updated = patchEntity(g2t1.getId(), json, g2t1, authHeaders(USER1.getName())); + assertEquals(Status.APPROVED, g2t1Updated.getStatus()); + assertApprovalTask(g2t1, TaskStatus.Closed); // The Request Approval task is closed + + // + // Glossary terms g2t2 created is in `Draft` status. Automatically a Request Approval task is created. + // Only a reviewer can resolve the task. Resolving the task changes g2t1 status from `Draft` to `Approved`. + // + GlossaryTerm g2t2 = createTerm(glossary2, null, "g2t2"); + assertEquals(Status.DRAFT, g2t2.getStatus()); + Thread approvalTask = assertApprovalTask(g2t2, TaskStatus.Open); // A Request Approval task is opened + int taskId = approvalTask.getTask().getId(); + + // Even admin can't resolve the task + ResolveTask resolveTask = new ResolveTask().withNewValue(Status.APPROVED.value()); + assertResponse( + () -> taskTest.resolveTask(taskId, resolveTask, ADMIN_AUTH_HEADERS), FORBIDDEN, notReviewer("admin")); + + // Reviewer resolves the task. Glossary is approved. And task is resolved. + taskTest.resolveTask(taskId, resolveTask, authHeaders(USER1.getName())); + assertApprovalTask(g2t2, TaskStatus.Closed); // A Request Approval task is opened + g2t2 = getEntity(g2t2.getId(), authHeaders(USER1.getName())); + assertEquals(Status.APPROVED, g2t2.getStatus()); + + // + // Glossary terms g2t3 created is in `Draft` status. Automatically a Request Approval task is created. + // Only a reviewer can close the task. Closing the task moves g2t1 from `Draft` to `Rejected` state. + // + GlossaryTerm g2t3 = createTerm(glossary2, null, "g2t3"); + assertEquals(Status.DRAFT, g2t3.getStatus()); + approvalTask = assertApprovalTask(g2t3, TaskStatus.Open); // A Request Approval task is opened + int taskId2 = approvalTask.getTask().getId(); + + // Even admin can't close the task + assertResponse(() -> taskTest.closeTask(taskId2, "comment", ADMIN_AUTH_HEADERS), FORBIDDEN, notReviewer("admin")); + + // Reviewer closes the task. Glossary term is rejected. And task is resolved. + taskTest.closeTask(taskId2, "Rejected", authHeaders(USER1.getName())); + assertApprovalTask(g2t3, TaskStatus.Closed); // A Request Approval task is opened + g2t3 = getEntity(g2t3.getId(), authHeaders(USER1.getName())); + assertEquals(Status.REJECTED, g2t3.getStatus()); + + // + // Glossary terms g2t4 created is in `Draft` status. Automatically a Request Approval task is created. + // Only a reviewer changes the status to `Rejected`. This automatically closes Request Approval task. + // + final GlossaryTerm g2t4 = createTerm(glossary2, null, "g2t4"); + assertEquals(Status.DRAFT, g2t4.getStatus()); + assertApprovalTask(g2t4, TaskStatus.Open); // A Request Approval task is opened + + // Non reviewer - even Admin - can't change the `Draft` to `Approved` status using PATCH + String json2 = JsonUtils.pojoToJson(g2t4); + g2t4.setStatus(Status.REJECTED); + assertResponse(() -> patchEntity(g2t4.getId(), json2, g2t4, ADMIN_AUTH_HEADERS), FORBIDDEN, notReviewer("admin")); + + // A reviewer can change the `Draft` to `Rejected` status using PATCH + GlossaryTerm g2t4Updated = patchEntity(g2t4.getId(), json2, g2t4, authHeaders(USER1.getName())); + assertEquals(Status.REJECTED, g2t4Updated.getStatus()); + assertApprovalTask(g2t4, TaskStatus.Closed); // The Request Approval task is closed + } + + private Thread assertApprovalTask(GlossaryTerm term, TaskStatus expectedTaskStatus) throws HttpResponseException { + String entityLink = new EntityLink(Entity.GLOSSARY_TERM, term.getFullyQualifiedName()).getLinkString(); + ThreadList threads = taskTest.listTasks(entityLink, null, null, expectedTaskStatus, 100, ADMIN_AUTH_HEADERS); + assertEquals(threads.getData().size(), 1); + Thread taskThread = threads.getData().get(0); + TaskDetails taskDetails = taskThread.getTask(); + assertNotNull(taskDetails); + assertEquals(expectedTaskStatus, taskDetails.getStatus()); + return taskThread; + } + @Test void patch_addDeleteTags(TestInfo test) throws IOException { // Create glossary term1 in glossary g1 @@ -363,7 +479,7 @@ public class GlossaryTermResourceTest extends EntityResourceTest