From 1131589132fd8397ca555ca743ca364d8d70b8ee Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Mon, 28 Sep 2026 15:03:46 +0200 Subject: [PATCH] fix(api-model): DataWrapper.items is a List, so a caller can read it by position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `items` is an ordered JSON array on the wire, in the order the caller asked for — `sortBy` for a read, insertion order for a bulk write — but it was typed as `Collection`, which throws that away. Every caller reading a single result had to write client.timeseries().retrieve(request).getItems().iterator().next() because `Collection` has no `get(int)`. This is not hypothetical: the SDK docs described `getItems().get(0)` on 27 pages and none of them compiled, and the Python client says `retrieve_datapoints(rf)[0]` for the same read. The envelope was the only reason Java looked worse than the language it wraps. The getter narrows to `List`; the setter still takes any `Collection`, so the graph endpoints that hand over a `Set` keep compiling. Three behaviours are held fixed on purpose, and `DataWrapperItemsListTest` pins each: - a list is held as given, not copied, so a caller mutating the list it passed still sees the envelope change; - a non-list is copied and keeps that collection's iteration order; - `null` is still stored as `null` — several request validators test `getItems() == null` to reject a body that sent `"items": null`, and an absent array must not arrive as an empty one. The wire shape does not change in either direction. `Iterable` was considered instead and rejected: it does not help read the first of one, and an envelope carrying `nextCursor` that answers a for-each with a single page invites a silent paging bug. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: jgjesdal --- .../datahub/api/responses/DataWrapper.java | 22 ++++- .../responses/DataWrapperItemsListTest.java | 81 +++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 datahub-api-model/src/test/java/ai/intellistream/datahub/api/responses/DataWrapperItemsListTest.java diff --git a/datahub-api-model/src/main/java/ai/intellistream/datahub/api/responses/DataWrapper.java b/datahub-api-model/src/main/java/ai/intellistream/datahub/api/responses/DataWrapper.java index d1e46888..d0ecd036 100644 --- a/datahub-api-model/src/main/java/ai/intellistream/datahub/api/responses/DataWrapper.java +++ b/datahub-api-model/src/main/java/ai/intellistream/datahub/api/responses/DataWrapper.java @@ -12,6 +12,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.List; @JsonRootName(value = "data") @Schema(name="DataWrapper", description="DataWrapper with items") @@ -20,10 +21,15 @@ public class DataWrapper { // Bounded on the request side: every create/update/delete endpoint that is not GraphDataWrapper // binds this envelope, and an unbounded items[] is how a caller turns many small entities into // bulk storage. Responses are never bean-validated, so paging is unaffected. + // + // A List, not a Collection: items[] is an ordered JSON array on the wire, the order is the one + // the caller asked for (sort_by, or insertion for a bulk write), and every reader wants to + // address it by position. Typing it as Collection threw that away and left callers writing + // getItems().iterator().next() to read the first of one. @JacksonXmlElementWrapper(useWrapping = false) @Valid @Size(max = FieldLimits.BATCH_ITEMS_MAX) - private Collection items = new ArrayList<>(); + private List items = new ArrayList<>(); /** * Policy warnings raised while writing these items, or null when there were none. @@ -62,12 +68,22 @@ public DataWrapper setNextCursor(String nextCursor) { } @JsonInclude(JsonInclude.Include.ALWAYS) - public Collection getItems() { + public List getItems() { return items; } + /** + * Still takes any {@link Collection}, so the callers that hand one that is not a list — the + * graph endpoints, whose nodes and relations are sets — keep compiling. + * + *

A list is kept as it is rather than copied, so a caller that mutates the list it passed + * still sees the envelope change, exactly as before this field was narrowed. Only a non-list + * is copied, and it takes that collection's iteration order. {@code null} is still stored as + * {@code null}: an inbound {@code "items": null} has to stay distinguishable from an empty + * array, because that is what several request validators test for. + */ public DataWrapper setItems(Collection items) { - this.items = items; + this.items = items == null || items instanceof List ? (List) items : new ArrayList<>(items); return this; } diff --git a/datahub-api-model/src/test/java/ai/intellistream/datahub/api/responses/DataWrapperItemsListTest.java b/datahub-api-model/src/test/java/ai/intellistream/datahub/api/responses/DataWrapperItemsListTest.java new file mode 100644 index 00000000..4b4ab390 --- /dev/null +++ b/datahub-api-model/src/test/java/ai/intellistream/datahub/api/responses/DataWrapperItemsListTest.java @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 +package ai.intellistream.datahub.api.responses; + +import org.junit.jupiter.api.Test; +import tools.jackson.core.type.TypeReference; +import tools.jackson.databind.json.JsonMapper; + +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; + +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; + +/** + * {@code items} is a {@link List}, and the three things that had to keep working when it stopped + * being a {@code Collection}: a caller may still hand over a set, a list it hands over is still the + * one the envelope holds, and {@code "items": null} is still null rather than empty. + * + *

The narrowing is what lets a caller read the first of one result by position. Reading it + * through {@code getItems().iterator().next()} was the only way while this was a {@code Collection}, + * and every SDK example that reads a single series had to do exactly that. + */ +class DataWrapperItemsListTest { + + private final JsonMapper mapper = JsonMapper.builder().build(); + + @Test + void itemsIsAListAndIsAddressableByPosition() { + DataWrapper wrapper = new DataWrapper().setItems(List.of("first", "second")); + + assertInstanceOf(List.class, wrapper.getItems()); + assertEquals("first", wrapper.getItems().get(0)); + assertEquals("second", wrapper.getItems().get(1)); + } + + @Test + void aListIsHeldAsGivenRatherThanCopied() { + // Callers that mutate the list they passed still see the envelope change, as before. + List given = new ArrayList<>(List.of("a")); + DataWrapper wrapper = new DataWrapper().setItems(given); + + assertSame(given, wrapper.getItems()); + given.add("b"); + assertEquals(List.of("a", "b"), wrapper.getItems()); + } + + @Test + void aSetIsAcceptedAndKeepsItsIterationOrder() { + // The graph endpoints hand over sets; DataWrapper still takes any Collection. + DataWrapper wrapper = + new DataWrapper().setItems(new LinkedHashSet<>(List.of("x", "y", "z"))); + + assertEquals(List.of("x", "y", "z"), wrapper.getItems()); + } + + @Test + void nullItemsStayNull() { + // Several request validators test `getItems() == null` to reject a body that sent + // "items": null, so an absent array must not arrive as an empty one. + assertNull(new DataWrapper().setItems(null).getItems()); + + DataWrapper parsed = mapper.readValue( + "{\"items\":null}", new TypeReference>() {}); + assertNull(parsed.getItems()); + } + + @Test + void theWireShapeIsUnchanged() { + DataWrapper wrapper = new DataWrapper().setItems(List.of("x", "y")); + assertEquals("{\"items\":[\"x\",\"y\"]}", mapper.writeValueAsString(wrapper)); + + DataWrapper parsed = mapper.readValue( + "{\"items\":[\"x\",\"y\"],\"nextCursor\":\"c1\"}", + new TypeReference>() {}); + assertEquals(List.of("x", "y"), parsed.getItems()); + assertEquals("c1", parsed.getNextCursor()); + } +}