Prevent settings files being overwritten before they are read.

docs/source/properties.rst

@@ -237,3 +237,8 @@ Settings
 ------------
 
 Settings are properties with an additional feature: they are saved to disk. This means that settings will be automatically restored after the server is restarted. The function `~lt.setting` can be used to declare a `~lt.DataSetting` or decorate a function to make a `~lt.FunctionalSetting` in the same way that `~lt.property` can. It is usually imported as ``lt.setting``\ .
+
+.. note::
+
+    Settings are currently loaded when Things are attached to the server: this happens *after* ``__init__`` has been called and any `~lt.thing_slot`\ s have been populated, but *before* ``__enter__`` is called.
+    This means that, if you write to a setting during ``__init__``\ , that value may be overwritten when the settings are loaded.  Effectively, writing to settings during ``__init__`` modifies the default value. Prior to LabThings v0.3.0, writing to settings during ``__init__`` would raise an exception.

src/labthings_fastapi/thing.py

@@ -122,7 +122,10 @@
             `.create_thing_without_server` which generates a mock interface.
         """
         self._thing_server_interface = thing_server_interface
-        self._disable_saving_settings: bool = False
+        # Prevent settings being saved to the settings file before
+        # they have been loaded from the settings file (which would overwrite
+        # the file with default values).
+        self._disable_saving_settings: bool = True
 
     def __init_subclass__(cls, **kwargs: Any) -> None:
         r"""Validate the class settings.
@@ -275,7 +278,11 @@
         """
         settings = self._read_settings_file()
         if settings is None:
-            # Return if no settings can be loaded from file.
+            # If no settings were read, we don't need to update their values.
+            # We should, however, allow the settings file to be saved, as we
+            # have established that we're not going to overwrite anything of
+            # value.
+            self._disable_saving_settings = False
             return
 
         # Stop recursion by not allowing settings to be saved as we're reading them.
@@ -287,7 +294,7 @@
                     # Load the key from the JSON file using the setting's model
                     setting.set(setting.validate(value))
                 except ValidationError:
                     self.logger.warning(
                         f"Could not load setting {name} from settings file "
                         f"because of a validation error.",
                         exc_info=True,
@@ -359,9 +366,9 @@
         Some measure of caching here is a nice aim for the future, but not yet
         implemented.
         """
         if self._labthings_thing_state is None:
             self._labthings_thing_state = {}
         return self._labthings_thing_state
 
     def validate_thing_description(self) -> None:
         """Raise an exception if the thing description is not valid."""
@@ -452,7 +459,7 @@
         inv_id = get_invocation_id()
         server = self._thing_server_interface._server()
         if server is None:
             raise RuntimeError("Could not get server from thing_server_interface")
         action_manager = server.action_manager
         this_invocation = action_manager.get_invocation(inv_id)
         return this_invocation.log

tests/test_settings.py

@@ -112,6 +112,16 @@ class SubclassOfThingWithSettings(ThingWithSettings):
     """A subclass, so we can check it doesn't share settings with its parent."""
 
 
+class ThingThatWritesSettingOnInit(ThingWithSettings):
+    """A check that writing settings values during __init__ is safe."""
+
+    def __init__(self, **kwargs):
+        """Initialise, including writing a setting."""
+        super().__init__(**kwargs)
+
+        self.boolsetting = True  # Override the default value
+
+
 def _get_setting_file(server: lt.ThingServer, name: str):
     """Find the location of the settings file for a given Thing on a server."""
     path = server.things[name]._thing_server_interface.settings_file_path
@@ -161,10 +171,13 @@ def tempdir():
         yield tempdir
 
 
-def test_setting_available():
+@pytest.mark.parametrize("cls", [ThingWithSettings, ThingThatWritesSettingOnInit])
+def test_setting_available(cls: type[ThingWithSettings]):
     """Check default settings are available before connecting to server"""
-    thing = create_thing_without_server(ThingWithSettings)
-    assert not thing.boolsetting
+    thing = create_thing_without_server(cls)
+    # ThingWithSettings modifies boolsetting during __init__, effectively
+    # changing the default value.
+    assert thing.boolsetting is (cls is ThingThatWritesSettingOnInit)
     assert thing.stringsetting == "foo"
     assert thing.floatsetting == 1.0
     assert thing.localonlysetting == "Local-only default."
@@ -269,14 +282,27 @@ def test_settings_dict_internal_update(tempdir):
         assert not os.path.isfile(setting_file)
 
 
-def test_settings_load(tempdir, caplog):
-    """Check settings can be loaded from disk when added to server"""
-    server = lt.ThingServer.from_things(
-        {"thing": ThingWithSettings}, settings_folder=tempdir
-    )
+@pytest.mark.parametrize("cls", [ThingWithSettings, ThingThatWritesSettingOnInit])
+def test_settings_load(tempdir, caplog, cls: type[ThingWithSettings]):
+    """Check settings are loaded from disk when added to server.
+
+    This manually writes a settings file, then checks the settings are
+    correctly loaded.
+
+    We check this with and without a write to settings during __init__ to
+    test for #383 - that's done by the parametrization of `cls`.
+    """
+    server = lt.ThingServer.from_things({"thing": cls}, settings_folder=tempdir)
+    thing = server.things["thing"]
+    assert isinstance(thing, cls)
+    # Check the settings have their default values
+    for name in ["floatsetting", "stringsetting", "dictsetting"]:
+        assert getattr(thing, name) == _settings_dict()[name]
+    assert thing.tuplesetting == (1, 2)  # _settings_dict returns a list.
     setting_file = _get_setting_file(server, "thing")
     del server
     setting_json = json.dumps(
+        # Note: these are not the default values.
         _settings_dict(
             floatsetting=3.0,
             stringsetting="bar",
@@ -289,12 +315,12 @@ def test_settings_load(tempdir, caplog):
         file_obj.write(setting_json)
     with caplog.at_level(logging.WARNING):
         # Add thing to server and check new settings are loaded
-        server = lt.ThingServer.from_things(
-            {"thing": ThingWithSettings}, settings_folder=tempdir
-        )
+        server = lt.ThingServer.from_things({"thing": cls}, settings_folder=tempdir)
     assert len(caplog.records) == 0
     thing = server.things["thing"]
-    assert isinstance(thing, ThingWithSettings)
+    assert isinstance(thing, cls)
+    # boolsetting is modified in __init__ for one class, but as it's loaded
+    # from the file, we always get the value in _settings_dict
     assert not thing.boolsetting
     assert thing.stringsetting == "bar"
     assert thing.floatsetting == 3.0