fix(workflows,tasks): approve/reject condition scheme, Task V2 migration & resolve, v1122 workflow-handler init, TagUpdate version bump#29931
Conversation
…ve/reject Task V2 seeds use `approve`/`reject` on outbound edges from `userApprovalTask` nodes, but the v1105 workflow migration and various runtime write paths were still emitting the legacy `true`/`false` boolean strings. This produced duplicate outbound edges on migrated glossary approval workflows, blocked Task V2 tasks from resolving against pre-migration Flowable process instances, and left auto-approval + multi-approval writing values that no longer matched the deployed BPMN. The WorkflowBuilder UI still only offered True/False when authoring a userApprovalTask edge, so admin-authored workflows silently ended up with dead edges. Backend - Reuse a single set of condition constants (`APPROVE_CONDITION`, `REJECT_CONDITION`, `LEGACY_APPROVE_CONDITION`, `LEGACY_REJECT_CONDITION`) from `Workflow`. - v1105 `MigrationUtil` now writes `approve`/`reject` on `ApprovalForUpdates` outbound edges instead of `true`/`false`. - v200 `MigrationUtil.TaskWorkflow.runRecognizerFeedbackTaskTypeMigration` now runs a one-shot approval-edge condition rewrite so any DB that already ran the buggy v1105 migration ends up with `approve`/`reject` on `userApprovalTask` outbound edges and dedups duplicates left behind by earlier runs. - `TaskWorkflowHandler.resolveWorkflowResult` inspects the deployed BPMN first via `WorkflowHandler.getExpectedResultForActiveTask`, so already-running Flowable process instances (frozen with `_result == 'true'`) keep firing their outbound edges even after the WD entity is migrated. - `WorkflowHandler.handleMultiApproval` and `AutoApproveServiceTaskImpl.resolvePositiveResult` now write whichever condition string the deployed BPMN's outbound flow expects. - Legacy `RecognizerFeedbackApproval`/`DataQualityReview` carveout removed from `TaskWorkflowHandler.resolveWorkflowResult`; the new BPMN-inspection fallback covers both. - `WorkflowDefinitionRepository.validateApprovalConditions` accepts approve/reject (plus legacy true/false) on `userApprovalTask` outbound edges. Non-approval conditional nodes continue to require true/false. - `RecognizerFeedbackReviewWorkflow.json` seed uses `approve`/`reject`. - Restore `CreateApprovalTaskImpl` as a `@Deprecated(forRemoval = true)` subclass of `CreateTask` so pre-Task-V2 Flowable ProcessDefinitions whose frozen BPMN references the historical FQN resolve to a live class instead of raising `ClassNotFoundException`. UI - Add `ConditionValue.APPROVE`/`.REJECT` and `AVAILABLE_OPTIONS.APPROVAL_CONDITION_VALUES`. - `ConnectionConditionModal` offers Approve/Reject when the source node is a `userApprovalTask`; other conditional nodes keep True/False. - `WorkflowSerializer` renders `approve` in green and `reject` in red. Tests - Update `WorkflowDefinitionResourceIT` fixtures (29 approval outbound edges swapped to approve/reject) and the corresponding validation assertion. - Update `v1105/MigrationUtilTest` fixtures + assertions for the new scheme. - Update `WorkflowOssRestrictions.spec.ts` fixtures.
yan-3005
added
the
safe to test
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| APPROVE = 'APPROVE', | ||
| REJECT = 'REJECT', |
There was a problem hiding this comment.
When the workflow builder sends the new approval option values, it can send APPROVE and REJECT, but the backend validation and BPMN result matching added here expect lowercase approve and reject. Editing an approval edge through the UI can therefore fail validation as missing approve/reject flows, even though the user selected the correct labels.
| APPROVE = 'APPROVE', | |
| REJECT = 'REJECT', | |
| APPROVE = 'approve', | |
| REJECT = 'reject', |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Address gitar-bot review notes on #29931: - v200 approval-edge migration: comment why createOrUpdate → deploy is intentional (fresh ACT_RE_PROCDEF with approve/reject conditions). - WorkflowHandler condition regex: comment the Edge#getFlowableCondition contract that emits `${var == 'X'}` for every edge, including legacy boolean conditions, so no unquoted form ever reaches the deployed BPMN.
Jest test CoverageUI tests summary
|
🔴 Playwright Results — 1 failure(s), 37 flaky✅ 4529 passed · ❌ 1 failed · 🟡 37 flaky · ⏭️ 99 skipped
Genuine Failures (failed on all attempts)❌
|
| APPROVE = 'APPROVE', | ||
| REJECT = 'REJECT', |
There was a problem hiding this comment.
The approval option values are still APPROVE and REJECT. ConnectionConditionModal now uses these values for userApprovalTask edges, so a newly selected approval edge can submit uppercase conditions. The backend approval validation is case-sensitive and accepts lowercase approve/reject plus legacy true/false, so saving a workflow edited through this modal can still fail as missing approve/reject flows. Keep the option labels as Approve/Reject, but send the backend condition strings as lowercase values.
| APPROVE = 'APPROVE', | |
| REJECT = 'REJECT', | |
| APPROVE = 'approve', | |
| REJECT = 'reject', |
Address greptile P1: WorkflowValidationService.toLowerCase() runs on save, but keeping the enum literal at 'approve'/'reject' eliminates the mismatch class entirely and matches the backend seed conditions.
MySQL 8.0 has no `CREATE INDEX IF NOT EXISTS` (Postgres does). The previous form failed at v200 with a syntax error and aborted the migration on the first run. Guard the index creation via information_schema.statistics, mirroring the approvedById guard earlier in the same file. Postgres continues to use `CREATE INDEX IF NOT EXISTS`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dition-scheme # Conflicts: # bootstrap/sql/migrations/native/2.0.0/mysql/schemaChanges.sql # bootstrap/sql/migrations/native/2.0.0/postgres/schemaChanges.sql
…nition migration
Every other version-migration that touches workflow definitions (v1105, v170,
v171, v190, v11212, v1131, v200) calls `initializeWorkflowHandler()` at the top
of `runDataMigration()`. v1122 was the only miss — it wrapped
`MigrationUtil.migrateWorkflowDefinitions()` in a defensive try/catch instead.
`migrateWorkflowDefinitions` calls `repository.createOrUpdate` on each workflow
def, which triggers `WorkflowDefinitionRepository.postUpdate` →
`WorkflowHandler.getInstance().deploy(...)`. Without initialization,
`getInstance()` throws `UnhandledServerException("WorkflowHandler is not
initialized.")` — the outer try/catch swallows it and the migration reports
SUCCESS with zero definitions actually redeployed. Workflow-backed features
(Knowledge Page approvals, Data Access Requests, Recognizer feedback,
data-quality incidents, custom workflows) may then run on stale definitions
until a server restart re-initializes and redeploys them.
Fix on both mysql/v1122 and postgres/v1122: add `initializeWorkflowHandler()`
before the migrateWorkflowDefinitions call. Keep the try/catch as
belt-and-suspenders so a single failing definition still doesn't abort the
whole hop.
Resolving a TagUpdate task on 2.0 applies the tag to the entity but leaves the entity version, changeDescription, and taskResolved change event stale — regressed vs 1.13 where a task-driven tag write bumped the entity minor version. Root cause: `TaskWorkflowHandler.applyMergeTagsAction` wrote directly to `tag_usage` via `EntityRepository.applyTags`/`applyTagsDelete`, which sidesteps the versioned entity update path (no PATCH, no store-update, no change event). Route entity-level tag tasks (fieldPath null / "tags" / "entity") through `repository.patch` via a new `FieldPathUtils.updateFieldTags` helper — mirroring the description task's `updateFieldDescription` path. It rebuilds the entity's `tags` list from the payload's `tagsToAdd`/`tagsToRemove`, diffs JSON to produce a `JsonPatch`, and applies it. Version bumps once, change event fires, alerts trigger — parity with a user-driven PATCH of entity.tags. Column/field-level tag tasks stay on the direct DAO write: reflection-based in-memory mutation on nested Column objects produces a JSON patch that `storeEntity` flags as `entityChanged=false` (nested tag lists round-trip differently through Jackson), silently dropping the tag update. Keeping the DAO path there preserves existing behavior — tags apply but no version bump, a pre-existing gap that is not this fix's scope. Refactored `applyMergeTagsAction` into a 15-line orchestrator plus a `TagResolution` record and helpers for payload extraction, patch write, and DAO write. Verified: - 2.0 fresh entity-level TagUpdate resolve: tags applied + version 0.1→0.2 - 2.0 fresh column-level TagUpdate resolve: tags applied (existing behavior) - 2.0 fresh TagUpdate close: no write, no version bump - 1.13 pending RequestTag → migrated to 2.0 → resolve: tags applied + bumped
…om/open-metadata/OpenMetadata into fix/user-approval-condition-scheme
…prawl
The prior tag-bump fix added ~260 lines to FieldPathUtils (updateFieldTags +
container-navigation + reflection setter + nested descent helpers) but only
the entity-level path was actually wired — column/field-level was intentionally
skipped because reflection-based mutation on nested Column objects makes
storeEntity flag entityChanged=false and drop the tag update. Most of that
helper was unreachable dead weight.
Collapse the fix into TaskWorkflowHandler itself: entity-level tag tasks now
snapshot entity JSON, mutate entity.tags in-memory (union-add / remove /
dedupe by tagFQN), diff, and apply via repository.patch — the same code path
a user-driven PATCH /v1/tables/{id} on tags takes. Column-level stays on the
existing applyTags/applyTagsDelete DAO write (pre-existing behavior, no bump).
FieldPathUtils reverts to its pre-fix state.
Verified on 2.0:
- fresh entity-level TagUpdate resolve: tags applied + version 0.1→0.2
- fresh column-level TagUpdate resolve: tags applied (existing behavior)
- fresh TagUpdate close: no write, no bump
…om/open-metadata/OpenMetadata into fix/user-approval-condition-scheme
|
|
Code Review 👍 Approved with suggestions 5 resolved / 6 findingsStandardizes workflow approval edges to 'approve'/'reject' across backend, UI, and migrations with runtime BPMN inspection to maintain legacy support. Address the unbounded semaphore acquire in the task-resolution process to prevent potential thread exhaustion. 💡 Performance: Task-resolution semaphore uses unbounded acquire() on request threadsresolveTaskInternal now calls taskResolutionPermits.acquire() with no timeout, and it is reached from HTTP request threads (e.g. EntityRepository/TagRepository approval updates via TaskWorkflowHandler.resolveTask). If the 8 permits are held by resolutions that are slow or stuck touching Flowable, incoming request threads block here indefinitely, past their own client timeouts, with no upper bound on the in-JVM queue depth. Consider tryAcquire(permit, timeout) and returning false (or surfacing a 503/retryable error) when the wait exceeds a bound, so a saturated Flowable pool degrades to a fast, visible failure instead of silently piling up blocked request threads. Bound the wait for a permit so request threads fail fast instead of blocking indefinitely.✅ 5 resolved✅ Performance: v200 migration redeploys every WorkflowDefinition on edge rewrite
✅ Quality: Condition-value extraction from BPMN relies on brittle regex
✅ Bug: SUPERSEDED_BY_NEWER_RUN not whitelisted in WorkflowFailureListener
✅ Performance: List endpoints now eagerly hydrate all heavy fields by default
✅ Quality: Duplicated legacy umbrella instance-id resolution logic
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Summary
approve/rejecton userApprovalTask outbound edges, but v1105 migration and runtime paths were still emitting legacytrue/false, producing duplicate edges + stuck resolves.approve/rejectacross backend + UI + tests, with a BPMN-inspection safety layer so pre-migration Flowable process instances keep working.CreateApprovalTaskImplas a deprecated shim so any frozen pre-Task-V2 ProcessDefinition still resolves the class it references.Backend
Workflow:APPROVE_CONDITION/REJECT_CONDITION/LEGACY_APPROVE_CONDITION/LEGACY_REJECT_CONDITION.MigrationUtilnow writesapprove/rejectonApprovalForUpdatesoutbound edges.MigrationUtil.TaskWorkflow.runRecognizerFeedbackTaskTypeMigrationruns a one-shot rewrite that normalizesuserApprovalTaskoutbound edges toapprove/rejectand dedups leftover duplicates from prior v1105 runs.TaskWorkflowHandler.resolveWorkflowResultnow inspects the deployed BPMN first via a newWorkflowHandler.getExpectedResultForActiveTaskhelper, so running Flowable instances get whichever condition string their frozen BPMN expects.WorkflowHandler.handleMultiApprovalandAutoApproveServiceTaskImpl.resolvePositiveResultwrite the deployed BPMN's expected condition string.RecognizerFeedbackApproval/DataQualityReviewcarveout inTaskWorkflowHandler— the BPMN-inspection fallback covers both.WorkflowDefinitionRepository.validateApprovalConditionsaccepts approve/reject (plus legacy true/false) on userApprovalTask outbound edges. Non-approval conditional nodes still require true/false.RecognizerFeedbackReviewWorkflow.jsonseed usesapprove/reject.CreateApprovalTaskImplas an empty@Deprecated(forRemoval = true, since = "2.0") extends CreateTaskshim so pre-Task-V2 ProcessDefinitions whose BPMN references the historical FQN don't hitClassNotFoundException.UI
WorkflowBuilder.constants.tsaddsConditionValue.APPROVE/.REJECTandAVAILABLE_OPTIONS.APPROVAL_CONDITION_VALUES.ConnectionConditionModal.tsxoffers Approve/Reject when the source node isuserApprovalTask(other conditional nodes keep True/False).WorkflowSerializer.tsrendersapprovein green andrejectin red.Tests
WorkflowDefinitionResourceIT— 29 approval outbound edges rewritten fromtrue/falsetoapprove/reject; corresponding validation assertion updated.v1105/MigrationUtilTest— migrated fixture + assertion updated.WorkflowOssRestrictions.spec.ts— 4 approval outbound edges rewritten.Test plan
mvn -pl openmetadata-service compilemvn -pl openmetadata-service test-compilemvn -pl openmetadata-service spotless:checkmvn -pl openmetadata-service test -Dtest='MigrationUtil*,TaskWorkflow*Test,WorkflowHandler*Test'— 169 tests, 0 failuresmvn -pl openmetadata-integration-tests test -Dtest='WorkflowDefinitionResourceIT'— 53 tests, 0 failures, 9 pre-existing @disabled skipsFollow-up in this PR — v200 open-task + suggestion migration + resolve
Closes #30134. Additional fixes covering the migration + resolve path for open tasks and suggestions, surfaced end-to-end while validating the approve/reject work. Each landed as its own commit so bisect stays useful.
Migration + runtime resolution
WorkflowHandler.getRuntimeWorkflowInstanceIdfalls back toworkflowInstanceExecutionId→workflow_instance_state_time_serieswhen the task-scopedworkflowInstanceIdFlowable variable is missing (pre-2.0 umbrellas never wrote it).migrateThreadTasksToTaskEntity) sets the umbrellaworkflowInstanceIdon every migrated Task V2 row via a shared static helper. Closes theshouldCreateWorkflowManagedTaskgate sopostCreate → triggerWorkflowManagedTaskdoes not spawn a duplicate task-scoped subprocess. The 1.13 umbrella keeps ownership of the Flowable userTask; the resolve endpoint completes it, the umbrella advances, and the History card reaches FINISHED.about.idby FQN viaEntityRepository.getByNamewhen the source thread / suggestion JSON lacks it. TheMENTIONED_INentity_relationship row now lands so the Task V2 resolve endpoint can patch the target entity (previously silent no-op on migratedSuggestion/DescriptionUpdate/TagUpdate).Tag / suggestion resolve
SuggestTagLabelmigration writespayload.fieldPath = "tags"explicitly. Without this the tag JSON was migrated withfieldPath = "description"becauseextractFieldPathFromEntityLinkdefaults there for entity-level entity links.TaskWorkflowHandler.resolveTagTargetFqnreturns the entity FQN (not<entityFQN>.tags) whenfieldPathis"tags"or ends in.tags. PreviouslyapplyTagswrote atag_usagerow keyed on<entityFQN>.tagswhich no reader ever joined against — the entity read back withtags: [].Read-path field defaults
TaskResourcelist endpoints defaultfields=to a narrowLIST_FIELDS = "assignees,about,createdBy". Enough for the UI card (fixes the "No Owners" render, since the UI wasn't passingfields=explicitly); no per-row hydration of the heaviercomments/payload/domains/watchersrelationships. Single-task GETs (/tasks/{id},/tasks/name/{fqn}) still default to the fullFIELDSset — one task, ~8 relationship queries, not a page-hydration concern.Perf
ACT_HI_VARINST → workflow_instance_state_time_seriesumbrella lookup: one join per migration page instead of two per row. Consolidated the per-row helper intolookupLegacyUmbrellaWorkflowInstanceIds(Handle, Collection<String>); both the raw-SQL migration and the Task V2 cutover pre-collect thread ids and use the batched form. Prevents the CLI JDBI handle from holding for hours on catalogs with thousands of pre-2.0 open tasks.workflow_instance_state_time_series(workflowInstanceExecutionId)in v200schemaChanges.sql(MySQL guarded viainformation_schema.statisticssince MySQL 8.0 has noCREATE INDEX IF NOT EXISTS; Postgres viaCREATE INDEX IF NOT EXISTS) so both the batched migration join and the runtime executionId fallback avoid full-scanning the state series.Verification — fresh 1.13 → 2.0 upgrade with a mixed data set (open GlossaryApproval, closed GlossaryApproval, DescriptionUpdate, TagUpdate, SuggestDescription, SuggestTagLabel):
RUNNINGApproved, WIFINISHED, card 9/9 stages FINISHEDApproved, description unchangedApproved, description unchangedApproved,tags: []Approved,tags: []GET /v1/tasks[/{id}]assignees: [],about: nullon default projectionSee #30134 for the full failure-mode summary.
Follow-up commits after initial review
fix(migration,v1122)— every other version-migration that touches workflow definitions (v1105,v170,v171,v190,v11212,v1131,v200) callsinitializeWorkflowHandler()at the top ofrunDataMigration(). v1122 was the only miss — it wrappedMigrationUtil.migrateWorkflowDefinitions()in a defensive try/catch instead.migrateWorkflowDefinitionscallsrepository.createOrUpdateon each workflow def, which triggersWorkflowDefinitionRepository.postUpdate→WorkflowHandler.getInstance().deploy(...). Without initialization,getInstance()throwsUnhandledServerException("WorkflowHandler is not initialized.")— the outer try/catch swallows it and the migration reports SUCCESS with zero definitions actually redeployed. Workflow-backed features (Knowledge Page approvals, DAR, Recognizer feedback, DQ incidents, custom workflows) may then run on stale definitions until a server restart re-initializes and redeploys them. Fix addsinitializeWorkflowHandler()on bothmysql/v1122andpostgres/v1122, keeps the try/catch as belt-and-suspenders.fix(tasks)— entity version bump on TagUpdate resolve — resolving a TagUpdate task on 2.0 applied the tag totag_usagebut left the entity version,changeDescription, andtaskResolvedchange event stale (regressed vs 1.13 where a task-driven tag write bumped the entity minor version).TaskWorkflowHandler.applyMergeTagsActionwrote directly totag_usageviaEntityRepository.applyTags/applyTagsDelete, sidestepping the versioned entity update path. Entity-level tag tasks now snapshot entity JSON, mutateentity.tagsin-memory (union-add / remove / dedupe by tagFQN), diff, and apply viarepository.patch— the same code path a user-drivenPATCH /v1/tables/{id}on tags takes. Version bumps once, change event fires,tag_usagesyncs. Column/field-level tag tasks stay on the direct DAO write: reflection-based mutation on nestedColumnobjects produces a JSON patch thatstoreEntityflags asentityChanged=false(nested tag lists round-trip differently through Jackson) and silently drops the tag update, so the DAO path preserves existing tag-write behavior there (tags apply, no bump — pre-existing gap not in scope).0.10.1 → 0.2, change event firesVerified 2.0 fresh and 1.13 → 2.0 migrated on MySQL.
Greptile Summary
This PR updates approval workflow handling and Task V2 migration paths. The main changes are:
approveandreject.Confidence Score: 5/5
This looks safe to merge from the scoped follow-up review.
Important Files Changed
Comments Outside Diff (1)
openmetadata-ui/src/main/resources/ui/src/components/WorkflowDefinitions/WorkflowBuilder/ConnectionConditionModal.tsx, line 108-133 (link)An existing approval edge with
condition.valueset toREJECTstill fails this strict option check because the available approval options are lowercase. When the modal opens, the value is treated as invalid and replaced with the defaultapprove. Saving after that can turn the reject branch into an approve branch, or make the backend reject the workflow as missing a reject path. Normalize approval values before validating them instead of defaulting unknown casing to approve.Reviews (20): Last reviewed commit: "Merge branch 'fix/user-approval-conditio..." | Re-trigger Greptile
Context used (3)