Approval workflow for classification feedback

[edg956]

Describe your changes:

Implemets #25177

Here's how the new feedback looks like

---

Summary by Gitar

• Feedback approval workflow:
  • New RecognizerFeedback schema tracks user feedback on auto-applied classification tags with 4 feedback types (FALSE_POSITIVE, INCORRECT_CLASSIFICATION, OVERLY_BROAD, CONTEXT_SPECIFIC)
  • Workflow nodes for user approval task (createRecognizerFeedbackApprovalTask) and automated apply/reject tasks with configurable thresholds
  • Auto-approval logic in CheckFeedbackSubmitterIsReviewerImpl when feedback submitter is also the reviewer
• Workflow event handling:
  • Registry-based handler pattern in WorkflowEventConsumer with entity-specific handlers for RECOGNIZER_FEEDBACK events
  • New Registry utility class supports default fallback behavior for unregistered entity types
• New UI component:
  • FeedbackApprovalTask.tsx displays feedback details including type, submitter, timestamp, and entity link with proper i18n support
• Task system integration:
  • Modified TaskFeedCard and TaskTab components to recognize RecognizerFeedbackApproval task type and handle approval/rejection like glossary tasks

_{This will update automatically on new commits.}

---

Type of change:

• Bug fix
• Improvement
• New feature
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation

Checklist:

• I have read the CONTRIBUTING document.
• My PR title is Fixes <issue-number>: <short explanation>
• I have commented on my code, particularly in hard-to-understand areas.
• For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

[github-actions]

Jest test Coverage

UI tests summary

Lines	Statements	Branches	Functions
	66.2% (54843/82850)	44.9% (28109/62606)	47.95% (8617/17971)

[gitar-bot]

🔍 CI failure analysis for 78cb0bb: CI failures in maven-sonarcloud-ci and maven-postgresql-ci: 1 test failure related to PR (403 reviewer permission error in GlossaryTermResourceTest), 5 unrelated ODCS failures in DataContractResourceTest.

Issue

Maven CI Test Failures - Mixed Related and Unrelated Issues

Both maven-sonarcloud-ci (job 61160951006) and maven-postgresql-ci jobs show test failures:

• maven-postgresql-ci: 6 test failures (1 related, 5 unrelated)
• maven-sonarcloud-ci: 5 test failures (all unrelated, no GlossaryTermResourceTest failure)

Test Failure Summary

RELATED FAILURE (only in maven-postgresql-ci):

1. GlossaryTermResourceTest.testGlossaryTermCsvImportSameCSV:4494
   • Error: HttpResponseException: status code: 403, reason phrase: User 'admin' is not a reviewer
   • Location: EntityResourceTest.deleteEntity:5104
   • Issue: Test attempts to delete a glossary term but encounters 403 Forbidden error

UNRELATED FAILURES (in both CI jobs):
2-6. DataContractResourceTest (5 failures):

• testExportDataContractToODCS:5718 - expected v3.0.2 but got v3.1.0
• testExportDataContractToODCSByFqn:5768 - expected v3.0.2 but got v3.1.0
• testExportDataContractToODCSYaml:5748 - expected true but got false
• testODCSExportWithSLA:6014 - expected true but got false
• testODCSRoundTrip:5948 - expected 3 but got 1

Test Results Summary (maven-sonarcloud-ci):

• ✅ Passed: 7,762 tests
• ❌ Failed: 5 tests (all unrelated)
• ⏭️ Skipped: 701 tests
• ⏱️ Duration: ~3h 14min

Root Cause

GlossaryTermResourceTest Failure (RELATED - PostgreSQL CI only)

This PR introduces a Tag Recognizer Feedback Review Workflow with new reviewer permission checks. The failure occurs in maven-postgresql-ci but NOT in maven-sonarcloud-ci, suggesting a race condition or environmental difference.

The test testGlossaryTermCsvImportSameCSV deletes a glossary term entity, but the PR's reviewer permission validation logic appears to be triggering during entity deletion when it shouldn't:

GlossaryTermResourceTest.testGlossaryTermCsvImportSameCSV:4494
  → EntityResourceTest.deleteEntity:5104
    → HttpResponseException: 403 - User 'admin' is not a reviewer

This suggests:

1. The new workflow's reviewer validation (in CheckFeedbackSubmitterIsReviewerImpl) may be applied more broadly than intended
2. The reviewer check might be incorrectly triggered during glossary term deletion operations
3. The issue may be intermittent/race-based since it only appears in PostgreSQL CI, not SonarCloud CI

DataContractResourceTest Failures (UNRELATED)

These 5 failures appear in both CI jobs and involve ODCS (Open Data Contract Standard) export/import:

• Version mismatch: Tests expect v3.0.2 but get v3.1.0
• Export validation failures: Boolean and count assertions failing

This PR modifies 65 files related to:

• Tag recognizer feedback workflow (Java)
• UI components for feedback approval (TypeScript/React)
• Workflow definitions and schemas (JSON)
• Internationalization files (JSON)

Zero changes to:

• Data contract export/import logic
• ODCS version handling
• DataContractResourceTest or related code

These failures are likely pre-existing or due to recent dependency updates.

Details

PR Changes and Reviewer Permission Logic

The PR adds reviewer-related components:

1. New Service Tasks:

   • CheckFeedbackSubmitterIsReviewerImpl - Validates if feedback submitter is a reviewer
   • SetApprovalAssigneesImpl - Sets reviewers as approval assignees
   • AutoApproveServiceTaskImpl - Auto-approves when no reviewers or submitter is reviewer

2. Integration Tests:

   • Tests verify reviewer-based approval logic works correctly
   • All integration tests pass

3. Workflow Logic:

   setAssigneesVariable → checkSubmitterIsReviewer → submitterIsReviewerGateway
     ↓ (not reviewer)              ↓ (is reviewer)
   Create Approval Task       Auto-approve
   

The reviewer check appears to be leaking into entity deletion operations that should not require reviewer permissions.

Why DataContractResourceTest Failures Are Unrelated

Evidence of unrelated nature:

1. This PR contains zero changes to data contract code
2. Failures involve ODCS version mismatch (v3.0.2 vs v3.1.0)
3. Failures appear consistently in both CI jobs
4. The PR's scope is entirely focused on tag recognizer feedback workflows

Likely causes:

• Recent dependency update changing ODCS version
• Pre-existing issues in main branch
• Test expectations need updating

Non-Critical Warnings

Expected warnings (appear in normal test runs, non-blocking):

• Schema files not found for AI entities
• RdfRepository initialization failures
• GlossaryTermApprovalWorkflow not found during migration
• DisruptorQueue InterruptedException during test shutdown
• Settings cache failures with default fallback

These are test infrastructure artifacts and don't cause test failures.

Code Review 👍 Approved with suggestions 17 resolved / 20 findings

Well-structured approval workflow implementation with robust auto-approval logic and comprehensive UI components. One important issue around exception handling in the event consumer should be addressed.

⚠️ Bug: Missing null check on `handler.apply(event)` result

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java

In WorkflowEventConsumer.handleEvent(), after obtaining the handler and calling handler.apply(event), the code checks if (variables != null && !variables.isEmpty()). However, the handleTagRecognizerFeedback method returns an empty HashMap early if the entity type doesn't match, but it could also return null in other edge cases.

The issue is that if the handler throws an exception during execution, or if there's a mismatch between the handler registration and actual handling logic, the code may fail silently or behave unexpectedly.

Additionally, in handleTagRecognizerFeedback, if feedbackRepository.get(event.getEntityId()) throws an exception (e.g., entity not found), there's no explicit handling - it will propagate up and could cause the entire event consumer to fail.

Suggested fix: Add explicit null checking and consider wrapping the handler execution in a try-catch to handle cases where the feedback entity may have been deleted between event creation and processing, similar to how defaultHandler handles EntityNotFoundException.

More details 💡 2 suggestions ✅ 17 resolved

💡 Edge Case: No validation for `RECOGNIZER_FEEDBACK` in `validEntityTypes`

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowEventConsumer.java

The RECOGNIZER_FEEDBACK entity type is registered in the handler registry but is not added to the validEntityTypes set. While the code appears to work because the registry-based approach is used instead, this creates an inconsistency in the design:

1. Other entity types use validEntityTypes for validation
2. RECOGNIZER_FEEDBACK bypasses this validation through the registry pattern

This is not necessarily a bug since the handler registry check occurs and the default handler doesn't apply to unregistered entity types. However, it makes the code harder to understand and maintain. Consider either:

• Adding RECOGNIZER_FEEDBACK to validEntityTypes for consistency
• Adding a comment explaining the design decision that registry-based handlers don't need to be in validEntityTypes

💡 Quality: Duplicated repository instantiation pattern

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/ApplyRecognizerFeedbackImpl.java 📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/RejectRecognizerFeedbackImpl.java 📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CheckFeedbackSubmitterIsReviewerImpl.java

Throughout the new code, RecognizerFeedbackRepository is instantiated repeatedly using the same pattern:

RecognizerFeedbackRepository repo = new RecognizerFeedbackRepository(Entity.getCollectionDAO());

This pattern appears in:

• ApplyRecognizerFeedbackImpl.execute()
• RejectRecognizerFeedbackImpl.execute()
• CheckFeedbackSubmitterIsReviewerImpl.execute()
• CreateRecognizerFeedbackApprovalTaskImpl
• FilterEntityImpl
• WorkflowEventConsumer.handleTagRecognizerFeedback()
• TagRepository.RecognizerFeedbackTaskWorkflow

Consider creating a utility method or using dependency injection to reduce code duplication and make testing easier. For example:

private RecognizerFeedbackRepository getRecognizerFeedbackRepository() {
    return new RecognizerFeedbackRepository(Entity.getCollectionDAO());
}

✅ Quality: Raw Map type used without type parameters

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/ApplyRecognizerFeedbackImpl.java:45 📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/RejectRecognizerFeedbackImpl.java:44
The code uses raw Map.class type when deserializing JSON, which suppresses type safety warnings and loses compile-time type checking.

In both ApplyRecognizerFeedbackImpl.java and RejectRecognizerFeedbackImpl.java:

Map<String, Object> inputNamespaceMap =
    JsonUtils.readOrConvertValue(inputNamespaceMapExpr.getValue(execution), Map.class);

Consider using TypeReference or suppressing the warning with @SuppressWarnings("unchecked") if type erasure prevents proper generic usage here.

✅ Edge Case: NumberFormatException risk in threshold parsing

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CreateRecognizerFeedbackApprovalTaskImpl.java:73
In CreateRecognizerFeedbackApprovalTaskImpl.java, the threshold parsing uses Integer.parseInt() without a try-catch for invalid number formats. While the thresholds typically come from configuration, malformed values would cause an unhandled NumberFormatException.

if (thresholdStr != null && !thresholdStr.isEmpty()) {
    approvalThreshold = Integer.parseInt(thresholdStr);
}

Consider wrapping in try-catch to handle parse errors gracefully, or using a validation/default approach.

✅ Edge Case: Potential NPE when delegateTask.getAssignee() returns null

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CreateRecognizerFeedbackApprovalTaskImpl.java:101
In getAssignees() method, when candidates.isEmpty() is true, the code calls delegateTask.getAssignee() which may return null. This null value is then passed directly to getEntityReferenceFromLinkString() which will cause a NullPointerException in MessageParser.EntityLink.parse().

Consider adding a null check:

} else {
  String assignee = delegateTask.getAssignee();
  if (assignee != null) {
    assignees.add(getEntityReferenceFromLinkString(assignee));
  }
}

Or return an empty list when there are no candidates and no assignee, which could then be handled by the upstream code to either auto-approve or fail gracefully.

✅ Edge Case: Thread variable assigned but immediately overwritten

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CreateRecognizerFeedbackApprovalTaskImpl.java:142
In createRecognizerFeedbackApprovalTask(), the thread variable is first retrieved from feedRepository.getTask(), then set to null after termination, and immediately afterward a new Thread is created overwriting the variable. The retrieval-and-null pattern seems to exist only to terminate existing tasks but the flow could be simplified:

try {
    thread = feedRepository.getTask(about, TaskType.RecognizerFeedbackApproval, TaskStatus.Open);
    WorkflowHandler.getInstance()
        .terminateTaskProcessInstance(thread.getId(), "A Newer Process Instance is Running.");
    thread = null;  // Set to null
} catch (EntityNotFoundException ex) {
    thread = null;  // Also set to null
}
// thread is always null here, then immediately assigned
thread = new Thread()...

Consider simplifying by not using thread for the existing task lookup, using a separate local variable instead.

✅ Bug: Missing text in taskLinkTitleElement for RecognizerFeedback

📄 openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/TaskFeedCard/TaskFeedCardNew.component.tsx:163
In TaskFeedCardNew.component.tsx, when the task is a RecognizerFeedbackApproval, the entity name and entity type display is intentionally hidden ({!isRecognizerFeedback && ...}), but the remaining Typography.Text now displays {tagName} instead of the entity FQN:

{!isRecognizerFeedback && (
  <Typography.Text
    className="break-all header-link text-sm"
    data-testid="entity-link">
    {tagName}  // This should be {getNameFromFQN(entityFQN)} based on original code
  </Typography.Text>
)}

The line shows {tagName} but this block is hidden for recognizer feedback tasks. However, for non-recognizer tasks this will now show tagName (which would be empty string) instead of the entity FQN. This appears to be a regression - the original code showed {getNameFromFQN(entityFQN)} here.

...and 12 more from earlier reviews

What Works Well

Clean architecture with registry-based handler pattern for entity-specific workflow event handling. Good test coverage including integration tests for the workflow and unit tests for UI components. The auto-approval logic when the submitter is also a reviewer is a thoughtful UX improvement. Comprehensive i18n support across all 19 language files.

Recommendations

Consider adding exception handling in handleTagRecognizerFeedback similar to the defaultHandler to gracefully handle cases where the feedback entity may be deleted between event creation and processing.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ui'

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
5.6% Duplication on New Code

See analysis details on SonarQube Cloud