Centralize transaction scoping per-request (dependency) and remove scattered conn.commit() in handlers

This commit is contained in:
jableader 2025-10-19 13:12:28 +11:00
parent 99b699eec9
commit 322e14c26c
15 changed files with 166 additions and 64 deletions

View file

@ -12,11 +12,27 @@ from common import ProblemDetails
from settings import settings 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]: async def get_db() -> AsyncGenerator[aiosqlite.Connection, None]:
sql_db = await db.connect(settings.database_path) sql_db = await db.connect(settings.database_path)
# Connection-level configuration
try:
# 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: try:
yield sql_db yield sql_db
await sql_db.commit()
except Exception:
await sql_db.rollback()
raise
finally: finally:
await sql_db.close() await sql_db.close()

View file

@ -56,7 +56,6 @@ async def create_meal(
return validation_response return validation_response
await meals.insert_meal(conn, meal) await meals.insert_meal(conn, meal)
await conn.commit()
response.headers["Location"] = f"/api/v1/meals/{meal.id}" response.headers["Location"] = f"/api/v1/meals/{meal.id}"
return meal return meal
@ -81,9 +80,9 @@ async def update_meal(
return validation_response return validation_response
await meals.update_meal(conn, meal) 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", @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 meals.mark_consumed(conn, meal, consumed_date or datetime.datetime.now().astimezone())
await shopping.remove_request(conn, person, meal=meal) await shopping.remove_request(conn, person, meal=meal)
await conn.commit()
return meal return meal
@ -126,8 +124,6 @@ async def delete_meal(
await shopping.remove_request(conn, person, meal=meal) await shopping.remove_request(conn, person, meal=meal)
await meals.delete_meal(conn, meal.id) await meals.delete_meal(conn, meal.id)
await conn.commit()
return meal return meal

View file

@ -94,6 +94,5 @@ async def create_person(
person: persons.Person, response: Response, conn: aiosqlite.Connection = Depends(get_db) person: persons.Person, response: Response, conn: aiosqlite.Connection = Depends(get_db)
) -> persons.Person: ) -> persons.Person:
await persons.insert_person(conn, person) await persons.insert_person(conn, person)
await conn.commit()
response.headers["Location"] = f"/api/v1/persons/{person.id}" response.headers["Location"] = f"/api/v1/persons/{person.id}"
return person return person

View file

@ -60,7 +60,8 @@ async def parse_ingredients(
continue continue
if had_links: if had_links:
await conn.commit() # Transaction will commit at end of request
pass
await ingredients.match_existing_products(conn, result) await ingredients.match_existing_products(conn, result)
return result return result
@ -147,11 +148,12 @@ async def list_recipes(
has_more = len(paged) > limit has_more = len(paged) > limit
items = paged[:limit] items = paged[:limit]
# load ingredients for items # 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: for r in items:
r.ingredients = [] r.ingredients = by_recipe.get(r.id, [])
async for ing in ingredients.find_ingredients_by_recipe_id(conn, r.id):
r.ingredients.append(ing)
next_cursor = str(items[-1].id) if has_more and items else None next_cursor = str(items[-1].id) if has_more and items else None
# Compute prevCursor via DB helper # Compute prevCursor via DB helper
prev_cursor: Optional[str] = None prev_cursor: Optional[str] = None
@ -222,8 +224,7 @@ async def create_recipe(
await ingredients.insert_ingredient(conn, ingredient) await ingredients.insert_ingredient(conn, ingredient)
await conn.commit() # Transaction will commit at end of request
# Set Location to the new resource # Set Location to the new resource
response.headers["Location"] = f"/api/v1/recipes/{recipe.id}" response.headers["Location"] = f"/api/v1/recipes/{recipe.id}"
return recipe return recipe
@ -253,5 +254,4 @@ async def delete_recipe(
return error_response(request, 404, "Recipe not found") return error_response(request, 404, "Recipe not found")
await recipes.hide_recipe(conn, recipe_id, user) await recipes.hide_recipe(conn, recipe_id, user)
await conn.commit()
return recipe return recipe

View file

@ -103,8 +103,6 @@ async def purchase_ingredients(shopping_list: shopping.ShoppingList, conn: aiosq
) )
await shopping.purchase(conn, shopping_list) await shopping.purchase(conn, shopping_list)
await conn.commit()
result = PurchasedShoppingList(list=shopping_list) result = PurchasedShoppingList(list=shopping_list)
await shopping.to_lookups( await shopping.to_lookups(
conn, conn,
@ -138,7 +136,6 @@ async def sync_my_shopping_list(requests: List[ingredients.Ingredient], conn: ai
await ingredients.insert_ingredient(conn, r) await ingredients.insert_ingredient(conn, r)
await shopping.request(conn, person, ingredient=r) await shopping.request(conn, person, ingredient=r)
await conn.commit()
return await get_my_shopping_list(conn, person) 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") return error_response(request, 404, "Meal not found")
response = await shopping.request(conn, person, meal=meal) response = await shopping.request(conn, person, meal=meal)
await conn.commit()
return response 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") return error_response(request, 404, "Meal not found")
await shopping.remove_request(conn, person, meal=meal) await shopping.remove_request(conn, person, meal=meal)
await conn.commit()
return {} return {}

View file

@ -10,6 +10,7 @@ from ingredients.db import (
find_ingredient_by_id as find_ingredient_by_id, find_ingredient_by_id as find_ingredient_by_id,
find_ingredients_by_meal_id as find_ingredients_by_meal_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_id as find_ingredients_by_recipe_id,
find_ingredients_by_recipe_ids as find_ingredients_by_recipe_ids,
insert_ingredient as insert_ingredient, insert_ingredient as insert_ingredient,
) )
from products import Product, add_missing_tags, find_product_by_tag, get_or_create from products import Product, add_missing_tags, find_product_by_tag, get_or_create

View file

@ -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 pydantic import field_validator, Field
from common import ApiModel from common import ApiModel
@ -64,6 +64,9 @@ async def create(conn):
FOREIGN KEY (meal_id) REFERENCES Meal(id) 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): 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]: async def find_ingredients_by_meal_id(conn, meal_id: int) -> AsyncIterator[Ingredient]:
ingredient_cols = [f"ingredient.{key}" for key in Ingredient.KEYS] ingredient_cols = [f"ingredient.{key}" for key in Ingredient.KEYS]
product_cols = [f"product.{key}" for key in Product.KEYS] product_cols = [f"product.{key}" for key in Product.KEYS]

View file

@ -5,6 +5,7 @@ from pydantic import Field
from common import ApiModel from common import ApiModel
import persons import persons
from persons import get_by_ids as persons_get_by_ids
from ingredients import ( from ingredients import (
Ingredient, Ingredient,
delete_ingredients_by_meal_id, delete_ingredients_by_meal_id,
@ -61,6 +62,8 @@ async def create(conn):
FOREIGN KEY(person_id) REFERENCES Person(id) 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( 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): async def insert_meal_participant(conn, meal_id: int, person_id: int, role: str):
await conn.execute( await conn.execute(
@ -172,6 +178,8 @@ async def find_upcoming_meals_by_date_range(
async def load_participants(conn, meal: Meal) -> None: async def load_participants(conn, meal: Meal) -> None:
# Fetch all participant links
links: list[tuple[int, str]] = []
async with conn.execute( async with conn.execute(
""" """
SELECT person_id, role FROM MealParticipant SELECT person_id, role FROM MealParticipant
@ -180,18 +188,28 @@ async def load_participants(conn, meal: Meal) -> None:
(meal.id,), (meal.id,),
) as cursor: ) as cursor:
async for row in cursor: async for row in cursor:
person = await persons.get_by_id(conn, row[0]) links.append((int(row[0]), str(row[1])))
if row[1] == "chef":
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: if person:
meal.chefs.append(person) meal.chefs.append(person)
elif row[1] == "cleanup": elif role == "cleanup":
if person: if person:
meal.cleanup.append(person) meal.cleanup.append(person)
elif row[1] == "consumer": elif role == "consumer":
if person: if person:
meal.consumers.append(person) meal.consumers.append(person)
else: else:
raise Exception(f"Unknown role: {row[1]}") raise Exception(f"Unknown role: {role}")
async def load_recipes(conn, meal: Meal) -> None: async def load_recipes(conn, meal: Meal) -> None:

View file

@ -30,15 +30,7 @@
"content": { "content": {
"application/json": { "application/json": {
"schema": { "schema": {
"anyOf": [
{
"$ref": "#/components/schemas/Product" "$ref": "#/components/schemas/Product"
},
{
"type": "null"
}
],
"title": "Response Createproduct"
} }
} }
} }
@ -89,7 +81,9 @@
"description": "Successful Response", "description": "Successful Response",
"content": { "content": {
"application/json": { "application/json": {
"schema": {} "schema": {
"$ref": "#/components/schemas/Recipe-Output"
}
} }
} }
}, },
@ -295,7 +289,9 @@
"description": "Successful Response", "description": "Successful Response",
"content": { "content": {
"application/json": { "application/json": {
"schema": {} "schema": {
"$ref": "#/components/schemas/Recipe-Output"
}
} }
} }
}, },
@ -344,7 +340,9 @@
"description": "Successful Response", "description": "Successful Response",
"content": { "content": {
"application/json": { "application/json": {
"schema": {} "schema": {
"$ref": "#/components/schemas/Recipe-Output"
}
} }
} }
}, },
@ -395,7 +393,9 @@
"description": "Successful Response", "description": "Successful Response",
"content": { "content": {
"application/json": { "application/json": {
"schema": {} "schema": {
"$ref": "#/components/schemas/Recipe-Output"
}
} }
} }
}, },

View file

@ -9,6 +9,7 @@ from persons.db import (
compute_prev_cursor as compute_prev_cursor, compute_prev_cursor as compute_prev_cursor,
get_by_id as get_by_id, get_by_id as get_by_id,
get_by_name as get_by_name, get_by_name as get_by_name,
get_by_ids as get_by_ids,
insert_person as insert_person, insert_person as insert_person,
search_by_name as search_by_name, search_by_name as search_by_name,
) )

View file

@ -18,6 +18,8 @@ async def create(conn):
name TEXT UNIQUE 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]: 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]) 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 def get_all(conn) -> AsyncIterator[Person]:
async with conn.execute( async with conn.execute(
""" """

View file

@ -118,7 +118,7 @@ async def insert_product(conn, product: Product, data: dict):
) as cursor: ) as cursor:
product.id = cursor.lastrowid product.id = cursor.lastrowid
await conn.commit() # Commit handled by outer transaction
async def add_tag(conn, product: Product, tag: str): 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), (product.id, tag),
) )
await conn.commit() # Commit handled by outer transaction
async def get_tags(conn, product: Product) -> AsyncIterator[str]: async def get_tags(conn, product: Product) -> AsyncIterator[str]:

View file

@ -65,6 +65,9 @@ async def create(conn):
FOREIGN KEY (hidden_by_id) REFERENCES Person(id) 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): def _as_insert_field(recipe: Recipe, name: str):

View file

@ -76,21 +76,32 @@ Acceptance criteria
--- ---
## Phase 3 — Data access and performance ## Phase 3 — Data access and performance
- [ ] Centralize transaction scoping per-request (middleware or dependency) and remove scattered conn.commit() in handlers - [x] Centralize transaction scoping per-request (dependency) and remove scattered conn.commit() in handlers
- [ ] Add DB PRAGMAs on connect (WAL, foreign_keys=ON) - 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) - [ ] Batch-load related data to avoid N+1 (recipes/ingredients, meals/participants)
- [ ] Add indexes for common filters/joins - [ ] Batch-load meal participants (fetch IDs once, bulk load persons)
- [ ] ingredients.recipe_id - [x] Batch-load recipe ingredients across a page in `api/recipes.list_recipes`
- [ ] ingredients.meal_id - [x] Add indexes for common filters/joins
- [ ] meal_participants.meal_id, role - [x] ingredients.recipe_id
- [ ] meal_recipes.meal_id - [x] ingredients.meal_id
- [ ] recipes.date_hidden - [x] meal_participants.meal_id, role
- [ ] persons.name (for LIKE queries) - [x] meal_recipes.meal_id
- [x] recipes.date_hidden (+ name, id composite for pagination)
- [x] persons.name (for LIKE queries)
Acceptance criteria Acceptance criteria
- Noticeable reduction in query count for list/detail endpoints (verified by logging/sqlite trace) - Noticeable reduction in query count for list/detail endpoints (verified by logging/sqlite trace)
- No functional regressions; tests still green - 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 ## 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: 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: 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-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 ## Next actions
- Phase 3: Plan DB PRAGMAs and indexes; add transaction scoping per request - Phase 3: Implement batch-loading to eliminate N+1
- Phase 3: Plan DB PRAGMAs and indexes; add transaction scoping per request - Batch-load meal participants and persons
- Phase 5: Add fixtures for DB/auth and tests for health + 201 Location - 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 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) Follow-ups (v2 candidates)
- Adopt 201 Created for create endpoints and adjust tests/clients - Adopt 201 Created for create endpoints and adjust tests/clients

View file

@ -79,6 +79,10 @@ async def create(conn):
FOREIGN KEY(recipe_id) REFERENCES Recipe(id) 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: def validate_request(request: ShoppingListItem) -> None: