Code cleanup necessary for Glossary workflow (#13219)

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java

@@ -198,6 +198,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
   protected final boolean supportsVotes;
   @Getter protected final boolean supportsDomain;
   protected final boolean supportsDataProducts;
+  @Getter protected final boolean supportsReviewers;
   protected boolean quoteFqn = false; // Entity fqns not hierarchical such user, teams, services need to be quoted
 
   /** Fields that can be updated during PATCH operation */
@@ -256,6 +257,11 @@ public abstract class EntityRepository<T extends EntityInterface> {
       this.patchFields.addField(allowedFields, FIELD_DOMAIN);
       this.putFields.addField(allowedFields, FIELD_DOMAIN);
     }
+    this.supportsReviewers = allowedFields.contains(FIELD_REVIEWERS);
+    if (supportsReviewers) {
+      this.patchFields.addField(allowedFields, FIELD_REVIEWERS);
+      this.putFields.addField(allowedFields, FIELD_REVIEWERS);
+    }
     this.supportsDataProducts = allowedFields.contains(FIELD_DATA_PRODUCTS);
     if (supportsDataProducts) {
       this.patchFields.addField(allowedFields, FIELD_DATA_PRODUCTS);
@@ -626,7 +632,6 @@ public abstract class EntityRepository<T extends EntityInterface> {
 
   public final T create(UriInfo uriInfo, T entity) {
     entity = withHref(uriInfo, createInternal(entity));
-    postCreate(entity);
     return entity;
   }
 
@@ -685,16 +690,6 @@ public abstract class EntityRepository<T extends EntityInterface> {
   }
 
   public final PutResponse<T> createOrUpdate(UriInfo uriInfo, T updated) {
-    PutResponse<T> response = createOrUpdateInternal(uriInfo, updated);
-    if (response.getStatus() == Status.CREATED) {
-      postCreate(response.getEntity());
-    } else if (response.getStatus() == Status.OK) {
-      postUpdate(response.getEntity());
-    }
-    return response;
-  }
-
-  public final PutResponse<T> createOrUpdateInternal(UriInfo uriInfo, T updated) {
     T original = JsonUtils.readValue(dao.findJsonByFqn(updated.getFullyQualifiedName(), ALL), entityClass);
     if (original == null) { // If an original entity does not exist then create it, else update
       return new PutResponse<>(Status.CREATED, withHref(uriInfo, createNewEntity(updated)), RestUtil.ENTITY_CREATED);
@@ -785,7 +780,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
             .withCurrentVersion(entity.getVersion())
             .withPreviousVersion(change.getPreviousVersion());
     entity.setChangeDescription(change);
-    postUpdate(JsonUtils.deepCopy(entity, entityClass));
+    postUpdate(entity);
     return new PutResponse<>(Status.OK, changeEvent, RestUtil.ENTITY_FIELDS_CHANGED);
   }
 
@@ -1014,6 +1009,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
     storeExtension(entity);
     storeRelationshipsInternal(entity);
     setInheritedFields(entity, new Fields(allowedFields));
+    postCreate(entity);
     return entity;
   }
 

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java

@@ -21,6 +21,7 @@ import static org.openmetadata.schema.type.Relationship.ADDRESSED_TO;
 import static org.openmetadata.schema.type.Relationship.CREATED;
 import static org.openmetadata.schema.type.Relationship.IS_ABOUT;
 import static org.openmetadata.schema.type.Relationship.REPLIED_TO;
+import static org.openmetadata.schema.type.TaskStatus.Open;
 import static org.openmetadata.service.Entity.USER;
 import static org.openmetadata.service.exception.CatalogExceptionMessage.ANNOUNCEMENT_INVALID_START_TIME;
 import static org.openmetadata.service.exception.CatalogExceptionMessage.ANNOUNCEMENT_OVERLAP;
@@ -101,7 +102,7 @@ import org.openmetadata.service.util.ResultList;
 @Slf4j
 public class FeedRepository {
   private final CollectionDAO dao;
-  private static final MessageDecorator<FeedMessage> feedMessageFormatter = new FeedMessageDecorator();
+  private static final MessageDecorator<FeedMessage> FEED_MESSAGE_FORMATTER = new FeedMessageDecorator();
 
   public FeedRepository(CollectionDAO dao) {
     this.dao = dao;
@@ -307,7 +308,7 @@ public class FeedRepository {
     closeTask(threadContext, user, new CloseTask());
   }
 
-  private String getTagFQNs(List<TagLabel> tags) {
+  private static String getTagFQNs(List<TagLabel> tags) {
     return tags.stream().map(TagLabel::getTagFQN).collect(Collectors.joining(", "));
   }
 
@@ -315,26 +316,16 @@ public class FeedRepository {
     // Add a post to the task
     String message;
     if (closingComment != null) {
-      message = String.format("Closed the Task with comment - %s", closingComment);
+      message = closeTaskMessage(closingComment);
     } else {
       // The task was resolved with an update.
       // Add a default message to the Task thread with updated description/tag
       TaskDetails task = thread.getTask();
       TaskType type = task.getType();
       if (EntityUtil.isDescriptionTask(type)) {
-        message =
+        message = resolveDescriptionTaskMessage(task);
-            String.format(
-                "Resolved the Task with Description - %s",
-                feedMessageFormatter.getPlaintextDiff(task.getOldValue(), task.getNewValue()));
       } else if (EntityUtil.isTagTask(type)) {
-        String oldValue =
+        message = resolveTagTaskMessage(task);
-            task.getOldValue() != null
-                ? getTagFQNs(JsonUtils.readObjects(task.getOldValue(), TagLabel.class))
-                : StringUtils.EMPTY;
-        String newValue = getTagFQNs(JsonUtils.readObjects(task.getNewValue(), TagLabel.class));
-        message =
-            String.format(
-                "Resolved the Task with Tag(s) - %s", feedMessageFormatter.getPlaintextDiff(oldValue, newValue));
       } else {
         message = "Resolved the Task.";
       }
@@ -352,6 +343,9 @@ public class FeedRepository {
   private void closeTask(ThreadContext threadContext, String user, CloseTask closeTask) {
     Thread thread = threadContext.getThread();
     TaskDetails task = thread.getTask();
+    if (task.getStatus() != Open) {
+      return;
+    }
     TaskWorkflow workflow = threadContext.getTaskWorkflow();
     workflow.closeTask(user, closeTask);
     task.withStatus(TaskStatus.Closed).withClosedBy(user).withClosedAt(System.currentTimeMillis());
@@ -971,6 +965,26 @@ public class FeedRepository {
     return user != null ? FullyQualifiedName.buildHash(user.getFullyQualifiedName()) : null;
   }
 
+  public static String resolveDescriptionTaskMessage(TaskDetails task) {
+    return String.format(
+        "Resolved the Task with Description - %s",
+        FEED_MESSAGE_FORMATTER.getPlaintextDiff(task.getOldValue(), task.getNewValue()));
+  }
+
+  public static String resolveTagTaskMessage(TaskDetails task) {
+    String oldValue =
+        task.getOldValue() != null
+            ? getTagFQNs(JsonUtils.readObjects(task.getOldValue(), TagLabel.class))
+            : StringUtils.EMPTY;
+    String newValue = getTagFQNs(JsonUtils.readObjects(task.getNewValue(), TagLabel.class));
+    return String.format(
+        "Resolved the Task with Tag(s) - %s", FEED_MESSAGE_FORMATTER.getPlaintextDiff(oldValue, newValue));
+  }
+
+  public static String closeTaskMessage(String closingComment) {
+    return String.format("Closed the Task with comment - %s", closingComment);
+  }
+
   public static class FilteredThreads {
     @Getter private final List<Thread> threads;
     @Getter private final int totalCount;

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryRepository.java

@@ -62,8 +62,8 @@ import org.openmetadata.service.util.FullyQualifiedName;
 
 @Slf4j
 public class GlossaryRepository extends EntityRepository<Glossary> {
-  private static final String UPDATE_FIELDS = "reviewers";
+  private static final String UPDATE_FIELDS = "";
-  private static final String PATCH_FIELDS = "reviewers";
+  private static final String PATCH_FIELDS = "";
 
   public GlossaryRepository(CollectionDAO dao) {
     super(

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java

@@ -56,8 +56,8 @@ import org.openmetadata.service.util.RestUtil;
 
 @Slf4j
 public class GlossaryTermRepository extends EntityRepository<GlossaryTerm> {
-  private static final String UPDATE_FIELDS = "references,relatedTerms,reviewers,synonyms";
+  private static final String UPDATE_FIELDS = "references,relatedTerms,synonyms";
-  private static final String PATCH_FIELDS = "references,relatedTerms,reviewers,synonyms";
+  private static final String PATCH_FIELDS = "references,relatedTerms,synonyms";
 
   public GlossaryTermRepository(CollectionDAO dao) {
     super(

openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java

@@ -245,7 +245,6 @@ public abstract class EntityResource<T extends EntityInterface, K extends Entity
     authorizer.authorize(securityContext, operationContext, getResourceContextById(id));
     PatchResponse<T> response = repository.patch(uriInfo, id, securityContext.getUserPrincipal().getName(), patch);
     addHref(uriInfo, response.getEntity());
-    repository.postUpdate(response.getEntity());
     return response.toResponse();
   }
 

openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java

@@ -86,7 +86,7 @@ import org.openmetadata.service.util.TestUtils.UpdateType;
 
 @TestMethodOrder(MethodOrderer.OrderAnnotation.class)
 public class GlossaryTermResourceTest extends EntityResourceTest<GlossaryTerm, CreateGlossaryTerm> {
-  private final GlossaryResourceTest glossaryResourceTest = new GlossaryResourceTest();
+  private final GlossaryResourceTest glossaryTest = new GlossaryResourceTest();
 
   public GlossaryTermResourceTest() {
     super(
@@ -189,8 +189,8 @@ public class GlossaryTermResourceTest extends EntityResourceTest<GlossaryTerm, C
   @Test
   void test_inheritDomain(TestInfo test) throws IOException {
     // When domain is not set for a glossary term, carry it forward from the glossary
-    CreateGlossary createGlossary = glossaryResourceTest.createRequest(test).withDomain(DOMAIN.getFullyQualifiedName());
+    CreateGlossary createGlossary = glossaryTest.createRequest(test).withDomain(DOMAIN.getFullyQualifiedName());
-    Glossary glossary = glossaryResourceTest.createEntity(createGlossary, ADMIN_AUTH_HEADERS);
+    Glossary glossary = glossaryTest.createEntity(createGlossary, ADMIN_AUTH_HEADERS);
 
     // Create term t1 in the glossary without domain
     CreateGlossaryTerm create =
@@ -342,9 +342,7 @@ public class GlossaryTermResourceTest extends EntityResourceTest<GlossaryTerm, C
 
     // g1 glossary is not empty and can't be deleted
     assertResponse(
-        () -> glossaryResourceTest.deleteEntity(g1.getId(), ADMIN_AUTH_HEADERS),
+        () -> glossaryTest.deleteEntity(g1.getId(), ADMIN_AUTH_HEADERS), BAD_REQUEST, entityIsNotEmpty(GLOSSARY));
-        BAD_REQUEST,
-        entityIsNotEmpty(GLOSSARY));
 
     // t1 is not empty and can't be deleted
     assertResponse(() -> deleteEntity(t1.getId(), ADMIN_AUTH_HEADERS), BAD_REQUEST, entityIsNotEmpty(GLOSSARY_TERM));
@@ -368,8 +366,8 @@ public class GlossaryTermResourceTest extends EntityResourceTest<GlossaryTerm, C
     assertFalse(table.getTags().isEmpty()); // tag t1 still exists
 
     // Delete the entire glossary
-    glossaryResourceTest.deleteAndCheckEntity(g1, true, true, ADMIN_AUTH_HEADERS);
+    glossaryTest.deleteAndCheckEntity(g1, true, true, ADMIN_AUTH_HEADERS);
-    glossaryResourceTest.assertEntityDeleted(g1.getId(), true);
+    glossaryTest.assertEntityDeleted(g1.getId(), true);
     assertEntityDeleted(t1.getId(), true);
 
     // Check to see the tags assigned are deleted
@@ -622,16 +620,16 @@ public class GlossaryTermResourceTest extends EntityResourceTest<GlossaryTerm, C
 
   public Glossary createGlossary(TestInfo test, List<EntityReference> reviewers, EntityReference owner)
       throws IOException {
-    return createGlossary(glossaryResourceTest.getEntityName(test), reviewers, owner);
+    return createGlossary(glossaryTest.getEntityName(test), reviewers, owner);
   }
 
   public Glossary createGlossary(String name, List<EntityReference> reviewers, EntityReference owner)
       throws IOException {
-    CreateGlossary create = glossaryResourceTest.createRequest(name).withReviewers(getFqns(reviewers)).withOwner(owner);
+    CreateGlossary create = glossaryTest.createRequest(name).withReviewers(getFqns(reviewers)).withOwner(owner);
-    return glossaryResourceTest.createAndCheckEntity(create, ADMIN_AUTH_HEADERS);
+    return glossaryTest.createAndCheckEntity(create, ADMIN_AUTH_HEADERS);
   }
 
   public Glossary getGlossary(String name) throws IOException {
-    return glossaryResourceTest.getEntityByName(name, glossaryResourceTest.getAllowedFields(), ADMIN_AUTH_HEADERS);
+    return glossaryTest.getEntityByName(name, glossaryTest.getAllowedFields(), ADMIN_AUTH_HEADERS);
   }
 }