From 6ed51ba0a9c76150cdd61cc330126c174d7e218d Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Tue, 28 Jul 2026 15:35:14 -0500 Subject: [PATCH 1/3] Add typed exception hierarchy; raise instead of failing silently Closes #47, #43, #44, #49. Also fixes Node.__repr__ raising AttributeError on nodes without credentials or transports. --- docs/source/api.rst | 14 +++ pyproject.toml | 2 +- src/oshconnect/__init__.py | 11 +++ src/oshconnect/exceptions.py | 116 +++++++++++++++++++++++++ src/oshconnect/node.py | 63 +++++++++----- src/oshconnect/oshconnectapi.py | 50 ++++++++--- src/oshconnect/resources/base.py | 44 ++++++++++ src/oshconnect/resources/datastream.py | 17 ++-- src/oshconnect/resources/system.py | 66 +++++++------- tests/test_csapi_serialization.py | 103 +++++++++++++++++++++- uv.lock | 2 +- 11 files changed, 407 insertions(+), 81 deletions(-) create mode 100644 src/oshconnect/exceptions.py diff --git a/docs/source/api.rst b/docs/source/api.rst index 19e73f2..c985cf4 100644 --- a/docs/source/api.rst +++ b/docs/source/api.rst @@ -21,6 +21,20 @@ Core Application --- +Exceptions +---------- +Every error OSHConnect raises deliberately descends from ``OSHConnectError``, +so callers can catch library failures without also swallowing their own bugs. +``OSHConnectError`` subclasses the builtin ``Exception``, so existing +``except Exception:`` handlers keep working. + +.. automodule:: oshconnect.exceptions + :members: + :undoc-members: + :show-inheritance: + +--- + Streamable Resources -------------------- These are the primary objects for interacting with systems, datastreams, and control streams on an OSH node. diff --git a/pyproject.toml b/pyproject.toml index 3fc5383..fc0b5b7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "oshconnect" -version = "0.5.3a1" +version = "0.5.5a0" 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/__init__.py b/src/oshconnect/__init__.py index 4b677e2..2f0f1f7 100644 --- a/src/oshconnect/__init__.py +++ b/src/oshconnect/__init__.py @@ -5,6 +5,17 @@ # Contact Email: ian@botts-inc.com # ============================================================================== +# Exceptions — every deliberate failure descends from OSHConnectError, +# which subclasses Exception so existing `except Exception:` still works. +from .exceptions import ( + OSHConnectError, + ConfigurationError, + ResourceRequestError, + ResourceInsertError, + MissingLocationHeaderError, + ResourceDiscoveryError, +) + # Core resources from .oshconnectapi import OSHConnect from .streamableresource import Node, System, Datastream, ControlStream, StreamableModes, Status diff --git a/src/oshconnect/exceptions.py b/src/oshconnect/exceptions.py new file mode 100644 index 0000000..cfbdd15 --- /dev/null +++ b/src/oshconnect/exceptions.py @@ -0,0 +1,116 @@ +# ============================================================================= +# Copyright (c) 2026 Georobotix Innovative Research +# Date: 2026/7/28 +# Author: Ian Patterson +# Contact Email: ian.patterson@georobotix.us +# ============================================================================= + +"""Typed exceptions raised by OSHConnect. + +Every error the library raises deliberately descends from +`OSHConnectError`, so callers can wrap an OSHConnect operation and catch +*its* failures without also swallowing `KeyError`, `AttributeError`, and +every other bug in their own code:: + + from oshconnect.exceptions import OSHConnectError, ResourceInsertError + + try: + system.insert_self() + except ResourceInsertError as e: + if e.status_code == 507: # server out of disk — worth retrying + schedule_retry() + else: + raise + +`OSHConnectError` subclasses the builtin `Exception`, so pre-existing +``except Exception:`` handlers keep working unchanged. + +The HTTP free-function layer in `oshconnect.api_helpers` deliberately does +*not* raise these — it returns raw `requests.Response` objects and leaves +status interpretation to the caller. These exceptions come from the +wrapper layer (`System`, `Datastream`, `ControlStream`, `Node`, +`OSHConnect`), which does interpret responses on the caller's behalf. +""" +from __future__ import annotations + + +class OSHConnectError(Exception): + """Base class for every error OSHConnect raises deliberately. + + Catch this to handle any OSHConnect-originated failure while letting + genuine programming errors propagate. + """ + + +class ConfigurationError(OSHConnectError): + """The library was asked to act on an object it hasn't been given. + + Raised for caller-side wiring mistakes that are detectable before any + HTTP request is attempted — e.g. inserting a system into a `Node` that + was never registered with the `OSHConnect` instance. + """ + + +class ResourceRequestError(OSHConnectError): + """Base for failures tied to a specific CS API HTTP exchange. + + Carries whatever the response made available. Every field is optional + because not every call site has all of them — check for ``None`` + rather than assuming. + + :param message: Human-readable description; becomes ``str(exc)``. + :param status_code: HTTP status from the response, when there was one. + :param response_text: Raw response body, when there was one. + :param resource_type: The CS API resource involved, e.g. ``'system'``. + :param resource_label: Caller-facing name of the specific resource, + e.g. the system's label or the datastream's name. + """ + + def __init__(self, message: str, *, status_code: int = None, + response_text: str = None, resource_type: str = None, + resource_label: str = None): + super().__init__(message) + self.status_code = status_code + self.response_text = response_text + self.resource_type = resource_type + self.resource_label = resource_label + + +class ResourceInsertError(ResourceRequestError): + """A create (POST) of a CS API resource did not succeed. + + Raised when the server returns a non-OK response to a resource + creation request — inserting a system, datastream, control stream, or + observation. + """ + + +class MissingLocationHeaderError(ResourceInsertError): + """A create POST succeeded but the server omitted ``Location``. + + The resource was very likely created; OSHConnect just cannot learn its + server-assigned id, so the local wrapper cannot be linked to it. A + distinct type because the remediation differs from an outright + rejection: the caller may need to re-discover rather than re-POST, to + avoid creating a duplicate. Subclasses `ResourceInsertError` so + callers that don't care about the distinction can catch the broader + type. + """ + + +class ResourceDiscoveryError(ResourceRequestError): + """A listing / discovery (GET) request did not succeed. + + Distinguishes a genuine failure — server down, bad credentials, 5xx — + from the legitimately-empty result that discovery otherwise returns. + """ + + +__all__ = [ + "OSHConnectError", + "ConfigurationError", + "ResourceRequestError", + "ResourceInsertError", + "MissingLocationHeaderError", + "ResourceDiscoveryError", +] diff --git a/src/oshconnect/node.py b/src/oshconnect/node.py index 5c270cc..6e545c3 100644 --- a/src/oshconnect/node.py +++ b/src/oshconnect/node.py @@ -35,6 +35,7 @@ from .csapi4py.default_api_helpers import APIHelper from .csapi4py.mqtt import MQTTCommClient from .csapi4py.nats import NatsCommClient +from .exceptions import ResourceDiscoveryError from .resource_datamodels import SystemResource if TYPE_CHECKING: @@ -196,6 +197,17 @@ def __init__(self, protocol: str, address: str, port: int, username: str = None, self.address = address self.server_root = server_root self.port = port + # Bind every declared dataclass field up front, even when the + # corresponding feature is off. The generated `__repr__` reads all + # of them unconditionally, so a field left unset made `repr(node)` + # raise `AttributeError: 'Node' object has no attribute + # '_basic_auth'` (anonymous nodes) or `'_mqtt_client'` / + # '_nats_client'` (transport-less nodes) — which then surfaced + # anywhere a Node appeared in an error message or a pytest failure + # report, burying the real problem under a repr traceback. + self._basic_auth = None + self._mqtt_client = None + self._nats_client = None self.is_secure = username is not None and password is not None if self.is_secure: self.add_basicauth(username, password) @@ -307,8 +319,14 @@ def discover_systems(self) -> list[System] | None: The new systems are appended to this node's internal list and also returned for convenience. - :return: List of newly-created `System` objects, or ``None`` if - the HTTP request failed. + :return: List of newly-created `System` objects. An empty list + means the server has no systems — a failure raises instead of + returning a falsy value, so the two are distinguishable. + :raises ResourceDiscoveryError: if the listing request fails. + Previously this returned ``None``, which the common + ``for s in node.discover_systems() or []:`` idiom silently + turned into a zero-iteration loop — an auth failure and an + empty server looked identical. See GitHub issue #49. """ # Deferred runtime import: System -> StreamableResource -> Node would # otherwise close a cycle when this module is first loaded. @@ -317,25 +335,28 @@ def discover_systems(self) -> list[System] | None: APIResourceTypes.SYSTEM, params={'f': 'application/sml+json'}, ) - if result.ok: - new_systems = [] - system_objs = result.json()['items'] - for system_json in system_objs: - system = SystemResource.model_validate(system_json, by_alias=True) - # Route through the canonical factory so the parsed - # `SystemResource` is bound to the wrapper via - # `set_system_resource(...)`. The previous manual - # `System(label=..., name=..., urn=..., resource_id=...)` - # call dropped the parsed resource on the floor — - # any caller reaching for `_underlying_resource` - # (deep-copy round-trip, cross-node sync, geometry, - # validTime, properties) saw only a thin shell. - sys_obj = System.from_resource(system, parent_node=self) - self._systems.append(sys_obj) - new_systems.append(sys_obj) - return new_systems - else: - return None + if not result.ok: + raise ResourceDiscoveryError( + f'Failed to list systems on {self._api_helper.get_base_url()}: ' + f'HTTP {result.status_code} — {result.text}', + status_code=result.status_code, response_text=result.text, + resource_type='system', + ) + new_systems = [] + for system_json in result.json()['items']: + system = SystemResource.model_validate(system_json, by_alias=True) + # Route through the canonical factory so the parsed + # `SystemResource` is bound to the wrapper via + # `set_system_resource(...)`. The previous manual + # `System(label=..., name=..., urn=..., resource_id=...)` + # call dropped the parsed resource on the floor — + # any caller reaching for `_underlying_resource` + # (deep-copy round-trip, cross-node sync, geometry, + # validTime, properties) saw only a thin shell. + sys_obj = System.from_resource(system, parent_node=self) + self._systems.append(sys_obj) + new_systems.append(sys_obj) + return new_systems def get_api_helper(self) -> APIHelper: """Return the `APIHelper` this node uses for HTTP calls.""" diff --git a/src/oshconnect/oshconnectapi.py b/src/oshconnect/oshconnectapi.py index ce6c8d2..eb4abcd 100644 --- a/src/oshconnect/oshconnectapi.py +++ b/src/oshconnect/oshconnectapi.py @@ -13,6 +13,7 @@ from .events.builder import EventBuilder from .csapi4py.default_api_helpers import APIHelper from .datastore import DataStore +from .exceptions import ConfigurationError from .resource_datamodels import DatastreamResource from .streamableresource import Node, System, SessionManager, Datastream, ControlStream from .styling import Styling @@ -252,16 +253,40 @@ def set_timeperiod(self, start_time: str, end_time: str): # """ # return self._datasource_handler.get_messages() + def _require_registered_node(self, target_node: Node) -> None: + """Guard: the target node must be registered with this instance. + + These methods previously guarded on ``if target_node in + self._nodes:`` with no ``else``, so an unregistered node made them + fall off the end and return ``None`` — no POST attempted, no + exception, no log line. Callers of `create_and_insert_system` got + ``None`` back from a method documented to return the created + system, and only found out later when that ``None`` was + dereferenced. See GitHub issue #43. + + :raises ConfigurationError: if the node isn't registered. + """ + if target_node not in self._nodes: + # Identify the node by its address rather than `!r` — a full + # Node repr drags in the API helper, session, and system list, + # which buries the actual message. + raise ConfigurationError( + f"Node {target_node.get_address()} is not registered with this " + f"OSHConnect instance; call add_node() before adding systems " + f"to it." + ) + def _insert_system(self, system: System, target_node: Node): """ Create a system on the target node. :param system: System object :param target_node: Node object, must be within the OSHConnect instance :return: the created system + :raises ConfigurationError: if ``target_node`` isn't registered. """ - if target_node in self._nodes: - self.add_system_to_node(system, target_node, insert_resource=True) - return system + self._require_registered_node(target_node) + self.add_system_to_node(system, target_node, insert_resource=True) + return system def add_datastream(self, datastream: DatastreamResource, system: str | System) -> str: """ @@ -300,12 +325,12 @@ def add_system_to_node(self, system: System, target_node: Node, insert_resource: :param system: System object :param target_node: Node object, must be within the OSHConnect instance :param insert_resource: Whether to insert the system into the target node's server, default is False - :return: + :return: None. + :raises ConfigurationError: if ``target_node`` isn't registered with this instance. """ - if target_node in self._nodes: - target_node.add_system(system, insert_resource=insert_resource) - self._systems.append(system) - return + self._require_registered_node(target_node) + target_node.add_system(system, insert_resource=insert_resource) + self._systems.append(system) def create_and_insert_system(self, system_opts: dict, target_node: Node): """ @@ -313,11 +338,12 @@ def create_and_insert_system(self, system_opts: dict, target_node: Node): :param system_opts: System object parameters :param target_node: Node object, must be within the OSHConnect instance :return: the created system + :raises ConfigurationError: if ``target_node`` isn't registered with this instance. Previously this returned ``None``, which callers then dereferenced far from the actual mistake. """ - if target_node in self._nodes: - new_system = System(**system_opts) - self.add_system_to_node(new_system, target_node, insert_resource=True) - return new_system + self._require_registered_node(target_node) + new_system = System(**system_opts) + self.add_system_to_node(new_system, target_node, insert_resource=True) + return new_system def remove_system(self, system_id: str): pass diff --git a/src/oshconnect/resources/base.py b/src/oshconnect/resources/base.py index b94c93c..dd4b8d7 100644 --- a/src/oshconnect/resources/base.py +++ b/src/oshconnect/resources/base.py @@ -43,6 +43,7 @@ from ..csapi4py.default_api_helpers import resource_type_to_endpoint from ..csapi4py.mqtt import MQTTCommClient, mqtt_topic_format_token from ..csapi4py.nats import NatsCommClient +from ..exceptions import MissingLocationHeaderError, ResourceInsertError from ..resource_datamodels import ControlStreamResource from ..resource_datamodels import DatastreamResource from ..resource_datamodels import SystemResource @@ -52,6 +53,49 @@ from ..node import Node +def new_resource_id_from_response(res, *, resource_type: str, + resource_label: str = None) -> str: + """Extract a server-assigned resource id from a create-POST response. + + Every CS API create call follows the same contract — POST the body, + read the new id off the ``Location`` header — and every one of them + can fail the same two ways. Centralised here so all five call sites + (`System.insert_self`, `System.add_insert_datastream`, + `System.add_insert_controlstream`, + `System.add_and_insert_control_stream`, + `Datastream.insert_observation_dict`) report failures identically. + + :param res: The `requests.Response` from the create request. + :param resource_type: Human-readable resource name for the message, + e.g. ``'system'`` or ``'observation'``. + :param resource_label: The specific resource's caller-facing name, + included in the message when available. + :return: The new resource's server-assigned id. + :raises ResourceInsertError: if the response is not OK. + :raises MissingLocationHeaderError: if the response is OK but carries + no ``Location`` header, so the id cannot be recovered. + """ + named = f' {resource_label!r}' if resource_label is not None else '' + if not res.ok: + raise ResourceInsertError( + f'Failed to create {resource_type}{named}: ' + f'HTTP {res.status_code} — {res.text}', + status_code=res.status_code, response_text=res.text, + resource_type=resource_type, resource_label=resource_label, + ) + location = res.headers.get('Location') + if location is None: + raise MissingLocationHeaderError( + f'Created {resource_type}{named} (HTTP {res.status_code}) but the ' + f'response carried no Location header, so its server-assigned id ' + f'cannot be determined. The resource was most likely created — ' + f're-discover rather than re-POSTing, to avoid a duplicate.', + status_code=res.status_code, response_text=res.text, + resource_type=resource_type, resource_label=resource_label, + ) + return location.split('/')[-1] + + class SchemaFetchWarning(UserWarning): """A datastream/control-stream schema fetch or parse failed during `Node.discover_systems` / `System.discover_datastreams` / diff --git a/src/oshconnect/resources/datastream.py b/src/oshconnect/resources/datastream.py index 7a21477..95c1ed1 100644 --- a/src/oshconnect/resources/datastream.py +++ b/src/oshconnect/resources/datastream.py @@ -32,7 +32,7 @@ ) from ..swe_binary import SWEBinaryCodec from ..timemanagement import TimeInstant -from .base import StreamableModes, StreamableResource +from .base import StreamableModes, StreamableResource, new_resource_id_from_response if TYPE_CHECKING: from ..node import Node @@ -101,16 +101,19 @@ def create_observation(self, obs_data: dict) -> ObservationResource: def insert_observation_dict(self, obs_data: dict): """POST an observation dict to ``/datastreams/{id}/observations``. - :raises Exception: if the server returns a non-OK response. + :return: The new observation's server-assigned id. + :raises ResourceInsertError: if the server returns a non-OK + response. Unlike the previous bare ``Exception``, this carries + the status code alongside the body. + :raises MissingLocationHeaderError: if the POST succeeded but no + ``Location`` header came back. """ res = self._parent_node.get_api_helper().create_resource(APIResourceTypes.OBSERVATION, obs_data, parent_res_id=self._resource_id, req_headers={'Content-Type': 'application/json'}) - if res.ok: - obs_id = res.headers['Location'].split('/')[-1] - return obs_id - else: - raise Exception(f'Failed to insert observation: {res.text}') + return new_resource_id_from_response( + res, resource_type='observation', + resource_label=self._underlying_resource.name) def start(self): """Start the datastream. PULL/BIDIRECTIONAL subscribes to the diff --git a/src/oshconnect/resources/system.py b/src/oshconnect/resources/system.py index 2a15344..8747429 100644 --- a/src/oshconnect/resources/system.py +++ b/src/oshconnect/resources/system.py @@ -21,6 +21,7 @@ from ..csapi4py.constants import APIResourceTypes, ContentTypes from ..encoding import JSONEncoding +from ..exceptions import ResourceDiscoveryError from ..resource_datamodels import ControlStreamResource, DatastreamResource, SystemResource from ..schema_datamodels import ( JSONCommandSchema, SWEBinaryDatastreamRecordSchema, @@ -29,7 +30,7 @@ ) from ..swe_components import DataRecordSchema from ..timemanagement import TimeInstant, TimePeriod, TimeUtils -from .base import SchemaFetchWarning, StreamableResource +from .base import SchemaFetchWarning, StreamableResource, new_resource_id_from_response from .controlstream import ControlStream from .datastream import Datastream @@ -173,6 +174,13 @@ def discover_datastreams(self) -> list[Datastream]: api = self._parent_node.get_api_helper() res = api.get_resource(APIResourceTypes.SYSTEM, self._resource_id, APIResourceTypes.DATASTREAM) + if not res.ok: + raise ResourceDiscoveryError( + f'Failed to list datastreams for system {self._resource_id!r}: ' + f'HTTP {res.status_code} — {res.text}', + status_code=res.status_code, response_text=res.text, + resource_type='datastream', resource_label=self.label, + ) datastream_json = res.json()['items'] datastreams = [] @@ -238,6 +246,13 @@ def discover_controlstreams(self) -> list[ControlStream]: api = self._parent_node.get_api_helper() res = api.get_resource(APIResourceTypes.SYSTEM, self._resource_id, APIResourceTypes.CONTROL_CHANNEL) + if not res.ok: + raise ResourceDiscoveryError( + f'Failed to list control streams for system ' + f'{self._resource_id!r}: HTTP {res.status_code} — {res.text}', + status_code=res.status_code, response_text=res.text, + resource_type='control stream', resource_label=self.label, + ) controlstream_json = res.json()['items'] controlstreams = [] @@ -394,14 +409,9 @@ def add_insert_datastream(self, datastream_schema: DatastreamResource): req_headers={'Content-Type': ContentTypes.JSON.value}, parent_res_id=self._resource_id) - if res.ok: - datastream_id = res.headers['Location'].split('/')[-1] - datastream_schema.ds_id = datastream_id - else: - raise Exception( - f'Failed to create datastream {datastream_schema.name!r}: ' - f'HTTP {res.status_code} — {res.text}' - ) + datastream_schema.ds_id = new_resource_id_from_response( + res, resource_type='datastream', + resource_label=datastream_schema.name) new_ds = Datastream(self._parent_node, datastream_schema) new_ds.set_parent_resource_id(self._underlying_resource.system_id) @@ -442,14 +452,9 @@ def add_insert_controlstream(self, controlstream_resource: ControlStreamResource parent_res_id=self._resource_id, ) - if res.ok: - cs_id = res.headers['Location'].split('/')[-1] - controlstream_resource.cs_id = cs_id - else: - raise Exception( - f'Failed to create control stream {controlstream_resource.name!r}: ' - f'HTTP {res.status_code} — {res.text}' - ) + controlstream_resource.cs_id = new_resource_id_from_response( + res, resource_type='control stream', + resource_label=controlstream_resource.name) new_cs = ControlStream(node=self._parent_node, controlstream_resource=controlstream_resource) new_cs.set_parent_resource_id(self._underlying_resource.system_id) @@ -526,14 +531,9 @@ def add_and_insert_control_stream(self, control_stream_record_schema: DataRecord control_stream_resource.model_dump_json(by_alias=True, exclude_none=True), req_headers={'Content-Type': 'application/json'}, parent_res_id=self._resource_id) - if res.ok: - control_channel_id = res.headers['Location'].split('/')[-1] - control_stream_resource.cs_id = control_channel_id - else: - raise Exception( - f'Failed to create control stream {control_stream_resource.name!r}: ' - f'HTTP {res.status_code} — {res.text}' - ) + control_stream_resource.cs_id = new_resource_id_from_response( + res, resource_type='control stream', + resource_label=control_stream_resource.name) new_cs = ControlStream(node=self._parent_node, controlstream_resource=control_stream_resource) new_cs.set_parent_resource_id(self._underlying_resource.system_id) @@ -564,17 +564,11 @@ def insert_self(self): body_resource.model_dump_json(by_alias=True, exclude_none=True), req_headers={'Content-Type': 'application/sml+json'}) - if res.ok: - location = res.headers['Location'] - sys_id = location.split('/')[-1] - 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}' - ) + sys_id = new_resource_id_from_response( + res, resource_type='system', resource_label=self.label) + self._resource_id = sys_id + if self._underlying_resource is not None: + self._underlying_resource.system_id = sys_id 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 6193487..e488697 100644 --- a/tests/test_csapi_serialization.py +++ b/tests/test_csapi_serialization.py @@ -21,6 +21,12 @@ from pydantic import ValidationError from oshconnect import Node +from oshconnect.exceptions import ( + ConfigurationError, + MissingLocationHeaderError, + ResourceDiscoveryError, + ResourceInsertError, +) from oshconnect.resource_datamodels import ( ControlStreamResource, DatastreamResource, @@ -324,10 +330,22 @@ def test_insert_self_raises_on_failed_post(node, monkeypatch): # 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"): + with pytest.raises(ResourceInsertError, match=r"HTTP 500"): + sys.insert_self() + with pytest.raises(ResourceInsertError, match=r"disk full"): sys.insert_self() - with pytest.raises(Exception, match=r"disk full"): + + # ...and they're also reachable as structured attributes, so callers + # can branch on the status code without parsing the message. + try: sys.insert_self() + except ResourceInsertError as e: + assert e.status_code == 500 + assert "disk full" in e.response_text + assert e.resource_type == "system" + assert e.resource_label == "Doomed" + # Back-compat: pre-existing `except Exception:` handlers still catch. + assert isinstance(e, Exception) def test_add_system_does_not_attach_on_failed_insert(node, monkeypatch): @@ -339,11 +357,90 @@ def test_add_system_does_not_attach_on_failed_insert(node, monkeypatch): capture_request(monkeypatch, "post", response=MockResponse(status=500)) - with pytest.raises(Exception, match=r"Failed to insert system"): + with pytest.raises(ResourceInsertError, match=r"Failed to create system"): node.add_system(sys, insert_resource=True) assert sys not in node.systems() +def test_repr_of_credential_less_node_does_not_raise(): + """`Node` is a dataclass whose generated `__repr__` reads every declared + field, but `_basic_auth` was only assigned when credentials were passed + — so `repr()` on any anonymous node raised AttributeError. That poisoned + error messages and pytest failure reports (which repr their fixtures) + with a confusing secondary exception.""" + anon = Node(protocol="http", address="localhost", port=8282) + assert anon._basic_auth is None + assert "Node" in repr(anon) # the point: it doesn't raise + + secure = Node(protocol="http", address="localhost", port=8282, + username="u", password="p") + assert secure._basic_auth is not None + assert "Node" in repr(secure) + + +def test_unregistered_node_raises_instead_of_silent_noop(node): + """`add_system_to_node` / `create_and_insert_system` used to guard on + `if target_node in self._nodes:` with no else, so an unregistered node + made them fall off the end and return None — nothing POSTed, nothing + attached, no error. `create_and_insert_system` is documented to return + the created system, so callers dereferenced the None far from the + mistake. See GitHub issue #43.""" + from oshconnect import OSHConnect + + osh = OSHConnect(name="unregistered-node-test") + sys = System(label="Orphan", urn="urn:test:orphan:1", parent_node=node) + + # `node` was never passed to osh.add_node(...) + with pytest.raises(ConfigurationError, match=r"not registered"): + osh.add_system_to_node(sys, node, insert_resource=True) + + with pytest.raises(ConfigurationError, match=r"add_node\(\)"): + osh.create_and_insert_system( + {"label": "Orphan2", "urn": "urn:test:orphan:2"}, node) + + +def test_insert_self_raises_on_2xx_without_location(node, monkeypatch): + """A 2xx with no `Location` header used to raise a bare + `KeyError: 'Location'`, whose traceback pointed at a dict lookup and + gave no hint the POST had actually succeeded. It now raises a typed + error saying so. See GitHub issue #44.""" + sys = System(label="NoLoc", urn="urn:test:noloc:1", parent_node=node) + + # 201 Created, but no Location header (spec-legal; also what some + # proxies produce after stripping or rewriting headers). + capture_request(monkeypatch, "post", response=MockResponse(status=201)) + + with pytest.raises(MissingLocationHeaderError, + match=r"no Location header"): + sys.insert_self() + + # It's a ResourceInsertError too, so callers that don't care about the + # distinction can catch the broader type. + with pytest.raises(ResourceInsertError): + sys.insert_self() + + +def test_discover_systems_raises_on_failed_listing(node, monkeypatch): + """A failed listing must be distinguishable from an empty server. + Previously both were falsy, so `for s in node.discover_systems() or []:` + turned a 401 into a silent zero-iteration loop. See GitHub issue #49.""" + capture_request(monkeypatch, "get", response=MockResponse( + payload={"error": "unauthorized"}, status=401)) + + with pytest.raises(ResourceDiscoveryError, match=r"HTTP 401") as excinfo: + node.discover_systems() + assert excinfo.value.status_code == 401 + + +def test_discover_systems_returns_empty_list_when_server_has_none(node, monkeypatch): + """The other half of #49: a genuinely empty server still returns a + plain empty list rather than raising.""" + capture_request(monkeypatch, "get", response=MockResponse( + payload={"items": []}, status=200)) + + assert node.discover_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 diff --git a/uv.lock b/uv.lock index a619a5a..9ada52a 100644 --- a/uv.lock +++ b/uv.lock @@ -570,7 +570,7 @@ wheels = [ [[package]] name = "oshconnect" -version = "0.5.3a1" +version = "0.5.5a0" source = { virtual = "." } dependencies = [ { name = "pydantic" }, From 305bc622ccded95697031818af4346cea5aad2bb Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Tue, 28 Jul 2026 15:40:25 -0500 Subject: [PATCH 2/3] Log to named module loggers, add package NullHandler Closes #48, #45. Library no longer writes to the consumer's root logger; adds mocked CI coverage for the cross-node sync insert path. --- README.md | 25 +++++ pyproject.toml | 2 +- src/oshconnect/__init__.py | 25 +++++ src/oshconnect/events/handler.py | 4 +- src/oshconnect/oshconnectapi.py | 6 +- src/oshconnect/resources/base.py | 43 +++---- src/oshconnect/resources/controlstream.py | 8 +- src/oshconnect/resources/datastream.py | 8 +- src/oshconnect/resources/system.py | 8 +- tests/test_logging.py | 131 ++++++++++++++++++++++ tests/test_node_sync_mocked.py | 80 +++++++++++++ uv.lock | 2 +- 12 files changed, 309 insertions(+), 33 deletions(-) create mode 100644 tests/test_logging.py create mode 100644 tests/test_node_sync_mocked.py diff --git a/README.md b/README.md index 976baf3..faf9a66 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,31 @@ CI (`.github/workflows/tests.yaml`) runs the suite with `--cov` on every push across Python 3.12 / 3.13 / 3.14 and uploads `coverage.xml` as a workflow artifact (downloadable from the run page). +## Logging + +OSHConnect logs to the `oshconnect` logger namespace — every module uses +`logging.getLogger(__name__)`, so records arrive as `oshconnect.node`, +`oshconnect.resources.system`, `oshconnect.csapi4py.mqtt`, and so on. + +The library never configures logging on your behalf: the package logger +carries a `NullHandler`, so nothing is emitted until you opt in. One call +controls the whole library without touching the root logger or any other +package: + +```python +import logging + +logging.basicConfig(level=logging.INFO) # your app's choice +logging.getLogger("oshconnect").setLevel(logging.DEBUG) # verbose OSHConnect +logging.getLogger("oshconnect.csapi4py.mqtt").setLevel(logging.WARNING) # ...but quiet MQTT +``` + +Note that discovery additionally raises `SchemaFetchWarning` through the +`warnings` module when an individual datastream or control-stream schema +fetch fails. That's deliberate and separate from logging — discovery +doesn't raise on per-resource schema failures, so the warning is how you +catch them programmatically (`warnings.catch_warnings`). + ## Documentation Coverage [`interrogate`](https://interrogate.readthedocs.io/) reports what fraction of diff --git a/pyproject.toml b/pyproject.toml index fc0b5b7..e682ac2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "oshconnect" -version = "0.5.5a0" +version = "0.5.5a1" 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/__init__.py b/src/oshconnect/__init__.py index 2f0f1f7..3a3c728 100644 --- a/src/oshconnect/__init__.py +++ b/src/oshconnect/__init__.py @@ -84,6 +84,13 @@ from .csapi4py.constants import ObservationFormat, APIResourceTypes, ContentTypes __all__ = [ + # Exceptions + "OSHConnectError", + "ConfigurationError", + "ResourceRequestError", + "ResourceInsertError", + "MissingLocationHeaderError", + "ResourceDiscoveryError", # Core resources "OSHConnect", "Node", @@ -153,3 +160,21 @@ "DataStore", "SQLiteDataStore", ] + +# --------------------------------------------------------------------------- +# Logging hygiene (kept last so it doesn't push the imports above out of +# top-of-file position, which flake8 flags as E402). +# +# A library must not configure logging for the application embedding it. +# Attaching a NullHandler to the package logger keeps OSHConnect from +# implicitly installing a stderr handler on the ROOT logger the first time +# it warns. Consumers opt in explicitly: +# +# logging.getLogger("oshconnect").setLevel(logging.DEBUG) +# +# Every module logs to `oshconnect.` via logging.getLogger(__name__), +# so that single call controls the whole library and nothing else. +# --------------------------------------------------------------------------- +import logging as _logging # noqa: E402 + +_logging.getLogger(__name__).addHandler(_logging.NullHandler()) diff --git a/src/oshconnect/events/handler.py b/src/oshconnect/events/handler.py index 7a340d4..c2a1370 100644 --- a/src/oshconnect/events/handler.py +++ b/src/oshconnect/events/handler.py @@ -14,6 +14,8 @@ from .core import DefaultEventTypes, Event from .listeners import CallbackListener, IEventListener +logger = logging.getLogger(__name__) + class EventHandler(object): """ @@ -115,7 +117,7 @@ def publish(self, evt: Event): try: listener.handle_events(evt) except Exception as e: - logging.error("Error in event listener %s: %s", listener, e) + logger.error("Error in event listener %s: %s", listener, e) finally: self.publish_lock = False self.commit_changes() diff --git a/src/oshconnect/oshconnectapi.py b/src/oshconnect/oshconnectapi.py index eb4abcd..82a5aad 100644 --- a/src/oshconnect/oshconnectapi.py +++ b/src/oshconnect/oshconnectapi.py @@ -19,6 +19,8 @@ from .styling import Styling from .timemanagement import TemporalModes, TimeManagement, TimePeriod +logger = logging.getLogger(__name__) + class OSHConnect: _name: str @@ -54,7 +56,7 @@ def __init__(self, name: str, datastore: DataStore = None, **kwargs): self._datagroups = [] self._tasks = [] self._playback_mode = TemporalModes.REAL_TIME - logging.info(f"OSHConnect instance {name} created") + logger.info(f"OSHConnect instance {name} created") self._session_manager = SessionManager() self._event_bus = EventHandler() @@ -96,7 +98,7 @@ def remove_node(self, node_id: str): ) def save_config(self): - logging.info(f"Saving configuration for {self._name}") + logger.info(f"Saving configuration for {self._name}") data = {} for node in self._nodes: diff --git a/src/oshconnect/resources/base.py b/src/oshconnect/resources/base.py index dd4b8d7..774e5c5 100644 --- a/src/oshconnect/resources/base.py +++ b/src/oshconnect/resources/base.py @@ -49,6 +49,8 @@ from ..resource_datamodels import SystemResource from ..timemanagement import TimePeriod +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from ..node import Node @@ -98,16 +100,19 @@ def new_resource_id_from_response(res, *, resource_type: str, class SchemaFetchWarning(UserWarning): """A datastream/control-stream schema fetch or parse failed during - `Node.discover_systems` / `System.discover_datastreams` / - `System.discover_controlstreams`. + `System.discover_datastreams` / `System.discover_controlstreams`. + + (`Node.discover_systems` is *not* a source: it lists systems and never + fetches a schema. The two `System` methods above are the only ones + that do.) Discovery deliberately does not raise on per-resource schema failures — one broken schema would otherwise poison the entire listing. The matching wrapper is still appended (with `record_schema` / `command_schema` left as ``None``), but the original exception is surfaced both here - (via ``warnings.warn``) and in the root logger at ERROR level (with a - full traceback via ``exc_info=True``). Filter or capture this category - if you want to react programmatically. + (via ``warnings.warn``) and on the ``oshconnect.resources.system`` + logger at ERROR level (with a full traceback via ``exc_info=True``). + Filter or capture this category if you want to react programmatically. """ @@ -228,7 +233,7 @@ def start(self): tasks. Logs and returns silently if `initialize` hasn't been called. """ if self._status != Status.INITIALIZED.value: - logging.warning(f"Streamable resource {self._id} not initialized. Call initialize() first.") + logger.warning(f"Streamable resource {self._id} not initialized. Call initialize() first.") return self._status = Status.STARTING.value self._status = Status.STARTED.value @@ -244,13 +249,13 @@ async def stream(self): try: async with session.ws_connect(self.ws_url, auth=self._parent_node.get_basicauth()) as ws: - logging.info(f"Streamable resource {self._id} started.") + logger.info(f"Streamable resource {self._id} started.") read_task = asyncio.create_task(self._read_from_ws(ws)) write_task = asyncio.create_task(self._write_to_ws(ws)) await asyncio.gather(read_task, write_task) except Exception as e: - logging.error(f"Error in streamable resource {self._id}: {e}") - logging.error(traceback.format_exc()) + logger.error(f"Error in streamable resource {self._id}: {e}") + logger.error(traceback.format_exc()) def init_mqtt(self): """Wire the MQTT subscribe-acknowledged callback if a client exists. @@ -260,7 +265,7 @@ def init_mqtt(self): `ControlStream.init_mqtt`). """ if self._mqtt_client is None: - logging.warning(f"No MQTT client configured for streamable resource {self._id}.") + logger.warning(f"No MQTT client configured for streamable resource {self._id}.") return self._mqtt_client.set_on_subscribe(self._default_on_subscribe) @@ -268,7 +273,7 @@ def init_mqtt(self): # self.get_mqtt_topic() def _default_on_subscribe(self, client, userdata, mid, granted_qos, properties): - logging.debug("OSH Subscribed: mid=%s granted_qos=%s", mid, granted_qos) + logger.debug("OSH Subscribed: mid=%s granted_qos=%s", mid, granted_qos) def get_mqtt_topic(self, subresource: APIResourceTypes | None = None, data_topic: bool = True, format: str | None = None): @@ -474,7 +479,7 @@ def subscribe_events(self, callback=None, qos: int = 0) -> str: :return: The event topic string that was subscribed to. """ if self._mqtt_client is None: - logging.warning(f"No MQTT client configured for streamable resource {self._id}.") + logger.warning(f"No MQTT client configured for streamable resource {self._id}.") return "" event_topic = self.get_event_topic() cb = callback if callback is not None else self._mqtt_sub_callback @@ -587,29 +592,29 @@ def subscribe_mqtt(self, topic: str, qos: int = 0): :param qos: MQTT QoS level. Default 0. """ if self._mqtt_client is None: - logging.warning(f"No MQTT client configured for streamable resource {self._id}.") + logger.warning(f"No MQTT client configured for streamable resource {self._id}.") return self._mqtt_client.subscribe(topic, qos=qos, msg_callback=self._mqtt_sub_callback) def _publish_mqtt(self, topic, payload): if self._mqtt_client is None: - logging.warning("No MQTT client configured for streamable resource %s.", self._id) + logger.warning("No MQTT client configured for streamable resource %s.", self._id) return - logging.debug("Publishing to MQTT topic %s", topic) + logger.debug("Publishing to MQTT topic %s", topic) self._mqtt_client.publish(topic, payload, qos=0) async def _write_to_mqtt(self): while self._status == Status.STARTED.value: try: msg = self._outbound_deque.popleft() - logging.debug("Publishing outbound message from %s", self._id) + logger.debug("Publishing outbound message from %s", self._id) self._publish_mqtt(self._topic, msg) except IndexError: await asyncio.sleep(0.05) except Exception as e: - logging.error("Error in Write To MQTT %s: %s\n%s", self._id, e, traceback.format_exc()) + logger.error("Error in Write To MQTT %s: %s\n%s", self._id, e, traceback.format_exc()) if self._status == Status.STOPPED.value: - logging.debug("MQTT write task stopping: resource %s stopped", self._id) + logger.debug("MQTT write task stopping: resource %s stopped", self._id) def publish(self, payload, topic: str = None): """ @@ -639,7 +644,7 @@ def subscribe(self, topic=None, callback=None, qos=0): self._mqtt_client.subscribe(t, qos=qos, msg_callback=callback) def _mqtt_sub_callback(self, client, userdata, msg): - logging.debug("Received MQTT message on topic %s (%s bytes)", msg.topic, len(msg.payload)) + logger.debug("Received MQTT message on topic %s (%s bytes)", msg.topic, len(msg.payload)) # Appends to right of deque self._inbound_deque.append(msg.payload) self._emit_inbound_event(msg) diff --git a/src/oshconnect/resources/controlstream.py b/src/oshconnect/resources/controlstream.py index fb20d31..5f19a1d 100644 --- a/src/oshconnect/resources/controlstream.py +++ b/src/oshconnect/resources/controlstream.py @@ -26,6 +26,8 @@ from ..resource_datamodels import ControlStreamResource from .base import StreamableModes, StreamableResource +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from ..node import Node @@ -123,10 +125,10 @@ def start(self): loop = asyncio.get_running_loop() loop.create_task(self._write_to_mqtt()) except RuntimeError: - logging.warning("No running event loop — MQTT write task for %s not started. " - "Call start() from within an async context.", self._id) + logger.warning("No running event loop — MQTT write task for %s not started. " + "Call start() from within an async context.", self._id) except Exception as e: - logging.error("Error starting MQTT write task for %s: %s\n%s", self._id, e, traceback.format_exc()) + logger.error("Error starting MQTT write task for %s: %s\n%s", self._id, e, traceback.format_exc()) def get_inbound_deque(self) -> deque: """Return the deque receiving inbound command payloads.""" diff --git a/src/oshconnect/resources/datastream.py b/src/oshconnect/resources/datastream.py index 95c1ed1..a779098 100644 --- a/src/oshconnect/resources/datastream.py +++ b/src/oshconnect/resources/datastream.py @@ -34,6 +34,8 @@ from ..timemanagement import TimeInstant from .base import StreamableModes, StreamableResource, new_resource_id_from_response +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from ..node import Node @@ -129,10 +131,10 @@ def start(self): loop = asyncio.get_running_loop() loop.create_task(self._write_to_mqtt()) except RuntimeError: - logging.warning("No running event loop — MQTT write task for %s not started. " - "Call start() from within an async context.", self._id) + logger.warning("No running event loop — MQTT write task for %s not started. " + "Call start() from within an async context.", self._id) except Exception as e: - logging.error("Error starting MQTT write task for %s: %s\n%s", self._id, e, traceback.format_exc()) + logger.error("Error starting MQTT write task for %s: %s\n%s", self._id, e, traceback.format_exc()) def init_mqtt(self): """Set ``self._topic`` to the datastream's observation data topic diff --git a/src/oshconnect/resources/system.py b/src/oshconnect/resources/system.py index 8747429..28a11b3 100644 --- a/src/oshconnect/resources/system.py +++ b/src/oshconnect/resources/system.py @@ -34,6 +34,8 @@ from .controlstream import ControlStream from .datastream import Datastream +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from ..node import Node @@ -195,7 +197,7 @@ def discover_datastreams(self) -> list[Datastream]: f"supported schema format (have: {datastream_objs.formats}); " "skipping schema fetch." ) - logging.warning(msg) + logger.warning(msg) warnings.warn(msg, SchemaFetchWarning, stacklevel=2) else: try: @@ -211,7 +213,7 @@ def discover_datastreams(self) -> list[Datastream]: f"Failed to fetch {obs_format} schema for datastream " f"{datastream_objs.ds_id}: {type(e).__name__}: {e}" ) - logging.error(msg, exc_info=True) + logger.error(msg, exc_info=True) warnings.warn(msg, SchemaFetchWarning, stacklevel=2) datastreams.append(new_ds) @@ -274,7 +276,7 @@ def discover_controlstreams(self) -> list[ControlStream]: f"Failed to fetch command schema for control stream " f"{controlstream_objs.cs_id}: {type(e).__name__}: {e}" ) - logging.error(msg, exc_info=True) + logger.error(msg, exc_info=True) warnings.warn(msg, SchemaFetchWarning, stacklevel=2) controlstreams.append(new_cs) diff --git a/tests/test_logging.py b/tests/test_logging.py new file mode 100644 index 0000000..cb2f791 --- /dev/null +++ b/tests/test_logging.py @@ -0,0 +1,131 @@ +"""Library logging hygiene — GitHub issue #48. + +OSHConnect used to log via the module-level `logging.warning(...)` / +`logging.error(...)` functions, which write to the **root** logger. Two +consequences for anything embedding the library: + +1. An application could not raise OSHConnect to DEBUG or silence its + warnings without reconfiguring the root logger, affecting every other + library in the process. +2. Calling `logging.warning(...)` on the root logger auto-installs a + stderr handler when none exists — a library writing to its consumer's + stderr uninvited. + +These tests pin the fix: every module logs to `oshconnect.`, and +the package logger carries a NullHandler so the library never configures +logging on the application's behalf. +""" +from __future__ import annotations + +import logging +import pkgutil +import re +from pathlib import Path + +import oshconnect + +SRC_ROOT = Path(__file__).resolve().parents[1] / "src" / "oshconnect" + +# Matches a root-logger call: `logging.warning(`, `logging.error(`, etc., +# but not `logger.warning(` / `self.logging.error(`. +ROOT_LOGGER_CALL = re.compile( + r"(?:^|[^.\w])logging\.(debug|info|warning|error|exception)\s*\(") + + +def test_no_module_logs_to_the_root_logger(): + """Regression guard: a new `logging.warning(...)` anywhere under + src/oshconnect would silently reintroduce the root-logger leak, and + nothing else in the suite would notice.""" + offenders = [] + for path in SRC_ROOT.rglob("*.py"): + for lineno, line in enumerate(path.read_text().splitlines(), 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + if ROOT_LOGGER_CALL.search(line): + rel = path.relative_to(SRC_ROOT.parent.parent) + offenders.append(f"{rel}:{lineno}: {stripped}") + + assert not offenders, ( + "These call the root logger directly; use a module-level " + "`logger = logging.getLogger(__name__)` instead:\n " + + "\n ".join(offenders) + ) + + +def test_package_logger_has_a_null_handler(): + """Without this, the first OSHConnect warning installs a stderr handler + on the root logger for the whole host application.""" + handlers = logging.getLogger("oshconnect").handlers + assert any(isinstance(h, logging.NullHandler) for h in handlers), ( + f"oshconnect package logger has no NullHandler (handlers={handlers})" + ) + + +def test_logging_modules_use_their_own_dunder_name(): + """Each module's logger must be `oshconnect.` so that + configuring `oshconnect` controls all of them and nothing else.""" + mismatched = [] + for path in SRC_ROOT.rglob("*.py"): + text = path.read_text() + if "logging.getLogger(" not in text: + continue + for lineno, line in enumerate(text.splitlines(), 1): + if "logging.getLogger(" not in line or line.strip().startswith("#"): + continue + # __name__ is the only correct argument; a hardcoded string + # drifts the moment a module is renamed or moved. + if "__name__" not in line: + rel = path.relative_to(SRC_ROOT.parent.parent) + mismatched.append(f"{rel}:{lineno}: {line.strip()}") + + assert not mismatched, ( + "getLogger() should be passed __name__:\n " + "\n ".join(mismatched) + ) + + +def test_consumer_can_silence_oshconnect_without_touching_root(caplog): + """The actual user-facing win: one setLevel on the package logger + controls the whole library, and OSHConnect records never land on a + handler the consumer didn't ask for.""" + pkg_logger = logging.getLogger("oshconnect") + child = logging.getLogger("oshconnect.resources.base") + + original_level = pkg_logger.level + try: + pkg_logger.setLevel(logging.CRITICAL) + # `caplog.set_level` would override the very thing under test, so + # attach at the root and assert on propagation instead. + with caplog.at_level(logging.DEBUG, logger="oshconnect"): + pass + pkg_logger.setLevel(logging.CRITICAL) + caplog.clear() + child.warning("should be suppressed by the package-level setLevel") + assert not [r for r in caplog.records if r.name.startswith("oshconnect")] + + pkg_logger.setLevel(logging.DEBUG) + caplog.clear() + with caplog.at_level(logging.DEBUG): + child.warning("should now be visible") + assert any(r.name == "oshconnect.resources.base" + for r in caplog.records) + finally: + pkg_logger.setLevel(original_level) + + +def test_every_submodule_imports_cleanly(): + """The NullHandler wiring lives in `oshconnect/__init__.py`; make sure + adding it didn't break import of any submodule (e.g. via a cycle).""" + failures = [] + for mod in pkgutil.walk_packages(oshconnect.__path__, + prefix="oshconnect."): + name = mod.name + try: + __import__(name) + except Exception as e: # noqa: BLE001 - reporting, not handling + # Transport/codec extras raise a helpful RuntimeError when the + # optional dep is absent; that's by design, not an import break. + if isinstance(e, ImportError) and "oshconnect[" in str(e): + continue + failures.append(f"{name}: {type(e).__name__}: {e}") + assert not failures, "submodules failed to import:\n " + "\n ".join(failures) diff --git a/tests/test_node_sync_mocked.py b/tests/test_node_sync_mocked.py new file mode 100644 index 0000000..c99941e --- /dev/null +++ b/tests/test_node_sync_mocked.py @@ -0,0 +1,80 @@ +"""Mocked coverage for the cross-node sync insert path — GitHub issue #45. + +`tests/test_node_to_node_sync.py` exercises this flow properly, but every +test in it is `@pytest.mark.network` and CI runs `-m "not network"`. That +left `System.insert_self()`'s server-facing path — and the helper that +calls it during sync — with no CI coverage at all, which matters now that +#42 made a failed POST raise rather than return quietly. + +These tests import the real `_ensure_dest_system` helper from that module +(the network markers are per-test, so importing is safe) and drive it +against mocked HTTP, so a regression in the raise/no-raise contract fails +in CI instead of waiting for someone to run the live suite. +""" +from __future__ import annotations + +import pytest + +from oshconnect import Node +from oshconnect.exceptions import ResourceInsertError +from tests.helpers import MockResponse, capture_request +from tests.test_node_to_node_sync import _ensure_dest_system + + +@pytest.fixture +def node() -> Node: + return Node(protocol="http", address="localhost", port=8282) + + +def test_ensure_dest_system_reuses_an_existing_system(node, monkeypatch): + """When the destination already has a system, sync must adopt it and + report `created_by_us=False` so cleanup doesn't delete a system the + test didn't create.""" + capture_request(monkeypatch, "get", response=MockResponse( + payload={"items": [{ + "type": "PhysicalSystem", + "id": "existing-sys-1", + "uniqueId": "urn:test:existing:1", + "label": "Already There", + }]}, + status=200, + )) + + system, created = _ensure_dest_system(node) + + assert created is False + assert system._resource_id == "existing-sys-1" + + +def test_ensure_dest_system_creates_one_when_none_exist(node, monkeypatch): + """Empty destination → build a System locally and POST it, picking the + new id off the Location header.""" + capture_request(monkeypatch, "get", response=MockResponse( + payload={"items": []}, status=200)) + captured = capture_request(monkeypatch, "post", response=MockResponse( + status=201, + headers={"Location": + "http://localhost:8282/sensorhub/api/systems/new-sys-9"}, + )) + + system, created = _ensure_dest_system(node) + + assert created is True + assert system._resource_id == "new-sys-9" + assert captured["called"] is True + + +def test_ensure_dest_system_propagates_a_failed_insert(node, monkeypatch): + """The regression this file exists for: a rejected POST during sync + must surface as a typed error, not leave a system with no server-side + id for a later call to trip over. See GitHub issues #42 and #45.""" + capture_request(monkeypatch, "get", response=MockResponse( + payload={"items": []}, status=200)) + capture_request(monkeypatch, "post", response=MockResponse( + payload={"error": "no space left on device"}, status=500)) + + with pytest.raises(ResourceInsertError) as excinfo: + _ensure_dest_system(node) + + assert excinfo.value.status_code == 500 + assert "no space left" in excinfo.value.response_text diff --git a/uv.lock b/uv.lock index 9ada52a..ac4f337 100644 --- a/uv.lock +++ b/uv.lock @@ -570,7 +570,7 @@ wheels = [ [[package]] name = "oshconnect" -version = "0.5.5a0" +version = "0.5.5a1" source = { virtual = "." } dependencies = [ { name = "pydantic" }, From 3ee1db0a27d31b531be53a97128d0a8ad3f57359 Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Tue, 28 Jul 2026 15:44:36 -0500 Subject: [PATCH 3/3] Assert effective log level directly instead of via caplog records --- tests/test_logging.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/tests/test_logging.py b/tests/test_logging.py index cb2f791..dc1609a 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -93,20 +93,27 @@ def test_consumer_can_silence_oshconnect_without_touching_root(caplog): original_level = pkg_logger.level try: + # Assert on effective level rather than captured records: caplog + # installs its own handler and manipulates levels, so a "no records + # captured" assertion would also pass if the call simply never ran. + # `isEnabledFor` fails only for the reason this test is about. pkg_logger.setLevel(logging.CRITICAL) - # `caplog.set_level` would override the very thing under test, so - # attach at the root and assert on propagation instead. - with caplog.at_level(logging.DEBUG, logger="oshconnect"): - pass - pkg_logger.setLevel(logging.CRITICAL) - caplog.clear() - child.warning("should be suppressed by the package-level setLevel") - assert not [r for r in caplog.records if r.name.startswith("oshconnect")] + assert child.isEnabledFor(logging.WARNING) is False, ( + "setting the package logger to CRITICAL must suppress a child " + "module's warnings — that's the whole point of the namespace" + ) pkg_logger.setLevel(logging.DEBUG) + assert child.isEnabledFor(logging.DEBUG) is True + + # And the level genuinely comes from the `oshconnect` parent, not + # from the root logger — the leak this replaced. + assert child.level == logging.NOTSET + assert child.getEffectiveLevel() == logging.DEBUG + caplog.clear() with caplog.at_level(logging.DEBUG): - child.warning("should now be visible") + child.warning("visible once the package logger allows it") assert any(r.name == "oshconnect.resources.base" for r in caplog.records) finally: