114 lines
5.9 KiB
Markdown
114 lines
5.9 KiB
Markdown
|
|
# 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.
|