fix: Changes needed to get the new TCK tests for tasks/list to pass #460
Conversation
- Sort tasks by status.timestamp DESC (most recent first), then ID ASC - Add lastUpdatedAfter parameter filtering - Update InMemoryTaskStore and JpaDatabaseTaskStore sorting logic - Add timestamp denormalized column to JpaTask for efficient querying - Update all transport handlers (JSON-RPC, gRPC, REST)
Summary of ChangesHello @kabir, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses necessary changes to enable the successful execution of new TCK tests for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to the task listing functionality to align with new TCK tests. The key changes include implementing keyset pagination based on a composite sort key (timestamp and ID), adding a lastUpdatedAfter filter, and ensuring consistent behavior across both JPA and in-memory task stores. The protobuf specification has also been updated to include pageSize in the ListTasksResponse.
The changes are well-tested, with new unit tests covering the new pagination and filtering logic. My review focuses on improving code maintainability by reducing duplication, fixing a potential null pointer exception in the in-memory implementation, and removing temporary debug logging.
| java.time.Instant taskTimestamp = task.getStatus().timestamp().toInstant() | ||
| .truncatedTo(java.time.temporal.ChronoUnit.MILLIS); | ||
| int timestampCompare = taskTimestamp.compareTo(tokenTimestamp); | ||
|
|
||
| if (timestampCompare < 0 || (timestampCompare == 0 && task.getId().compareTo(tokenId) > 0)) { | ||
| // This task is after the token, search left half | ||
| right = mid; | ||
| } else { | ||
| // This task is before or equal to token, search right half | ||
| left = mid + 1; | ||
| } |
There was a problem hiding this comment.
There is a potential NullPointerException here. If a Task has a null status, task.getStatus() will be null, and calling .timestamp() on it will cause an NPE. The filtering logic allows tasks with null status to be included in allFilteredTasks if params.status() is not specified. The sorting logic correctly handles this by placing them at the end, but this binary search does not. You should add a null check for task.getStatus() before accessing its properties.
java.time.Instant taskTimestamp = (task.getStatus() != null && task.getStatus().timestamp() != null)
? task.getStatus().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS)
: null;
if (taskTimestamp == null) {
// Task with null timestamp is always "before" a token with a timestamp, as they are sorted last.
// So, we search in the right half.
left = mid + 1;
} else {
int timestampCompare = taskTimestamp.compareTo(tokenTimestamp);
if (timestampCompare < 0 || (timestampCompare == 0 && task.getId().compareTo(tokenId) > 0)) {
// This task is after the token, search left half
right = mid;
} else {
// This task is before or equal to token, search right half
left = mid + 1;
}
}| if (params.pageToken() != null && !params.pageToken().isEmpty()) { | ||
| queryBuilder.append(" AND t.id > :pageToken"); | ||
| String[] tokenParts = params.pageToken().split(":", 2); | ||
| if (tokenParts.length == 2) { | ||
| // Keyset pagination: get tasks where timestamp < tokenTimestamp OR (timestamp = tokenTimestamp AND id > tokenId) | ||
| // All tasks have timestamps (TaskStatus canonical constructor ensures this) | ||
| queryBuilder.append(" AND (t.statusTimestamp < :tokenTimestamp OR (t.statusTimestamp = :tokenTimestamp AND t.id > :tokenId))"); | ||
| } else { | ||
| // Legacy ID-only pageToken format is not supported with timestamp-based sorting | ||
| // Throw error to prevent incorrect pagination results | ||
| throw new io.a2a.spec.InvalidParamsError(null, "Invalid pageToken format: expected 'timestamp:id'", null); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for parsing and validating the pageToken is duplicated here and in the block for setting query parameters (lines 267-286). This increases the maintenance burden and risk of inconsistencies. Consider refactoring to parse the token once at the beginning of the method, store the timestamp and ID in local variables, and then reuse them for building the query and setting parameters. This would centralize the validation and parsing logic.
| // DEBUG: Log store contents for totalSize investigation | ||
| LOGGER.debug("=== InMemoryTaskStore.list() DEBUG ==="); | ||
| LOGGER.debug("Total tasks in store: {}", tasks.size()); | ||
| LOGGER.debug("Filter contextId: {}", params.contextId()); | ||
| LOGGER.debug("Filter status: {}", params.status()); | ||
| tasks.values().forEach(t -> LOGGER.debug(" Task: id={}, contextId={}", t.getId(), t.getContextId())); |
There was a problem hiding this comment.
These detailed debug logs seem to have been added for troubleshooting during development. While useful, they are quite verbose and could clutter the logs. It's good practice to remove such temporary debugging statements before merging the pull request.
| (Task t) -> (t.getStatus() != null && t.getStatus().timestamp() != null) | ||
| // Truncate to milliseconds for consistency with pageToken precision | ||
| ? t.getStatus().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS) | ||
| : null, |
There was a problem hiding this comment.
The null check for t.getStatus().timestamp() is redundant. The canonical constructor of TaskStatus ensures that the timestamp is never null, defaulting to OffsetDateTime.now(ZoneOffset.UTC) if one isn't provided. You can simplify this to just check t.getStatus() != null.
| (Task t) -> (t.getStatus() != null && t.getStatus().timestamp() != null) | |
| // Truncate to milliseconds for consistency with pageToken precision | |
| ? t.getStatus().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS) | |
| : null, | |
| (Task t) -> (t.getStatus() != null) | |
| // Truncate to milliseconds for consistency with pageToken precision | |
| ? t.getStatus().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS) | |
| : null, |
| LOGGER.debug("=== ProtoUtils.ToProto.listTasksResult() DEBUG ==="); | ||
| LOGGER.debug("Input result.totalSize(): {}", result.totalSize()); | ||
| LOGGER.debug("Input result.pageSize(): {}", result.pageSize()); | ||
| LOGGER.debug("Input result.tasks().size(): {}", result.tasks().size()); |
There was a problem hiding this comment.
These detailed debug logs seem to have been added for troubleshooting. It's good practice to remove temporary debugging statements like these before merging to keep the codebase clean and avoid log clutter, even at the DEBUG level.
|
This one now has some conflicts. |
2 participants
I started working on the TCK tests a2aproject/a2a-tck#93 but then found changes were needed, and a lot of them are affected by the not-yet-merged a2aproject/A2A#1160.