From a50d02170d6d324066a2d3e2f372c168804c6220 Mon Sep 17 00:00:00 2001 From: Ion Reguera Date: Tue, 28 Jul 2026 14:07:35 +0200 Subject: [PATCH] fix(parsers): stop coercing free-text fields; rc11 str_to_value() ran every scalar through int -> float -> strtobool -> Uuid regardless of its key, so free text was silently rewritten on save. Since strtobool accepts the truth abbreviations, a cue named n/N/f/F was persisted as False, y/Y/t/T as True, and any bare digit as an int -- 18 of the 62 alphanumeric single characters, plus yes/no/true/false/on/off. Reported as "can't name a cue with a single letter" (ClickUp 869cqbpxa). There was never a length rule: NameStringType has always been minLength=1, and the frontend has no validator. The failing characters were exactly strtobool's vocabulary, which is why searching for a min-length check found nothing. Separately, a cue named lowercase none/null hit the ['none','null',''] -> None branch, serialised to , and failed NameStringType's minLength=1 -- a hard XMLSchemaValidationError at save rather than silent corruption. The new short-circuit precedes that branch, fixing both. Adds STRING_TYPED_KEYS and an optional key argument to str_to_value, threaded through all four call sites. Keeping key optional leaves existing single-argument callers unaffected. Only name, description and file_name are reachable today; output_name, parameter_name, icon and color are currently shielded by unrelated bypasses in outputsParser, _normalize_fade_parameters and the GenericDict fallback. They are listed anyway so fixing those bypasses cannot silently reintroduce this bug. 'id' is deliberately excluded -- the Uuid() branch is the only thing producing Uuid objects on parse. Closes ClickUp 869cqbpxa. --- CHANGELOG.md | 18 ++++ src/cuemsutils/__init__.py | 2 +- src/cuemsutils/xml/Parsers.py | 47 ++++++++- tests/test_name_coercion.py | 184 ++++++++++++++++++++++++++++++++++ 4 files changed, 245 insertions(+), 6 deletions(-) create mode 100644 tests/test_name_coercion.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 018dc77..4a1ebb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ # Changelog +## 0.1.0rc11 — 2026-07-28 + +Free-text fields are no longer type-coerced during parsing (closes ClickUp 869cqbpxa). + +### Fixed +- `CuemsParser.str_to_value` no longer coerces values whose key names a string-typed field. It previously ran every scalar through `int` → `float` → `strtobool` → `Uuid` regardless of key, so free text was silently rewritten. Because `strtobool` accepts the truth abbreviations, a cue named `n`/`N`/`f`/`F` was persisted as `False`, `y`/`Y`/`t`/`T` as `True`, and any bare digit as an `int` — 18 of the 62 alphanumeric single characters were corrupted, along with the words `yes`/`no`/`true`/`false`/`on`/`off`. Sergio reported the symptom as "can't name a cue with a single letter"; there was never a length rule, the failing characters were exactly `strtobool`'s vocabulary. The affected keys reachable in practice are `name`, `description` and `file_name`. +- A cue named lowercase `none` or `null` hit the `['none', 'null', '']` → `None` branch, serialised to ``, and failed `NameStringType`'s `minLength=1` — a hard `XMLSchemaValidationError` at save time rather than silent corruption. The new short-circuit precedes that branch, so both failure modes are fixed together. + +### Added +- `STRING_TYPED_KEYS` in `xml/Parsers.py` — the set of keys exempt from coercion. `name`, `description` and `file_name` are the ones reachable today; `output_name`, `parameter_name`, `icon`, `color` and `unix_name` are defensive entries, currently shielded by unrelated bypasses in `outputsParser`, `_normalize_fade_parameters` and the `GenericDict` fallback, listed so that fixing any of those bypasses cannot silently reintroduce this bug. +- `str_to_value` takes an optional `key` argument, threaded through all four call sites (`CuemsScriptParser`, `CueListParser`, `GenericParser`, `fade_profileParser`). The argument is optional, so existing single-argument callers are unaffected. +- `tests/test_name_coercion.py` — exhaustive sweep over all 62 alphanumeric single characters for each reachable key, the boolean/nullish word set, a full XML round-trip, and negative tests pinning that `enabled`/`autoload`/`timecode`/`loop` still coerce and that `id` still parses to a `Uuid`. + +### Notes +- `id` is deliberately **not** in the allowlist: the `Uuid()` branch inside `str_to_value` is the only thing that produces `Uuid` objects on parse (the parsers assign via raw `dict.__setitem__` and never reach the property setters), so adding it would downgrade every cue, script and media id to a plain `str`. The consequence is that `DmxSceneType.id` (`script.xsd:403`, declared `xs:string`) cannot be protected by this mechanism — accepted, since DMX scene ids are system-assigned rather than operator-typed. +- Projects saved before this release have the corrupted name baked into their `cue_script.xml`; the original text is unrecoverable (`n`, `no`, `N`, `off` all collapse to `False`) and must be renamed by hand. +- `cuems-nodeconf` calls the inherited `str_to_value` without a key (`NodeXmlBuilders.py:80`), so node names remain exposed to the identical bug. Tracked separately. + ## 0.1.0rc8 — 2026-05-20 Production call-site migration to the `CTimecode` v2 API, removal of a long-deprecated method, and a new required settings field for `gradient-motiond` integration. diff --git a/src/cuemsutils/__init__.py b/src/cuemsutils/__init__.py index 34b69c1..b041f11 100644 --- a/src/cuemsutils/__init__.py +++ b/src/cuemsutils/__init__.py @@ -1,4 +1,4 @@ # SPDX-FileCopyrightText: 2025-present Adrià (StageLab) # # SPDX-License-Identifier: GPL-3.0 -__version__ = "0.1.0rc10" +__version__ = "0.1.0rc11" diff --git a/src/cuemsutils/xml/Parsers.py b/src/cuemsutils/xml/Parsers.py index 82a87ee..f55a97a 100644 --- a/src/cuemsutils/xml/Parsers.py +++ b/src/cuemsutils/xml/Parsers.py @@ -13,6 +13,28 @@ #TODO: XML_ROOT_TAG get from constants storage XML_ROOT_TAG = 'CuemsScript' +# Keys that must never be type-coerced by str_to_value(). Without this, a cue named +# "n" is saved as False, one named "1" as int 1, and one named "none" becomes None -> +# -> XSD minLength violation, i.e. a hard save error. See ClickUp 869cqbpxa. +# +# 'name', 'description' and 'file_name' are the keys actually reachable today. The +# rest are defensive only: they are currently shielded by bypasses in outputsParser +# (builds output objects directly), _normalize_fade_parameters (diverts 'parameters' +# before the scalar branch) and the GenericDict fallback in GenericParser.parse() +# (get_class('ui_properties') misses because the class is UI_properties). They are +# listed so that fixing any of those bypasses cannot silently reintroduce this bug. +# +# 'id' is deliberately ABSENT: the Uuid() branch in str_to_value is the only thing +# that produces Uuid objects on parse (parsers assign via raw dict.__setitem__ and so +# never hit the property setters). Adding 'id' here would downgrade every cue, script +# and media id to a plain str. +STRING_TYPED_KEYS = frozenset({ + # reachable today + 'name', 'description', 'file_name', + # defensive -- see above + 'output_name', 'parameter_name', 'icon', 'color', 'unix_name', +}) + class GenericDict(dict): pass @@ -56,7 +78,22 @@ def get_first_key(self, _dict): def get_contained_dict(self, _dict): return list(_dict.values())[0] - def str_to_value(self, _string): + def str_to_value(self, _string, key = None): + """Decode a string-encoded scalar into its Python type. + + Args: + _string: The value to decode. Non-str values pass through unchanged. + key: The dict key ``_string`` was stored under, when known. Values + whose key is in :data:`STRING_TYPED_KEYS` are returned verbatim + so free-text fields are never coerced (ClickUp 869cqbpxa). + + Returns: + The decoded value, or ``_string`` unchanged for string-typed keys. + """ + # Must precede every coercion branch below, including the none/null one: + # a cue legitimately named "none" would otherwise become None. + if key in STRING_TYPED_KEYS: + return _string if not isinstance(_string, str): return _string if _string in ['none', 'null', '']: @@ -94,7 +131,7 @@ def parse(self): parser_class, class_string = self.get_parser_class(k) self.item_csp[k] = parser_class(init_dict=v, class_string=class_string).parse() else: - v = self.str_to_value(v) + v = self.str_to_value(v, key = k) self.item_csp[k] = v return self.item_csp @@ -128,7 +165,7 @@ def parse(self): self.item_clp[k] = value_parser_class(init_dict=v, class_string=value_class_string).parse() else: - v = self.str_to_value(v) + v = self.str_to_value(v, key = k) self.item_clp[k] = v return self.item_clp @@ -177,7 +214,7 @@ def parse(self): else: self.item_gp[dict_key] = local_list else: - dict_value = self.str_to_value(dict_value) + dict_value = self.str_to_value(dict_value, key = dict_key) self.item_gp[dict_key] = dict_value return self.item_gp @@ -360,7 +397,7 @@ def parse(self): pcls(init_dict=li, class_string=pstr).parse() for li in dict_value ] else: - d[dict_key] = self.str_to_value(dict_value) + d[dict_key] = self.str_to_value(dict_value, key = dict_key) return FadeProfile(d) diff --git a/tests/test_name_coercion.py b/tests/test_name_coercion.py new file mode 100644 index 0000000..ea2e09c --- /dev/null +++ b/tests/test_name_coercion.py @@ -0,0 +1,184 @@ +"""Free-text fields must survive parsing verbatim. + +Regression tests for ClickUp 869cqbpxa: ``CuemsParser.str_to_value`` used to +coerce every scalar regardless of its key, so a cue named ``n`` was saved as +``False``, one named ``1`` as int ``1``, and one named ``none`` collapsed to +``None`` -> ```` -> a hard XSD ``minLength`` failure on save. +""" + +import string +from datetime import datetime, timezone +from pathlib import Path + +import pytest + +from cuemsutils.cues import AudioCue, CueList, CuemsScript +from cuemsutils.cues.MediaCue import Media, Region +from cuemsutils.tools.Uuid import Uuid +from cuemsutils.xml import XmlReaderWriter +from cuemsutils.xml.Parsers import STRING_TYPED_KEYS, CuemsParser + +TMP_DIR = Path(__file__).parent / "tmp" +TMP_DIR.mkdir(exist_ok=True) + +# The strtobool truth abbreviations -- the single characters that used to break. +TRUTHY_TOKENS = ["y", "Y", "t", "T", "yes", "true", "on"] +FALSY_TOKENS = ["n", "N", "f", "F", "no", "false", "off"] +# Hit the ['none', 'null', ''] -> None branch, which raised on save. +NULLISH_TOKENS = ["none", "null"] + +UUID_STR = "1f301cf8-dd03-4b40-ac17-ef0e5e7988be" + + +def _str_to_value(value, key=None): + """Call the parser helper without needing a constructed parser.""" + return CuemsParser.str_to_value(None, value, key=key) + + +def _audio_cue(name="placeholder"): + cue = AudioCue({ + "Media": Media({ + "file_name": "f.wav", + "id": "", + "duration": "00:00:00.000", + "regions": [ + Region({"id": 0, "loop": 1, "in_time": None, "out_time": None}) + ], + }), + "ui_properties": {"warning": None}, + }) + cue.name = name + return cue + + +def _script(cue_name="placeholder"): + cuelist = CueList({"contents": [_audio_cue(cue_name)]}) + cuelist.name = "main" + script = CuemsScript({"CueList": cuelist}) + script.name = "proj" + # Dates required by the CuemsScript schema assertion (modified >= created). + now = datetime.now(timezone.utc).isoformat() + script.created = now + script.modified = now + return script + + +def _roundtrip_cue_name(name, tmp_name): + """Write a script whose cue is named ``name``, read it back, return the name.""" + path = str(TMP_DIR / tmp_name) + writer = XmlReaderWriter(schema_name="script", xmlfile=path) + writer.write_from_object(_script(name)) + assert writer.validate() is None + loaded = XmlReaderWriter(schema_name="script", xmlfile=path).read_to_objects() + return loaded.cuelist.contents[0].name + + +# --------------------------------------------------------------------------- +# str_to_value -- string-typed keys are never coerced +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("char", list(string.ascii_letters + string.digits)) +@pytest.mark.parametrize("key", ["name", "description", "file_name"]) +def test_single_characters_survive_for_string_typed_keys(char, key): + """All 62 alphanumeric single characters must pass through untouched. + + 18 of them (y/Y/t/T/n/N/f/F and the ten digits) used to be corrupted. + """ + result = _str_to_value(char, key=key) + assert result == char + assert isinstance(result, str) + + +@pytest.mark.parametrize("token", TRUTHY_TOKENS + FALSY_TOKENS + NULLISH_TOKENS) +def test_boolean_and_nullish_words_survive_as_names(token): + result = _str_to_value(token, key="name") + assert result == token + assert isinstance(result, str) + + +def test_every_string_typed_key_is_protected(): + """Guards the allowlist itself, including the defensive entries.""" + for key in STRING_TYPED_KEYS: + assert _str_to_value("n", key=key) == "n" + assert _str_to_value("1", key=key) == "1" + assert _str_to_value("none", key=key) == "none" + + +# --------------------------------------------------------------------------- +# ...but coercion still happens everywhere it must +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("key", ["enabled", "autoload", "timecode", "loop"]) +@pytest.mark.parametrize( + "value,expected", + [("true", True), ("false", False), ("True", True), ("False", False)], +) +def test_boolean_keys_still_coerce(key, value, expected): + assert _str_to_value(value, key=key) is expected + + +def test_numeric_and_nullish_keys_still_coerce(): + assert _str_to_value("1", key="loop") == 1 + assert _str_to_value("42", key="master_vol") == 42 + assert _str_to_value("none", key="target") is None + assert _str_to_value("", key="target") is None + + +def test_id_is_not_allowlisted_and_still_becomes_a_uuid(): + """'id' must stay coercible -- the Uuid() branch is the only thing that + produces Uuid objects on parse.""" + assert "id" not in STRING_TYPED_KEYS + assert isinstance(_str_to_value(UUID_STR, key="id"), Uuid) + + +def test_unkeyed_calls_are_unchanged(): + """The key argument is optional; existing callers pass one positional arg.""" + assert _str_to_value("n") is False + assert _str_to_value("1") == 1 + assert _str_to_value("none") is None + + +# --------------------------------------------------------------------------- +# Full XML roundtrip through the editor's save path +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("name", ["a", "n", "y", "t", "f", "N", "1", "0", "no", "on"]) +def test_cue_name_survives_xml_roundtrip(name): + assert _roundtrip_cue_name(name, f"test_name_coercion_{name}.xml") == name + + +@pytest.mark.parametrize("name", NULLISH_TOKENS) +def test_nullish_cue_name_no_longer_fails_validation(name): + """These used to raise XMLSchemaValidationError (minLength=1) on save.""" + assert _roundtrip_cue_name(name, f"test_name_coercion_null_{name}.xml") == name + + +def test_parser_preserves_name_through_the_editor_save_path(): + """CuemsParser is what CuemsDBProject.update() runs on the frontend payload.""" + parsed = CuemsParser({"AudioCue": {"name": "n", "description": "off"}}).parse() + assert parsed["name"] == "n" + assert parsed["description"] == "off" + + +# --------------------------------------------------------------------------- +# output_name: currently shielded by outputsParser, but assert the consumer +# that would break first if that bypass is ever removed (plan section 4b/5). +# --------------------------------------------------------------------------- + +def test_get_all_output_names_handles_numeric_output_name(): + """MediaCue.get_all_output_names slices output_name -- an int would raise + TypeError: 'int' object is not subscriptable.""" + parsed = CuemsParser({ + "AudioCue": { + "name": "cue", + "outputs": { + "AudioCueOutput": [ + {"output_name": "1", "output_vol": "80", "channels": {}} + ] + }, + } + }).parse() + # Returns (node_id, output_id) tuples split at the UUID boundary. + names = parsed.get_all_output_names() + assert names == [("1", "")] + assert all(isinstance(part, str) for pair in names for part in pair)