From 180587cd46196746839b7583a922c7c2457be396 Mon Sep 17 00:00:00 2001 From: Nano Taboada <87288+nanotaboada@users.noreply.github.com> Date: Sun, 22 Mar 2026 12:03:04 -0300 Subject: [PATCH 1/2] docs(adr): initialize Architecture Decision Records (#482) Add docs/adr/ with 7 ADRs covering key decisions: SQLite as database engine, no Alembic with manual seed scripts, UUID surrogate primary key with v4/v5 split, squad number as mutation key, full replace PUT with no PATCH, in-memory caching with aiocache, and integration-only test strategy. Includes index (README.md), Nygard template, and updates to README, CONTRIBUTING, and CHANGELOG. ADRs for Alembic (#2), PATCH (#461), and unit tests (#490) reference their respective open issues. Closes #482 Co-authored-by: Claude Sonnet 4.6 --- CHANGELOG.md | 7 ++ CONTRIBUTING.md | 28 ++++++-- README.md | 6 ++ docs/adr/0001-sqlite-as-database-engine.md | 58 ++++++++++++++++ .../0002-no-alembic-manual-seed-scripts.md | 55 +++++++++++++++ docs/adr/0003-uuid-surrogate-primary-key.md | 66 ++++++++++++++++++ docs/adr/0004-squad-number-as-mutation-key.md | 66 ++++++++++++++++++ docs/adr/0005-full-replace-put-no-patch.md | 56 +++++++++++++++ .../0006-in-memory-caching-with-aiocache.md | 64 +++++++++++++++++ .../0007-integration-only-test-strategy.md | 68 +++++++++++++++++++ docs/adr/README.md | 21 ++++++ docs/adr/template.md | 31 +++++++++ 12 files changed, 522 insertions(+), 4 deletions(-) create mode 100644 docs/adr/0001-sqlite-as-database-engine.md create mode 100644 docs/adr/0002-no-alembic-manual-seed-scripts.md create mode 100644 docs/adr/0003-uuid-surrogate-primary-key.md create mode 100644 docs/adr/0004-squad-number-as-mutation-key.md create mode 100644 docs/adr/0005-full-replace-put-no-patch.md create mode 100644 docs/adr/0006-in-memory-caching-with-aiocache.md create mode 100644 docs/adr/0007-integration-only-test-strategy.md create mode 100644 docs/adr/README.md create mode 100644 docs/adr/template.md diff --git a/CHANGELOG.md b/CHANGELOG.md index eea9224..a91313a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,13 @@ This project uses famous football coaches as release codenames, following an A-Z ## [Unreleased] +### Added + +- Architecture Decision Records (ADRs) in `docs/adr/` documenting key + decisions: SQLite, no Alembic, UUID primary key, squad number as + mutation key, full replace PUT, in-memory caching, integration-only + tests (#482) + --- ## [2.0.0 - Capello] - 2026-03-17 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d8aff5d..f79bee8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,27 @@ We value **incremental, detail‑first contributions** over big rewrites or abst - Run `pytest` before pushing. - Ensure coverage isn’t regressing. -## 3. Pull Request Workflow +## 3. Architecture Decision Records + +Significant architectural decisions are documented as ADRs in +`docs/adr/`. Before changing or replacing a decision captured in an +ADR, write a new ADR that supersedes it — do not edit the original. + +**When to write an ADR:** apply the three-part test: +1. A real fork existed — a genuine alternative was considered and + rejected. +2. The code doesn't explain the why — a new contributor reading the + source cannot infer the reasoning. +3. Revisiting it would be costly — changing it requires significant + rework. + +All three must be true. Process conventions (commit format, branch +naming) and project principles (audience, tone) do not belong in ADRs. + +ADRs are append-only. To change a decision: write a new ADR with +status `SUPERSEDED by ADR-NNNN` on the old one. + +## 4. Pull Request Workflow - **One logical change per PR.** - **Rebase or squash** before opening to keep history clean. @@ -45,13 +65,13 @@ We value **incremental, detail‑first contributions** over big rewrites or abst - Use Conventional Commit format. - Explain _what_ and _why_ concisely in the PR body. -## 4. Issue Reporting +## 5. Issue Reporting - Search open issues before creating a new one. - Include clear steps to reproduce and environment details. - Prefer **focused** issues—don’t bundle multiple topics. -## 5. Automation & Checks +## 6. Automation & Checks All PRs and pushes go through CI: @@ -62,7 +82,7 @@ All PRs and pushes go through CI: PRs must pass all checks to be reviewed. -## 6. Code of Conduct & Support +## 7. Code of Conduct & Support - See `CODE_OF_CONDUCT.md` for guidelines and reporting. - For questions or planning, open an issue and use the `discussion` label, or mention a maintainer. diff --git a/README.md b/README.md index 8566ae5..5e89bc9 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ Proof of Concept for a RESTful API built with [Python 3](https://www.python.org/ - [Releases](#releases) - [Environment Variables](#environment-variables) - [Command Summary](#command-summary) +- [Architecture Decisions](#architecture-decisions) - [Contributing](#contributing) - [Legal](#legal) @@ -404,6 +405,11 @@ PYTHONUNBUFFERED=1 | `docker compose down` | Stop Docker container | | `docker compose down -v` | Stop and remove Docker volume | +## Architecture Decisions + +Key architectural decisions are documented as ADRs in +[`docs/adr/`](docs/adr/README.md). + ## Contributing Contributions are welcome! Please read [CONTRIBUTING.md](CONTRIBUTING.md) for details on the code of conduct and the process for submitting pull requests. diff --git a/docs/adr/0001-sqlite-as-database-engine.md b/docs/adr/0001-sqlite-as-database-engine.md new file mode 100644 index 0000000..94dd4d2 --- /dev/null +++ b/docs/adr/0001-sqlite-as-database-engine.md @@ -0,0 +1,58 @@ +# ADR-0001: SQLite as Database Engine + +Date: 2026-03-21 + +## Status + +Accepted + +## Context + +This project is a proof-of-concept REST API. A database engine was +required to persist player data. The realistic alternatives were: + +- **PostgreSQL** — the standard choice for production REST APIs; requires + a running server process, a connection string, and either a local + installation or a Docker service alongside the application. +- **MySQL / MariaDB** — similar trade-offs to PostgreSQL. +- **SQLite** — an embedded, file-based engine; no server process, no + installation beyond a driver, and the database file can be committed + to the repository pre-seeded. + +The project includes a pre-seeded database file (`storage/players-sqlite3.db`) +that contains all 26 players from the Argentina 2022 World Cup squad. +SQLAlchemy 2.0 abstracts most engine-specific SQL, and aiosqlite provides +an async driver compatible with the rest of the async stack. + +## Decision + +We will use SQLite as the database engine via `aiosqlite` and +SQLAlchemy 2.0 (async). + +The deciding factor is operational simplicity for a PoC: zero server +infrastructure, a self-contained database file that can be seeded once +and committed, and a one-command startup (`uv run uvicorn main:app`). +The async driver (`aiosqlite`) keeps the stack consistent with the +rest of the async I/O patterns, and SQLAlchemy abstracts enough of the +engine differences that migrating to PostgreSQL later would require +minimal changes to the application code. + +## Consequences + +**Positive:** +- No external service dependency — the application runs with a single + command and no Docker Compose required for development. +- The pre-seeded database file ships with the repository, making the + project immediately runnable with real data. +- SQLAlchemy 2.0 abstracts most engine-specific behavior; a future + migration to PostgreSQL is feasible with driver and connection string + changes only. + +**Negative:** +- SQLite does not support concurrent writes. This is acceptable for a + single-instance PoC but rules out horizontal scaling without a + database change. +- Some PostgreSQL features (e.g. `RETURNING`, advisory locks, full-text + search) are unavailable or behave differently. +- The committed database file can accumulate stale data if seed scripts + are updated but the file is not regenerated manually. diff --git a/docs/adr/0002-no-alembic-manual-seed-scripts.md b/docs/adr/0002-no-alembic-manual-seed-scripts.md new file mode 100644 index 0000000..464eca3 --- /dev/null +++ b/docs/adr/0002-no-alembic-manual-seed-scripts.md @@ -0,0 +1,55 @@ +# ADR-0002: No Alembic — Manual Seed Scripts + +Date: 2026-03-21 + +## Status + +Accepted. Migration to Alembic is under consideration — tracked in +issue #2. + +## Context + +SQLAlchemy projects typically use Alembic to manage schema migrations: +versioned migration scripts, upgrade/downgrade paths, and a migration +history table. The alternatives considered were: + +- **Alembic** — the standard SQLAlchemy migration tool; provides + versioned, reversible migrations and is well-supported in production. +- **Manual seed scripts** — standalone Python scripts in `tools/` that + create the schema and populate data directly using SQLAlchemy models; + no migration history, no upgrade/downgrade concept. +- **No seeding** — start from an empty database and rely on the API to + create all data; unsuitable since the project ships a fixed dataset + (Argentina 2022 squad). + +The project uses a fixed, pre-seeded SQLite database file +(`storage/players-sqlite3.db`) committed to the repository. Schema +evolution has been infrequent and handled by regenerating the file +manually from updated seed scripts. + +## Decision + +We will use standalone seed scripts in `tools/` instead of Alembic. + +For a PoC with a stable schema and a single committed database file, +Alembic adds tooling overhead (initialization, migration directory, +`env.py` configuration) without meaningful benefit. The seed scripts +(`tools/seed_001_starting_eleven.py`, `tools/seed_002_substitutes.py`) +are self-contained and produce a deterministic output using UUID v5 +for stable, reproducible primary keys. + +## Consequences + +**Positive:** +- No Alembic dependency or configuration; simpler project setup. +- Seed scripts are plain Python, easy to read and modify. +- Deterministic UUID v5 keys mean the seeded database is identical + across environments and safe to reference in tests. + +**Negative:** +- No migration history. Schema changes require manually updating the + database file and rerunning seed scripts; there is no upgrade path. +- Not suitable for a production deployment where data must be preserved + across schema changes. +- As schema complexity grows, the absence of migration tooling becomes + increasingly costly. Issue #2 tracks the future adoption of Alembic. diff --git a/docs/adr/0003-uuid-surrogate-primary-key.md b/docs/adr/0003-uuid-surrogate-primary-key.md new file mode 100644 index 0000000..99ad44d --- /dev/null +++ b/docs/adr/0003-uuid-surrogate-primary-key.md @@ -0,0 +1,66 @@ +# ADR-0003: UUID Surrogate Primary Key with v4/v5 Split + +Date: 2026-03-21 + +## Status + +Accepted + +## Context + +A primary key strategy was required for the `players` table. The +candidates considered were: + +- **Auto-increment integer** — simple, compact, sequential; but leaks + row count and insertion order to clients, and integer IDs are + predictable, which can be a security concern for resource enumeration. +- **UUID v4** — randomly generated, opaque, non-sequential; no + information leaked; standard for API-created resources. +- **UUID v7 / ULID** — time-ordered UUIDs; better index locality than + v4 but add a dependency and are less universally supported. +- **Natural key (`squad_number`)** — human-readable, but not stable + across squads or seasons; unsuitable as a surrogate. + +An additional constraint was seeded data: the project ships 26 +pre-seeded players that must have stable, reproducible primary keys +across environments so that tests can reference them by ID. + +UUID v4 (random) cannot satisfy this: regenerating the seed would +produce different IDs each time. UUID v5 (deterministic, derived from +a namespace and a name) produces the same UUID for the same input, +making seeded records reproducible. + +## Decision + +We will use a UUID surrogate primary key stored as a hyphenated string +(`HyphenatedUUID` custom SQLAlchemy type). API-created records receive +a UUID v4 (random); migration-seeded records receive a UUID v5 +(deterministic, derived from the player's squad number). + +The v4/v5 split preserves the benefits of each approach in its context: +randomness for API-created records (opaque, non-predictable), and +determinism for seeded records (stable across environments, safe for +test fixtures). + +The UUID is intentionally opaque to external clients. It is exposed +only via `GET /players/{player_id}` and `POST /players/` responses. +All mutation endpoints (PUT, DELETE) use `squad_number` as the +identifier — see ADR-0004. + +## Consequences + +**Positive:** +- UUIDs are opaque; no information about row count or insertion order + is exposed to clients. +- The v5/v4 split means seeded records have stable IDs across + environments, safe to hard-code in tests. +- `HyphenatedUUID` stores IDs as standard hyphenated strings in SQLite, + readable without special tooling. + +**Negative:** +- UUIDs are larger than integers (36 chars as strings), which has a + minor storage and index performance cost — acceptable at PoC scale. +- The v4/v5 distinction is non-obvious; developers unfamiliar with the + codebase may not know why seeded records have deterministic IDs. +- Clients cannot use the UUID to infer insertion order or paginate + reliably by ID (UUID v4 is random). diff --git a/docs/adr/0004-squad-number-as-mutation-key.md b/docs/adr/0004-squad-number-as-mutation-key.md new file mode 100644 index 0000000..f17bf70 --- /dev/null +++ b/docs/adr/0004-squad-number-as-mutation-key.md @@ -0,0 +1,66 @@ +# ADR-0004: Squad Number as the Mutation Key + +Date: 2026-03-21 + +## Status + +Accepted + +## Context + +The API exposes two identifiers for a player: + +- **UUID** (`id`) — surrogate key; opaque, server-generated, stable. + See ADR-0003. +- **Squad number** (`squad_number`) — natural key; human-readable, + domain-meaningful (e.g. number 10 = Messi), unique within a squad. + +A design choice was required for which identifier mutation endpoints +(PUT, DELETE) and secondary GET endpoints should use as their path +parameter. The candidates were: + +- **UUID for all endpoints** — consistent use of the surrogate key; + clients must store UUIDs after POST or GET to perform mutations. +- **Squad number for mutations** — uses the natural, human-meaningful + key for the operations that domain users care about; UUID remains + available for internal/service-to-service use via + `GET /players/{player_id}`. + +Up to v1.x, the API used UUID for PUT and DELETE. Version 2.0.0 +(Capello) changed mutation endpoints to use squad number, introduced +`GET /players/squadnumber/{squad_number}` as a lookup alternative, and +retained `GET /players/{player_id}` for UUID-based lookup. + +## Decision + +We will use `squad_number` as the path parameter for all mutation +endpoints (`PUT /players/squadnumber/{squad_number}`, +`DELETE /players/squadnumber/{squad_number}`) and for the secondary +GET endpoint (`GET /players/squadnumber/{squad_number}`). + +Squad number is the identifier that domain users reason about. Requiring +clients to store and re-use a UUID to update or delete a player they +identified by squad number adds unnecessary indirection. Keeping UUID +lookup available (`GET /players/{player_id}`) preserves +service-to-service use cases where a stable opaque key is preferred. + +As a consequence of using squad number on PUT, the request body's +`squad_number` field must match the path parameter. A mismatch returns +HTTP 400; the path parameter is always authoritative. + +## Consequences + +**Positive:** +- Mutation endpoints are intuitive for domain users: + `PUT /players/squadnumber/10` clearly targets Messi's record. +- Clients do not need to perform a GET to obtain a UUID before mutating. +- UUID remains available for stable, opaque internal references. + +**Negative:** +- Squad numbers can change (a player re-assigned to a different number + requires a careful update sequence). The API has no special handling + for squad number changes — it is treated as any other field update. +- Two lookup endpoints (`/players/{id}` and + `/players/squadnumber/{squad_number}`) increase surface area slightly. +- The mismatch guard on PUT (body squad number must equal path + parameter) is an additional contract constraint clients must respect. diff --git a/docs/adr/0005-full-replace-put-no-patch.md b/docs/adr/0005-full-replace-put-no-patch.md new file mode 100644 index 0000000..6b4c1f9 --- /dev/null +++ b/docs/adr/0005-full-replace-put-no-patch.md @@ -0,0 +1,56 @@ +# ADR-0005: Full Replace PUT, No PATCH + +Date: 2026-03-21 + +## Status + +Accepted. Partial update support via PATCH is under consideration — +tracked in issue #461. + +## Context + +HTTP defines two semantics for updating a resource: + +- **PUT** — full replacement; the request body represents the complete + new state of the resource. Fields absent from the body are + overwritten with their zero/null values. +- **PATCH** — partial update; the request body contains only the fields + to change. Requires a patch format (JSON Merge Patch, JSON Patch) or + a convention for interpreting absent fields. + +Both are common in REST APIs. The choice affects the Pydantic model +design (all fields required vs optional), the service layer logic +(overwrite all vs merge), and the client contract (must send all fields +vs only changed fields). + +The current implementation uses a single `PlayerRequestModel` where all +domain fields are required. The service's +`update_by_squad_number_async` overwrites every field on the existing +record using the request body values. + +## Decision + +We will implement PUT as a full replacement operation only, with no +PATCH endpoint. + +Full replace is simpler to implement correctly: no merge logic, no +ambiguity about what an absent field means, and a single request model +covers both POST and PUT. For a PoC with a small, flat domain model +(10 fields), requiring the full resource on update is not a meaningful +burden for clients. + +## Consequences + +**Positive:** +- Simple, unambiguous semantics: the request body is the new state. +- No additional Pydantic model or merge logic required. +- No need to decide on a patch format (JSON Merge Patch vs JSON Patch). + +**Negative:** +- Clients must send the full resource even to change a single field, + which is verbose and risks accidental data loss if a field is omitted. +- Not suitable for resources with many fields, binary data, or + frequently partial updates. +- Issue #461 tracks the addition of PATCH for partial updates, which + would require an optional-fields model and merge logic in the service + layer. diff --git a/docs/adr/0006-in-memory-caching-with-aiocache.md b/docs/adr/0006-in-memory-caching-with-aiocache.md new file mode 100644 index 0000000..5d3b8f9 --- /dev/null +++ b/docs/adr/0006-in-memory-caching-with-aiocache.md @@ -0,0 +1,64 @@ +# ADR-0006: In-Memory Caching with aiocache + +Date: 2026-03-21 + +## Status + +Accepted + +## Context + +The `GET /players/` endpoint retrieves all players on every request. +With a fixed dataset and no real-time update requirements, repeated +database reads for the same data are unnecessary. The caching +strategies considered were: + +- **No cache** — simplest; every request hits the database. Acceptable + for a PoC but leaves an obvious optimization undemonstrated. +- **HTTP cache headers** (`Cache-Control`, `ETag`, `Last-Modified`) — + standard HTTP caching; delegates caching to the client or a reverse + proxy. Requires no server-side storage but adds response header logic + and shifts responsibility to the client. +- **In-process memory cache** — stores serialized results in the server + process memory; fast, zero infrastructure, invalidated explicitly on + write operations. +- **Distributed cache (Redis, Memcached)** — shares cache across + multiple server instances; requires additional infrastructure and a + client library. + +The project is a single-instance PoC with no horizontal scaling +requirement. The dataset is small (26 players) and changes only via the +API, making explicit cache invalidation straightforward. + +## Decision + +We will use `aiocache` with `SimpleMemoryCache` for in-process caching +of the `GET /players/` response, with a 10-minute TTL and explicit +cache invalidation on POST, PUT, and DELETE. + +`aiocache` is async-native and integrates cleanly with the FastAPI/ +async stack. `SimpleMemoryCache` requires no external service. The +single cache key (`"players"`) is sufficient for a flat collection +endpoint with no filtering or pagination. The `X-Cache: HIT/MISS` +response header makes cache behaviour observable to clients and +useful for testing. + +## Consequences + +**Positive:** +- Zero infrastructure: no Redis or Memcached service required. +- Async-native: no blocking I/O in the cache layer. +- Explicit invalidation on writes keeps cache consistency simple and + predictable. +- `X-Cache` header makes cache behaviour transparent and testable. + +**Negative:** +- In-process cache is not shared across multiple instances. A + horizontal scaling deployment would serve stale data from some + instances until their TTL expires or a write invalidates their local + cache. +- Cache is lost on process restart; the first request after restart + always hits the database. +- Only `GET /players/` (the full collection) is cached. Individual + player lookups (`GET /players/{id}`, + `GET /players/squadnumber/{n}`) are not cached. diff --git a/docs/adr/0007-integration-only-test-strategy.md b/docs/adr/0007-integration-only-test-strategy.md new file mode 100644 index 0000000..c185af0 --- /dev/null +++ b/docs/adr/0007-integration-only-test-strategy.md @@ -0,0 +1,68 @@ +# ADR-0007: Integration-Only Test Strategy + +Date: 2026-03-21 + +## Status + +Accepted. Unit tests with mocking for the service and route layers are +under consideration — tracked in issue #490. + +## Context + +The test suite could be structured in several ways: + +- **Unit tests with mocks** — each layer (routes, services) is tested + in isolation with its dependencies mocked. Fast, focused, but + requires maintaining mock objects that can diverge from real + behaviour. +- **Integration tests against a real database** — tests run against the + actual application stack, including the SQLite database. Slower per + test but exercises the full request/response cycle and catches + interaction bugs that mocks would miss. +- **Mixed** — unit tests for complex business logic, integration tests + for the HTTP layer. + +The project uses a pre-seeded SQLite database file committed to the +repository. FastAPI's `TestClient` (backed by `httpx`) allows +synchronous integration tests that exercise the full stack — routing, +dependency injection, service logic, ORM queries, and the database — +without a running server process. + +All tests live in `tests/test_main.py`, use stubs from +`tests/player_stub.py` for consistent test data, and rely on a +`function`-scoped `client` fixture from `conftest.py` for per-test +isolation. + +## Decision + +We will use integration tests only, exercising the full stack via +`TestClient` against the real pre-seeded SQLite database. No mocking. + +For a thin CRUD API where the business logic is primarily data +transformation and persistence, integration tests provide the highest +confidence per line of test code. The pre-seeded SQLite database is +fast enough for a small dataset, and `TestClient` makes the tests +synchronous and straightforward to write. Mocks would require +maintaining fake implementations of the service and ORM layers that +could silently diverge from reality. + +## Consequences + +**Positive:** +- Tests exercise the full stack; interaction bugs between layers are + caught. +- No mock infrastructure to maintain; tests remain valid as the + implementation evolves. +- `TestClient` makes tests synchronous and easy to read despite the + async application stack. +- 100% coverage is achievable and meaningful — covered lines are + exercised against a real database. + +**Negative:** +- Tests depend on the state of the pre-seeded database; a corrupted or + out-of-date database file breaks the suite. +- No isolation between application layers in tests; a failure does not + pinpoint which layer is responsible without further investigation. +- As business logic grows more complex, the absence of unit tests for + the service layer becomes a gap. Issue #490 tracks the addition of + unit tests with mocking for service and route layers. diff --git a/docs/adr/README.md b/docs/adr/README.md new file mode 100644 index 0000000..4118800 --- /dev/null +++ b/docs/adr/README.md @@ -0,0 +1,21 @@ +# Architecture Decision Records + +This directory contains Architecture Decision Records (ADRs) for the +`python-samples-fastapi-restful` project. + +An ADR captures a decision that had real alternatives, whose reasoning +cannot be inferred from the code alone, and that would be costly to +reverse. See [CONTRIBUTING.md](../../CONTRIBUTING.md) for when to write +one and the three-part test. + +## Index + +| # | Title | Status | Date | +|---|-------|--------|------| +| [0001](0001-sqlite-as-database-engine.md) | SQLite as Database Engine | Accepted | 2026-03-21 | +| [0002](0002-no-alembic-manual-seed-scripts.md) | No Alembic — Manual Seed Scripts | Accepted | 2026-03-21 | +| [0003](0003-uuid-surrogate-primary-key.md) | UUID Surrogate Primary Key with v4/v5 Split | Accepted | 2026-03-21 | +| [0004](0004-squad-number-as-mutation-key.md) | Squad Number as the Mutation Key | Accepted | 2026-03-21 | +| [0005](0005-full-replace-put-no-patch.md) | Full Replace PUT, No PATCH | Accepted | 2026-03-21 | +| [0006](0006-in-memory-caching-with-aiocache.md) | In-Memory Caching with aiocache | Accepted | 2026-03-21 | +| [0007](0007-integration-only-test-strategy.md) | Integration-Only Test Strategy | Accepted | 2026-03-21 | diff --git a/docs/adr/template.md b/docs/adr/template.md new file mode 100644 index 0000000..3d073fd --- /dev/null +++ b/docs/adr/template.md @@ -0,0 +1,31 @@ +# ADR-NNNN: Title (Short Noun Phrase) + +Date: YYYY-MM-DD + +## Status + +[PROPOSED | ACCEPTED | DEPRECATED | SUPERSEDED by ADR-NNNN] + +## Context + + + +## Decision + + + +## Consequences + + From f7757274e431021b003bcf9df8c6b5f184e12dc7 Mon Sep 17 00:00:00 2001 From: Nano Taboada <87288+nanotaboada@users.noreply.github.com> Date: Sun, 22 Mar 2026 13:06:13 -0300 Subject: [PATCH 2/2] docs(adr): apply targeted fixes from PR #535 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ADR-0003: clarify UUID v5 values are pre-computed constants in seed scripts, not derived at runtime from squad number - ADR-0005: replace "all domain fields are required" with accurate breakdown — 4 required fields, 6 optional - ADR-0007: rephrase hardcoded file paths to generic structure description - template.md: add explicit Alternatives Considered section Co-authored-by: Claude Sonnet 4.6 --- docs/adr/0003-uuid-surrogate-primary-key.md | 7 +++++-- docs/adr/0005-full-replace-put-no-patch.md | 10 ++++++---- docs/adr/0007-integration-only-test-strategy.md | 7 +++---- docs/adr/template.md | 14 +++++++++++--- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/docs/adr/0003-uuid-surrogate-primary-key.md b/docs/adr/0003-uuid-surrogate-primary-key.md index 99ad44d..99c8e14 100644 --- a/docs/adr/0003-uuid-surrogate-primary-key.md +++ b/docs/adr/0003-uuid-surrogate-primary-key.md @@ -28,14 +28,17 @@ across environments so that tests can reference them by ID. UUID v4 (random) cannot satisfy this: regenerating the seed would produce different IDs each time. UUID v5 (deterministic, derived from a namespace and a name) produces the same UUID for the same input, -making seeded records reproducible. +making seeded records reproducible. The project stores pre-computed +UUID v5 constants directly in the seed scripts rather than deriving +them at runtime from the squad number. ## Decision We will use a UUID surrogate primary key stored as a hyphenated string (`HyphenatedUUID` custom SQLAlchemy type). API-created records receive a UUID v4 (random); migration-seeded records receive a UUID v5 -(deterministic, derived from the player's squad number). +(deterministic, pre-computed and stored as constants in the seed +scripts). The v4/v5 split preserves the benefits of each approach in its context: randomness for API-created records (opaque, non-predictable), and diff --git a/docs/adr/0005-full-replace-put-no-patch.md b/docs/adr/0005-full-replace-put-no-patch.md index 6b4c1f9..e0b8b01 100644 --- a/docs/adr/0005-full-replace-put-no-patch.md +++ b/docs/adr/0005-full-replace-put-no-patch.md @@ -23,10 +23,12 @@ design (all fields required vs optional), the service layer logic (overwrite all vs merge), and the client contract (must send all fields vs only changed fields). -The current implementation uses a single `PlayerRequestModel` where all -domain fields are required. The service's -`update_by_squad_number_async` overwrites every field on the existing -record using the request body values. +The current implementation uses a single `PlayerRequestModel` shared by +both POST and PUT. Four fields are required (`first_name`, `last_name`, +`squad_number`, `position`); the remaining six are optional (`middle_name`, +`date_of_birth`, `abbr_position`, `team`, `league`, `starting11`). The +service's `update_by_squad_number_async` overwrites every field on the +existing record using the request body values. ## Decision diff --git a/docs/adr/0007-integration-only-test-strategy.md b/docs/adr/0007-integration-only-test-strategy.md index c185af0..76f08ed 100644 --- a/docs/adr/0007-integration-only-test-strategy.md +++ b/docs/adr/0007-integration-only-test-strategy.md @@ -28,10 +28,9 @@ synchronous integration tests that exercise the full stack — routing, dependency injection, service logic, ORM queries, and the database — without a running server process. -All tests live in `tests/test_main.py`, use stubs from -`tests/player_stub.py` for consistent test data, and rely on a -`function`-scoped `client` fixture from `conftest.py` for per-test -isolation. +All tests live in a single test module under `tests/`, use a stubs +module for consistent test data, and rely on a `function`-scoped +`client` fixture in `conftest.py` for per-test isolation. ## Decision diff --git a/docs/adr/template.md b/docs/adr/template.md index 3d073fd..8845121 100644 --- a/docs/adr/template.md +++ b/docs/adr/template.md @@ -10,9 +10,17 @@ Date: YYYY-MM-DD + +## Alternatives Considered + + ## Decision