diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/events/ChangeEventHandler.java b/openmetadata-service/src/main/java/org/openmetadata/service/events/ChangeEventHandler.java index 0739cb2a93e..df5581a2668 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/events/ChangeEventHandler.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/events/ChangeEventHandler.java @@ -95,6 +95,7 @@ public class ChangeEventHandler implements EventHandler { .withEntityId(changeEvent.getEntityId()) .withEntityType(changeEvent.getEntityType()) .withUserName(changeEvent.getUserName()) + .withImpersonatedBy(changeEvent.getImpersonatedBy()) .withTimestamp(changeEvent.getTimestamp()) .withChangeDescription(changeEvent.getChangeDescription()) .withCurrentVersion(changeEvent.getCurrentVersion()); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/formatter/util/FormatterUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/formatter/util/FormatterUtil.java index e332c6aded8..60a71383e72 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/formatter/util/FormatterUtil.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/formatter/util/FormatterUtil.java @@ -307,6 +307,7 @@ public class FormatterUtil { ? null : entityInterface.getDomains().stream().map(EntityReference::getId).toList()) .withUserName(updateBy) + .withImpersonatedBy(entityInterface.getImpersonatedBy()) .withTimestamp(entityInterface.getUpdatedAt()) .withChangeDescription(entityInterface.getChangeDescription()) .withCurrentVersion(entityInterface.getVersion()); @@ -396,6 +397,7 @@ public class FormatterUtil { .withDomains(thread.getDomains()) .withEntityType(entityType) .withUserName(updateBy) + .withImpersonatedBy(thread.getImpersonatedBy()) .withTimestamp(thread.getUpdatedAt()); // Include changeDescription if present diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java index e2f551acc36..fd52fa07fa6 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java @@ -25,6 +25,7 @@ import org.openmetadata.service.resources.feeds.MessageParser; @Slf4j public class WorkflowEventConsumer implements Destination { + public static final String GOVERNANCE_BOT = "governance-bot"; private final SubscriptionDestination subscriptionDestination; private final EventSubscription eventSubscription; @@ -105,6 +106,18 @@ public class WorkflowEventConsumer implements Destination { EventType eventType = event.getEventType(); String entityType = event.getEntityType(); + // Skip events from governance-bot to prevent infinite loops + // These are system-initiated workflow changes that shouldn't trigger new workflows + if (GOVERNANCE_BOT.equals(event.getUserName()) + || (event.getImpersonatedBy() != null + && GOVERNANCE_BOT.equals(event.getImpersonatedBy()))) { + LOG.debug( + "Skipping workflow-initiated event from governance-bot for entity {} of type: {}", + event.getEntityFullyQualifiedName(), + event.getEntityType()); + return; + } + if (validEventTypes.contains(eventType) && validEntityTypes.contains(entityType)) { String signal = String.format("%s-%s", entityType, eventType.toString()); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/SetEntityAttributeImpl.java b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/SetEntityAttributeImpl.java index d32fb47c8bb..afcba6150d9 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/SetEntityAttributeImpl.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/SetEntityAttributeImpl.java @@ -56,14 +56,24 @@ public class SetEntityAttributeImpl implements JavaDelegate { } String updatedByNamespace = (String) inputNamespaceMap.get(UPDATED_BY_VARIABLE); - String user = + String actualUser = Optional.ofNullable(updatedByNamespace) .map(ns -> (String) varHandler.getNamespacedVariable(ns, UPDATED_BY_VARIABLE)) - .orElse("governance-bot"); + .orElse(null); - // Apply the field change using shared utility + // Apply the field change using shared utility with bot impersonation // Note: fieldValue can be null to clear/remove a field value - EntityFieldUtils.setEntityField(entity, entityType, user, fieldName, fieldValue, true); + // When actualUser is available, use it as the user and mark 'governance-bot' as impersonator + // Otherwise, use 'governance-bot' directly (for system-initiated workflows) + if (actualUser != null && !actualUser.isEmpty()) { + // User-initiated workflow: preserve actual user, mark bot as impersonator + EntityFieldUtils.setEntityField( + entity, entityType, actualUser, fieldName, fieldValue, true, "governance-bot"); + } else { + // System-initiated workflow: use governance-bot directly + EntityFieldUtils.setEntityField( + entity, entityType, "governance-bot", fieldName, fieldValue, true, null); + } } catch (Exception exc) { LOG.error( 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 7accc9807b2..d004906660a 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 @@ -1171,9 +1171,8 @@ public abstract class EntityRepository { if (updatedBy != null) { entity.setUpdatedBy(updatedBy); } - if (impersonatedBy != null) { - entity.setImpersonatedBy(impersonatedBy); - } + // Always set impersonatedBy to clear it when null (regular user operations) + entity.setImpersonatedBy(impersonatedBy); return createNewEntity(entity); } @@ -1440,9 +1439,8 @@ public abstract class EntityRepository { T original = findByNameOrNull(updated.getFullyQualifiedName(), ALL); if (original == null) { // If an original entity does not exist then create it, else update - if (impersonatedBy != null) { - updated.setImpersonatedBy(impersonatedBy); - } + // Always set impersonatedBy to clear it when null (regular user operations) + updated.setImpersonatedBy(impersonatedBy); return new PutResponse<>( Status.CREATED, withHref(uriInfo, createNewEntity(updated)), ENTITY_CREATED); } @@ -1459,9 +1457,8 @@ public abstract class EntityRepository { UriInfo uriInfo, T updated, String updatedBy, String impersonatedBy) { T original = findByNameOrNull(updated.getFullyQualifiedName(), ALL); if (original == null) { // If an original entity does not exist then create it, else update - if (impersonatedBy != null) { - updated.setImpersonatedBy(impersonatedBy); - } + // Always set impersonatedBy to clear it when null (regular user operations) + updated.setImpersonatedBy(impersonatedBy); return new PutResponse<>( Status.CREATED, withHref(uriInfo, createNewEntity(updated)), ENTITY_CREATED); } @@ -1600,9 +1597,8 @@ public abstract class EntityRepository { setFieldsInternal(original, putFields); updated.setUpdatedBy(updatedBy); updated.setUpdatedAt(System.currentTimeMillis()); - if (impersonatedBy != null) { - updated.setImpersonatedBy(impersonatedBy); - } + // Always set impersonatedBy to clear it when null (regular user operations) + updated.setImpersonatedBy(impersonatedBy); // If the entity state is soft-deleted, recursively undelete the entity and it's children if (Boolean.TRUE.equals(original.getDeleted())) { restoreEntity(updated.getUpdatedBy(), original.getId()); @@ -1633,9 +1629,8 @@ public abstract class EntityRepository { setFieldsInternal(original, putFields); updated.setUpdatedBy(updatedBy); updated.setUpdatedAt(System.currentTimeMillis()); - if (impersonatedBy != null) { - updated.setImpersonatedBy(impersonatedBy); - } + // Always set impersonatedBy to clear it when null (regular user operations) + updated.setImpersonatedBy(impersonatedBy); // If the entity state is soft-deleted, recursively undelete the entity and it's children if (Boolean.TRUE.equals(original.getDeleted())) { restoreEntity(updated.getUpdatedBy(), original.getId()); @@ -1805,10 +1800,10 @@ public abstract class EntityRepository { updated.setDomains(validatedDomains); restorePatchAttributes(original, updated); - // Set impersonatedBy after all attribute restoration to prevent it from being overwritten - if (impersonatedBy != null) { - updated.setImpersonatedBy(impersonatedBy); - } + // Always set impersonatedBy to the passed value (which can be null) + // This ensures that when regular users make changes (impersonatedBy=null), + // any existing impersonatedBy value is cleared, preventing it from persisting + updated.setImpersonatedBy(impersonatedBy); // Update the attributes and relationships of an entity EntityUpdater entityUpdater; diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java index a69576e04e9..c375389f651 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java @@ -526,6 +526,20 @@ public class FeedRepository { JsonPatch patch = JsonUtils.getJsonPatch(origJson, updatedEntityJson); EntityRepository repository = threadContext.getEntityRepository(); repository.patch(null, aboutEntity.getId(), user, patch); + if (!origJson.equals(updatedEntityJson)) { + ChangeEvent changeEvent = + new ChangeEvent() + .withId(UUID.randomUUID()) + .withEventType(EventType.ENTITY_UPDATED) + .withEntityId(aboutEntity.getId()) + .withEntityType(threadContext.getAbout().getEntityType()) + .withEntityFullyQualifiedName(aboutEntity.getFullyQualifiedName()) + .withUserName(user) + .withTimestamp(System.currentTimeMillis()) + .withEntity(updatedEntity); + + Entity.getCollectionDAO().changeEventDAO().insert(JsonUtils.pojoToMaskedJson(changeEvent)); + } // Update the attributes threadContext.getThread().getTask().withNewValue(resolveTask.getNewValue()); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityFieldUtils.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityFieldUtils.java index 7f1829cb2b5..804b0fb0636 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityFieldUtils.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/EntityFieldUtils.java @@ -17,6 +17,7 @@ import jakarta.json.JsonPatch; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; +import java.util.UUID; import lombok.extern.slf4j.Slf4j; import org.openmetadata.schema.EntityInterface; import org.openmetadata.schema.entity.classification.Tag; @@ -24,8 +25,10 @@ import org.openmetadata.schema.entity.data.GlossaryTerm; import org.openmetadata.schema.entity.teams.Team; import org.openmetadata.schema.entity.teams.User; import org.openmetadata.schema.type.AssetCertification; +import org.openmetadata.schema.type.ChangeEvent; import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.EntityStatus; +import org.openmetadata.schema.type.EventType; import org.openmetadata.schema.type.Include; import org.openmetadata.schema.type.TagLabel; import org.openmetadata.schema.utils.JsonUtils; @@ -59,6 +62,17 @@ public class EntityFieldUtils { String fieldName, String fieldValue, boolean applyPatch) { + setEntityField(entity, entityType, user, fieldName, fieldValue, applyPatch, null); + } + + public static void setEntityField( + EntityInterface entity, + String entityType, + String user, + String fieldName, + String fieldValue, + boolean applyPatch, + String impersonatedBy) { // Store original state for patch creation String originalJson = applyPatch ? JsonUtils.pojoToJson(entity) : null; @@ -121,8 +135,23 @@ public class EntityFieldUtils { if (applyPatch) { String updatedJson = JsonUtils.pojoToJson(entity); JsonPatch patch = JsonUtils.getJsonPatch(originalJson, updatedJson); - EntityRepository entityRepository = Entity.getEntityRepository(entityType); - entityRepository.patch(null, entity.getId(), user, patch, null); + if (!originalJson.equals(updatedJson)) { + EntityRepository entityRepository = Entity.getEntityRepository(entityType); + entityRepository.patch(null, entity.getId(), user, patch, null, impersonatedBy); + ChangeEvent changeEvent = + new ChangeEvent() + .withId(UUID.randomUUID()) + .withEventType(EventType.ENTITY_UPDATED) + .withEntityId(entity.getId()) + .withEntityType(entityType) + .withEntityFullyQualifiedName(entity.getFullyQualifiedName()) + .withUserName(user) + .withImpersonatedBy(impersonatedBy) + .withTimestamp(System.currentTimeMillis()) + .withEntity(entity); + + Entity.getCollectionDAO().changeEventDAO().insert(JsonUtils.pojoToMaskedJson(changeEvent)); + } } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java index a59f7a042e9..ec9d381c44e 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java @@ -1277,7 +1277,7 @@ public class GlossaryResourceTest extends EntityResourceTest page2 = getAssets(term.getId(), 2, 2, ADMIN_AUTH_HEADERS); assertFalse(page2.getData().isEmpty()); } + + @Test + void test_WorkflowTriggerOnDescriptionApprovalByNonReviewer(TestInfo test) throws Exception { + // Test scenario: + // 1. Create a glossary term with USER1 as reviewer (should be auto-approved) + // 2. USER1 requests a description update, asking USER2 (non-reviewer) for approval + // 3. When USER2 approves, verify the workflow IS triggered: + // - Description is updated + // - Term moves to IN_REVIEW status + // - Approval task is created for USER1 + + try { + // Step 1: Create glossary with no reviewers + // Use simple names without special characters to avoid SQL syntax issues + String simpleName = "glossary_workflow_test_" + System.currentTimeMillis(); + Glossary glossary = createGlossary(simpleName, null, null); + + // Create term with USER1 as reviewer + String termName = "term_workflow_test_" + System.currentTimeMillis(); + CreateGlossaryTerm createRequest = + createRequest(termName) + .withDescription("Initial description") + .withGlossary(glossary.getFullyQualifiedName()) + .withReviewers(listOf(USER1.getEntityReference())); + + // Create as USER1 (who is the reviewer) - should be auto-approved + GlossaryTerm term = createEntity(createRequest, authHeaders(USER1.getName())); + + // Wait a bit for any workflow to process + java.lang.Thread.sleep(2000); + + // Verify term is approved since creator is the reviewer + GlossaryTerm autoApprovedTerm = getEntity(term.getId(), "", authHeaders(USER1.getName())); + assertEquals( + EntityStatus.APPROVED, + autoApprovedTerm.getEntityStatus(), + "Term should be auto-approved when creator is reviewer"); + + // Record initial version for later comparison + double initialVersion = autoApprovedTerm.getVersion(); + + // Step 2: USER1 (reviewer) requests a description update, asking USER2 for approval + // Create UpdateDescription task + String newDescription = "Updated description needing approval"; + String entityLink = + new MessageParser.EntityLink(Entity.GLOSSARY_TERM, term.getFullyQualifiedName()) + .getLinkString(); + + CreateTaskDetails taskDetails = + new CreateTaskDetails() + .withType(TaskType.UpdateDescription) + .withOldValue(term.getDescription()) + .withSuggestion(newDescription) + .withAssignees(List.of(USER2.getEntityReference())); + + CreateThread createThread = + new CreateThread() + .withMessage("Please approve this description update") + .withFrom(USER1.getName()) + .withAbout(entityLink) + .withTaskDetails(taskDetails) + .withType(ThreadType.Task); + + Thread descriptionTask = taskTest.createAndCheck(createThread, authHeaders(USER1.getName())); + assertNotNull(descriptionTask); + assertEquals(TaskStatus.Open, descriptionTask.getTask().getStatus()); + + // Verify that USER2 can see the task + ThreadList tasks = + taskTest.listTasks(entityLink, null, null, null, 100, authHeaders(USER2.getName())); + assertTrue( + tasks.getData().stream().anyMatch(t -> t.getId().equals(descriptionTask.getId())), + "USER2 should be able to see the task"); + + // Step 3: USER2 (non-reviewer) approves the description update + // This should trigger a workflow because USER2 is NOT a reviewer + + // USER2 resolves the task (approves the description change) + ResolveTask resolveTask = new ResolveTask().withNewValue(newDescription); + taskTest.resolveTask( + descriptionTask.getTask().getId(), resolveTask, authHeaders(USER2.getName())); + + // Task resolution should have closed the description task immediately + // Wait for the ChangeEvent to be processed and workflow to trigger + java.lang.Thread.sleep(15000); // Give enough time for workflow processing + + // Step 4: Verify the workflow was triggered + // When a non-reviewer (USER2) approves a change, the workflow should: + // 1. Update the description (immediate effect) + // 2. Move the term to IN_REVIEW status + // 3. Create a new approval task for the actual reviewers (USER1) + + GlossaryTerm updatedTerm = getEntity(term.getId(), "", ADMIN_AUTH_HEADERS); + + // Verify description was updated immediately + assertEquals( + newDescription, + updatedTerm.getDescription(), + "Description should be updated after approval"); + + // CRITICAL: Verify term moved to IN_REVIEW status (workflow was triggered) + assertEquals( + EntityStatus.IN_REVIEW, + updatedTerm.getEntityStatus(), + "Term MUST move to IN_REVIEW when non-reviewer approves changes - this proves workflow triggered"); + + // Verify version was incremented (entity was modified) + assertTrue( + updatedTerm.getVersion() > initialVersion, + "Version should be incremented after task resolution and workflow processing"); + + // Step 5: Verify a new approval task was created for USER1 (the reviewer) + // Wait a bit more for task creation + java.lang.Thread.sleep(5000); + + // The workflow MUST create an approval task + Thread approvalTask = assertApprovalTask(term, TaskStatus.Open); + assertNotNull(approvalTask, "Workflow MUST create an approval task for the reviewer"); + + // Verify the task is assigned to USER1 (the reviewer) + assertTrue( + approvalTask.getTask().getAssignees().stream() + .anyMatch(a -> a.getId().equals(USER1.getEntityReference().getId())), + "The approval task MUST be assigned to USER1 (the reviewer)"); + + LOG.info( + "Test completed: Workflow successfully triggered when non-reviewer approved description change"); + + // Clean up: Resolve the approval task + try { + taskTest.resolveTask( + approvalTask.getTask().getId(), + new ResolveTask().withNewValue("Approved"), + authHeaders(USER1.getName())); + java.lang.Thread.sleep(2000); + } catch (Exception e) { + // Ignore cleanup errors + } + + } finally { + // Clean up: Re-suspend the workflow to not affect other tests + WorkflowHandler.getInstance().suspendWorkflow("GlossaryTermApprovalWorkflow"); + } + } } diff --git a/openmetadata-spec/src/main/resources/json/schema/type/changeEvent.json b/openmetadata-spec/src/main/resources/json/schema/type/changeEvent.json index 7d6db2114d3..be7a76028b9 100644 --- a/openmetadata-spec/src/main/resources/json/schema/type/changeEvent.json +++ b/openmetadata-spec/src/main/resources/json/schema/type/changeEvent.json @@ -45,6 +45,10 @@ "description": "Name of the user whose activity resulted in the change.", "type": "string" }, + "impersonatedBy": { + "description": "Bot user that performed the action on behalf of the actual user.", + "$ref": "../type/basic.json#/definitions/impersonatedBy" + }, "timestamp": { "description": "Timestamp when the change was made in Unix epoch time milliseconds.", "$ref": "basic.json#/definitions/timestamp" diff --git a/openmetadata-ui/src/main/resources/ui/src/generated/events/api/eventSubscriptionDiagnosticInfo.ts b/openmetadata-ui/src/main/resources/ui/src/generated/events/api/eventSubscriptionDiagnosticInfo.ts index 9c3c3fc2dd3..25a453ff2d0 100644 --- a/openmetadata-ui/src/main/resources/ui/src/generated/events/api/eventSubscriptionDiagnosticInfo.ts +++ b/openmetadata-ui/src/main/resources/ui/src/generated/events/api/eventSubscriptionDiagnosticInfo.ts @@ -105,6 +105,10 @@ export interface ChangeEvent { * Unique identifier for the event. */ id: string; + /** + * Bot user that performed the action on behalf of the actual user. + */ + impersonatedBy?: string; /** * Change that lead to this version of the entity. */ diff --git a/openmetadata-ui/src/main/resources/ui/src/generated/events/api/typedEvent.ts b/openmetadata-ui/src/main/resources/ui/src/generated/events/api/typedEvent.ts index 65d225cba01..f8968dcbb02 100644 --- a/openmetadata-ui/src/main/resources/ui/src/generated/events/api/typedEvent.ts +++ b/openmetadata-ui/src/main/resources/ui/src/generated/events/api/typedEvent.ts @@ -79,6 +79,10 @@ export interface ChangeEvent { * Unique identifier for the event. */ id?: string; + /** + * Bot user that performed the action on behalf of the actual user. + */ + impersonatedBy?: string; /** * Change that lead to this version of the entity. */ @@ -235,6 +239,10 @@ export interface ChangeEventClass { * Unique identifier for the event. */ id: string; + /** + * Bot user that performed the action on behalf of the actual user. + */ + impersonatedBy?: string; /** * Change that lead to this version of the entity. */ diff --git a/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEvent.ts b/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEvent.ts index a154d431464..2d301b4f6b6 100644 --- a/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEvent.ts +++ b/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEvent.ts @@ -85,6 +85,10 @@ export interface ChangeEvent { * Unique identifier for the event. */ id: string; + /** + * Bot user that performed the action on behalf of the actual user. + */ + impersonatedBy?: string; /** * Change that lead to this version of the entity. */ diff --git a/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEventResponse.ts b/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEventResponse.ts index 43fd2aa53fb..cfc953f2c3f 100644 --- a/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEventResponse.ts +++ b/openmetadata-ui/src/main/resources/ui/src/generated/events/failedEventResponse.ts @@ -85,6 +85,10 @@ export interface ChangeEvent { * Unique identifier for the event. */ id: string; + /** + * Bot user that performed the action on behalf of the actual user. + */ + impersonatedBy?: string; /** * Change that lead to this version of the entity. */ diff --git a/openmetadata-ui/src/main/resources/ui/src/generated/type/changeEvent.ts b/openmetadata-ui/src/main/resources/ui/src/generated/type/changeEvent.ts index 12a2f5e92a6..bc54cc11b0b 100644 --- a/openmetadata-ui/src/main/resources/ui/src/generated/type/changeEvent.ts +++ b/openmetadata-ui/src/main/resources/ui/src/generated/type/changeEvent.ts @@ -57,6 +57,10 @@ export interface ChangeEvent { * Unique identifier for the event. */ id: string; + /** + * Bot user that performed the action on behalf of the actual user. + */ + impersonatedBy?: string; /** * Change that lead to this version of the entity. */