feat: update tasks/list implementation to match A2A 0.4.0 spec #459
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 updates the task listing functionality to conform with the A2A 0.4.0 specification. It introduces a new sorting mechanism that prioritizes the most recently updated tasks and adds a filtering capability to retrieve tasks updated after a specific timestamp. These changes are implemented across both in-memory and JPA task stores, with corresponding updates to the API handlers to expose the new functionality. 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 updates the task listing functionality to align with the A2A 0.4.0 specification, introducing sorting by status timestamp and filtering by lastUpdatedAfter. The changes are implemented across the JPA and in-memory task stores, as well as the REST and other transport handlers. My review identified a critical issue in the pagination logic within JpaDatabaseTaskStore. The current implementation for pageToken is incorrect for the new composite sort order and will lead to broken pagination. I've also pointed out that the corresponding unit test is insufficient as it uses data that masks this bug. The in-memory implementation and other changes appear correct.
...re-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java
Show resolved
Hide resolved
...atabase-jpa/src/test/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStoreTest.java
Outdated
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully updates the task listing logic to align with the A2A 0.4.0 specification, introducing sorting by timestamp and a lastUpdatedAfter filter. The implementation of keyset pagination is a solid improvement. However, I've identified a few important issues that should be addressed. There is a critical NullPointerException risk in the InMemoryTaskStore's new sorting logic. Additionally, the fallback mechanism for legacy page tokens in JpaDatabaseTaskStore is logically flawed and could lead to incorrect pagination results. I've also noted a performance regression in the in-memory store's pagination and some duplicated code that could be refactored for better maintainability. Addressing these points will significantly improve the robustness and quality of the implementation.
server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java
Outdated
Show resolved
Hide resolved
...re-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request successfully updates the task listing and pagination logic to align with the A2A 0.4.0 specification, introducing sorting by timestamp and keyset pagination. The changes are comprehensive, touching the JPA and in-memory task stores, as well as the REST and other request handlers. The denormalization of statusTimestamp in JpaTask is a good approach for efficient querying, and the new tests are thorough.
I've identified a few issues, including a critical bug in the error handling for malformed page tokens in the JPA store, a high-severity issue with the backward-compatibility logic for legacy page tokens that will lead to incorrect pagination, and a performance regression in the in-memory store's pagination logic. My review includes suggestions to address these points.
...re-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java
Show resolved
Hide resolved
...re-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively updates the task listing implementation to align with the A2A 0.4.0 specification, introducing timestamp-based sorting and filtering. The changes are well-executed, including the denormalization of the timestamp for efficient querying and the addition of comprehensive tests for the new keyset pagination logic. My review includes a couple of suggestions to enhance code maintainability and improve test readability.
| 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 to parse and validate the pageToken is duplicated. It's first handled here to build the query string, and then again in lines 267-286 to set the query parameters. This redundancy can lead to maintainability issues and potential inconsistencies if one part is modified but the other is not.
Consider refactoring to parse the token once at the beginning of the method, store the parts, and reuse them. Here's an example of how you could structure it:
// At the start of the list() method:
String[] tokenParts = null;
if (params.pageToken() != null && !params.pageToken().isEmpty()) {
tokenParts = params.pageToken().split(":", 2);
if (tokenParts.length != 2) {
throw new io.a2a.spec.InvalidParamsError(null, "Invalid pageToken format: expected 'timestamp:id'", null);
}
}
// ... later in query building ...
if (tokenParts != null) {
queryBuilder.append(" AND (t.statusTimestamp < :tokenTimestamp OR (t.statusTimestamp = :tokenTimestamp AND t.id > :tokenId))");
}
// ... later in parameter setting ...
if (tokenParts != null) {
try {
long timestampMillis = Long.parseLong(tokenParts[0]);
String tokenId = tokenParts[1];
Instant tokenTimestamp = Instant.ofEpochMilli(timestampMillis);
query.setParameter("tokenTimestamp", tokenTimestamp);
query.setParameter("tokenId", tokenId);
} catch (NumberFormatException e) {
throw new io.a2a.spec.InvalidParamsError(null, "Invalid pageToken format: timestamp must be numeric milliseconds", null);
}
}| public void testListTasksInvalidPageTokenFormat() { | ||
| // Create a task | ||
| Task task = new Task.Builder() | ||
| .id("task-invalid-token") | ||
| .contextId("context-invalid-token") | ||
| .status(new TaskStatus(TaskState.WORKING)) | ||
| .build(); | ||
| taskStore.save(task); | ||
|
|
||
| // Test 1: Legacy ID-only pageToken should throw InvalidParamsError | ||
| ListTasksParams params1 = new ListTasksParams.Builder() | ||
| .contextId("context-invalid-token") | ||
| .pageToken("task-invalid-token") // ID-only format (legacy) | ||
| .build(); | ||
|
|
||
| try { | ||
| taskStore.list(params1); | ||
| throw new AssertionError("Expected InvalidParamsError for legacy ID-only pageToken"); | ||
| } catch (io.a2a.spec.InvalidParamsError e) { | ||
| // Expected - legacy format not supported | ||
| assertTrue(e.getMessage().contains("Invalid pageToken format"), | ||
| "Error message should mention invalid format"); | ||
| } | ||
|
|
||
| // Test 2: Malformed timestamp in pageToken should throw InvalidParamsError | ||
| ListTasksParams params2 = new ListTasksParams.Builder() | ||
| .contextId("context-invalid-token") | ||
| .pageToken("not-a-number:task-id") // Invalid timestamp | ||
| .build(); | ||
|
|
||
| try { | ||
| taskStore.list(params2); | ||
| throw new AssertionError("Expected InvalidParamsError for malformed timestamp"); | ||
| } catch (io.a2a.spec.InvalidParamsError e) { | ||
| // Expected - malformed timestamp | ||
| assertTrue(e.getMessage().contains("timestamp must be numeric"), | ||
| "Error message should mention numeric timestamp requirement"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test uses try-catch blocks with AssertionError to verify that exceptions are thrown. While this works, JUnit 5 provides the assertThrows method, which is more idiomatic and makes the test more readable and concise. Refactoring to use assertThrows would improve the test's clarity and align it with modern testing practices.
public void testListTasksInvalidPageTokenFormat() {
// Create a task
Task task = new Task.Builder()
.id("task-invalid-token")
.contextId("context-invalid-token")
.status(new TaskStatus(TaskState.WORKING))
.build();
taskStore.save(task);
// Test 1: Legacy ID-only pageToken should throw InvalidParamsError
ListTasksParams params1 = new ListTasksParams.Builder()
.contextId("context-invalid-token")
.pageToken("task-invalid-token") // ID-only format (legacy)
.build();
var e1 = assertThrows(io.a2a.spec.InvalidParamsError.class, () -> taskStore.list(params1));
assertTrue(e1.getMessage().contains("Invalid pageToken format"),
"Error message should mention invalid format");
// Test 2: Malformed timestamp in pageToken should throw InvalidParamsError
ListTasksParams params2 = new ListTasksParams.Builder()
.contextId("context-invalid-token")
.pageToken("not-a-number:task-id") // Invalid timestamp
.build();
var e2 = assertThrows(io.a2a.spec.InvalidParamsError.class, () -> taskStore.list(params2));
assertTrue(e2.getMessage().contains("timestamp must be numeric"),
"Error message should mention numeric timestamp requirement");
}
…oject#459) - 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) Follows up on a2aproject#455 and a2aproject#359
4 participants
Follows up on #455 and #359