From 3f7a58b417615d0c9ba814a74d66bc4fed89e522 Mon Sep 17 00:00:00 2001 From: DemchaAV Date: Sun, 14 Jun 2026 13:11:54 +0100 Subject: [PATCH 1/2] fix(api): fail loud on non-finite chart values and empty paths Two authoring-time validation gaps on the new 1.8.0 primitives, both caught before 1.8.0 ships: - ChartData.Series accepted NaN/Infinity, which poisoned NiceScale axis derivation (NaN != NaN defeats the degenerate-range guard) and only surfaced as a misleading 'height must be finite: NaN' thrown deep in ChartPrimitive. Reject non-finite values in the Series constructor, naming the series and the offending index; null stays a valid gap. - PathNode accepted a [MoveTo, Close] segment list (size 2, MoveTo-first) that renders an empty subpath, while its own error message promised a 'drawing segment'. Require at least one LineTo/CubicTo after the MoveTo so the contract the message advertises is actually enforced. Adds ChartDataTest (new) and a PathNodeTest case. Full suite green. --- CHANGELOG.md | 10 +++ .../compose/document/chart/ChartData.java | 15 ++++- .../compose/document/node/PathNode.java | 22 ++++++- .../compose/document/chart/ChartDataTest.java | 65 +++++++++++++++++++ .../compose/document/node/PathNodeTest.java | 9 +++ 5 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 src/test/java/com/demcha/compose/document/chart/ChartDataTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 5301b9d8d..1ff0b6b01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,16 @@ Entries land here as they merge. ### Public API +- **Stricter authoring-time validation on the new vector/chart primitives.** + `ChartData.Series` now rejects non-finite values (`NaN` / ±∞) at + construction — naming the series and the offending index — instead of + letting them poison axis derivation and surface as a misleading + "height must be finite" failure deep in the layout pass (`null` entries + are still allowed as gaps). `PathNode` now requires at least one drawing + segment (`LineTo` or `CubicTo`) after the leading `MoveTo`, so a + `[MoveTo, Close]` path that renders nothing is rejected up front rather + than slipping past the size check the error message already promised to + enforce. - **Block-level horizontal alignment** (`@since 1.8.0`). Fixed-size flow children (paths, images, SVG icons, barcodes, shape containers) left-align by default — there was no built-in way to centre or right-align one without diff --git a/src/main/java/com/demcha/compose/document/chart/ChartData.java b/src/main/java/com/demcha/compose/document/chart/ChartData.java index 6d441b10e..689785ab5 100644 --- a/src/main/java/com/demcha/compose/document/chart/ChartData.java +++ b/src/main/java/com/demcha/compose/document/chart/ChartData.java @@ -74,18 +74,31 @@ public int categoryCount() { /** * One named value series. {@code null} entries are allowed and mean a * missing point — a gap in a line, a skipped bar — distinct from {@code 0}. + * Non-finite entries ({@code NaN} / ±∞) are rejected at construction: they + * would otherwise poison axis derivation and only surface as a misleading + * "height must be finite" failure deep inside the layout pass. * * @param name legend label for this series * @param values value per category, aligned by index; entries may be null + * (a gap) but must otherwise be finite */ public record Series(String name, List values) { /** - * Normalizes the name and tolerates {@code null} value entries. + * Normalizes the name, tolerates {@code null} value entries (gaps), + * and rejects non-finite values with the offending index named. */ public Series { name = name == null ? "" : name; Objects.requireNonNull(values, "values"); values = java.util.Collections.unmodifiableList(new ArrayList<>(values)); + for (int i = 0; i < values.size(); i++) { + Double v = values.get(i); + if (v != null && !Double.isFinite(v)) { + throw new IllegalArgumentException( + "series '" + name + "' value at index " + i + + " must be finite or null, got " + v); + } + } } /** diff --git a/src/main/java/com/demcha/compose/document/node/PathNode.java b/src/main/java/com/demcha/compose/document/node/PathNode.java index 7cd8191b0..3006052b6 100644 --- a/src/main/java/com/demcha/compose/document/node/PathNode.java +++ b/src/main/java/com/demcha/compose/document/node/PathNode.java @@ -34,7 +34,8 @@ * @param width resolved box width * @param height resolved box height * @param segments normalized path segments; must start with a - * {@link DocumentPathSegment.MoveTo} + * {@link DocumentPathSegment.MoveTo} and contain at least + * one drawing segment (LineTo or CubicTo) * @param fillColor optional fill colour (non-zero winding rule) * @param fillPaint optional gradient fill; wins over {@code fillColor} * @param stroke optional outline stroke @@ -74,15 +75,30 @@ public record PathNode( name = name == null ? "" : name; Objects.requireNonNull(segments, "segments"); segments = List.copyOf(segments); - if (segments.size() < 2) { + if (segments.isEmpty()) { throw new IllegalArgumentException( - "path needs at least a MoveTo and one drawing segment: " + segments.size()); + "path needs at least a MoveTo and one drawing segment"); } if (!(segments.get(0) instanceof DocumentPathSegment.MoveTo)) { throw new IllegalArgumentException( "path must start with a MoveTo segment, found: " + segments.get(0).getClass().getSimpleName()); } + boolean hasDrawingSegment = false; + for (DocumentPathSegment segment : segments) { + if (segment instanceof DocumentPathSegment.LineTo + || segment instanceof DocumentPathSegment.CubicTo) { + hasDrawingSegment = true; + break; + } + } + if (!hasDrawingSegment) { + // A lone MoveTo, or a MoveTo plus only Close/MoveTo, draws nothing. + // The old size>=2 check let [MoveTo, Close] through while the + // message still promised a drawing segment — now it is enforced. + throw new IllegalArgumentException( + "path needs at least a MoveTo and one drawing segment (LineTo or CubicTo)"); + } padding = padding == null ? DocumentInsets.zero() : padding; margin = margin == null ? DocumentInsets.zero() : margin; dashPattern = dashPattern == null ? DocumentDashPattern.NONE : dashPattern; diff --git a/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java b/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java new file mode 100644 index 000000000..bc317efb0 --- /dev/null +++ b/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java @@ -0,0 +1,65 @@ +package com.demcha.compose.document.chart; + +import com.demcha.compose.document.chart.ChartData.Series; +import org.junit.jupiter.api.Test; + +import java.util.Arrays; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Value-level contract of {@link ChartData} and its {@link Series}: null + * entries are tolerated as gaps, but non-finite values are rejected at + * authoring time (rather than poisoning axis derivation and surfacing as a + * misleading "height must be finite" failure deep in the layout pass). + */ +class ChartDataTest { + + @Test + void nullValuesAreAllowedAsGaps() { + Series s = new Series("revenue", Arrays.asList(1.0, null, 3.0)); + + assertThat(s.values()).containsExactly(1.0, null, 3.0); + } + + @Test + void nanValueIsRejectedNamingTheSeriesAndIndex() { + assertThatThrownBy(() -> new Series("revenue", Arrays.asList(1.0, Double.NaN, 3.0))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("revenue") + .hasMessageContaining("index 1") + .hasMessageContaining("finite"); + } + + @Test + void infiniteValuesAreRejected() { + assertThatThrownBy(() -> Series.of("a", 1.0, Double.POSITIVE_INFINITY)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("finite"); + assertThatThrownBy(() -> Series.of("b", Double.NEGATIVE_INFINITY)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("finite"); + } + + @Test + void finiteSeriesBuildsCleanly() { + ChartData data = ChartData.builder() + .categories("Q1", "Q2") + .series("revenue", 10.0, 20.0) + .build(); + + assertThat(data.seriesCount()).isEqualTo(1); + assertThat(data.categoryCount()).isEqualTo(2); + assertThat(data.series().get(0).values()).containsExactly(10.0, 20.0); + } + + @Test + void raggedSeriesIsRejected() { + assertThatThrownBy(() -> new ChartData(List.of("Q1", "Q2"), + List.of(Series.of("revenue", 10.0)))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("values but there are"); + } +} diff --git a/src/test/java/com/demcha/compose/document/node/PathNodeTest.java b/src/test/java/com/demcha/compose/document/node/PathNodeTest.java index ef0d48083..340ba0419 100644 --- a/src/test/java/com/demcha/compose/document/node/PathNodeTest.java +++ b/src/test/java/com/demcha/compose/document/node/PathNodeTest.java @@ -41,6 +41,15 @@ void pathNeedsAtLeastOneDrawingSegment() { .hasMessageContaining("at least a MoveTo and one drawing segment"); } + @Test + void moveToFollowedOnlyByCloseDrawsNothingAndIsRejected() { + // [MoveTo, Close] is size 2 and starts with a MoveTo, so it slipped + // past the old size>=2 guard, yet it renders an empty subpath. + assertThatThrownBy(() -> node(List.of(moveTo(0.1, 0.1), close()))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("at least a MoveTo and one drawing segment"); + } + @Test void coordinatesMustBeFinite() { assertThatThrownBy(() -> cubicTo(Double.NaN, 0, 0, 0, 1, 1)) From 8f560db65d6543ceb8ba0de1c6c7bd064bbd4010 Mon Sep 17 00:00:00 2001 From: DemchaAV Date: Sun, 14 Jun 2026 13:57:00 +0100 Subject: [PATCH 2/2] review-fix: drop the over-strict PathNode guard (SVG-import regression) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of #187 found the PathNode 'require a drawing segment' guard is a false positive on the headline @Beta SVG-import path: SvgIcon.node() threw IllegalArgumentException for a visible-painted element that lowers to [MoveTo, Close] (e.g. d="M12 12 Z"), which rendered harmlessly on develop. Real exporters emit such stray subpaths, and the guard was only chasing a cosmetic 'message overpromises' nit — not worth a behavioural regression on a leaf primitive that the SVG reader feeds. Revert PathNode + PathNodeTest to develop verbatim; the PR now carries only the ChartData.Series non-finite rejection (correct and valuable). Also tighten ChartDataTest's 'index 1' substring to 'index 1 must be finite' so it cannot accidentally match 'index 10'. Full suite green (1357). The cleaner fix for the stray-subpath case (drop drawing-less layers in SvgIconReader, which also covers the pre-existing lone-MoveTo fragility) belongs in the separate SVG-hardening lane, not here. --- CHANGELOG.md | 15 +++++-------- .../compose/document/node/PathNode.java | 22 +++---------------- .../compose/document/chart/ChartDataTest.java | 3 +-- .../compose/document/node/PathNodeTest.java | 9 -------- 4 files changed, 9 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff0b6b01..0a314d99d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,16 +10,11 @@ Entries land here as they merge. ### Public API -- **Stricter authoring-time validation on the new vector/chart primitives.** - `ChartData.Series` now rejects non-finite values (`NaN` / ±∞) at - construction — naming the series and the offending index — instead of - letting them poison axis derivation and surface as a misleading - "height must be finite" failure deep in the layout pass (`null` entries - are still allowed as gaps). `PathNode` now requires at least one drawing - segment (`LineTo` or `CubicTo`) after the leading `MoveTo`, so a - `[MoveTo, Close]` path that renders nothing is rejected up front rather - than slipping past the size check the error message already promised to - enforce. +- **`ChartData.Series` rejects non-finite values.** A `NaN` / ±∞ entry now + fails at construction — naming the series and the offending index — + instead of poisoning axis derivation and surfacing as a misleading + "height must be finite" failure deep in the layout pass. `null` entries + are still allowed as gaps. - **Block-level horizontal alignment** (`@since 1.8.0`). Fixed-size flow children (paths, images, SVG icons, barcodes, shape containers) left-align by default — there was no built-in way to centre or right-align one without diff --git a/src/main/java/com/demcha/compose/document/node/PathNode.java b/src/main/java/com/demcha/compose/document/node/PathNode.java index 3006052b6..7cd8191b0 100644 --- a/src/main/java/com/demcha/compose/document/node/PathNode.java +++ b/src/main/java/com/demcha/compose/document/node/PathNode.java @@ -34,8 +34,7 @@ * @param width resolved box width * @param height resolved box height * @param segments normalized path segments; must start with a - * {@link DocumentPathSegment.MoveTo} and contain at least - * one drawing segment (LineTo or CubicTo) + * {@link DocumentPathSegment.MoveTo} * @param fillColor optional fill colour (non-zero winding rule) * @param fillPaint optional gradient fill; wins over {@code fillColor} * @param stroke optional outline stroke @@ -75,30 +74,15 @@ public record PathNode( name = name == null ? "" : name; Objects.requireNonNull(segments, "segments"); segments = List.copyOf(segments); - if (segments.isEmpty()) { + if (segments.size() < 2) { throw new IllegalArgumentException( - "path needs at least a MoveTo and one drawing segment"); + "path needs at least a MoveTo and one drawing segment: " + segments.size()); } if (!(segments.get(0) instanceof DocumentPathSegment.MoveTo)) { throw new IllegalArgumentException( "path must start with a MoveTo segment, found: " + segments.get(0).getClass().getSimpleName()); } - boolean hasDrawingSegment = false; - for (DocumentPathSegment segment : segments) { - if (segment instanceof DocumentPathSegment.LineTo - || segment instanceof DocumentPathSegment.CubicTo) { - hasDrawingSegment = true; - break; - } - } - if (!hasDrawingSegment) { - // A lone MoveTo, or a MoveTo plus only Close/MoveTo, draws nothing. - // The old size>=2 check let [MoveTo, Close] through while the - // message still promised a drawing segment — now it is enforced. - throw new IllegalArgumentException( - "path needs at least a MoveTo and one drawing segment (LineTo or CubicTo)"); - } padding = padding == null ? DocumentInsets.zero() : padding; margin = margin == null ? DocumentInsets.zero() : margin; dashPattern = dashPattern == null ? DocumentDashPattern.NONE : dashPattern; diff --git a/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java b/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java index bc317efb0..69f1c0997 100644 --- a/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java +++ b/src/test/java/com/demcha/compose/document/chart/ChartDataTest.java @@ -29,8 +29,7 @@ void nanValueIsRejectedNamingTheSeriesAndIndex() { assertThatThrownBy(() -> new Series("revenue", Arrays.asList(1.0, Double.NaN, 3.0))) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("revenue") - .hasMessageContaining("index 1") - .hasMessageContaining("finite"); + .hasMessageContaining("index 1 must be finite"); } @Test diff --git a/src/test/java/com/demcha/compose/document/node/PathNodeTest.java b/src/test/java/com/demcha/compose/document/node/PathNodeTest.java index 340ba0419..ef0d48083 100644 --- a/src/test/java/com/demcha/compose/document/node/PathNodeTest.java +++ b/src/test/java/com/demcha/compose/document/node/PathNodeTest.java @@ -41,15 +41,6 @@ void pathNeedsAtLeastOneDrawingSegment() { .hasMessageContaining("at least a MoveTo and one drawing segment"); } - @Test - void moveToFollowedOnlyByCloseDrawsNothingAndIsRejected() { - // [MoveTo, Close] is size 2 and starts with a MoveTo, so it slipped - // past the old size>=2 guard, yet it renders an empty subpath. - assertThatThrownBy(() -> node(List.of(moveTo(0.1, 0.1), close()))) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("at least a MoveTo and one drawing segment"); - } - @Test void coordinatesMustBeFinite() { assertThatThrownBy(() -> cubicTo(Double.NaN, 0, 0, 0, 1, 1))