-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
0f4211b
489923c
e0969e0
04f3348
9d05181
f2aa647
f91f405
e6a15c1
9f06b75
40a3f36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
|
||
| store(threadContext); | ||
| storeRelationships(threadContext); | ||
|
|
@@ -1189,6 +1193,45 @@ 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"); | ||
| } | ||
| 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 -> {} | ||
| } | ||
|
Comment on lines
+1201
to
+1211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π‘ Edge Case: Switch on TaskType lacks default branch for future enum valuesThe switch at line 1201 explicitly lists all 8 current Suggested fix: Was this helpful? React with π / π | Reply |
||
| } | ||
|
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(); | ||
|
|
||
There was a problem hiding this comment.
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
Conversationthreads with non-nulltaskDetailsare rejected, but it doesnβt coverAnnouncementthreads carryingtaskDetails. SincecreateThreadhas a dedicatedAnnouncementbranch, adding a similar 400 test forThreadType.Announcementwould help prevent regressions where announcements accidentally accept and persist task payloads.