Skip to content

Commit 23dd672

Browse files
kabirkabir
committed
fix(server): address policy review
1 parent 6d43ff6 commit 23dd672

16 files changed

Lines changed: 122 additions & 57 deletions

File tree

‎compat-0.3/reference/rest/src/test/java/org/a2aproject/sdk/compat03/server/rest/quarkus/A2AServerRoutes_v0_3_Test.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,9 +118,7 @@ public void testGetTaskRawAliasValidationOrder() {
118118
fixture(true);
119119
params.set(alias, value);
120120
routes.getTask(rc);
121-
assertError(value.equals("-1") ? 500 : 422,
122-
value.equals("-1") ? "InternalError" : "InvalidParamsError",
123-
value.equals("-1") ? "Invalid history length" : "bad history_length or historyLength");
121+
assertError(422, "InvalidParamsError", "bad history_length or historyLength");
124122
verifyNoInteractions(handler);
125123
}
126124
}

‎compat-0.3/reference/rest/src/test/java/org/a2aproject/sdk/compat03/server/rest/quarkus/QuarkusA2ARest_v0_3_Test.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public void testHistoryAliasWireValidation() throws Exception {
5959
{"historyLength=", "422", "InvalidParamsError", "bad history_length or historyLength"},
6060
{"history_length=&historyLength=bad", "422", "InvalidParamsError",
6161
"Only one of 'history_length' or 'historyLength' may be specified"},
62-
{"history_length=-1", "500", "InternalError", "Invalid history length"},
62+
{"history_length=-1", "422", "InvalidParamsError", "bad history_length or historyLength"},
6363
{"history_length=2", "404", "TaskNotFoundError", "Task not found"},
6464
{"historyLength=2", "404", "TaskNotFoundError", "Task not found"},
6565
{"history_length=2&historyLength=bad", "422", "InvalidParamsError",

‎compat-0.3/transport/jsonrpc/src/main/java/org/a2aproject/sdk/compat03/transport/jsonrpc/handler/JSONRPCRequestDecoder_v0_3.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,10 @@ public static DecodeResult decode(@Nullable String body) {
106106
}
107107
}
108108

109-
@SuppressWarnings("NullAway") // Let Gson preserve the JSON parse error and message for a null body.
110109
private static com.google.gson.JsonObject parseBody(@Nullable String body) {
110+
if (body == null) {
111+
throw new JsonSyntaxException("Request body is required");
112+
}
111113
return JsonParser.parseString(body).getAsJsonObject();
112114
}
113115

‎compat-0.3/transport/jsonrpc/src/test/java/org/a2aproject/sdk/compat03/transport/jsonrpc/handler/JSONRPCRequestDecoder_v0_3_Test.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public void testNullBodyIsJsonParseError() {
6060
DecodeFailure failure = assertInstanceOf(DecodeFailure.class, JSONRPCRequestDecoder_v0_3.decode(null));
6161
assertInstanceOf(JSONParseError_v0_3.class, failure.error());
6262
assertEquals(-32700, failure.error().getCode());
63-
assertNotNull(failure.error().getMessage());
63+
assertEquals("Request body is required", failure.error().getMessage());
6464
}
6565

6666
@Test

‎compat-0.3/transport/rest/src/main/java/org/a2aproject/sdk/compat03/transport/rest/handler/RestHandler_v0_3.java‎

Lines changed: 26 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,7 @@ public HTTPRestResponse sendStreamingMessage(String body, ServerCallContext cont
124124

125125
public HTTPRestResponse cancelTask(@Nullable String taskId, ServerCallContext context) {
126126
try {
127-
if (Assert.isNullOrEmpty(taskId)) {
128-
throw new InvalidParamsError_v0_3("bad task id");
129-
}
130-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
127+
String requiredTaskId = requireNonEmpty("task id", taskId);
131128
TaskIdParams_v0_3 params = new TaskIdParams_v0_3(requiredTaskId);
132129
Task_v0_3 task = requestHandler.onCancelTask(params, context);
133130
if (task != null) {
@@ -145,15 +142,12 @@ public HTTPRestResponse cancelTask(@Nullable String taskId, ServerCallContext co
145142

146143
public HTTPRestResponse setTaskPushNotificationConfiguration(@Nullable String taskId, String body, ServerCallContext context) {
147144
try {
148-
if (Assert.isNullOrEmpty(taskId)) {
149-
throw new InvalidParamsError_v0_3("bad task id");
150-
}
145+
String requiredTaskId = requireNonEmpty("task id", taskId);
151146
if (!agentCard.capabilities().pushNotifications()) {
152147
throw new PushNotificationNotSupportedError_v0_3();
153148
}
154149
org.a2aproject.sdk.compat03.grpc.CreateTaskPushNotificationConfigRequest.Builder builder = org.a2aproject.sdk.compat03.grpc.CreateTaskPushNotificationConfigRequest.newBuilder();
155150
parseRequestBody(body, builder);
156-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
157151
String parent = "tasks/" + requiredTaskId;
158152
if (!builder.getParent().isBlank() && !parent.equals(builder.getParent())) {
159153
throw new InvalidParamsError_v0_3("bad parent");
@@ -195,10 +189,12 @@ public HTTPRestResponse setTaskPushNotificationConfiguration(@Nullable String ta
195189
}
196190

197191
public HTTPRestResponse resubscribeTask(@Nullable String taskId, ServerCallContext context) {
198-
if (Assert.isNullOrEmpty(taskId)) {
199-
return createErrorResponse(new InvalidParamsError_v0_3("bad task id"));
192+
String requiredTaskId;
193+
try {
194+
requiredTaskId = requireNonEmpty("task id", taskId);
195+
} catch (InvalidParamsError_v0_3 e) {
196+
return createErrorResponse(e);
200197
}
201-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
202198
try {
203199
if (!agentCard.capabilities().streaming()) {
204200
return createErrorResponse(new InvalidRequestError_v0_3("Streaming is not supported by the agent"));
@@ -217,10 +213,12 @@ public HTTPRestResponse resubscribeTask(@Nullable String taskId, ServerCallConte
217213

218214
public HTTPRestResponse getTask(@Nullable String taskId, @Nullable String historyLengthSnakeCase,
219215
@Nullable String historyLengthCamelCase, ServerCallContext context) {
220-
if (Assert.isNullOrEmpty(taskId)) {
221-
return createErrorResponse(new InvalidParamsError_v0_3("bad task id"));
216+
String requiredTaskId;
217+
try {
218+
requiredTaskId = requireNonEmpty("task id", taskId);
219+
} catch (InvalidParamsError_v0_3 e) {
220+
return createErrorResponse(e);
222221
}
223-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
224222
if (historyLengthSnakeCase != null && historyLengthCamelCase != null) {
225223
return createErrorResponse(new InvalidParamsError_v0_3(
226224
"Only one of 'history_length' or 'historyLength' may be specified"));
@@ -235,6 +233,9 @@ public HTTPRestResponse getTask(@Nullable String taskId, @Nullable String histor
235233
} catch (NumberFormatException e) {
236234
return createErrorResponse(new InvalidParamsError_v0_3("bad history_length or historyLength"));
237235
}
236+
if (historyLength < 0) {
237+
return createErrorResponse(new InvalidParamsError_v0_3("bad history_length or historyLength"));
238+
}
238239
try {
239240
TaskQueryParams_v0_3 params = new TaskQueryParams_v0_3(requiredTaskId, historyLength);
240241
Task_v0_3 task = requestHandler.onGetTask(params, context);
@@ -256,10 +257,7 @@ public HTTPRestResponse getTask(@Nullable String taskId, @Nullable String histor
256257
*/
257258
public HTTPRestResponse getTaskPushNotificationConfiguration(@Nullable String taskId, @Nullable String configId, ServerCallContext context) {
258259
try {
259-
if (Assert.isNullOrEmpty(taskId)) {
260-
throw new InvalidParamsError_v0_3("bad task id");
261-
}
262-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
260+
String requiredTaskId = requireNonEmpty("task id", taskId);
263261
if (!agentCard.capabilities().pushNotifications()) {
264262
throw new PushNotificationNotSupportedError_v0_3();
265263
}
@@ -277,10 +275,7 @@ public HTTPRestResponse getTaskPushNotificationConfiguration(@Nullable String ta
277275

278276
public HTTPRestResponse listTaskPushNotificationConfigurations(@Nullable String taskId, ServerCallContext context) {
279277
try {
280-
if (Assert.isNullOrEmpty(taskId)) {
281-
throw new InvalidParamsError_v0_3("bad task id");
282-
}
283-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
278+
String requiredTaskId = requireNonEmpty("task id", taskId);
284279
if (!agentCard.capabilities().pushNotifications()) {
285280
throw new PushNotificationNotSupportedError_v0_3();
286281
}
@@ -298,14 +293,8 @@ public HTTPRestResponse listTaskPushNotificationConfigurations(@Nullable String
298293

299294
public HTTPRestResponse deleteTaskPushNotificationConfiguration(@Nullable String taskId, @Nullable String configId, ServerCallContext context) {
300295
try {
301-
if (Assert.isNullOrEmpty(taskId)) {
302-
throw new InvalidParamsError_v0_3("bad task id");
303-
}
304-
String requiredTaskId = Assert.checkNotNullParam("taskId", taskId);
305-
if (Assert.isNullOrEmpty(configId)) {
306-
throw new InvalidParamsError_v0_3("bad config id");
307-
}
308-
String requiredConfigId = Assert.checkNotNullParam("configId", configId);
296+
String requiredTaskId = requireNonEmpty("task id", taskId);
297+
String requiredConfigId = requireNonEmpty("config id", configId);
309298
if (!agentCard.capabilities().pushNotifications()) {
310299
throw new PushNotificationNotSupportedError_v0_3();
311300
}
@@ -336,6 +325,13 @@ private void parseRequestBody(String body, com.google.protobuf.Message.Builder b
336325
}
337326
}
338327

328+
private static String requireNonEmpty(String parameter, @Nullable String value) {
329+
if (Assert.isNullOrEmpty(value)) {
330+
throw new InvalidParamsError_v0_3("bad " + parameter);
331+
}
332+
return Assert.checkNotNullParam(parameter, value);
333+
}
334+
339335
private void validate(String json) {
340336
try {
341337
JsonParser.parseString(json);

‎compat-0.3/transport/rest/src/test/java/org/a2aproject/sdk/compat03/transport/rest/handler/RestHandler_v0_3_Test.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -497,10 +497,10 @@ public void testSharedErrorConversionPreservesWireMessage() {
497497
assertEquals("{\"error\": \"org.a2aproject.sdk.compat03.spec.TaskNotFoundError_v0_3\", "
498498
+ "\"message\": \"Task not found\"}", response.getBody());
499499
response = handler.getTask("task", "-1", null, callContext);
500-
assertEquals(500, response.getStatusCode());
500+
assertEquals(422, response.getStatusCode());
501501
assertEquals("application/json", response.getContentType());
502-
assertEquals("{\"error\": \"org.a2aproject.sdk.compat03.spec.InternalError_v0_3\", "
503-
+ "\"message\": \"Invalid history length\"}", response.getBody());
502+
assertEquals("{\"error\": \"org.a2aproject.sdk.compat03.spec.InvalidParamsError_v0_3\", "
503+
+ "\"message\": \"bad history_length or historyLength\"}", response.getBody());
504504
}
505505

506506
private RestHandler_v0_3 validationHandler(boolean capabilities, RequestHandler backend) {
@@ -538,11 +538,10 @@ public void testGetTaskRawAliases() {
538538
"Only one of 'history_length' or 'historyLength' may be specified");
539539
}
540540
for (String value : new String[] {"", "bad", "2147483648", "-1"}) {
541-
int status = value.equals("-1") ? 500 : 422;
542-
String type = value.equals("-1") ? "InternalError" : "InvalidParamsError";
543-
String message = value.equals("-1") ? "Invalid history length" : "bad history_length or historyLength";
544-
assertWireError(handler.getTask("task", value, null, callContext), status, type, message);
545-
assertWireError(handler.getTask("task", null, value, callContext), status, type, message);
541+
assertWireError(handler.getTask("task", value, null, callContext), 422, "InvalidParamsError",
542+
"bad history_length or historyLength");
543+
assertWireError(handler.getTask("task", null, value, callContext), 422, "InvalidParamsError",
544+
"bad history_length or historyLength");
546545
}
547546
verifyNoInteractions(backend);
548547
for (String[] aliases : new String[][] {{null, null}, {"2", null}, {null, "2"}}) {

‎reference/jsonrpc/src/test/java/org/a2aproject/sdk/server/apps/quarkus/A2AServerRoutesTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -958,6 +958,7 @@ private void setField(Object target, String fieldName, Object value) {
958958
throw new RuntimeException("Failed to set field: " + fieldName, e);
959959
}
960960
}
961+
961962
@Test
962963
public void testEmptyMessageIdDecodeErrorRetainsId() {
963964
routes.invokeJSONRPCHandler("""

‎server-common/src/main/java/org/a2aproject/sdk/server/AgentCardOperation.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,19 @@
55

66
import org.a2aproject.sdk.spec.A2AError;
77
import org.a2aproject.sdk.spec.AgentCard;
8+
import org.a2aproject.sdk.spec.InternalError;
89
import org.a2aproject.sdk.util.Assert;
9-
import org.jspecify.annotations.Nullable;
1010

1111
/** The capability and transport-specific failure for an operation. */
1212
public record AgentCardOperation(Predicate<AgentCard> capability,
13-
@Nullable Supplier<? extends A2AError> capabilityFailure) {
13+
Supplier<? extends A2AError> capabilityFailure) {
1414
public AgentCardOperation {
1515
Assert.checkNotNullParam("capability", capability);
16+
Assert.checkNotNullParam("capabilityFailure", capabilityFailure);
1617
}
1718

1819
public static AgentCardOperation none() {
19-
return new AgentCardOperation(card -> true, null);
20+
return new AgentCardOperation(card -> true,
21+
() -> new InternalError("No capability failure was configured"));
2022
}
2123
}

‎server-common/src/main/java/org/a2aproject/sdk/server/AgentCardRequestPolicy.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public AgentCardRequestPolicy(AgentCardResolver resolver) {
2020
public EffectiveAgentCard preflight(@Nullable String tenant, ServerCallContext context,
2121
AgentCardTransportValidator validator, AgentCardOperation operation) {
2222
String normalizedTenant = normalizeTenant(tenant);
23-
AgentCard card = resolver.resolveEffectiveCard(normalizedTenant);
23+
AgentCard card = resolver.resolveEffectiveCardForNormalizedTenant(normalizedTenant);
2424
validator.validate(card);
2525
validateContext(card, context);
2626
validateCapability(card, operation);
@@ -30,7 +30,7 @@ public EffectiveAgentCard preflight(@Nullable String tenant, ServerCallContext c
3030
public AgentCard resolveAndValidateCapability(@Nullable String tenant,
3131
AgentCardTransportValidator validator, AgentCardOperation operation) {
3232
String normalizedTenant = normalizeTenant(tenant);
33-
AgentCard card = resolver.resolveEffectiveCard(normalizedTenant);
33+
AgentCard card = resolver.resolveEffectiveCardForNormalizedTenant(normalizedTenant);
3434
validator.validate(card);
3535
validateCapability(card, operation);
3636
return card;
@@ -50,7 +50,7 @@ private static void validateContext(AgentCard card, ServerCallContext context) {
5050

5151
private static void validateCapability(AgentCard card, AgentCardOperation operation) {
5252
if (!operation.capability().test(card)) {
53-
throw Objects.requireNonNull(operation.capabilityFailure(), "Capability failure is required").get();
53+
throw operation.capabilityFailure().get();
5454
}
5555
}
5656

‎server-common/src/main/java/org/a2aproject/sdk/server/AgentCardResolver.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ public static AgentCardResolver of(Supplier<@Nullable AgentCard> publicCard,
3030
/** Selects public then extended cards, never using defaults for a routed nonblank tenant. */
3131
public AgentCard resolveEffectiveCard(@Nullable String tenant) {
3232
String normalizedTenant = normalizeTenantForResolution(tenant);
33+
return resolveEffectiveCardForNormalizedTenant(normalizedTenant);
34+
}
35+
36+
/** Resolves a card for a tenant already normalized by the shared request policy. */
37+
AgentCard resolveEffectiveCardForNormalizedTenant(@Nullable String normalizedTenant) {
3338
if (router != null && normalizedTenant != null) {
3439
AgentCard card = router.resolvePublicCard(normalizedTenant);
3540
if (card == null) {

0 commit comments

Comments
 (0)