fix(api): reject non-finite chart values#187
Merged
Merged
Conversation
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.
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.
DemchaAV
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ChartData.SeriesacceptedNaN/Infinity. A non-finite value poisonedNiceScaleaxis derivation (NaN != NaNdefeats the degenerate-range guard) and only surfaced as a misleading"height must be finite: NaN"thrown deep insideChartPrimitive— far from the bad data. (Senior-review audit follow-up; caught before 1.8.0 ships.)What
ChartData.Seriesrejects non-finite values at construction, naming the series and offending index;nullstays a valid gap. Verified: thenullguard runs before auto-unboxing (no NPE), and the check sits beforeChartData's ragged-length check so aNaNis reported as the NaN error.ChartDataTest(5 cases: null-gap allowed, NaN rejected naming the index, ±∞ rejected, finite builds, ragged rejected).Review note
This PR originally also tightened
PathNodeto require a drawing segment. Independent review found that change was a false positive on the@BetaSVG-import path (SvgIcon.node()threw on a[MoveTo, Close]element that rendered harmlessly ondevelop). It was chasing a cosmetic message nit, so it was reverted —PathNodeis byte-identical todevelop. The proper stray-subpath fix (drop drawing-less layers inSvgIconReader) is deferred to the SVG-hardening lane.Tests
Full suite 1357 green.
Lane: canonical (
document.chart). Additive validation only.