From 91a1a14e77cad74e9287d73455106a91b947efd2 Mon Sep 17 00:00:00 2001 From: Ben Huddart Date: Tue, 28 Jul 2026 21:18:17 +0100 Subject: [PATCH] fix(figure): validate data_prefix instead of emitting unparseable GLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Figure(data_prefix=...)` was used verbatim to build sidecar filenames without any checking, so a prefix GLE cannot parse only failed much later, at compile time, with a message pointing at generated GLE rather than at the bad value: glp.figure(data_prefix="mk+white") # >> Error: left hand side contains unquoted string # on `data mk+white_0.dat d1=c1,c2` `Figure.__init__` now raises ValueError (naming the offending characters and their positions) so the failure surfaces in Python at the point the value was supplied. Validate rather than sanitize. The per-series `data_name=` path runs through `_sanitize_data_stem`, but the two arguments differ in kind: `data_name` takes a free-form *label* ("Observed Signal") whose mapping to a filename is an implementation detail, whereas `data_prefix` exists precisely so callers can *predict* sidecar names in batch pipelines. Silently rewriting `mk+white` to `mk_white` would trade a GLE parse error for a missing file even further downstream, in the caller's own tooling. `_sanitize_data_stem` also lowercases, so reusing it would have quietly turned the documented `data_prefix='experimentA'` into `experimenta_0.dat` — a regression in documented behaviour. The rejected set is drawn from GLE 4.3.10, not assumption: compiling a `data ` statement for every printable ASCII punctuation character shows only `!`, `"`, `+` and whitespace break its unquoted-filename tokenizer. `.`, `-`, `#`, `,`, `=`, `*` and non-ASCII all parse fine and stay allowed. `/` and `\` are additionally rejected on every platform as path separators, since a prefix names a filename stem, not a path. `Figure.from_dict` restores the prefix verbatim, bypassing validation: that is recorded state, not user input. The recognizer derives a prefix from filenames already in a parsed `.gle`, and a quoted `data "my file_0.dat"` legitimately yields `my file`; validating there would make such a figure impossible to reload and would break GUI undo/redo, which restores every snapshot through `from_dict`. Co-Authored-By: Claude Opus 5 --- docs/guides/CONFIGURATION.md | 21 ++++++++ docs/source/getting_started.rst | 6 +++ src/gleplot/__init__.py | 8 ++- src/gleplot/axes.py | 77 +++++++++++++++++++++++++++ src/gleplot/figure.py | 34 ++++++++++-- tests/unit/test_plotting.py | 94 +++++++++++++++++++++++++++++++++ 6 files changed, 234 insertions(+), 6 deletions(-) diff --git a/docs/guides/CONFIGURATION.md b/docs/guides/CONFIGURATION.md index 6241e6f..64575ac 100644 --- a/docs/guides/CONFIGURATION.md +++ b/docs/guides/CONFIGURATION.md @@ -282,6 +282,27 @@ Typical side files: For semantic per-series names, use the `data_name` keyword in generated-data methods such as `plot()` and `fill_between()`. +#### Allowed characters + +The prefix is used **verbatim**, so the sidecar names stay predictable — +`data_prefix='experimentA'` yields `experimentA_0.dat`, not `experimenta_0.dat`. +In exchange it is validated when the figure is created, and an unusable prefix +raises `ValueError` immediately instead of producing a `.gle` script that fails +at compile time: + + glp.figure(data_prefix='mk+white') + # ValueError: data_prefix 'mk+white' contains characters that cannot appear + # in a GLE data filename: '+' at index 2. ... + +Rejected: whitespace, control characters, and `!`, `"`, `+` (GLE cannot parse +these in the unquoted filename of a `data` statement), plus the path separators +`/` and `\`. An empty or whitespace-only prefix is rejected too; pass `None` for +the default `data_N.dat` naming. + +Most other punctuation is allowed, including `.`, `-`, `_` and `#`. Note that +`data_name` behaves differently: it takes a free-form *label* and sanitizes it +into a filename stem, because no caller depends on its exact spelling. + ## Resetting to Defaults Reset all global configurations to defaults: diff --git a/docs/source/getting_started.rst b/docs/source/getting_started.rst index 488b6e0..1d23346 100644 --- a/docs/source/getting_started.rst +++ b/docs/source/getting_started.rst @@ -133,6 +133,12 @@ This produces files such as: - ``experimentA_plot.gle`` - ``experimentA_0.dat`` +The prefix is used verbatim (note the preserved capital ``A``), so it must be +usable as a GLE data filename. Whitespace, control characters, ``!``, ``"``, +``+`` and the path separators ``/`` and ``\`` raise ``ValueError`` when the +figure is created, rather than producing a script that fails to compile. See +``docs/guides/CONFIGURATION.md`` for the full list. + New: Shared Axes in Subplots ---------------------------- diff --git a/src/gleplot/__init__.py b/src/gleplot/__init__.py index 349192c..98137a7 100644 --- a/src/gleplot/__init__.py +++ b/src/gleplot/__init__.py @@ -117,7 +117,9 @@ def figure( Marker configuration. If None, uses global default. data_prefix : str, optional Custom prefix for data file names (e.g., 'test9' creates 'test9_0.dat', 'test9_1.dat'). - If None, uses global counter with ``data_`` prefix. + If None, uses global counter with ``data_`` prefix. Used verbatim, so + it must be usable as a GLE data filename: whitespace, control + characters and any of ``! " + / \\`` raise ``ValueError``. Returns ------- @@ -263,7 +265,9 @@ def subplots( will show y-axis labels and ticks. Default: False data_prefix : str, optional Custom prefix for data file names (e.g., 'test9' creates 'test9_0.dat', 'test9_1.dat'). - If None, uses global counter with ``data_`` prefix. + If None, uses global counter with ``data_`` prefix. Used verbatim, so + it must be usable as a GLE data filename: whitespace, control + characters and any of ``! " + / \\`` raise ``ValueError``. Returns ------- diff --git a/src/gleplot/axes.py b/src/gleplot/axes.py index 3e09a3a..174e477 100644 --- a/src/gleplot/axes.py +++ b/src/gleplot/axes.py @@ -104,6 +104,83 @@ def _sanitize_data_stem(name: object) -> str: return text or "data" +#: Characters rejected in a figure-level ``data_prefix``. +#: +#: Two disjoint groups, both verified against GLE 4.3.10 rather than assumed: +#: +#: 1. ``!``, ``"``, ``+`` and any whitespace -- GLE's tokenizer cannot read +#: these inside the *unquoted* filename of a ``data `` statement. +#: ``data mk+white_0.dat`` aborts the compile with +#: ``>> Error: left hand side contains unquoted string``. A sweep of the +#: printable ASCII punctuation found exactly these; ``. - # $ % & ( ) [ ] { +#: } , ; : = @ ~ ^ * ? < > | \ ` '`` and non-ASCII all parse fine, so they +#: are deliberately *not* rejected. +#: 2. ``/`` and ``\\`` -- path separators. A ``data_prefix`` names a filename +#: stem, not a path; either one would redirect the sidecar into a +#: directory (or, on the platform where it is not a separator, produce a +#: filename that stops resolving when the script moves). Rejected on every +#: platform so a prefix means the same thing everywhere. +#: +#: C0 control characters (including NUL) are rejected as well -- see +#: :func:`_validate_data_prefix`. +_DATA_PREFIX_FORBIDDEN = '!"+/\\' + + +def _describe_char(ch: str) -> str: + """Render a character for an error message, naming invisible ones.""" + names = {" ": "space", "\t": "tab", "\n": "newline", "\r": "carriage return"} + if ch in names: + return names[ch] + if ch.isspace() or ord(ch) < 0x20 or ord(ch) == 0x7F: + return f"U+{ord(ch):04X}" + return repr(ch) + + +def _validate_data_prefix(prefix: object) -> str: + """Validate a figure-level ``data_prefix``, or raise ``ValueError``. + + Returns the prefix unchanged on success. Unlike :func:`_sanitize_data_stem` + -- which quietly rewrites the *label-like* ``data_name=`` argument into a + filename stem -- this rejects a bad prefix instead of repairing it. The + whole point of ``data_prefix`` is that the caller can predict the sidecar + filenames (batch pipelines glob for them), so silently renaming + ``mk+white`` to ``mk_white`` would trade a GLE compile error for a missing + file much further downstream. Sanitizing here would also lowercase, which + would break documented usage such as ``data_prefix='experimentA'``. + + See :data:`_DATA_PREFIX_FORBIDDEN` for what is rejected and why. + """ + if not isinstance(prefix, str): + raise TypeError( + f"data_prefix must be a str, got {type(prefix).__name__}: {prefix!r}" + ) + if not prefix or not prefix.strip(): + raise ValueError( + "data_prefix must be a non-empty filename stem; got " + f"{prefix!r}. Pass None to use the default 'data_N.dat' naming." + ) + + bad = [ + (idx, ch) + for idx, ch in enumerate(prefix) + if ch in _DATA_PREFIX_FORBIDDEN + or ch.isspace() + or ord(ch) < 0x20 + or ord(ch) == 0x7F + ] + if bad: + detail = ", ".join(f"{_describe_char(c)} at index {i}" for i, c in bad) + raise ValueError( + f"data_prefix {prefix!r} contains characters that cannot appear in " + f"a GLE data filename: {detail}. Forbidden: whitespace, control " + 'characters, and any of ! " + / \\ (GLE cannot parse the first ' + "four in an unquoted 'data' statement; the last two are path " + "separators). Most other punctuation, including . - _ and #, is " + "allowed." + ) + return prefix + + def _looks_numeric(token: str) -> bool: """True if ``token`` would parse as a float (int/float/exponent form). diff --git a/src/gleplot/figure.py b/src/gleplot/figure.py index 988c321..003b5e1 100644 --- a/src/gleplot/figure.py +++ b/src/gleplot/figure.py @@ -3,7 +3,7 @@ import numpy as np from pathlib import Path from typing import Tuple, Optional, Literal -from .axes import Axes +from .axes import Axes, _validate_data_prefix from .writer import GLEWriter from .compiler import GLECompiler, SUFFIX_TO_COMPILE_FORMAT from .colors import rgb_to_gle @@ -64,6 +64,20 @@ def __init__( data_prefix : str, optional Custom prefix for data file names (e.g., 'test9' creates 'test9_0.dat', 'test9_1.dat'). If None, uses global counter with ``data_`` prefix. + + Used verbatim -- it is never sanitized, so the sidecar names stay + predictable (``'experimentA'`` yields ``experimentA_0.dat``, not + ``experimenta_0.dat``). In exchange it is validated: a prefix + containing whitespace, a control character, or any of + ``! " + / \\`` raises ``ValueError`` here rather than producing a + script GLE fails to parse. See + :func:`gleplot.axes._validate_data_prefix`. + + Raises + ------ + ValueError + If ``data_prefix`` is empty/whitespace-only or contains a + character that is not usable in a GLE data filename. """ self.figsize = figsize self.dpi = dpi @@ -77,8 +91,12 @@ def __init__( self.sharex = sharex self.sharey = sharey - # Custom data file naming - self.data_prefix = data_prefix + # Custom data file naming. Validated (not sanitized) so a prefix GLE + # cannot parse fails here, at the point the bad value was supplied, + # instead of at compile time inside a generated 'data ' line. + self.data_prefix = ( + _validate_data_prefix(data_prefix) if data_prefix is not None else None + ) self._local_data_counter = 0 # Local counter when using custom prefix self._used_data_files: set[str] = set() self._subplot_adjust: dict[str, float] = {} @@ -1624,9 +1642,17 @@ def from_dict(cls, d: dict) -> "Figure": marker=marker, sharex=fig_block.get("sharex", False), sharey=fig_block.get("sharey", False), - data_prefix=fig_block.get("data_prefix"), ) + # Restore the prefix verbatim, bypassing __init__'s validation. This is + # recorded state, not a user-supplied value: the recognizer derives a + # prefix from filenames already present in a parsed .gle, and those can + # legitimately contain characters __init__ rejects (a quoted + # 'data "my file_0.dat"' yields the prefix 'my file'). Re-validating + # here would make such a figure impossible to round-trip -- and would + # break GUI undo/redo, which restores every snapshot through from_dict. + fig.data_prefix = fig_block.get("data_prefix") + fig._local_data_counter = fig_block.get("local_data_counter", 0) fig._used_data_files = set(fig_block.get("used_data_files", [])) fig._subplot_adjust = { diff --git a/tests/unit/test_plotting.py b/tests/unit/test_plotting.py index a432589..6e1e806 100644 --- a/tests/unit/test_plotting.py +++ b/tests/unit/test_plotting.py @@ -232,6 +232,100 @@ def test_data_name_collision_is_disambiguated(self): self.assertEqual(self.ax.lines[1]['data_file'], 'model-range_1.dat') +class TestDataPrefixValidation(unittest.TestCase): + """Test that a figure-level data_prefix is validated, not sanitized. + + Unlike ``data_name`` (a label, sanitized into a stem by the tests above), + ``data_prefix`` is used verbatim to build sidecar filenames. A prefix GLE + cannot parse must therefore fail in Python at the point it is supplied, + rather than producing a script that aborts at compile time with + ``>> Error: left hand side contains unquoted string``. + """ + + def tearDown(self): + glp.close() + + def test_plus_in_prefix_raises_instead_of_failing_at_compile_time(self): + """The reported reproducer: 'mk+white' produced unparseable GLE.""" + with self.assertRaises(ValueError) as ctx: + glp.figure(data_prefix='mk+white') + + message = str(ctx.exception) + self.assertIn('mk+white', message) # names the offending value + self.assertIn("'+'", message) # ...and the offending character + + def test_forbidden_characters_are_rejected(self): + """Every character GLE cannot read in an unquoted data filename.""" + # '!', '"' and '+' were confirmed against GLE 4.3.10 by compiling a + # data file containing each; '/' and '\\' are path separators. + for prefix in ['a!b', 'a"b', 'a+b', 'a/b', 'a\\b']: + with self.subTest(prefix=prefix): + with self.assertRaises(ValueError): + glp.figure(data_prefix=prefix) + + def test_whitespace_and_control_characters_are_rejected(self): + for prefix in ['a b', 'a\tb', 'a\nb', 'a\x00b', 'a\x1fb']: + with self.subTest(prefix=prefix): + with self.assertRaises(ValueError): + glp.figure(data_prefix=prefix) + + def test_empty_prefix_is_rejected(self): + for prefix in ['', ' ']: + with self.subTest(prefix=prefix): + with self.assertRaises(ValueError): + glp.figure(data_prefix=prefix) + + def test_non_string_prefix_raises_type_error(self): + with self.assertRaises(TypeError): + glp.figure(data_prefix=42) + + def test_none_prefix_keeps_default_naming(self): + """None is not a bad value -- it selects the global data_N counter.""" + fig = glp.figure(data_prefix=None) + self.assertIsNone(fig.data_prefix) + + def test_valid_prefix_is_preserved_verbatim(self): + """Case and legal punctuation must survive: no sanitizing here. + + Sanitizing would lowercase, silently turning the documented + ``data_prefix='experimentA'`` into ``experimenta_0.dat``. + """ + for prefix in ['experimentA', 'run42', 'exp-1.2', 'a#b', 'a,b', 'a=b']: + with self.subTest(prefix=prefix): + fig = glp.figure(data_prefix=prefix) + self.assertEqual(fig.data_prefix, prefix) + + def test_valid_prefix_drives_sidecar_filenames(self): + fig = glp.figure(data_prefix='experimentA') + ax = fig.add_subplot(111) + ax.plot([1, 2, 3], [1, 2, 3]) + ax.plot([1, 2, 3], [3, 2, 1]) + + self.assertEqual(ax.lines[0]['data_file'], 'experimentA_0.dat') + self.assertEqual(ax.lines[1]['data_file'], 'experimentA_1.dat') + + def test_subplots_validates_prefix_too(self): + with self.assertRaises(ValueError): + glp.subplots(1, 2, data_prefix='mk+white') + + def test_from_dict_restores_a_prefix_init_would_reject(self): + """Recorded state round-trips even when it would fail validation. + + The recognizer derives a prefix from filenames already present in a + parsed .gle, and a quoted ``data "my file_0.dat"`` yields the prefix + ``my file``. from_dict restores such a figure verbatim -- validating + there would make the figure impossible to reload and would break GUI + undo/redo, which restores every snapshot through from_dict. + """ + fig = glp.figure() + fig.add_subplot(111).plot([1, 2], [1, 2]) + fig.data_prefix = 'my file' + + restored = glp.Figure.from_dict(fig.to_dict()) + + self.assertEqual(restored.data_prefix, 'my file') + + class TestErrorBars(unittest.TestCase): """Test error bar functionality."""