fix: normalize legacy recognizer feedback review payloads

[PRADDZY]

Describe your changes:

Related to #29233

While investigating #29233, I found current integration tests already expect recognizer feedback review tasks to be exposed as DataQualityReview, so this PR does not change the public task type.

Instead, it fixes the legacy payload path that still writes and reads recognizer feedback review tasks using data and metadata.recognizer. Current resolver and UI paths expect feedback and recognizer.

This change:

• normalizes legacy DataQualityReview payloads on read in TaskRepository
• treats legacy data payloads as feedback approval tasks in TaskFormExecutionResolver
• updates the v200 migration path to emit modern payload keys
• adds regression tests for the resolver and migration behavior

Type of change:

• Bug fix

High-level design:

N/A - small change.

Tests:

Use cases covered

• Legacy recognizer feedback review tasks migrated through v200 continue to resolve as feedback approval tasks.
• New v200 migrations emit feedback and recognizer payload keys expected by current task code.

Unit tests

• I added unit tests for the new and changed logic.
• Files updated:
  • openmetadata-service/src/test/java/org/openmetadata/service/tasks/TaskFormExecutionResolverTest.java
  • openmetadata-service/src/test/java/org/openmetadata/service/migration/utils/v200/MigrationUtilTest.java
• Coverage %: not measured locally.

Backend integration tests

• Not applicable for this draft fix. Existing TagRecognizerFeedbackIT already asserts the current DataQualityReview task type expectations.

Ingestion integration tests

• Not applicable.

Playwright (UI) tests

• Not applicable.

Manual testing performed

• Ran mvn --% -pl openmetadata-service -am -DskipITs -Dtest=TaskFormExecutionResolverTest,MigrationUtilTest -Dsurefire.failIfNoSpecifiedTests=false test
• The build did not reach the touched tests because current main failed in unrelated search and elasticsearch compile paths under openmetadata-service.

UI screen recording / screenshots:

Not applicable.

Checklist:

• I have read the CONTRIBUTING document.
• My PR title is Fixes <issue-number>: <short explanation>
• My PR is linked to a GitHub issue via Fixes #<issue-number> above.
• 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.
• For UI changes: I attached a screen recording and/or screenshots above.
• I have added tests (unit / integration / Playwright as applicable) and listed them above.
• I have added a test that covers the exact scenario we are fixing.

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[gitar-bot]

💡 Quality: Generic migration reuses recognizer-feedback constant for "data" key

In the Generic branch of the v200 task payload migration, the suggestion value is now written using RecognizerFeedbackTaskPayloadKeys.LEGACY_DATA (= "data"). The Generic task type is unrelated to recognizer feedback, so borrowing a constant from RecognizerFeedbackTaskPayloadKeys is semantically misleading — a reader could reasonably infer Generic tasks are recognizer-feedback payloads. It also couples this branch to a class whose LEGACY_DATA name implies a legacy/deprecated key, whereas for Generic tasks data is the intended modern key. Functionally the string value is unchanged, so this is purely a clarity concern. Consider defining a Generic-specific constant (e.g. GENERIC_DATA = "data") local to the migration or a shared, neutrally-named payload-keys class rather than reusing the recognizer-feedback constant.

Introduce a Generic-specific constant (GENERIC_DATA_KEY = "data") instead of reusing RecognizerFeedbackTaskPayloadKeys.LEGACY_DATA.:

case "Generic" -> {
  ObjectNode payload = JsonUtils.getObjectNode();
  if (taskDetails.has("suggestion") && !taskDetails.get("suggestion").isNull()) {
    payload.put(GENERIC_DATA_KEY, taskDetails.get("suggestion").asText());
  }
  yield payload;
}

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review 👍 Approved with suggestions 4 resolved / 5 findings

Normalizes legacy recognizer feedback payloads in TaskRepository and migrations, resolving key mapping and redundancy issues. Update the v200 migration to use a dedicated constant instead of the generic recognizer-feedback key for clarity.

💡 Quality: Generic migration reuses recognizer-feedback constant for "data" key

📄 openmetadata-service/src/main/java/org/openmetadata/service/migration/utils/v200/MigrationUtil.java:856-863

In the Generic branch of the v200 task payload migration, the suggestion value is now written using RecognizerFeedbackTaskPayloadKeys.LEGACY_DATA (= "data"). The Generic task type is unrelated to recognizer feedback, so borrowing a constant from RecognizerFeedbackTaskPayloadKeys is semantically misleading — a reader could reasonably infer Generic tasks are recognizer-feedback payloads. It also couples this branch to a class whose LEGACY_DATA name implies a legacy/deprecated key, whereas for Generic tasks data is the intended modern key. Functionally the string value is unchanged, so this is purely a clarity concern. Consider defining a Generic-specific constant (e.g. GENERIC_DATA = "data") local to the migration or a shared, neutrally-named payload-keys class rather than reusing the recognizer-feedback constant.

Introduce a Generic-specific constant (GENERIC_DATA_KEY = "data") instead of reusing RecognizerFeedbackTaskPayloadKeys.LEGACY_DATA.

case "Generic" -> {
  ObjectNode payload = JsonUtils.getObjectNode();
  if (taskDetails.has("suggestion") && !taskDetails.get("suggestion").isNull()) {
    payload.put(GENERIC_DATA_KEY, taskDetails.get("suggestion").asText());
  }
  yield payload;
}

✅ 4 resolved

✅ Edge Case: hasFeedbackPayload("data") may misclassify other Review tasks

📄 openmetadata-service/src/main/java/org/openmetadata/service/tasks/TaskFormExecutionResolver.java:348-355 📄 openmetadata-service/src/main/java/org/openmetadata/service/tasks/TaskFormExecutionResolver.java:181 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TaskRepository.java:291-294
hasFeedbackPayload now returns true whenever a payload contains a top-level feedback OR data key (TaskFormExecutionResolver.java:354). This method is consulted for every TaskCategory.Review task (defaultBinding, line 181), which includes both DataQualityReview and PipelineReview (see TaskWorkflowLifecycleResolver mapping). The legacy normalization in TaskRepository is correctly gated to TaskEntityType.DataQualityReview only, but the resolver's data check is gated only by the broader Review category. The current PipelineReview default schema does not use a top-level data key, so this is not an active bug, but it is a fragile coupling: any Review-category task whose payload happens to carry a generic data field would be silently routed to FEEDBACK_APPROVAL with EDIT_ALL permission. Consider tightening the legacy check to the DataQualityReview type (mirroring the TaskRepository gate) or to a recognizer-specific marker rather than a generic data key.

✅ Quality: No unit test for TaskRepository.normalizeDataQualityReviewPayload

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TaskRepository.java:291-305
The PR adds normalizeDataQualityReviewPayload in TaskRepository (lines 291-329) to translate legacy data/metadata.recognizer payloads into feedback/recognizer on read, but the added tests only cover the resolver (TaskFormExecutionResolverTest) and the migration (MigrationUtilTest). The read-side normalization logic — including the guards (!has("feedback"), null checks on data and metadata.recognizer) and the non-object payload short-circuit — is not directly exercised by any test. Per the repository's testing philosophy (bug fixes must include a failing test for the exact scenario), add a focused test that feeds a legacy DataQualityReview payload ({data:{...}, metadata:{recognizer:{...}}}) through setFields/normalization and asserts the resulting payload exposes feedback and recognizer.

✅ Quality: Magic string literals for payload keys in normalization

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TaskRepository.java:309-321 📄 openmetadata-service/src/main/java/org/openmetadata/service/tasks/TaskFormExecutionResolver.java:354
normalizeDataQualityReviewPayload hardcodes the string literals "feedback", "data", "recognizer", and "metadata" (TaskRepository.java:309-321), and the same literals are duplicated in TaskFormExecutionResolver (hasFeedbackPayload) and in MigrationUtil's buildThreadTaskPayload. The project standards call for constants/enums instead of magic strings, and centralizing these keys would prevent drift between the writer (migration), the read-normalizer (TaskRepository), and the consumer (resolver) — the exact kind of divergence this PR is fixing. Extract these to shared constants used by all three sites.

✅ Performance: Redundant deepCopy after valueToTree in normalization

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TaskRepository.java:301-306
In normalizeDataQualityReviewPayload, JsonUtils.valueToTree(payload) already produces a freshly-allocated tree, so the subsequent ((ObjectNode) payloadNode).deepCopy() (TaskRepository.java:301-306) creates a second full copy that is not needed for correctness. Since setFields runs on every task read (including per-entity in setFieldsInBulk), this doubles JSON tree allocation for every DataQualityReview task on list/get paths. Operate directly on the payloadNode cast to ObjectNode and drop the deepCopy().

🤖 Prompt for agents

Code Review: Normalizes legacy recognizer feedback payloads in TaskRepository and migrations, resolving key mapping and redundancy issues. Update the v200 migration to use a dedicated constant instead of the generic recognizer-feedback key for clarity.

1. 💡 Quality: Generic migration reuses recognizer-feedback constant for "data" key
   Files: openmetadata-service/src/main/java/org/openmetadata/service/migration/utils/v200/MigrationUtil.java:856-863

   In the `Generic` branch of the v200 task payload migration, the suggestion value is now written using `RecognizerFeedbackTaskPayloadKeys.LEGACY_DATA` (= "data"). The `Generic` task type is unrelated to recognizer feedback, so borrowing a constant from `RecognizerFeedbackTaskPayloadKeys` is semantically misleading — a reader could reasonably infer Generic tasks are recognizer-feedback payloads. It also couples this branch to a class whose `LEGACY_DATA` name implies a legacy/deprecated key, whereas for Generic tasks `data` is the intended modern key. Functionally the string value is unchanged, so this is purely a clarity concern. Consider defining a Generic-specific constant (e.g. `GENERIC_DATA = "data"`) local to the migration or a shared, neutrally-named payload-keys class rather than reusing the recognizer-feedback constant.

   Fix (Introduce a Generic-specific constant (GENERIC_DATA_KEY = "data") instead of reusing RecognizerFeedbackTaskPayloadKeys.LEGACY_DATA.):
   case "Generic" -> {
     ObjectNode payload = JsonUtils.getObjectNode();
     if (taskDetails.has("suggestion") && !taskDetails.get("suggestion").isNull()) {
       payload.put(GENERIC_DATA_KEY, taskDetails.get("suggestion").asText());
     }
     yield payload;
   }

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose

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