From c4927e82a36a8246912788f7e3b10bcfea2714fa Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:33:23 -0400 Subject: [PATCH] fix(config): normalize nullable arrays and objects instead of refusing them (#642) * fix(config): normalize nullable arrays and objects instead of refusing them `element_style._nullable` widens every `customization.modes.` override with 'null' so a blank means "inherit the base", which turns a colour declared "array" into ["array", "null"]. `normalize_config_values` only knew how to convert null/integer/number/boolean out of a union, so a valid [0, 249, 0] matched nothing and logged Could not normalize field customization.modes.upcoming.odds_text.text_color: value=[0, 249, 0], type=, schema_type=['array', 'null'] The warning was the harmless half. It `continue`d past the single-type handling below, where `prop_type == 'array'` coerces items, so a nullable array never had its items normalized while a plain one did. A form posts numbers as strings, so ["0", "249", "0"] survived to the validator and was rejected with "Expected type integer, got str" -- setting a per-mode colour in the web UI failed outright. Every per-mode override of a structural or string type was exposed, across all eight scoreboard plugins, not only colours. Re-enter the single-type handling with the matched member rather than bailing, accept a string that matches, and warn only on a genuine mismatch so the diagnostic still reaches the validator. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ET8e5weDrb5Ju5QTKLU7zh * fix(config): convert only integral numbers for integer array items Review catch on the previous commit. Routing nullable arrays into the shared item handling made its integer coercion reachable for them, and that coercion called int(v) on any number: a client sending [2.5, 249, 0] for an RGB array got 2 stored and a 200 back, so a wrong value was silently corrected into a valid-looking one rather than refused. Convert only genuinely integral values, at both the union-item and the plain 'array' item branch so the two cannot drift. A whole float -- 2.0, which is all JSON can express for an integer -- still converts. This also settles an inconsistency that predates the change: int('2.5') raises, so the string form was always preserved and rejected while the numeric form was truncated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ET8e5weDrb5Ju5QTKLU7zh --------- Co-authored-by: Claude Opus 5 (1M context) --- ...est_normalize_nullable_structural_types.py | 164 ++++++++++++++++++ web_interface/blueprints/api_v3/plugins.py | 44 ++++- 2 files changed, 202 insertions(+), 6 deletions(-) create mode 100644 test/web_interface/test_normalize_nullable_structural_types.py diff --git a/test/web_interface/test_normalize_nullable_structural_types.py b/test/web_interface/test_normalize_nullable_structural_types.py new file mode 100644 index 00000000..9649b5be --- /dev/null +++ b/test/web_interface/test_normalize_nullable_structural_types.py @@ -0,0 +1,164 @@ +"""Tests for the union-type branch of normalize_config_values, which the +plugin-config save path (_prepare_plugin_config_for_save in api_v3/plugins.py) +runs before validation. + +``customization.modes.`` overrides are generated by +``element_style._nullable``, which widens every field's declared type with +``'null'`` so a blank means "inherit the base". That turns a colour declared +``"array"`` into ``["array", "null"]``. The union branch only knew how to +convert null/integer/number/boolean, so a valid ``[0, 249, 0]`` matched nothing: +it logged "Could not normalize field ..." and, because it then skipped the +single-type handling, never normalized the array's items either. +""" + +import logging + +import pytest + +from web_interface.blueprints.api_v3.plugins import ( + _prepare_plugin_config_for_save, +) + + +class _SchemaMgr: + """Minimal stand-in for the real schema manager. + + Defaults are empty and validation always passes, so these tests isolate the + normalization pass rather than re-testing the validator. + """ + + def generate_default_config(self, plugin_id, use_cache=True): + return {} + + def validate_config_against_schema(self, config, schema, plugin_id): + return True, [] + + +def _rgb_prop(nullable): + """An RGB colour field as the base schema (plain) or a mode override.""" + prop = {"type": "array", "items": {"type": "integer"}, + "minItems": 3, "maxItems": 3} + if nullable: + prop = dict(prop, type=["array", "null"], default=None) + return prop + + +def _schema(nullable): + """A scoreboard-shaped schema: customization.modes.upcoming.odds_text.""" + return { + "type": "object", + "properties": { + "enabled": {"type": "boolean"}, + "customization": { + "type": "object", + "properties": { + "modes": { + "type": "object", + "properties": { + "upcoming": { + "type": "object", + "properties": { + "odds_text": { + "type": "object", + "properties": { + "text_color": _rgb_prop(nullable), + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + +def _save(config, nullable=True): + regular, _secrets, error = _prepare_plugin_config_for_save( + "baseball-scoreboard", config, _schema(nullable), _SchemaMgr(), + is_json=True, + ) + assert error is None + return regular + + +def _colour(config): + return (config["customization"]["modes"]["upcoming"] + ["odds_text"]["text_color"]) + + +def _submit(value): + return {"enabled": True, + "customization": {"modes": {"upcoming": { + "odds_text": {"text_color": value}}}}} + + +class TestNullableArrayOverride: + """['array', 'null'] holding a real list is valid, not a failure.""" + + def test_valid_rgb_survives_the_save(self) -> None: + """The exact value from the bug report is stored unchanged.""" + assert _colour(_save(_submit([0, 249, 0]))) == [0, 249, 0] + + def test_valid_rgb_does_not_warn(self, caplog) -> None: + """No "Could not normalize" for a value that already matches.""" + with caplog.at_level(logging.WARNING): + _save(_submit([0, 249, 0])) + assert "Could not normalize field" not in caplog.text + + def test_items_are_normalized_like_a_plain_array(self) -> None: + """The real defect: form data posts strings, and a nullable array used + to keep them while a plain 'array' turned them into ints.""" + nullable = _colour(_save(_submit(["0", "249", "0"]))) + plain = _colour(_save(_submit(["0", "249", "0"]), nullable=False)) + assert nullable == [0, 249, 0] + assert nullable == plain + + def test_null_still_means_inherit(self) -> None: + """Widening the type must not break the blank-means-inherit case.""" + assert _colour(_save(_submit(None))) is None + + @pytest.mark.parametrize("blank", ["", " ", "null", "None"]) + def test_blank_strings_still_become_null(self, blank) -> None: + """A cleared form field arrives as a string and still means inherit.""" + assert _colour(_save(_submit(blank))) is None + + +class TestFractionalItemsReachTheValidator: + """int(2.5) == 2 would store a corrected number where the client sent a + wrong one, so the save looks successful and the config is quietly wrong. + Routing nullable arrays into the shared item handling made this reachable + for them too, so both paths convert only genuinely integral values.""" + + @pytest.mark.parametrize("nullable", [True, False]) + def test_a_fractional_item_is_not_truncated(self, nullable) -> None: + """2.5 reaches the validator intact, which then refuses it for an + integer item. Storing 2 would make the save look successful.""" + assert _colour(_save(_submit([2.5, 249, 0]), + nullable=nullable)) == [2.5, 249, 0] + + @pytest.mark.parametrize("nullable", [True, False]) + def test_a_whole_float_still_converts(self, nullable) -> None: + """JSON has no int/float distinction, so 2.0 is a legitimate integer.""" + assert _colour(_save(_submit([2.0, 249.0, 0.0]), + nullable=nullable)) == [2, 249, 0] + + @pytest.mark.parametrize("nullable", [True, False]) + def test_a_fractional_string_is_preserved_too(self, nullable) -> None: + """int('2.5') raises, so this path always preserved the value -- + pinned so the numeric and string cases cannot drift apart again.""" + assert _colour(_save(_submit(["2.5", "249", "0"]), + nullable=nullable)) == ["2.5", 249, 0] + + +class TestUnionFallbackStillReports: + """A union the value genuinely does not match must still warn.""" + + def test_unmatched_value_warns_and_passes_through(self, caplog) -> None: + """An object where ['array', 'null'] is declared is a real mismatch, + so it keeps reaching the validator with the diagnostic intact.""" + with caplog.at_level(logging.WARNING): + saved = _save(_submit({"r": 0})) + assert "Could not normalize field" in caplog.text + assert _colour(saved) == {"r": 0} diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index a5ebefbf..6fce4411 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -2080,10 +2080,29 @@ def _prepare_plugin_config_for_save(plugin_id, plugin_config, schema, schema_mgr normalized[key] = value.strip().lower() in ('true', '1', 'on', 'yes') continue - # Nothing converted: keep the value for validation to report. - logger.warning(f"Could not normalize field {field_path}: value={repr(value)}, type={type(value)}, schema_type={prop_type}") - normalized[key] = value - continue + # The scalar conversions above are the only ones this branch + # knows, so a union naming a structural or string type fell + # through here even when the value already matched it -- + # customization.modes. makes every override nullable + # (see element_style._nullable), so every per-mode colour is + # ['array', 'null'] and warned on a perfectly valid [r, g, b]. + # Worse than the noise: `continue` skipped the single-type + # handling below, so a nullable array never had its items + # normalized and form-posted ["0", "249", "0"] stayed strings + # where a plain 'array' field would have become ints. Re-enter + # that handling with the matched member instead. + if isinstance(value, list) and 'array' in prop_type: + prop_type = 'array' + elif isinstance(value, dict) and 'object' in prop_type: + prop_type = 'object' + elif isinstance(value, str) and 'string' in prop_type: + normalized[key] = value + continue + else: + # Nothing converted: keep the value for validation to report. + logger.warning(f"Could not normalize field {field_path}: value={repr(value)}, type={type(value)}, schema_type={prop_type}") + normalized[key] = value + continue if isinstance(value, dict) and prop_type == 'object' and 'properties' in prop_schema: # Recursively normalize nested objects @@ -2112,7 +2131,16 @@ def _prepare_plugin_config_for_save(plugin_id, plugin_config, schema, schema_mgr except (ValueError, TypeError, OverflowError): pass elif isinstance(v, (int, float)): - normalized_array.append(int(v)) + # Only a genuinely integral value converts. + # int(2.5) == 2 would store a silently + # corrected number where the client sent a + # wrong one; leaving it lets the validator + # reject it. A whole float (2.0 out of JSON) + # is integral and still converts. + if isinstance(v, int) or float(v).is_integer(): + normalized_array.append(int(v)) + else: + normalized_array.append(v) continue elif 'number' in item_type: if isinstance(v, str): @@ -2138,7 +2166,11 @@ def _prepare_plugin_config_for_save(plugin_id, plugin_config, schema, schema_mgr except (ValueError, TypeError, OverflowError): normalized_array.append(v) elif isinstance(v, (int, float)): - normalized_array.append(int(v)) + # Integral only -- see the union branch above. + if isinstance(v, int) or float(v).is_integer(): + normalized_array.append(int(v)) + else: + normalized_array.append(v) else: normalized_array.append(v) normalized[key] = normalized_array