Fix: Accidental mutation of old model causes invalid JSON Patch (#802)

* fix #801

accidental mutation of old parent model caused invalid JSON patches

* add build_py to cmdclass

distutils has also been deprecated

Author: rmorshea

docs/source/about/changelog.rst

@@ -27,6 +27,7 @@ Unreleased
 
 - :issue:`789` - Conditionally rendered components cannot use contexts
 - :issue:`773` - Use strict equality check for text, numeric, and binary types in hooks
+- :issue:`801` - Accidental mutation of old model causes invalid JSON Patch
 
 **Changed**
 

noxfile.py

@@ -98,7 +98,7 @@ def example(session: Session) -> None:
 def docs(session: Session) -> None:
     """Build and display documentation in the browser (automatically reloads on change)"""
     install_requirements_file(session, "build-docs")
-    install_idom_dev(session, extras="all")
+    install_idom_dev(session)
     session.run(
         "python",
         "scripts/live_docs.py",
@@ -188,7 +188,7 @@ def test_python_suite(session: Session) -> None:
         session.install(".[all]")
     else:
         posargs += ["--cov=src/idom", "--cov-report", "term"]
-        install_idom_dev(session, extras="all")
+        install_idom_dev(session)
 
     session.run("pytest", *posargs)
 
@@ -234,7 +234,7 @@ def test_python_build(session: Session) -> None:
 def test_docs(session: Session) -> None:
     """Verify that the docs build and that doctests pass"""
     install_requirements_file(session, "build-docs")
-    install_idom_dev(session, extras="all")
+    install_idom_dev(session)
     session.run(
         "sphinx-build",
         "-a",  # re-write all output files
@@ -370,7 +370,8 @@ def install_requirements_file(session: Session, name: str) -> None:
     session.install("-r", str(file_path))
 
 
-def install_idom_dev(session: Session, extras: str = "stable") -> None:
+def install_idom_dev(session: Session, extras: str = "all") -> None:
+    session.run("pip", "--version")
     if "--no-install" not in session.posargs:
         session.install("-e", f".[{extras}]")
     else:

requirements/pkg-extras.txt

@@ -12,7 +12,7 @@ uvicorn[standard] >=0.13.4
 
 # extra=flask
 flask
-markupsafe<2.1
+markupsafe>=1.1.1,<2.1
 flask-cors
 flask-sock
 

setup.py

@@ -5,13 +5,12 @@
 import subprocess
 import sys
 import traceback
-from distutils import log
-from distutils.command.build import build  # type: ignore
-from distutils.command.sdist import sdist  # type: ignore
+from logging import StreamHandler, getLogger
 from pathlib import Path
 
 from setuptools import find_packages, setup
 from setuptools.command.develop import develop
+from setuptools.command.sdist import sdist
 
 
 if sys.platform == "win32":
@@ -22,6 +21,15 @@ def list2cmdline(cmd_list):
         return " ".join(map(pipes.quote, cmd_list))
 
 
+log = getLogger()
+log.addHandler(StreamHandler(sys.stdout))
+
+
+# -----------------------------------------------------------------------------
+# Basic Constants
+# -----------------------------------------------------------------------------
+
+
 # the name of the project
 NAME = "idom"
 
@@ -167,10 +175,18 @@ def run(self):
 
 package["cmdclass"] = {
     "sdist": build_javascript_first(sdist),
-    "build": build_javascript_first(build),
     "develop": build_javascript_first(develop),
 }
 
+if sys.version_info < (3, 10, 6):
+    from distutils.command.build import build
+
+    package["cmdclass"]["build"] = build_javascript_first(build)
+else:
+    from setuptools.command.build_py import build_py
+
+    package["cmdclass"]["build_py"] = build_javascript_first(build_py)
+
 
 # -----------------------------------------------------------------------------
 # Install It

src/idom/core/layout.py

@@ -230,10 +230,17 @@ def _render_component(
         else:
             key, index = new_state.key, new_state.index
             parent.children_by_key[key] = new_state
-            # need to do insertion in case where old_state is None and we're appending
-            parent.model.current["children"][index : index + 1] = [
-                new_state.model.current
-            ]
+            # need to add this model to parent's children without mutating parent model
+            old_parent_model = parent.model.current
+            old_parent_children = old_parent_model["children"]
+            parent.model.current = {
+                **old_parent_model,
+                "children": [
+                    *old_parent_children[:index],
+                    new_state.model.current,
+                    *old_parent_children[index + 1 :],
+                ],
+            }
         finally:
             # avoid double render
             self._rendering_queue.remove_if_pending(life_cycle_state.id)

tests/test_client.py

@@ -5,13 +5,15 @@
 from playwright.async_api import Browser
 
 import idom
+from idom.backend.utils import find_available_port
 from idom.testing import BackendFixture, DisplayFixture
 
 
 JS_DIR = Path(__file__).parent / "js"
 
 
 async def test_automatic_reconnect(browser: Browser):
+    port = find_available_port("localhost")
     page = await browser.new_page()
 
     # we need to wait longer here because the automatic reconnect is not instant
@@ -22,7 +24,7 @@ def OldComponent():
         return idom.html.p({"id": "old-component"}, "old")
 
     async with AsyncExitStack() as exit_stack:
-        server = await exit_stack.enter_async_context(BackendFixture(port=8000))
+        server = await exit_stack.enter_async_context(BackendFixture(port=port))
         display = await exit_stack.enter_async_context(
             DisplayFixture(server, driver=page)
         )
@@ -43,7 +45,7 @@ def NewComponent():
         return idom.html.p({"id": f"new-component-{state}"}, f"new-{state}")
 
     async with AsyncExitStack() as exit_stack:
-        server = await exit_stack.enter_async_context(BackendFixture(port=8000))
+        server = await exit_stack.enter_async_context(BackendFixture(port=port))
         display = await exit_stack.enter_async_context(
             DisplayFixture(server, driver=page)
         )

tests/test_core/test_hooks.py

@@ -11,12 +11,10 @@
     current_hook,
     strictly_equal,
 )
-from idom.core.layout import Layout
-from idom.core.serve import render_json_patch
+from idom.core.layout import Layout, LayoutUpdate
 from idom.testing import DisplayFixture, HookCatcher, assert_idom_did_log, poll
 from idom.testing.logs import assert_idom_did_not_log
 from idom.utils import Ref
-from tests.tooling.asserts import assert_same_items
 
 
 async def test_must_be_rendering_in_layout_to_use_hooks():
@@ -42,31 +40,35 @@ def SimpleStatefulComponent():
     sse = SimpleStatefulComponent()
 
     async with idom.Layout(sse) as layout:
-        patch_1 = await render_json_patch(layout)
-        assert patch_1.path == ""
-        assert_same_items(
-            patch_1.changes,
-            [
-                {"op": "add", "path": "/tagName", "value": ""},
-                {
-                    "op": "add",
-                    "path": "/children",
-                    "value": [{"children": ["0"], "tagName": "div"}],
-                },
-            ],
+        update_1 = await layout.render()
+        assert update_1 == LayoutUpdate(
+            path="",
+            old=None,
+            new={
+                "tagName": "",
+                "children": [{"tagName": "div", "children": ["0"]}],
+            },
         )
 
-        patch_2 = await render_json_patch(layout)
-        assert patch_2.path == ""
-        assert patch_2.changes == [
-            {"op": "replace", "path": "/children/0/children/0", "value": "1"}
-        ]
-
-        patch_3 = await render_json_patch(layout)
-        assert patch_3.path == ""
-        assert patch_3.changes == [
-            {"op": "replace", "path": "/children/0/children/0", "value": "2"}
-        ]
+        update_2 = await layout.render()
+        assert update_2 == LayoutUpdate(
+            path="",
+            old=update_1.new,
+            new={
+                "tagName": "",
+                "children": [{"tagName": "div", "children": ["1"]}],
+            },
+        )
+
+        update_3 = await layout.render()
+        assert update_3 == LayoutUpdate(
+            path="",
+            old=update_2.new,
+            new={
+                "tagName": "",
+                "children": [{"tagName": "div", "children": ["2"]}],
+            },
+        )
 
 
 async def test_set_state_callback_identity_is_preserved():