From a3d79aa8f5743ddbe4b0c229832bcf48d86d19e1 Mon Sep 17 00:00:00 2001 From: Ryan Morshead Date: Wed, 14 Jun 2023 22:18:43 -0600 Subject: [PATCH] report better error for attrs that cannot be serialized (#1008) * report better error for attrs that cannot be serialized also misc changes to config options to make this eaiser in particular ability to specify options as parents * add more tests * no need to be clever - just inherit * fix tests * ignore missing cov * add test for non-json serializable attrs * add changelog entry * fix tests * try node 16? https://github.com/nodesource/distributions#using-ubuntu-3 * install npm explicitey * try install in different order * update first * explicitely install npm i guess... --- docs/Dockerfile | 4 +- docs/source/about/changelog.rst | 5 +- src/py/reactpy/reactpy/_option.py | 51 +++++++++++----- src/py/reactpy/reactpy/backend/_common.py | 2 +- src/py/reactpy/reactpy/config.py | 67 ++++++++++++++------- src/py/reactpy/reactpy/core/serve.py | 14 ++++- src/py/reactpy/reactpy/core/vdom.py | 15 +++-- src/py/reactpy/tests/test__option.py | 30 ++++++++- src/py/reactpy/tests/test_config.py | 24 ++++++++ src/py/reactpy/tests/test_core/test_vdom.py | 44 ++++++++------ src/py/reactpy/tests/test_utils.py | 4 -- 11 files changed, 185 insertions(+), 75 deletions(-) diff --git a/docs/Dockerfile b/docs/Dockerfile index 76a8ad7ee..39b9c51be 100644 --- a/docs/Dockerfile +++ b/docs/Dockerfile @@ -4,8 +4,8 @@ WORKDIR /app/ # Install NodeJS # -------------- -RUN curl -sL https://deb.nodesource.com/setup_14.x | bash - -RUN apt-get install -yq nodejs build-essential +RUN curl -fsSL https://deb.nodesource.com/setup_16.x | bash - +RUN apt-get install -y build-essential nodejs npm RUN npm install -g npm@8.5.0 # Install Poetry diff --git a/docs/source/about/changelog.rst b/docs/source/about/changelog.rst index f739ce980..62e7d47eb 100644 --- a/docs/source/about/changelog.rst +++ b/docs/source/about/changelog.rst @@ -23,7 +23,10 @@ more info, see the :ref:`Contributor Guide `. Unreleased ---------- -No changes. +**Fixed** + +- :issue:`930` - better traceback for JSON serialization errors (via :pull:`1008`) +- :issue:`437` - explain that JS component attributes must be JSON (via :pull:`1008`) v1.0.0 diff --git a/src/py/reactpy/reactpy/_option.py b/src/py/reactpy/reactpy/_option.py index 1421f33a3..09d0304a9 100644 --- a/src/py/reactpy/reactpy/_option.py +++ b/src/py/reactpy/reactpy/_option.py @@ -8,6 +8,7 @@ _O = TypeVar("_O") logger = getLogger(__name__) +UNDEFINED = cast(Any, object()) class Option(Generic[_O]): @@ -16,8 +17,9 @@ class Option(Generic[_O]): def __init__( self, name: str, - default: _O | Option[_O], + default: _O = UNDEFINED, mutable: bool = True, + parent: Option[_O] | None = None, validator: Callable[[Any], _O] = lambda x: cast(_O, x), ) -> None: self._name = name @@ -28,12 +30,15 @@ def __init__( if name in os.environ: self._current = validator(os.environ[name]) - self._default: _O - if isinstance(default, Option): - self._default = default.default - default.subscribe(lambda value: setattr(self, "_default", value)) - else: + if parent is not None: + if not (parent.mutable and self.mutable): + raise TypeError("Parent and child options must be mutable") + self._default = parent.default + parent.subscribe(self.set_current) + elif default is not UNDEFINED: self._default = default + else: + raise TypeError("Must specify either a default or a parent option") logger.debug(f"{self._name}={self.current}") @@ -81,11 +86,19 @@ def set_current(self, new: Any) -> None: Raises a ``TypeError`` if this option is not :attr:`Option.mutable`. """ + old = self.current + if new is old: + return None + if not self._mutable: msg = f"{self} cannot be modified after initial load" raise TypeError(msg) - old = self.current - new = self._current = self._validator(new) + + try: + new = self._current = self._validator(new) + except ValueError as error: + raise ValueError(f"Invalid value for {self._name}: {new!r}") from error + logger.debug(f"{self._name}={self._current}") if new != old: for sub_func in self._subscribers: @@ -119,15 +132,23 @@ def __repr__(self) -> str: return f"Option({self._name}={self.current!r})" -class DeprecatedOption(Option[_O]): # nocov - def __init__(self, message: str, *args: Any, **kwargs: Any) -> None: - self._deprecation_message = message +class DeprecatedOption(Option[_O]): + """An option that will warn when it is accessed""" + + def __init__(self, *args: Any, message: str, **kwargs: Any) -> None: super().__init__(*args, **kwargs) + self._deprecation_message = message @Option.current.getter # type: ignore def current(self) -> _O: - warn( - self._deprecation_message, - DeprecationWarning, - ) + try: + # we access the current value during init to debug log it + # no need to warn unless it's actually used. since this attr + # is only set after super().__init__ is called, we can check + # for it to determine if it's being accessed by a user. + msg = self._deprecation_message + except AttributeError: + pass + else: + warn(msg, DeprecationWarning) return super().current diff --git a/src/py/reactpy/reactpy/backend/_common.py b/src/py/reactpy/reactpy/backend/_common.py index 80b4eeee1..17983a033 100644 --- a/src/py/reactpy/reactpy/backend/_common.py +++ b/src/py/reactpy/reactpy/backend/_common.py @@ -114,7 +114,7 @@ def vdom_head_elements_to_html(head: Sequence[VdomDict] | VdomDict | str) -> str head = cast(VdomDict, {**head, "tagName": ""}) return vdom_to_html(head) else: - return vdom_to_html(html._(head)) + return vdom_to_html(html._(*head)) @dataclass diff --git a/src/py/reactpy/reactpy/config.py b/src/py/reactpy/reactpy/config.py index 6dc29096c..8371e6d08 100644 --- a/src/py/reactpy/reactpy/config.py +++ b/src/py/reactpy/reactpy/config.py @@ -3,44 +3,65 @@ variables or, for those which allow it, a programmatic interface. """ +from __future__ import annotations + from pathlib import Path from tempfile import TemporaryDirectory -from reactpy._option import Option as _Option +from reactpy._option import Option -REACTPY_DEBUG_MODE = _Option( - "REACTPY_DEBUG_MODE", - default=False, - validator=lambda x: bool(int(x)), -) -"""This immutable option turns on/off debug mode +TRUE_VALUES = {"true", "1"} +FALSE_VALUES = {"false", "0"} -The string values ``1`` and ``0`` are mapped to ``True`` and ``False`` respectively. -When debug is on, extra validation measures are applied that negatively impact -performance but can be used to catch bugs during development. Additionally, the default -log level for ReactPy is set to ``DEBUG``. -""" +def boolean(value: str | bool | int) -> bool: + if isinstance(value, bool): + return value + elif isinstance(value, int): + return bool(value) + elif not isinstance(value, str): + raise TypeError(f"Expected str or bool, got {type(value).__name__}") + + if value.lower() in TRUE_VALUES: + return True + elif value.lower() in FALSE_VALUES: + return False + else: + raise ValueError( + f"Invalid boolean value {value!r} - expected " + f"one of {list(TRUE_VALUES | FALSE_VALUES)}" + ) + -REACTPY_CHECK_VDOM_SPEC = _Option( - "REACTPY_CHECK_VDOM_SPEC", - default=REACTPY_DEBUG_MODE, - validator=lambda x: bool(int(x)), +REACTPY_DEBUG_MODE = Option( + "REACTPY_DEBUG_MODE", default=False, validator=boolean, mutable=True ) -"""This immutable option turns on/off checks which ensure VDOM is rendered to spec +"""Get extra logs and validation checks at the cost of performance. -The string values ``1`` and ``0`` are mapped to ``True`` and ``False`` respectively. +This will enable the following: -By default this check is off. When ``REACTPY_DEBUG_MODE=1`` this will be turned on but can -be manually disablled by setting ``REACTPY_CHECK_VDOM_SPEC=0`` in addition. +- :data:`REACTPY_CHECK_VDOM_SPEC` +- :data:`REACTPY_CHECK_JSON_ATTRS` +""" + +REACTPY_CHECK_VDOM_SPEC = Option("REACTPY_CHECK_VDOM_SPEC", parent=REACTPY_DEBUG_MODE) +"""Checks which ensure VDOM is rendered to spec For more info on the VDOM spec, see here: :ref:`VDOM JSON Schema` """ -# Because these web modules will be linked dynamically at runtime this can be temporary +REACTPY_CHECK_JSON_ATTRS = Option("REACTPY_CHECK_JSON_ATTRS", parent=REACTPY_DEBUG_MODE) +"""Checks that all VDOM attributes are JSON serializable + +The VDOM spec is not able to enforce this on its own since attributes could anything. +""" + +# Because these web modules will be linked dynamically at runtime this can be temporary. +# Assigning to a variable here ensures that the directory is not deleted until the end +# of the program. _DEFAULT_WEB_MODULES_DIR = TemporaryDirectory() -REACTPY_WEB_MODULES_DIR = _Option( +REACTPY_WEB_MODULES_DIR = Option( "REACTPY_WEB_MODULES_DIR", default=Path(_DEFAULT_WEB_MODULES_DIR.name), validator=Path, @@ -52,7 +73,7 @@ set of publicly available APIs for working with the client. """ -REACTPY_TESTING_DEFAULT_TIMEOUT = _Option( +REACTPY_TESTING_DEFAULT_TIMEOUT = Option( "REACTPY_TESTING_DEFAULT_TIMEOUT", 5.0, mutable=False, diff --git a/src/py/reactpy/reactpy/core/serve.py b/src/py/reactpy/reactpy/core/serve.py index 61a7e4ce6..3a530e854 100644 --- a/src/py/reactpy/reactpy/core/serve.py +++ b/src/py/reactpy/reactpy/core/serve.py @@ -7,6 +7,7 @@ from anyio import create_task_group from anyio.abc import TaskGroup +from reactpy.config import REACTPY_DEBUG_MODE from reactpy.core.types import LayoutEventMessage, LayoutType, LayoutUpdateMessage logger = getLogger(__name__) @@ -49,7 +50,18 @@ async def _single_outgoing_loop( layout: LayoutType[LayoutUpdateMessage, LayoutEventMessage], send: SendCoroutine ) -> None: while True: - await send(await layout.render()) + update = await layout.render() + try: + await send(update) + except Exception: # nocov + if not REACTPY_DEBUG_MODE.current: + msg = ( + "Failed to send update. More info may be available " + "if you enabling debug mode by setting " + "`reactpy.config.REACTPY_DEBUG_MODE.current = True`." + ) + logger.error(msg) + raise async def _single_incoming_loop( diff --git a/src/py/reactpy/reactpy/core/vdom.py b/src/py/reactpy/reactpy/core/vdom.py index 0548c6afc..840a09c7c 100644 --- a/src/py/reactpy/reactpy/core/vdom.py +++ b/src/py/reactpy/reactpy/core/vdom.py @@ -1,6 +1,6 @@ from __future__ import annotations -import logging +import json from collections.abc import Mapping, Sequence from functools import wraps from typing import Any, Protocol, cast, overload @@ -8,7 +8,7 @@ from fastjsonschema import compile as compile_json_schema from reactpy._warnings import warn -from reactpy.config import REACTPY_DEBUG_MODE +from reactpy.config import REACTPY_CHECK_JSON_ATTRS, REACTPY_DEBUG_MODE from reactpy.core._f_back import f_module_name from reactpy.core.events import EventHandler, to_event_handler_function from reactpy.core.types import ( @@ -25,9 +25,6 @@ VdomJson, ) -logger = logging.getLogger() - - VDOM_JSON_SCHEMA = { "$schema": "http://json-schema.org/draft-07/schema", "$ref": "#/definitions/element", @@ -199,6 +196,8 @@ def vdom( attributes, event_handlers = separate_attributes_and_event_handlers(attributes) if attributes: + if REACTPY_CHECK_JSON_ATTRS.current: + json.dumps(attributes) model["attributes"] = attributes if children: @@ -325,18 +324,18 @@ def _is_single_child(value: Any) -> bool: def _validate_child_key_integrity(value: Any) -> None: if hasattr(value, "__iter__") and not hasattr(value, "__len__"): - logger.error( + warn( f"Did not verify key-path integrity of children in generator {value} " "- pass a sequence (i.e. list of finite length) in order to verify" ) else: for child in value: if isinstance(child, ComponentType) and child.key is None: - logger.error(f"Key not specified for child in list {child}") + warn(f"Key not specified for child in list {child}", UserWarning) elif isinstance(child, Mapping) and "key" not in child: # remove 'children' to reduce log spam child_copy = {**child, "children": _EllipsisRepr()} - logger.error(f"Key not specified for child in list {child_copy}") + warn(f"Key not specified for child in list {child_copy}", UserWarning) class _CustomVdomDictConstructor(Protocol): diff --git a/src/py/reactpy/tests/test__option.py b/src/py/reactpy/tests/test__option.py index 63f2fada8..929e17488 100644 --- a/src/py/reactpy/tests/test__option.py +++ b/src/py/reactpy/tests/test__option.py @@ -33,7 +33,7 @@ def test_option_validator(): opt.current = "0" assert opt.current is False - with pytest.raises(ValueError, match="invalid literal for int"): + with pytest.raises(ValueError, match="Invalid value"): opt.current = "not-an-int" @@ -102,10 +102,36 @@ def test_option_subscribe(): def test_deprecated_option(): - opt = DeprecatedOption("is deprecated!", "A_FAKE_OPTION", None) + opt = DeprecatedOption("A_FAKE_OPTION", None, message="is deprecated!") with pytest.warns(DeprecationWarning, match="is deprecated!"): assert opt.current is None with pytest.warns(DeprecationWarning, match="is deprecated!"): opt.current = "something" + + +def test_option_parent(): + parent_opt = Option("A_FAKE_OPTION", "default-value", mutable=True) + child_opt = Option("A_FAKE_OPTION", parent=parent_opt) + assert child_opt.mutable + assert child_opt.current == "default-value" + + parent_opt.current = "new-value" + assert child_opt.current == "new-value" + + +def test_option_parent_child_must_be_mutable(): + mut_parent_opt = Option("A_FAKE_OPTION", "default-value", mutable=True) + immu_parent_opt = Option("A_FAKE_OPTION", "default-value", mutable=False) + with pytest.raises(TypeError, match="must be mutable"): + Option("A_FAKE_OPTION", parent=mut_parent_opt, mutable=False) + with pytest.raises(TypeError, match="must be mutable"): + Option("A_FAKE_OPTION", parent=immu_parent_opt, mutable=None) + + +def test_no_default_or_parent(): + with pytest.raises( + TypeError, match="Must specify either a default or a parent option" + ): + Option("A_FAKE_OPTION") diff --git a/src/py/reactpy/tests/test_config.py b/src/py/reactpy/tests/test_config.py index ecbbb998c..3428c3e28 100644 --- a/src/py/reactpy/tests/test_config.py +++ b/src/py/reactpy/tests/test_config.py @@ -27,3 +27,27 @@ def test_reactpy_debug_mode_toggle(): # just check that nothing breaks config.REACTPY_DEBUG_MODE.current = True config.REACTPY_DEBUG_MODE.current = False + + +def test_boolean(): + assert config.boolean(True) is True + assert config.boolean(False) is False + assert config.boolean(1) is True + assert config.boolean(0) is False + assert config.boolean("true") is True + assert config.boolean("false") is False + assert config.boolean("True") is True + assert config.boolean("False") is False + assert config.boolean("TRUE") is True + assert config.boolean("FALSE") is False + assert config.boolean("1") is True + assert config.boolean("0") is False + + with pytest.raises(ValueError): + config.boolean("2") + + with pytest.raises(ValueError): + config.boolean("") + + with pytest.raises(TypeError): + config.boolean(None) diff --git a/src/py/reactpy/tests/test_core/test_vdom.py b/src/py/reactpy/tests/test_core/test_vdom.py index 76e26e46f..37abad1d2 100644 --- a/src/py/reactpy/tests/test_core/test_vdom.py +++ b/src/py/reactpy/tests/test_core/test_vdom.py @@ -280,28 +280,36 @@ def test_invalid_vdom(value, error_message_pattern): validate_vdom_json(value) -@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only logs in debug mode") -def test_debug_log_cannot_verify_keypath_for_genereators(caplog): - reactpy.vdom("div", (1 for i in range(10))) - assert len(caplog.records) == 1 - assert caplog.records[0].message.startswith( - "Did not verify key-path integrity of children in generator" - ) - caplog.records.clear() - +@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only warns in debug mode") +def test_warn_cannot_verify_keypath_for_genereators(): + with pytest.warns(UserWarning) as record: + reactpy.vdom("div", (1 for i in range(10))) + assert len(record) == 1 + assert ( + record[0] + .message.args[0] + .startswith("Did not verify key-path integrity of children in generator") + ) -@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only logs in debug mode") -def test_debug_log_dynamic_children_must_have_keys(caplog): - reactpy.vdom("div", [reactpy.vdom("div")]) - assert len(caplog.records) == 1 - assert caplog.records[0].message.startswith("Key not specified for child") - caplog.records.clear() +@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only warns in debug mode") +def test_warn_dynamic_children_must_have_keys(): + with pytest.warns(UserWarning) as record: + reactpy.vdom("div", [reactpy.vdom("div")]) + assert len(record) == 1 + assert record[0].message.args[0].startswith("Key not specified for child") @reactpy.component def MyComponent(): return reactpy.vdom("div") - reactpy.vdom("div", [MyComponent()]) - assert len(caplog.records) == 1 - assert caplog.records[0].message.startswith("Key not specified for child") + with pytest.warns(UserWarning) as record: + reactpy.vdom("div", [MyComponent()]) + assert len(record) == 1 + assert record[0].message.args[0].startswith("Key not specified for child") + + +@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="only checked in debug mode") +def test_raise_for_non_json_attrs(): + with pytest.raises(TypeError, match="JSON serializable"): + reactpy.html.div({"non_json_serializable_object": object()}) diff --git a/src/py/reactpy/tests/test_utils.py b/src/py/reactpy/tests/test_utils.py index c71057f15..ca3080358 100644 --- a/src/py/reactpy/tests/test_utils.py +++ b/src/py/reactpy/tests/test_utils.py @@ -204,10 +204,6 @@ def test_del_html_body_transform(): html.div(SOME_OBJECT), f"
{html_escape(str(SOME_OBJECT))}
", ), - ( - html.div({"someAttribute": SOME_OBJECT}), - f'
', - ), ( html.div( "hello", html.a({"href": "https://example.com"}, "example"), "world"