From 5a9242ccc9170631c23e7eeadc46b88f3cb23927 Mon Sep 17 00:00:00 2001 From: jableader Date: Sun, 26 Oct 2025 15:14:05 +1100 Subject: [PATCH] Tighten api endpoints --- api/deps.py | 4 +- api/shopping.py | 146 +++++++++++++++++++---- openapi.json | 277 +++++++++++++++++++++++++++++++------------- products/models.py | 3 +- tighten-api-spec.md | 113 ++++++++++++++++++ 5 files changed, 438 insertions(+), 105 deletions(-) create mode 100644 tighten-api-spec.md diff --git a/api/deps.py b/api/deps.py index 3babe01..5748f9f 100644 --- a/api/deps.py +++ b/api/deps.py @@ -1,9 +1,9 @@ from __future__ import annotations -from typing import Annotated, AsyncGenerator, Optional +from typing import AsyncGenerator, Optional import aiosqlite -from fastapi import Cookie, Depends, Request, HTTPException +from fastapi import Cookie, Depends, HTTPException, Request from fastapi.responses import JSONResponse import db diff --git a/api/shopping.py b/api/shopping.py index 3c375a5..660c7b3 100644 --- a/api/shopping.py +++ b/api/shopping.py @@ -1,6 +1,7 @@ from __future__ import annotations -from typing import Dict, List +from datetime import datetime +from typing import Dict, List, Literal import aiosqlite from fastapi import APIRouter, Depends, Request, Response @@ -10,23 +11,103 @@ import meals import persons import recipes import shopping +from shopping.models import StoreEnum from api.deps import cookie_person, error_response, get_db from common import ApiModel, Field, ProblemDetails + +# Outward-facing models to reduce unnecessary nulls in API responses +class ListIngredientItem(ApiModel): + kind: Literal["ingredient"] = "ingredient" + id: int = -1 + ingredient_id: int + person_id: int + created_date: datetime + # These may be present when the ingredient is part of a requested meal + list_id: int | None = None + meal_id: int | None = None + recipe_id: int | None = None + + +class RequestedMealItem(ApiModel): + kind: Literal["requestedMeal"] = "requestedMeal" + id: int = -1 + person_id: int + meal_id: int + created_date: datetime + + +# Input DTOs (separate from internal DB/domain models) +class IngredientPurchaseItemIn(ApiModel): + ingredient_id: int + person_id: int + created_date: datetime | None = None + meal_id: int | None = None + recipe_id: int | None = None + + +class PurchaseListIn(ApiModel): + store_name: StoreEnum + items: List[IngredientPurchaseItemIn] + + +# Output DTOs for purchased lists +class ShoppingListOut(ApiModel): + id: int + created_date: datetime + store_name: StoreEnum + purchased_by_id: int + purchased_by: persons.Person | None = None + items: List[ListIngredientItem] = Field(default_factory=list) + + router = APIRouter(prefix="/shopping", tags=["shopping"]) class CurrentShoppingList(ApiModel): - outstanding_items: List[shopping.ShoppingListItem] - requested_meals: List[shopping.ShoppingListItem] - purchased_items: List[shopping.ShoppingListItem] = Field(default_factory=list) + outstanding_items: List[ListIngredientItem] + requested_meals: List[RequestedMealItem] + purchased_items: List[ListIngredientItem] = Field(default_factory=list) ingredients_lookup: Dict[int, ingredients.Ingredient] = Field(default_factory=dict) meals_lookup: Dict[int, meals.Meal] = Field(default_factory=dict) - shopping_list_lookup: Dict[int, shopping.ShoppingList] = Field(default_factory=dict) + shopping_list_lookup: Dict[int, ShoppingListOut] = Field(default_factory=dict) recipes_lookup: Dict[int, recipes.Recipe] = Field(default_factory=dict) +# Mapping helpers from domain -> outward API +def _to_ingredient_item(item: shopping.ShoppingListItem) -> ListIngredientItem: + return ListIngredientItem( + id=item.id, + ingredient_id=item.ingredient_id if item.ingredient_id is not None else -1, + person_id=item.person_id, + created_date=item.created_date, + list_id=item.list_id, + meal_id=item.meal_id, + recipe_id=item.recipe_id, + ) + + +def _to_meal_item(item: shopping.ShoppingListItem) -> RequestedMealItem: + return RequestedMealItem( + id=item.id, + person_id=item.person_id, + meal_id=item.meal_id if item.meal_id is not None else -1, + created_date=item.created_date, + ) + + +def _to_shopping_list_out(sl: shopping.ShoppingList) -> ShoppingListOut: + return ShoppingListOut( + id=sl.id, + created_date=sl.created_date, + store_name=sl.store_name, + purchased_by_id=sl.purchased_by_id, + purchased_by=sl.purchased_by, + items=[_to_ingredient_item(i) for i in sl.items], + ) + + @router.get( "/current", response_model=CurrentShoppingList, @@ -46,24 +127,30 @@ async def get_current_shopping_list( ) = await shopping.get_outstanding_requests(conn) other_shopping_list_ids = {item.list_id for item in purchased_requests} - shopping_list_lookup = {} + # Load full lists for additional lookups + other_lists_domain: Dict[int, shopping.ShoppingList] = {} for list_id in other_shopping_list_ids: if list_id is not None: sl = await shopping.load_shopping_list(conn, list_id) if sl is not None: - shopping_list_lookup[list_id] = sl + other_lists_domain[list_id] = sl # Add any additional items from shopping lists to the existing lookups - additional_items = [item for sl in shopping_list_lookup.values() for item in sl.items] + additional_items = [item for sl in other_lists_domain.values() for item in sl.items] if additional_items: await shopping.to_lookups( conn, additional_items, meals_lookup, recipes_lookup, ingredients_lookup ) + # Convert domain shopping lists to outward form for response + shopping_list_lookup: Dict[int, ShoppingListOut] = { + k: _to_shopping_list_out(v) for k, v in other_lists_domain.items() + } + return CurrentShoppingList( - outstanding_items=outstanding_requests, - requested_meals=meal_requests, - purchased_items=purchased_requests, + outstanding_items=[_to_ingredient_item(i) for i in outstanding_requests], + requested_meals=[_to_meal_item(i) for i in meal_requests], + purchased_items=[_to_ingredient_item(i) for i in purchased_requests], meals_lookup=meals_lookup, shopping_list_lookup=shopping_list_lookup, ingredients_lookup=ingredients_lookup, @@ -72,7 +159,7 @@ async def get_current_shopping_list( class PurchasedShoppingList(ApiModel): - list: shopping.ShoppingList + list: ShoppingListOut meals_lookup: Dict[int, meals.Meal] = Field(default_factory=dict) ingredients_lookup: Dict[int, ingredients.Ingredient] = Field(default_factory=dict) recipes_lookup: Dict[int, recipes.Recipe] = Field(default_factory=dict) @@ -102,7 +189,7 @@ async def get_shopping_list( conn, shopping_list.items ) return PurchasedShoppingList( - list=shopping_list, + list=_to_shopping_list_out(shopping_list), meals_lookup=meals_lookup, recipes_lookup=recipes_lookup, ingredients_lookup=ingredients_lookup, @@ -128,7 +215,7 @@ async def get_shopping_list( }, ) async def purchase_ingredients( - shopping_list: shopping.ShoppingList, + shopping_list: PurchaseListIn, request: Request, conn: aiosqlite.Connection = Depends(get_db), person: persons.Person = Depends(cookie_person), @@ -137,19 +224,32 @@ async def purchase_ingredients( if not person: return error_response(request, 401, "Unauthorized") - # Attach purchaser to ensure purchased_by_id is set via BaseLinkedModel - shopping_list = shopping.ShoppingList( - purchased_by=person, items=shopping_list.items, store_name=shopping_list.store_name + # Map outward input DTO to domain model + domain_items: List[shopping.ShoppingListItem] = [] + for it in shopping_list.items: + created = it.created_date or datetime.now().astimezone() + domain_items.append( + shopping.ShoppingListItem( + ingredient_id=it.ingredient_id, + person_id=it.person_id, + meal_id=it.meal_id, + recipe_id=it.recipe_id, + created_date=created, + ) + ) + + domain_list = shopping.ShoppingList( + purchased_by=person, items=domain_items, store_name=shopping_list.store_name ) try: - await shopping.purchase(conn, shopping_list) + await shopping.purchase(conn, domain_list) except ValueError as e: # Map domain validation errors to a proper Problem Details response return error_response(request, 400, str(e)) - result = PurchasedShoppingList(list=shopping_list) + result = PurchasedShoppingList(list=_to_shopping_list_out(domain_list)) await shopping.to_lookups( conn, - shopping_list.items, + domain_list.items, result.meals_lookup, result.recipes_lookup, result.ingredients_lookup, @@ -202,7 +302,7 @@ class MealIdWrapper(ApiModel): @router.post( "/current/meals/me", - response_model=shopping.ShoppingListItem, + response_model=RequestedMealItem, operation_id="requestMeal", summary="Request a meal for shopping", responses={ @@ -218,13 +318,13 @@ async def request_meal( request: Request, conn: aiosqlite.Connection = Depends(get_db), person: persons.Person = Depends(cookie_person), -) -> shopping.ShoppingListItem | Response: +) -> RequestedMealItem | Response: meal = await meals.find_meal_by_id(conn, r.meal_id) if not meal: return error_response(request, 404, "Meal not found") response = await shopping.request(conn, person, meal=meal) - return response + return _to_meal_item(response) class Ok(ApiModel): diff --git a/openapi.json b/openapi.json index 866a2bc..68f99aa 100644 --- a/openapi.json +++ b/openapi.json @@ -859,7 +859,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/ShoppingList" + "$ref": "#/components/schemas/PurchaseListIn" } } } @@ -1058,7 +1058,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/ShoppingListItem" + "$ref": "#/components/schemas/RequestedMealItem" } } } @@ -1397,21 +1397,21 @@ "properties": { "outstandingItems": { "items": { - "$ref": "#/components/schemas/ShoppingListItem" + "$ref": "#/components/schemas/ListIngredientItem" }, "type": "array", "title": "Outstandingitems" }, "requestedMeals": { "items": { - "$ref": "#/components/schemas/ShoppingListItem" + "$ref": "#/components/schemas/RequestedMealItem" }, "type": "array", "title": "Requestedmeals" }, "purchasedItems": { "items": { - "$ref": "#/components/schemas/ShoppingListItem" + "$ref": "#/components/schemas/ListIngredientItem" }, "type": "array", "title": "Purchaseditems" @@ -1432,7 +1432,7 @@ }, "shoppingListLookup": { "additionalProperties": { - "$ref": "#/components/schemas/ShoppingList" + "$ref": "#/components/schemas/ShoppingListOut" }, "type": "object", "title": "Shoppinglistlookup" @@ -1575,6 +1575,129 @@ ], "title": "Ingredient" }, + "IngredientPurchaseItemIn": { + "properties": { + "ingredientId": { + "type": "integer", + "title": "Ingredientid" + }, + "personId": { + "type": "integer", + "title": "Personid" + }, + "createdDate": { + "anyOf": [ + { + "type": "string", + "format": "date-time" + }, + { + "type": "null" + } + ], + "title": "Createddate" + }, + "mealId": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "title": "Mealid" + }, + "recipeId": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "title": "Recipeid" + } + }, + "type": "object", + "required": [ + "ingredientId", + "personId" + ], + "title": "IngredientPurchaseItemIn" + }, + "ListIngredientItem": { + "properties": { + "kind": { + "type": "string", + "enum": [ + "ingredient" + ], + "const": "ingredient", + "title": "Kind", + "default": "ingredient" + }, + "id": { + "type": "integer", + "title": "Id", + "default": -1 + }, + "ingredientId": { + "type": "integer", + "title": "Ingredientid" + }, + "personId": { + "type": "integer", + "title": "Personid" + }, + "createdDate": { + "type": "string", + "format": "date-time", + "title": "Createddate" + }, + "listId": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "title": "Listid" + }, + "mealId": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "title": "Mealid" + }, + "recipeId": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "title": "Recipeid" + } + }, + "type": "object", + "required": [ + "ingredientId", + "personId", + "createdDate" + ], + "title": "ListIngredientItem" + }, "LoginBody": { "properties": { "username": { @@ -2069,10 +2192,30 @@ ], "title": "ProductUrl" }, + "PurchaseListIn": { + "properties": { + "storeName": { + "$ref": "#/components/schemas/StoreEnum" + }, + "items": { + "items": { + "$ref": "#/components/schemas/IngredientPurchaseItemIn" + }, + "type": "array", + "title": "Items" + } + }, + "type": "object", + "required": [ + "storeName", + "items" + ], + "title": "PurchaseListIn" + }, "PurchasedShoppingList": { "properties": { "list": { - "$ref": "#/components/schemas/ShoppingList" + "$ref": "#/components/schemas/ShoppingListOut" }, "mealsLookup": { "additionalProperties": { @@ -2314,26 +2457,61 @@ ], "title": "Recipe" }, - "ShoppingList": { + "RequestedMealItem": { "properties": { + "kind": { + "type": "string", + "enum": [ + "requestedMeal" + ], + "const": "requestedMeal", + "title": "Kind", + "default": "requestedMeal" + }, "id": { "type": "integer", "title": "Id", "default": -1 }, + "personId": { + "type": "integer", + "title": "Personid" + }, + "mealId": { + "type": "integer", + "title": "Mealid" + }, + "createdDate": { + "type": "string", + "format": "date-time", + "title": "Createddate" + } + }, + "type": "object", + "required": [ + "personId", + "mealId", + "createdDate" + ], + "title": "RequestedMealItem" + }, + "ShoppingListOut": { + "properties": { + "id": { + "type": "integer", + "title": "Id" + }, "createdDate": { "type": "string", "format": "date-time", "title": "Createddate" }, "storeName": { - "$ref": "#/components/schemas/StoreEnum", - "default": "" + "$ref": "#/components/schemas/StoreEnum" }, "purchasedById": { "type": "integer", - "title": "Purchasedbyid", - "default": -1 + "title": "Purchasedbyid" }, "purchasedBy": { "anyOf": [ @@ -2347,79 +2525,20 @@ }, "items": { "items": { - "$ref": "#/components/schemas/ShoppingListItem" + "$ref": "#/components/schemas/ListIngredientItem" }, "type": "array", "title": "Items" } }, "type": "object", - "title": "ShoppingList" - }, - "ShoppingListItem": { - "properties": { - "id": { - "type": "integer", - "title": "Id", - "default": -1 - }, - "listId": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "Listid" - }, - "personId": { - "type": "integer", - "title": "Personid", - "default": -1 - }, - "ingredientId": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "Ingredientid" - }, - "recipeId": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "Recipeid" - }, - "mealId": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "Mealid" - }, - "createdDate": { - "type": "string", - "format": "date-time", - "title": "Createddate" - } - }, - "type": "object", - "title": "ShoppingListItem" + "required": [ + "id", + "createdDate", + "storeName", + "purchasedById" + ], + "title": "ShoppingListOut" }, "StoreEnum": { "type": "string", diff --git a/products/models.py b/products/models.py index 0f5ac19..b817135 100644 --- a/products/models.py +++ b/products/models.py @@ -2,9 +2,10 @@ from __future__ import annotations from typing import ClassVar, List, Optional -from common import ApiModel from pydantic import PrivateAttr +from common import ApiModel + class Product(ApiModel): KEYS: ClassVar[List[str]] = [ diff --git a/tighten-api-spec.md b/tighten-api-spec.md new file mode 100644 index 0000000..bcf8006 --- /dev/null +++ b/tighten-api-spec.md @@ -0,0 +1,113 @@ +# Tighten Public API Nullability + +Make the external API more consistent and predictable by eliminating unnecessary nulls (nullable fields) in models and responses. This plan lists concrete, low-risk changes, their rationale, and exact files/lines to modify. Each task is checkable and includes verification steps. + +Date: 2025-10-21 + +## Principles + +- Prefer non-nullable types where domain requires a value (DB constraints, logic always sets it). +- Keep optional only when a field truly may be absent by design (e.g., hiddenBy, prevCursor when first page). +- Preserve backward compatibility where feasible. When changing response shapes, update tests and OpenAPI examples. +- Pydantic already excludes None on serialization in some places; we still tighten model types to improve OpenAPI and client SDKs. + +## Quick wins (low risk) + +- [x] Page.total is non-nullable with default 0 + - Why: Pagination always returns a number. Current `Optional[int]` leads to `null` in schema and potential nulls in responses. + - Change: in `common.py`, change `total: Optional[int]` to `total: int = Field(default=0, description="Total count")`. + - Verify: + - [ ] mypy/pyright/ruff pass. + - [ ] Tests for persons/recipes list remain green. + - [ ] OpenAPI shows `total` as `integer` (no anyOf null). + +- [x] Recipe.created_by_id non-nullable + - Why: DB enforces NOT NULL and creation flow always sets it. + - Change: in `recipes/models.py` set `created_by_id: int` (remove Optional). Keep `created_by: Optional[Person]` (hydrated field). + - Knock-on: `api/recipes.load_full_recipe` can drop the `if r.created_by_id is not None` guard. + - Verify: + - [ ] All recipe-related tests green. + - [ ] OpenAPI for Recipe shows `createdById` required. + +- [x] Product.raw_data excluded from public schema + - Why: Internal/testing helper currently typed as `Optional[dict]` -> visible as nullable in OpenAPI. + - Change: in `products/models.py` use a `PrivateAttr` (with a `raw_data` property) so it stays out of the schema without creating Input/Output variants. + - Verify: + - [ ] Product schema in OpenAPI does not include `rawData`. + - [ ] Tests referencing raw_data still pass (field remains available in code, excluded from schema/response). + +## Shopping models and endpoints + +`ShoppingListItem` currently represents two cases (ingredient request vs meal request), so several linking fields are nullable. We can reduce nulls in the public API by introducing outward-facing variants while keeping the DB model as-is. + +- [ ] Optional: Introduce discriminated union for API returns (medium change) + - Rationale: Return `oneOf` in OpenAPI with variant-specific required fields; eliminates irrelevant nullable properties for each variant. + - Approach (sketch): + - Define `ListIngredientItem` and `RequestedMealItem` pydantic models with a `kind` discriminator. + - Update `api/shopping.py` response models (CurrentShoppingList and PurchasedShoppingList) to use `Union[ListIngredientItem, RequestedMealItem]` for item arrays. + - Conversion helpers in `shopping` module to map from `ShoppingListItem` DB model to the outward union. + - Verify: + - [ ] Update tests in `tests/test_shopping_api.py` to accept the new shape while preserving field meanings. + - [ ] OpenAPI shows `oneOf` for shopping list items. + +- [ ] Tighten invariants without breaking shape (keep for now) + - Keep model but document invariants (only one of ingredient_id/meal_id required; recipe_id optional when meal request). Repository already validates; consider pydantic validators later. + +## Persons and Recipes listings + +- [ ] Ensure total is populated for Person and Recipe lists + - Already implemented in `api/persons.py` and `api/recipes.py` using repository `count_*` helpers. After making `Page.total` non-nullable, nothing else required. + +## Authentication dependency + +- [x] Provide strict non-null person dependency for protected endpoints + - Why: Many endpoints assume an authenticated user; typing as non-null simplifies signatures and docs. + - Change: + - Consolidated on a single dependency `cookie_person` (strict): `Cookie(..., alias="user_id")` and raises 401 if missing/unknown. + - Removed `require_cookie_person` and switched usages to `cookie_person`. + - Verify: + - [x] Endpoint signatures updated. + - [x] Unauthorized behavior covered by handlers; overall tests still pass. + +## File-by-file checklist (edits) + +- [x] `common.py` + - [x] Page.total -> `int = Field(default=0, ...)` + +- [x] `recipes/models.py` + - [x] `created_by_id: int` + +- [x] `api/recipes.py` + - [x] In `load_full_recipe`, set `r.created_by = await persons.get_by_id(conn, r.created_by_id)` unconditionally. + +- [x] `products/models.py` + - [x] `raw_data` moved to `PrivateAttr` with property; kept out of schema. + +- [x] `api/deps.py` + - [x] Added `require_cookie_person(...) -> persons.Person` that raises 401. + - [x] Updated protected endpoints to depend on `require_cookie_person`. + +- [ ] (Optional) Shopping API union types + - [x] Add outward-facing union models and mapping helpers. + - [x] Update `api/shopping.py` response models to use union. + +## Tests and validation + +- [x] Run format/lint/typecheck + - make format && make lint && make typecheck +- [x] Run tests + - make test +- [x] Export OpenAPI and inspect schema + - make openapi (confirm: Recipe.createdById required; Product has single schema; Page.total non-nullable; cookie param required on protected ops.) + +## Rollout notes + +- API change in OpenAPI (non-null total; createdById required). Client SDKs generated from the spec may need re-gen. Runtime remains compatible because server fills these fields. +- Even without the optional union refactor, the quick wins remove several unnecessary nulls. + +## Acceptance criteria + +- OpenAPI no longer marks Page.total and Recipe.createdById as nullable. +- Product.rawData does not appear in the public schema. +- All tests pass; no runtime regressions. +- Optional: Shopping list items use `oneOf` variants.