Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculate number of spares #417 #434

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
1bebf69
Initial spares calculation when updating definition #417
joelvdavies Nov 29, 2024
6de588c
Initial working spares calculation for items updates #417
joelvdavies Nov 29, 2024
996e7a8
Add some comments and clean up slightly #417
joelvdavies Nov 29, 2024
48c60a4
Update mock data script and dump to include number of spares #417
joelvdavies Dec 2, 2024
4237f52
Update and add repo unit tests #417
joelvdavies Dec 2, 2024
1559a05
Update SettingService unit tests #417
joelvdavies Dec 2, 2024
d1480fb
Update ItemService unit tests #417
joelvdavies Dec 2, 2024
4aae41e
Add unit tests to service utils #417
joelvdavies Dec 3, 2024
162dcc5
Add missing assert in catalogue item service update test and align ot…
joelvdavies Dec 3, 2024
6d6789d
Initial spares calculation e2e tests #413
joelvdavies Dec 3, 2024
28c9838
Resolve outstanding todos and fix linting #417
joelvdavies Dec 4, 2024
e134cf9
Write lock all catalogue items when setting spares and reduce logs #417
joelvdavies Dec 5, 2024
58a1e87
Add WriteConflictError to routers #417
joelvdavies Dec 5, 2024
e08e647
Update after merge #417
joelvdavies Dec 5, 2024
49bb383
Fix tests and linting after rebase #417
joelvdavies Dec 6, 2024
0f8e745
Initial implementation of preventing catalogue item creation while se…
joelvdavies Dec 6, 2024
7dcc444
Fix write lock on catalogue items when spares definition not assigned…
joelvdavies Dec 6, 2024
1008b31
Merge branch 'handle-property-migration-conflict-#412' into calculate…
joelvdavies Dec 6, 2024
85a17d1
Update and add more tests #417
joelvdavies Dec 6, 2024
1a13d12
Merge branch 'handle-property-migration-conflict-#412' into calculate…
joelvdavies Dec 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified data/mock_data.dump
Binary file not shown.
3 changes: 2 additions & 1 deletion inventory_management_system_api/core/database.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
:returns: MongoDB session that has a transaction started on it.
"""
Expand Down
49 changes: 41 additions & 8 deletions inventory_management_system_api/repositories/catalogue_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def has_child_elements(self, catalogue_item_id: CustomObjectId, session: ClientS
item = self._items_collection.find_one({"catalogue_item_id": catalogue_item_id}, session=session)
return item is not None

def list_ids(self, catalogue_category_id: str, session: ClientSession = None) -> List[ObjectId]:
def list_ids(self, catalogue_category_id: Optional[str] = None, session: ClientSession = None) -> List[ObjectId]:
"""
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
Expand All @@ -148,17 +148,23 @@ def list_ids(self, catalogue_category_id: str, session: ClientSession = None) ->
: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",
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)}, {"_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
Expand Down Expand Up @@ -211,3 +217,30 @@ def update_names_of_all_properties_with_id(
array_filters=[{"elem._id": CustomObjectId(property_id)}],
session=session,
)

def update_number_of_spares(
self,
catalogue_item_id: Optional[ObjectId],
number_of_spares: Optional[int],
session: Optional[ClientSession] = None,
) -> None:
"""
Updates the `number_of_spares` field using a given catalogue item id filter.

:param catalogue_item_id: The ID of the catalogue item to update or `None` if updating all.
:param number_of_spares: New number of spares to update to.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make a note here to say why it is optional?

:param session: PyMongo ClientSession to use for database operations
"""

if catalogue_item_id is not None:
self._catalogue_items_collection.update_one(
{"_id": catalogue_item_id},
{"$set": {"number_of_spares": number_of_spares}},
session=session,
)
else:
self._catalogue_items_collection.update_many(
{},
{"$set": {"number_of_spares": number_of_spares}},
session=session,
)
19 changes: 19 additions & 0 deletions inventory_management_system_api/repositories/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,22 @@ def update_names_of_all_properties_with_id(
)

# pylint:enable=duplicate-code

def count_with_usage_status_ids_in(
self,
catalogue_item_id: ObjectId,
usage_status_ids: List[CustomObjectId],
session: Optional[ClientSession] = None,
) -> 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:param usage_status_id: List of usage status IDs which should be included in the count.
:param usage_status_ids: 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
)
33 changes: 26 additions & 7 deletions inventory_management_system_api/repositories/setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +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(self._settings_collection.aggregate(SPARES_DEFINITION_GET_AGGREGATION_PIPELINE))
setting = result[0] if len(result) > 0 else None
else:
setting = self._settings_collection.find_one({"_id": out_model_type.SETTING_ID}, session=session)
setting = list(
self._settings_collection.aggregate(SPARES_DEFINITION_GET_AGGREGATION_PIPELINE, session=session)
)[0]

return out_model_type(**setting)

def write_lock(self, out_model_type: Type[SettingOutBaseT], session: ClientSession) -> None:
"""
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`.)

if setting is not None:
return out_model_type(**setting)
return 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}}, upsert=True, session=session
)
9 changes: 8 additions & 1 deletion inventory_management_system_api/routers/v1/catalogue_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@
MissingMandatoryProperty,
MissingRecordError,
NonLeafCatalogueCategoryError,
WriteConflictError,
)
from inventory_management_system_api.schemas.catalogue_item import (
CATALOGUE_ITEM_WITH_CHILD_NON_EDITABLE_FIELDS,
CatalogueItemPatchSchema,
CatalogueItemPostSchema,
CatalogueItemSchema,
CATALOGUE_ITEM_WITH_CHILD_NON_EDITABLE_FIELDS,
)
from inventory_management_system_api.services.catalogue_item import CatalogueItemService

Expand Down Expand Up @@ -107,6 +108,12 @@ def create_catalogue_item(
message = "Adding a catalogue item to a non-leaf catalogue category is not allowed"
logger.exception(message)
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=message) from exc
# pylint: disable=duplicate-code
except WriteConflictError as exc:
message = str(exc)
logger.exception(message)
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=message) from exc
# pylint: enable=duplicate-code


@router.patch(
Expand Down
19 changes: 19 additions & 0 deletions inventory_management_system_api/routers/v1/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
InvalidPropertyTypeError,
MissingMandatoryProperty,
MissingRecordError,
WriteConflictError,
)
from inventory_management_system_api.schemas.item import ItemPatchSchema, ItemPostSchema, ItemSchema
from inventory_management_system_api.services.item import ItemService
Expand Down Expand Up @@ -58,6 +59,12 @@ def create_item(item: ItemPostSchema, item_service: ItemServiceDep) -> ItemSchem
message = "Unable to create item"
logger.exception(message)
raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=message) from exc
# pylint: disable=duplicate-code
except WriteConflictError as exc:
message = str(exc)
logger.exception(message)
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=message) from exc
# pylint: enable=duplicate-code


@router.delete(
Expand All @@ -77,6 +84,12 @@ def delete_item(
message = "Item not found"
logger.exception(message)
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=message) from exc
# pylint: disable=duplicate-code
except WriteConflictError as exc:
message = str(exc)
logger.exception(message)
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=message) from exc
# pylint: enable=duplicate-code


@router.get(path="", summary="Get items", response_description="List of items")
Expand Down Expand Up @@ -161,3 +174,9 @@ def partial_update_item(
message = "Cannot change the catalogue item of an item"
logger.exception(message)
raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=message) from exc
# pylint: disable=duplicate-code
except WriteConflictError as exc:
message = str(exc)
logger.exception(message)
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=message) from exc
# pylint: enable=duplicate-code
8 changes: 7 additions & 1 deletion inventory_management_system_api/routers/v1/setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from fastapi import APIRouter, Depends, HTTPException, status

from inventory_management_system_api.core.exceptions import InvalidObjectIdError, MissingRecordError
from inventory_management_system_api.core.exceptions import InvalidObjectIdError, MissingRecordError, WriteConflictError
from inventory_management_system_api.schemas.setting import SparesDefinitionPutSchema, SparesDefinitionSchema
from inventory_management_system_api.services.setting import SettingService

Expand Down Expand Up @@ -37,6 +37,12 @@ def update_spares_definition(
message = "A specified usage status does not exist"
logger.exception(message)
raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=message) from exc
# pylint: disable=duplicate-code
except WriteConflictError as exc:
message = str(exc)
logger.exception(message)
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=message) from exc
# pylint: enable=duplicate-code


@router.get(
Expand Down
33 changes: 25 additions & 8 deletions inventory_management_system_api/services/catalogue_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,19 @@
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 (
ChildElementsExistError,
InvalidActionError,
MissingRecordError,
NonLeafCatalogueCategoryError,
)
from inventory_management_system_api.models.catalogue_item import CatalogueItemIn, CatalogueItemOut
from inventory_management_system_api.models.setting import SparesDefinitionOut
from inventory_management_system_api.repositories.catalogue_category import CatalogueCategoryRepo
from inventory_management_system_api.repositories.catalogue_item import CatalogueItemRepo
from inventory_management_system_api.repositories.manufacturer import ManufacturerRepo
from inventory_management_system_api.repositories.setting import SettingRepo
from inventory_management_system_api.schemas.catalogue_item import (
CATALOGUE_ITEM_WITH_CHILD_NON_EDITABLE_FIELDS,
CatalogueItemPatchSchema,
Expand All @@ -39,6 +42,7 @@ def __init__(
catalogue_item_repository: Annotated[CatalogueItemRepo, Depends(CatalogueItemRepo)],
catalogue_category_repository: Annotated[CatalogueCategoryRepo, Depends(CatalogueCategoryRepo)],
manufacturer_repository: Annotated[ManufacturerRepo, Depends(ManufacturerRepo)],
setting_repository: Annotated[SettingRepo, Depends(SettingRepo)],
) -> None:
"""
Initialise the `CatalogueItemService` with a `CatalogueItemRepo`, `CatalogueCategoryRepo`
Expand All @@ -47,10 +51,12 @@ def __init__(
:param catalogue_item_repository: The `CatalogueItemRepo` repository to use.
:param catalogue_category_repository: The `CatalogueCategoryRepo` repository to use.
:param manufacturer_repository: The `ManufacturerRepo` repository to use.
:param setting_repository: The `SettingRepo` repository to use.
"""
self._catalogue_item_repository = catalogue_item_repository
self._catalogue_category_repository = catalogue_category_repository
self._manufacturer_repository = manufacturer_repository
self._setting_repository = setting_repository

def create(self, catalogue_item: CatalogueItemPostSchema) -> CatalogueItemOut:
"""
Expand Down Expand Up @@ -88,15 +94,26 @@ 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)

return self._catalogue_item_repository.create(
CatalogueItemIn(
**{
**catalogue_item.model_dump(),
"properties": supplied_properties,
},
number_of_spares=None,
# 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:
# 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(
CatalogueItemIn(
**{
**catalogue_item.model_dump(),
"properties": supplied_properties,
},
number_of_spares=None if spares_definition is None else 0,
),
session=session,
)
)

def get(self, catalogue_item_id: str) -> Optional[CatalogueItemOut]:
"""
Expand Down
Loading
Loading