diff --git a/client/transport/grpc/src/main/java/org/a2aproject/sdk/client/transport/grpc/GrpcTransport.java b/client/transport/grpc/src/main/java/org/a2aproject/sdk/client/transport/grpc/GrpcTransport.java index 2a57fec64..7eb805f00 100644 --- a/client/transport/grpc/src/main/java/org/a2aproject/sdk/client/transport/grpc/GrpcTransport.java +++ b/client/transport/grpc/src/main/java/org/a2aproject/sdk/client/transport/grpc/GrpcTransport.java @@ -252,9 +252,7 @@ public TaskPushNotificationConfig getTaskPushNotificationConfiguration(GetTaskPu @Nullable ClientCallContext context) throws A2AClientException { checkNotNullParam("request", request); checkNotNullParam("taskId", request.taskId()); - if(request.id() == null) { - throw new IllegalArgumentException("Id must not be null"); - } + checkNotNullParam("id", request.id()); org.a2aproject.sdk.grpc.GetTaskPushNotificationConfigRequest grpcRequest = org.a2aproject.sdk.grpc.GetTaskPushNotificationConfigRequest.newBuilder() .setTaskId(request.taskId()) @@ -333,7 +331,7 @@ public void subscribeToTask(TaskIdParams request, Consumer e A2AServiceStub stubWithMetadata = createAsyncStubWithMetadata(context, payloadAndHeaders); stubWithMetadata.subscribeToTask(grpcRequest, streamObserver); } catch (StatusRuntimeException e) { - throw GrpcErrorMapper.mapGrpcError(e, "Failed to subscribe task push notification config: "); + throw GrpcErrorMapper.mapGrpcError(e, "Failed to subscribe to task: "); } } diff --git a/spec/src/main/java/org/a2aproject/sdk/spec/DataPart.java b/spec/src/main/java/org/a2aproject/sdk/spec/DataPart.java index e70c68394..42c668a20 100644 --- a/spec/src/main/java/org/a2aproject/sdk/spec/DataPart.java +++ b/spec/src/main/java/org/a2aproject/sdk/spec/DataPart.java @@ -7,6 +7,9 @@ import com.google.gson.ToNumberPolicy; import org.a2aproject.sdk.util.Assert; import org.a2aproject.sdk.spec.util.CollectionCopies; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Map; import org.jspecify.annotations.Nullable; @@ -23,7 +26,6 @@ *
  • JSON objects: {@code Map}
    • *
  • JSON arrays: {@code List} *
  • Primitives: {@code String}, {@code Number}, {@code Boolean}
    • - *
  • Null values: {@code null}
    • * *

    * Example usage: @@ -42,7 +44,7 @@ * DataPart primitive = new DataPart(42); * } * - * @param data the structured data (required, supports JSON objects, arrays, primitives, and null) + * @param data the structured data (required, supports JSON objects, arrays, and primitives) * @param metadata additional metadata for the part * @see Part * @see Message @@ -59,24 +61,25 @@ public record DataPart(Object data, @Nullable Map metadata) impl public static final String DATA = "data"; /** - * Compact constructor with validation. + * Compact constructor with validation and defensive copying. *

    - * Note: For mutable data types (Map, List), callers should ensure immutability - * by using {@code Map.copyOf()} or {@code List.copyOf()} before passing to this constructor. + * For mutable data types ({@code Map} and {@code List}), an unmodifiable defensive + * copy is created. Primitives and other immutable values are stored as-is. * - * @param data the structured data (supports JSON objects, arrays, primitives, and null) + * @param data the structured data (required, supports JSON objects, arrays, and primitives) + * @param metadata additional metadata for the part * @throws IllegalArgumentException if data is null */ public DataPart (Object data, @Nullable Map metadata) { Assert.checkNotNullParam("data", data); this.metadata = CollectionCopies.unmodifiableNullableShallowMap(metadata); - this.data = data; + this.data = defensivelyCopy(data); } /** * Constructor. * - * @param data the structured data (supports JSON objects, arrays, primitives, and not null) + * @param data the structured data (required, supports JSON objects, arrays, and primitives) * @throws IllegalArgumentException if data is null */ public DataPart(Object data) { @@ -137,4 +140,24 @@ public static DataPart fromJson(String json, @Nullable Map metad private static final Gson JSON_PARSER = new GsonBuilder() .setObjectToNumberStrategy(ToNumberPolicy.LONG_OR_DOUBLE) .create(); + + /** + * Creates a defensive copy of mutable collection types to ensure immutability. + *

    + * For {@code Map} and {@code List} instances, returns an unmodifiable copy preserving + * null elements. For all other types (primitives, Strings, immutable objects), returns + * the value as-is. + * + * @param data the data value to potentially copy + * @return an unmodifiable copy for collections, or the original value for immutable types + */ + private static Object defensivelyCopy(Object data) { + if (data instanceof Map map) { + return CollectionCopies.unmodifiableShallowMap(map); + } + if (data instanceof List list) { + return Collections.unmodifiableList(new ArrayList<>(list)); + } + return data; + } } diff --git a/spec/src/test/java/org/a2aproject/sdk/spec/DataPartTest.java b/spec/src/test/java/org/a2aproject/sdk/spec/DataPartTest.java index 8ae7732dc..1c75d4bfd 100644 --- a/spec/src/test/java/org/a2aproject/sdk/spec/DataPartTest.java +++ b/spec/src/test/java/org/a2aproject/sdk/spec/DataPartTest.java @@ -3,9 +3,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -108,4 +110,62 @@ void testFromJson_nullLiteralThrows() { void testFromJson_invalidJsonThrows() { assertThrows(IllegalArgumentException.class, () -> DataPart.fromJson("{invalid}")); } + + @SuppressWarnings("unchecked") + @Test + void testDataMapIsDefensivelyCopiedAndImmutable() { + Map data = new HashMap<>(); + data.put("temperature", 22.5); + + DataPart part = new DataPart(data); + + assertThrows(UnsupportedOperationException.class, () -> ((Map) part.data()).put("humidity", 65)); + data.put("humidity", 65); + assertEquals(Map.of("temperature", 22.5), part.data()); + } + + @Test + void testDataListIsDefensivelyCopiedAndImmutable() { + List data = new ArrayList<>(); + data.add("a"); + + DataPart part = new DataPart(data); + + assertThrows(UnsupportedOperationException.class, () -> ((List) part.data()).add("b")); + data.add("b"); + assertEquals(List.of("a"), part.data()); + } + + @Test + void testDataMapPreservesNullValues() { + Map data = new HashMap<>(); + data.put("source", null); + + DataPart part = new DataPart(data); + + assertTrue(part.data() instanceof Map); + assertTrue(((Map) part.data()).containsKey("source")); + assertNull(((Map) part.data()).get("source")); + } + + @Test + void testDataListPreservesNullElements() { + List data = new ArrayList<>(); + data.add(null); + + DataPart part = new DataPart(data); + + assertTrue(part.data() instanceof List); + assertEquals(1, ((List) part.data()).size()); + assertNull(((List) part.data()).get(0)); + } + + @Test + void testDataPrimitiveStoredAsIs() { + Integer data = 42; + + DataPart part = new DataPart(data); + + assertSame(data, part.data()); + } }