From 322e14c26c51970251949f65059e68125a732363 Mon Sep 17 00:00:00 2001 From: jableader Date: Sun, 19 Oct 2025 13:12:28 +1100 Subject: [PATCH] Centralize transaction scoping per-request (dependency) and remove scattered conn.commit() in handlers --- api/deps.py | 20 +++++++++++++++-- api/meals.py | 8 ++----- api/persons.py | 1 - api/recipes.py | 18 ++++++++-------- api/shopping.py | 5 ----- ingredients/__init__.py | 1 + ingredients/db.py | 33 +++++++++++++++++++++++++++- meals/db.py | 42 +++++++++++++++++++++++++----------- openapi.json | 26 +++++++++++----------- persons/__init__.py | 1 + persons/db.py | 23 ++++++++++++++++++++ products/db.py | 4 ++-- recipes/db.py | 3 +++ refactor-project-strategy.md | 41 ++++++++++++++++++++++++----------- shopping/db.py | 4 ++++ 15 files changed, 166 insertions(+), 64 deletions(-) diff --git a/api/deps.py b/api/deps.py index 103d616..5f9b086 100644 --- a/api/deps.py +++ b/api/deps.py @@ -12,11 +12,27 @@ from common import ProblemDetails from settings import settings -# Dependency to create SQLite connection +# Dependency to create SQLite connection with PRAGMAs and per-request transaction async def get_db() -> AsyncGenerator[aiosqlite.Connection, None]: sql_db = await db.connect(settings.database_path) + # Connection-level configuration try: - yield sql_db + # Enable FK enforcement + await sql_db.execute("PRAGMA foreign_keys=ON;") + # Prefer WAL for better concurrency; ignore result + async with sql_db.execute("PRAGMA journal_mode=WAL;") as _: + await _.fetchone() + # Reasonable durability/perf tradeoff + await sql_db.execute("PRAGMA synchronous=NORMAL;") + # Begin a transaction for the whole request + await sql_db.execute("BEGIN;") + + try: + yield sql_db + await sql_db.commit() + except Exception: + await sql_db.rollback() + raise finally: await sql_db.close() diff --git a/api/meals.py b/api/meals.py index 78a9455..fd9060a 100644 --- a/api/meals.py +++ b/api/meals.py @@ -56,7 +56,6 @@ async def create_meal( return validation_response await meals.insert_meal(conn, meal) - await conn.commit() response.headers["Location"] = f"/api/v1/meals/{meal.id}" return meal @@ -81,9 +80,9 @@ async def update_meal( return validation_response await meals.update_meal(conn, meal) - await conn.commit() - return await get_meal(meal_id, conn) + # Re-fetch and return the updated meal. Pass request and conn explicitly to avoid Depends resolution. + return await get_meal(meal_id, request, conn) @router.post("/{meal_id}/consumed", response_model=meals.Meal, operation_id="markMealConsumed", summary="Mark a meal as consumed", @@ -108,7 +107,6 @@ async def mark_consumed( await meals.mark_consumed(conn, meal, consumed_date or datetime.datetime.now().astimezone()) await shopping.remove_request(conn, person, meal=meal) - await conn.commit() return meal @@ -126,8 +124,6 @@ async def delete_meal( await shopping.remove_request(conn, person, meal=meal) await meals.delete_meal(conn, meal.id) - - await conn.commit() return meal diff --git a/api/persons.py b/api/persons.py index 289898e..b68758e 100644 --- a/api/persons.py +++ b/api/persons.py @@ -94,6 +94,5 @@ async def create_person( person: persons.Person, response: Response, conn: aiosqlite.Connection = Depends(get_db) ) -> persons.Person: await persons.insert_person(conn, person) - await conn.commit() response.headers["Location"] = f"/api/v1/persons/{person.id}" return person diff --git a/api/recipes.py b/api/recipes.py index 5a0dca4..9f2eaac 100644 --- a/api/recipes.py +++ b/api/recipes.py @@ -60,7 +60,8 @@ async def parse_ingredients( continue if had_links: - await conn.commit() + # Transaction will commit at end of request + pass await ingredients.match_existing_products(conn, result) return result @@ -147,11 +148,12 @@ async def list_recipes( has_more = len(paged) > limit items = paged[:limit] - # load ingredients for items - for r in items: - r.ingredients = [] - async for ing in ingredients.find_ingredients_by_recipe_id(conn, r.id): - r.ingredients.append(ing) + # Batch-load ingredients for the page to avoid N+1 queries + if items: + recipe_ids = [r.id for r in items] + by_recipe = await ingredients.find_ingredients_by_recipe_ids(conn, recipe_ids) + for r in items: + r.ingredients = by_recipe.get(r.id, []) next_cursor = str(items[-1].id) if has_more and items else None # Compute prevCursor via DB helper prev_cursor: Optional[str] = None @@ -222,8 +224,7 @@ async def create_recipe( await ingredients.insert_ingredient(conn, ingredient) - await conn.commit() - + # Transaction will commit at end of request # Set Location to the new resource response.headers["Location"] = f"/api/v1/recipes/{recipe.id}" return recipe @@ -253,5 +254,4 @@ async def delete_recipe( return error_response(request, 404, "Recipe not found") await recipes.hide_recipe(conn, recipe_id, user) - await conn.commit() return recipe diff --git a/api/shopping.py b/api/shopping.py index 362ab37..84e6144 100644 --- a/api/shopping.py +++ b/api/shopping.py @@ -103,8 +103,6 @@ async def purchase_ingredients(shopping_list: shopping.ShoppingList, conn: aiosq ) await shopping.purchase(conn, shopping_list) - await conn.commit() - result = PurchasedShoppingList(list=shopping_list) await shopping.to_lookups( conn, @@ -138,7 +136,6 @@ async def sync_my_shopping_list(requests: List[ingredients.Ingredient], conn: ai await ingredients.insert_ingredient(conn, r) await shopping.request(conn, person, ingredient=r) - await conn.commit() return await get_my_shopping_list(conn, person) @@ -154,7 +151,6 @@ async def request_meal(r: MealIdWrapper, request: Request, conn: aiosqlite.Conne return error_response(request, 404, "Meal not found") response = await shopping.request(conn, person, meal=meal) - await conn.commit() return response @@ -166,7 +162,6 @@ async def unrequest_meal(meal_id: int, request: Request, conn: aiosqlite.Connect return error_response(request, 404, "Meal not found") await shopping.remove_request(conn, person, meal=meal) - await conn.commit() return {} diff --git a/ingredients/__init__.py b/ingredients/__init__.py index e7d8733..50217db 100644 --- a/ingredients/__init__.py +++ b/ingredients/__init__.py @@ -10,6 +10,7 @@ from ingredients.db import ( find_ingredient_by_id as find_ingredient_by_id, find_ingredients_by_meal_id as find_ingredients_by_meal_id, find_ingredients_by_recipe_id as find_ingredients_by_recipe_id, + find_ingredients_by_recipe_ids as find_ingredients_by_recipe_ids, insert_ingredient as insert_ingredient, ) from products import Product, add_missing_tags, find_product_by_tag, get_or_create diff --git a/ingredients/db.py b/ingredients/db.py index 5b8bfba..e8096c4 100644 --- a/ingredients/db.py +++ b/ingredients/db.py @@ -1,4 +1,4 @@ -from typing import Any, AsyncIterator, ClassVar, List, Optional +from typing import Any, AsyncIterator, ClassVar, List, Optional, Dict from pydantic import field_validator, Field from common import ApiModel @@ -64,6 +64,9 @@ async def create(conn): FOREIGN KEY (meal_id) REFERENCES Meal(id) );""" ) + # Useful indexes + await conn.execute("CREATE INDEX IF NOT EXISTS idx_ingredient_recipe_id ON Ingredient(recipe_id);") + await conn.execute("CREATE INDEX IF NOT EXISTS idx_ingredient_meal_id ON Ingredient(meal_id);") async def insert_ingredient(conn, ingredient: Ingredient): @@ -135,6 +138,34 @@ async def find_ingredients_by_recipe_id(conn, recipe_id: int) -> AsyncIterator[I ) +async def find_ingredients_by_recipe_ids(conn, recipe_ids: List[int]) -> dict[int, List[Ingredient]]: + """Fetch ingredients for many recipes in one query. Returns recipe_id -> [Ingredient].""" + if not recipe_ids: + return {} + placeholders = ",".join(["?"] * len(recipe_ids)) + ingredient_cols = [f"ingredient.{key}" for key in Ingredient.KEYS] + product_cols = [f"product.{key}" for key in Product.KEYS] + query = f""" + SELECT {','.join(ingredient_cols + product_cols)} + FROM Ingredient AS ingredient + LEFT JOIN Product AS product ON ingredient.product_id = product.id + WHERE ingredient.recipe_id IN ({placeholders}) + ORDER BY ingredient.recipe_id, ingredient.id + """ + result: dict[int, List[Ingredient]] = {rid: [] for rid in recipe_ids} + async with conn.execute(query, recipe_ids) as cursor: + async for row in cursor: + product_map = {k: v for k, v in zip(Product.KEYS, row[len(Ingredient.KEYS) :])} + product = Product(**product_map) if product_map["id"] else None + ing = Ingredient( + **{k: v for k, v in zip(Ingredient.KEYS, row[: len(Ingredient.KEYS)])}, + product=product, + ) + if ing.recipe_id is not None: + result.setdefault(int(ing.recipe_id), []).append(ing) + return result + + async def find_ingredients_by_meal_id(conn, meal_id: int) -> AsyncIterator[Ingredient]: ingredient_cols = [f"ingredient.{key}" for key in Ingredient.KEYS] product_cols = [f"product.{key}" for key in Product.KEYS] diff --git a/meals/db.py b/meals/db.py index 25ea4d4..2486f7f 100644 --- a/meals/db.py +++ b/meals/db.py @@ -5,6 +5,7 @@ from pydantic import Field from common import ApiModel import persons +from persons import get_by_ids as persons_get_by_ids from ingredients import ( Ingredient, delete_ingredients_by_meal_id, @@ -61,6 +62,8 @@ async def create(conn): FOREIGN KEY(person_id) REFERENCES Person(id) );""" ) + # Useful indexes + await conn.execute("CREATE INDEX IF NOT EXISTS idx_meal_participants_meal_role ON MealParticipant(meal_id, role);") await conn.execute( """ @@ -73,6 +76,9 @@ async def create(conn): );""" ) + # Index for faster lookup of recipes by meal + await conn.execute("CREATE INDEX IF NOT EXISTS idx_meal_recipes_meal_id ON MealRecipe(meal_id);") + async def insert_meal_participant(conn, meal_id: int, person_id: int, role: str): await conn.execute( @@ -172,6 +178,8 @@ async def find_upcoming_meals_by_date_range( async def load_participants(conn, meal: Meal) -> None: + # Fetch all participant links + links: list[tuple[int, str]] = [] async with conn.execute( """ SELECT person_id, role FROM MealParticipant @@ -180,18 +188,28 @@ async def load_participants(conn, meal: Meal) -> None: (meal.id,), ) as cursor: async for row in cursor: - person = await persons.get_by_id(conn, row[0]) - if row[1] == "chef": - if person: - meal.chefs.append(person) - elif row[1] == "cleanup": - if person: - meal.cleanup.append(person) - elif row[1] == "consumer": - if person: - meal.consumers.append(person) - else: - raise Exception(f"Unknown role: {row[1]}") + links.append((int(row[0]), str(row[1]))) + + if not links: + return + + # Bulk load persons by id + unique_ids = sorted({pid for pid, _ in links}) + people = await persons_get_by_ids(conn, unique_ids) + + for pid, role in links: + person = people.get(pid) + if role == "chef": + if person: + meal.chefs.append(person) + elif role == "cleanup": + if person: + meal.cleanup.append(person) + elif role == "consumer": + if person: + meal.consumers.append(person) + else: + raise Exception(f"Unknown role: {role}") async def load_recipes(conn, meal: Meal) -> None: diff --git a/openapi.json b/openapi.json index eaae1d9..e3f5ca4 100644 --- a/openapi.json +++ b/openapi.json @@ -30,15 +30,7 @@ "content": { "application/json": { "schema": { - "anyOf": [ - { - "$ref": "#/components/schemas/Product" - }, - { - "type": "null" - } - ], - "title": "Response Createproduct" + "$ref": "#/components/schemas/Product" } } } @@ -89,7 +81,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/Recipe-Output" + } } } }, @@ -295,7 +289,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/Recipe-Output" + } } } }, @@ -344,7 +340,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/Recipe-Output" + } } } }, @@ -395,7 +393,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/Recipe-Output" + } } } }, diff --git a/persons/__init__.py b/persons/__init__.py index c8db194..33e0771 100644 --- a/persons/__init__.py +++ b/persons/__init__.py @@ -9,6 +9,7 @@ from persons.db import ( compute_prev_cursor as compute_prev_cursor, get_by_id as get_by_id, get_by_name as get_by_name, + get_by_ids as get_by_ids, insert_person as insert_person, search_by_name as search_by_name, ) diff --git a/persons/db.py b/persons/db.py index 5d6b990..6635109 100644 --- a/persons/db.py +++ b/persons/db.py @@ -18,6 +18,8 @@ async def create(conn): name TEXT UNIQUE );""" ) + # Useful indexes for search and pagination + await conn.execute("CREATE INDEX IF NOT EXISTS idx_person_name ON Person(name);") async def search_by_name(conn, name: str) -> AsyncIterator[Person]: @@ -63,6 +65,27 @@ async def get_by_id(conn, id: int) -> Optional[Person]: return Person(id=row[0], name=row[1]) +async def get_by_ids(conn, ids: List[int]) -> dict[int, Person]: + """Fetch many persons in a single query. Returns a dict id->Person. + + If ids is empty, returns {}. + """ + if not ids: + return {} + placeholders = ",".join(["?"] * len(ids)) + query = f""" + SELECT id, name + FROM Person + WHERE id IN ({placeholders}) + """ + result: dict[int, Person] = {} + async with conn.execute(query, ids) as cursor: + async for row in cursor: + p = Person(id=row[0], name=row[1]) + result[p.id] = p + return result + + async def get_all(conn) -> AsyncIterator[Person]: async with conn.execute( """ diff --git a/products/db.py b/products/db.py index 4d1158b..70d440d 100644 --- a/products/db.py +++ b/products/db.py @@ -118,7 +118,7 @@ async def insert_product(conn, product: Product, data: dict): ) as cursor: product.id = cursor.lastrowid - await conn.commit() + # Commit handled by outer transaction async def add_tag(conn, product: Product, tag: str): @@ -130,7 +130,7 @@ async def add_tag(conn, product: Product, tag: str): (product.id, tag), ) - await conn.commit() + # Commit handled by outer transaction async def get_tags(conn, product: Product) -> AsyncIterator[str]: diff --git a/recipes/db.py b/recipes/db.py index 738cc68..6d90158 100644 --- a/recipes/db.py +++ b/recipes/db.py @@ -65,6 +65,9 @@ async def create(conn): FOREIGN KEY (hidden_by_id) REFERENCES Person(id) );""" ) + # Useful indexes for filtering/pagination + await conn.execute("CREATE INDEX IF NOT EXISTS idx_recipe_hidden_id ON Recipe(date_hidden, id);") + await conn.execute("CREATE INDEX IF NOT EXISTS idx_recipe_name_hidden_id ON Recipe(name, date_hidden, id);") def _as_insert_field(recipe: Recipe, name: str): diff --git a/refactor-project-strategy.md b/refactor-project-strategy.md index dabaef2..f617f50 100644 --- a/refactor-project-strategy.md +++ b/refactor-project-strategy.md @@ -76,21 +76,32 @@ Acceptance criteria --- ## Phase 3 — Data access and performance -- [ ] Centralize transaction scoping per-request (middleware or dependency) and remove scattered conn.commit() in handlers -- [ ] Add DB PRAGMAs on connect (WAL, foreign_keys=ON) +- [x] Centralize transaction scoping per-request (dependency) and remove scattered conn.commit() in handlers + - Implemented in `api/deps.get_db`: PRAGMAs + BEGIN/commit/rollback per request + - Removed explicit `await conn.commit()` calls from handlers and product DB helpers +- [x] Add DB PRAGMAs on connect (WAL, foreign_keys=ON, synchronous=NORMAL) - [ ] Batch-load related data to avoid N+1 (recipes/ingredients, meals/participants) -- [ ] Add indexes for common filters/joins - - [ ] ingredients.recipe_id - - [ ] ingredients.meal_id - - [ ] meal_participants.meal_id, role - - [ ] meal_recipes.meal_id - - [ ] recipes.date_hidden - - [ ] persons.name (for LIKE queries) + - [ ] Batch-load meal participants (fetch IDs once, bulk load persons) + - [x] Batch-load recipe ingredients across a page in `api/recipes.list_recipes` +- [x] Add indexes for common filters/joins + - [x] ingredients.recipe_id + - [x] ingredients.meal_id + - [x] meal_participants.meal_id, role + - [x] meal_recipes.meal_id + - [x] recipes.date_hidden (+ name, id composite for pagination) + - [x] persons.name (for LIKE queries) Acceptance criteria - Noticeable reduction in query count for list/detail endpoints (verified by logging/sqlite trace) - No functional regressions; tests still green +Status: In progress + +Notes +- PRAGMAs applied on connect and write transactions now scoped to each HTTP request +- Indexes added to improve common lookups and pagination +- Batch-loading of recipe ingredients implemented; meal participants batching is planned as a follow-up to complete Phase 3 acceptance criteria + --- ## Phase 4 — Modeling and validation @@ -163,14 +174,18 @@ Note: We can adopt this structure gradually without moving DB code immediately; - 2025-10-18: Moved dev reverse proxy to app lifespan; removed dead code from main.py; deduplicated models; tests all passing - 2025-10-18: Fixed FastAPI startup errors by normalizing Request usage/order; removed duplicate placeholder routes; added main.py shims for get_duplicates/validate_meal; full test suite green - 2025-10-18: Phase 2 complete — Added cookieAuth security to OpenAPI and annotated protected endpoints; normalized response_model across handlers; added Location headers on create endpoints while keeping 200 status for v1 compatibility; documented ProblemDetails responses in OpenAPI; regenerated openapi.json; full test suite still green +- 2025-10-19: Phase 3 (partially complete) — Added PRAGMAs (foreign_keys=ON, WAL, synchronous=NORMAL) and per-request transactions in `api/deps.get_db`; removed scattered commits in handlers and product DB; created indexes for ingredients, meal participants/recipes, recipes, persons, and shopping; tests remain green. Batch-loading participants and recipe-ingredient pages deferred as a follow-up within Phase 3. +- 2025-10-19: Fixed SQLite error during test setup by creating the `MealRecipe` table before indexing it; corrected `update_meal` to call `get_meal` with explicit `(request, conn)` avoiding a Depends object leak. Full test suite now passes (100%). Batch-loading of recipe ingredients is in place; meal participant batching remains outstanding. --- ## Next actions -- Phase 3: Plan DB PRAGMAs and indexes; add transaction scoping per request -- Phase 3: Plan DB PRAGMAs and indexes; add transaction scoping per request -- Phase 5: Add fixtures for DB/auth and tests for health + 201 Location - - Phase 4: Move remaining request/response models and helpers (ProductUrl, CurrentShoppingList, PurchasedShoppingList, LoginBody, validate_meal/get_duplicates) fully into feature modules and update tests to import from there; then remove back-compat shims from main.py +- Phase 3: Implement batch-loading to eliminate N+1 + - Batch-load meal participants and persons + - Batch-load recipe ingredients for list pages + - Optionally add lightweight query logging to validate reductions +- Phase 4: Move remaining request/response models and helpers (ProductUrl, CurrentShoppingList, PurchasedShoppingList, LoginBody, validate_meal/get_duplicates) fully into feature modules and update tests to import from there; then remove back-compat shims from main.py +- Phase 5: Add fixtures for DB/auth and tests for health + Location headers; consider adding perf checks Follow-ups (v2 candidates) - Adopt 201 Created for create endpoints and adjust tests/clients diff --git a/shopping/db.py b/shopping/db.py index e1b3875..b763cd6 100644 --- a/shopping/db.py +++ b/shopping/db.py @@ -79,6 +79,10 @@ async def create(conn): FOREIGN KEY(recipe_id) REFERENCES Recipe(id) );""" ) + # Useful indexes for queries + await conn.execute("CREATE INDEX IF NOT EXISTS idx_shopping_item_list_id ON ShoppingListItem(list_id);") + await conn.execute("CREATE INDEX IF NOT EXISTS idx_shopping_item_meal_id ON ShoppingListItem(meal_id);") + await conn.execute("CREATE INDEX IF NOT EXISTS idx_shopping_item_person_null_list ON ShoppingListItem(person_id, ingredient_id) WHERE list_id IS NULL;") def validate_request(request: ShoppingListItem) -> None: