Handle import errors more cleanly

This uses a WrapValidator to improve the handling of errors in an ImportString.

The problem, identified in #222, is that `TypeError` and `ValueError` are treated specially by `pydantic`.
This means that if either of those exceptions are raised during module import, we get a confusing validation error.

My wrap validator ensures that we get either a sensible import-related error (e.g. moule not found, name not found in module) or a single clear error stating that an error occurred during module import.

This does not dump a traceback, because doing so during validation tends to lead to very confusing messages.
However, it should make it obvious that the problem is an unimportable module, and point someone in the right direction.

Author: rwb27

src/labthings_fastapi/server/config_model.py

@@ -6,17 +6,64 @@
 start servers based on configuration files or strings.
 """
 
-from pydantic import BaseModel, Field, ImportString, AliasChoices, field_validator
+from pydantic import (
+    BaseModel,
+    Field,
+    ImportString,
+    AliasChoices,
+    field_validator,
+    ValidationError,
+    ValidatorFunctionWrapHandler,
+    WrapValidator,
+)
+from pydantic_core import PydanticCustomError
 from typing import Any, Annotated, TypeAlias
 from collections.abc import Mapping, Sequence, Iterable
 
 
+def contain_import_errors(value: Any, handler: ValidatorFunctionWrapHandler) -> Any:
+    """Prevent errors during import from causing odd validation errors.
+
+    This is used to wrap the pydantic ImportString validator, and ensures that any
+    module that won't import shows up with a single clear error.
+
+    :param value: The value being validated.
+    :param handler: The validator handler.
+
+    :return: The validated value.
+
+    :raises PydanticCustomError: if an import error occurs.
+    :raises Exception: if the ImportString logic raises a ValidationError
+        containing only import errors, this will be re-raised. All other
+        exceptions are wrapped in a PydanticCustomError.
+    """
+    try:
+        return handler(value)
+    except Exception as e:
+        # ImportErrors get wrapped as ValidationErrors, and that's fine:
+        # it results in a sensible error message.
+        if isinstance(e, ValidationError):
+            if all(err["type"] == "import_error" for err in e.errors()):
+                raise
+        raise PydanticCustomError(
+            "import_error",
+            "An exception was raised when importing '{name}'.",
+            {"name": str(value), "error": e},
+        ) from e
+
+
+ThingImportString = Annotated[
+    ImportString,
+    WrapValidator(contain_import_errors),
+]
+
+
 # The type: ignore below is a spurious warning about `kwargs`.
 # see https://github.com/pydantic/pydantic/issues/3125
 class ThingConfig(BaseModel):  # type: ignore[no-redef]
     r"""The information needed to add a `.Thing` to a `.ThingServer`\ ."""
 
-    cls: ImportString = Field(
+    cls: ThingImportString = Field(
         validation_alias=AliasChoices("cls", "class"),
         description="The Thing subclass to add to the server.",
     )
@@ -51,7 +98,7 @@ class ThingConfig(BaseModel):  # type: ignore[no-redef]
 ]
 
 
-ThingsConfig: TypeAlias = Mapping[ThingName, ThingConfig | ImportString]
+ThingsConfig: TypeAlias = Mapping[ThingName, ThingConfig | ThingImportString]
 
 
 class ThingServerConfig(BaseModel):

tests/test_server_config_model.py

@@ -2,17 +2,17 @@
 
 from pydantic import ValidationError
 import pytest
-from labthings_fastapi.server import config_model as cm
+from labthings_fastapi.server.config_model import ThingConfig, ThingServerConfig
 import labthings_fastapi.example_things
 from labthings_fastapi.example_things import MyThing
 
 
 def test_ThingConfig():
     """Test the ThingConfig model loads classes as expected."""
     # We should be able to create a valid config with just a class
-    direct = cm.ThingConfig(cls=labthings_fastapi.example_things.MyThing)
+    direct = ThingConfig(cls=labthings_fastapi.example_things.MyThing)
     # Equivalently, we should be able to pass a string
-    fromstr = cm.ThingConfig(cls="labthings_fastapi.example_things:MyThing")
+    fromstr = ThingConfig(cls="labthings_fastapi.example_things:MyThing")
     assert direct.cls is MyThing
     assert fromstr.cls is MyThing
     # In the absence of supplied arguments, default factories should be used
@@ -21,14 +21,14 @@ def test_ThingConfig():
     assert direct.thing_slots == {}
 
     with pytest.raises(ValidationError, match="No module named"):
-        cm.ThingConfig(cls="missing.module")
+        ThingConfig(cls="missing.module")
 
 
 VALID_THING_CONFIGS = {
     "direct": MyThing,
     "string": "labthings_fastapi.example_things:MyThing",
-    "model_d": cm.ThingConfig(cls=MyThing),
-    "model_s": cm.ThingConfig(cls="labthings_fastapi.example_things:MyThing"),
+    "model_d": ThingConfig(cls=MyThing),
+    "model_s": ThingConfig(cls="labthings_fastapi.example_things:MyThing"),
     "dict_d": {"cls": MyThing},
     "dict_da": {"class": MyThing},
     "dict_s": {"cls": "labthings_fastapi.example_things:MyThing"},
@@ -71,28 +71,49 @@ def test_ThingConfig():
 def test_ThingServerConfig():
     """Check validation of the whole server config."""
     # Things should be able to be specified as a string, a class, or a ThingConfig
-    config = cm.ThingServerConfig(things=VALID_THING_CONFIGS)
+    config = ThingServerConfig(things=VALID_THING_CONFIGS)
     assert len(config.thing_configs) == 8
     for v in config.thing_configs.values():
         assert v.cls is MyThing
 
     # When we validate from a dict, the same options work
-    config = cm.ThingServerConfig.model_validate({"things": VALID_THING_CONFIGS})
+    config = ThingServerConfig.model_validate({"things": VALID_THING_CONFIGS})
     assert len(config.thing_configs) == 8
     for v in config.thing_configs.values():
         assert v.cls is MyThing
 
     # Check invalid configs are picked up
     for spec in INVALID_THING_CONFIGS:
         with pytest.raises(ValidationError):
-            cm.ThingServerConfig(things={"thing": spec})
+            ThingServerConfig(things={"thing": spec})
 
     # Check valid names are allowed
     for name in VALID_THING_NAMES:
-        sc = cm.ThingServerConfig(things={name: MyThing})
+        sc = ThingServerConfig(things={name: MyThing})
         assert sc.thing_configs[name].cls is MyThing
 
     # Check bad names raise errors
     for name in INVALID_THING_NAMES:
         with pytest.raises(ValidationError):
-            cm.ThingServerConfig(things={name: MyThing})
+            ThingServerConfig(things={name: MyThing})
+
+
+def test_unimportable_module():
+    """Test that unimportable modules raise errors as expected."""
+    expected_message = "exception was raised when importing 'tests.unimportable"
+    with pytest.raises(ValidationError, match=expected_message):
+        # This checks RuntimErrors get reported with a single error
+        ThingConfig(cls="tests.unimportable.runtimeerror:SomeClass")
+    with pytest.raises(ValidationError, match=expected_message):
+        # This checks ValueErrors get reported with a single error
+        # rather than getting swallowed by a ValidationError
+        ThingConfig(cls="tests.unimportable.valueerror:SomeClass")
+    with pytest.raises(
+        ValidationError,
+        match="No module named 'tests.unimportable.missing'",
+    ):
+        # This checks normal ImportErrors get reported as usual
+        ThingConfig(cls="tests.unimportable.missing:SomeClass")
+    with pytest.raises(ValidationError, match="cannot import name 'MissingClass'"):
+        # This checks normal ImportErrors get reported as usual
+        ThingConfig(cls="tests.unimportable:MissingClass")

tests/unimportable/__init__.py

@@ -0,0 +1,5 @@
+"""A collection of submodules that can't be imported.
+
+This is to help test ImportString error handling.
+See test_server_config_model.py.
+"""

tests/unimportable/runtimeerror.py

@@ -0,0 +1,7 @@
+"""A module that can't be imported due to a runtimeerror.
+
+This is to help test ImportString error handling.
+See test_server_config_model.py.
+"""
+
+raise RuntimeError("This module should not be importable!")

tests/unimportable/valueerror.py

@@ -0,0 +1,7 @@
+"""This module raises a ValueError when imported.
+
+This is to help test ImportString error handling.
+See test_server_config_model.py.
+"""
+
+raise ValueError("This module should not be importable due to ValueError!")