Fixes #: Activity feed creation API restrictions and UI JSON parsing crash#26679
Fixes #: Activity feed creation API restrictions and UI JSON parsing crash#26679
Conversation
Co-authored-by: aniketkatkar97 <51777795+aniketkatkar97@users.noreply.github.com> Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/5e663e5d-d6cb-45d3-8a84-971ab57b20bc
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
sonika-shah
added
safe to test
There was a problem hiding this comment.
Pull request overview
This PR tightens backend validation for activity feed thread creation and hardens the UI against previously-persisted corrupted tag-task payloads that could crash the Tasks UI during render.
Changes:
- Backend: Rejects
taskDetailson non-Taskthread types and introducesvalidateTaskDetails()checks for tag-task JSON fields. - UI: Wraps
oldValue/newValue/suggestionJSON parsing inuseMemowithtry/catchfallbacks to prevent render-time crashes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java | Adds taskDetails restrictions by thread type and validates tag-task JSON payload inputs during thread creation. |
| openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/shared/TagsTask.tsx | Adds safe parsing for persisted tag-task JSON fields to avoid UI crashes when encountering bad data. |
Jest test CoverageUI tests summary
|
🟡 Playwright Results — all passed (26 flaky)✅ 3659 passed · ❌ 0 failed · 🟡 26 flaky · ⏭️ 89 skipped
🟡 26 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
| ); | ||
| } | ||
| }, [oldValue, newValue]); | ||
| }, [oldValue, newValue, parsedOldValue, parsedNewValue]); |
There was a problem hiding this comment.
💡 Quality: useMemo deps include both raw and parsed values redundantly
The diffView and suggestionDiff useMemo hooks at lines 92 and 112 list both the raw values (oldValue, newValue, suggestion) AND the parsed memoized values (parsedOldValue, parsedNewValue, parsedSuggestion) as dependencies. Since the parsed values are themselves derived from the raw values via useMemo, including both is redundant — the parsed values will always change when raw values change. The dependency arrays should only include the parsed values, since those are the only variables actually used inside the memo callbacks.
Suggested fix:
Change the dependency arrays to only include the parsed values:
// line 92
}, [parsedOldValue, parsedNewValue]);
// line 112
}, [parsedOldValue, parsedSuggestion]);
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
| private Thread createThread(ThreadContext threadContext) { | ||
| Thread thread = threadContext.getThread(); | ||
| if (thread.getType() == ThreadType.Task) { | ||
| validateTaskDetails(thread); | ||
| validateAssignee(thread); | ||
| thread.getTask().withId(getNextTaskId()); | ||
| } else if (thread.getType() == ThreadType.Announcement) { | ||
| // Validate start and end time for announcement | ||
| validateAnnouncement(thread); | ||
| } else if (thread.getTask() != null) { | ||
| throw new IllegalArgumentException( | ||
| "taskDetails can only be provided for threads of type Task"); | ||
| } |
There was a problem hiding this comment.
createThread currently rejects taskDetails for non-Task threads only in the final else if branch. If a client posts an Announcement thread with non-null taskDetails, it will hit the Announcement branch and bypass the rejection, so corrupted task payloads can still be persisted on announcements. Consider checking thread.getTask() != null for all non-ThreadType.Task threads (including Announcement) before the type-specific branches, or add the same rejection inside the Announcement branch.
| "taskDetails can only be provided for threads of type Task"); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
The new integration coverage validates that Conversation threads with non-null taskDetails are rejected, but it doesn’t cover Announcement threads carrying taskDetails. Since createThread has a dedicated Announcement branch, adding a similar 400 test for ThreadType.Announcement would help prevent regressions where announcements accidentally accept and persist task payloads.
| @Test | |
| @Test | |
| void post_announcementThreadWithTaskDetails_400(TestNamespace ns) throws Exception { | |
| Table table = createTestTable(ns); | |
| String about = String.format("<#E::table::%s>", table.getFullyQualifiedName()); | |
| User assigneeUser = SdkClients.adminClient().users().getByName("admin"); | |
| EntityReference assignee = assigneeUser.getEntityReference(); | |
| CreateTaskDetails taskDetails = | |
| new CreateTaskDetails() | |
| .withType(TaskType.RequestDescription) | |
| .withAssignees(List.of(assignee)) | |
| .withOldValue("old") | |
| .withSuggestion("new"); | |
| AnnouncementDetails announcementDetails = | |
| new AnnouncementDetails() | |
| .withStartTime(LocalDateTime.now(ZoneOffset.UTC).plusMinutes(1)) | |
| .withEndTime(LocalDateTime.now(ZoneOffset.UTC).plusMinutes(2)); | |
| CreateThread createThread = | |
| new CreateThread() | |
| .withFrom(ADMIN_USER) | |
| .withMessage("Announcement with task payload") | |
| .withAbout(about) | |
| .withType(ThreadType.Announcement) | |
| .withAnnouncementDetails(announcementDetails) | |
| .withTaskDetails(taskDetails); | |
| assertThrows( | |
| Exception.class, | |
| () -> createThread(createThread), | |
| "taskDetails can only be provided for threads of type Task"); | |
| } | |
| @Test |
| switch (task.getType()) { | ||
| case RequestTag, UpdateTag -> { | ||
| validateTagLabelArray(task.getOldValue(), "oldValue", task.getType()); | ||
| validateTagLabelArray(task.getSuggestion(), "suggestion", task.getType()); | ||
| } | ||
| case RequestTestCaseFailureResolution, RecognizerFeedbackApproval -> { | ||
| rejectField(task.getOldValue(), "oldValue", task.getType()); | ||
| rejectField(task.getSuggestion(), "suggestion", task.getType()); | ||
| } | ||
| case RequestDescription, UpdateDescription, RequestApproval, Generic -> {} | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Switch on TaskType lacks default branch for future enum values
The switch at line 1201 explicitly lists all 8 current TaskType values, but has no default branch. If a new TaskType is added to the enum in the future, it will silently pass through validateTaskDetails without any validation — the same class of bug this PR is fixing. Adding a default branch that either throws or logs a warning would make this future-proof.
Suggested fix:
switch (task.getType()) {
case RequestTag, UpdateTag -> {
validateTagLabelArray(task.getOldValue(), "oldValue", task.getType());
validateTagLabelArray(task.getSuggestion(), "suggestion", task.getType());
}
case RequestTestCaseFailureResolution, RecognizerFeedbackApproval -> {
rejectField(task.getOldValue(), "oldValue", task.getType());
rejectField(task.getSuggestion(), "suggestion", task.getType());
}
case RequestDescription, UpdateDescription, RequestApproval, Generic -> {}
default -> LOG.warn("No field validation defined for task type: {}", task.getType());
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review 👍 Approved with suggestions 3 resolved / 5 findingsFixes activity feed creation API restrictions and UI JSON parsing crash by addressing validation gaps and redundant field checks. Consider removing redundant dependencies in useMemo hooks and adding a default branch to the TaskType switch for future enum values. 💡 Quality: useMemo deps include both raw and parsed values redundantly📄 openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/shared/TagsTask.tsx:92 📄 openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/shared/TagsTask.tsx:112 The Suggested fix💡 Edge Case: Switch on TaskType lacks default branch for future enum values📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java:1201-1211 The switch at line 1201 explicitly lists all 8 current Suggested fix✅ 3 resolved✅ Bug: isValidJson allows non-array JSON, UI expects TagLabel[]
✅ Quality: Field-name validation is redundant with Jackson deserialization
✅ Edge Case: Description tasks skip oldValue/suggestion validation entirely
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
The feed API accepted and stored tasks with plain-string values in
suggestion/oldValuefields that must be JSON arrays (tag tasks), and allowedtaskDetailson non-Taskthread types. Stored corrupted payloads caused an unrecoverable full-page crash in the UI (Unexpected token 'h', "this is a d"... is not valid JSON).Backend (
FeedRepository.java)validateTaskDetails(): forRequestTag/UpdateTagtask types, rejectssuggestionoroldValuethat is not valid JSON (viaJsonUtils.isValidJson())createThread(): rejects any thread of typeConversation,Announcement, etc. that carries a non-nulltaskDetailsUI (
TagsTask.tsx)JSON.parse(oldValue),JSON.parse(newValue),JSON.parse(suggestion)calls into individualuseMemohooks withtry/catch, falling back to[]on parse failure — prevents the error boundary from catching a render-time exception on already-persisted bad dataType of change:
Checklist:
Fixes <issue-number>: <short explanation>Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
repository.apache.org/usr/lib/jvm/temurin-21-jdk-amd64/bin/java /usr/lib/jvm/temurin-21-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.13/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.13/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.13 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.13/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/OpenMetadata/OpenMetadata/openmetadata-service org.codehaus.plexus.classworlds.launcher.Launcher spotless:check -q(dns block)s3.amazonaws.com/usr/lib/jvm/temurin-21-jdk-amd64/bin/java /usr/lib/jvm/temurin-21-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.13/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.13/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.13 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.13/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/OpenMetadata/OpenMetadata/openmetadata-service org.codehaus.plexus.classworlds.launcher.Launcher spotless:check -q(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.