Merge pull request #50 from Botts-Innovative-Research/fix/insert-self-silent-failure

Raise on failed system POST instead of swallowing it

Author: tipatterson-dev

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

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 = [

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()

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)."""

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`.

tests/test_csapi_serialization.py

@@ -311,6 +311,59 @@ 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))
+
+    # 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
+    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
 # ===========================================================================