From 2d366d004a8fcb39f49f31e4e6e6217afeb36809 Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Tue, 28 Jul 2026 13:19:21 -0500 Subject: [PATCH 1/2] Raise on failed system POST instead of swallowing it insert_self() left _resource_id unset on a non-ok response, so the failure only surfaced later as an AttributeError; _resource_id is now also initialized to None on every streamable. Fixes #42. --- pyproject.toml | 2 +- src/oshconnect/node.py | 8 +++++++ src/oshconnect/resources/base.py | 7 +++++- src/oshconnect/resources/system.py | 11 ++++++++++ tests/test_csapi_serialization.py | 35 ++++++++++++++++++++++++++++++ uv.lock | 2 +- 6 files changed, 62 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index d32f562..3fc5383 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "oshconnect" -version = "0.5.2a1" +version = "0.5.3a1" description = "Library for interfacing with OSH, helping guide visualization efforts, and providing a place to store configurations. Implements OGC CS API Part 3 (Pub/Sub) MQTT topic conventions including :data topics and resource event topics." readme = "README.md" authors = [ diff --git a/src/oshconnect/node.py b/src/oshconnect/node.py index fdac48b..5c270cc 100644 --- a/src/oshconnect/node.py +++ b/src/oshconnect/node.py @@ -354,11 +354,19 @@ def add_system(self, system: System, insert_resource: bool = False) -> System: in-memory only; useful when reconstructing state from a datastore or staging a system before a deferred POST. + A failed POST propagates out of ``insert_self()`` and the system + is *not* attached — better a loud failure here than a system that + looks attached but has no server-side id, which previously only + surfaced later as an ``AttributeError`` from the first child + resource call. + :param system: ``System`` object to attach. :param insert_resource: Whether to POST the system to the server before attaching it locally. :return: The same ``System`` (now parented to this node and tracked in ``self.systems()``). + :raises Exception: if ``insert_resource=True`` and the server + rejects the POST. """ if insert_resource: system.insert_self() diff --git a/src/oshconnect/resources/base.py b/src/oshconnect/resources/base.py index 43b673e..b94c93c 100644 --- a/src/oshconnect/resources/base.py +++ b/src/oshconnect/resources/base.py @@ -107,7 +107,7 @@ class StreamableResource(Generic[T], ABC): :param connection_mode: One of `StreamableModes`. Default ``PUSH``. """ _id: UUID - _resource_id: str + _resource_id: str | None # _canonical_link: str _topic: str _status: str = Status.STOPPED.value @@ -135,6 +135,11 @@ def __init__(self, node: Node, connection_mode: StreamableModes = StreamableMode self._outbound_deque = deque() self._subscribe_topic = None self._parent_resource_id = None + # Always present, even before the resource exists server-side, so + # pre-insert access is a clean `is None` check rather than an + # AttributeError far from the failed POST that caused it. + # Subclasses overwrite this when they know the server-assigned id. + self._resource_id = None def get_streamable_id(self) -> UUID: """Return the local UUID assigned at construction (not the server-side ID).""" diff --git a/src/oshconnect/resources/system.py b/src/oshconnect/resources/system.py index fc76d53..2a15344 100644 --- a/src/oshconnect/resources/system.py +++ b/src/oshconnect/resources/system.py @@ -549,6 +549,12 @@ def insert_self(self): the body before POST so a re-POSTed (e.g. cross-node-synced) system doesn't leak the source server's identifier or links to the destination — the destination assigns its own. + + :raises Exception: if the server returns a non-OK response. The + failure is raised here rather than swallowed — otherwise + ``_resource_id`` stays ``None`` and the error resurfaces much + later (and much less legibly) from the first child-resource + call that needs the system's id. """ body_resource = self.to_system_resource().model_copy(deep=True) body_resource.system_id = None @@ -564,6 +570,11 @@ def insert_self(self): self._resource_id = sys_id if self._underlying_resource is not None: self._underlying_resource.system_id = sys_id + else: + raise Exception( + f'Failed to insert system {self.label!r} ({self.urn!r}): ' + f'HTTP {res.status_code} — {res.text}' + ) def retrieve_resource(self): """GET ``/systems/{id}`` and refresh the underlying `SystemResource`. diff --git a/tests/test_csapi_serialization.py b/tests/test_csapi_serialization.py index 3d26d0e..9a5cd01 100644 --- a/tests/test_csapi_serialization.py +++ b/tests/test_csapi_serialization.py @@ -311,6 +311,41 @@ def test_insert_self_strips_id_and_links_from_body(node, monkeypatch): assert sys._resource_id == "dest-id-xyz" +def test_insert_self_raises_on_failed_post(node, monkeypatch): + """A rejected POST must raise with the status code and body, not be + swallowed. Previously `insert_self()` returned normally on a non-ok + response, leaving `_resource_id` unset — the failure only surfaced + later as an AttributeError from `add_insert_datastream()`. See + GitHub issue #42.""" + sys = System(label="Doomed", urn="urn:test:fail:1", parent_node=node) + + capture_request(monkeypatch, "post", response=MockResponse( + payload={"error": "disk full"}, status=500)) + + with pytest.raises(Exception, match=r"Failed to insert system"): + sys.insert_self() + + +def test_resource_id_is_none_before_insert(node): + """`_resource_id` exists (as None) on every wrapper from construction, + so pre-insert access is a clean None check rather than an + AttributeError. Guards the `from_resource`-without-id path too, which + never passed a `resource_id` kwarg. See GitHub issue #42.""" + sys = System(label="Uninserted", urn="urn:test:noid:1", parent_node=node) + assert sys._resource_id is None + + res = SystemResource.from_smljson_dict({ + "type": "PhysicalSystem", + "uniqueId": "urn:test:noid:2", + "label": "No Server Id", + }) + from_res = System.from_resource(res, node) + assert from_res._resource_id is None + # retrieve_resource() already guards on `is None`; without the base + # init it would AttributeError before reaching that check. + assert from_res.retrieve_resource() is None + + # =========================================================================== # Datastream: resource representation, schema document, observations # =========================================================================== diff --git a/uv.lock b/uv.lock index 89afc1b..a619a5a 100644 --- a/uv.lock +++ b/uv.lock @@ -570,7 +570,7 @@ wheels = [ [[package]] name = "oshconnect" -version = "0.5.1a22" +version = "0.5.3a1" source = { virtual = "." } dependencies = [ { name = "pydantic" }, From 6202feb3591bf7be320a616be671e2782d1bebd8 Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Tue, 28 Jul 2026 13:21:16 -0500 Subject: [PATCH 2/2] Cover the add_system(insert_resource=True) path and document the raise --- docs/source/architecture/insertion.md | 5 +++++ tests/test_csapi_serialization.py | 20 +++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/docs/source/architecture/insertion.md b/docs/source/architecture/insertion.md index 794b342..246ad94 100644 --- a/docs/source/architecture/insertion.md +++ b/docs/source/architecture/insertion.md @@ -44,6 +44,11 @@ The same pattern applies if you skip the `OSHConnect` convenience and build a `System` directly: just call `system.insert_self()` and the wrapper handles dump → POST → ID-capture itself. +If the server rejects the POST, `insert_self()` raises with the status +code and response body rather than returning quietly — otherwise the +system's `_resource_id` would stay `None` and the failure would only +resurface much later, from the first child-resource call that needs it. + ## Inserting a Datastream Similar shape, but the body is wrapped inside a diff --git a/tests/test_csapi_serialization.py b/tests/test_csapi_serialization.py index 9a5cd01..6193487 100644 --- a/tests/test_csapi_serialization.py +++ b/tests/test_csapi_serialization.py @@ -322,10 +322,28 @@ def test_insert_self_raises_on_failed_post(node, monkeypatch): capture_request(monkeypatch, "post", response=MockResponse( payload={"error": "disk full"}, status=500)) - with pytest.raises(Exception, match=r"Failed to insert system"): + # Status code and response body both belong in the message — they are + # the only diagnostic the caller gets. + with pytest.raises(Exception, match=r"HTTP 500"): + sys.insert_self() + with pytest.raises(Exception, match=r"disk full"): sys.insert_self() +def test_add_system_does_not_attach_on_failed_insert(node, monkeypatch): + """The issue's actual repro: `add_system(insert_resource=True)` against + a node that rejects the POST must raise, and must not leave a system + with no server-side id sitting in the node's collection. See GitHub + issue #42.""" + sys = System(label="Doomed", urn="urn:test:fail:2", parent_node=node) + + capture_request(monkeypatch, "post", response=MockResponse(status=500)) + + with pytest.raises(Exception, match=r"Failed to insert system"): + node.add_system(sys, insert_resource=True) + assert sys not in node.systems() + + def test_resource_id_is_none_before_insert(node): """`_resource_id` exists (as None) on every wrapper from construction, so pre-insert access is a clean None check rather than an