From 1b1ada31d7d2c5e2b2f8fd10f08bd355febc134e Mon Sep 17 00:00:00 2001 From: Kabir Khan Date: Fri, 16 Jan 2026 14:17:13 +0000 Subject: [PATCH 1/3] fix: ListTasks parameter validation and serialization - Fix pageSize=0 validation using hasPageSize() instead of zeroToNull - Add includingDefaultValueFields() for complete JSON responses - Validate enum UNRECOGNIZED, negative timestamps across transports - Support multiple timestamp/status formats in REST - Ensure consistent InvalidParamsError behavior across JSON-RPC, gRPC, REST Co-Authored-By: Claude Sonnet 4.5 --- .../database/jpa/JpaDatabaseTaskStore.java | 5 +- .../a2a/server/tasks/InMemoryTaskStore.java | 5 +- .../a2a/grpc/mapper/A2ACommonFieldMapper.java | 42 ++++++++-- .../grpc/mapper/ListTasksParamsMapper.java | 3 +- .../java/io/a2a/grpc/utils/JSONRPCUtils.java | 8 +- .../java/io/a2a/spec/ListTasksParams.java | 15 +++- .../grpc/handler/GrpcHandlerTest.java | 49 ++++++++++++ .../jsonrpc/handler/JSONRPCHandlerTest.java | 28 +++++++ .../transport/rest/handler/RestHandler.java | 70 ++++++++++++----- .../rest/handler/RestHandlerTest.java | 78 +++++++++++++++++++ 10 files changed, 273 insertions(+), 30 deletions(-) diff --git a/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java b/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java index 49dad1728..a4a1f4f66 100644 --- a/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java +++ b/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java @@ -362,7 +362,10 @@ public ListTasksResult list(ListTasksParams params) { private Task transformTask(Task task, int historyLength, boolean includeArtifacts) { // Limit history if needed (keep most recent N messages) List history = task.history(); - if (historyLength > 0 && history != null && history.size() > historyLength) { + if (historyLength == 0) { + // When historyLength is 0, return empty history + history = List.of(); + } else if (historyLength > 0 && history != null && history.size() > historyLength) { history = history.subList(history.size() - historyLength, history.size()); } diff --git a/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java b/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java index d2d9362ca..d9fb956df 100644 --- a/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java +++ b/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java @@ -144,7 +144,10 @@ public ListTasksResult list(ListTasksParams params) { private Task transformTask(Task task, int historyLength, boolean includeArtifacts) { // Limit history if needed (keep most recent N messages) List history = task.history(); - if (historyLength > 0 && history != null && history.size() > historyLength) { + if (historyLength == 0) { + // When historyLength is 0, return empty history + history = List.of(); + } else if (historyLength > 0 && history != null && history.size() > historyLength) { history = history.subList(history.size() - historyLength, history.size()); } diff --git a/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java b/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java index c3f031e97..9c0327c2d 100644 --- a/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java +++ b/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java @@ -285,7 +285,23 @@ default Map metadataFromProto(Struct struct) { */ @Named("zeroToNull") default Integer zeroToNull(int value) { - return value > 0 ? value : null; + return value != 0 ? value : null; + } + + /** + * Converts protobuf int to Integer, preserving all values including 0. + *

+ * Unlike zeroToNull, this method preserves 0 values, allowing compact constructor + * validation to catch invalid values (e.g., pageSize=0 must fail validation). + * For truly optional fields where 0 means "unset", use zeroToNull instead. + * Use this with {@code @Mapping(qualifiedByName = "intToIntegerOrNull")}. + * + * @param value the protobuf int value + * @return Integer (never null for primitive int input) + */ + @Named("intToIntegerOrNull") + default Integer intToIntegerOrNull(int value) { + return value; } /** @@ -337,13 +353,20 @@ default long instantToMillis(Instant instant) { * Converts protobuf milliseconds-since-epoch (int64) to domain Instant. *

* Returns null if input is 0 (protobuf default for unset field). + * Throws InvalidParamsError for negative values (invalid timestamps). * Use this with {@code @Mapping(qualifiedByName = "millisToInstant")}. * * @param millis milliseconds since epoch * @return domain Instant, or null if millis is 0 + * @throws InvalidParamsError if millis is negative */ @Named("millisToInstant") default Instant millisToInstant(long millis) { + if (millis < 0L) { + throw new InvalidParamsError(null, + "lastUpdatedAfter must be a valid timestamp (non-negative milliseconds since epoch), got: " + millis, + null); + } return millis > 0L ? Instant.ofEpochMilli(millis) : null; } @@ -351,21 +374,28 @@ default Instant millisToInstant(long millis) { // Enum Conversions (handling UNSPECIFIED/UNKNOWN) // ======================================================================== /** - * Converts protobuf TaskState to domain TaskState, treating UNSPECIFIED/UNKNOWN as null. + * Converts protobuf TaskState to domain TaskState, treating UNSPECIFIED as null. *

- * Protobuf enums default to UNSPECIFIED (0 value) when unset. The domain may also have - * UNKNOWN for unparseable values. Both should map to null for optional fields. + * Protobuf enums default to UNSPECIFIED (0 value) when unset, which maps to null for optional fields. + * However, UNRECOGNIZED (invalid enum values from JSON) throws InvalidParamsError for proper validation. * Use this with {@code @Mapping(qualifiedByName = "taskStateOrNull")}. * * @param state the protobuf TaskState - * @return domain TaskState or null if UNSPECIFIED/UNKNOWN + * @return domain TaskState or null if UNSPECIFIED + * @throws InvalidParamsError if state is UNRECOGNIZED (invalid enum value) */ @Named("taskStateOrNull") default io.a2a.spec.TaskState taskStateOrNull(io.a2a.grpc.TaskState state) { if (state == null || state == io.a2a.grpc.TaskState.TASK_STATE_UNSPECIFIED) { return null; } + // Reject invalid enum values (e.g., "INVALID_STATUS" from JSON) + if (state == io.a2a.grpc.TaskState.UNRECOGNIZED) { + throw new InvalidParamsError(null, + "Invalid task state value. Must be one of: SUBMITTED, WORKING, INPUT_REQUIRED, AUTH_REQUIRED, COMPLETED, CANCELED, FAILED, REJECTED", + null); + } io.a2a.spec.TaskState result = TaskStateMapper.INSTANCE.fromProto(state); - return result == io.a2a.spec.TaskState.UNKNOWN ? null : result; + return result; } } diff --git a/spec-grpc/src/main/java/io/a2a/grpc/mapper/ListTasksParamsMapper.java b/spec-grpc/src/main/java/io/a2a/grpc/mapper/ListTasksParamsMapper.java index 05d45d68a..3e666a8e5 100644 --- a/spec-grpc/src/main/java/io/a2a/grpc/mapper/ListTasksParamsMapper.java +++ b/spec-grpc/src/main/java/io/a2a/grpc/mapper/ListTasksParamsMapper.java @@ -38,7 +38,8 @@ public interface ListTasksParamsMapper { */ @Mapping(target = "contextId", source = "contextId", qualifiedByName = "emptyToNull") @Mapping(target = "status", source = "status", qualifiedByName = "taskStateOrNull") - @Mapping(target = "pageSize", source = "pageSize", qualifiedByName = "zeroToNull") + // pageSize: Check if field is set using hasPageSize() to distinguish unset (null) from explicit 0 (validation error) + @Mapping(target = "pageSize", expression = "java(request.hasPageSize() ? request.getPageSize() : null)") @Mapping(target = "pageToken", source = "pageToken", qualifiedByName = "emptyToNull") @Mapping(target = "historyLength", source = "historyLength", qualifiedByName = "zeroToNull") @Mapping(target = "lastUpdatedAfter", source = "lastUpdatedAfter", qualifiedByName = "millisToInstant") diff --git a/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java b/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java index ad46a8f38..590c8b53d 100644 --- a/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java +++ b/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java @@ -467,6 +467,10 @@ private static JsonProcessingException convertProtoBufExceptionToJsonProcessingE if (message.contains(prefix)) { return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id); } + prefix = "Invalid enum value:"; + if (message.contains(prefix)) { + return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id); + } // Try to extract specific error details using regex patterns Matcher matcher = EXTRACT_WRONG_TYPE.matcher(message); @@ -548,7 +552,7 @@ public static String toJsonRPCRequest(@Nullable String requestId, String method, output.name("method").value(method); } if (payload != null) { - String resultValue = JsonFormat.printer().omittingInsignificantWhitespace().print(payload); + String resultValue = JsonFormat.printer().includingDefaultValueFields().omittingInsignificantWhitespace().print(payload); output.name("params").jsonValue(resultValue); } output.endObject(); @@ -571,7 +575,7 @@ public static String toJsonRPCResultResponse(Object requestId, com.google.protob output.name("id").value(number.longValue()); } } - String resultValue = JsonFormat.printer().omittingInsignificantWhitespace().print(builder); + String resultValue = JsonFormat.printer().includingDefaultValueFields().omittingInsignificantWhitespace().print(builder); output.name("result").jsonValue(resultValue); output.endObject(); return result.toString(); diff --git a/spec/src/main/java/io/a2a/spec/ListTasksParams.java b/spec/src/main/java/io/a2a/spec/ListTasksParams.java index 6813dcf45..81fc0eb08 100644 --- a/spec/src/main/java/io/a2a/spec/ListTasksParams.java +++ b/spec/src/main/java/io/a2a/spec/ListTasksParams.java @@ -29,7 +29,7 @@ public record ListTasksParams( ) { /** * Compact constructor for validation. - * Validates that the tenant parameter is not null. + * Validates that the tenant parameter is not null and parameters are within valid ranges. * * @param contextId filter by context ID * @param status filter by task status @@ -39,9 +39,22 @@ public record ListTasksParams( * @param lastUpdatedAfter filter by last update timestamp * @param includeArtifacts whether to include artifacts * @param tenant the tenant identifier + * @throws InvalidParamsError if pageSize or historyLength are out of valid range */ public ListTasksParams { Assert.checkNotNullParam("tenant", tenant); + + // Validate pageSize (1-100) + if (pageSize != null && (pageSize < 1 || pageSize > 100)) { + throw new InvalidParamsError(null, + "pageSize must be between 1 and 100, got: " + pageSize, null); + } + + // Validate historyLength (>= 0) + if (historyLength != null && historyLength < 0) { + throw new InvalidParamsError(null, + "historyLength must be non-negative, got: " + historyLength, null); + } } /** * Default constructor for listing all tasks. diff --git a/transport/grpc/src/test/java/io/a2a/transport/grpc/handler/GrpcHandlerTest.java b/transport/grpc/src/test/java/io/a2a/transport/grpc/handler/GrpcHandlerTest.java index 77a2c955f..a3ce7ca2a 100644 --- a/transport/grpc/src/test/java/io/a2a/transport/grpc/handler/GrpcHandlerTest.java +++ b/transport/grpc/src/test/java/io/a2a/transport/grpc/handler/GrpcHandlerTest.java @@ -21,6 +21,8 @@ import io.a2a.grpc.GetTaskRequest; import io.a2a.grpc.ListTaskPushNotificationConfigRequest; import io.a2a.grpc.ListTaskPushNotificationConfigResponse; +import io.a2a.grpc.ListTasksRequest; +import io.a2a.grpc.ListTasksResponse; import io.a2a.grpc.Message; import io.a2a.grpc.Part; import io.a2a.grpc.PushNotificationConfig; @@ -1209,6 +1211,53 @@ private void assertGrpcError(StreamRecorder streamRecorder, Status.Code e Assertions.assertTrue(streamRecorder.getValues().isEmpty()); } + @Test + public void testListTasksNegativeTimestampReturnsInvalidArgument() { + TestGrpcHandler handler = new TestGrpcHandler(CARD, requestHandler, internalExecutor); + StreamRecorder responseObserver = StreamRecorder.create(); + + // Negative timestamp should trigger validation error + ListTasksRequest request = ListTasksRequest.newBuilder() + .setLastUpdatedAfter(-1L) + .setTenant("") + .build(); + + handler.listTasks(request, responseObserver); + + // Should return error with INVALID_ARGUMENT status + Assertions.assertTrue(responseObserver.getError() != null); + StatusRuntimeException error = (StatusRuntimeException) responseObserver.getError(); + assertEquals(Status.INVALID_ARGUMENT.getCode(), error.getStatus().getCode()); + } + + @Test + public void testListTasksEmptyResultIncludesAllFields() throws Exception { + TestGrpcHandler handler = new TestGrpcHandler(CARD, requestHandler, internalExecutor); + StreamRecorder responseObserver = StreamRecorder.create(); + + // Query for a context that doesn't exist - should return empty result + ListTasksRequest request = ListTasksRequest.newBuilder() + .setContextId("nonexistent-context-id") + .setTenant("") + .build(); + + handler.listTasks(request, responseObserver); + + // Should succeed with empty result + Assertions.assertNull(responseObserver.getError()); + List responses = responseObserver.getValues(); + assertEquals(1, responses.size()); + + ListTasksResponse response = responses.get(0); + // Verify all fields are present (even if empty/default) + Assertions.assertNotNull(response.getTasksList(), "tasks field should not be null"); + assertEquals(0, response.getTasksList().size(), "tasks should be empty list"); + assertEquals(0, response.getTotalSize(), "totalSize should be 0"); + assertEquals(0, response.getPageSize(), "pageSize should be 0"); + // nextPageToken will be empty string for empty results (protobuf default) + Assertions.assertNotNull(response.getNextPageToken()); + } + private static class TestGrpcHandler extends GrpcHandler { private final AgentCard card; private final RequestHandler handler; diff --git a/transport/jsonrpc/src/test/java/io/a2a/transport/jsonrpc/handler/JSONRPCHandlerTest.java b/transport/jsonrpc/src/test/java/io/a2a/transport/jsonrpc/handler/JSONRPCHandlerTest.java index a55233adb..3436a34b1 100644 --- a/transport/jsonrpc/src/test/java/io/a2a/transport/jsonrpc/handler/JSONRPCHandlerTest.java +++ b/transport/jsonrpc/src/test/java/io/a2a/transport/jsonrpc/handler/JSONRPCHandlerTest.java @@ -30,12 +30,15 @@ import io.a2a.jsonrpc.common.wrappers.GetTaskResponse; import io.a2a.jsonrpc.common.wrappers.ListTaskPushNotificationConfigRequest; import io.a2a.jsonrpc.common.wrappers.ListTaskPushNotificationConfigResponse; +import io.a2a.jsonrpc.common.wrappers.ListTasksResult; import io.a2a.jsonrpc.common.wrappers.SendMessageRequest; import io.a2a.jsonrpc.common.wrappers.SendMessageResponse; import io.a2a.jsonrpc.common.wrappers.SendStreamingMessageRequest; import io.a2a.jsonrpc.common.wrappers.SendStreamingMessageResponse; import io.a2a.jsonrpc.common.wrappers.SetTaskPushNotificationConfigRequest; import io.a2a.jsonrpc.common.wrappers.SetTaskPushNotificationConfigResponse; +import io.a2a.jsonrpc.common.wrappers.ListTasksRequest; +import io.a2a.jsonrpc.common.wrappers.ListTasksResponse; import io.a2a.jsonrpc.common.wrappers.SubscribeToTaskRequest; import io.a2a.server.ServerCallContext; import io.a2a.server.auth.UnauthenticatedUser; @@ -56,7 +59,9 @@ import io.a2a.spec.Event; import io.a2a.spec.GetTaskPushNotificationConfigParams; import io.a2a.spec.InternalError; +import io.a2a.spec.InvalidParamsError; import io.a2a.spec.InvalidRequestError; +import io.a2a.spec.ListTasksParams; import io.a2a.spec.ListTaskPushNotificationConfigParams; import io.a2a.spec.Message; import io.a2a.spec.MessageSendParams; @@ -1959,4 +1964,27 @@ public void testNoVersionDefaultsToCurrentVersionSuccess() { assertNull(response.getError()); Assertions.assertSame(message, response.getResult()); } + + @Test + public void testListTasksEmptyResultIncludesAllFields() { + JSONRPCHandler handler = new JSONRPCHandler(CARD, requestHandler, internalExecutor); + + // Query for a context that doesn't exist - should return empty result + ListTasksParams params = ListTasksParams.builder() + .contextId("nonexistent-context-id") + .tenant("") + .build(); + ListTasksRequest request = new ListTasksRequest("1", params); + ListTasksResponse response = handler.onListTasks(request, callContext); + + // Should return success with all fields present (not null) + assertNull(response.getError()); + ListTasksResult result = response.getResult(); + Assertions.assertNotNull(result); + Assertions.assertNotNull(result.tasks(), "tasks field should not be null"); + assertEquals(0, result.tasks().size(), "tasks should be empty list"); + assertEquals(0, result.totalSize(), "totalSize should be 0"); + assertEquals(0, result.pageSize(), "pageSize should be 0"); + // nextPageToken can be null for empty results + } } diff --git a/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java b/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java index 87e57f415..d0a229cbc 100644 --- a/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java +++ b/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java @@ -219,21 +219,41 @@ public HTTPRestResponse listTasks(@Nullable String contextId, @Nullable String s paramsBuilder.contextId(contextId); } if (status != null) { + TaskState taskState = null; + + // Try JSON format first (e.g., "working", "completed") try { - paramsBuilder.status(TaskState.fromString(status)); + taskState = TaskState.fromString(status); } catch (IllegalArgumentException e) { - try { - paramsBuilder.status(TaskState.valueOf(status)); - } catch (IllegalArgumentException valueOfError) { - String validStates = Arrays.stream(TaskState.values()) - .map(TaskState::asString) - .collect(Collectors.joining(", ")); - Map errorData = new HashMap<>(); - errorData.put("parameter", "status"); - errorData.put("reason", "Must be one of: " + validStates); - throw new InvalidParamsError(null, "Invalid params", errorData); + // Try protobuf enum format (e.g., "TASK_STATE_WORKING") + if (status.startsWith("TASK_STATE_")) { + String enumName = status.substring("TASK_STATE_".length()); + try { + taskState = TaskState.valueOf(enumName); + } catch (IllegalArgumentException protoError) { + // Fall through to error handling below + } + } else { + // Try enum constant name directly (e.g., "WORKING") + try { + taskState = TaskState.valueOf(status); + } catch (IllegalArgumentException valueOfError) { + // Fall through to error handling below + } } } + + if (taskState == null) { + String validStates = Arrays.stream(TaskState.values()) + .map(TaskState::asString) + .collect(Collectors.joining(", ")); + Map errorData = new HashMap<>(); + errorData.put("parameter", "status"); + errorData.put("reason", "Must be one of: " + validStates); + throw new InvalidParamsError(null, "Invalid params", errorData); + } + + paramsBuilder.status(taskState); } if (pageSize != null) { paramsBuilder.pageSize(pageSize); @@ -247,12 +267,25 @@ public HTTPRestResponse listTasks(@Nullable String contextId, @Nullable String s paramsBuilder.tenant(tenant); if (lastUpdatedAfter != null) { try { - paramsBuilder.lastUpdatedAfter(Instant.parse(lastUpdatedAfter)); - } catch (DateTimeParseException e) { - Map errorData = new HashMap<>(); - errorData.put("parameter", "lastUpdatedAfter"); - errorData.put("reason", "Must be valid ISO-8601 timestamp"); - throw new InvalidParamsError(null, "Invalid params", errorData); + // Try parsing as Unix milliseconds first (integer) + long millis = Long.parseLong(lastUpdatedAfter); + if (millis < 0L) { + Map errorData = new HashMap<>(); + errorData.put("parameter", "lastUpdatedAfter"); + errorData.put("reason", "Must be a non-negative timestamp value, got: " + millis); + throw new InvalidParamsError(null, "Invalid params", errorData); + } + paramsBuilder.lastUpdatedAfter(Instant.ofEpochMilli(millis)); + } catch (NumberFormatException nfe) { + // Fall back to ISO-8601 format + try { + paramsBuilder.lastUpdatedAfter(Instant.parse(lastUpdatedAfter)); + } catch (DateTimeParseException e) { + Map errorData = new HashMap<>(); + errorData.put("parameter", "lastUpdatedAfter"); + errorData.put("reason", "Must be valid Unix milliseconds or ISO-8601 timestamp"); + throw new InvalidParamsError(null, "Invalid params", errorData); + } } } if (includeArtifacts != null) { @@ -338,7 +371,8 @@ private void validate(String json) { private HTTPRestResponse createSuccessResponse(int statusCode, com.google.protobuf.Message.Builder builder) { try { - String jsonBody = JsonFormat.printer().print(builder); + // Include default value fields to ensure empty arrays, zeros, etc. are present in JSON + String jsonBody = JsonFormat.printer().includingDefaultValueFields().print(builder); return new HTTPRestResponse(statusCode, "application/json", jsonBody); } catch (InvalidProtocolBufferException e) { return createErrorResponse(new InternalError("Failed to serialize response: " + e.getMessage())); diff --git a/transport/rest/src/test/java/io/a2a/transport/rest/handler/RestHandlerTest.java b/transport/rest/src/test/java/io/a2a/transport/rest/handler/RestHandlerTest.java index 1ed1492e1..a2e6ef6e9 100644 --- a/transport/rest/src/test/java/io/a2a/transport/rest/handler/RestHandlerTest.java +++ b/transport/rest/src/test/java/io/a2a/transport/rest/handler/RestHandlerTest.java @@ -879,4 +879,82 @@ public void testNoVersionDefaultsToCurrentVersionSuccess() { Assertions.assertEquals("application/json", response.getContentType()); Assertions.assertNotNull(response.getBody()); } + + @Test + public void testListTasksNegativeTimestampReturns422() { + RestHandler handler = new RestHandler(CARD, requestHandler, internalExecutor); + + // Negative timestamp should return 422 (Invalid params) + RestHandler.HTTPRestResponse response = handler.listTasks(null, null, null, null, + null, "-1", null, "", callContext); + + Assertions.assertEquals(422, response.getStatusCode()); + Assertions.assertEquals("application/json", response.getContentType()); + Assertions.assertTrue(response.getBody().contains("InvalidParamsError")); + } + + @Test + public void testListTasksUnixMillisecondsTimestamp() { + RestHandler handler = new RestHandler(CARD, requestHandler, internalExecutor); + taskStore.save(MINIMAL_TASK); + + // Unix milliseconds timestamp should be accepted + String timestampMillis = String.valueOf(System.currentTimeMillis() - 10000); + RestHandler.HTTPRestResponse response = handler.listTasks(null, null, null, null, + null, timestampMillis, null, "", callContext); + + Assertions.assertEquals(200, response.getStatusCode()); + Assertions.assertEquals("application/json", response.getContentType()); + Assertions.assertTrue(response.getBody().contains("tasks")); + } + + @Test + public void testListTasksProtobufEnumStatus() { + RestHandler handler = new RestHandler(CARD, requestHandler, internalExecutor); + taskStore.save(MINIMAL_TASK); + + // Protobuf enum format (TASK_STATE_SUBMITTED) should be accepted + RestHandler.HTTPRestResponse response = handler.listTasks(null, "TASK_STATE_SUBMITTED", null, null, + null, null, null, "", callContext); + + Assertions.assertEquals(200, response.getStatusCode()); + Assertions.assertEquals("application/json", response.getContentType()); + Assertions.assertTrue(response.getBody().contains(MINIMAL_TASK.id())); + } + + @Test + public void testListTasksEnumConstantStatus() { + RestHandler handler = new RestHandler(CARD, requestHandler, internalExecutor); + taskStore.save(MINIMAL_TASK); + + // Enum constant format (SUBMITTED) should be accepted + RestHandler.HTTPRestResponse response = handler.listTasks(null, "SUBMITTED", null, null, + null, null, null, "", callContext); + + Assertions.assertEquals(200, response.getStatusCode()); + Assertions.assertEquals("application/json", response.getContentType()); + Assertions.assertTrue(response.getBody().contains(MINIMAL_TASK.id())); + } + + @Test + public void testListTasksEmptyResultIncludesAllFields() { + RestHandler handler = new RestHandler(CARD, requestHandler, internalExecutor); + + // Query for a context that doesn't exist - should return empty result with all fields + RestHandler.HTTPRestResponse response = handler.listTasks("nonexistent-context-id", null, null, null, + null, null, null, "", callContext); + + Assertions.assertEquals(200, response.getStatusCode()); + Assertions.assertEquals("application/json", response.getContentType()); + + String body = response.getBody(); + // Verify all required fields are present (not missing) + Assertions.assertTrue(body.contains("\"tasks\""), "Response should contain tasks field"); + Assertions.assertTrue(body.contains("\"totalSize\""), "Response should contain totalSize field"); + Assertions.assertTrue(body.contains("\"pageSize\""), "Response should contain pageSize field"); + Assertions.assertTrue(body.contains("\"nextPageToken\""), "Response should contain nextPageToken field"); + // Verify empty array, not null + Assertions.assertTrue(body.contains("\"tasks\":[]") || body.contains("\"tasks\": []"), + "tasks should be empty array"); + } } From a9db41b5c417c76181da815553f3d84704319c4c Mon Sep 17 00:00:00 2001 From: Kabir Khan Date: Fri, 16 Jan 2026 15:45:03 +0000 Subject: [PATCH 2/3] refactor: Address Gemini code review feedback High Priority: - Fix NPE in InMemoryTaskStore binary search when task.status() is null - Add null checks for status and timestamp in pagination logic Code Quality Improvements: - Refactor JpaDatabaseTaskStore to parse pageToken once, eliminating duplication - Simplify redundant null check in InMemoryTaskStore sorting (timestamp always set) - Refactor JSONRPCUtils prefix handling to iterate over array instead of repeated if blocks - Extract pageSize constants (MIN=1, MAX=100, DEFAULT=50) in ListTasksParams - Extract TASK_STATE_PREFIX constant in RestHandler Error Message Improvements: - Make millisToInstant error message generic (not specific to lastUpdatedAfter) - Generate valid task states list dynamically from enum instead of hardcoded string API Consistency: - Make tenant validation use InvalidParamsError for consistency with other validations - Remove unused Assert import from ListTasksParams Build Hygiene: - Add *.args to .gitignore to prevent build artifact commits Co-Authored-By: Claude Sonnet 4.5 --- .gitignore | 1 + .../database/jpa/JpaDatabaseTaskStore.java | 56 +++++++++---------- .../a2a/server/tasks/InMemoryTaskStore.java | 28 ++++++---- .../a2a/grpc/mapper/A2ACommonFieldMapper.java | 8 ++- .../java/io/a2a/grpc/utils/JSONRPCUtils.java | 18 ++---- .../java/io/a2a/spec/ListTasksParams.java | 24 ++++---- .../transport/rest/handler/RestHandler.java | 5 +- 7 files changed, 72 insertions(+), 68 deletions(-) diff --git a/.gitignore b/.gitignore index dcc2f1cb8..bc62b4d6b 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ pom.xml.releaseBackup pom.xml.versionsBackup release.properties .flattened-pom.xml +*.args # Eclipse .project diff --git a/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java b/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java index a4a1f4f66..840773ea6 100644 --- a/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java +++ b/extras/task-store-database-jpa/src/main/java/io/a2a/extras/taskstore/database/jpa/JpaDatabaseTaskStore.java @@ -226,6 +226,25 @@ public ListTasksResult list(ListTasksParams params) { LOGGER.debug("Listing tasks with params: contextId={}, status={}, pageSize={}, pageToken={}", params.contextId(), params.status(), params.pageSize(), params.pageToken()); + // Parse pageToken once at the beginning + Instant tokenTimestamp = null; + String tokenId = null; + if (params.pageToken() != null && !params.pageToken().isEmpty()) { + String[] tokenParts = params.pageToken().split(":", 2); + if (tokenParts.length == 2) { + try { + long timestampMillis = Long.parseLong(tokenParts[0]); + tokenId = tokenParts[1]; + tokenTimestamp = Instant.ofEpochMilli(timestampMillis); + } catch (NumberFormatException e) { + throw new io.a2a.spec.InvalidParamsError(null, + "Invalid pageToken format: timestamp must be numeric milliseconds", null); + } + } else { + throw new io.a2a.spec.InvalidParamsError(null, "Invalid pageToken format: expected 'timestamp:id'", null); + } + } + // 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"); @@ -249,18 +268,9 @@ public ListTasksResult list(ListTasksParams params) { } // Apply pagination cursor using keyset pagination for composite sort (timestamp DESC, id ASC) - // PageToken format: "timestamp_millis:taskId" (e.g., "1699999999000:task-123") - if (params.pageToken() != null && !params.pageToken().isEmpty()) { - 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); - } + if (tokenTimestamp != null) { + // Keyset pagination: get tasks where timestamp < tokenTimestamp OR (timestamp = tokenTimestamp AND id > tokenId) + queryBuilder.append(" AND (t.statusTimestamp < :tokenTimestamp OR (t.statusTimestamp = :tokenTimestamp AND t.id > :tokenId))"); } // Sort by status timestamp descending (most recent first), then by ID for stable ordering @@ -279,25 +289,9 @@ public ListTasksResult list(ListTasksParams params) { if (params.lastUpdatedAfter() != null) { query.setParameter("lastUpdatedAfter", params.lastUpdatedAfter()); } - if (params.pageToken() != null && !params.pageToken().isEmpty()) { - String[] tokenParts = params.pageToken().split(":", 2); - if (tokenParts.length == 2) { - // Parse keyset pagination parameters - try { - long timestampMillis = Long.parseLong(tokenParts[0]); - String tokenId = tokenParts[1]; - - // All tasks have timestamps (TaskStatus canonical constructor ensures this) - Instant tokenTimestamp = Instant.ofEpochMilli(timestampMillis); - query.setParameter("tokenTimestamp", tokenTimestamp); - query.setParameter("tokenId", tokenId); - } catch (NumberFormatException e) { - // Malformed timestamp in pageToken - throw new io.a2a.spec.InvalidParamsError(null, - "Invalid pageToken format: timestamp must be numeric milliseconds", null); - } - } - // Note: Legacy ID-only format already rejected in query building phase + if (tokenTimestamp != null) { + query.setParameter("tokenTimestamp", tokenTimestamp); + query.setParameter("tokenId", tokenId); } // Apply page size limit (+1 to check for next page) diff --git a/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java b/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java index d9fb956df..d7af86095 100644 --- a/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java +++ b/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java @@ -57,7 +57,7 @@ public ListTasksResult list(ListTasksParams params) { task.status().timestamp() != null && task.status().timestamp().toInstant().isAfter(params.lastUpdatedAfter()))) .sorted(Comparator.comparing( - (Task t) -> (t.status() != null && t.status().timestamp() != null) + (Task t) -> (t.status() != null) // Truncate to milliseconds for consistency with pageToken precision ? t.status().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS) : null, @@ -90,18 +90,24 @@ public ListTasksResult list(ListTasksParams params) { int mid = left + (right - left) / 2; Task task = allFilteredTasks.get(mid); - // All tasks have timestamps (TaskStatus canonical constructor ensures this) - // Truncate to milliseconds for consistency with pageToken precision - java.time.Instant taskTimestamp = task.status().timestamp().toInstant() - .truncatedTo(java.time.temporal.ChronoUnit.MILLIS); - int timestampCompare = taskTimestamp.compareTo(tokenTimestamp); + java.time.Instant taskTimestamp = (task.status() != null && task.status().timestamp() != null) + ? task.status().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS) + : null; - if (timestampCompare < 0 || (timestampCompare == 0 && task.id().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 + 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.id().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; + } } } startIndex = left; diff --git a/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java b/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java index 9c0327c2d..723df2e53 100644 --- a/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java +++ b/spec-grpc/src/main/java/io/a2a/grpc/mapper/A2ACommonFieldMapper.java @@ -364,7 +364,7 @@ default long instantToMillis(Instant instant) { default Instant millisToInstant(long millis) { if (millis < 0L) { throw new InvalidParamsError(null, - "lastUpdatedAfter must be a valid timestamp (non-negative milliseconds since epoch), got: " + millis, + "Timestamp must be a non-negative number of milliseconds since epoch, but got: " + millis, null); } return millis > 0L ? Instant.ofEpochMilli(millis) : null; @@ -391,8 +391,12 @@ default io.a2a.spec.TaskState taskStateOrNull(io.a2a.grpc.TaskState state) { } // Reject invalid enum values (e.g., "INVALID_STATUS" from JSON) if (state == io.a2a.grpc.TaskState.UNRECOGNIZED) { + String validStates = java.util.Arrays.stream(io.a2a.spec.TaskState.values()) + .filter(s -> s != io.a2a.spec.TaskState.UNKNOWN) + .map(Enum::name) + .collect(java.util.stream.Collectors.joining(", ")); throw new InvalidParamsError(null, - "Invalid task state value. Must be one of: SUBMITTED, WORKING, INPUT_REQUIRED, AUTH_REQUIRED, COMPLETED, CANCELED, FAILED, REJECTED", + "Invalid task state value. Must be one of: " + validStates, null); } io.a2a.spec.TaskState result = TaskStateMapper.INSTANCE.fromProto(state); diff --git a/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java b/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java index 590c8b53d..c7d1df72f 100644 --- a/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java +++ b/spec-grpc/src/main/java/io/a2a/grpc/utils/JSONRPCUtils.java @@ -458,18 +458,12 @@ private static JsonProcessingException convertProtoBufExceptionToJsonProcessingE return new JsonProcessingException(ERROR_MESSAGE.formatted("unknown parsing error")); } - // Extract field name if present in error message - String prefix = "Cannot find field: "; - if (message.contains(prefix)) { - return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id); - } - prefix = "Invalid value for"; - if (message.contains(prefix)) { - return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id); - } - prefix = "Invalid enum value:"; - if (message.contains(prefix)) { - return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id); + // Extract field name if present in error message - check common prefixes + String[] prefixes = {"Cannot find field: ", "Invalid value for", "Invalid enum value:"}; + for (String prefix : prefixes) { + if (message.contains(prefix)) { + return new InvalidParamsJsonMappingException(ERROR_MESSAGE.formatted(message.substring(message.indexOf(prefix) + prefix.length())), id); + } } // Try to extract specific error details using regex patterns diff --git a/spec/src/main/java/io/a2a/spec/ListTasksParams.java b/spec/src/main/java/io/a2a/spec/ListTasksParams.java index 81fc0eb08..d6b242fae 100644 --- a/spec/src/main/java/io/a2a/spec/ListTasksParams.java +++ b/spec/src/main/java/io/a2a/spec/ListTasksParams.java @@ -2,7 +2,6 @@ import java.time.Instant; -import io.a2a.util.Assert; import org.jspecify.annotations.Nullable; /** @@ -27,6 +26,9 @@ public record ListTasksParams( @Nullable Boolean includeArtifacts, String tenant ) { + private static final int MIN_PAGE_SIZE = 1; + private static final int MAX_PAGE_SIZE = 100; + private static final int DEFAULT_PAGE_SIZE = 50; /** * Compact constructor for validation. * Validates that the tenant parameter is not null and parameters are within valid ranges. @@ -39,15 +41,17 @@ public record ListTasksParams( * @param lastUpdatedAfter filter by last update timestamp * @param includeArtifacts whether to include artifacts * @param tenant the tenant identifier - * @throws InvalidParamsError if pageSize or historyLength are out of valid range + * @throws InvalidParamsError if tenant is null or if pageSize or historyLength are out of valid range */ public ListTasksParams { - Assert.checkNotNullParam("tenant", tenant); + if (tenant == null) { + throw new InvalidParamsError(null, "Parameter 'tenant' may not be null", null); + } // Validate pageSize (1-100) - if (pageSize != null && (pageSize < 1 || pageSize > 100)) { + if (pageSize != null && (pageSize < MIN_PAGE_SIZE || pageSize > MAX_PAGE_SIZE)) { throw new InvalidParamsError(null, - "pageSize must be between 1 and 100, got: " + pageSize, null); + "pageSize must be between " + MIN_PAGE_SIZE + " and " + MAX_PAGE_SIZE + ", got: " + pageSize, null); } // Validate historyLength (>= 0) @@ -80,13 +84,13 @@ public ListTasksParams(Integer pageSize, String pageToken) { */ public int getEffectivePageSize() { if (pageSize == null) { - return 50; + return DEFAULT_PAGE_SIZE; } - if (pageSize < 1) { - return 1; + if (pageSize < MIN_PAGE_SIZE) { + return MIN_PAGE_SIZE; } - if (pageSize > 100) { - return 100; + if (pageSize > MAX_PAGE_SIZE) { + return MAX_PAGE_SIZE; } return pageSize; } diff --git a/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java b/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java index d0a229cbc..4d9a9667d 100644 --- a/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java +++ b/transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java @@ -69,6 +69,7 @@ public class RestHandler { private static final Logger log = Logger.getLogger(RestHandler.class.getName()); + private static final String TASK_STATE_PREFIX = "TASK_STATE_"; // Fields set by constructor injection cannot be final. We need a noargs constructor for // Jakarta compatibility, and it seems that making fields set by constructor injection @@ -226,8 +227,8 @@ public HTTPRestResponse listTasks(@Nullable String contextId, @Nullable String s taskState = TaskState.fromString(status); } catch (IllegalArgumentException e) { // Try protobuf enum format (e.g., "TASK_STATE_WORKING") - if (status.startsWith("TASK_STATE_")) { - String enumName = status.substring("TASK_STATE_".length()); + if (status.startsWith(TASK_STATE_PREFIX)) { + String enumName = status.substring(TASK_STATE_PREFIX.length()); try { taskState = TaskState.valueOf(enumName); } catch (IllegalArgumentException protoError) { From 4a21608fe2c7f029047b6260af07e0986762357d Mon Sep 17 00:00:00 2001 From: Kabir Khan Date: Fri, 16 Jan 2026 15:57:46 +0000 Subject: [PATCH 3/3] Gemini fixes --- .../a2a/server/tasks/InMemoryTaskStore.java | 2 +- .../java/io/a2a/spec/ListTasksParams.java | 22 +++++-------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java b/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java index d7af86095..f1243e272 100644 --- a/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java +++ b/server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java @@ -57,7 +57,7 @@ public ListTasksResult list(ListTasksParams params) { task.status().timestamp() != null && task.status().timestamp().toInstant().isAfter(params.lastUpdatedAfter()))) .sorted(Comparator.comparing( - (Task t) -> (t.status() != null) + (Task t) -> (t.status() != null && t.status().timestamp() != null) // Truncate to milliseconds for consistency with pageToken precision ? t.status().timestamp().toInstant().truncatedTo(java.time.temporal.ChronoUnit.MILLIS) : null, diff --git a/spec/src/main/java/io/a2a/spec/ListTasksParams.java b/spec/src/main/java/io/a2a/spec/ListTasksParams.java index d6b242fae..6fb9ff5d1 100644 --- a/spec/src/main/java/io/a2a/spec/ListTasksParams.java +++ b/spec/src/main/java/io/a2a/spec/ListTasksParams.java @@ -78,33 +78,23 @@ public ListTasksParams(Integer pageSize, String pageToken) { } /** - * Validates and returns the effective page size (between 1 and 100, defaults to 50). + * Returns the effective page size (defaults to 50 if not specified). + * Values are validated in the constructor to be within the range [1, 100]. * * @return the effective page size */ public int getEffectivePageSize() { - if (pageSize == null) { - return DEFAULT_PAGE_SIZE; - } - if (pageSize < MIN_PAGE_SIZE) { - return MIN_PAGE_SIZE; - } - if (pageSize > MAX_PAGE_SIZE) { - return MAX_PAGE_SIZE; - } - return pageSize; + return pageSize != null ? pageSize : DEFAULT_PAGE_SIZE; } /** - * Returns the effective history length (non-negative, defaults to 0). + * Returns the effective history length (defaults to 0 if not specified). + * Values are validated in the constructor to be non-negative. * * @return the effective history length */ public int getEffectiveHistoryLength() { - if (historyLength == null || historyLength < 0) { - return 0; - } - return historyLength; + return historyLength != null ? historyLength : 0; } /**