diff --git a/inventory_management_system_api/core/database.py b/inventory_management_system_api/core/database.py index f07d0e40..a71f4ba2 100644 --- a/inventory_management_system_api/core/database.py +++ b/inventory_management_system_api/core/database.py @@ -39,7 +39,8 @@ def start_session_transaction(action_description: str) -> Generator[ClientSessio Also handles write conflicts. - :param action_description: Description of what the transaction is doing so it can be used in any raised errors. + :param action_description: Description of what the contents of the transaction is doing so it can be used in any + raised errors. :raises WriteConflictError: If there a write conflict during the transaction. """ diff --git a/inventory_management_system_api/repositories/catalogue_item.py b/inventory_management_system_api/repositories/catalogue_item.py index 7c8c13e6..a936c5e6 100644 --- a/inventory_management_system_api/repositories/catalogue_item.py +++ b/inventory_management_system_api/repositories/catalogue_item.py @@ -138,8 +138,6 @@ def has_child_elements(self, catalogue_item_id: CustomObjectId, session: ClientS return item is not None def list_ids(self, catalogue_category_id: Optional[str] = None, session: ClientSession = None) -> List[ObjectId]: - # TODO: Update description and tests - # TODO: Make sure where it is used elsewhere specifies catalogue_category_id = """ Retrieve a list of all catalogue item ids with a specific catalogue_category_id from a MongoDB database. Performs a projection to only include _id. (Required for mass updates of properties @@ -150,20 +148,23 @@ def list_ids(self, catalogue_category_id: Optional[str] = None, session: ClientS :return: A list object catalogue item ObjectId's or an empty list if no catalogue items are returned by the database. """ - logger.info( - "Finding the id's of all catalogue items within the catalogue category with ID '%s' in the database", - # TODO: Update this when catalogue_category_id is None? - catalogue_category_id, - ) + + query = {} + if catalogue_category_id: + catalogue_category_id = CustomObjectId(catalogue_category_id) + query["catalogue_category_id"] = catalogue_category_id + + message = "Retrieving IDs of all catalogue items from the database" + if not query: + logger.info(message) + else: + logger.info("%s matching the provided catalogue category ID filter", message) + logger.debug("Provided catalogue category ID filter: %s", catalogue_category_id) # Using distinct has a 16MB limit # https://stackoverflow.com/questions/29771192/how-do-i-get-a-list-of-just-the-objectids-using-pymongo # For 100000 documents, using list comprehension takes about 0.85 seconds vs 0.50 seconds for distinct - return self._catalogue_items_collection.find( - {"catalogue_category_id": CustomObjectId(catalogue_category_id)} if catalogue_category_id else {}, - {"_id": 1}, - session=session, - ).distinct("_id") + return self._catalogue_items_collection.find(query, {"_id": 1}, session=session).distinct("_id") def insert_property_to_all_matching( self, catalogue_category_id: str, property_in: PropertyIn, session: ClientSession = None @@ -222,9 +223,17 @@ def update_number_of_spares( catalogue_item_id: ObjectId, number_of_spares: Optional[int], session: Optional[ClientSession] = None, - ): + ) -> None: + """ + Updates the `number_of_spares` field for a catalogue item. - self._catalogue_items_collection.update_many( + :param catalogue_item_id: The ID of the catalogue item to update. + :param number_of_spares: New number of spares to update to. + :param session: PyMongo ClientSession to use for database operations + """ + logger.info("Updating the number of spares of the catalogue item with ID %s", catalogue_item_id) + + self._catalogue_items_collection.update_one( {"_id": catalogue_item_id}, {"$set": {"number_of_spares": number_of_spares}}, session=session, diff --git a/inventory_management_system_api/repositories/item.py b/inventory_management_system_api/repositories/item.py index b576656c..90009de9 100644 --- a/inventory_management_system_api/repositories/item.py +++ b/inventory_management_system_api/repositories/item.py @@ -188,9 +188,15 @@ def count_with_usage_statuses_ids_in( catalogue_item_id: ObjectId, usage_status_ids: List[CustomObjectId], session: Optional[ClientSession] = None, - ): - # TODO: Comment/log - # TODO: Look at https://stackoverflow.com/questions/60237515/mongodb-returning-wrong-count + ) -> int: + """ + Counts the number of items within a catalogue item with a `usage_status_id` contained within the given list. + + :param catalogue_item_id: ID of the catalogue item for which items should be counted. + :param usage_status_id: List of usage status IDs which should be included in the count. + :param session: PyMongo ClientSession to use for database operations. + :return: Number of items counted. + """ return self._items_collection.count_documents( {"catalogue_item_id": catalogue_item_id, "usage_status_id": {"$in": usage_status_ids}}, session=session diff --git a/inventory_management_system_api/services/item.py b/inventory_management_system_api/services/item.py index 1d52b7f8..b2be5537 100644 --- a/inventory_management_system_api/services/item.py +++ b/inventory_management_system_api/services/item.py @@ -4,9 +4,11 @@ """ import logging -from typing import Annotated, List, Optional +from contextlib import contextmanager +from typing import Annotated, Generator, List, Optional from fastapi import Depends +from pymongo.client_session import ClientSession from inventory_management_system_api.core.custom_object_id import CustomObjectId from inventory_management_system_api.core.database import start_session_transaction @@ -80,6 +82,7 @@ def create(self, item: ItemPostSchema) -> ItemOut: catalogue_item = self._catalogue_item_repository.get(catalogue_item_id) if not catalogue_item: raise MissingRecordError(f"No catalogue item found with ID: {catalogue_item_id}") + catalogue_item_id = CustomObjectId(catalogue_item_id) try: catalogue_category_id = catalogue_item.catalogue_category_id @@ -101,35 +104,13 @@ def create(self, item: ItemPostSchema) -> ItemOut: defined_properties = catalogue_category.properties properties = utils.process_properties(defined_properties, supplied_properties) - # Need to recalculate number of spares after adding these need to either succeed or fail together. Also need - # to be able to write lock the catalogue item document in the process to prevent further changes while - # recalculating - with start_session_transaction("adding item") as session: - # Write lock the catalogue item to prevent any further item updates for it until the transaction - # completes - # TODO: Change to just str in the prepare_for_number_of_spares_update? Same below - utils.prepare_for_number_of_spares_update( - CustomObjectId(catalogue_item_id), self._catalogue_item_repository, session - ) - - new_item = self._item_repository.create( + # Update number of spares when creating + with self._start_transaction_impacting_number_of_spares("creating item", catalogue_item_id) as session: + return self._item_repository.create( ItemIn(**{**item.model_dump(), "properties": properties, "usage_status": usage_status.value}), session=session, ) - # Obtain the current spares definition - spares_definition = self._setting_repository.get(SparesDefinitionOut, session=session) - if spares_definition: - utils.perform_number_of_spares_update( - CustomObjectId(catalogue_item_id), - utils.get_usage_status_ids(spares_definition), - self._catalogue_item_repository, - self._item_repository, - session=session, - ) - - return new_item - def get(self, item_id: str) -> Optional[ItemOut]: """ Retrieve an item by its ID @@ -149,6 +130,7 @@ def list(self, system_id: Optional[str], catalogue_item_id: Optional[str]) -> Li """ return self._item_repository.list(system_id, catalogue_item_id) + # pylint:disable=too-many-locals def update(self, item_id: str, item: ItemPatchSchema) -> ItemOut: """ Update an item by its ID. @@ -190,7 +172,6 @@ def update(self, item_id: str, item: ItemPatchSchema) -> ItemOut: # If catalogue item ID not supplied then it will be fetched, and its parent catalogue category. # the defined (at a catalogue category level) and supplied properties will be used to find # missing supplied properties. They will then be processed and validated. - if "properties" in update_data: catalogue_item = self._catalogue_item_repository.get(stored_item.catalogue_item_id) @@ -209,59 +190,34 @@ def update(self, item_id: str, item: ItemPatchSchema) -> ItemOut: update_data["properties"] = utils.process_properties(defined_properties, supplied_properties) - # TODO: Comment and cleanup - if not updating_usage_status: - return self._item_repository.update(item_id, ItemIn(**{**stored_item.model_dump(), **update_data})) - else: - with start_session_transaction("updating item") as session: - utils.prepare_for_number_of_spares_update(catalogue_item_id, self._catalogue_item_repository, session) - - new_item = self._item_repository.update( + # Spares only need updating if the usage status is being updated + if updating_usage_status: + with self._start_transaction_impacting_number_of_spares("updating item", catalogue_item_id) as session: + return self._item_repository.update( item_id, ItemIn(**{**stored_item.model_dump(), **update_data}), session=session ) + else: + return self._item_repository.update(item_id, ItemIn(**{**stored_item.model_dump(), **update_data})) - # Obtain the current spares definition - spares_definition = self._setting_repository.get(SparesDefinitionOut, session=session) - if spares_definition: - utils.perform_number_of_spares_update( - catalogue_item_id, - utils.get_usage_status_ids(spares_definition), - self._catalogue_item_repository, - self._item_repository, - session=session, - ) - return new_item + # pylint:enable=too-many-locals def delete(self, item_id: str) -> None: """ Delete an item by its ID. :param item_id: The ID of the item to delete. + :raises MissingRecordError: If the item doesn't exist. """ - # TODO: Update comment and tests for raising error instead of repo delete item = self.get(item_id) if item is None: raise MissingRecordError(f"No item found with ID: {str(item_id)}") - # TODO: Comment below - with start_session_transaction("deleting item") as session: - catalogue_item_id = CustomObjectId(item.catalogue_item_id) - - utils.prepare_for_number_of_spares_update(catalogue_item_id, self._catalogue_item_repository, session) - + # Update number of spares when deleting + with self._start_transaction_impacting_number_of_spares( + "deleting item", CustomObjectId(item.catalogue_item_id) + ) as session: self._item_repository.delete(item_id, session=session) - # Obtain the current spares definition - spares_definition = self._setting_repository.get(SparesDefinitionOut, session=session) - if spares_definition: - utils.perform_number_of_spares_update( - catalogue_item_id, - utils.get_usage_status_ids(spares_definition), - self._catalogue_item_repository, - self._item_repository, - session=session, - ) - def _merge_missing_properties( self, properties: List[PropertyOut], supplied_properties: List[PropertyPostSchema] ) -> List[PropertyPostSchema]: @@ -287,3 +243,43 @@ def _merge_missing_properties( else: merged_properties.append(PropertyPostSchema(**prop.model_dump())) return merged_properties + + @contextmanager + def _start_transaction_impacting_number_of_spares( + self, action_description: str, catalogue_item_id: CustomObjectId + ) -> Generator[ClientSession, None, None]: + """ + Handles recalculation of the `number_of_spares` of a catalogue item for updates that will impact it. + + Starts a MongoDB session and transaction, then write locks the catalogue item before yielding to allow + an update to take place using the returned session. Once any tasks using the session context have finished + it will finish by recalculating the number of spares for the catalogue item before finishing the transaction. + This write lock should prevent similar actions from occurring during the update to prevent an incorrect + spares calculation e.g. if another item was added between counting documents and then updating the spares field + it would cause a miscount. It also ensures any action executed using the session will either fail or succeed + with the spares update. + + :param action_description: Description of what the contents of the transaction is doing so it can be used in any + raised errors. + :param catalogue_item_id: ID of the effected catalogue item which will need its `number_of_spares` + recalculating + """ + + with start_session_transaction(action_description) as session: + # Write lock the catalogue item to prevent any further item updates for it until the transaction completes + utils.prepare_for_number_of_spares_recalculation( + catalogue_item_id, self._catalogue_item_repository, session + ) + + yield session + + # Update the number of spares + spares_definition = self._setting_repository.get(SparesDefinitionOut, session=session) + if spares_definition: + utils.perform_number_of_spares_recalculation( + catalogue_item_id, + utils.get_usage_status_ids_from_spares_definition(spares_definition), + self._catalogue_item_repository, + self._item_repository, + session=session, + ) diff --git a/inventory_management_system_api/services/setting.py b/inventory_management_system_api/services/setting.py index fceb47c4..ab98b1e4 100644 --- a/inventory_management_system_api/services/setting.py +++ b/inventory_management_system_api/services/setting.py @@ -7,7 +7,6 @@ from fastapi import Depends -from inventory_management_system_api.core.custom_object_id import CustomObjectId from inventory_management_system_api.core.database import start_session_transaction from inventory_management_system_api.core.exceptions import MissingRecordError from inventory_management_system_api.models.setting import SparesDefinitionIn, SparesDefinitionOut @@ -62,7 +61,7 @@ def update_spares_definition(self, spares_definition: SparesDefinitionPutSchema) # Need all updates to the number of spares to succeed or fail together with assigning the new definition # Also need to be able to write lock the catalogue item documents in the process to prevent further changes - # while recalculating. + # 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 new_spares_definition = self._setting_repository.upsert( @@ -74,12 +73,14 @@ def update_spares_definition(self, spares_definition: SparesDefinitionPutSchema) # Usage status id that constitute a spare in the new definition (obtain it now to save processing # repeatedly) - usage_status_ids = utils.get_usage_status_ids(new_spares_definition) + usage_status_ids = utils.get_usage_status_ids_from_spares_definition(new_spares_definition) # Recalculate for each catalogue item for catalogue_item_id in catalogue_item_ids: - utils.prepare_for_number_of_spares_update(catalogue_item_id, self._catalogue_item_repository, session) - utils.perform_number_of_spares_update( + utils.prepare_for_number_of_spares_recalculation( + catalogue_item_id, self._catalogue_item_repository, session + ) + utils.perform_number_of_spares_recalculation( catalogue_item_id, usage_status_ids, self._catalogue_item_repository, self._item_repository, session ) diff --git a/inventory_management_system_api/services/utils.py b/inventory_management_system_api/services/utils.py index d69254be..5d8fcae9 100644 --- a/inventory_management_system_api/services/utils.py +++ b/inventory_management_system_api/services/utils.py @@ -250,28 +250,54 @@ def _merge_non_mandatory_properties( return properties -def get_usage_status_ids(spares_definition: SparesDefinitionOut) -> list[CustomObjectId]: - # TODO: Comment +def get_usage_status_ids_from_spares_definition(spares_definition: SparesDefinitionOut) -> list[CustomObjectId]: + """ + Returns a list of usage status ids from a spares definition model in the format required for + `perform_number_of_spares_update`. + + :param spares_definition: Spares definition. + :return: List of usage status ids converted to CustomObjectId's. + """ return [CustomObjectId(usage_status.id) for usage_status in spares_definition.usage_statuses] -def prepare_for_number_of_spares_update( +def prepare_for_number_of_spares_recalculation( catalogue_item_id: ObjectId, catalogue_item_repository: CatalogueItemRepo, session: ClientSession ) -> None: - # TODO: Comment + """ + Prepares a catalogue item for a recalculation of the `number_of_spares` field. + + Should be called prior to `perform_number_of_spares_recalculation` in order to ensure the catalogue item is write + locked to avoid other similar updates interfering. + + :param catalogue_item_id: ID of the catalogue item that the spares will be recalculated for. + :param catalogue_item_repository: `CatalogueItemRepo` repository to use + :param session: Session to use for the recalculation. A transaction should already have been started. + """ catalogue_item_repository.update_number_of_spares(catalogue_item_id, None, session=session) -def perform_number_of_spares_update( +def perform_number_of_spares_recalculation( catalogue_item_id: ObjectId, usage_status_ids: list[CustomObjectId], catalogue_item_repository: CatalogueItemRepo, item_repository: ItemRepo, session: ClientSession, ) -> None: - # TODO: Comment - include must use prepare first + """ + Performs a recalculation of the `number_of_spares` field for a catalogue item. + + Should after `perform_number_of_spares_recalculation` in order to ensure the catalogue item is write locked to avoid + other similar updates interfering. + + :param catalogue_item_id: ID of the catalogue item that the spares will be recalculated for. + :param usage_status_ids: Usage status ids that should constitute a spare. + :param catalogue_item_repository: `CatalogueItemRepo` repository to use. + :param item_repository: `ItemRepo` repository to use. + :param session: Session to use for the recalculation. A transaction should already have been started. + """ # Now calculate the new number of spares new_number_of_spares = item_repository.count_with_usage_statuses_ids_in( diff --git a/test/unit/services/conftest.py b/test/unit/services/conftest.py index 10b419a4..177477e6 100644 --- a/test/unit/services/conftest.py +++ b/test/unit/services/conftest.py @@ -176,6 +176,8 @@ def fixture_catalogue_item_service( ) +# pylint:disable=too-many-arguments +# pylint:disable=too-many-positional-arguments @pytest.fixture(name="item_service") def fixture_item_service( item_repository_mock: Mock, @@ -183,14 +185,18 @@ def fixture_item_service( catalogue_item_repository_mock: Mock, system_repository_mock: Mock, usage_status_repository_mock: Mock, + setting_repository_mock: Mock, ) -> ItemService: """ Fixture to create an `ItemService` instance with mocked `ItemRepo`, `CatalogueItemRepo`, `CatalogueCategoryRepo`, - `SystemRepo` and `UsageStatusRepo` dependencies. + `SystemRepo`, `UsageStatusRepo` and `SettingRepo` dependencies. :param item_repository_mock: Mocked `ItemRepo` instance. :param catalogue_category_repository_mock: Mocked `CatalogueCategoryRepo` instance. :param catalogue_item_repository_mock: Mocked `CatalogueItemRepo` instance. + :param system_repository_mock: Mocked `SystemRepo` instance. + :param usage_status_repository_mock: Mocked `UsageStatusRepo` instance. + :param setting_repository_mock: Mocked `SettingRepo` instance. :return: `ItemService` instance with the mocked dependencies. """ return ItemService( @@ -199,6 +205,7 @@ def fixture_item_service( catalogue_item_repository_mock, system_repository_mock, usage_status_repository_mock, + setting_repository_mock, ) @@ -247,15 +254,24 @@ def fixture_usage_status_service(usage_status_repository_mock: Mock) -> UsageSta @pytest.fixture(name="setting_service") -def fixture_setting_service(setting_repository_mock: Mock, usage_status_repository_mock: Mock) -> SettingService: +def fixture_setting_service( + setting_repository_mock: Mock, + catalogue_item_repository_mock: Mock, + item_repository_mock: Mock, + usage_status_repository_mock: Mock, +) -> SettingService: """ Fixture to create a `SettingService` instance with mocked `SettingRepo` and `UsageStatusRepo` dependencies. :param setting_repository_mock: Mocked `SettingRepo` instance. + :param catalogue_item_repository_mock: Mocked `CatalogueItemRepo` instance. + :param item_repository_mock: Mocked `ItemRepo` instance. :param usage_status_repository_mock: Mocked `UsageStatusRepo` instance. :return: `SettingService` instance with the mocked dependency. """ - return SettingService(setting_repository_mock, usage_status_repository_mock) + return SettingService( + setting_repository_mock, catalogue_item_repository_mock, item_repository_mock, usage_status_repository_mock + ) class ServiceTestHelpers: