Skip to content

Commit 21810d7

Browse files
dfa1claude
andcommitted
refactor(reader): ArrayNode carries the typed EncodingId
Collapse the KnownArrayNode/UnknownArrayNode sealed pair into a single ArrayNode record: with EncodingId sealed, the Known/Unknown bit lives in the id type (WellKnown vs Custom), so the parallel node hierarchy and its switch dances at every dispatch site encode nothing. ReadRegistry returns to the single string-keyed map hit; the allowUnknown passthrough and error messages are unchanged for both former variants. The redundant ArrayNode.of factory is gone — the canonical constructor takes the typed id. Hardening from review: a crafted file with a blank encoding id in the spec table now fails as VortexException in FlatSegmentDecoder (EncodingId.parse rejects blank with IllegalArgumentException, which must not escape the untrusted read path — previously "" flowed to the unknown-id path and could pass through allowUnknown); WriteRegistry's builder TreeMap orders by wire string instead of natural ordering, which a Custom-keyed registration would break. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent ea88a91 commit 21810d7

48 files changed

Lines changed: 252 additions & 289 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

core/src/main/java/io/github/dfa1/vortex/core/model/EncodingId.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,15 @@ public sealed interface EncodingId extends Serializable permits EncodingId.WellK
2222
/// @return the wire string of this encoding id
2323
String id();
2424

25-
/// Total parse: every non-null string has a typed representation.
26-
/// Returns the matching [WellKnown] constant, else a [Custom] wrapping the raw string.
25+
/// Parses a wire string into its typed representation: the matching [WellKnown] constant,
26+
/// else a [Custom] wrapping the raw string. Total over every non-blank string; blank input
27+
/// is not a valid encoding id and is rejected by the [Custom] constructor — callers parsing
28+
/// untrusted input must guard blank ids and raise their own domain error.
2729
///
2830
/// @param raw the raw encoding id string (e.g. `"vortex.primitive"`)
2931
/// @return the matching [WellKnown] constant, or a [Custom] wrapping `raw` if none matches
32+
/// @throws NullPointerException if `raw` is `null`
33+
/// @throws IllegalArgumentException if `raw` is blank
3034
static EncodingId parse(String raw) {
3135
WellKnown known = WellKnown.byId(raw);
3236
return known != null ? known : new Custom(raw);

core/src/test/java/io/github/dfa1/vortex/core/model/EncodingIdTest.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import org.junit.jupiter.api.Test;
55
import org.junit.jupiter.params.ParameterizedTest;
66
import org.junit.jupiter.params.provider.EnumSource;
7+
import org.junit.jupiter.params.provider.ValueSource;
78

89
import static org.assertj.core.api.Assertions.assertThat;
910
import static org.assertj.core.api.Assertions.assertThatThrownBy;
@@ -33,6 +34,15 @@ void parse_unknownId_returnsCustomWrappingRawId() {
3334
assertThat(result).isEqualTo(new EncodingId.Custom(raw));
3435
assertThat(result.id()).isEqualTo(raw);
3536
}
37+
38+
@ParameterizedTest
39+
@ValueSource(strings = {"", " ", " "})
40+
void parse_blankId_throwsIllegalArgumentException(String blank) {
41+
// Given / When / Then — blank is not a valid id; parse must not silently wrap it,
42+
// so untrusted-input callers are forced to guard it into their own domain error
43+
assertThatThrownBy(() -> EncodingId.parse(blank))
44+
.isInstanceOf(IllegalArgumentException.class);
45+
}
3646
}
3747

3848
@Nested

reader/src/main/java/io/github/dfa1/vortex/reader/FlatSegmentDecoder.java

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@
1010
import io.github.dfa1.vortex.core.fbs.FbsBuffer;
1111
import io.github.dfa1.vortex.reader.decode.ArrayNode;
1212
import io.github.dfa1.vortex.reader.decode.DecodeContext;
13-
import io.github.dfa1.vortex.reader.decode.KnownArrayNode;
14-
import io.github.dfa1.vortex.reader.decode.UnknownArrayNode;
1513

1614
import java.lang.foreign.MemorySegment;
1715
import java.lang.foreign.SegmentAllocator;
@@ -87,6 +85,11 @@ private static ArrayNode convertArrayNode(
8785
"array tree depth exceeds limit (" + MAX_ARRAY_TREE_DEPTH + ")");
8886
}
8987
String rawEncodingId = encodingSpecs.get(fbs.encoding());
88+
if (rawEncodingId.isBlank()) {
89+
// EncodingId.parse rejects blank ids with IllegalArgumentException; the file is
90+
// untrusted input, so a blank spec entry must surface as VortexException instead.
91+
throw new VortexException("blank encoding id at array spec index " + fbs.encoding());
92+
}
9093

9194
ArrayNode[] children = new ArrayNode[fbs.childrenLength()];
9295
for (int i = 0; i < children.length; i++) {
@@ -99,11 +102,6 @@ private static ArrayNode convertArrayNode(
99102
}
100103

101104
MemorySegment meta = fbs.metadataAsSegment();
102-
return switch (EncodingId.parse(rawEncodingId)) {
103-
case EncodingId.WellKnown wellKnown ->
104-
new KnownArrayNode(wellKnown, meta, children, bufferIndices);
105-
case EncodingId.Custom custom ->
106-
new UnknownArrayNode(custom.id(), meta, children, bufferIndices);
107-
};
105+
return new ArrayNode(EncodingId.parse(rawEncodingId), meta, children, bufferIndices);
108106
}
109107
}

reader/src/main/java/io/github/dfa1/vortex/reader/ReadRegistry.java

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
import io.github.dfa1.vortex.reader.decode.ArrayNode;
88
import io.github.dfa1.vortex.reader.decode.DecodeContext;
99
import io.github.dfa1.vortex.reader.decode.EncodingDecoder;
10-
import io.github.dfa1.vortex.reader.decode.KnownArrayNode;
11-
import io.github.dfa1.vortex.reader.decode.UnknownArrayNode;
1210

1311
import java.lang.foreign.MemorySegment;
1412
import java.util.Collections;
@@ -76,21 +74,14 @@ public boolean hasDecoder(EncodingId encodingId) {
7674
/// @return the decoded [Array]
7775
public Array decode(DecodeContext ctx) {
7876
ArrayNode node = ctx.node();
79-
EncodingDecoder decoder = switch (node) {
80-
case KnownArrayNode k -> decoders.get(k.encodingId().id());
81-
case UnknownArrayNode _ -> null;
82-
};
77+
EncodingDecoder decoder = decoders.get(node.encodingId().id());
8378
if (decoder != null) {
8479
return decoder.decode(ctx);
8580
}
8681
if (allowUnknown) {
8782
return decodeUnknown(ctx, node);
8883
}
89-
String id = switch (node) {
90-
case KnownArrayNode k -> k.encodingId().id();
91-
case UnknownArrayNode u -> u.rawEncodingId();
92-
};
93-
throw new VortexException("no decoder registered for " + id);
84+
throw new VortexException("no decoder registered for " + node.encodingId().id());
9485
}
9586

9687
/// Decodes the array described by `ctx` and returns its primary backing segment.
@@ -99,25 +90,16 @@ public Array decode(DecodeContext ctx) {
9990
/// @return the primary [MemorySegment] of the decoded array
10091
public MemorySegment decodeAsSegment(DecodeContext ctx) {
10192
ArrayNode node = ctx.node();
102-
EncodingDecoder decoder = switch (node) {
103-
case KnownArrayNode k -> decoders.get(k.encodingId().id());
104-
case UnknownArrayNode _ -> null;
105-
};
93+
EncodingDecoder decoder = decoders.get(node.encodingId().id());
10694
if (decoder != null) {
10795
return decoder.decode(ctx).materialize(ctx.arena());
10896
}
109-
String id = switch (node) {
110-
case KnownArrayNode k -> k.encodingId().id();
111-
case UnknownArrayNode u -> u.rawEncodingId();
112-
};
113-
throw new VortexException("no decoder registered for " + id + " (or encoding has no primary segment)");
97+
throw new VortexException("no decoder registered for " + node.encodingId().id()
98+
+ " (or encoding has no primary segment)");
11499
}
115100

116101
private static UnknownArray decodeUnknown(DecodeContext ctx, ArrayNode node) {
117-
String rawId = switch (node) {
118-
case KnownArrayNode k -> k.encodingId().id();
119-
case UnknownArrayNode u -> u.rawEncodingId();
120-
};
102+
String rawId = node.encodingId().id();
121103
MemorySegment[] bufs = new MemorySegment[node.bufferIndices().length];
122104
for (int i = 0; i < bufs.length; i++) {
123105
bufs[i] = ctx.buffer(i);

reader/src/main/java/io/github/dfa1/vortex/reader/decode/ArrayNode.java

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -4,40 +4,26 @@
44

55
import java.lang.foreign.MemorySegment;
66

7-
/// Encoded array node as stored in a Flat layout segment.
8-
/// In-file representation before decoding; mirrors the Go ArrayNode struct.
7+
/// Encoded array node as stored in a Flat layout segment — the in-file representation before
8+
/// decoding.
99
///
10-
/// Sealed: a node is either [KnownArrayNode] (id resolves to an [EncodingId.WellKnown]) or
11-
/// [UnknownArrayNode] (id is an arbitrary string only meaningful for
12-
/// [io.github.dfa1.vortex.reader.ReadRegistry#isAllowUnknown()] passthrough decode).
13-
public sealed interface ArrayNode permits KnownArrayNode, UnknownArrayNode {
14-
15-
/// Factory that builds the right node kind for `encodingId`: a [KnownArrayNode] for a
16-
/// well-known id, or an [UnknownArrayNode] for a [EncodingId.Custom] one.
17-
/// Mostly used by tests and helper code that converts an `EncodeNode` tree back into
18-
/// an `ArrayNode` tree.
19-
///
20-
/// @param encodingId the encoding identifier
21-
/// @param metadata encoding-specific metadata bytes, or `null`
22-
/// @param children child nodes
23-
/// @param bufferIndices segment buffer indices for this node
24-
/// @return a [KnownArrayNode] for a well-known id, else an [UnknownArrayNode]
25-
static ArrayNode of(EncodingId encodingId, MemorySegment metadata, ArrayNode[] children,
26-
int[] bufferIndices) {
27-
return switch (encodingId) {
28-
case EncodingId.WellKnown wellKnown ->
29-
new KnownArrayNode(wellKnown, metadata, children, bufferIndices);
30-
case EncodingId.Custom custom ->
31-
new UnknownArrayNode(custom.id(), metadata, children, bufferIndices);
32-
};
33-
}
34-
35-
/// @return encoding-specific metadata bytes, or `null`
36-
MemorySegment metadata();
37-
38-
/// @return child nodes
39-
ArrayNode[] children();
40-
41-
/// @return segment buffer indices for this node
42-
int[] bufferIndices();
10+
/// The encoding id is carried typed, as either an [EncodingId.WellKnown] constant or an
11+
/// [EncodingId.Custom] wrapping a raw wire string, so the Known/Unknown distinction lives in the id
12+
/// itself rather than in a node subtype. Whether an id is decodable is a separate,
13+
/// decode-time question owned by the [io.github.dfa1.vortex.reader.ReadRegistry], not a property of
14+
/// the node: an [EncodingId.Custom] id, or a [EncodingId.WellKnown] id no registered decoder
15+
/// claims, either decodes as a passthrough [io.github.dfa1.vortex.reader.array.UnknownArray] (when
16+
/// [io.github.dfa1.vortex.reader.ReadRegistry#isAllowUnknown()] is set) or fails the decode.
17+
///
18+
/// @param encodingId the encoding identifier, [EncodingId.WellKnown] or [EncodingId.Custom]
19+
/// @param metadata encoding-specific metadata bytes, or `null`
20+
/// @param children child nodes
21+
/// @param bufferIndices segment buffer indices for this node
22+
@SuppressWarnings("java:S6218") // internal data carrier; record components are arrays of immutable primitives or refs that flow through pipelines without ever being compared.
23+
public record ArrayNode(
24+
EncodingId encodingId,
25+
MemorySegment metadata,
26+
ArrayNode[] children,
27+
int[] bufferIndices
28+
) {
4329
}

reader/src/main/java/io/github/dfa1/vortex/reader/decode/KnownArrayNode.java

Lines changed: 0 additions & 20 deletions
This file was deleted.

reader/src/main/java/io/github/dfa1/vortex/reader/decode/UnknownArrayNode.java

Lines changed: 0 additions & 21 deletions
This file was deleted.

reader/src/test/java/io/github/dfa1/vortex/reader/FlatSegmentDecoderDecodeTest.java

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.github.dfa1.vortex.core.fbs.FbsArrayNode;
44
import io.github.dfa1.vortex.core.fbs.FbsBuffer;
55
import io.github.dfa1.vortex.core.fbs.FbsBuilder;
6+
import io.github.dfa1.vortex.core.error.VortexException;
67
import io.github.dfa1.vortex.core.model.DType;
78
import io.github.dfa1.vortex.reader.array.Array;
89
import io.github.dfa1.vortex.reader.array.UnknownArray;
@@ -14,15 +15,16 @@
1415

1516
import static io.github.dfa1.vortex.core.io.PTypeIO.LE_INT;
1617
import static org.assertj.core.api.Assertions.assertThat;
18+
import static org.assertj.core.api.Assertions.catchThrowable;
1719

1820
/// Successful flat-segment decode path — complements [FlatSegmentBoundsSecurityTest] (which only
1921
/// drives the rejection paths). A buffer descriptor with non-zero padding exercises the offset
20-
/// walk, and an unknown encoding id exercises the `UnknownArrayNode` fallback through an
22+
/// walk, and an unknown encoding id exercises the unknown-id passthrough through an
2123
/// allow-unknown registry. Together these pin two otherwise-untested spots:
2224
/// - the `dataOffset += padding` accumulation: with padding > 0, flipping `+=` to `-=` slices at a
2325
/// negative offset and fails, so a clean decode proves the addition.
24-
/// - the `orElseGet(() -> new UnknownArrayNode(...))` fallback: returning `null` there yields a
25-
/// null node and the decode would not produce an `UnknownArray`.
26+
/// - the unknown-id node construction: mishandling an unresolvable id there yields a
27+
/// node the decode would not turn into an `UnknownArray`.
2628
class FlatSegmentDecoderDecodeTest {
2729

2830
@Test
@@ -46,11 +48,36 @@ void decode_unknownEncodingWithBufferPadding_returnsUnknownArray() {
4648
DType.I32, 0, arena);
4749

4850
// Then — the allow-unknown path produced an UnknownArray (proves both the +padding
49-
// walk and the UnknownArrayNode fallback ran)
51+
// walk and the unknown-id passthrough ran)
5052
assertThat(result).isInstanceOf(UnknownArray.class);
5153
}
5254
}
5355

56+
@Test
57+
void decode_blankEncodingId_throwsVortexException() {
58+
ReadRegistry registry = ReadRegistry.builder().allowUnknown().build();
59+
FlatSegmentDecoder sut = new FlatSegmentDecoder(registry);
60+
61+
try (Arena arena = Arena.ofConfined()) {
62+
// Given — a zero-length FlatBuffer string in the spec table decodes to "", which
63+
// EncodingId.parse rejects with IllegalArgumentException; untrusted input must
64+
// surface as VortexException instead, even under allowUnknown
65+
byte[] fb = arrayFlatBufferOneBuffer(0, 0L);
66+
MemorySegment seg = arena.allocate((long) fb.length + 4);
67+
MemorySegment.copy(MemorySegment.ofArray(fb), 0, seg, 0, fb.length);
68+
seg.set(LE_INT, fb.length, fb.length);
69+
70+
// When
71+
Throwable result = catchThrowable(() -> sut.decode(seg, List.of(""),
72+
DType.I32, 0, arena));
73+
74+
// Then
75+
assertThat(result)
76+
.isInstanceOf(VortexException.class)
77+
.hasMessageContaining("blank encoding id");
78+
}
79+
}
80+
5481
/// Builds an `Array` FlatBuffer with a single buffer descriptor of the given padding/length.
5582
private static byte[] arrayFlatBufferOneBuffer(int padding, long length) {
5683
FbsBuilder b = new FbsBuilder();

reader/src/test/java/io/github/dfa1/vortex/reader/ReadRegistryTest.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import io.github.dfa1.vortex.reader.decode.ArrayNode;
99
import io.github.dfa1.vortex.reader.decode.DecodeContext;
1010
import io.github.dfa1.vortex.reader.decode.EncodingDecoder;
11-
import io.github.dfa1.vortex.reader.decode.UnknownArrayNode;
1211
import org.junit.jupiter.api.Test;
1312

1413
import java.lang.foreign.Arena;
@@ -25,7 +24,7 @@ class ReadRegistryTest {
2524
void decodeUnknownEncodingThrowsByDefault() {
2625
// Given
2726
ReadRegistry sut = ReadRegistry.empty();
28-
ArrayNode node = new UnknownArrayNode("some.unknown",
27+
ArrayNode node = new ArrayNode(EncodingId.parse("some.unknown"),
2928
MemorySegment.ofArray(new byte[0]), new ArrayNode[0], new int[0]);
3029
DecodeContext ctx = new DecodeContext(node, DTypes.I32, 0L,
3130
new MemorySegment[0], sut, Arena.ofAuto());
@@ -40,7 +39,7 @@ void decodeUnknownEncodingThrowsByDefault() {
4039
void decodeKnownEncodingWithoutDecoderThrowsByDefault() {
4140
// Given — EncodingId is known but no decoder registered for it
4241
ReadRegistry sut = ReadRegistry.empty();
43-
ArrayNode node = ArrayNode.of(EncodingId.VORTEX_PRIMITIVE,
42+
ArrayNode node = new ArrayNode(EncodingId.VORTEX_PRIMITIVE,
4443
MemorySegment.ofArray(new byte[0]), new ArrayNode[0], new int[0]);
4544
DecodeContext ctx = new DecodeContext(node, DTypes.I32, 0L,
4645
new MemorySegment[0], sut, Arena.ofAuto());
@@ -55,7 +54,7 @@ void decodeKnownEncodingWithoutDecoderThrowsByDefault() {
5554
void decodeKnownEncodingWithoutDecoderReturnsUnknownArrayWhenAllowed() {
5655
// Given
5756
ReadRegistry sut = ReadRegistry.builder().allowUnknown().build();
58-
ArrayNode node = ArrayNode.of(EncodingId.VORTEX_PRIMITIVE,
57+
ArrayNode node = new ArrayNode(EncodingId.VORTEX_PRIMITIVE,
5958
MemorySegment.ofArray(new byte[0]), new ArrayNode[0], new int[0]);
6059
DecodeContext ctx = new DecodeContext(node, DTypes.I32, 0L,
6160
new MemorySegment[0], sut, Arena.ofAuto());
@@ -75,7 +74,7 @@ void decodeUnknownEncodingReturnsUnknownArrayWhenAllowed() {
7574
MemorySegment metadata = MemorySegment.ofArray(new byte[]{1, 2, 3});
7675
MemorySegment buf = Arena.ofAuto().allocate(4);
7776
buf.set(java.lang.foreign.ValueLayout.JAVA_INT, 0, 42);
78-
ArrayNode node = new UnknownArrayNode("some.unknown",
77+
ArrayNode node = new ArrayNode(EncodingId.parse("some.unknown"),
7978
metadata, new ArrayNode[0], new int[]{0});
8079
DecodeContext ctx = new DecodeContext(node, DTypes.I32, 5L,
8180
new MemorySegment[]{buf}, sut, Arena.ofAuto());
@@ -101,9 +100,9 @@ void decodeUnknownEncodingWrapsChildrenAsUnknown() {
101100
ReadRegistry sut = ReadRegistry.builder().allowUnknown().build();
102101
// Child uses a known id; allow-unknown still wraps it unknown because
103102
// its parent is unknown — mirrors Rust decode_foreign in vortex-array/src/serde.rs:380.
104-
ArrayNode child = ArrayNode.of(EncodingId.VORTEX_PRIMITIVE,
103+
ArrayNode child = new ArrayNode(EncodingId.VORTEX_PRIMITIVE,
105104
MemorySegment.ofArray(new byte[0]), new ArrayNode[0], new int[0]);
106-
ArrayNode parent = new UnknownArrayNode("some.unknown",
105+
ArrayNode parent = new ArrayNode(EncodingId.parse("some.unknown"),
107106
MemorySegment.ofArray(new byte[0]), new ArrayNode[]{child}, new int[0]);
108107
DecodeContext ctx = new DecodeContext(parent, DTypes.I32, 0L,
109108
new MemorySegment[0], sut, Arena.ofAuto());

0 commit comments

Comments
 (0)