From 5363c70a5b70aae1ab39696744a74d7a3bfd9ae9 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 11:20:55 -0700 Subject: [PATCH 01/13] fix(governance): resolve inherited reviewers in the approval gate The gate that decides whether an approval task is created (CheckGlossaryTermHasReviewers -> CheckEntityAttributesImpl) evaluated `some reviewers` over whatever the entity read returned, so it was correct only when that read had both requested and applied field inheritance. A glossary term that inherits its reviewers from its parent glossary could therefore be judged to have none, skip the approval task, and settle in Draft. Two sibling paths already resolve the chain explicitly -- setDefaultStatus and SetApprovalAssigneesImpl -- so "what status" and "who gets the task" were robust while "should there be a task at all" was not. TagRepository.setInheritedFields compounds the exposure by swallowing exceptions and silently skipping inheritance, leaving the same gate with an empty reviewers list. Add an EntityRepository.getEffectiveReviewers hook, overridden by GlossaryTermRepository (term -> parent term -> glossary) and TagRepository (tag -> classification), and have the gate resolve effective reviewers before evaluating the rule. The resolved reviewers are injected into the rule input map rather than onto the entity, which may be request-cached and must not be mutated. This covers both gates using the reviewers rule: GlossaryApprovalWorkflow and AIAssetApprovalWorkflow, and needs no workflow migration -- it fixes the data the rule sees, so already-customized workflow definitions benefit unchanged. Adds GlossaryTermInheritedReviewerApprovalIT covering direct, glossary-inherited, parent-term-inherited, inherited-with-owner-set, and remove-direct-reviewer fallback scenarios. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...ossaryTermInheritedReviewerApprovalIT.java | 410 ++++++++++++++++++ .../impl/CheckEntityAttributesImpl.java | 32 +- .../service/jdbi3/EntityRepository.java | 15 + .../service/jdbi3/GlossaryTermRepository.java | 10 + .../service/jdbi3/TagRepository.java | 18 + 5 files changed, 484 insertions(+), 1 deletion(-) create mode 100644 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java new file mode 100644 index 000000000000..f36986009cd5 --- /dev/null +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java @@ -0,0 +1,410 @@ +/* + * Copyright 2024 Collate + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.it.tests; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.stream.Collectors; +import org.awaitility.Awaitility; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; +import org.openmetadata.it.bootstrap.SharedEntities; +import org.openmetadata.it.util.SdkClients; +import org.openmetadata.it.util.TestNamespace; +import org.openmetadata.it.util.TestNamespaceExtension; +import org.openmetadata.schema.api.data.CreateGlossary; +import org.openmetadata.schema.api.data.CreateGlossaryTerm; +import org.openmetadata.schema.entity.data.Glossary; +import org.openmetadata.schema.entity.data.GlossaryTerm; +import org.openmetadata.schema.entity.tasks.Task; +import org.openmetadata.schema.entity.teams.User; +import org.openmetadata.schema.type.EntityReference; +import org.openmetadata.schema.type.EntityStatus; +import org.openmetadata.schema.type.TaskEntityStatus; +import org.openmetadata.sdk.models.ListResponse; +import org.openmetadata.sdk.network.HttpMethod; +import org.openmetadata.sdk.network.RequestOptions; +import org.openmetadata.service.Entity; + +/** + * Reproduces the Glossary Approval regression where a term whose reviewers are inherited (from + * the parent Glossary) does NOT get an approval task, while a term with reviewers attached + * directly does. + * + *

Reported scenario (see the attached workflow-history screenshot on the issue): + * + *

    + *
  1. A reviewer is attached to the term itself → the {@code GlossaryTermApprovalWorkflow} + * triggers, the term moves to {@code In Review} and an open approval task is created for the + * reviewer. (Works.) + *
  2. The reviewer is removed from the term so the term inherits the reviewer from its + * Glossary → no approval task is created and the term settles in {@code Draft}. (Bug.) + *
+ * + *

The workflow's {@code CheckGlossaryTermHasReviewers} node (JsonLogic {@code some reviewers ...}) + * is expected to see inherited reviewers because the node re-fetches the term with {@code + * getEntity(entityLink, "*", Include.ALL)} — which applies {@code setInheritedFields}. If inheritance + * is not honored on that read path, the term is routed to a terminal status (Draft/Approved) and no + * task is ever assigned to the inherited reviewer. + * + *

{@link #test_directReviewerOnTerm_createsOpenApprovalTask} is the control (direct reviewers) and + * must pass on every build. {@link #test_inheritedReviewerFromGlossary_createsOpenApprovalTask} and + * {@link #test_inheritedReviewerFromParentTerm_createsOpenApprovalTask} assert the post-fix invariant + * — inherited reviewers must produce the same {@code In Review} + open-task outcome as direct + * reviewers. On an affected build they fail fast: the approval workflow reaches a terminal status + * without ever creating a task. + */ +@ExtendWith(TestNamespaceExtension.class) +@Execution(ExecutionMode.CONCURRENT) +public class GlossaryTermInheritedReviewerApprovalIT { + + private static final String APPROVAL_WORKFLOW = "GlossaryTermApprovalWorkflow"; + private static final Set TERMINAL_WORKFLOW_STATUSES = + Set.of("FINISHED", "EXCEPTION", "FAILURE"); + // The approval outcome is driven by the async Flowable job executor (BPMN trigger → reviewer + // check + // → status transition → approval task), which contends with the rest of the concurrent suite, so + // the budgets mirror the generous ones in GlossaryTermMoveApprovalIT. + private static final Duration TASK_TIMEOUT = Duration.ofMinutes(5); + private static final Duration STATUS_TIMEOUT = Duration.ofMinutes(3); + private static final Duration POLL_INTERVAL = Duration.ofSeconds(2); + + protected SharedEntities shared() { + return SharedEntities.get(); + } + + /** USER1 carries the AllowAll test-admin role, so it is a valid approval-task assignee. */ + protected User reviewer() { + return shared().USER1; + } + + /** A distinct user used as a term owner (and as a removable direct reviewer). */ + protected User otherUser() { + return shared().USER2; + } + + /** + * Control: a reviewer attached directly to the term drives it to {@code In Review} with an open + * approval task assigned to that reviewer. This path is known to work and must always pass. + */ + @Test + void test_directReviewerOnTerm_createsOpenApprovalTask(TestNamespace ns) throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ false); + GlossaryTerm term = + createTerm(glossary, "direct_reviewer_term", /* attachReviewerDirectly */ true); + + Task task = awaitOpenApprovalTask(term.getId(), term.getFullyQualifiedName()); + assertAssigneeIsReviewer(task); + waitForTermStatus(term.getId(), EntityStatus.IN_REVIEW); + } + + /** + * Bug: the glossary carries the reviewer and the term attaches none, so the term inherits the + * reviewer. The approval workflow must produce the SAME outcome as the direct case — {@code In + * Review} plus an open approval task assigned to the inherited reviewer. On an affected build the + * workflow instead finishes in {@code Draft} without creating a task, and this fails fast. + */ + @Test + void test_inheritedReviewerFromGlossary_createsOpenApprovalTask(TestNamespace ns) + throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ true); + GlossaryTerm term = + createTerm(glossary, "inherited_reviewer_term", /* attachReviewerDirectly */ false); + + // Sanity: the term reports the reviewer via inheritance on a normal GET (Include.ALL), which is + // exactly the read the workflow node performs. If this holds but no task is created, the bug is + // isolated to the workflow's reviewer-check read path. + assertTermInheritsReviewer(term.getId()); + + Task task = awaitOpenApprovalTask(term.getId(), term.getFullyQualifiedName()); + assertAssigneeIsReviewer(task); + waitForTermStatus(term.getId(), EntityStatus.IN_REVIEW); + } + + /** + * Bug variant: the reviewer is attached to a parent term and the child attaches none, so the child + * inherits the reviewer from the parent term (a different inheritance source than the Glossary, + * resolved by a distinct branch of {@code resolveEffectiveReviewers}). The parent is created BY the + * reviewer so it auto-approves (updatedBy == reviewer) and leaves no lingering task under it. The + * child must reach {@code In Review} with an open task assigned to the inherited reviewer. + */ + @Test + void test_inheritedReviewerFromParentTerm_createsOpenApprovalTask(TestNamespace ns) + throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ false); + GlossaryTerm parent = createReviewerOwnedTerm(glossary, "parent_with_reviewer"); + GlossaryTerm child = createChildTerm(glossary, parent, "child_inherits_reviewer"); + + assertTermInheritsReviewer(child.getId()); + + Task task = awaitOpenApprovalTask(child.getId(), child.getFullyQualifiedName()); + assertAssigneeIsReviewer(task); + waitForTermStatus(child.getId(), EntityStatus.IN_REVIEW); + } + + /** + * Faithful shape of the reported "hello world" term: an OWNER and a description are set, and the + * reviewer is inherited from the glossary (none on the term). This isolates whether an owner on the + * term suppresses inherited-reviewer resolution at the approval gate (it must not). + */ + @Test + void test_inheritedReviewerWithOwnerSet_createsOpenApprovalTask(TestNamespace ns) + throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ true); + CreateGlossaryTerm create = + new CreateGlossaryTerm() + .withName(ns.shortPrefix("owned_inherited")) + .withGlossary(glossary.getFullyQualifiedName()) + .withDescription("hello world") + .withOwners(List.of(otherUser().getEntityReference())); + GlossaryTerm term = SdkClients.adminClient().glossaryTerms().create(create); + + assertTermInheritsReviewer(term.getId()); + Task task = awaitOpenApprovalTask(term.getId(), term.getFullyQualifiedName()); + assertAssigneeIsReviewer(task); + waitForTermStatus(term.getId(), EntityStatus.IN_REVIEW); + } + + /** + * The reported Run-2 sequence: the glossary carries the reviewer; the term starts with a DIFFERENT + * direct reviewer, then that direct reviewer is removed so the term must fall back to the + * glossary-inherited reviewer and STILL be gated as "has reviewers" (task kept, not dropped to a + * terminal status). + */ + @Test + void test_removeDirectReviewer_fallsBackToGlossaryInheritedReviewer(TestNamespace ns) + throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ true); // glossary reviewer = USER1 + CreateGlossaryTerm create = + new CreateGlossaryTerm() + .withName(ns.shortPrefix("removed_direct")) + .withGlossary(glossary.getFullyQualifiedName()) + .withDescription("hello world") + .withReviewers(List.of(otherUser().getEntityReference())); // direct reviewer = USER2 + GlossaryTerm term = SdkClients.adminClient().glossaryTerms().create(create); + awaitOpenApprovalTask(term.getId(), term.getFullyQualifiedName()); + + removeDirectReviewers(term.getId()); // now the term inherits USER1 from the glossary + assertTermInheritsReviewer(term.getId()); + + // Decisive signal: the workflow must NOT drop the term to a terminal status without a task once + // the only remaining reviewer is inherited. (Task re-assignment timing is asserted elsewhere.) + awaitOpenApprovalTask(term.getId(), term.getFullyQualifiedName()); + } + + private void removeDirectReviewers(UUID termId) throws Exception { + JsonNode patch = + new ObjectMapper().readTree("[{\"op\":\"replace\",\"path\":\"/reviewers\",\"value\":[]}]"); + SdkClients.adminClient().glossaryTerms().patch(termId.toString(), patch); + } + + private Glossary createGlossary(TestNamespace ns, boolean withReviewer) { + CreateGlossary create = + new CreateGlossary() + .withName(ns.shortPrefix("inh")) + .withDescription("Glossary for inherited-reviewer approval test"); + if (withReviewer) { + create.withReviewers(List.of(reviewer().getEntityReference())); + } + return ns.trackRoot(Entity.GLOSSARY, SdkClients.adminClient().glossaries().create(create)); + } + + private GlossaryTerm createTerm(Glossary glossary, String name, boolean attachReviewerDirectly) { + CreateGlossaryTerm create = + new CreateGlossaryTerm() + .withName(name) + .withGlossary(glossary.getFullyQualifiedName()) + // A non-empty description is required to pass CheckGlossaryTermIsReadyToBeReviewed. + .withDescription("Term created by inherited-reviewer approval test"); + if (attachReviewerDirectly) { + create.withReviewers(List.of(reviewer().getEntityReference())); + } + return SdkClients.adminClient().glossaryTerms().create(create); + } + + private GlossaryTerm createChildTerm(Glossary glossary, GlossaryTerm parent, String name) { + CreateGlossaryTerm create = + new CreateGlossaryTerm() + .withName(name) + .withGlossary(glossary.getFullyQualifiedName()) + .withParent(parent.getFullyQualifiedName()) + .withDescription("Child term created by inherited-reviewer approval test"); + return SdkClients.adminClient().glossaryTerms().create(create); + } + + /** + * Creates a term with the reviewer attached directly, AS that reviewer, so the approval workflow + * auto-approves it (the updatedBy-is-reviewer branch) and it holds no open task of its own. + */ + private GlossaryTerm createReviewerOwnedTerm(Glossary glossary, String name) { + CreateGlossaryTerm create = + new CreateGlossaryTerm() + .withName(name) + .withGlossary(glossary.getFullyQualifiedName()) + .withDescription("Parent term created by inherited-reviewer approval test") + .withReviewers(List.of(reviewer().getEntityReference())); + return SdkClients.user1Client().glossaryTerms().create(create); + } + + private void assertTermInheritsReviewer(UUID termId) { + GlossaryTerm term = + SdkClients.adminClient().glossaryTerms().get(termId.toString(), "reviewers"); + Set reviewerIds = entityReferenceIds(term.getReviewers()); + assertTrue( + reviewerIds.contains(reviewer().getId()), + "Term " + + termId + + " should inherit reviewer " + + reviewer().getId() + + " but resolved reviewers were " + + reviewerIds); + } + + /** + * Waits until the term has an OPEN approval task, or fails fast the moment the approval workflow + * settles in a terminal state without one (the inherited-reviewers bug). Failing on a terminal, + * task-less workflow surfaces the regression in seconds instead of hanging for the full timeout. + */ + private Task awaitOpenApprovalTask(UUID termId, String termFqn) { + Map filters = + Map.of("limit", "100", "status", TaskEntityStatus.Open.value(), "aboutEntity", termFqn); + Awaitility.await("open approval task for " + termFqn) + .atMost(TASK_TIMEOUT) + .pollInterval(POLL_INTERVAL) + .until( + () -> + !safeListTasks(filters).isEmpty() + || approvalWorkflowSettledWithoutTask(termFqn, filters)); + List tasks = safeListTasks(filters); + if (tasks.isEmpty()) { + fail( + "Expected the Glossary Approval Workflow to create an OPEN approval task for " + + termFqn + + " (reviewer resolved via inheritance), but the workflow settled without one. " + + "term status=" + + safeCurrentStatus(termId) + + ", workflow instance statuses=" + + safeWorkflowStatuses(termFqn) + + ". Inherited reviewers were not honored — the term went straight to a terminal " + + "status instead of 'In Review' with an approval task."); + } + Task task = tasks.get(0); + assertNotNull(task.getId()); + return task; + } + + private boolean approvalWorkflowSettledWithoutTask(String termFqn, Map filters) { + boolean settledWithoutTask = false; + if (safeListTasks(filters).isEmpty()) { + List statuses = safeWorkflowStatuses(termFqn); + settledWithoutTask = + !statuses.isEmpty() && statuses.stream().allMatch(TERMINAL_WORKFLOW_STATUSES::contains); + } + return settledWithoutTask; + } + + private void assertAssigneeIsReviewer(Task task) { + Set assigneeIds = entityReferenceIds(task.getAssignees()); + assertTrue( + assigneeIds.contains(reviewer().getId()), + "Approval task " + + task.getId() + + " should be assigned to the (inherited) reviewer " + + reviewer().getId() + + " but assignees were " + + assigneeIds); + } + + private void waitForTermStatus(UUID termId, EntityStatus expected) { + Awaitility.await("glossary term " + termId + " should reach status " + expected) + .atMost(STATUS_TIMEOUT) + .pollInterval(POLL_INTERVAL) + .ignoreExceptions() + .untilAsserted(() -> assertEquals(expected, safeCurrentStatus(termId))); + } + + private List safeListTasks(Map filters) { + List tasks; + try { + ListResponse response = SdkClients.adminClient().tasks().listWithFilters(filters); + tasks = response.getData() == null ? List.of() : response.getData(); + } catch (RuntimeException e) { + tasks = List.of(); + } + return tasks; + } + + private EntityStatus safeCurrentStatus(UUID termId) { + EntityStatus status; + try { + status = SdkClients.adminClient().glossaryTerms().get(termId.toString()).getEntityStatus(); + } catch (RuntimeException e) { + status = null; + } + return status; + } + + private List safeWorkflowStatuses(String termFqn) { + List statuses; + try { + statuses = approvalWorkflowStatuses(termFqn); + } catch (Exception e) { + statuses = List.of(); + } + return statuses; + } + + private List approvalWorkflowStatuses(String termFqn) throws Exception { + long now = System.currentTimeMillis(); + RequestOptions options = + RequestOptions.builder() + .queryParam("entityLink", String.format("<#E::%s::%s>", Entity.GLOSSARY_TERM, termFqn)) + .queryParam("workflowDefinitionName", APPROVAL_WORKFLOW) + .queryParam("startTs", String.valueOf(now - Duration.ofHours(1).toMillis())) + .queryParam("endTs", String.valueOf(now + Duration.ofHours(1).toMillis())) + .queryParam("limit", "50") + .build(); + String response = + SdkClients.adminClient() + .getHttpClient() + .executeForString(HttpMethod.GET, "/v1/governance/workflowInstances", null, options); + JsonNode data = new ObjectMapper().readTree(response).path("data"); + List statuses = new ArrayList<>(); + if (data.isArray()) { + for (JsonNode instance : data) { + statuses.add(instance.path("status").asText()); + } + } + return statuses; + } + + private Set entityReferenceIds(List references) { + return references == null + ? Set.of() + : references.stream().map(EntityReference::getId).collect(Collectors.toSet()); + } +} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java index 999dda28fbe3..1d791d13eb63 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java @@ -1,11 +1,15 @@ package org.openmetadata.service.governance.workflows.elements.nodes.automatedTask.impl; +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; +import static org.openmetadata.service.Entity.FIELD_REVIEWERS; import static org.openmetadata.service.governance.workflows.Workflow.EXCEPTION_VARIABLE; import static org.openmetadata.service.governance.workflows.Workflow.RELATED_ENTITY_VARIABLE; import static org.openmetadata.service.governance.workflows.Workflow.RESULT_VARIABLE; import static org.openmetadata.service.governance.workflows.Workflow.WORKFLOW_RUNTIME_EXCEPTION; import static org.openmetadata.service.governance.workflows.WorkflowHandler.getProcessDefinitionKeyFromId; +import java.util.List; +import java.util.Map; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.exception.ExceptionUtils; import org.flowable.common.engine.api.delegate.Expression; @@ -13,10 +17,13 @@ import org.flowable.engine.delegate.DelegateExecution; import org.flowable.engine.delegate.JavaDelegate; import org.openmetadata.schema.EntityInterface; +import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.Include; import org.openmetadata.schema.utils.JsonUtils; +import org.openmetadata.service.Entity; import org.openmetadata.service.governance.workflows.WorkflowVariableHandler; import org.openmetadata.service.governance.workflows.WorkflowVariableHandler.InputNamespaces; +import org.openmetadata.service.jdbi3.EntityRepository; import org.openmetadata.service.resources.feeds.MessageParser; import org.openmetadata.service.rules.RuleEngine; @@ -52,10 +59,33 @@ private Boolean checkAttributes( boolean result; try { - result = (boolean) RuleEngine.getInstance().apply(rules, JsonUtils.getMap(entity)); + result = + (boolean) + RuleEngine.getInstance() + .apply(rules, buildRuleData(entityLink.getEntityType(), entity)); } catch (Exception e) { throw new RuntimeException(e); } return result; } + + /** + * Approval gates ask "does this entity have reviewers?" and route to a terminal status when the + * answer is no. An entity that inherits its reviewers — a glossary term under a reviewed glossary — + * must answer yes, otherwise no approval task is ever created and the term settles in Draft. The + * raw {@code reviewers} field carries inherited entries only when the read that produced the entity + * applied inheritance, so resolve them explicitly here, the same way the approval-task assignee node + * does. The entity is left untouched because it may be request-cached; only the rule input changes. + */ + private Map buildRuleData(String entityType, EntityInterface entity) { + Map ruleData = JsonUtils.getMap(entity); + EntityRepository repository = Entity.getEntityRepository(entityType); + if (repository.isSupportsReviewers() && nullOrEmpty(entity.getReviewers())) { + List effectiveReviewers = repository.getEffectiveReviewersUntyped(entity); + if (!nullOrEmpty(effectiveReviewers)) { + ruleData.put(FIELD_REVIEWERS, JsonUtils.convertValue(effectiveReviewers, List.class)); + } + } + return ruleData; + } } 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 a6143c97601f..9700952dac92 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 @@ -7864,6 +7864,21 @@ public final void inheritReviewers(T entity, Fields fields, EntityInterface pare } } + /** + * Reviewers that govern this entity's approval: those set directly on it, or — when it has none — + * those it inherits from its parent. Approval decisions must use this rather than the raw {@code + * reviewers} field, which carries inherited entries only when the read that produced the entity + * both requested and applied inheritance. Subclasses whose entities inherit reviewers override it. + */ + public List getEffectiveReviewers(T entity) { + return listOrEmpty(entity.getReviewers()); + } + + @SuppressWarnings("unchecked") + public List getEffectiveReviewersUntyped(EntityInterface entity) { + return getEffectiveReviewers((T) entity); + } + private List inheritedEntityReferences(List references) { if (nullOrEmpty(references)) { return Collections.emptyList(); 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 d5f2c7508e9d..1a4b29d7fff1 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 @@ -1779,6 +1779,16 @@ protected void updateTaskWithNewReviewers(GlossaryTerm term) { term.getUpdatedBy()); } + /** + * A term with no reviewers of its own is still governed by its parent term's — or its glossary's — + * reviewers, so approval gates must resolve the chain explicitly instead of trusting the read-time + * {@code reviewers} field. + */ + @Override + public List getEffectiveReviewers(GlossaryTerm glossaryTerm) { + return resolveEffectiveReviewers(glossaryTerm); + } + private List resolveEffectiveReviewers(GlossaryTerm term) { if (!nullOrEmpty(term.getReviewers())) { return term.getReviewers(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java index 66de0bc8489f..37b2b5677815 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java @@ -20,6 +20,7 @@ import static org.openmetadata.service.Entity.CLASSIFICATION; import static org.openmetadata.service.Entity.FIELD_CERTIFICATION; import static org.openmetadata.service.Entity.FIELD_NAME; +import static org.openmetadata.service.Entity.FIELD_REVIEWERS; import static org.openmetadata.service.Entity.TAG; import static org.openmetadata.service.Entity.TEAM; import static org.openmetadata.service.exception.CatalogExceptionMessage.notReviewer; @@ -249,6 +250,23 @@ public void setInheritedFields(Tag tag, Fields fields) { } } + /** + * A tag with no reviewers of its own is still governed by its classification's, so approval gates + * must resolve the chain explicitly rather than trust the read-time {@code reviewers} field — + * {@link #setInheritedFields(Tag, Fields)} silently skips inheritance when the classification + * cannot be loaded. + */ + @Override + public List getEffectiveReviewers(Tag tag) { + List reviewers = listOrEmpty(tag.getReviewers()); + if (reviewers.isEmpty() && tag.getClassification() != null) { + Classification classification = + Entity.getEntity(tag.getClassification(), FIELD_REVIEWERS, NON_DELETED); + reviewers = listOrEmpty(classification.getReviewers()); + } + return reviewers; + } + @Override public void setInheritedFields(List tags, Fields fields) { if (tags == null || tags.isEmpty()) { From 5b3d8e41e4a32f424bf53df2c2a88200e019729c Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 11:34:22 -0700 Subject: [PATCH 02/13] refactor(governance): resolve effective reviewers generically, not per entity type Replace the GlossaryTermRepository and TagRepository overrides with a single generic implementation on EntityRepository that walks getParentEntity -- the same hook the inheritance path already uses -- so any entity declaring an inheritance parent participates without type-specific code. GlossaryTermRepository already implements that hook as "parent term, else glossary", which is exactly the walk the removed override duplicated. Reading the parent applies the parent's own inheritance, so a glossary's reviewers still surface for a term nested under an intermediate term that has none. Covered by a new grandparent test. Resolution now goes through resolveInheritanceParentLeniently, which swallows EntityNotFoundException. This fixes a regression the previous commit introduced: the per-type lookups used Entity.getEntity(..., NON_DELETED), which throws when the parent is missing or soft-deleted, turning a previously silent skip into a BpmnError that aborted the approval gate and left the term with no task at all. TagRepository is reverted to its original state. Covering tags generically would require overriding getParentEntity there, which also feeds create-permission evaluation in CreateResourceContext -- a behaviour change out of scope here. Tags keep their existing read-path inheritance; their setInheritedFields still swallows exceptions and silently skips inheritance, which is worth addressing separately. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...ossaryTermInheritedReviewerApprovalIT.java | 20 ++++++++++++++++ .../service/jdbi3/EntityRepository.java | 23 +++++++++++++++---- .../service/jdbi3/GlossaryTermRepository.java | 10 -------- .../service/jdbi3/TagRepository.java | 18 --------------- 4 files changed, 38 insertions(+), 33 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java index f36986009cd5..e6b3414cfb04 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java @@ -166,6 +166,26 @@ void test_inheritedReviewerFromParentTerm_createsOpenApprovalTask(TestNamespace waitForTermStatus(child.getId(), EntityStatus.IN_REVIEW); } + /** + * Grandparent inheritance: the GLOSSARY holds the reviewer, an intermediate term holds none, and + * the leaf holds none. Effective-reviewer resolution takes a single hop to the parent term, so this + * only succeeds if reading that parent applies its own inheritance and surfaces the glossary's + * reviewer — the assumption the generic resolution rests on. + */ + @Test + void test_inheritedReviewerFromGrandparentGlossary_createsOpenApprovalTask(TestNamespace ns) + throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ true); + GlossaryTerm middle = createTerm(glossary, "middle_no_reviewer", false); + GlossaryTerm leaf = createChildTerm(glossary, middle, "leaf_inherits_from_glossary"); + + assertTermInheritsReviewer(leaf.getId()); + + Task task = awaitOpenApprovalTask(leaf.getId(), leaf.getFullyQualifiedName()); + assertAssigneeIsReviewer(task); + waitForTermStatus(leaf.getId(), EntityStatus.IN_REVIEW); + } + /** * Faithful shape of the reported "hello world" term: an OWNER and a description are set, and the * reviewer is inherited from the glossary (none on the term). This isolates whether an owner on the 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 9700952dac92..91ceb9802850 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 @@ -7866,16 +7866,29 @@ public final void inheritReviewers(T entity, Fields fields, EntityInterface pare /** * Reviewers that govern this entity's approval: those set directly on it, or — when it has none — - * those it inherits from its parent. Approval decisions must use this rather than the raw {@code + * those of its inheritance parent. Approval decisions must use this rather than the raw {@code * reviewers} field, which carries inherited entries only when the read that produced the entity - * both requested and applied inheritance. Subclasses whose entities inherit reviewers override it. + * both requested and applied inheritance. + * + *

Resolution goes through {@link #getParentEntity}, the same hook the inheritance path uses, so + * any entity declaring an inheritance parent participates without type-specific code. Reading the + * parent applies the parent's own inheritance, so a grandparent's reviewers (a glossary's, for a + * term nested under another term) surface through the single hop. A parent that is missing or + * concurrently deleted degrades to "no reviewers" instead of failing the caller. */ - public List getEffectiveReviewers(T entity) { - return listOrEmpty(entity.getReviewers()); + public final List getEffectiveReviewers(T entity) { + List reviewers = listOrEmpty(entity.getReviewers()); + if (reviewers.isEmpty() && supportsReviewers) { + EntityInterface parent = resolveInheritanceParentLeniently(entity, FIELD_REVIEWERS); + if (parent != null) { + reviewers = listOrEmpty(parent.getReviewers()); + } + } + return reviewers; } @SuppressWarnings("unchecked") - public List getEffectiveReviewersUntyped(EntityInterface entity) { + public final List getEffectiveReviewersUntyped(EntityInterface entity) { return getEffectiveReviewers((T) entity); } 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 1a4b29d7fff1..d5f2c7508e9d 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 @@ -1779,16 +1779,6 @@ protected void updateTaskWithNewReviewers(GlossaryTerm term) { term.getUpdatedBy()); } - /** - * A term with no reviewers of its own is still governed by its parent term's — or its glossary's — - * reviewers, so approval gates must resolve the chain explicitly instead of trusting the read-time - * {@code reviewers} field. - */ - @Override - public List getEffectiveReviewers(GlossaryTerm glossaryTerm) { - return resolveEffectiveReviewers(glossaryTerm); - } - private List resolveEffectiveReviewers(GlossaryTerm term) { if (!nullOrEmpty(term.getReviewers())) { return term.getReviewers(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java index 37b2b5677815..66de0bc8489f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java @@ -20,7 +20,6 @@ import static org.openmetadata.service.Entity.CLASSIFICATION; import static org.openmetadata.service.Entity.FIELD_CERTIFICATION; import static org.openmetadata.service.Entity.FIELD_NAME; -import static org.openmetadata.service.Entity.FIELD_REVIEWERS; import static org.openmetadata.service.Entity.TAG; import static org.openmetadata.service.Entity.TEAM; import static org.openmetadata.service.exception.CatalogExceptionMessage.notReviewer; @@ -250,23 +249,6 @@ public void setInheritedFields(Tag tag, Fields fields) { } } - /** - * A tag with no reviewers of its own is still governed by its classification's, so approval gates - * must resolve the chain explicitly rather than trust the read-time {@code reviewers} field — - * {@link #setInheritedFields(Tag, Fields)} silently skips inheritance when the classification - * cannot be loaded. - */ - @Override - public List getEffectiveReviewers(Tag tag) { - List reviewers = listOrEmpty(tag.getReviewers()); - if (reviewers.isEmpty() && tag.getClassification() != null) { - Classification classification = - Entity.getEntity(tag.getClassification(), FIELD_REVIEWERS, NON_DELETED); - reviewers = listOrEmpty(classification.getReviewers()); - } - return reviewers; - } - @Override public void setInheritedFields(List tags, Fields fields) { if (tags == null || tags.isEmpty()) { From 22d0be9d395ea0e32331c38766823cd1cf29de71 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 11:48:53 -0700 Subject: [PATCH 03/13] test(governance): unit-test the reviewers gate; rename buildRuleData to buildRuleContext Add CheckEntityAttributesImplTest, which drives the delegate against the rule GlossaryApprovalWorkflow.json actually ships and asserts the node result. Unlike the integration tests, the unit level can force the failing condition directly -- an entity read that returns no reviewers on the term while the parent supplies them -- so reviewersRule_inheritedReviewersOnly_evaluatesTrue is a genuine fail-without-fix test: reverting the gate to JsonUtils.getMap(entity) fails it with "expected: but was: ", the exact false that routes a term to Draft. Also covers direct reviewers, no reviewers anywhere, an inherited reference without an FQN, and an entity type that does not support reviewers. Rename buildRuleData to buildRuleContext: RuleEngine.apply names its parameter `context`, so the helper now matches the vocabulary of the API it feeds. The Map return type is required -- RuleEngine.apply takes a Map, and every other call site passes JsonUtils.getMap(entity). Applying the reviewers override to that map rather than to the entity is deliberate and now documented: getMap already returns a detached copy, while the entity may be request-cached. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../impl/CheckEntityAttributesImpl.java | 10 +- .../impl/CheckEntityAttributesImplTest.java | 223 ++++++++++++++++++ 2 files changed, 230 insertions(+), 3 deletions(-) create mode 100644 openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java index 1d791d13eb63..fce066060343 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java @@ -62,7 +62,7 @@ private Boolean checkAttributes( result = (boolean) RuleEngine.getInstance() - .apply(rules, buildRuleData(entityLink.getEntityType(), entity)); + .apply(rules, buildRuleContext(entityLink.getEntityType(), entity)); } catch (Exception e) { throw new RuntimeException(e); } @@ -75,9 +75,13 @@ private Boolean checkAttributes( * must answer yes, otherwise no approval task is ever created and the term settles in Draft. The * raw {@code reviewers} field carries inherited entries only when the read that produced the entity * applied inheritance, so resolve them explicitly here, the same way the approval-task assignee node - * does. The entity is left untouched because it may be request-cached; only the rule input changes. + * does. + * + *

The override is applied to the rule context rather than to the entity: {@link + * JsonUtils#getMap} already returns a detached copy, so overriding a key on it is free, whereas the + * entity may be request-cached and mutating it would leak into other reads in the same request. */ - private Map buildRuleData(String entityType, EntityInterface entity) { + private Map buildRuleContext(String entityType, EntityInterface entity) { Map ruleData = JsonUtils.getMap(entity); EntityRepository repository = Entity.getEntityRepository(entityType); if (repository.isSupportsReviewers() && nullOrEmpty(entity.getReviewers())) { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java new file mode 100644 index 000000000000..1e20a602d397 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java @@ -0,0 +1,223 @@ +/* + * Copyright 2024 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.service.governance.workflows.elements.nodes.automatedTask.impl; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.flowable.common.engine.api.delegate.Expression; +import org.flowable.engine.delegate.DelegateExecution; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.openmetadata.schema.entity.data.GlossaryTerm; +import org.openmetadata.schema.type.EntityReference; +import org.openmetadata.schema.type.Include; +import org.openmetadata.service.Entity; +import org.openmetadata.service.jdbi3.EntityRepository; +import org.openmetadata.service.resources.feeds.MessageParser; + +/** + * Covers the reviewers gate of the Glossary Approval Workflow. + * + *

{@code CheckGlossaryTermHasReviewers} decides whether an approval task is created at all. It + * evaluates the shipped JsonLogic rule below against the term, and routes a {@code false} result to a + * terminal status. A term whose reviewers are inherited from its parent glossary must still + * answer {@code true}; otherwise no approval task is ever created and the term settles in Draft. + * + *

These tests pin that behaviour at the unit level, where the failing condition can be forced + * directly: an entity read that returns no reviewers on the term while the parent supplies + * them. {@link #reviewersRule_inheritedReviewersOnly_evaluatesTrue()} fails without the + * effective-reviewer resolution in {@code CheckEntityAttributesImpl} and passes with it. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +class CheckEntityAttributesImplTest { + + /** The rule shipped in GlossaryApprovalWorkflow.json for CheckGlossaryTermHasReviewers. */ + private static final String HAS_REVIEWERS_RULE = + "{\"and\":[{\"some\":[{\"var\":\"reviewers\"},{\"!=\":[{\"var\":\"fullyQualifiedName\"},null]}]}]}"; + + private static final String NODE_ID = "CheckGlossaryTermHasReviewers"; + private static final String RESULT_KEY = NODE_ID + "_result"; + + @Mock private DelegateExecution execution; + @Mock private Expression rulesExpr; + @Mock private Expression inputNamespaceMapExpr; + + @SuppressWarnings("rawtypes") + @Mock + private EntityRepository mockRepository; + + private CheckEntityAttributesImpl delegate; + private MockedStatic mockedEntity; + private Map capturedVars; + + @BeforeEach + void setUp() throws Exception { + delegate = new CheckEntityAttributesImpl(); + injectField(delegate, "rulesExpr", rulesExpr); + injectField(delegate, "inputNamespaceMapExpr", inputNamespaceMapExpr); + + when(inputNamespaceMapExpr.getValue(execution)).thenReturn("{\"relatedEntity\":\"global\"}"); + when(rulesExpr.getValue(execution)).thenReturn(HAS_REVIEWERS_RULE); + when(execution.getProcessDefinitionId()).thenReturn("GlossaryTermApprovalWorkflow:1:1"); + when(execution.getCurrentActivityId()).thenReturn(NODE_ID); + when(execution.getVariable("global_relatedEntity")) + .thenReturn("<#E::glossaryTerm::Property.hello world>"); + when(mockRepository.isSupportsReviewers()).thenReturn(true); + + mockedEntity = mockStatic(Entity.class); + mockedEntity.when(() -> Entity.getEntityRepository(anyString())).thenReturn(mockRepository); + + capturedVars = new HashMap<>(); + doAnswer( + invocation -> { + capturedVars.put(invocation.getArgument(0), invocation.getArgument(1)); + return null; + }) + .when(execution) + .setVariable(anyString(), any()); + } + + @AfterEach + void tearDown() { + mockedEntity.close(); + } + + /** + * The reported bug. The term carries no reviewers of its own — the read returned none — but its + * glossary supplies one, so effective-reviewer resolution must make the gate answer true. Without + * that resolution the rule sees an empty array, {@code some} is vacuously false, and the term is + * routed to Draft with no approval task. + */ + @Test + void reviewersRule_inheritedReviewersOnly_evaluatesTrue() { + givenRelatedEntity(termWithReviewers(null)); + givenEffectiveReviewers(reviewer("manoj")); + + delegate.execute(execution); + + assertTrue( + result(), + "Gate must see the inherited reviewer and allow the approval task to be created; " + + "a false result here is what leaves the term in Draft"); + } + + /** A reviewer set directly on the term is unaffected by the resolution. */ + @Test + void reviewersRule_directReviewersOnTerm_evaluatesTrue() { + givenRelatedEntity(termWithReviewers(List.of(reviewer("manoj")))); + + delegate.execute(execution); + + assertTrue(result(), "A directly attached reviewer must satisfy the gate"); + } + + /** Nothing to inherit anywhere: the gate must still answer false and auto-approve. */ + @Test + void reviewersRule_noReviewersAnywhere_evaluatesFalse() { + givenRelatedEntity(termWithReviewers(null)); + givenEffectiveReviewers(); + + delegate.execute(execution); + + assertFalse(result(), "With no reviewers on the term or its parents the gate must be false"); + } + + /** + * A reviewer reference without a fullyQualifiedName does not satisfy the shipped rule, which tests + * {@code fullyQualifiedName != null} per element. Resolution must not paper over that. + */ + @Test + void reviewersRule_inheritedReviewerWithoutFqn_evaluatesFalse() { + givenRelatedEntity(termWithReviewers(null)); + givenEffectiveReviewers(new EntityReference().withType(Entity.USER)); + + delegate.execute(execution); + + assertFalse(result(), "A reviewer reference with no FQN must not satisfy the rule"); + } + + /** + * Resolution must not be attempted for entities that do not support reviewers, and the gate must + * fall back to whatever the entity itself carries. + */ + @Test + void reviewersRule_entityWithoutReviewerSupport_evaluatesFalse() { + when(mockRepository.isSupportsReviewers()).thenReturn(false); + givenRelatedEntity(termWithReviewers(null)); + + delegate.execute(execution); + + assertFalse(result(), "An entity type without reviewer support must not satisfy the gate"); + assertEquals(1, capturedVars.size(), "Only the node result should be written"); + } + + private void givenRelatedEntity(GlossaryTerm term) { + mockedEntity + .when( + () -> + Entity.getEntity( + any(MessageParser.EntityLink.class), anyString(), any(Include.class))) + .thenReturn(term); + } + + @SuppressWarnings("unchecked") + private void givenEffectiveReviewers(EntityReference... reviewers) { + when(mockRepository.getEffectiveReviewersUntyped(any())).thenReturn(List.of(reviewers)); + } + + private GlossaryTerm termWithReviewers(List reviewers) { + return new GlossaryTerm() + .withName("hello world") + .withFullyQualifiedName("Property.hello world") + .withDescription("hello world") + .withReviewers(reviewers); + } + + private EntityReference reviewer(String name) { + return new EntityReference() + .withType(Entity.USER) + .withName(name) + .withFullyQualifiedName(name) + .withInherited(true); + } + + private boolean result() { + return Boolean.TRUE.equals(capturedVars.get(RESULT_KEY)); + } + + private static void injectField(Object target, String fieldName, Object value) throws Exception { + Field field = target.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + } +} From f4a39d97e4c81e80a35fb618672a02bd4e190cde Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 12:45:50 -0700 Subject: [PATCH 04/13] test(glossary): add inherited-reviewer approval repro for the stale parent cache Adds a Playwright spec and an integration test that drive the ordering the bug needs: the glossary is created with NO reviewers, a first term runs the approval workflow (populating EntityRepository.inheritanceParentCache on a workflow executor thread), and only then is the reviewer added. Every term created afterwards must still be gated as "has reviewers". That cache is a static ThreadLocal cleared only by ImpersonationCleanupFilter, a JAX-RS response filter, so workflow threads never clear it and can serve a reviewer-less glossary snapshot indefinitely. A server restart clears it, which matches the reported recovery. Both tests currently PASS in-process: the embedded integration environment does not keep a poisoned executor thread alive across the ordering, so the sequence alone does not reproduce there. They stand as regression guards, and the Playwright spec is the one that exercises a real deployed stack where the executor threads are long lived. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...ossaryTermInheritedReviewerApprovalIT.java | 45 ++++ .../GlossaryInheritedReviewerApproval.spec.ts | 244 ++++++++++++++++++ 2 files changed, 289 insertions(+) create mode 100644 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java index e6b3414cfb04..b59c1ca4b5e3 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java @@ -93,6 +93,9 @@ public class GlossaryTermInheritedReviewerApprovalIT { private static final Duration STATUS_TIMEOUT = Duration.ofMinutes(3); private static final Duration POLL_INTERVAL = Duration.ofSeconds(2); + /** Terms created after the reviewer is added, to land on a poisoned executor thread. */ + private static final int POISONED_THREAD_PROBE_COUNT = 6; + protected SharedEntities shared() { return SharedEntities.get(); } @@ -166,6 +169,48 @@ void test_inheritedReviewerFromParentTerm_createsOpenApprovalTask(TestNamespace waitForTermStatus(child.getId(), EntityStatus.IN_REVIEW); } + /** + * Stale inheritance-parent cache. {@code EntityRepository.inheritanceParentCache} is a static + * ThreadLocal cleared only by {@code ImpersonationCleanupFilter}, a JAX-RS response filter — so on + * Flowable's async job-executor threads it is never cleared for the life of the process. + * + *

This drives the reported ordering: the glossary starts with NO reviewers, a first term is + * created so the workflow caches that reviewer-less glossary snapshot on a job thread, and only then + * is the reviewer added. Every term created afterwards must still be gated as "has reviewers"; a + * term that lands in a terminal status instead means a job thread served the stale parent. + * + *

Several terms are created because the executor has a pool of threads and only the poisoned ones + * exhibit the bug — one term could be routed to a clean thread and pass by luck. + */ + @Test + void test_reviewerAddedToGlossaryAfterFirstTerm_stillGatesLaterTerms(TestNamespace ns) + throws Exception { + Glossary glossary = createGlossary(ns, /* withReviewer */ false); + + // Poison: this term's workflow caches the glossary parent while it has no reviewers. + GlossaryTerm seed = createTerm(glossary, "seed_before_reviewer", false); + waitForTermStatus(seed.getId(), EntityStatus.APPROVED); + + addReviewerToGlossary(glossary.getId()); + + for (int i = 0; i < POISONED_THREAD_PROBE_COUNT; i++) { + GlossaryTerm term = createTerm(glossary, "after_reviewer_" + i, false); + assertTermInheritsReviewer(term.getId()); + awaitOpenApprovalTask(term.getId(), term.getFullyQualifiedName()); + } + } + + private void addReviewerToGlossary(UUID glossaryId) throws Exception { + String patch = + String.format( + "[{\"op\":\"add\",\"path\":\"/reviewers\",\"value\":" + + "[{\"id\":\"%s\",\"type\":\"user\"}]}]", + reviewer().getId()); + SdkClients.adminClient() + .glossaries() + .patch(glossaryId.toString(), new ObjectMapper().readTree(patch)); + } + /** * Grandparent inheritance: the GLOSSARY holds the reviewer, an intermediate term holds none, and * the leaf holds none. Effective-reviewer resolution takes a single hop to the parent term, so this diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts new file mode 100644 index 000000000000..f56cf1ca33b5 --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -0,0 +1,244 @@ +/* + * Copyright 2024 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { APIRequestContext, expect, test } from '@playwright/test'; +import { Glossary } from '../../../support/glossary/Glossary'; +import { GlossaryTerm } from '../../../support/glossary/GlossaryTerm'; +import { UserClass } from '../../../support/user/UserClass'; +import { performAdminLogin } from '../../../utils/admin'; +import { redirectToHomePage, uuid } from '../../../utils/common'; + +/** + * Reproduction for the Glossary Approval bug where a term whose reviewers are INHERITED from its + * glossary is treated as having none: no approval task is created and the term settles in Draft. + * + * The ordering matters. The approval workflow resolves inherited fields through + * `EntityRepository.inheritanceParentCache`, a static ThreadLocal cleared only by + * `ImpersonationCleanupFilter` — a JAX-RS response filter. Workflow execution threads never pass + * through that filter, so a glossary snapshot captured while it had NO reviewers can be served + * indefinitely. These tests therefore create the glossary WITHOUT reviewers first, let a term run the + * workflow (populating that cache), and only then add the reviewer. Every term created afterwards + * must still be gated as "has reviewers". + * + * Terms are created in a batch because the executor uses a thread pool and only threads that cached + * the stale parent misbehave — a single term could be routed to a clean thread and pass by luck. + */ + +const REVIEWER_ADDED_PROBE_COUNT = 6; +const STATUS_TIMEOUT = 120_000; + +const reviewer = new UserClass(); +const glossary = new Glossary(); + +type TermStatus = 'Draft' | 'In Review' | 'Approved' | 'Rejected' | string; + +const createTermWithoutReviewers = async ( + apiContext: APIRequestContext, + glossaryFqn: string, + name: string +) => { + const response = await apiContext.post('/api/v1/glossaryTerms', { + data: { + name, + displayName: name, + // A non-empty description is required to pass the "ready to be reviewed" gate. + description: 'Term used by the inherited-reviewer approval reproduction', + glossary: glossaryFqn, + }, + }); + + expect(response.status()).toBe(201); + + return response.json(); +}; + +/** + * Polls the term until the workflow has settled it. The workflow runs asynchronously, so the term is + * briefly `Unprocessed`/`Draft` before the gate resolves; we wait for a status the workflow actually + * commits to rather than sampling once and racing it. + */ +const waitForSettledStatus = async ( + apiContext: APIRequestContext, + termId: string +): Promise => { + let status: TermStatus = ''; + + await expect + .poll( + async () => { + const response = await apiContext.get( + `/api/v1/glossaryTerms/${termId}?fields=reviewers` + ); + const term = await response.json(); + status = term.entityStatus; + + return status; + }, + { + message: `Glossary term ${termId} never reached a settled workflow status`, + timeout: STATUS_TIMEOUT, + } + ) + .toMatch(/In Review|Approved|Rejected|Draft/); + + return status; +}; + +const getOpenApprovalTaskCount = async ( + apiContext: APIRequestContext, + termFqn: string +) => { + const response = await apiContext.get( + `/api/v1/tasks?limit=100&status=Open&aboutEntity=${encodeURIComponent( + termFqn + )}` + ); + const body = await response.json(); + + return (body.data ?? []).length; +}; + +test.describe( + 'Glossary Approval - inherited reviewers', + { tag: ['@Features', '@Governance'] }, + () => { + test.beforeAll( + 'Setup reviewer and reviewer-less glossary', + async ({ browser }) => { + const { apiContext, afterAction } = await performAdminLogin(browser); + + await reviewer.create(apiContext); + // Deliberately created with NO reviewers so the first term's workflow caches a + // reviewer-less snapshot of this glossary. + await glossary.create(apiContext); + + await afterAction(); + } + ); + + test.afterAll('Cleanup', async ({ browser }) => { + const { apiContext, afterAction } = await performAdminLogin(browser); + + await glossary.delete(apiContext); + await reviewer.delete(apiContext); + + await afterAction(); + }); + + test('term inherits reviewer added to the glossary after an earlier term ran the workflow', async ({ + browser, + }) => { + test.slow(); + + const { apiContext, afterAction } = await performAdminLogin(browser); + const glossaryFqn = glossary.responseData.fullyQualifiedName; + + await test.step('A term created before any reviewer exists is auto-approved', async () => { + const seed = await createTermWithoutReviewers( + apiContext, + glossaryFqn, + `seed_before_reviewer_${uuid()}` + ); + + const status = await waitForSettledStatus(apiContext, seed.id); + + expect(status).toBe('Approved'); + }); + + await test.step('Add a reviewer to the glossary', async () => { + const response = await apiContext.patch( + `/api/v1/glossaries/${glossary.responseData.id}`, + { + data: [ + { + op: 'add', + path: '/reviewers', + value: [{ id: reviewer.responseData.id, type: 'user' }], + }, + ], + headers: { 'Content-Type': 'application/json-patch+json' }, + } + ); + + expect(response.status()).toBe(200); + }); + + await test.step('Every term created afterwards inherits the reviewer and gets an approval task', async () => { + for (let index = 0; index < REVIEWER_ADDED_PROBE_COUNT; index++) { + const term = await createTermWithoutReviewers( + apiContext, + glossaryFqn, + `after_reviewer_${index}_${uuid()}` + ); + + const status = await waitForSettledStatus(apiContext, term.id); + + expect( + status, + `Term ${term.name} inherits a reviewer from the glossary, so the approval workflow ` + + `must move it to "In Review". "Draft" means the reviewers gate saw none — the ` + + `inherited-reviewer bug.` + ).toBe('In Review'); + + expect( + await getOpenApprovalTaskCount(apiContext, term.fullyQualifiedName), + `Term ${term.name} must have an open approval task for the inherited reviewer` + ).toBeGreaterThan(0); + } + }); + + await afterAction(); + }); + + test('inherited reviewer is shown on the term page and it is not left in Draft', async ({ + browser, + page, + }) => { + test.slow(); + + const { apiContext, afterAction } = await performAdminLogin(browser); + // Built from a Glossary constructed without reviewers, so the term is created with an empty + // reviewers list and must inherit the one added to the glossary server-side. + const term = new GlossaryTerm(glossary); + await term.create(apiContext); + + await waitForSettledStatus(apiContext, term.responseData.id); + await afterAction(); + + await redirectToHomePage(page); + await term.visitEntityPage(page); + + await expect(page.locator('[data-testid="loader"]')).toHaveCount(0); + + await test.step('Inherited reviewer is displayed', async () => { + await expect( + page.getByTestId('glossary-reviewer').getByTestId('owner-link') + ).toContainText( + reviewer.responseData.displayName ?? reviewer.responseData.name + ); + }); + + await test.step('Term reached In Review, not Draft', async () => { + await expect(page.getByTestId('status-badge')).toContainText( + 'In Review' + ); + }); + + await test.step('Cleanup term', async () => { + const { apiContext: cleanupContext, afterAction: cleanupAction } = + await performAdminLogin(browser); + await term.delete(cleanupContext); + await cleanupAction(); + }); + }); + } +); From 803630af06c7de4558126c47b907a7922bf93400 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 13:54:16 -0700 Subject: [PATCH 05/13] fix(governance): resolve inherited fields freshly instead of from a never-cleared cache Glossary terms whose reviewers were inherited from their glossary were judged to have none: the approval workflow's reviewers gate evaluated false, no approval task was created, and the term settled in Draft. Restarting the server fixed it. EntityRepository.inheritanceParentCache was a static ThreadLocal with no TTL, no size bound and no eviction, cleared only by ImpersonationCleanupFilter -- a JAX-RS response filter, so only on HTTP request threads. The approval workflow does not run on one: a change event reaches WorkflowEventConsumer on a Quartz thread and signals the process synchronously, so the filter, the attribute gates and the status transition all execute there. That thread never cleared the cache, so a glossary read before its reviewer was added was served for the life of the pod. Being a ThreadLocal it also cannot be evicted from the thread that performs the write, which is why no invalidation existed. It affected every inheritable field -- owners and domains as well as reviewers. Delete it. Parent lookups now fall through to the Guava L1 and the Redis L2, which are invalidated inline on write and across nodes via CacheInvalidationPubSub; with Redis disabled they fall through to the database. The batch path still issues one bulk load per parent type, so list endpoints do not regress to an N+1 -- the cost lands on repeated single-entity reads within one request. Its superset-field matching also disappears, which only TableRepository and DatabaseSchemaRepository relied on; narrower is a strict subset, so correctness is unchanged. Two threads that run entity reads still never cleared the remaining per-request ThreadLocals, so PerRequestContextCleaner now centralises that set and is called from the Quartz consumer tick and, via WorkflowThreadCleanupListener, at Flowable job completion. Workflow reads additionally open a FreshReadScope, which forces fromCache=false. Their reads are gating decisions where a stale answer silently routes an entity to the wrong terminal status, and cross-node L1 invalidation requires Redis; workflow volume is low, so the fresh read is the cheaper side of that trade. This makes the reviewers-only getEffectiveReviewers helper added earlier on this branch redundant, so it is reverted: it covered only reviewers and replaced the list rather than merging own and inherited entries the way inheritance does. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../service/OpenMetadataApplication.java | 6 +- .../changeEvent/AbstractEventConsumer.java | 8 + .../governance/workflows/WorkflowHandler.java | 25 ++- .../WorkflowThreadCleanupListener.java | 61 ++++++ .../impl/CheckEntityAttributesImpl.java | 36 +--- .../jdbi3/DatabaseSchemaRepository.java | 2 +- .../service/jdbi3/EntityRepository.java | 198 ++++-------------- .../service/jdbi3/TableRepository.java | 2 +- .../security/ImpersonationCleanupFilter.java | 14 +- .../service/util/FreshReadScope.java | 61 ++++++ .../util/PerRequestContextCleaner.java | 46 ++++ .../WorkflowThreadCleanupListenerTest.java | 84 ++++++++ .../impl/CheckEntityAttributesImplTest.java | 84 ++------ ...EntityRepositoryInheritanceParentTest.java | 174 +++++++++++++++ 14 files changed, 521 insertions(+), 280 deletions(-) create mode 100644 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java create mode 100644 openmetadata-service/src/main/java/org/openmetadata/service/util/FreshReadScope.java create mode 100644 openmetadata-service/src/main/java/org/openmetadata/service/util/PerRequestContextCleaner.java create mode 100644 openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListenerTest.java create mode 100644 openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryInheritanceParentTest.java diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java b/openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java index 6cb36a62dd11..a982b69c7dab 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java @@ -397,9 +397,9 @@ public void run(OpenMetadataApplicationConfig catalogConfig, Environment environ environment.jersey().register(ETagRequestFilter.class); environment.jersey().register(ETagResponseFilter.class); - // Clears per-request ThreadLocals (inheritanceParentCache, ReadBundleContext, - // RequestEntityCache, impersonation context) after every response so state - // cannot leak across requests that share a Jetty worker thread. + // Clears per-request ThreadLocals (ReadBundleContext, RequestEntityCache, impersonation + // context) after every response so state cannot leak across requests that share a Jetty + // worker thread. Non-HTTP pools clear the same set via PerRequestContextCleaner. environment.jersey().register(ImpersonationCleanupFilter.class); // Register User Activity Tracking diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java b/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java index 39978464c333..79d46ad4bda8 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java @@ -46,6 +46,7 @@ import org.openmetadata.service.notifications.recipients.RecipientResolver; import org.openmetadata.service.notifications.recipients.context.Recipient; import org.openmetadata.service.util.DIContainer; +import org.openmetadata.service.util.PerRequestContextCleaner; import org.quartz.DisallowConcurrentExecution; import org.quartz.Job; import org.quartz.JobDetail; @@ -490,6 +491,11 @@ record CursorPlan(long offset, long pendingGapSince, int recordCount, boolean sk @Override public void execute(JobExecutionContext jobExecutionContext) { + // Quartz worker threads are long lived and never pass through the JAX-RS response filter, so + // per-request ThreadLocal caches would otherwise persist across ticks and serve indefinitely + // stale reads. Destinations on this thread read entities (governance workflows resolve + // inherited reviewers here), so start each tick from a clean slate. + PerRequestContextCleaner.clear(); this.init(jobExecutionContext); if (this.eventSubscription == null) { LOG.error("Skipping job execution - EventSubscription could not be loaded"); @@ -520,6 +526,8 @@ public void execute(JobExecutionContext jobExecutionContext) { } else if (gapStateChanged) { persistPendingGapState(jobExecutionContext); } + // Bound retention while this worker sits idle between ticks. + PerRequestContextCleaner.clear(); } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowHandler.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowHandler.java index d84a3262ce87..3a5300b5dc5d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowHandler.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowHandler.java @@ -79,6 +79,7 @@ import org.openmetadata.service.jdbi3.WorkflowInstanceStateRepository; import org.openmetadata.service.jdbi3.locator.ConnectionType; import org.openmetadata.service.resources.services.ingestionpipelines.IngestionPipelineMapper; +import org.openmetadata.service.util.FreshReadScope; @Slf4j public class WorkflowHandler { @@ -283,8 +284,9 @@ public void initializeNewProcessEngine( // Add Expression Manager processEngineConfiguration.setExpressionManager(new DefaultExpressionManager(expressionMap)); - // Add Global Failure Listener - processEngineConfiguration.setEventListeners(List.of(new WorkflowFailureListener())); + // Add Global Failure Listener + per-job ThreadLocal cleanup for the async executor pool + processEngineConfiguration.setEventListeners( + List.of(new WorkflowFailureListener(), new WorkflowThreadCleanupListener())); boolean engineBuilt = false; try { @@ -698,9 +700,17 @@ public ProcessInstance triggerByKey( } } + /** + * Signals are delivered synchronously, so the whole workflow — filters, attribute gates, status + * transitions — runs inline on the caller's thread. Those gates decide whether an entity gets an + * approval task at all, so they read fresh rather than trusting an in-process cache that a write on + * another node may not have invalidated. + */ public void triggerWithSignal(String signal, Map variables) { RuntimeService runtimeService = processEngine.getRuntimeService(); - runtimeService.signalEventReceived(signal, variables); + try (FreshReadScope.Handle ignored = FreshReadScope.enter()) { + runtimeService.signalEventReceived(signal, variables); + } } private void unlockJobsOnStartup() { @@ -913,6 +923,15 @@ public boolean resolveLegacyThreadTask(UUID customTaskId, Map va private boolean resolveTaskInternal( UUID customTaskId, Map variables, boolean legacyThreadTask) { + // Completing a user task continues the workflow inline, re-running the attribute gates that + // decide the entity's next status — same freshness requirement as triggerWithSignal. + try (FreshReadScope.Handle ignored = FreshReadScope.enter()) { + return resolveTaskWithFreshReads(customTaskId, variables, legacyThreadTask); + } + } + + private boolean resolveTaskWithFreshReads( + UUID customTaskId, Map variables, boolean legacyThreadTask) { TaskService taskService = processEngine.getTaskService(); LOG.debug("[WorkflowTask] RESOLVE: customTaskId='{}' variables={}", customTaskId, variables); // Admission control: bound how many resolutions touch Flowable at once so an approval burst diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java new file mode 100644 index 000000000000..21b8ca6f6363 --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java @@ -0,0 +1,61 @@ +/* + * Copyright 2024 Collate + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.service.governance.workflows; + +import org.flowable.common.engine.api.delegate.event.FlowableEvent; +import org.flowable.common.engine.api.delegate.event.FlowableEventListener; +import org.openmetadata.service.util.PerRequestContextCleaner; + +/** + * Clears per-request ThreadLocal caches once an async job finishes. + * + *

Flowable's async-executor threads are pooled and long lived, and never pass through the JAX-RS + * response filter that clears these ThreadLocals for HTTP requests. Without this, a parent entity + * read by one job is served to every later job on the same thread for the life of the process. + * + *

Only the job-completion events are handled: both are dispatched on the async-executor thread + * itself, after the job's delegates have finished, which mirrors the response filter's "clear once + * the unit of work completes" semantics. Note the synchronous workflow path (a change event + * signalling a process inline) does not produce these events — that thread is cleaned by {@code + * AbstractEventConsumer}. + */ +public class WorkflowThreadCleanupListener implements FlowableEventListener { + + @Override + public void onEvent(FlowableEvent event) { + // Registered engine-wide, so every event lands here: keep the check cheap and first. + switch (event.getType().name()) { + case "JOB_EXECUTION_SUCCESS", "JOB_EXECUTION_FAILURE" -> PerRequestContextCleaner.clear(); + default -> { + /* not a job boundary */ + } + } + } + + /** Cleanup must never fail the job that triggered it. */ + @Override + public boolean isFailOnException() { + return false; + } + + @Override + public boolean isFireOnTransactionLifecycleEvent() { + return false; + } + + @Override + public String getOnTransaction() { + return null; + } +} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java index fce066060343..999dda28fbe3 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java @@ -1,15 +1,11 @@ package org.openmetadata.service.governance.workflows.elements.nodes.automatedTask.impl; -import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; -import static org.openmetadata.service.Entity.FIELD_REVIEWERS; import static org.openmetadata.service.governance.workflows.Workflow.EXCEPTION_VARIABLE; import static org.openmetadata.service.governance.workflows.Workflow.RELATED_ENTITY_VARIABLE; import static org.openmetadata.service.governance.workflows.Workflow.RESULT_VARIABLE; import static org.openmetadata.service.governance.workflows.Workflow.WORKFLOW_RUNTIME_EXCEPTION; import static org.openmetadata.service.governance.workflows.WorkflowHandler.getProcessDefinitionKeyFromId; -import java.util.List; -import java.util.Map; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.exception.ExceptionUtils; import org.flowable.common.engine.api.delegate.Expression; @@ -17,13 +13,10 @@ import org.flowable.engine.delegate.DelegateExecution; import org.flowable.engine.delegate.JavaDelegate; import org.openmetadata.schema.EntityInterface; -import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.Include; import org.openmetadata.schema.utils.JsonUtils; -import org.openmetadata.service.Entity; import org.openmetadata.service.governance.workflows.WorkflowVariableHandler; import org.openmetadata.service.governance.workflows.WorkflowVariableHandler.InputNamespaces; -import org.openmetadata.service.jdbi3.EntityRepository; import org.openmetadata.service.resources.feeds.MessageParser; import org.openmetadata.service.rules.RuleEngine; @@ -59,37 +52,10 @@ private Boolean checkAttributes( boolean result; try { - result = - (boolean) - RuleEngine.getInstance() - .apply(rules, buildRuleContext(entityLink.getEntityType(), entity)); + result = (boolean) RuleEngine.getInstance().apply(rules, JsonUtils.getMap(entity)); } catch (Exception e) { throw new RuntimeException(e); } return result; } - - /** - * Approval gates ask "does this entity have reviewers?" and route to a terminal status when the - * answer is no. An entity that inherits its reviewers — a glossary term under a reviewed glossary — - * must answer yes, otherwise no approval task is ever created and the term settles in Draft. The - * raw {@code reviewers} field carries inherited entries only when the read that produced the entity - * applied inheritance, so resolve them explicitly here, the same way the approval-task assignee node - * does. - * - *

The override is applied to the rule context rather than to the entity: {@link - * JsonUtils#getMap} already returns a detached copy, so overriding a key on it is free, whereas the - * entity may be request-cached and mutating it would leak into other reads in the same request. - */ - private Map buildRuleContext(String entityType, EntityInterface entity) { - Map ruleData = JsonUtils.getMap(entity); - EntityRepository repository = Entity.getEntityRepository(entityType); - if (repository.isSupportsReviewers() && nullOrEmpty(entity.getReviewers())) { - List effectiveReviewers = repository.getEffectiveReviewersUntyped(entity); - if (!nullOrEmpty(effectiveReviewers)) { - ruleData.put(FIELD_REVIEWERS, JsonUtils.convertValue(effectiveReviewers, List.class)); - } - } - return ruleData; - } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DatabaseSchemaRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DatabaseSchemaRepository.java index 9cbbe0afe43f..a849eedadc79 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DatabaseSchemaRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DatabaseSchemaRepository.java @@ -550,7 +550,7 @@ public void setInheritedFields(DatabaseSchema schema, Fields fields) { ? (needsRetention ? "owners,domains,retentionPeriod" : "owners,domains") : "retentionPeriod"; Database database = - getOrLoadInheritanceParent(schema.getDatabase(), inheritanceFields, Database.class); + loadInheritanceParentLeniently(schema.getDatabase(), inheritanceFields, Database.class); if (database == null) { return; } 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 91ceb9802850..486b3f3080cb 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 @@ -271,6 +271,7 @@ import org.openmetadata.service.util.EntityUtil; import org.openmetadata.service.util.EntityUtil.Fields; import org.openmetadata.service.util.EntityUtil.RelationIncludes; +import org.openmetadata.service.util.FreshReadScope; import org.openmetadata.service.util.FullyQualifiedName; import org.openmetadata.service.util.LineageUtil; import org.openmetadata.service.util.ListWithOffsetFunction; @@ -343,8 +344,6 @@ public abstract class EntityRepository { public record EntityHistoryWithOffset(EntityHistory entityHistory, int nextOffset) {} - private record InheritanceCacheKey(String entityType, UUID entityId, String fieldsKey) {} - private static final int STRING_OBJECT_OVERHEAD_BYTES = 40; // Conservative upper-bound weight for a String: length() * 2 (UTF-16 worst-case) + 40 (header). @@ -607,9 +606,6 @@ public Integer load(String key) { */ private final ThreadLocal> parentCacheForPrepare = new ThreadLocal<>(); - private static final ThreadLocal> - inheritanceParentCache = ThreadLocal.withInitial(HashMap::new); - protected final ChangeSummarizer changeSummarizer; // Lock manager for preventing orphaned entities during cascade deletion @@ -958,19 +954,12 @@ protected void clearRelationshipsForUpdateMany(List entities) { */ @SuppressWarnings("unused") protected void setInheritedFields(T entity, Fields fields) { - if (!requiresParentForInheritance(entity, fields)) { - return; - } - String inheritableFields = getInheritableFields(); - EntityReference parentRef = getParentReference(entity); - EntityInterface parent = getCachedInheritanceParent(parentRef, inheritableFields); - if (parent == null) { - parent = resolveInheritanceParentLeniently(entity, inheritableFields); - cacheInheritanceParent(parentRef, inheritableFields, parent); - } - if (parent != null) { - // Keep single-entity inheritance path aligned with batch/recursive inheritance path. - applyInheritance(entity, fields, parent); + if (requiresParentForInheritance(entity, fields)) { + EntityInterface parent = resolveInheritanceParentLeniently(entity, getInheritableFields()); + if (parent != null) { + // Keep single-entity inheritance path aligned with batch/recursive inheritance path. + applyInheritance(entity, fields, parent); + } } } @@ -1176,51 +1165,23 @@ protected void setParentCache(Map cache) { /** Clear the parent cache after bulk prepare. */ public void clearParentCache() { parentCacheForPrepare.remove(); - inheritanceParentCache.remove(); - } - - public static void clearInheritanceParentCache() { - inheritanceParentCache.remove(); } - private EntityInterface getCachedInheritanceParent(EntityReference parentRef, String fields) { - if (parentRef == null || parentRef.getId() == null || nullOrEmpty(parentRef.getType())) { - return null; - } - Map cache = inheritanceParentCache.get(); - InheritanceCacheKey directKey = inheritanceCacheKey(parentRef, fields); - EntityInterface direct = cache.get(directKey); - if (direct != null) { - return direct; - } - - // Reuse a superset entry when the same parent was already loaded with broader fields - // (for example "owners,domains,retentionPeriod" can serve "owners,domains"). - Set requestedFields = parseFieldSet(directKey.fieldsKey()); - for (Entry entry : cache.entrySet()) { - InheritanceCacheKey cachedKey = entry.getKey(); - if (!cachedKey.entityType().equals(parentRef.getType()) - || !cachedKey.entityId().equals(parentRef.getId())) { - continue; - } - if (parseFieldSet(cachedKey.fieldsKey()).containsAll(requestedFields)) { - return entry.getValue(); - } - } - return null; - } - - protected final

P getOrLoadInheritanceParent( + /** + * Loads an inheritance parent, tolerating a parent that has been hard-deleted since the child was + * read. Returns null (skip inheritance) rather than propagating, matching {@link + * #resolveInheritanceParentLeniently}. + */ + protected final

P loadInheritanceParentLeniently( EntityReference parentRef, String fields, Class

parentClass) { - if (parentRef == null || parentRef.getId() == null || nullOrEmpty(parentRef.getType())) { - return null; - } - EntityInterface parent = getCachedInheritanceParent(parentRef, fields); - if (parent == null) { + P result = null; + if (parentRef != null && parentRef.getId() != null && !nullOrEmpty(parentRef.getType())) { try { - parent = + EntityInterface parent = Entity.getEntityForInheritance(parentRef.getType(), parentRef.getId(), fields, ALL); - cacheInheritanceParent(parentRef, fields, parent); + if (parentClass.isInstance(parent)) { + result = parentClass.cast(parent); + } } catch (EntityNotFoundException e) { LOG.debug( "Inheritance parent {} {} no longer exists; skipping inheritance", @@ -1228,52 +1189,7 @@ protected final

P getOrLoadInheritanceParent( parentRef.getId()); } } - if (!parentClass.isInstance(parent)) { - return null; - } - return parentClass.cast(parent); - } - - private void cacheInheritanceParent( - EntityReference parentRef, String fields, EntityInterface parent) { - if (parentRef == null || parentRef.getId() == null || nullOrEmpty(parentRef.getType())) { - return; - } - if (parent == null || parent.getId() == null) { - return; - } - inheritanceParentCache.get().put(inheritanceCacheKey(parentRef, fields), parent); - } - - private InheritanceCacheKey inheritanceCacheKey(EntityReference parentRef, String fields) { - return new InheritanceCacheKey( - parentRef.getType(), parentRef.getId(), normalizeFieldList(fields)); - } - - private String normalizeFieldList(String fields) { - if (fields == null || fields.isBlank()) { - return ""; - } - // Canonicalize field order so cache keys are stable across equivalent requests - // (e.g. "owners,domains" and "domains, owners" should share the same parent entry). - return fields - .lines() - .flatMap(line -> Stream.of(line.split(","))) - .map(String::trim) - .filter(field -> !field.isEmpty()) - .distinct() - .sorted() - .collect(Collectors.joining(",")); - } - - private Set parseFieldSet(String fields) { - if (fields == null || fields.isBlank()) { - return Collections.emptySet(); - } - return Stream.of(fields.split(",")) - .map(String::trim) - .filter(field -> !field.isEmpty()) - .collect(Collectors.toSet()); + return result; } /** @@ -1307,32 +1223,14 @@ protected void setInheritedFields(List entities, Fields fields) { var refsByType = parentRefMap.values().stream().collect(Collectors.groupingBy(EntityReference::getType)); + // One bulk load per parent type keeps list endpoints off an N+1, which is what the removed + // thread-local parent cache was really buying on this path. var loadedParents = new HashMap(); - var missingRefsByType = new HashMap>(); for (var entry : refsByType.entrySet()) { - var missingRefs = new ArrayList(); - for (var ref : entry.getValue()) { - var cachedParent = getCachedInheritanceParent(ref, inheritableFields); - if (cachedParent != null) { - loadedParents.put(ref.getId(), cachedParent); - } else { - missingRefs.add(ref); - } - } - if (!missingRefs.isEmpty()) { - missingRefsByType.put(entry.getKey(), missingRefs); - } - } - - for (var entry : missingRefsByType.entrySet()) { List parents = Entity.getEntitiesForInheritance(entry.getValue(), inheritableFields, ALL); for (var parent : parents) { loadedParents.put(parent.getId(), parent); - var parentRef = parentRefMap.get(parent.getId()); - if (parentRef != null) { - cacheInheritanceParent(parentRef, inheritableFields, parent); - } } } @@ -1542,18 +1440,18 @@ public final T get( Fields fields, RelationIncludes relationIncludes, boolean fromCache) { + final boolean useCache = cacheAllowed(fromCache); T requestCachedEntity = - RequestEntityCache.getById( - entityType, id, fields, relationIncludes, fromCache, entityClass); + RequestEntityCache.getById(entityType, id, fields, relationIncludes, useCache, entityClass); if (requestCachedEntity != null) { return withHref(uriInfo, requestCachedEntity); } - if (!fromCache) { + if (!useCache) { CACHE_WITH_ID.invalidate(new ImmutablePair<>(entityType, id)); } T entity = - withPhase("entityLookup", () -> find(id, relationIncludes.getDefaultInclude(), fromCache)); + withPhase("entityLookup", () -> find(id, relationIncludes.getDefaultInclude(), useCache)); ReadPlan readPlan = withPhase("readCreatePlan", () -> createReadPlan(entity, fields, relationIncludes)); ReadBundle readBundle = withPhase("buildReadBundle", () -> buildReadBundle(entity, readPlan)); @@ -1570,7 +1468,7 @@ public final T get( "requestCachePutById", () -> RequestEntityCache.putById( - entityType, id, fields, relationIncludes, fromCache, hydratedEntity, entityClass)); + entityType, id, fields, relationIncludes, useCache, hydratedEntity, entityClass)); if (hydratedEntity.getFullyQualifiedName() != null) { withPhase( "requestCachePutByName", @@ -1580,7 +1478,7 @@ public final T get( hydratedEntity.getFullyQualifiedName(), fields, relationIncludes, - fromCache, + useCache, hydratedEntity, entityClass)); } @@ -1628,7 +1526,17 @@ public final T find(UUID id, Include include) throws EntityNotFoundException { return find(id, include, true); } + /** + * Callers ask for cached reads by default; a fresh-read scope overrides them. Governance workflows + * open that scope because their reads drive gating decisions, where a stale answer routes an entity + * to the wrong terminal status silently. + */ + private static boolean cacheAllowed(boolean fromCache) { + return fromCache && !FreshReadScope.isActive(); + } + public final T find(UUID id, Include include, boolean fromCache) throws EntityNotFoundException { + fromCache = cacheAllowed(fromCache); var notFoundCache = CacheBundle.getNotFoundCache(); if (!fromCache) { // On the explicit-bypass path the L1 cache is being skipped entirely, so checking the @@ -1758,6 +1666,7 @@ public final T getByName( Fields fields, RelationIncludes relationIncludes, boolean fromCache) { + fromCache = cacheAllowed(fromCache); fqn = quoteFqn ? quoteName(fqn) : fqn; T requestCachedEntity = RequestEntityCache.getByName( @@ -2274,6 +2183,7 @@ public final T findByName(String fqn, Include include) { } public final T findByName(String fqn, Include include, boolean fromCache) { + fromCache = cacheAllowed(fromCache); fqn = quoteFqn ? quoteName(fqn) : fqn; var notFoundCache = CacheBundle.getNotFoundCache(); if (!fromCache) { @@ -7864,34 +7774,6 @@ public final void inheritReviewers(T entity, Fields fields, EntityInterface pare } } - /** - * Reviewers that govern this entity's approval: those set directly on it, or — when it has none — - * those of its inheritance parent. Approval decisions must use this rather than the raw {@code - * reviewers} field, which carries inherited entries only when the read that produced the entity - * both requested and applied inheritance. - * - *

Resolution goes through {@link #getParentEntity}, the same hook the inheritance path uses, so - * any entity declaring an inheritance parent participates without type-specific code. Reading the - * parent applies the parent's own inheritance, so a grandparent's reviewers (a glossary's, for a - * term nested under another term) surface through the single hop. A parent that is missing or - * concurrently deleted degrades to "no reviewers" instead of failing the caller. - */ - public final List getEffectiveReviewers(T entity) { - List reviewers = listOrEmpty(entity.getReviewers()); - if (reviewers.isEmpty() && supportsReviewers) { - EntityInterface parent = resolveInheritanceParentLeniently(entity, FIELD_REVIEWERS); - if (parent != null) { - reviewers = listOrEmpty(parent.getReviewers()); - } - } - return reviewers; - } - - @SuppressWarnings("unchecked") - public final List getEffectiveReviewersUntyped(EntityInterface entity) { - return getEffectiveReviewers((T) entity); - } - private List inheritedEntityReferences(List references) { if (nullOrEmpty(references)) { return Collections.emptyList(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java index a4852345223f..2d486633af13 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java @@ -351,7 +351,7 @@ public void setInheritedFields(Table table, Fields fields) { ? (needsRetention ? "owners,domains,retentionPeriod" : "owners,domains") : "retentionPeriod"; DatabaseSchema schema = - getOrLoadInheritanceParent( + loadInheritanceParentLeniently( table.getDatabaseSchema(), inheritanceFields, DatabaseSchema.class); if (schema == null) { return; diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/ImpersonationCleanupFilter.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/ImpersonationCleanupFilter.java index fca87cf2cb1a..adc634637d4b 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/ImpersonationCleanupFilter.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/ImpersonationCleanupFilter.java @@ -19,11 +19,7 @@ import jakarta.ws.rs.container.ContainerResponseContext; import jakarta.ws.rs.container.ContainerResponseFilter; import jakarta.ws.rs.ext.Provider; -import org.openmetadata.service.Entity; -import org.openmetadata.service.jdbi3.EntityRepository; -import org.openmetadata.service.jdbi3.ReadBundleContext; -import org.openmetadata.service.resources.filters.ETagRequestFilter; -import org.openmetadata.service.util.RequestEntityCache; +import org.openmetadata.service.util.PerRequestContextCleaner; /** * Response filter to clean up ThreadLocal impersonation context after each request. This prevents @@ -37,12 +33,6 @@ public class ImpersonationCleanupFilter implements ContainerResponseFilter { public void filter( ContainerRequestContext requestContext, ContainerResponseContext responseContext) { // Always clear ThreadLocal after request completes (success or failure) - ImpersonationContext.clear(); - ActivePersonaContext.clear(); - ETagRequestFilter.clearIfMatchHeader(); - RequestEntityCache.clear(); - ReadBundleContext.clear(); - EntityRepository.clearInheritanceParentCache(); - Entity.clearRepositoryThreadLocals(); + PerRequestContextCleaner.clear(); } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/FreshReadScope.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/FreshReadScope.java new file mode 100644 index 000000000000..9f05fac620b5 --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/FreshReadScope.java @@ -0,0 +1,61 @@ +/* + * Copyright 2024 Collate + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.service.util; + +/** + * Thread-scoped marker that forces entity reads to bypass the in-JVM caches and go to the database. + * + *

Governance workflows use this. Their decisions are gates — "does this term have reviewers?" — + * and a stale answer silently routes an entity to the wrong terminal status with no error and no + * retry. The in-JVM L1 is invalidated on write locally and, when Redis is configured, across nodes + * via pub/sub; but with Redis disabled a multi-node deployment has no cross-node invalidation at + * all, so another node's L1 can answer from before the write. Workflow volume is low, so paying for + * a fresh read is the cheaper side of that trade. + * + *

Scope it with try-with-resources so the marker is always restored, including on exceptions: + * + *

{@code
+ * try (FreshReadScope.Handle ignored = FreshReadScope.enter()) {
+ *   ...
+ * }
+ * }
+ * + *

{@link #enter()} restores the previous value rather than clearing, so nesting is safe. This is + * the same shape as {@link org.openmetadata.service.cache.EntityCacheBypass}, which opts out of the + * Redis layer; this one opts out of the in-process caches. + */ +public final class FreshReadScope { + + private static final ThreadLocal ACTIVE = ThreadLocal.withInitial(() -> Boolean.FALSE); + + private FreshReadScope() {} + + /** True when the calling thread is inside a fresh-read scope. */ + public static boolean isActive() { + return Boolean.TRUE.equals(ACTIVE.get()); + } + + /** Enters a fresh-read scope. Close the returned handle to restore the previous state. */ + public static Handle enter() { + boolean previous = ACTIVE.get(); + ACTIVE.set(Boolean.TRUE); + return () -> ACTIVE.set(previous); + } + + @FunctionalInterface + public interface Handle extends AutoCloseable { + @Override + void close(); + } +} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/PerRequestContextCleaner.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/PerRequestContextCleaner.java new file mode 100644 index 000000000000..282288d6acb8 --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/PerRequestContextCleaner.java @@ -0,0 +1,46 @@ +/* + * Copyright 2024 Collate + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.service.util; + +import org.openmetadata.service.Entity; +import org.openmetadata.service.jdbi3.ReadBundleContext; +import org.openmetadata.service.resources.filters.ETagRequestFilter; +import org.openmetadata.service.security.ActivePersonaContext; +import org.openmetadata.service.security.ImpersonationContext; + +/** + * Clears the ThreadLocal state that is scoped to a single unit of work. + * + *

These ThreadLocals are caches and request context, not pending work, so dropping them can only + * cost a re-read. They were originally cleared only by the JAX-RS response filter, which meant any + * pool that never serves an HTTP request — the Quartz change-event consumer, Flowable's async job + * executor — accumulated them for the life of the process and served indefinitely stale reads. + * + *

Deliberately excluded: {@code LineageUtil.DEFERRED_LINEAGE_ES} and {@code + * SearchRepository.DEFERRED_SEARCH_WRITES}. Those hold pending writes, so clearing them + * would silently drop search and lineage updates rather than merely forcing a re-read. + */ +public final class PerRequestContextCleaner { + + private PerRequestContextCleaner() {} + + public static void clear() { + ImpersonationContext.clear(); + ActivePersonaContext.clear(); + ETagRequestFilter.clearIfMatchHeader(); + RequestEntityCache.clear(); + ReadBundleContext.clear(); + Entity.clearRepositoryThreadLocals(); + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListenerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListenerTest.java new file mode 100644 index 000000000000..3fc3cfbdacf7 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListenerTest.java @@ -0,0 +1,84 @@ +/* + * Copyright 2024 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.service.governance.workflows; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.flowable.common.engine.api.delegate.event.FlowableEngineEventType; +import org.flowable.common.engine.api.delegate.event.FlowableEvent; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.openmetadata.service.security.ImpersonationContext; + +/** + * Flowable's async-executor threads are pooled and never pass through the JAX-RS response filter, so + * without a job-boundary hook the per-request ThreadLocals survive from one job to the next and serve + * stale reads for the life of the process. + */ +class WorkflowThreadCleanupListenerTest { + + private final WorkflowThreadCleanupListener listener = new WorkflowThreadCleanupListener(); + + @AfterEach + void tearDown() { + ImpersonationContext.clear(); + } + + @Test + void jobCompletion_clearsPerRequestContext() { + for (FlowableEngineEventType type : + new FlowableEngineEventType[] { + FlowableEngineEventType.JOB_EXECUTION_SUCCESS, + FlowableEngineEventType.JOB_EXECUTION_FAILURE + }) { + ImpersonationContext.setImpersonatedBy("someone"); + assertNotNull(ImpersonationContext.getImpersonatedBy(), "precondition for " + type); + + listener.onEvent(eventOfType(type)); + + assertNull( + ImpersonationContext.getImpersonatedBy(), + type + " must clear per-request context so the next job on this thread starts clean"); + } + } + + @Test + void unrelatedEvent_leavesContextAlone() { + ImpersonationContext.setImpersonatedBy("someone"); + + listener.onEvent(eventOfType(FlowableEngineEventType.ENTITY_CREATED)); + + assertNotNull( + ImpersonationContext.getImpersonatedBy(), + "Only job boundaries are cleanup points; the listener sees every engine event"); + } + + /** Cleanup is best-effort: it must never be able to fail the job that triggered it. */ + @Test + void listenerNeverFailsTheJob() { + assertFalse(listener.isFailOnException()); + assertFalse(listener.isFireOnTransactionLifecycleEvent()); + assertNull(listener.getOnTransaction()); + } + + private FlowableEvent eventOfType(FlowableEngineEventType type) { + FlowableEvent event = mock(FlowableEvent.class); + when(event.getType()).thenReturn(type); + return event; + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java index 1e20a602d397..6f17c08a40ed 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImplTest.java @@ -13,7 +13,6 @@ package org.openmetadata.service.governance.workflows.elements.nodes.automatedTask.impl; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -41,21 +40,19 @@ import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.Include; import org.openmetadata.service.Entity; -import org.openmetadata.service.jdbi3.EntityRepository; import org.openmetadata.service.resources.feeds.MessageParser; /** * Covers the reviewers gate of the Glossary Approval Workflow. * - *

{@code CheckGlossaryTermHasReviewers} decides whether an approval task is created at all. It - * evaluates the shipped JsonLogic rule below against the term, and routes a {@code false} result to a - * terminal status. A term whose reviewers are inherited from its parent glossary must still - * answer {@code true}; otherwise no approval task is ever created and the term settles in Draft. + *

{@code CheckGlossaryTermHasReviewers} decides whether an approval task is created at all: it + * evaluates the shipped JsonLogic rule below against the term and routes a {@code false} result to a + * terminal status, leaving the term in Draft with no task. These tests pin how that rule reads the + * entity it is handed. * - *

These tests pin that behaviour at the unit level, where the failing condition can be forced - * directly: an entity read that returns no reviewers on the term while the parent supplies - * them. {@link #reviewersRule_inheritedReviewersOnly_evaluatesTrue()} fails without the - * effective-reviewer resolution in {@code CheckEntityAttributesImpl} and passes with it. + *

Whether a term that inherits its reviewers arrives here with them populated is a property + * of the read path, not of this delegate — it is covered by the inheritance tests and by {@code + * GlossaryTermInheritedReviewerApprovalIT}. */ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -72,10 +69,6 @@ class CheckEntityAttributesImplTest { @Mock private Expression rulesExpr; @Mock private Expression inputNamespaceMapExpr; - @SuppressWarnings("rawtypes") - @Mock - private EntityRepository mockRepository; - private CheckEntityAttributesImpl delegate; private MockedStatic mockedEntity; private Map capturedVars; @@ -92,10 +85,8 @@ void setUp() throws Exception { when(execution.getCurrentActivityId()).thenReturn(NODE_ID); when(execution.getVariable("global_relatedEntity")) .thenReturn("<#E::glossaryTerm::Property.hello world>"); - when(mockRepository.isSupportsReviewers()).thenReturn(true); mockedEntity = mockStatic(Entity.class); - mockedEntity.when(() -> Entity.getEntityRepository(anyString())).thenReturn(mockRepository); capturedVars = new HashMap<>(); doAnswer( @@ -112,75 +103,39 @@ void tearDown() { mockedEntity.close(); } - /** - * The reported bug. The term carries no reviewers of its own — the read returned none — but its - * glossary supplies one, so effective-reviewer resolution must make the gate answer true. Without - * that resolution the rule sees an empty array, {@code some} is vacuously false, and the term is - * routed to Draft with no approval task. - */ + /** Reviewers present on the entity — whether set directly or applied by inheritance. */ @Test - void reviewersRule_inheritedReviewersOnly_evaluatesTrue() { - givenRelatedEntity(termWithReviewers(null)); - givenEffectiveReviewers(reviewer("manoj")); - - delegate.execute(execution); - - assertTrue( - result(), - "Gate must see the inherited reviewer and allow the approval task to be created; " - + "a false result here is what leaves the term in Draft"); - } - - /** A reviewer set directly on the term is unaffected by the resolution. */ - @Test - void reviewersRule_directReviewersOnTerm_evaluatesTrue() { + void reviewersRule_reviewersPresentOnEntity_evaluatesTrue() { givenRelatedEntity(termWithReviewers(List.of(reviewer("manoj")))); delegate.execute(execution); - assertTrue(result(), "A directly attached reviewer must satisfy the gate"); + assertTrue(result(), "A term carrying a reviewer must satisfy the gate"); } - /** Nothing to inherit anywhere: the gate must still answer false and auto-approve. */ + /** No reviewers anywhere: the gate must be false so the term auto-approves. */ @Test - void reviewersRule_noReviewersAnywhere_evaluatesFalse() { + void reviewersRule_noReviewers_evaluatesFalse() { givenRelatedEntity(termWithReviewers(null)); - givenEffectiveReviewers(); delegate.execute(execution); - assertFalse(result(), "With no reviewers on the term or its parents the gate must be false"); + assertFalse(result(), "With no reviewers the gate must be false"); } /** - * A reviewer reference without a fullyQualifiedName does not satisfy the shipped rule, which tests - * {@code fullyQualifiedName != null} per element. Resolution must not paper over that. + * The shipped rule tests {@code fullyQualifiedName != null} per element, so a reference missing its + * FQN does not satisfy it. Pinned because inheritance copies references between entities. */ @Test - void reviewersRule_inheritedReviewerWithoutFqn_evaluatesFalse() { - givenRelatedEntity(termWithReviewers(null)); - givenEffectiveReviewers(new EntityReference().withType(Entity.USER)); + void reviewersRule_reviewerWithoutFqn_evaluatesFalse() { + givenRelatedEntity(termWithReviewers(List.of(new EntityReference().withType(Entity.USER)))); delegate.execute(execution); assertFalse(result(), "A reviewer reference with no FQN must not satisfy the rule"); } - /** - * Resolution must not be attempted for entities that do not support reviewers, and the gate must - * fall back to whatever the entity itself carries. - */ - @Test - void reviewersRule_entityWithoutReviewerSupport_evaluatesFalse() { - when(mockRepository.isSupportsReviewers()).thenReturn(false); - givenRelatedEntity(termWithReviewers(null)); - - delegate.execute(execution); - - assertFalse(result(), "An entity type without reviewer support must not satisfy the gate"); - assertEquals(1, capturedVars.size(), "Only the node result should be written"); - } - private void givenRelatedEntity(GlossaryTerm term) { mockedEntity .when( @@ -190,11 +145,6 @@ private void givenRelatedEntity(GlossaryTerm term) { .thenReturn(term); } - @SuppressWarnings("unchecked") - private void givenEffectiveReviewers(EntityReference... reviewers) { - when(mockRepository.getEffectiveReviewersUntyped(any())).thenReturn(List.of(reviewers)); - } - private GlossaryTerm termWithReviewers(List reviewers) { return new GlossaryTerm() .withName("hello world") diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryInheritanceParentTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryInheritanceParentTest.java new file mode 100644 index 000000000000..3c1d83eca480 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryInheritanceParentTest.java @@ -0,0 +1,174 @@ +/* + * Copyright 2024 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.openmetadata.service.jdbi3; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.UUID; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openmetadata.schema.EntityInterface; +import org.openmetadata.schema.entity.data.Pipeline; +import org.openmetadata.schema.type.EntityReference; +import org.openmetadata.service.Entity; +import org.openmetadata.service.util.EntityUtil.Fields; +import org.openmetadata.service.util.EntityUtil.RelationIncludes; + +/** + * Inheritance must reflect the parent's current state, not a snapshot taken earlier on the same + * thread. + * + *

Parent lookups used to be memoized in a {@code static ThreadLocal} on {@code EntityRepository} + * that had no TTL, no size bound, and no eviction, and was cleared only by the JAX-RS response + * filter. Any pool that never serves an HTTP request — the Quartz change-event consumer that runs + * governance workflows, Flowable's async job executor — therefore kept the first snapshot forever. + * In production that meant a glossary read before its reviewer was added kept reporting no + * reviewers, so the approval workflow created no task and left terms in Draft until the pod + * restarted. + * + *

{@link #inheritance_reflectsParentMutation_onRepeatedReadsFromOneThread()} fails against that + * memoization: the second read returns the stale parent. + */ +class EntityRepositoryInheritanceParentTest { + + private CollectionDAO daoCollection; + private ParentTrackingPipelineRepo repository; + private Pipeline parent; + + /** Serves a single mutable parent and counts how many times inheritance asked for it. */ + private static class ParentTrackingPipelineRepo extends EntityRepository { + private final Pipeline parent; + private int parentLoads; + + ParentTrackingPipelineRepo(CollectionDAO.PipelineDAO dao, Pipeline parent) { + super("pipelines", Entity.PIPELINE, Pipeline.class, dao, "domains", "domains"); + this.parent = parent; + } + + @Override + public EntityReference getParentReference(Pipeline entity) { + return parent.getEntityReference(); + } + + /** + * Returns a fresh snapshot each time, as a real read does. Handing back the same mutable + * instance would let a memoized reference appear up to date and hide staleness. + */ + @Override + public EntityInterface getParentEntity(Pipeline entity, String fields) { + parentLoads++; + return new Pipeline() + .withId(parent.getId()) + .withName(parent.getName()) + .withFullyQualifiedName(parent.getFullyQualifiedName()) + .withDomains(parent.getDomains() == null ? null : new ArrayList<>(parent.getDomains())); + } + + @Override + protected void setFields(Pipeline entity, Fields fields, RelationIncludes r) {} + + @Override + protected void clearFields(Pipeline entity, Fields fields) {} + + @Override + protected void prepare(Pipeline entity, boolean update) {} + + @Override + protected void storeEntity(Pipeline entity, boolean update) {} + + @Override + protected void storeRelationships(Pipeline entity) {} + } + + @BeforeEach + void setUp() { + daoCollection = mock(CollectionDAO.class); + when(daoCollection.relationshipDAO()) + .thenReturn(mock(CollectionDAO.EntityRelationshipDAO.class)); + Entity.setCollectionDAO(daoCollection); + + parent = + new Pipeline() + .withId(UUID.randomUUID()) + .withName("parent") + .withFullyQualifiedName("parent") + .withDomains(new ArrayList<>()); + repository = new ParentTrackingPipelineRepo(mock(CollectionDAO.PipelineDAO.class), parent); + } + + @AfterEach + void tearDown() { + Entity.cleanup(); + } + + @Test + void inheritance_reflectsParentMutation_onRepeatedReadsFromOneThread() { + Fields domains = new Fields(Set.of(Entity.FIELD_DOMAINS)); + + // First read on this thread: the parent has no domains yet, so nothing is inherited. + Pipeline before = childPipeline(); + repository.setInheritedFields(before, domains); + assertTrue( + before.getDomains() == null || before.getDomains().isEmpty(), + "Nothing to inherit before the parent has a domain"); + + // The parent gains a domain — as a glossary gains a reviewer. + EntityReference domain = + new EntityReference() + .withId(UUID.randomUUID()) + .withType(Entity.DOMAIN) + .withName("finance") + .withFullyQualifiedName("finance"); + parent.setDomains(new ArrayList<>(List.of(domain))); + + // Second read on the SAME thread must see it. A memoized parent would still report none. + Pipeline after = childPipeline(); + repository.setInheritedFields(after, domains); + + assertEquals( + 1, + after.getDomains() == null ? 0 : after.getDomains().size(), + "Second read on the same thread must reflect the parent's current domains, " + + "not a snapshot cached during the first read"); + assertEquals("finance", after.getDomains().get(0).getFullyQualifiedName()); + } + + @Test + void inheritance_loadsParentOnEveryRead_soNoStaleSnapshotCanSurvive() { + Fields domains = new Fields(Set.of(Entity.FIELD_DOMAINS)); + + repository.setInheritedFields(childPipeline(), domains); + repository.setInheritedFields(childPipeline(), domains); + + assertEquals( + 2, + repository.parentLoads, + "Each read resolves the parent; memoizing it across reads is what made inherited " + + "fields go stale on long-lived non-HTTP threads"); + } + + private Pipeline childPipeline() { + return new Pipeline() + .withId(UUID.randomUUID()) + .withName("child") + .withFullyQualifiedName("child"); + } +} From 89881959405d90f99ce4fb53b53b4f93733c82b1 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 14:24:25 -0700 Subject: [PATCH 06/13] fix(tests): wait for a committed status; match job events by enum waitForSettledStatus accepted Draft as settled, but a term under a reviewed parent is written as Draft at creation, before the workflow runs. The poll therefore returned on its first sample and every assertion read the pre-workflow value, so the spec reported the inherited-reviewer bug on a healthy build. Wait only for a status the workflow itself commits; a term that never leaves Draft now times out with a message naming the bug, which is the real signal. WorkflowThreadCleanupListener matched job-boundary events by name string. A Flowable rename would have fallen through to the no-op branch and silently stopped clearing the per-request ThreadLocals on the async-executor pool, restoring the stale-read leak with no compile error. Compare against the enum constants instead. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/WorkflowThreadCleanupListener.java | 13 +++++++------ .../GlossaryInheritedReviewerApproval.spec.ts | 16 +++++++++++----- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java index 21b8ca6f6363..0325a979653c 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java @@ -13,6 +13,7 @@ package org.openmetadata.service.governance.workflows; +import org.flowable.common.engine.api.delegate.event.FlowableEngineEventType; import org.flowable.common.engine.api.delegate.event.FlowableEvent; import org.flowable.common.engine.api.delegate.event.FlowableEventListener; import org.openmetadata.service.util.PerRequestContextCleaner; @@ -34,12 +35,12 @@ public class WorkflowThreadCleanupListener implements FlowableEventListener { @Override public void onEvent(FlowableEvent event) { - // Registered engine-wide, so every event lands here: keep the check cheap and first. - switch (event.getType().name()) { - case "JOB_EXECUTION_SUCCESS", "JOB_EXECUTION_FAILURE" -> PerRequestContextCleaner.clear(); - default -> { - /* not a job boundary */ - } + // Registered engine-wide, so every event lands here: keep the check cheap and first. Compare + // against the enum constants rather than their names so that if Flowable renames an event type, + // this fails to compile instead of silently never clearing again. + if (event.getType() == FlowableEngineEventType.JOB_EXECUTION_SUCCESS + || event.getType() == FlowableEngineEventType.JOB_EXECUTION_FAILURE) { + PerRequestContextCleaner.clear(); } } diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts index f56cf1ca33b5..148f06d3106a 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -62,9 +62,13 @@ const createTermWithoutReviewers = async ( }; /** - * Polls the term until the workflow has settled it. The workflow runs asynchronously, so the term is - * briefly `Unprocessed`/`Draft` before the gate resolves; we wait for a status the workflow actually - * commits to rather than sampling once and racing it. + * Polls the term until the approval workflow has committed a status. + * + *

`Draft` is deliberately NOT a settled status. A term under a reviewed parent is written as + * `Draft` at creation, before the workflow runs, so accepting it here would return on the very first + * sample and every assertion would read the pre-workflow value. Waiting for a status the workflow + * itself sets means a term that never leaves `Draft` — the bug under test — times out with the + * message below rather than being misreported as a wrong-status failure. */ const waitForSettledStatus = async ( apiContext: APIRequestContext, @@ -84,11 +88,13 @@ const waitForSettledStatus = async ( return status; }, { - message: `Glossary term ${termId} never reached a settled workflow status`, + message: + `Glossary term ${termId} never left Draft — the approval workflow did not commit a ` + + `status. For a term inheriting a reviewer this is the inherited-reviewer bug.`, timeout: STATUS_TIMEOUT, } ) - .toMatch(/In Review|Approved|Rejected|Draft/); + .toMatch(/In Review|Approved|Rejected/); return status; }; From 00b552bc9ffd7fd83d2c6048d3d904fa2bcc6231 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 6 Aug 2026 14:55:46 -0700 Subject: [PATCH 07/13] fix(events): clear per-request context on every tick exit path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exit-side clear sat in the inner finally, which init() precedes and which the "subscription could not be loaded" early return skips entirely. Quartz worker threads are shared with every other scheduled job, so state left behind on those paths is inherited by whatever runs next on the thread — the entry-side clear only protects the next AbstractEventConsumer tick, not an unrelated job. Bracket the tick in try/finally so cleanup runs on success, error and early return alike. The accompanying test pins the end-to-end invariant and fails if cleanup is removed altogether. It does not isolate the exit clear from the entry clear: the paths that previously skipped cleanup run inside the private init(), which offers no seam to populate the ThreadLocals mid-tick. The unconditional exit clear is enforced structurally by the try/finally rather than by that test, and the javadoc says so. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../changeEvent/AbstractEventConsumer.java | 19 ++++++++---- .../AbstractEventConsumerTest.java | 30 +++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java b/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java index 79d46ad4bda8..d1e3b082706a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java @@ -491,11 +491,20 @@ record CursorPlan(long offset, long pendingGapSince, int recordCount, boolean sk @Override public void execute(JobExecutionContext jobExecutionContext) { - // Quartz worker threads are long lived and never pass through the JAX-RS response filter, so - // per-request ThreadLocal caches would otherwise persist across ticks and serve indefinitely - // stale reads. Destinations on this thread read entities (governance workflows resolve - // inherited reviewers here), so start each tick from a clean slate. + // Quartz worker threads are long lived, shared with every other scheduled job, and never pass + // through the JAX-RS response filter. Per-request ThreadLocal caches left behind here would be + // served to whatever runs next on this thread — indefinitely stale. Destinations on this thread + // read entities (governance workflows resolve inherited reviewers here), so bracket the whole + // tick: start clean, and leave clean however this exits. PerRequestContextCleaner.clear(); + try { + executeTick(jobExecutionContext); + } finally { + PerRequestContextCleaner.clear(); + } + } + + private void executeTick(JobExecutionContext jobExecutionContext) { this.init(jobExecutionContext); if (this.eventSubscription == null) { LOG.error("Skipping job execution - EventSubscription could not be loaded"); @@ -526,8 +535,6 @@ public void execute(JobExecutionContext jobExecutionContext) { } else if (gapStateChanged) { persistPendingGapState(jobExecutionContext); } - // Bound retention while this worker sits idle between ticks. - PerRequestContextCleaner.clear(); } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumerTest.java index 1392f00856d4..4cc31c584557 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumerTest.java @@ -38,6 +38,7 @@ import org.openmetadata.service.notifications.recipients.RecipientResolver; import org.openmetadata.service.notifications.recipients.context.EmailRecipient; import org.openmetadata.service.notifications.recipients.context.Recipient; +import org.openmetadata.service.security.ImpersonationContext; import org.openmetadata.service.util.DIContainer; import org.quartz.JobDataMap; import org.quartz.JobDetail; @@ -141,6 +142,35 @@ void testConstructor() { assertNotNull(testEventConsumer.dependencies); } + /** + * Quartz worker threads are pooled and shared with every other scheduled job, and never pass + * through the JAX-RS response filter that clears these ThreadLocals for HTTP requests. Whatever + * runs next on the thread inherits anything left behind, so a tick must leave it clean however it + * exits — including an early return when the subscription cannot be loaded. + * + *

Scope of this test: it pins the end-to-end invariant and fails if the cleanup is removed + * altogether. It does not isolate the exit-side clear from the entry-side one, because the + * only paths that previously skipped cleanup run inside the private {@code init}, which offers no + * seam for a test to populate the ThreadLocals mid-tick. That the exit clear is unconditional is + * enforced structurally, by the try/finally in {@code execute}. + */ + @Test + void execute_leavesThreadCleanForTheNextJob() { + ImpersonationContext.setImpersonatedBy("someone"); + + try { + testEventConsumer.execute(jobExecutionContext); + } catch (RuntimeException expectedInThisHarness) { + // The subscription cannot be resolved here, so the tick either returns early or throws. + // Either way the cleanup guarantee below must hold. + } + + assertNull( + ImpersonationContext.getImpersonatedBy(), + "A tick must leave the thread clean, or the next job scheduled onto it reads stale " + + "per-request state"); + } + @Test void testSendAlertMethod() { UUID receiverId = UUID.randomUUID(); From 8ad9f0c79b2dfe339a46dbc336bf93785fc1dde8 Mon Sep 17 00:00:00 2001 From: Ram Narayan Balaji Date: Fri, 7 Aug 2026 14:21:22 +0530 Subject: [PATCH 08/13] test(glossary): stabilize reviewer workflow test --- .../GlossaryInheritedReviewerApproval.spec.ts | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts index 148f06d3106a..30baaacbef67 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -15,7 +15,7 @@ import { Glossary } from '../../../support/glossary/Glossary'; import { GlossaryTerm } from '../../../support/glossary/GlossaryTerm'; import { UserClass } from '../../../support/user/UserClass'; import { performAdminLogin } from '../../../utils/admin'; -import { redirectToHomePage, uuid } from '../../../utils/common'; +import { uuid } from '../../../utils/common'; /** * Reproduction for the Glossary Approval bug where a term whose reviewers are INHERITED from its @@ -36,6 +36,8 @@ import { redirectToHomePage, uuid } from '../../../utils/common'; const REVIEWER_ADDED_PROBE_COUNT = 6; const STATUS_TIMEOUT = 120_000; +test.use({ storageState: 'playwright/.auth/admin.json' }); + const reviewer = new UserClass(); const glossary = new Glossary(); @@ -195,10 +197,17 @@ test.describe( `inherited-reviewer bug.` ).toBe('In Review'); - expect( - await getOpenApprovalTaskCount(apiContext, term.fullyQualifiedName), - `Term ${term.name} must have an open approval task for the inherited reviewer` - ).toBeGreaterThan(0); + await expect + .poll( + () => + getOpenApprovalTaskCount(apiContext, term.fullyQualifiedName), + { + message: + `Term ${term.name} must have an open approval task for the inherited ` + + `reviewer`, + } + ) + .toBeGreaterThan(0); } }); @@ -220,8 +229,9 @@ test.describe( await waitForSettledStatus(apiContext, term.responseData.id); await afterAction(); - await redirectToHomePage(page); - await term.visitEntityPage(page); + await page.goto( + `/glossary/${encodeURIComponent(term.responseData.fullyQualifiedName)}` + ); await expect(page.locator('[data-testid="loader"]')).toHaveCount(0); @@ -234,7 +244,7 @@ test.describe( }); await test.step('Term reached In Review, not Draft', async () => { - await expect(page.getByTestId('status-badge')).toContainText( + await expect(page.locator('.status-badge-label')).toContainText( 'In Review' ); }); From 8c3c90819c97f529ea39cf7b581b0672c5aab827 Mon Sep 17 00:00:00 2001 From: Ram Narayan Balaji Date: Fri, 7 Aug 2026 14:49:17 +0530 Subject: [PATCH 09/13] test(glossary): wait for approval task --- .../Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts index 30baaacbef67..b5dd5c65e7d0 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -205,6 +205,8 @@ test.describe( message: `Term ${term.name} must have an open approval task for the inherited ` + `reviewer`, + timeout: STATUS_TIMEOUT, + intervals: [3_000, 5_000, 10_000], } ) .toBeGreaterThan(0); From 2df8ebef951d91fd6ff6ea9be0e5b387a9f36b73 Mon Sep 17 00:00:00 2001 From: Ram Narayan Balaji Date: Fri, 7 Aug 2026 15:17:00 +0530 Subject: [PATCH 10/13] fix(workflow): cancel after entity delete --- .../service/jdbi3/EntityRepository.java | 62 +++++++++---------- 1 file changed, 31 insertions(+), 31 deletions(-) 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 c0dc8ff0fd45..8e855a6db25e 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 @@ -4851,23 +4851,6 @@ protected final void cleanup(String deletedBy, T entityInterface) { // Delete the extension data storing custom properties removeExtension(entityInterface); - // Cancel any governance workflow instances tied to this entity before the row - // goes away, so downstream nodes do not throw EntityNotFoundException. The - // Flowable query and cancel calls must never abort the delete transaction — - // a stray engine failure here shouldn't wedge entity deletes on a data plane - // the workflow engine has no authority over. - if (WorkflowHandler.isInitialized()) { - try { - WorkflowHandler.getInstance() - .cancelInstancesForEntity(entityInterface.getId(), "Entity deleted"); - } catch (Exception cancelEx) { - LOG.warn( - "Failed to cancel workflow instances for entity {}: {}", - entityInterface.getId(), - cancelEx.getMessage()); - } - } - // Delete all the threads that are about this entity Entity.getFeedRepository().deleteByAbout(entityInterface.getId()); @@ -4881,6 +4864,10 @@ protected final void cleanup(String deletedBy, T entityInterface) { return null; }); + // Flowable uses a separate transaction. Cancelling only after this one commits prevents a + // rolled-back entity delete from leaving a live entity without its workflow, and keeps the + // workflow queries out of the entity transaction's lock-hold time. + cancelWorkflowInstances(List.of(entityInterface.getId())); // Re-invalidate after the transaction commits. Any read that slipped in between the // pre-delete invalidate and the commit could have re-populated the cache from the // still-visible DB row; clearing again here guarantees the next read goes back to the @@ -6851,6 +6838,7 @@ private void bulkDeleteReferencesAndRows(List entities) { if (jdbi == null) { bulkCleanupReferences(entities); bulkDeleteEntityRows(entities); + cancelWorkflowInstances(entityIds(entities)); return; } jdbi.inTransaction( @@ -6859,6 +6847,32 @@ private void bulkDeleteReferencesAndRows(List entities) { bulkDeleteEntityRows(entities); return null; }); + // Keep Flowable's separate transaction outside the entity delete transaction. See cleanup(). + cancelWorkflowInstances(entityIds(entities)); + } + + private List entityIds(List entities) { + List entityIds = new ArrayList<>(entities.size()); + for (T entity : entities) { + entityIds.add(entity.getId()); + } + return entityIds; + } + + private void cancelWorkflowInstances(Collection entityIds) { + // Workflow-engine failures must never wedge a hard-delete; the entity rows are the source of + // truth and the workflow cleanup is best effort. + if (!WorkflowHandler.isInitialized()) { + return; + } + try (var ignored = phase("bulkHardDeleteWorkflows")) { + WorkflowHandler.getInstance().cancelInstancesForEntities(entityIds, "Entity deleted"); + } catch (Exception cancelEx) { + LOG.warn( + "Failed to cancel workflow instances for {} entities: {}", + entityIds.size(), + cancelEx.getMessage()); + } } private void bulkCleanupReferences(List entities) { @@ -6897,20 +6911,6 @@ private void bulkCleanupReferences(List entities) { // must be cleared here — but in one IN-list delete per chunk instead of one per entity. daoCollection.usageDAO().deleteByIds(entityIds); } - try (var ignored = phase("bulkHardDeleteWorkflows")) { - // Workflow-engine failures must never wedge a bulk hard-delete; the workflow - // instances are best-effort cleanup, the entity rows are the source of truth. - if (WorkflowHandler.isInitialized()) { - try { - WorkflowHandler.getInstance().cancelInstancesForEntities(entityIds, "Entity deleted"); - } catch (Exception cancelEx) { - LOG.warn( - "Failed to cancel workflow instances for {} entities: {}", - entityIds.size(), - cancelEx.getMessage()); - } - } - } try (var ignored = phase("bulkHardDeleteFeedThreads")) { Entity.getFeedRepository().deleteByAbout(entityIds); } From 45d423d090727570ddcefac2427025c3cb137b29 Mon Sep 17 00:00:00 2001 From: Ram Narayan Balaji Date: Fri, 7 Aug 2026 16:32:46 +0530 Subject: [PATCH 11/13] test(glossary): wait for inherited reviewer --- .../GlossaryInheritedReviewerApproval.spec.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts index b5dd5c65e7d0..e8e65802394a 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -115,6 +115,37 @@ const getOpenApprovalTaskCount = async ( return (body.data ?? []).length; }; +/** + * The workflow deliberately reads fresh data because its result is a gating decision. The term + * page uses the normal entity-read path, where a just-updated glossary can briefly retain its + * pre-patch cached parent. Wait until that normal GET exposes the inherited reviewer before + * asserting the rendered card; a real inheritance failure still exhausts STATUS_TIMEOUT. + */ +const waitForInheritedReviewer = async ( + apiContext: APIRequestContext, + termId: string +) => { + await expect + .poll( + async () => { + const response = await apiContext.get( + `/api/v1/glossaryTerms/${termId}?fields=reviewers` + ); + const term = await response.json(); + + return (term.reviewers ?? []).some( + ({ id }: { id: string }) => id === reviewer.responseData.id + ); + }, + { + message: `Term ${termId} never exposed its inherited reviewer on a normal GET`, + timeout: STATUS_TIMEOUT, + intervals: [3_000, 5_000, 10_000], + } + ) + .toBe(true); +}; + test.describe( 'Glossary Approval - inherited reviewers', { tag: ['@Features', '@Governance'] }, @@ -229,6 +260,7 @@ test.describe( await term.create(apiContext); await waitForSettledStatus(apiContext, term.responseData.id); + await waitForInheritedReviewer(apiContext, term.responseData.id); await afterAction(); await page.goto( From 2e0a8774794b00589eec98f4c78327305ff3f305 Mon Sep 17 00:00:00 2001 From: Ram Narayan Balaji Date: Fri, 7 Aug 2026 20:12:23 +0530 Subject: [PATCH 12/13] test(glossary): isolate inherited reviewer tests --- .../GlossaryInheritedReviewerApproval.spec.ts | 289 +++++++++--------- 1 file changed, 153 insertions(+), 136 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts index e8e65802394a..04a8d4c6a0b2 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -21,13 +21,9 @@ import { uuid } from '../../../utils/common'; * Reproduction for the Glossary Approval bug where a term whose reviewers are INHERITED from its * glossary is treated as having none: no approval task is created and the term settles in Draft. * - * The ordering matters. The approval workflow resolves inherited fields through - * `EntityRepository.inheritanceParentCache`, a static ThreadLocal cleared only by - * `ImpersonationCleanupFilter` — a JAX-RS response filter. Workflow execution threads never pass - * through that filter, so a glossary snapshot captured while it had NO reviewers can be served - * indefinitely. These tests therefore create the glossary WITHOUT reviewers first, let a term run the - * workflow (populating that cache), and only then add the reviewer. Every term created afterwards - * must still be gated as "has reviewers". + * The ordering matters. The test creates a glossary WITHOUT reviewers first, lets a term complete + * its workflow, and only then adds the reviewer. Every term created afterwards must inherit that + * reviewer and be gated as "has reviewers". * * Terms are created in a batch because the executor uses a thread pool and only threads that cached * the stale parent misbehave — a single term could be routed to a clean thread and pass by luck. @@ -38,9 +34,6 @@ const STATUS_TIMEOUT = 120_000; test.use({ storageState: 'playwright/.auth/admin.json' }); -const reviewer = new UserClass(); -const glossary = new Glossary(); - type TermStatus = 'Draft' | 'In Review' | 'Approved' | 'Rejected' | string; const createTermWithoutReviewers = async ( @@ -115,15 +108,32 @@ const getOpenApprovalTaskCount = async ( return (body.data ?? []).length; }; -/** - * The workflow deliberately reads fresh data because its result is a gating decision. The term - * page uses the normal entity-read path, where a just-updated glossary can briefly retain its - * pre-patch cached parent. Wait until that normal GET exposes the inherited reviewer before - * asserting the rendered card; a real inheritance failure still exhausts STATUS_TIMEOUT. - */ +const addReviewerToGlossary = async ( + apiContext: APIRequestContext, + glossary: Glossary, + reviewer: UserClass +) => { + const response = await apiContext.patch( + `/api/v1/glossaries/${glossary.responseData.id}`, + { + data: [ + { + op: 'add', + path: '/reviewers', + value: [{ id: reviewer.responseData.id, type: 'user' }], + }, + ], + headers: { 'Content-Type': 'application/json-patch+json' }, + } + ); + + expect(response.status()).toBe(200); +}; + const waitForInheritedReviewer = async ( apiContext: APIRequestContext, - termId: string + termId: string, + reviewerId: string ) => { await expect .poll( @@ -133,12 +143,13 @@ const waitForInheritedReviewer = async ( ); const term = await response.json(); - return (term.reviewers ?? []).some( - ({ id }: { id: string }) => id === reviewer.responseData.id + return term.reviewers?.some( + (termReviewer: { id: string; inherited?: boolean }) => + termReviewer.id === reviewerId && termReviewer.inherited === true ); }, { - message: `Term ${termId} never exposed its inherited reviewer on a normal GET`, + message: `Term ${termId} must expose the reviewer inherited from its glossary`, timeout: STATUS_TIMEOUT, intervals: [3_000, 5_000, 10_000], } @@ -150,101 +161,78 @@ test.describe( 'Glossary Approval - inherited reviewers', { tag: ['@Features', '@Governance'] }, () => { - test.beforeAll( - 'Setup reviewer and reviewer-less glossary', - async ({ browser }) => { - const { apiContext, afterAction } = await performAdminLogin(browser); - - await reviewer.create(apiContext); - // Deliberately created with NO reviewers so the first term's workflow caches a - // reviewer-less snapshot of this glossary. - await glossary.create(apiContext); - - await afterAction(); - } - ); - - test.afterAll('Cleanup', async ({ browser }) => { - const { apiContext, afterAction } = await performAdminLogin(browser); - - await glossary.delete(apiContext); - await reviewer.delete(apiContext); - - await afterAction(); - }); - test('term inherits reviewer added to the glossary after an earlier term ran the workflow', async ({ browser, }) => { test.slow(); const { apiContext, afterAction } = await performAdminLogin(browser); - const glossaryFqn = glossary.responseData.fullyQualifiedName; - - await test.step('A term created before any reviewer exists is auto-approved', async () => { - const seed = await createTermWithoutReviewers( - apiContext, - glossaryFqn, - `seed_before_reviewer_${uuid()}` - ); - - const status = await waitForSettledStatus(apiContext, seed.id); - - expect(status).toBe('Approved'); - }); - - await test.step('Add a reviewer to the glossary', async () => { - const response = await apiContext.patch( - `/api/v1/glossaries/${glossary.responseData.id}`, - { - data: [ - { - op: 'add', - path: '/reviewers', - value: [{ id: reviewer.responseData.id, type: 'user' }], - }, - ], - headers: { 'Content-Type': 'application/json-patch+json' }, - } - ); + const reviewer = new UserClass(); + const glossary = new Glossary(); - expect(response.status()).toBe(200); - }); + try { + await reviewer.create(apiContext); + await glossary.create(apiContext); + const glossaryFqn = glossary.responseData.fullyQualifiedName; - await test.step('Every term created afterwards inherits the reviewer and gets an approval task', async () => { - for (let index = 0; index < REVIEWER_ADDED_PROBE_COUNT; index++) { - const term = await createTermWithoutReviewers( + await test.step('A term created before any reviewer exists is auto-approved', async () => { + const seed = await createTermWithoutReviewers( apiContext, glossaryFqn, - `after_reviewer_${index}_${uuid()}` + `seed_before_reviewer_${uuid()}` ); - const status = await waitForSettledStatus(apiContext, term.id); - - expect( - status, - `Term ${term.name} inherits a reviewer from the glossary, so the approval workflow ` + - `must move it to "In Review". "Draft" means the reviewers gate saw none — the ` + - `inherited-reviewer bug.` - ).toBe('In Review'); - - await expect - .poll( - () => - getOpenApprovalTaskCount(apiContext, term.fullyQualifiedName), - { - message: - `Term ${term.name} must have an open approval task for the inherited ` + - `reviewer`, - timeout: STATUS_TIMEOUT, - intervals: [3_000, 5_000, 10_000], - } - ) - .toBeGreaterThan(0); - } - }); - - await afterAction(); + const status = await waitForSettledStatus(apiContext, seed.id); + + expect(status).toBe('Approved'); + }); + + await test.step('Add a reviewer to the glossary', async () => { + await addReviewerToGlossary(apiContext, glossary, reviewer); + }); + + await test.step('Every term created afterwards inherits the reviewer and gets an approval task', async () => { + for (let index = 0; index < REVIEWER_ADDED_PROBE_COUNT; index++) { + const term = await createTermWithoutReviewers( + apiContext, + glossaryFqn, + `after_reviewer_${index}_${uuid()}` + ); + + const status = await waitForSettledStatus(apiContext, term.id); + + expect( + status, + `Term ${term.name} inherits a reviewer from the glossary, so the approval workflow ` + + `must move it to "In Review". "Draft" means the reviewers gate saw none — the ` + + `inherited-reviewer bug.` + ).toBe('In Review'); + await waitForInheritedReviewer( + apiContext, + term.id, + reviewer.responseData.id + ); + + await expect + .poll( + () => + getOpenApprovalTaskCount(apiContext, term.fullyQualifiedName), + { + message: + `Term ${term.name} must have an open approval task for the inherited ` + + `reviewer`, + timeout: STATUS_TIMEOUT, + intervals: [3_000, 5_000, 10_000], + } + ) + .toBeGreaterThan(0); + } + }); + } finally { + await glossary.delete(apiContext); + await reviewer.delete(apiContext); + await afterAction(); + } }); test('inherited reviewer is shown on the term page and it is not left in Draft', async ({ @@ -254,41 +242,70 @@ test.describe( test.slow(); const { apiContext, afterAction } = await performAdminLogin(browser); - // Built from a Glossary constructed without reviewers, so the term is created with an empty - // reviewers list and must inherit the one added to the glossary server-side. - const term = new GlossaryTerm(glossary); - await term.create(apiContext); - - await waitForSettledStatus(apiContext, term.responseData.id); - await waitForInheritedReviewer(apiContext, term.responseData.id); - await afterAction(); - - await page.goto( - `/glossary/${encodeURIComponent(term.responseData.fullyQualifiedName)}` - ); - - await expect(page.locator('[data-testid="loader"]')).toHaveCount(0); - - await test.step('Inherited reviewer is displayed', async () => { - await expect( - page.getByTestId('glossary-reviewer').getByTestId('owner-link') - ).toContainText( - reviewer.responseData.displayName ?? reviewer.responseData.name - ); - }); + const reviewer = new UserClass(); + const glossary = new Glossary(); + let term: GlossaryTerm | undefined; + + try { + await reviewer.create(apiContext); + await glossary.create(apiContext); + + await test.step('A term created before any reviewer exists is auto-approved', async () => { + const seed = await createTermWithoutReviewers( + apiContext, + glossary.responseData.fullyQualifiedName, + `seed_before_reviewer_${uuid()}` + ); + + expect(await waitForSettledStatus(apiContext, seed.id)).toBe( + 'Approved' + ); + }); - await test.step('Term reached In Review, not Draft', async () => { - await expect(page.locator('.status-badge-label')).toContainText( - 'In Review' + await test.step('Add a reviewer to the glossary', async () => { + await addReviewerToGlossary(apiContext, glossary, reviewer); + }); + + // Built from a Glossary constructed without reviewers, so the term is created with an empty + // reviewers list and must inherit the one added to the glossary server-side. + term = new GlossaryTerm(glossary); + await term.create(apiContext); + + await test.step('Term exposes the inherited reviewer and reaches In Review', async () => { + expect(await waitForSettledStatus(apiContext, term.responseData.id)).toBe( + 'In Review' + ); + await waitForInheritedReviewer( + apiContext, + term.responseData.id, + reviewer.responseData.id + ); + }); + + await page.goto( + `/glossary/${encodeURIComponent(term.responseData.fullyQualifiedName)}` ); - }); - - await test.step('Cleanup term', async () => { - const { apiContext: cleanupContext, afterAction: cleanupAction } = - await performAdminLogin(browser); - await term.delete(cleanupContext); - await cleanupAction(); - }); + + await expect(page.locator('[data-testid="loader"]')).toHaveCount(0); + + await test.step('Inherited reviewer is displayed', async () => { + await expect( + page.getByTestId('glossary-reviewer').getByTestId('owner-link') + ).toContainText( + reviewer.responseData.displayName ?? reviewer.responseData.name + ); + }); + + await test.step('Term reached In Review, not Draft', async () => { + await expect(page.locator('.status-badge-label')).toContainText( + 'In Review' + ); + }); + } finally { + await glossary.delete(apiContext); + await reviewer.delete(apiContext); + await afterAction(); + } }); } ); From ae99517ff76e7f4d1c09a1396de15b70a9eac5b7 Mon Sep 17 00:00:00 2001 From: Ram Narayan Balaji Date: Fri, 7 Aug 2026 21:55:21 +0530 Subject: [PATCH 13/13] style(glossary): format inherited reviewer test --- .../Glossary/GlossaryInheritedReviewerApproval.spec.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts index 04a8d4c6a0b2..68c3c7fd7089 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts @@ -272,9 +272,9 @@ test.describe( await term.create(apiContext); await test.step('Term exposes the inherited reviewer and reaches In Review', async () => { - expect(await waitForSettledStatus(apiContext, term.responseData.id)).toBe( - 'In Review' - ); + expect( + await waitForSettledStatus(apiContext, term.responseData.id) + ).toBe('In Review'); await waitForInheritedReviewer( apiContext, term.responseData.id, @@ -283,7 +283,9 @@ test.describe( }); await page.goto( - `/glossary/${encodeURIComponent(term.responseData.fullyQualifiedName)}` + `/glossary/${encodeURIComponent( + term.responseData.fullyQualifiedName + )}` ); await expect(page.locator('[data-testid="loader"]')).toHaveCount(0);