From 7dcc44485e3f8ccb80ebd524bbd7a38575ac00e8 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Fri, 6 Dec 2024 14:01:21 +0000 Subject: [PATCH] Fix write lock on catalogue items when spares definition not assigned #417 --- .../repositories/setting.py | 31 +++++++++++------ .../services/catalogue_item.py | 9 +++-- .../services/setting.py | 9 ++--- test/unit/repositories/test_setting.py | 33 +++++++++++++++++++ test/unit/services/test_setting.py | 9 ++--- 5 files changed, 68 insertions(+), 23 deletions(-) diff --git a/inventory_management_system_api/repositories/setting.py b/inventory_management_system_api/repositories/setting.py index 6107e8a6..27b509f8 100644 --- a/inventory_management_system_api/repositories/setting.py +++ b/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 ) diff --git a/inventory_management_system_api/services/catalogue_item.py b/inventory_management_system_api/services/catalogue_item.py index c43431f2..fccb30df 100644 --- a/inventory_management_system_api/services/catalogue_item.py +++ b/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( diff --git a/inventory_management_system_api/services/setting.py b/inventory_management_system_api/services/setting.py index 75b5131e..af248608 100644 --- a/inventory_management_system_api/services/setting.py +++ b/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 diff --git a/test/unit/repositories/test_setting.py b/test/unit/repositories/test_setting.py index e465c52a..506682f1 100644 --- a/test/unit/repositories/test_setting.py +++ b/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() diff --git a/test/unit/services/test_setting.py b/test/unit/services/test_setting.py index 213cfb1a..8297b0e4 100644 --- a/test/unit/services/test_setting.py +++ b/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(