Fixes #31692: remove reviewer→approval-task assignee sync from repositories (#32912)

* Fixes #31692: wire GlossaryTerm approval-task reviewer sync

GlossaryTermRepository.updateTaskWithNewReviewers() existed but had no
caller, unlike the five sibling repositories (Tag, TestCase, DataProduct,
DataContract, Metric) that invoke it from an updateReviewers() override.
Reviewer/assignee consistency relied solely on the seeded Glossary Approval
Workflow re-triggering, which supersedes the open approval task with a new
one (new task id) rather than patching it in place, orphaning comment
history and coupling the guarantee to workflow configuration.

Add an updateReviewers() override in GlossaryTermUpdater following the
TagRepository pattern: when the reviewer list changes it patches the
existing open approval task's assignees in place. updateApprovalTaskAssignees
returns early when no open task exists, so terms without an approval
workflow are unaffected.

Add GlossaryTermReviewerTaskSyncIT: creates a term with one reviewer, waits
for its open approval task, adds a second reviewer, and asserts the original
task (fetched by id) is patched to include the new reviewer. Because the
repository patch is synchronous within the PATCH transaction and the
workflow safety net can only open a new task, this isolates the repository
mechanism from the workflow without disabling the seeded workflow.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Fixes #31692: remove reviewer→approval-task assignee sync from repositories

updateTaskWithNewReviewers / TaskRepository.updateApprovalTaskAssignees
overwrote an open approval task's assignees with the entity's reviewers
whenever the reviewer list changed. That is wrong: a userApprovalTask
chooses its assignees by rule (owners, reviewers, individual users or
teams), so forcing the reviewer list onto the task corrupts any task not
configured to assign reviewers. Task assignees are a point-in-time
snapshot owned by the workflow, not by the repository.

Remove the mechanism everywhere:
- updateReviewers() overrides in Tag, TestCase, DataProduct,
  DataContract, Metric (were live)
- dead updateTaskWithNewReviewers in GlossaryTerm and KnowledgePage
  (never called) plus GlossaryTerm's now-unused resolveEffectiveReviewers
- TaskRepository.updateApprovalTaskAssignees (no remaining callers)

This also reverts the wiring added earlier on this branch.

Behavior note: Tag/TestCase/DataProduct/DataContract/Metric have no
seeded approval workflow, so for a custom approval workflow this sync was
the only thing that moved an open task's assignees on a reviewer change.
After this change, changing reviewers leaves an existing open approval
task's assignees untouched — the intended semantics. GlossaryTerm and AI
assets have seeded workflows that already supersede the open task with a
fresh one assigned to the current reviewers, so their user-facing
behavior is unchanged.

Replace the prior in-place-patch IT with GlossaryTermReviewerChangeApprovalIT,
which asserts the acceptance criterion: adding a reviewer to an in-review
term leaves exactly one open approval task assigned to the new reviewers.

* test: assert reviewer change supersedes the open approval task

Require the settled open approval task to be a NEW task (id != original)
assigned to the added reviewer, and fail fast with diagnostics if the
approval workflow settles terminally without producing it, instead of a
bare Awaitility timeout.

* test: cover removal with unit + runtime approval-assignee tests

Replace the glossary supersession IT (which passed both before and after
the removal) with tests that target the invariant directly:

- SetApprovalAssigneesImplTest: an owners-only and an explicit-users
  userApprovalTask rule must not pull the entity's reviewers into the
  task's assignees — the principle the removed repository sync violated.
- WorkflowDefinitionResourceIT#test_reviewerChangeDoesNotOverwriteOwnerAssignedApprovalTask:
  a custom approval workflow assigns its task to owners on a tag (Group-2
  entity), triggered on Created only so a reviewer change cannot re-trigger
  it. Changing the tag's reviewers must leave the owner-assigned task's
  assignees untouched. This fails without the removal: the repository
  overwrote the open task's assignees with the reviewer list in the PATCH.

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Ram Narayan Balaji <ramnarayanb3005@gmail.com>
Co-authored-by: Ram Narayan Balaji <81347100+yan-3005@users.noreply.github.com>

Author: 3 people

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/WorkflowDefinitionResourceIT.java

@@ -27,10 +27,12 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.BiConsumer;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.awaitility.core.ConditionTimeoutException;
 import org.flowable.engine.ManagementService;
@@ -48,6 +50,7 @@
 import org.junit.jupiter.api.parallel.Execution;
 import org.junit.jupiter.api.parallel.ExecutionMode;
 import org.junit.jupiter.api.parallel.Isolated;
+import org.openmetadata.it.bootstrap.SharedEntities;
 import org.openmetadata.it.factories.MlModelServiceTestFactory;
 import org.openmetadata.it.util.SdkClients;
 import org.openmetadata.it.util.TestNamespace;
@@ -3626,6 +3629,190 @@ void test_MixedEntityTypesWithReviewerSupport() {
     LOG.info("test_MixedEntityTypesWithReviewerSupport completed successfully");
   }
 
+  /**
+   * Regression guard for #31692. A custom approval workflow assigns its task to <b>owners</b>
+   * (addOwners=true, addReviewers=false) on a {@code tag} — a Group-2 entity that used to wire
+   * {@code updateReviewers() -> updateTaskWithNewReviewers}. The workflow triggers on {@code Created}
+   * only, so changing the tag's reviewers afterwards cannot re-trigger it; the ONLY thing that could
+   * mutate the open task's assignees on a reviewer change was the removed repository sync. After the
+   * removal, changing reviewers must leave the owner-assigned approval task's assignees untouched —
+   * the added reviewer must NOT be injected as an approver. Before the removal this test fails: the
+   * repository overwrites the task's assignees with the reviewer list inside the PATCH transaction.
+   */
+  @Test
+  @Order(220)
+  void test_reviewerChangeDoesNotOverwriteOwnerAssignedApprovalTask(TestNamespace ns)
+      throws Exception {
+    LOG.info("Starting test_reviewerChangeDoesNotOverwriteOwnerAssignedApprovalTask");
+    OpenMetadataClient client = SdkClients.adminClient();
+    SharedEntities shared = SharedEntities.get();
+    ensureWorkflowEventConsumerIsActive(client);
+
+    String workflowName = "ownerAssignedTagApproval_" + UUID.randomUUID();
+    String workflowJson =
+        """
+            {
+              "name": "%s",
+              "displayName": "Owner Assigned Tag Approval",
+              "description": "Approval task assigned to owners, triggered on tag creation only",
+              "trigger": {
+                "type": "eventBasedEntity",
+                "config": {
+                  "entityTypes": ["tag"],
+                  "events": ["Created"]
+                },
+                "output": ["relatedEntity", "updatedBy"]
+              },
+              "nodes": [
+                {"name": "start", "displayName": "Start", "type": "startEvent", "subType": "startEvent"},
+                {
+                  "name": "ApproveTag",
+                  "displayName": "Approve Tag",
+                  "type": "userTask",
+                  "subType": "userApprovalTask",
+                  "config": {
+                    "assignees": {"addReviewers": false, "addOwners": true, "candidates": []},
+                    "approvalThreshold": 1,
+                    "rejectionThreshold": 1,
+                    "stageId": "review",
+                    "stageDisplayName": "Review",
+                    "taskStatus": "Open",
+                    "assigneeStrategy": "reviewers-and-assignees",
+                    "transitionMetadata": [
+                      {"id": "approve", "label": "Approve", "targetStageId": "approved", "targetTaskStatus": "Approved", "resolutionType": "Approved", "formRef": "approve", "requiresComment": false},
+                      {"id": "reject", "label": "Reject", "targetStageId": "rejected", "targetTaskStatus": "Rejected", "resolutionType": "Rejected", "formRef": "reject", "requiresComment": true}
+                    ]
+                  },
+                  "inputNamespaceMap": {"relatedEntity": "global"}
+                },
+                {"name": "endApproved", "displayName": "End Approved", "type": "endEvent", "subType": "endEvent"},
+                {"name": "endRejected", "displayName": "End Rejected", "type": "endEvent", "subType": "endEvent"}
+              ],
+              "edges": [
+                {"from": "start", "to": "ApproveTag"},
+                {"from": "ApproveTag", "to": "endApproved", "condition": "approve"},
+                {"from": "ApproveTag", "to": "endRejected", "condition": "reject"}
+              ],
+              "config": {"storeStageStatus": true}
+            }
+            """
+            .formatted(workflowName);
+
+    String createResponse =
+        client
+            .getHttpClient()
+            .executeForString(
+                HttpMethod.POST,
+                BASE_PATH,
+                MAPPER.readValue(workflowJson, CreateWorkflowDefinition.class),
+                RequestOptions.builder().build());
+    JsonNode created = MAPPER.readTree(createResponse);
+    assertTrue(created.has("id"));
+    trackWorkflowFromJson(created);
+    waitForWorkflowDeployment(client, workflowName);
+    ensureWorkflowEventConsumerIsActive(client);
+
+    // Classification + tag owned by USER1 with USER2 as an initial reviewer.
+    Classification classification =
+        client
+            .classifications()
+            .create(
+                new CreateClassification()
+                    .withName(
+                        ns.prefix("revguard")
+                            .substring(0, Math.min(25, ns.prefix("revguard").length())))
+                    .withDescription("Classification for reviewer-change approval guard"));
+    CreateTag createTag =
+        new CreateTag()
+            .withName("guardTag")
+            .withClassification(classification.getFullyQualifiedName())
+            .withDescription("Owner-assigned approval guard tag")
+            .withOwners(List.of(shared.USER1_REF))
+            .withReviewers(List.of(shared.USER2_REF));
+    Tag tag = client.tags().create(createTag);
+
+    Task task = awaitOpenApprovalTaskForEntity(tag.getFullyQualifiedName());
+    Set<UUID> assignees = assigneeIds(task);
+    assertTrue(
+        assignees.contains(shared.USER1.getId()),
+        "Owner must be assigned to the approval task, assignees=" + assignees);
+    assertFalse(
+        assignees.contains(shared.USER2.getId()),
+        "Reviewer must not be assigned to an owners-only approval task, assignees=" + assignees);
+
+    // Change reviewers: add USER3. The Created-only workflow cannot re-trigger on this Updated
+    // event.
+    JsonNode reviewerPatch =
+        MAPPER.readTree(
+            String.format(
+                "[{\"op\":\"replace\",\"path\":\"/reviewers\",\"value\":"
+                    + "[{\"id\":\"%s\",\"type\":\"user\"},{\"id\":\"%s\",\"type\":\"user\"}]}]",
+                shared.USER2.getId(), shared.USER3.getId()));
+    client.tags().patch(tag.getId().toString(), reviewerPatch);
+
+    // The open approval task's assignees must be unchanged — still the owner, never the reviewers.
+    Task afterPatch = openApprovalTaskById(tag.getFullyQualifiedName(), task.getId());
+    assertNotNull(
+        afterPatch, "The owner-assigned approval task must remain open after the reviewer change");
+    Set<UUID> afterAssignees = assigneeIds(afterPatch);
+    assertTrue(
+        afterAssignees.contains(shared.USER1.getId()),
+        "Owner must remain assigned after the reviewer change, assignees=" + afterAssignees);
+    assertFalse(
+        afterAssignees.contains(shared.USER3.getId()),
+        "A newly added reviewer must not be injected into an owners-only task, assignees="
+            + afterAssignees);
+    assertFalse(
+        afterAssignees.contains(shared.USER2.getId()),
+        "Reviewers must not leak into an owners-only task, assignees=" + afterAssignees);
+
+    LOG.info("test_reviewerChangeDoesNotOverwriteOwnerAssignedApprovalTask completed successfully");
+  }
+
+  private Task awaitOpenApprovalTaskForEntity(String entityFqn) {
+    await("open approval task for " + entityFqn)
+        .atMost(Duration.ofMinutes(5))
+        .pollInterval(Duration.ofSeconds(2))
+        .until(() -> !openApprovalTasks(entityFqn).isEmpty());
+    List<Task> tasks = openApprovalTasks(entityFqn);
+    assertFalse(tasks.isEmpty(), "Expected an open approval task for " + entityFqn);
+    return tasks.get(0);
+  }
+
+  private Task openApprovalTaskById(String entityFqn, UUID taskId) {
+    return openApprovalTasks(entityFqn).stream()
+        .filter(t -> taskId.equals(t.getId()))
+        .findFirst()
+        .orElse(null);
+  }
+
+  private List<Task> openApprovalTasks(String entityFqn) {
+    List<Task> tasks;
+    try {
+      ListResponse<Task> response =
+          SdkClients.adminClient()
+              .tasks()
+              .listWithFilters(
+                  Map.of(
+                      "limit",
+                      "100",
+                      "status",
+                      TaskEntityStatus.Open.value(),
+                      "aboutEntity",
+                      entityFqn));
+      tasks = response.getData() == null ? List.of() : response.getData();
+    } catch (RuntimeException e) {
+      tasks = List.of();
+    }
+    return tasks;
+  }
+
+  private Set<UUID> assigneeIds(Task task) {
+    return task.getAssignees() == null
+        ? Set.of()
+        : task.getAssignees().stream().map(EntityReference::getId).collect(Collectors.toSet());
+  }
+
   @Test
   @Order(34)
   void test_WorkflowValidationEndpoint() {

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

@@ -1507,16 +1507,6 @@ public DataContractUpdater(
       super(original, updated, operation, changeSource);
     }
 
-    @Override
-    public void updateReviewers() {
-      super.updateReviewers();
-      if (original.getReviewers() != null
-          && updated.getReviewers() != null
-          && !original.getReviewers().equals(updated.getReviewers())) {
-        updateTaskWithNewReviewers(updated);
-      }
-    }
-
     @Override
     public void entitySpecificUpdate(boolean consolidatingChanges) {
       preserveUnspecifiedODCSPassthrough();
@@ -1915,18 +1905,4 @@ private void closeApprovalTask(DataContract entity, String comment) {
     taskRepository.closeApprovalTaskForEntity(
         entity.getFullyQualifiedName(), entity.getUpdatedBy(), comment);
   }
-
-  protected void updateTaskWithNewReviewers(DataContract dataContract) {
-    dataContract =
-        Entity.getEntityByName(
-            Entity.DATA_CONTRACT,
-            dataContract.getFullyQualifiedName(),
-            "id,fullyQualifiedName,reviewers",
-            Include.ALL);
-    TaskRepository taskRepository = (TaskRepository) Entity.getEntityRepository(Entity.TASK);
-    taskRepository.updateApprovalTaskAssignees(
-        dataContract.getFullyQualifiedName(),
-        new ArrayList<>(dataContract.getReviewers()),
-        dataContract.getUpdatedBy());
-  }
 }

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

@@ -949,16 +949,6 @@ protected void resetForRetryAttempt() {
       capturedUpdatedDomains = null;
     }
 
-    @Override
-    public void updateReviewers() {
-      super.updateReviewers();
-      if (original.getReviewers() != null
-          && updated.getReviewers() != null
-          && !original.getReviewers().equals(updated.getReviewers())) {
-        updateTaskWithNewReviewers(updated);
-      }
-    }
-
     public List<EntityReference> getCapturedOriginalDomains() {
       return capturedOriginalDomains;
     }
@@ -1259,20 +1249,6 @@ private void closeApprovalTask(DataProduct entity, String comment) {
         entity.getFullyQualifiedName(), entity.getUpdatedBy(), comment);
   }
 
-  protected void updateTaskWithNewReviewers(DataProduct dataProduct) {
-    dataProduct =
-        Entity.getEntityByName(
-            Entity.DATA_PRODUCT,
-            dataProduct.getFullyQualifiedName(),
-            "id,fullyQualifiedName,reviewers",
-            Include.ALL);
-    TaskRepository taskRepository = (TaskRepository) Entity.getEntityRepository(Entity.TASK);
-    taskRepository.updateApprovalTaskAssignees(
-        dataProduct.getFullyQualifiedName(),
-        new ArrayList<>(dataProduct.getReviewers()),
-        dataProduct.getUpdatedBy());
-  }
-
   public org.openmetadata.schema.entity.data.DataContract getDataProductContract(
       UUID dataProductId) {
     try {

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

@@ -2512,44 +2512,6 @@ private List<GlossaryTerm> getNestedTerms(GlossaryTerm glossaryTerm) {
     return JsonUtils.readObjects(jsons, GlossaryTerm.class);
   }
 
-  protected void updateTaskWithNewReviewers(GlossaryTerm term) {
-    term =
-        Entity.getEntityByName(
-            Entity.GLOSSARY_TERM,
-            term.getFullyQualifiedName(),
-            "id,fullyQualifiedName,reviewers,parent,glossary",
-            Include.ALL);
-    TaskRepository taskRepository = (TaskRepository) Entity.getEntityRepository(Entity.TASK);
-    taskRepository.updateApprovalTaskAssignees(
-        term.getFullyQualifiedName(),
-        new ArrayList<>(resolveEffectiveReviewers(term)),
-        term.getUpdatedBy());
-  }
-
-  private List<EntityReference> resolveEffectiveReviewers(GlossaryTerm term) {
-    if (!nullOrEmpty(term.getReviewers())) {
-      return term.getReviewers();
-    }
-
-    if (term.getParent() != null) {
-      GlossaryTerm parentTerm =
-          Entity.getEntity(
-              term.getParent().withType(GLOSSARY_TERM), "reviewers", Include.NON_DELETED);
-      if (!nullOrEmpty(parentTerm.getReviewers())) {
-        return parentTerm.getReviewers();
-      }
-    }
-
-    if (term.getGlossary() != null) {
-      Glossary glossary = Entity.getEntity(term.getGlossary(), "reviewers", Include.NON_DELETED);
-      if (!nullOrEmpty(glossary.getReviewers())) {
-        return glossary.getReviewers();
-      }
-    }
-
-    return List.of();
-  }
-
   private void fetchAndSetRelatedTerms(List<GlossaryTerm> entities, Fields fields) {
     if (!fields.contains("relatedTerms") || entities.isEmpty()) {
       return;

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

@@ -745,20 +745,6 @@ private void updateArticles(Page original, Page updated) {
     }
   }
 
-  protected void updateTaskWithNewReviewers(Page page) {
-    Page currentPage =
-        Entity.getEntityByName(
-            KNOWLEDGE_PAGE_ENTITY,
-            page.getFullyQualifiedName(),
-            "id,fullyQualifiedName,reviewers",
-            Include.ALL);
-    TaskRepository taskRepository = (TaskRepository) Entity.getEntityRepository(Entity.TASK);
-    taskRepository.updateApprovalTaskAssignees(
-        currentPage.getFullyQualifiedName(),
-        new ArrayList<>(currentPage.getReviewers()),
-        currentPage.getUpdatedBy());
-  }
-
   @Override
   public void postUpdate(Page original, Page updated) {
     super.postUpdate(original, updated);

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

@@ -471,16 +471,6 @@ public MetricUpdater(Metric original, Metric updated, Operation operation) {
       super(original, updated, operation);
     }
 
-    @Override
-    public void updateReviewers() {
-      super.updateReviewers();
-      if (original.getReviewers() != null
-          && updated.getReviewers() != null
-          && !original.getReviewers().equals(updated.getReviewers())) {
-        updateTaskWithNewReviewers(updated);
-      }
-    }
-
     @Transaction
     @Override
     public void entitySpecificUpdate(boolean consolidatingChanges) {
@@ -701,18 +691,4 @@ private void closeApprovalTask(Metric entity, String comment) {
     taskRepository.closeApprovalTaskForEntity(
         entity.getFullyQualifiedName(), entity.getUpdatedBy(), comment);
   }
-
-  protected void updateTaskWithNewReviewers(Metric metric) {
-    metric =
-        Entity.getEntityByName(
-            Entity.METRIC,
-            metric.getFullyQualifiedName(),
-            "id,fullyQualifiedName,reviewers",
-            Include.ALL);
-    TaskRepository taskRepository = (TaskRepository) Entity.getEntityRepository(Entity.TASK);
-    taskRepository.updateApprovalTaskAssignees(
-        metric.getFullyQualifiedName(),
-        new ArrayList<>(metric.getReviewers()),
-        metric.getUpdatedBy());
-  }
 }