diff --git a/.coderabbit.yaml b/.coderabbit.yaml index c444db6..824c303 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -116,6 +116,15 @@ reviews: - Ensure pytest runs with -v for verbosity and --cov for coverage reporting - Validate coverage report upload step is present + - path: "alembic/versions/**/*.py" + instructions: | + - Migration files are append-only once merged. Fix forward with a new + migration; never edit existing versions. See ADR-0010. + - Seed data changes must go in a new migration, not standalone scripts. + ADR-0010 superseded direct seed scripts (ADR-0002); do not re-introduce + them. + - render_as_batch=True must remain enabled for SQLite batch ALTER support. + path_filters: - "!**/__pycache__/**" - "!**/.pytest_cache/**" @@ -169,19 +178,23 @@ reviews: If so, update the relevant sections of README.md to reflect the current state. Do not rewrite sections unrelated to the changes. - ## 3. .github/copilot-instructions.md + ## 3. CLAUDE.md If the PR introduces patterns, conventions, or architectural decisions that should guide future AI-assisted contributions, add or update the relevant - instructions in .github/copilot-instructions.md. + instructions in CLAUDE.md. Focus on things a developer (or AI assistant) unfamiliar with this specific stack implementation should know before writing code here. - name: "enforce http error handling" instructions: | Audit all HTTP handler functions in the changed files. - Verify that errors return appropriate HTTP status codes (400 for bad input, - 404 for not found, 500 for unexpected errors) and a consistent JSON error - body with at least a "message" field. + Verify that errors return appropriate HTTP status codes: + - 422 Unprocessable Entity for Pydantic validation failures (payload shape + or field constraint violations) — NOT 400 + - 400 Bad Request ONLY for semantic errors (e.g. squad number in the URL + does not match the body on PUT) + - 404 Not Found for missing resources + - 409 Conflict for duplicate squad number on POST Flag handlers that return 200 on error, swallow errors silently, or use bare status-only responses without a JSON body. Do not make changes; only report findings as a comment so fixes can be @@ -334,7 +347,7 @@ knowledge_base: code_guidelines: enabled: true filePatterns: - - ".github/copilot-instructions.md" + - "CLAUDE.md" learnings: scope: auto issues: diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md deleted file mode 100644 index f68ebbc..0000000 --- a/.github/copilot-instructions.md +++ /dev/null @@ -1,223 +0,0 @@ -# Custom Instructions - -## Overview - -REST API for managing football players built with Python and FastAPI. Implements -async CRUD operations with SQLAlchemy 2.0 (async), SQLite, Pydantic validation, -and in-memory caching. - -## Tech Stack - -- **Language**: Python 3.13 -- **Framework**: FastAPI + Uvicorn -- **ORM**: SQLAlchemy 2.0 (async) + aiosqlite -- **Database**: SQLite (local/test), PostgreSQL-compatible -- **Migrations**: Alembic (async, `render_as_batch=True`) -- **Validation**: Pydantic -- **Caching**: aiocache (in-memory, 10-minute TTL) -- **Testing**: pytest + pytest-cov + httpx -- **Linting/Formatting**: Flake8 + Black -- **Containerization**: Docker - -## Structure - -```text -main.py — application entry point: FastAPI setup, router registration -alembic.ini — Alembic configuration (sqlalchemy.url set dynamically) -alembic/ — Alembic migration environment and version scripts -routes/ — HTTP route definitions + dependency injection [HTTP layer] -services/ — async business logic + cache management [business layer] -schemas/ — SQLAlchemy ORM models (database schema) [data layer] -databases/ — async SQLAlchemy session setup + get_database_url() -models/ — Pydantic models for request/response validation -scripts/ — shell scripts for Docker (entrypoint.sh, healthcheck.sh) -tools/ — legacy standalone seed scripts (superseded by Alembic migrations) -tests/ — pytest integration tests -``` - -**Layer rule**: `Routes → Services → SQLAlchemy → SQLite`. Routes handle HTTP -concerns only; business logic belongs in services. Never skip a layer. - -## Coding Guidelines - -- **Naming**: snake_case (files, functions, variables), PascalCase (classes) -- **Type hints**: Required everywhere — functions, variables, return types -- **Async**: All routes and service functions must be `async def`; use - `AsyncSession` (never `Session`); use `aiosqlite` (never `sqlite3`); use - SQLAlchemy 2.0 `select()` (never `session.query()`) -- **API contract**: camelCase JSON via Pydantic `alias_generator=to_camel`; - Python internals stay snake_case -- **Models**: `PlayerRequestModel` (no `id`, used for POST/PUT) and - `PlayerResponseModel` (includes `id: UUID`, used for GET/POST responses). - One request model intentionally covers both POST and PUT — per-operation - differences (conflict check on POST, mismatch guard on PUT) are handled at - the route layer, not by duplicating the model. Never reintroduce the removed - `PlayerModel`; it was removed because a single flat model conflated ORM, - request, and response concerns. -- **Primary key**: UUID surrogate key (`id`) — opaque, internal, used for GET - by id only. UUID v4 for API-created records; UUID v5 (deterministic) for - migration-seeded records. `squad_number` is the natural key — - human-readable, domain-meaningful, used for all mutation endpoints (PUT, - DELETE) and preferred for all external consumers -- **Caching**: cache key `"players"` (hardcoded); clear on POST/PUT/DELETE; - `X-Cache` header (HIT/MISS) -- **Errors**: Catch specific exceptions with rollback in services; Pydantic - validation returns 422 (not 400); squad number mismatch on PUT returns 400 - (not 422 — it is a semantic error, not a validation failure) -- **Logging**: `logging` module only; never `print()` -- **Line length**: 88; complexity ≤ 10 -- **Import order**: stdlib → third-party → local -- **Tests**: integration tests against the real SQLite DB (seeded via - Alembic migrations) via `TestClient` — no mocking. Naming pattern - `test_request_{method}_{resource}_{context}_response_{outcome}`; - docstrings single-line, concise; `tests/player_fake.py` for test data; - `tests/conftest.py` provides a `function`-scoped `client` fixture for - isolation; `tests/test_main.py` excluded from Black; - `tests/test_migrations.py` covers Alembic downgrade paths -- **Decisions**: justify every decision on its own technical merits; never use - "another project does it this way" as a reason — that explains nothing and - may mean replicating a mistake -- **Avoid**: sync DB access, mixing sync/async, `print()`, missing type hints, - unhandled exceptions - -## Commands - -### Quick Start - -```bash -# Setup (using uv) -uv venv -source .venv/bin/activate # Linux/macOS; use .venv\Scripts\activate on Windows -uv pip install --group dev - -# Apply migrations (required once before first run, and after down -v) -uv run alembic upgrade head - -# Run application -uv run uvicorn main:app --reload --port 9000 # http://localhost:9000/docs - -# Run tests -uv run pytest # run tests -uv run pytest --cov=./ --cov-report=term # with coverage (target >=80%) - -# Linting and formatting -uv run flake8 . -uv run black --check . - -# Migration workflow -uv run alembic upgrade head # apply all pending migrations -uv run alembic downgrade -1 # roll back last migration -uv run alembic revision --autogenerate -m "desc" # generate migration from schema - -# Docker -docker compose up -docker compose down -v -``` - -### Pre-commit Checks - -1. Update `CHANGELOG.md` `[Unreleased]` section (Added / Changed / Fixed / - Removed) -2. `uv run flake8 .` — must pass -3. `uv run black --check .` — must pass -4. `uv run pytest` — all tests must pass -5. `uv run pytest --cov=./ --cov-report=term` — coverage must be ≥80% -6. Commit message follows Conventional Commits format (enforced by commitlint) - -### Commits - -Format: `type(scope): description (#issue)` — max 80 chars -Types: `feat` `fix` `chore` `docs` `test` `refactor` `ci` `perf` -Example: `feat(api): add player stats endpoint (#42)` - -### Releases - -Tags follow the format `v{MAJOR}.{MINOR}.{PATCH}-{COACH}` (e.g. -`v2.0.0-capello`). The CD pipeline validates the coach name against a fixed -list (A–Z): - -``` -ancelotti bielsa capello delbosque eriksson ferguson guardiola heynckes -inzaghi klopp kovac low mourinho nagelsmann ottmar pochettino queiroz -ranieri simeone tuchel unai vangaal wenger xavi yozhef zeman -``` - -Never suggest a release tag with a coach name not on this list. - -## Agent Mode - -### Proceed freely - -- Add/modify routes in `routes/player_route.py` and `routes/health_route.py` -- Add/modify service methods in `services/player_service.py` -- Add/modify Pydantic models in `models/player_model.py` (field additions or - docstring updates that don't change the API contract) -- Tests in `tests/` — maintain async patterns, naming convention, and - integration-test approach (no mocking) -- Documentation and docstring updates -- Lint/format fixes -- Refactoring within existing architectural patterns - -### Ask before changing - -- Database schema (`schemas/player_schema.py`) and Alembic migrations - (`alembic/versions/`) — schema changes require a new migration file; - seed data changes require updating the relevant migration and any test - fixtures that reference specific UUIDs -- `models/player_model.py` design decisions — especially splitting or merging - request/response models; discuss the rationale before restructuring -- Dependencies (`pyproject.toml` with PEP 735 dependency groups) -- CI/CD configuration (`.github/workflows/`) -- Docker setup (`Dockerfile`, `docker-compose.yml`, `scripts/`) -- Breaking API contract changes (field renames, type changes, removing fields) -- Global error handling middleware -- HTTP status codes assigned to existing error conditions - -### Never modify - -- `.env` files (secrets) -- `alembic/versions/` migration files once merged to `master` — migrations - are append-only; fix forward with a new migration, never edit history -- Production configurations - -### Creating Issues - -This project uses Spec-Driven Development (SDD): discuss in Plan mode first, create a GitHub Issue as the spec artifact, then implement. Always offer to draft an issue before writing code. - -**Feature request** (`enhancement` label): -- **Problem**: the pain point being solved -- **Proposed Solution**: expected behavior and functionality -- **Suggested Approach** *(optional)*: implementation plan if known -- **Acceptance Criteria**: at minimum — behaves as proposed, tests added/updated, no regressions -- **References**: related issues, docs, or examples - -**Bug report** (`bug` label): -- **Description**: clear summary of the bug -- **Steps to Reproduce**: numbered, minimal steps -- **Expected / Actual Behavior**: one section each -- **Environment**: runtime versions + OS -- **Additional Context**: logs, screenshots, stack traces -- **Possible Solution** *(optional)*: suggested fix or workaround - -### Key workflows - -**Add an endpoint**: Add Pydantic model in `models/` if the request/response -shape is new → add async service method in `services/` with error handling and -rollback → add route in `routes/` with `Depends(generate_async_session)` → -add tests following the naming pattern → run pre-commit checks. - -**Modify schema**: Update `schemas/player_schema.py` → run -`uv run alembic revision --autogenerate -m "description"` to generate a -migration → review and adjust the generated file in `alembic/versions/` → -run `uv run alembic upgrade head` → update `models/player_model.py` if the -API shape changes → update services and tests → run `pytest`. - -**After completing work**: Propose a branch name and commit message for user -approval. Do not create a branch, commit, or push until the user explicitly -confirms. Commit message format: - -```text -feat(scope): description (#issue) - -Co-authored-by: Claude Sonnet 4.6 -``` diff --git a/CHANGELOG.md b/CHANGELOG.md index cb28d07..90d7ea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,11 +51,27 @@ This project uses famous football coaches as release codenames, following an A-Z ...}` to route decorators (OpenAPI schema) and `Raises:` entries to docstrings; clarifies the 422 (Pydantic validation failure) vs 400 (semantic ambiguity) distinction (#568) +- `CLAUDE.md`: consolidated from `.github/copilot-instructions.md` as the + single source of truth for AI coding assistants; added Invariants section and + Architecture Decision Records section; added pre-commit step 7 for ADR + updates; corrected cache layer description (caching is in routes, not + services) (#590) +- `.coderabbit.yaml`: updated `enforce http error handling` finishing touch with + precise 422/400/404/409 status code guidance; updated `sync documentation` + finishing touch and `knowledge_base.code_guidelines` to reference `CLAUDE.md`; + added `alembic/versions/**/*.py` path instruction enforcing append-only + migration policy (#590) ### Fixed +- `docs/adr/README.md`: corrected ADR-0002 status from `Accepted` to + `Superseded`; added missing ADR-0010 row (#590) + ### Removed +- `.github/copilot-instructions.md`: content consolidated into `CLAUDE.md`; + GitHub Copilot is no longer in use (#590) + --- ## [2.1.1 - Eriksson] - 2026-04-11 diff --git a/CLAUDE.md b/CLAUDE.md index 8ef5c13..a79ad2b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,7 +1,244 @@ # CLAUDE.md -@.github/copilot-instructions.md - ## Claude Code - Run `/pre-commit` to execute the full pre-commit checklist for this project. + +## Overview + +REST API for managing football players built with Python and FastAPI. Implements +async CRUD operations with SQLAlchemy 2.0 (async), SQLite, Pydantic validation, +and in-memory caching. + +## Tech Stack + +- **Language**: Python 3.13 +- **Framework**: FastAPI + Uvicorn +- **ORM**: SQLAlchemy 2.0 (async) + aiosqlite +- **Database**: SQLite (local/test), PostgreSQL-compatible +- **Migrations**: Alembic (async, `render_as_batch=True`) +- **Validation**: Pydantic +- **Caching**: aiocache (in-memory, 10-minute TTL) +- **Testing**: pytest + pytest-cov + httpx +- **Linting/Formatting**: Flake8 + Black +- **Containerization**: Docker + +## Structure + +```text +main.py — application entry point: FastAPI setup, router registration +alembic.ini — Alembic configuration (sqlalchemy.url set dynamically) +alembic/ — Alembic migration environment and version scripts +routes/ — HTTP route definitions, caching + dependency injection [HTTP layer] +services/ — async business logic [business layer] +schemas/ — SQLAlchemy ORM models (database schema) [data layer] +databases/ — async SQLAlchemy session setup + get_database_url() +models/ — Pydantic models for request/response validation +scripts/ — shell scripts for Docker (entrypoint.sh, healthcheck.sh) +tools/ — legacy standalone seed scripts (superseded by Alembic migrations) +tests/ — pytest integration tests +``` + +**Layer rule**: `Routes → Services → SQLAlchemy → SQLite`. Routes handle HTTP +concerns only; business logic belongs in services. Never skip a layer. + +## Coding Guidelines + +- **Naming**: snake_case (files, functions, variables), PascalCase (classes) +- **Type hints**: Required everywhere — functions, variables, return types +- **Async**: All routes and service functions must be `async def`; use + `AsyncSession` (never `Session`); use `aiosqlite` (never `sqlite3`); use + SQLAlchemy 2.0 `select()` (never `session.query()`) +- **API contract**: camelCase JSON via Pydantic `alias_generator=to_camel`; + Python internals stay snake_case +- **Models**: `PlayerRequestModel` (no `id`, used for POST/PUT) and + `PlayerResponseModel` (includes `id: UUID`, used for GET/POST responses). + One request model intentionally covers both POST and PUT — per-operation + differences (conflict check on POST, mismatch guard on PUT) are handled at + the route layer, not by duplicating the model. Never reintroduce the removed + `PlayerModel`; it was removed because a single flat model conflated ORM, + request, and response concerns. +- **Primary key**: UUID surrogate key (`id`) — opaque, internal, used for GET + by id only. UUID v4 for API-created records; UUID v5 (deterministic) for + migration-seeded records. `squad_number` is the natural key — + human-readable, domain-meaningful, used for all mutation endpoints (PUT, + DELETE) and preferred for all external consumers +- **Caching**: cache key `"players"` (hardcoded); clear on POST/PUT/DELETE; + `X-Cache` header (HIT/MISS) +- **Errors**: Catch specific exceptions with rollback in services; Pydantic + validation returns 422 (not 400); squad number mismatch on PUT returns 400 + (not 422 — it is a semantic error, not a validation failure) +- **Logging**: `logging` module only; never `print()` +- **Line length**: 88; complexity ≤ 10 +- **Import order**: stdlib → third-party → local +- **Tests**: integration tests against the real SQLite DB (seeded via + Alembic migrations) via `TestClient` — no mocking. Naming pattern + `test_request_{method}_{resource}_{context}_response_{outcome}`; + docstrings single-line, concise; `tests/player_fake.py` for test data; + `tests/conftest.py` provides a `function`-scoped `client` fixture for + isolation; `tests/test_main.py` excluded from Black; + `tests/test_migrations.py` covers Alembic downgrade paths +- **Decisions**: justify every decision on its own technical merits; never use + "another project does it this way" as a reason — that explains nothing and + may mean replicating a mistake +- **Avoid**: sync DB access, mixing sync/async, `print()`, missing type hints, + unhandled exceptions + +## Commands + +### Quick Start + +```bash +# Setup (using uv) +uv venv +source .venv/bin/activate # Linux/macOS; use .venv\Scripts\activate on Windows +uv pip install --group dev + +# Apply migrations (required once before first run, and after down -v) +uv run alembic upgrade head + +# Run application +uv run uvicorn main:app --reload --port 9000 # http://localhost:9000/docs + +# Run tests +uv run pytest # run tests +uv run pytest --cov=./ --cov-report=term # with coverage (target >=80%) + +# Linting and formatting +uv run flake8 . +uv run black --check . + +# Migration workflow +uv run alembic upgrade head # apply all pending migrations +uv run alembic downgrade -1 # roll back last migration +uv run alembic revision --autogenerate -m "desc" # generate migration from schema + +# Docker +docker compose up +docker compose down -v +``` + +### Pre-commit Checks + +1. Update `CHANGELOG.md` `[Unreleased]` section (Added / Changed / Fixed / + Removed) +2. `uv run flake8 .` — must pass +3. `uv run black --check .` — must pass +4. `uv run pytest` — all tests must pass +5. `uv run pytest --cov=./ --cov-report=term` — coverage must be ≥80% +6. Commit message follows Conventional Commits format (enforced by commitlint) +7. If this commit introduces or changes an architectural decision, update + `CLAUDE.md` and create or amend the relevant ADR in `docs/adr/` + +### Commits + +Format: `type(scope): description (#issue)` — max 80 chars +Types: `feat` `fix` `chore` `docs` `test` `refactor` `ci` `perf` +Example: `feat(api): add player stats endpoint (#42)` + +### Releases + +Tags follow the format `v{MAJOR}.{MINOR}.{PATCH}-{COACH}` (e.g. +`v2.0.0-capello`). The CD pipeline validates the coach name against a fixed +list (A–Z): + +``` +ancelotti bielsa capello delbosque eriksson ferguson guardiola heynckes +inzaghi klopp kovac low mourinho nagelsmann ottmar pochettino queiroz +ranieri simeone tuchel unai vangaal wenger xavi yozhef zeman +``` + +Never suggest a release tag with a coach name not on this list. + +## Agent Mode + +### Proceed freely + +- Add/modify routes in `routes/player_route.py` and `routes/health_route.py` +- Add/modify service methods in `services/player_service.py` +- Add/modify Pydantic models in `models/player_model.py` (field additions or + docstring updates that don't change the API contract) +- Tests in `tests/` — maintain async patterns, naming convention, and + integration-test approach (no mocking) +- Documentation and docstring updates +- Lint/format fixes +- Refactoring within existing architectural patterns + +### Ask before changing + +- Database schema (`schemas/player_schema.py`) and Alembic migrations + (`alembic/versions/`) — schema changes require a new migration file; + seed data changes require updating the relevant migration and any test + fixtures that reference specific UUIDs +- `models/player_model.py` design decisions — especially splitting or merging + request/response models; discuss the rationale before restructuring +- Dependencies (`pyproject.toml` with PEP 735 dependency groups) +- CI/CD configuration (`.github/workflows/`) +- Docker setup (`Dockerfile`, `docker-compose.yml`, `scripts/`) +- Breaking API contract changes (field renames, type changes, removing fields) +- Global error handling middleware +- HTTP status codes assigned to existing error conditions + +### Never modify + +- `.env` files (secrets) +- `alembic/versions/` migration files once merged to `master` — migrations + are append-only; fix forward with a new migration, never edit history +- Production configurations + +### Creating Issues + +This project uses Spec-Driven Development (SDD): discuss in Plan mode first, create a GitHub Issue as the spec artifact, then implement. Always offer to draft an issue before writing code. + +**Feature request** (`enhancement` label): +- **Problem**: the pain point being solved +- **Proposed Solution**: expected behavior and functionality +- **Suggested Approach** *(optional)*: implementation plan if known +- **Acceptance Criteria**: at minimum — behaves as proposed, tests added/updated, no regressions +- **References**: related issues, docs, or examples + +**Bug report** (`bug` label): +- **Description**: clear summary of the bug +- **Steps to Reproduce**: numbered, minimal steps +- **Expected / Actual Behavior**: one section each +- **Environment**: runtime versions + OS +- **Additional Context**: logs, screenshots, stack traces +- **Possible Solution** *(optional)*: suggested fix or workaround + +### Key workflows + +**Add an endpoint**: Add Pydantic model in `models/` if the request/response +shape is new → add async service method in `services/` with error handling and +rollback → add route in `routes/` with `Depends(generate_async_session)` → +add tests following the naming pattern → run pre-commit checks. + +**Modify schema**: Update `schemas/player_schema.py` → run +`uv run alembic revision --autogenerate -m "description"` to generate a +migration → review and adjust the generated file in `alembic/versions/` → +run `uv run alembic upgrade head` → update `models/player_model.py` if the +API shape changes → update services and tests → run `pytest`. + +**After completing work**: Propose a branch name and commit message for user +approval. Do not create a branch, commit, or push until the user explicitly +confirms. Commit message format: + +```text +feat(scope): description (#issue) + +Co-authored-by: Claude Sonnet 4.6 +``` + +## Invariants (never change without explicit discussion) + +- **Port**: 9000 +- **API contract**: endpoints, HTTP status codes, and response shapes are fixed; + do not change them without explicit discussion +- **Commit format**: `type(scope): description (#issue)` — max 80 chars +- **Conventional Commits types**: `feat` `fix` `chore` `docs` `test` `refactor` `ci` `perf` +- **CHANGELOG.md**: `[Unreleased]` section must be updated before every commit + +## Architecture Decision Records + +Significant architectural decisions are documented in `docs/adr/` (ADR-0001–0010). +Load these before proposing structural changes. When a proposal would change an +accepted decision, create a new ADR rather than editing the existing one. diff --git a/docs/adr/README.md b/docs/adr/README.md index d2b1981..2b329c0 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -13,7 +13,7 @@ one and the three-part test. | # | 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 | +| [0002](0002-no-alembic-manual-seed-scripts.md) | No Alembic — Manual Seed Scripts | Superseded | 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 | @@ -21,3 +21,4 @@ one and the three-part test. | [0007](0007-integration-only-test-strategy.md) | Integration-Only Test Strategy | Accepted | 2026-03-21 | | [0008](0008-layered-architecture.md) | Layered Architecture with FastAPI Dependency Injection | Accepted | 2026-04-02 | | [0009](0009-docker-and-compose-strategy.md) | Docker and Compose Strategy | Accepted | 2026-04-02 | +| [0010](0010-alembic-migration-based-schema-management.md) | Alembic Migration-Based Schema Management | Accepted | 2026-04-09 |