From 06988d15f46f737cffc603f9ca7118c9098cab80 Mon Sep 17 00:00:00 2001
From: Callum Forrester <callum.forrester@diamond.ac.uk>
Date: Fri, 26 Jul 2024 12:07:10 +0100
Subject: [PATCH] Make environment error message be a non-empty string or None
 (#571)

The code was using `None` or `""` in various places to denote the lack
of an error message. `None` is a better semantic fit, so this PR forces
the user to pass either `None` or a string with at least 1 character.
---
 docs/reference/openapi.yaml    |  1 +
 src/blueapi/service/model.py   |  4 +++-
 src/blueapi/service/runner.py  | 12 +++++-------
 tests/service/test_rest_api.py |  9 ++-------
 tests/service/test_runner.py   |  2 +-
 5 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/docs/reference/openapi.yaml b/docs/reference/openapi.yaml
index 2e92e6585..df19cd95e 100644
--- a/docs/reference/openapi.yaml
+++ b/docs/reference/openapi.yaml
@@ -39,6 +39,7 @@ components:
       properties:
         error_message:
           description: If present - error loading context
+          minLength: 1
           title: Error Message
           type: string
         initialized:
diff --git a/src/blueapi/service/model.py b/src/blueapi/service/model.py
index c16c6b2d9..191a342e0 100644
--- a/src/blueapi/service/model.py
+++ b/src/blueapi/service/model.py
@@ -145,5 +145,7 @@ class EnvironmentResponse(BlueapiBaseModel):
 
     initialized: bool = Field(description="blueapi context initialized")
     error_message: str | None = Field(
-        default=None, description="If present - error loading context"
+        default=None,
+        description="If present - error loading context",
+        min_length=1,
     )
diff --git a/src/blueapi/service/runner.py b/src/blueapi/service/runner.py
index a83665ba6..055dd3130 100644
--- a/src/blueapi/service/runner.py
+++ b/src/blueapi/service/runner.py
@@ -7,9 +7,7 @@
 
 from blueapi.config import ApplicationConfig
 from blueapi.service.interface import InitialisationException, start_worker, stop_worker
-from blueapi.service.model import (
-    EnvironmentResponse,
-)
+from blueapi.service.model import EnvironmentResponse
 
 # The default multiprocessing start method is fork
 set_start_method("spawn", force=True)
@@ -39,7 +37,7 @@ def __init__(
         self._config = config or ApplicationConfig()
         self._subprocess = None
         self._use_subprocess = use_subprocess
-        self._state = EnvironmentResponse(initialized=False, error_message="")
+        self._state = EnvironmentResponse(initialized=False)
 
     def start(self):
         if self._subprocess is None and self._use_subprocess:
@@ -55,11 +53,11 @@ def start(self):
             )
             LOGGER.exception(self._state.error_message)
             return
-        self._state = EnvironmentResponse(initialized=True, error_message="")
+        self._state = EnvironmentResponse(initialized=True)
 
     def stop(self):
         if self._subprocess is not None:
-            self._state = EnvironmentResponse(initialized=False, error_message="")
+            self._state = EnvironmentResponse(initialized=False)
             try:
                 self._subprocess.apply(stop_worker)
             except InitialisationException:
@@ -71,7 +69,7 @@ def stop(self):
         if (not self._use_subprocess) and (
             self._state.initialized or self._state.error_message
         ):
-            self._state = EnvironmentResponse(initialized=False, error_message="")
+            self._state = EnvironmentResponse(initialized=False)
             stop_worker()
 
     def reload_context(self):
diff --git a/tests/service/test_rest_api.py b/tests/service/test_rest_api.py
index 1ae1d61fc..65cddd9bb 100644
--- a/tests/service/test_rest_api.py
+++ b/tests/service/test_rest_api.py
@@ -12,12 +12,7 @@
 
 from blueapi.core.bluesky_types import Plan
 from blueapi.service import main
-from blueapi.service.model import (
-    DeviceModel,
-    PlanModel,
-    StateChangeRequest,
-    WorkerTask,
-)
+from blueapi.service.model import DeviceModel, PlanModel, StateChangeRequest, WorkerTask
 from blueapi.worker.event import WorkerState
 from blueapi.worker.task import Task
 from blueapi.worker.worker import TrackableTask
@@ -546,7 +541,7 @@ def test_set_state_invalid_transition(
 def test_get_environment_idle(client: TestClient) -> None:
     assert client.get("/environment").json() == {
         "initialized": True,
-        "error_message": "",
+        "error_message": None,
     }
 
 
diff --git a/tests/service/test_runner.py b/tests/service/test_runner.py
index fb86fb591..bf5457113 100644
--- a/tests/service/test_runner.py
+++ b/tests/service/test_runner.py
@@ -86,4 +86,4 @@ def test_can_reload_after_an_error(pool_mock: MagicMock):
 
     runner.reload_context()
 
-    assert runner.state == EnvironmentResponse(initialized=True, error_message="")
+    assert runner.state == EnvironmentResponse(initialized=True, error_message=None)