Skip to content

Fixes #: Activity feed creation API restrictions and UI JSON parsing crash #26679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fixes #: Activity feed creation API restrictions and UI JSON parsing crash #26679

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f4211b
Initial plan
Copilot Mar 23, 2026
489923c
Fix activity feed creation API restrictions and UI JSON parsing crash
Copilot Mar 23, 2026
e0969e0
Merge branch 'main' into copilot/fix-activity-feed-api-restrictions
aniketkatkar97 Apr 2, 2026
04f3348
Merge branch 'main' into copilot/fix-activity-feed-api-restrictions
sonika-shah Apr 5, 2026
9d05181
Fix: Validate taskDetails fields against schema on task creation + test
sonika-shah Apr 5, 2026
f2aa647
Fix: Validate task fields per type using EntityUtil and reject invali…
sonika-shah Apr 5, 2026
f91f405
fix lint
sonika-shah Apr 5, 2026
e6a15c1
Merge branch 'main' into copilot/fix-activity-feed-api-restrictions
sonika-shah Apr 5, 2026
9f06b75
Fix task field validation to use exhaustive switch and add tests for…
sonika-shah Apr 13, 2026
40a3f36
Merge branch 'main' into copilot/fix-activity-feed-api-restrictions
sonika-shah Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/FeedResourceIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,88 @@ void patch_reassignTaskToBotUser_400(TestNamespace ns) throws Exception {
deleteThread(thread.getId());
}

@Test
void post_tagTaskWithValidJsonArrays_200(TestNamespace ns) throws Exception {
Table table = createTestTable(ns);
String about =
String.format("<#E::table::%s::columns::%s::tags>", table.getFullyQualifiedName(), "id");

User assigneeUser = SdkClients.adminClient().users().getByName("admin");
EntityReference assignee = assigneeUser.getEntityReference();

String validTagJson =
"[{\"tagFQN\":\"PersonalData.Personal\",\"source\":\"Classification\","
+ "\"labelType\":\"Manual\",\"state\":\"Confirmed\"}]";

CreateTaskDetails taskDetails =
new CreateTaskDetails()
.withType(TaskType.RequestTag)
.withAssignees(List.of(assignee))
.withOldValue("[]")
.withSuggestion(validTagJson);

CreateThread createThread =
new CreateThread()
.withFrom(ADMIN_USER)
.withMessage("Add tags to column")
.withAbout(about)
.withType(ThreadType.Task)
.withTaskDetails(taskDetails);

Thread taskThread = createThread(createThread);

assertNotNull(taskThread);
assertNotNull(taskThread.getTask());
assertEquals(TaskType.RequestTag, taskThread.getTask().getType());
assertEquals(validTagJson, taskThread.getTask().getSuggestion());

deleteThread(taskThread.getId());
}

@Test
void post_conversationThreadWithTaskDetails_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");

CreateThread createThread =
new CreateThread()
.withFrom(ADMIN_USER)
.withMessage("Conversation with task payload")
.withAbout(about)
.withType(ThreadType.Conversation)
.withTaskDetails(taskDetails);

assertThrows(
Exception.class,
() -> createThread(createThread),
"taskDetails can only be provided for threads of type Task");
}

@Test
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
@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

Copilot uses AI. Check for mistakes.
void post_taskWithoutTaskDetails_400(TestNamespace ns) {
CreateThread createThread =
new CreateThread()
.withFrom(ADMIN_USER)
.withMessage("Task with no details")
.withAbout("<#E::table::some.table>")
.withType(ThreadType.Task);

assertThrows(
Exception.class,
() -> createThread(createThread),
"taskDetails is required for threads of type Task");
}

@Test
void list_threadsWithPagination(TestNamespace ns) throws Exception {
Table table = createTestTable(ns);
Expand Down
41 changes: 41 additions & 0 deletions openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,11 +502,15 @@ public Thread getTask(EntityLink about, TaskType taskType, TaskStatus taskStatus
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");
}
Comment on lines 502 to 514
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
store(threadContext);
storeRelationships(threadContext);
Expand Down Expand Up @@ -1189,6 +1193,43 @@ private static long convertSecondsToMilliseconds(long seconds) {
.toEpochMilli();
}

private void validateTaskDetails(Thread thread) {
TaskDetails task = thread.getTask();
if (task == null) {
throw new IllegalArgumentException("taskDetails is required for threads of type Task");
}
TaskType taskType = task.getType();
if (!EntityUtil.isDescriptionTask(taskType) && !EntityUtil.isTagTask(taskType)) {
rejectField(task.getOldValue(), "oldValue", taskType);
rejectField(task.getSuggestion(), "suggestion", taskType);
}
if (EntityUtil.isTagTask(taskType)) {
validateTagLabelArray(task.getSuggestion(), "suggestion", taskType);
validateTagLabelArray(task.getOldValue(), "oldValue", taskType);
}
Comment on lines +1201 to +1211
Copy link
Copy Markdown

@gitar-bot gitar-bot Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

}
Comment thread
gitar-bot[bot] marked this conversation as resolved.

private void rejectField(Object value, String fieldName, TaskType taskType) {
if (value != null) {
throw new IllegalArgumentException(
String.format("taskDetails.%s is not applicable for task type %s", fieldName, taskType));
}
}

private void validateTagLabelArray(String value, String fieldName, TaskType taskType) {
if (value == null) {
return;
}
try {
JsonUtils.readObjects(value, TagLabel.class);
} catch (Exception e) {
throw new IllegalArgumentException(
String.format(
"taskDetails.%s must be a valid JSON array of tags for task type %s",
fieldName, taskType));
}
}

private void validateAssignee(Thread thread) {
if (thread != null && ThreadType.Task.equals(thread.getType())) {
String createdByUserName = thread.getCreatedBy();
Expand Down
45 changes: 30 additions & 15 deletions openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/shared/TagsTask.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,31 @@ const TagsTask: FC<TagsTaskProps> = ({

const isTaskClosed = task?.status === ThreadTaskStatus.Closed;

const parseTagLabels = (value?: string): TagLabel[] => {
try {
const parsed = JSON.parse(value ?? '[]');

return Array.isArray(parsed) ? parsed : [];
} catch {
return [];
}
};

const parsedOldValue = useMemo<TagLabel[]>(
() => parseTagLabels(oldValue),
[oldValue]
);

const parsedNewValue = useMemo<TagLabel[]>(
() => parseTagLabels(newValue),
[newValue]
);

const parsedSuggestion = useMemo<TagLabel[]>(
() => parseTagLabels(suggestion),
[suggestion]
);

const diffView = useMemo(() => {
if (!oldValue && !newValue) {
return (
Expand All @@ -61,15 +86,10 @@ const TagsTask: FC<TagsTaskProps> = ({
);
} else {
return (
<TagsDiffView
diffArr={diffArrays(
JSON.parse(oldValue ?? '[]'),
JSON.parse(newValue ?? '[]')
)}
/>
<TagsDiffView diffArr={diffArrays(parsedOldValue, parsedNewValue)} />
);
}
}, [oldValue, newValue]);
}, [oldValue, newValue, parsedOldValue, parsedNewValue]);
Copy link
Copy Markdown

@gitar-bot gitar-bot Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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


/**
*
Expand All @@ -86,15 +106,10 @@ const TagsTask: FC<TagsTaskProps> = ({
);
} else {
return (
<TagsDiffView
diffArr={diffArrays(
JSON.parse(oldValue ?? '[]'),
JSON.parse(suggestion ?? '[]')
)}
/>
<TagsDiffView diffArr={diffArrays(parsedOldValue, parsedSuggestion)} />
);
}
}, [suggestion, oldValue]);
}, [suggestion, oldValue, parsedOldValue, parsedSuggestion]);

return (
<div data-testid="task-tags-tabs">
Expand All @@ -116,7 +131,7 @@ const TagsTask: FC<TagsTaskProps> = ({
<div data-testid="update-tags">
{isTaskActionEdit && hasEditAccess ? (
<TagsTabs
tags={JSON.parse(oldValue ?? '[]')}
tags={parsedOldValue}
value={value ?? []}
onChange={onChange}
/>
Expand Down
Loading