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
• 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
• Type generation:
  • Generated TypeScript interfaces from JSON schemas including RecognizerFeedback with resolution tracking, status lifecycle, and recognizer metadata

_{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.25% (54097/81653)	44.71% (27560/61642)	48.14% (8522/17704)

[gitar-bot]

🔍 CI failure analysis for 763022c: maven-sonarcloud-ci: 129 failures total, TagResourceTest has only 2 pre-existing bulk loading failures. ALL recognizer feedback tests PASS. Same successful pattern as previous SonarCloud runs.

Issue

maven-sonarcloud-ci (Job 60811353904): 129 test failures

Root Cause

Same pre-existing test failures pattern - All recognizer feedback tests PASS

TagResourceTest Results:

• 109 tests run, only 2 failures
• ✅ ALL recognizer feedback tests PASSED
• ❌ 2 pre-existing failures (bulk loading efficiency)
• 8 tests skipped

Other 127 failures: Spread across many unrelated resource tests

Analysis

Consistent pattern with previous SonarCloud run (Job 60788456002):

Both SonarCloud runs show identical results:

• Previous: 130 failures, TagResourceTest: 2 failures
• Current: 129 failures, TagResourceTest: 2 failures
• Pattern: All recognizer feedback tests pass

Comparison Across All CI Jobs:

CI Job	Total Failures	TagResourceTest	Recognizer Tests
maven-postgresql-ci	133-134	2 (bulk loading)	✅ ALL PASS
maven-sonarcloud-ci	129-130	2 (bulk loading)	✅ ALL PASS
integration-tests-postgres-opensearch	5	N/A	❌ 5 FAIL
integration-tests-mysql-elasticsearch	5	N/A	❌ 5 FAIL
playwright-ci-postgresql (6,6)	2	N/A	0 (Glossary failures)
playwright-ci-postgresql (2,6)	3	N/A	0 (Glossary failures)
maven-collate-ci	External	N/A	Unknown

Summary

Status: ✅ All Recognizer Feedback Tests Pass in Unit Tests

maven-sonarcloud-ci shows the same successful pattern:

• 129 total test failures (all pre-existing)
• TagResourceTest: 109 tests, 2 failures (bulk loading efficiency only)
• ✅ All recognizer feedback tests PASS

Critical Finding:

Unit tests consistently show that:

1. Individual components work - all recognizer feedback unit tests pass
2. Integration fails - 5 integration tests fail due to workflow bugs

This confirms the code review findings that the bugs are in the workflow integration layer, not in the individual components.

Blocking Issues (from integration tests):

1. Missing null check in CreateRecognizerFeedbackApprovalTaskImpl
2. Terminated task thread not recreated properly

These bugs need to be fixed for integration tests to pass.

Code Review ⚠️ Changes requested 10 resolved / 13 findings

Well-structured approval workflow feature with comprehensive test coverage, but has 2 important logic issues to address: potential NPE with null createdBy and terminated task thread not being properly recreated.

⚠️ Edge Case: Potential NullPointerException when createdBy is null

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CreateRecognizerFeedbackApprovalTaskImpl.java:1614

In CreateRecognizerFeedbackApprovalTaskImpl.createRecognizerFeedbackApprovalTask(), the code accesses feedback.getCreatedBy().getFullyQualifiedName() without null checking. If createdBy is null, this will throw a NullPointerException.

The code at lines 1614-1618 does:

.withCreatedBy(feedback.getCreatedBy().getFullyQualifiedName())
...
.withUpdatedBy(feedback.getCreatedBy().getFullyQualifiedName())

While the publishChangeEvent method in RecognizerFeedbackRepository properly handles null with feedback.getCreatedBy() != null ? feedback.getCreatedBy().getName() : "unknown", this code path doesn't.

Suggested fix:

String createdByFqn = feedback.getCreatedBy() != null 
    ? feedback.getCreatedBy().getFullyQualifiedName() 
    : "unknown";
.withCreatedBy(createdByFqn)
.withUpdatedBy(createdByFqn)

⚠️ Bug: Existing task thread is terminated but not recreated properly

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/CreateRecognizerFeedbackApprovalTaskImpl.java:1593

In CreateRecognizerFeedbackApprovalTaskImpl.createRecognizerFeedbackApprovalTask(), when an existing open task is found, it terminates the old process instance but then returns the stale thread object without creating a new task:

try {
  thread = feedRepository.getTask(about, TaskType.RecognizerFeedbackApproval, TaskStatus.Open);
  WorkflowHandler.getInstance()
      .terminateTaskProcessInstance(thread.getId(), "A Newer Process Instance is Running.");
} catch (EntityNotFoundException ex) {
  // Only creates new task on EntityNotFoundException
  TaskDetails taskDetails = ...
  thread = new Thread()...
  feedRepository.create(thread);
}
return thread;

If an existing open task is found, the code terminates it but returns the old, now-terminated thread instead of creating a fresh one. The new task details are only created in the catch block. This means subsequent code will be working with a stale thread reference.

Suggested fix: Move the task creation logic outside the try-catch or create a new task after terminating the old one.

More details 💡 1 suggestion ✅ 10 resolved

💡 Quality: Generic Map type used without proper type parameters

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/ApplyRecognizerFeedbackImpl.java:1074

In ApplyRecognizerFeedbackImpl.execute() and RejectRecognizerFeedbackImpl.execute(), the code reads a JSON map without specifying proper type parameters:

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

The same pattern appears in CreateRecognizerFeedbackApprovalTaskImpl.notify():

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

Using raw Map.class triggers unchecked conversion warnings and could lead to ClassCastExceptions at runtime if the JSON contains unexpected types.

Suggested fix: Consider using a more type-safe approach or adding @SuppressWarnings("unchecked") with a comment explaining why it's safe.

✅ Bug: Thread.sleep in test is error-prone and flaky

📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TagResourceIT.java:524
The integration test uses java.lang.Thread.sleep(2000) to wait for asynchronous workflow operations. This is an anti-pattern that leads to flaky tests:

• If the system is slow, 2 seconds may not be enough
• If the system is fast, the test wastes time unnecessarily

Suggested fix: Use polling with a timeout (await/until pattern) to check for the expected condition, rather than a fixed sleep. The existing waitForRecognizerFeedbackTask method shows this pattern is already used elsewhere in the codebase.

✅ Edge Case: Registry.get() lacks null key validation

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/Registry.java:17-20
The Registry.get(String key) method doesn't validate if key is null before calling values.containsKey(key). While HashMap technically handles null keys, this is likely unintended behavior. Additionally, in WorkflowEventConsumer, if event.getEntityType() returns null, the handler lookup will return the default handler which may not be appropriate.

Consider adding a null check:

public V get(String key) {
  if (key == null) return defaultValue;
  if (values.containsKey(key)) return values.get(key);
  return defaultValue;
}

Or throw an explicit exception if null keys are invalid.

✅ Edge Case: Missing null check in Registry.get() causes NPE risk

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/Registry.java:17-20
In Registry.get(), when the key exists in the map, the method correctly returns the value. However, when the key doesn't exist AND defaultValue is null (which is permitted by the constructor), the method returns null.

The issue is in WorkflowEventConsumer.sendMessage() where handlerRegistry.get(event.getEntityType()) is called. If the entity type isn't registered and defaultValue is null, the handler.accept(event) call will throw a NullPointerException.

Looking at how it's used:

private static final Registry<Consumer<ChangeEvent>> handlerRegistry =
    new Registry<>(WorkflowEventConsumer::defaultHandler);

The default is actually set, but the Registry class itself is generic and could be misused elsewhere. Additionally, in sendMessage(), there's a redundant null check if (handler == null) that would never be true with the current setup because defaultHandler is always returned.

Suggested fix: Either:

1. Make the Registry constructor require a non-null default, or
2. Have get() always return the default when key is not found (current implementation does have a bug: it checks containsKey but then returns defaultValue only if key doesn't exist, which is correct but the null check in sendMessage suggests uncertainty about the behavior)

✅ Bug: Thread.sleep in test is error-prone and flaky

📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TagResourceIT.java:524
The integration test uses Thread.sleep(1000) to wait for workflow completion:

Thread.sleep(1000);

This approach is:

1. Flaky: CI environments may be slower, causing intermittent failures
2. Slow: Always waits the full duration even if the operation completes sooner
3. Error-prone: Sleep duration may need adjustment based on system load

Suggested fix: Use an await/polling pattern with timeout instead:

import static org.awaitility.Awaitility.await;
import java.util.concurrent.TimeUnit;

await().atMost(5, TimeUnit.SECONDS)
    .pollInterval(100, TimeUnit.MILLISECONDS)
    .until(() -> {
        RecognizerFeedback result = feedbackRepository.get(feedbackId);
        return result.getStatus() == RecognizerFeedback.Status.APPLIED;
    });

✅ Edge Case: Missing null check in Registry.get() causes NPE risk

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/Registry.java:17-20
The Registry.get(String key) method doesn't handle null keys. If a null key is passed, values.containsKey(null) will work, but it's semantically incorrect and may lead to unexpected behavior.

public V get(String key) {
  if (values.containsKey(key)) return values.get(key);
  return defaultValue;
}

Impact: If Registry.get(null) is called, it returns the value associated with null key if exists, or defaultValue. This may hide bugs where callers accidentally pass null keys.

Suggested fix:

public V get(String key) {
  if (key == null) {
    return defaultValue;
  }
  if (values.containsKey(key)) return values.get(key);
  return defaultValue;
}

...and 5 more from earlier reviews

What Works Well

Clean separation of workflow components with dedicated task implementations for apply/reject operations. Good i18n coverage across 18+ language files. Comprehensive integration tests covering approval, rejection, and auto-apply scenarios with proper async handling via Awaitility.

Recommendations

Consider adding validation in RecognizerFeedbackRepository.processFeedback() to ensure createdBy is set before downstream processing relies on it.

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 Hiding non-applicable rules.

Comment with these commands to change:

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

_{Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)}

[gitar-bot]

⚠️ Edge Case: Potential NullPointerException when createdBy is null

Details

In CreateRecognizerFeedbackApprovalTaskImpl.createRecognizerFeedbackApprovalTask(), the code accesses feedback.getCreatedBy().getFullyQualifiedName() without null checking. If createdBy is null, this will throw a NullPointerException.

The code at lines 1614-1618 does:

.withCreatedBy(feedback.getCreatedBy().getFullyQualifiedName())
...
.withUpdatedBy(feedback.getCreatedBy().getFullyQualifiedName())

While the publishChangeEvent method in RecognizerFeedbackRepository properly handles null with feedback.getCreatedBy() != null ? feedback.getCreatedBy().getName() : "unknown", this code path doesn't.

Suggested fix:

String createdByFqn = feedback.getCreatedBy() != null 
    ? feedback.getCreatedBy().getFullyQualifiedName() 
    : "unknown";
.withCreatedBy(createdByFqn)
.withUpdatedBy(createdByFqn)

---

Was this helpful? React with 👍 / 👎

[gitar-bot]

⚠️ Bug: Existing task thread is terminated but not recreated properly

Details

In CreateRecognizerFeedbackApprovalTaskImpl.createRecognizerFeedbackApprovalTask(), when an existing open task is found, it terminates the old process instance but then returns the stale thread object without creating a new task:

try {
  thread = feedRepository.getTask(about, TaskType.RecognizerFeedbackApproval, TaskStatus.Open);
  WorkflowHandler.getInstance()
      .terminateTaskProcessInstance(thread.getId(), "A Newer Process Instance is Running.");
} catch (EntityNotFoundException ex) {
  // Only creates new task on EntityNotFoundException
  TaskDetails taskDetails = ...
  thread = new Thread()...
  feedRepository.create(thread);
}
return thread;

If an existing open task is found, the code terminates it but returns the old, now-terminated thread instead of creating a fresh one. The new task details are only created in the catch block. This means subsequent code will be working with a stale thread reference.

Suggested fix: Move the task creation logic outside the try-catch or create a new task after terminating the old one.

---

Was this helpful? React with 👍 / 👎

[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
6.8% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud