Merge pull request #52 from Botts-Innovative-Research/fix/error-handling-and-logging

Typed exceptions and logging hygiene (umbrella #46)

Author: tipatterson-dev

README.md

@@ -102,6 +102,31 @@ and does not gate merges.
 Development happens on Python 3.14 (pinned in `.python-version`); 3.11 is the
 supported floor.
 
+## 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

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.

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "oshconnect"
-version = "0.5.4a0"
+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 = [

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
@@ -73,6 +84,13 @@
 from .csapi4py.constants import ObservationFormat, APIResourceTypes, ContentTypes
 
 __all__ = [
+    # Exceptions
+    "OSHConnectError",
+    "ConfigurationError",
+    "ResourceRequestError",
+    "ResourceInsertError",
+    "MissingLocationHeaderError",
+    "ResourceDiscoveryError",
     # Core resources
     "OSHConnect",
     "Node",
@@ -142,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.<module>` 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())

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

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",
+]

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