feat: add list tasks functionality #455
Conversation
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 delivers a significant enhancement by introducing a robust task listing mechanism. It allows users to efficiently retrieve tasks based on various criteria like context, status, and pagination, providing greater control over the data returned, such as limiting task history and including or excluding artifacts. This feature improves the client's ability to manage and query tasks, making the system more flexible and performant for task-related operations. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces the functionality to list tasks, including filtering and pagination capabilities. The changes are comprehensive, touching the client, server, different transport layers (gRPC, JSON-RPC, REST), and data stores (in-memory, JPA). The implementation is solid and well-tested. I've provided a few suggestions to improve code clarity, maintainability, and correctness, such as refactoring long methods, simplifying redundant code, and adding validation. There's also a high-severity suggestion to fix a potential data inconsistency bug in the denormalized fields of the JpaTask entity.
| private void updateDenormalizedFields(Task task) { | ||
| this.contextId = task.getContextId(); | ||
| if (task.getStatus() != null && task.getStatus().state() != null) { | ||
| this.state = task.getStatus().state().asString(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of updateDenormalizedFields doesn't handle the case where a task's state becomes null. If task.getStatus().state() is null, the this.state field will retain its old value, leading to a desynchronization with the JSON data. To ensure correctness, the state field should be explicitly set to null when the task's state is null.
private void updateDenormalizedFields(Task task) {
this.contextId = task.getContextId();
if (task.getStatus() != null) {
io.a2a.spec.TaskState taskState = task.getStatus().state();
this.state = (taskState != null) ? taskState.asString() : null;
} else {
this.state = null;
}
}| } catch (A2AClientException e) { | ||
| throw e; | ||
| } catch (IOException | InterruptedException e) { | ||
| throw new A2AClientException("Failed to list tasks: " + e, e); | ||
| } |
There was a problem hiding this comment.
The catch (A2AClientException e) block is redundant as it just re-throws the exception, which is already declared in the method's throws clause. You can remove this block to simplify the code.
} catch (IOException | InterruptedException e) {
throw new A2AClientException("Failed to list tasks: " + e, e);
}| } catch (A2AClientException e) { | ||
| throw e; | ||
| } catch (IOException | InterruptedException e) { | ||
| throw new A2AClientException("Failed to list tasks: " + e, e); | ||
| } |
There was a problem hiding this comment.
The catch (A2AClientException e) block is redundant as it just re-throws the exception, which is already declared in the method's throws clause. You can remove this block to simplify the code.
} catch (IOException | InterruptedException e) {
throw new A2AClientException("Failed to list tasks: " + e, e);
}| private String buildListTasksQueryString(ListTasksParams request) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| if (request.contextId() != null) { | ||
| sb.append("contextId=").append(URLEncoder.encode(request.contextId(), StandardCharsets.UTF_8)); | ||
| } | ||
| if (request.status() != null) { | ||
| if (sb.length() > 0) sb.append("&"); | ||
| sb.append("status=").append(request.status().asString()); | ||
| } | ||
| if (request.pageSize() != null) { | ||
| if (sb.length() > 0) sb.append("&"); | ||
| sb.append("pageSize=").append(request.pageSize()); | ||
| } | ||
| if (request.pageToken() != null) { | ||
| if (sb.length() > 0) sb.append("&"); | ||
| sb.append("pageToken=").append(URLEncoder.encode(request.pageToken(), StandardCharsets.UTF_8)); | ||
| } | ||
| if (request.historyLength() != null) { | ||
| if (sb.length() > 0) sb.append("&"); | ||
| sb.append("historyLength=").append(request.historyLength()); | ||
| } | ||
| if (request.includeArtifacts() != null && request.includeArtifacts()) { | ||
| if (sb.length() > 0) sb.append("&"); | ||
| sb.append("includeArtifacts=true"); | ||
| } | ||
| return sb.toString(); | ||
| } |
There was a problem hiding this comment.
The buildListTasksQueryString method can be simplified by using a List to collect query parameter parts and then joining them with &. This avoids repetitive checks for sb.length() > 0 and makes the code more readable.
private String buildListTasksQueryString(ListTasksParams request) {
java.util.List<String> queryParts = new java.util.ArrayList<>();
if (request.contextId() != null) {
queryParts.add("contextId=" + URLEncoder.encode(request.contextId(), StandardCharsets.UTF_8));
}
if (request.status() != null) {
queryParts.add("status=" + request.status().asString());
}
if (request.pageSize() != null) {
queryParts.add("pageSize=" + request.pageSize());
}
if (request.pageToken() != null) {
queryParts.add("pageToken=" + URLEncoder.encode(request.pageToken(), StandardCharsets.UTF_8));
}
if (request.historyLength() != null) {
queryParts.add("historyLength=" + request.historyLength());
}
if (request.includeArtifacts() != null && request.includeArtifacts()) {
queryParts.add("includeArtifacts=true");
}
return String.join("&", queryParts);
}| public ListTasksResult list(ListTasksParams params) { | ||
| LOGGER.debug("Listing tasks with params: contextId={}, status={}, pageSize={}, pageToken={}", | ||
| params.contextId(), params.status(), params.pageSize(), params.pageToken()); | ||
|
|
||
| // Build dynamic JPQL query with WHERE clauses for filtering | ||
| StringBuilder queryBuilder = new StringBuilder("SELECT t FROM JpaTask t WHERE 1=1"); | ||
| StringBuilder countQueryBuilder = new StringBuilder("SELECT COUNT(t) FROM JpaTask t WHERE 1=1"); | ||
|
|
||
| // Apply contextId filter using denormalized column | ||
| if (params.contextId() != null) { | ||
| queryBuilder.append(" AND t.contextId = :contextId"); | ||
| countQueryBuilder.append(" AND t.contextId = :contextId"); | ||
| } | ||
|
|
||
| // Apply status filter using denormalized column | ||
| if (params.status() != null) { | ||
| queryBuilder.append(" AND t.state = :state"); | ||
| countQueryBuilder.append(" AND t.state = :state"); | ||
| } | ||
|
|
||
| // Apply pagination cursor (tasks after pageToken) | ||
| if (params.pageToken() != null && !params.pageToken().isEmpty()) { | ||
| queryBuilder.append(" AND t.id > :pageToken"); | ||
| } | ||
|
|
||
| // Sort by task ID for consistent pagination | ||
| queryBuilder.append(" ORDER BY t.id"); | ||
|
|
||
| // Create and configure the main query | ||
| TypedQuery<JpaTask> query = em.createQuery(queryBuilder.toString(), JpaTask.class); | ||
|
|
||
| // Set filter parameters | ||
| if (params.contextId() != null) { | ||
| query.setParameter("contextId", params.contextId()); | ||
| } | ||
| if (params.status() != null) { | ||
| query.setParameter("state", params.status().asString()); | ||
| } | ||
| if (params.pageToken() != null && !params.pageToken().isEmpty()) { | ||
| query.setParameter("pageToken", params.pageToken()); | ||
| } | ||
|
|
||
| // Apply page size limit (+1 to check for next page) | ||
| int pageSize = params.getEffectivePageSize(); | ||
| query.setMaxResults(pageSize + 1); | ||
|
|
||
| // Execute query and deserialize tasks | ||
| List<JpaTask> jpaTasksPage = query.getResultList(); | ||
|
|
||
| // Determine if there are more results | ||
| boolean hasMore = jpaTasksPage.size() > pageSize; | ||
| if (hasMore) { | ||
| jpaTasksPage = jpaTasksPage.subList(0, pageSize); | ||
| } | ||
|
|
||
| // Get total count of matching tasks | ||
| TypedQuery<Long> countQuery = em.createQuery(countQueryBuilder.toString(), Long.class); | ||
| if (params.contextId() != null) { | ||
| countQuery.setParameter("contextId", params.contextId()); | ||
| } | ||
| if (params.status() != null) { | ||
| countQuery.setParameter("state", params.status().asString()); | ||
| } | ||
| int totalSize = countQuery.getSingleResult().intValue(); | ||
|
|
||
| // Deserialize tasks from JSON | ||
| List<Task> tasks = new ArrayList<>(); | ||
| for (JpaTask jpaTask : jpaTasksPage) { | ||
| try { | ||
| tasks.add(jpaTask.getTask()); | ||
| } catch (JsonProcessingException e) { | ||
| LOGGER.error("Failed to deserialize task with ID: {}", jpaTask.getId(), e); | ||
| throw new RuntimeException("Failed to deserialize task with ID: " + jpaTask.getId(), e); | ||
| } | ||
| } | ||
|
|
||
| // Determine next page token (ID of last task if there are more results) | ||
| String nextPageToken = null; | ||
| if (hasMore && !tasks.isEmpty()) { | ||
| nextPageToken = tasks.get(tasks.size() - 1).getId(); | ||
| } | ||
|
|
||
| // Apply post-processing transformations (history limiting, artifact removal) | ||
| int historyLength = params.getEffectiveHistoryLength(); | ||
| boolean includeArtifacts = params.shouldIncludeArtifacts(); | ||
|
|
||
| List<Task> transformedTasks = tasks.stream() | ||
| .map(task -> transformTask(task, historyLength, includeArtifacts)) | ||
| .toList(); | ||
|
|
||
| LOGGER.debug("Returning {} tasks out of {} total", transformedTasks.size(), totalSize); | ||
| return new ListTasksResult(transformedTasks, totalSize, transformedTasks.size(), nextPageToken); | ||
| } |
There was a problem hiding this comment.
This list method is quite long and handles multiple responsibilities (query building, parameter setting, execution, pagination, deserialization, transformation). Consider refactoring it into smaller, more focused private methods to improve readability and maintainability. For example:
- A method to build the main query and another for the count query.
- A method to set query parameters.
- A method to execute the query and handle pagination logic.
- A method to deserialize
JpaTaskobjects toTaskobjects.
| if (params.pageToken() != null && !params.pageToken().isEmpty()) { | ||
| for (int i = 0; i < allFilteredTasks.size(); i++) { | ||
| if (allFilteredTasks.get(i).getId().equals(params.pageToken())) { | ||
| startIndex = i + 1; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The pagination logic for handling pageToken involves a linear scan through allFilteredTasks to find the starting index. This has a time complexity of O(N), where N is the number of filtered tasks. For an in-memory store, this might be acceptable for small datasets, but it can become a performance bottleneck as the number of tasks grows. Consider using a more efficient approach for pagination, such as filtering the stream directly based on the pageToken to avoid iterating over tasks that are before the desired page.
| public ListTasksResult { | ||
| Assert.checkNotNullParam("tasks", tasks); | ||
| if (totalSize < 0) { | ||
| throw new IllegalArgumentException("totalSize must be non-negative"); | ||
| } | ||
| if (pageSize < 0) { | ||
| throw new IllegalArgumentException("pageSize must be non-negative"); | ||
| } | ||
| // Make defensive copy | ||
| tasks = List.copyOf(tasks); | ||
| } |
There was a problem hiding this comment.
The pageSize field should always be consistent with the number of tasks in the tasks list. To enforce this and prevent potential inconsistencies, it's a good practice to add a validation check in the compact constructor to ensure pageSize == tasks.size().
public ListTasksResult {
Assert.checkNotNullParam("tasks", tasks);
if (totalSize < 0) {
throw new IllegalArgumentException("totalSize must be non-negative");
}
if (pageSize < 0) {
throw new IllegalArgumentException("pageSize must be non-negative");
}
if (pageSize != tasks.size()) {
throw new IllegalArgumentException("pageSize must be equal to the number of tasks in the list. Got " + pageSize + ", but list has " + tasks.size() + " tasks.");
}
// Make defensive copy
tasks = List.copyOf(tasks);
}|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the functionality to list tasks with filtering and pagination. The changes span across the client, server, and spec modules, touching all transport layers (gRPC, JSON-RPC, REST). The implementation includes new API endpoints, data stores logic for both JPA and in-memory, and extensive tests. The code is well-structured. I've found one critical issue related to a missing field in a response builder that would cause client-side failures, and a performance issue in the in-memory task store implementation.
- 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 #455 and #359
…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
Fixes #359