Fix write lock on catalogue items when spares definition not assigned #…

…417

inventory_management_system_api/repositories/setting.py

@@ -92,23 +92,34 @@ def get(self, out_model_type: Type[SettingOutBaseT], session: ClientSession = No
         :return: Retrieved setting or `None` if not found.
         """
 
+        # First obtain the setting document
+        setting = self._settings_collection.find_one({"_id": out_model_type.SETTING_ID}, session=session)
+
+        # Now ensure the setting is actually assigned and doesn't just have a write `_lock` and `_id` fields
+        if setting is None or (len(setting.keys()) == 2 and "_lock" in setting):
+            return None
+
         if out_model_type is SparesDefinitionOut:
             # The spares definition contains a list of usage statuses - use an aggregate query here to obtain
             # the actual usage status entities instead of just their stored ID
 
-            result = list(
+            setting = list(
                 self._settings_collection.aggregate(SPARES_DEFINITION_GET_AGGREGATION_PIPELINE, session=session)
-            )
-            setting = result[0] if len(result) > 0 else None
-        else:
-            setting = self._settings_collection.find_one({"_id": out_model_type.SETTING_ID}, session=session)
+            )[0]
 
-        if setting is not None:
-            return out_model_type(**setting)
-        return None
+        return out_model_type(**setting)
 
     def write_lock(self, out_model_type: Type[SettingOutBaseT], session: ClientSession) -> None:
-        # TODO: Comment
+        """
+        Updates a field `_lock` inside a setting in the database to lock the document from further updates in other
+        transactions.
+
+        Will add the setting document if it doesn't already exist. To ensure it can still be locked if it hasn't
+        ever been assigned a value before. (The get handles this case ensuring it still returns `None`.)
+
+        :param out_model_type: The output type of the setting's model. Also contains the ID for lookup.
+        :param session: PyMongo ClientSession to use for database operations.
+        """
         self._settings_collection.update_one(
-            {"_id": out_model_type.SETTING_ID}, {"$set": {"_lock": None}}, session=session
+            {"_id": out_model_type.SETTING_ID}, {"$set": {"_lock": None}}, upsert=True, session=session
         )

inventory_management_system_api/services/catalogue_item.py

@@ -94,13 +94,16 @@ def create(self, catalogue_item: CatalogueItemPostSchema) -> CatalogueItemOut:
         supplied_properties = catalogue_item.properties if catalogue_item.properties else []
         supplied_properties = utils.process_properties(defined_properties, supplied_properties)
 
-        # TODO: Update tests and comment, also check name here for consistency with others
+        # Perform actual creation in a transaction as we need to ensure they cannot be added when in the process of
+        # recalculating spares through the spares definition being set
         with start_session_transaction("creating catalogue item") as session:
-            # TODO: Update number of spares to be 0 if its there
+            # TODO: Update tests to include this new logic
             # TODO: Handle if setting doesn't exist
-            # TODO: Explain these
+            # Write lock the spares definition - if this fails it either means another catalogue item is being created
+            # (unlikely) or that the spares definition has been set and is still recalculating
             self._setting_repository.write_lock(SparesDefinitionOut, session)
 
+            # Obtain the current spares definition as if there is one set, we know it has 0 items
             spares_definition = self._setting_repository.get(SparesDefinitionOut, session=session)
 
             return self._catalogue_item_repository.create(

inventory_management_system_api/services/setting.py

@@ -64,16 +64,13 @@ def update_spares_definition(self, spares_definition: SparesDefinitionPutSchema)
         # while recalculating
         with start_session_transaction("updating spares definition") as session:
             # Update spares definition first to ensure write locked to prevent further updates while calculating below
+            # also ensures catalogue items cannot be created
             new_spares_definition = self._setting_repository.upsert(
                 SparesDefinitionIn(**spares_definition.model_dump()), SparesDefinitionOut, session=session
             )
 
-            # TODO: Update tests to reflect new order here
-
-            # Write lock for spares definition document to prevent any more catalogue items being created
-            self._setting_repository.write_lock(SparesDefinitionOut, session)
-
-            # Write lock all catalogue items to prevent any other spares calculations from happening
+            # Write lock all catalogue items to prevent any other spares calculations from happening when items
+            # are added, deleted or their usage status modified
             utils.prepare_for_number_of_spares_recalculation(None, self._catalogue_item_repository, session)
 
             # Obtain a list of all catalogue item ids that will need to be recalculated

test/unit/repositories/test_setting.py

@@ -221,3 +221,36 @@ def test_get_non_existent_spares_definition(self):
         self.mock_get(SparesDefinitionOut, None)
         self.call_get(SparesDefinitionOut)
         self.check_get_success()
+
+
+class WriteLockDSL(SettingRepoDSL):
+    """Base class for `write_lock` tests."""
+
+    _write_lock_out_model_type: Type[SettingOutBaseT]
+
+    def call_write_lock(self, out_model_type: Type[SettingOutBaseT]) -> None:
+        """
+        Calls the `SettingRepo` `write_lock` method with the appropriate data from a prior call to `mock_get`.
+
+        :param out_model_type: The type of the setting's output model to be obtained.
+        """
+
+        self._write_lock_out_model_type = out_model_type
+        self.setting_repo.write_lock(out_model_type, self.mock_session)
+
+    def check_write_lock_success(self) -> None:
+        """Checks that a prior call to `call_write_lock` worked as expected."""
+
+        self.settings_collection.update_one.assert_called_once_with(
+            {"_id": self._write_lock_out_model_type.SETTING_ID}, {"$set": {"_lock": None}}, session=self.mock_session
+        )
+
+
+class TestWriteLock(WriteLockDSL):
+    """Tests for write locking a setting."""
+
+    def test_write_lock(self):
+        """Test write locking a setting."""
+
+        self.call_write_lock(ExampleSettingOut)
+        self.check_write_lock_success()

test/unit/services/test_setting.py

@@ -148,16 +148,17 @@ def check_update_spares_definition_success(self) -> None:
             self._expected_spares_definition_in, SparesDefinitionOut, session=expected_session
         )
 
+        # Ensure write locking all existing catalogue items
+        self.mock_utils.prepare_for_number_of_spares_recalculation.assert_called_once_with(
+            None, self.mock_catalogue_item_repository, expected_session
+        )
+
         # Ensure obtained list of all catalogue item ids and used them to recalculate the number of spares
         self.mock_catalogue_item_repository.list_ids.assert_called_once_with()
         self.mock_utils.get_usage_status_ids_from_spares_definition.assert_called_once_with(
             self._expected_spares_definition_out
         )
 
-        self.mock_utils.prepare_for_number_of_spares_recalculation.assert_called_once_with(
-            None, self.mock_catalogue_item_repository, expected_session
-        )
-
         expected_perform_number_of_spares_recalculation_calls = []
         for catalogue_item_id in self._expected_catalogue_item_ids:
             expected_perform_number_of_spares_recalculation_calls.append(