diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 49c23fe..6d5e441 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -27,7 +27,7 @@ services: retries: 5 minio: - image: quay.io/minio/minio:latest + image: pgsty/minio:RELEASE.2026-08-04T00-00-00Z container_name: grandflow-minio command: server /data --console-address ":9001" ports: diff --git a/docker-compose.local.yml b/docker-compose.local.yml index 1132d4a..e684e92 100644 --- a/docker-compose.local.yml +++ b/docker-compose.local.yml @@ -30,7 +30,7 @@ services: - grandflow minio: - image: quay.io/minio/minio:latest + image: pgsty/minio:RELEASE.2026-08-04T00-00-00Z container_name: grandflow-minio-local command: server /data --console-address ":9001" ports: diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md index aaebfd8..ae8a828 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md @@ -8,6 +8,7 @@ See proposal.md - Why for the failure mode. Relevant existing state: - `BudgetModel.categories` (`services/budget/app/models/budget.py`) is a plain `relationship(..., back_populates="budget")` with no `cascade` and no `passive_deletes` setting. - `delete_budget_service` (`services/budget/app/services/budget_services.py`) calls `get_budget_service`, which does **not** eager-load `categories` in this path, then calls `session.delete(budget)` + `commit()`. - Without `passive_deletes=True`, SQLAlchemy's unit-of-work treats "this budget is being deleted" as "I must disassociate its `categories` collection" — since there's no delete cascade configured, it lazy-loads the (unloaded) collection during flush and issues an `UPDATE budget_categories SET budget_id = NULL ...` for each row, which the DB rejects (`budget_id` is `nullable=False`), surfacing as `IntegrityError` → the generic 400. +- The `minio` service in `docker-compose.local.yml` (used by the `E2E Tests` workflow) and `docker-compose.dev.yml` uses `quay.io/minio/minio:latest`. That image now needs authentication (401), and `minio/minio` no longer exists on Docker Hub (404). Developer machines still have a cached copy, but a fresh CI runner can't pull one, so e2e fails before any test runs. - Because the collection load happens inside `flush()`, which the SQLAlchemy asyncio extension runs inside a greenlet, this doesn't crash as a `MissingGreenlet` — it produces a normal `IntegrityError`, so it slipped past the `async-persistence` capability's existing "no implicit lazy-load" guard without tripping any greenlet-specific detection. ## Goals / Non-Goals @@ -15,6 +16,7 @@ See proposal.md - Why for the failure mode. Relevant existing state: **Goals:** - Make `DELETE /api/v1/budgets/{id}` succeed for a budget whose only remaining "problem" is a category row (no lines, reports, receipts, or currency conversions). - Keep the DB as the single source of truth for the cascade, rather than teaching the ORM to duplicate it. +- Restore a pullable MinIO image so the `E2E Tests` workflow can verify the fix on a fresh runner. **Non-Goals:** - Cleaning up orphaned `budget_categories` rows when a line is deleted (separate, pre-existing bookkeeping gap; not required for this fix — see proposal.md). @@ -22,12 +24,21 @@ See proposal.md - Why for the failure mode. Relevant existing state: ## Decisions -**Use `passive_deletes=True` on `BudgetModel.categories`, not ORM-level `cascade="all, delete-orphan"`.** -- `passive_deletes=True` tells SQLAlchemy "don't manage this relationship's deletes yourself, the DB's `ON DELETE CASCADE` already does it" — it skips the lazy-load-and-null-out step entirely and just issues `DELETE FROM budgets WHERE id = ...`, letting Postgres cascade. -- Alternative considered: `cascade="all, delete-orphan"`. This would make SQLAlchemy load the collection and emit an explicit `DELETE FROM budget_categories WHERE id IN (...)` before deleting the budget — functionally similar end state, but it still requires the lazy-load (same eager-load discipline the `async-persistence` capability calls for) and does redundant work the DB already does via its own `ON DELETE CASCADE`. `passive_deletes=True` is the smaller, more precise fix that matches the existing DB-level intent from migration `000014`. -- `passive_deletes=True` requires the DB constraint to actually be `ON DELETE CASCADE` (true here, confirmed in the migration) — it would be the wrong choice if the constraint were `RESTRICT`/`NO ACTION`, since then rows would just silently fail to delete. +**Use `cascade="all, delete-orphan", passive_deletes=True` together on `BudgetModel.categories`, not `passive_deletes=True` alone.** +- `passive_deletes=True` tells SQLAlchemy "don't manage this relationship's deletes yourself, the DB's `ON DELETE CASCADE` already does it" — when the collection is *unloaded* (true today, since `get_budget` doesn't eager-load `categories`), it skips the lazy-load-and-null-out step entirely and just issues `DELETE FROM budgets WHERE id = ...`, letting Postgres cascade. +- `passive_deletes=True` alone is not sufficient by itself, though: it only changes what SQLAlchemy does with an *unmanaged* collection. If `categories` is already loaded on the instance being deleted (e.g. a future caller adds `selectinload(BudgetModel.categories)` to `get_budget`, or otherwise touches the collection before `session.delete()`), plain `passive_deletes=True` still walks the already-materialized collection and nulls out each child's `budget_id`, hitting the same `IntegrityError` this change fixes — verified experimentally (SQLite, FK enforcement on). +- Pairing it with `cascade="all, delete-orphan"` closes that gap: with both set, an *unloaded* collection still takes the passive path (no lazy-load, DB cascades), and a *loaded* collection is instead cleaned up by SQLAlchemy's own delete-orphan cascade rather than being nulled out — correct either way, at the cost of one extra `DELETE FROM budget_categories WHERE id IN (...)` in the loaded case, which the DB's `ON DELETE CASCADE` would have done anyway. This is SQLAlchemy's documented pairing for "let the DB cascade when possible, but stay correct if the collection happens to be loaded." +- Both settings still require the DB constraint to actually be `ON DELETE CASCADE` (true here, confirmed in migration `000014`) — the wrong choice if the constraint were `RESTRICT`/`NO ACTION`, since then rows would just silently fail to delete in the unloaded case. + +**Swap the `minio` image to `pgsty/minio:RELEASE.2026-08-04T00-00-00Z`, pinned, in both `docker-compose.local.yml` and `docker-compose.dev.yml`.** +- `pgsty/minio` is a community rebuild of the same MinIO server using upstream's own Dockerfile and entrypoint. Checked against the registry and compared with the cached upstream image: same entrypoint (`docker-entrypoint.sh`) and `Cmd` (`minio`), identical env (`MINIO_ROOT_*_FILE`, `MC_CONFIG_DIR`, minisign key), and `/usr/bin/mc` is present (a symlink to `mcli`), so the existing `mc ready local` healthcheck still works. Builds exist for amd64 and arm64. The rest of the service block stays unchanged. +- Pin a release tag rather than `:latest`, so the next upstream change is a deliberate bump, not a CI break. +- Alternatives rejected: `chainguard/minio` (free tier offers `:latest` only, can't be pinned); `bitnamilegacy/minio` (frozen in August 2025, different entrypoint and env scheme, would need the service block rewritten); switching to a different S3 server such as Garage, SeaweedFS or RustFS (a storage decision far larger than a CI fix). +- Production (`docker-compose.prod.yml`) doesn't run MinIO, so it's unaffected. ## Risks / Trade-offs - [`passive_deletes=True` silently relies on the DB constraint matching model expectations; if a future migration changes `budget_categories_budget_id_fkey` back to `RESTRICT` without updating the model, budget deletion would start failing again with a real (unmasked) `IntegrityError`] → Acceptable: that failure mode is the same generic `IntegrityError` → 400 path already in place for lines/reports, so it fails safe (blocks deletion, doesn't corrupt data) rather than silently. - [Orphaned categories with zero lines still accumulate until their budget is deleted] → Out of scope per Non-Goals; tracked separately as a known bookkeeping gap, not a correctness or security issue. +- [`pgsty/minio` is a single-maintainer community build and could go stale or disappear the same way] → The pinned tag keeps CI reproducible while it exists. If it goes away, the swap is the same one-line change per file, and moving to another S3-compatible server becomes the follow-up decision. +- [Existing dev/local `minio_data` volumes were written by an older MinIO release] → MinIO reads data written by older releases. Developers pick up the new image only when they recreate their own minio container. diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md index 6e03845..b080710 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md @@ -6,8 +6,9 @@ ## What Changes -- `BudgetModel.categories` gets `passive_deletes=True` so SQLAlchemy's ORM defers to the database's existing `ON DELETE CASCADE` on `budget_categories.budget_id` instead of trying to lazy-load the collection and null out each child's non-nullable `budget_id` during flush (which raises the spurious `IntegrityError`). -- Out of scope: cleaning up an orphaned category when its last line is deleted is a separate, pre-existing bookkeeping gap (also related to the already-known global-category-namespace issue) and is not needed to fix this bug — `passive_deletes=True` makes budget deletion correct regardless of whether an orphaned category row exists. +- `BudgetModel.categories` gets `cascade="all, delete-orphan", passive_deletes=True` so SQLAlchemy defers to the database's existing `ON DELETE CASCADE` on `budget_categories.budget_id` instead of lazy-loading the collection and nulling out each child's non-nullable `budget_id` during flush (which raises the spurious `IntegrityError`), and deletes the children itself if the collection happens to be loaded (see design.md). +- The `minio` service in `docker-compose.local.yml` and `docker-compose.dev.yml` switches from `quay.io/minio/minio:latest` to the pinned community build `pgsty/minio:RELEASE.2026-08-04T00-00-00Z`. MinIO stopped publishing public images (quay.io now returns 401, Docker Hub's `minio/minio` returns 404), so a fresh CI runner can't start the e2e stack — without this, the `E2E Tests` workflow can't verify the fix above. +- Out of scope: cleaning up an orphaned category when its last line is deleted is a separate, pre-existing bookkeeping gap (also related to the already-known global-category-namespace issue) and is not needed to fix this bug — the cascade settings make budget deletion correct regardless of whether an orphaned category row exists. ## Capabilities @@ -23,4 +24,5 @@ None — this restores compliance with the already-declared `async-persistence` - `services/budget/app/models/budget.py` (`BudgetModel.categories` relationship) - Fixes `DELETE /api/v1/budgets/{id}` for any budget that ever had a line added and removed -- Unblocks `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` in the `E2E Tests` CI workflow +- `docker-compose.local.yml`, `docker-compose.dev.yml` (`minio` service image only; `command`, env, volumes and healthcheck unchanged) +- Unblocks `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` in the `E2E Tests` CI workflow, which currently fails at image pull on every branch, `main` included diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md index f8215ad..4581f1f 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md @@ -2,9 +2,22 @@ Workflow rule: one task group = one GitHub sub-issue (of this change's parent issue) = one PR, merged before the next group starts. -## 1. Fix budget deletion cascade for orphaned categories + +## Tracking -- [ ] 1.1 Add `passive_deletes=True` to `BudgetModel.categories` in `services/budget/app/models/budget.py` -- [ ] 1.2 Add/extend a `services/budget` test that creates a budget, adds a line (auto-creating a category), deletes the line, then deletes the budget, and verify `DELETE /budgets/{id}` returns 200 with `success: true` instead of a 400 -- [ ] 1.3 Run `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` against the local docker-compose e2e stack (or the `E2E Tests` CI workflow) and verify the DELETE-budget assertion passes -- [ ] 1.4 Run `services/budget`'s test suite and lint clean; PR merged +Parent issue: [#295](https://github.com/arutsh/OpenGrantFlow/issues/295) · [Project board](https://github.com/users/arutsh/projects/8) + +| Group | Sub-issue | Branch | State | +| --- | --- | --- | --- | +| 1. Fix budget deletion cascade for orphaned categories | [#346](https://github.com/arutsh/OpenGrantFlow/issues/346) | `Budget/fix/Issue-346/category-cascade-delete-fix-group1` | open | + +_Generated by `scripts/flow.py sync budget-fix-295-category-cascade-delete-fix` — do not edit by hand._ + + +## 1. Fix budget deletion cascade for orphaned categories — Issue #346 + +- [x] 1.1 Add `cascade="all, delete-orphan", passive_deletes=True` to `BudgetModel.categories` in `services/budget/app/models/budget.py` +- [x] 1.2 Add/extend real-DB `services/budget` tests: (a) create a budget, add a line (auto-creating a category), delete the line, delete the budget, verify success with `categories` unloaded; (b) same, but force `categories` loaded (e.g. re-fetch with `selectinload`) before deleting, verify success there too +- [x] 1.3 Swap the `minio` service image in `docker-compose.local.yml` and `docker-compose.dev.yml` from `quay.io/minio/minio:latest` to `pgsty/minio:RELEASE.2026-08-04T00-00-00Z` (see design.md), so the `E2E Tests` workflow can pull it on a fresh runner +- [ ] 1.4 Run `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` against the local docker-compose e2e stack (or the `E2E Tests` CI workflow) and verify the DELETE-budget assertion passes +- [ ] 1.5 Run `services/budget`'s test suite and lint clean; PR merged diff --git a/services/budget/app/models/budget.py b/services/budget/app/models/budget.py index 500eb3c..48229b2 100644 --- a/services/budget/app/models/budget.py +++ b/services/budget/app/models/budget.py @@ -81,7 +81,10 @@ class BudgetModel(Base, AuditMixin): ) reports: Mapped[list["ReportModel"]] = relationship("ReportModel", back_populates="budget") categories: Mapped[list["BudgetCategoryModel"]] = relationship( - "BudgetCategoryModel", back_populates="budget" + "BudgetCategoryModel", + back_populates="budget", + cascade="all, delete-orphan", + passive_deletes=True, ) diff --git a/services/budget/tests/test_budget_crud.py b/services/budget/tests/test_budget_crud.py index ae0359c..570fcf2 100644 --- a/services/budget/tests/test_budget_crud.py +++ b/services/budget/tests/test_budget_crud.py @@ -1,8 +1,13 @@ import uuid import pytest +from sqlalchemy import select +from sqlalchemy.orm import selectinload -from app.crud.budget_crud import create_budget, update_budget +from app.crud.budget_crud import create_budget, delete_budget, update_budget +from app.crud.budget_category_crud import create_budget_category +from app.crud.budget_line_crud import create_budget_line, delete_budget_line +from app.models.budget import BudgetModel from shared.security.current_user_context import reset_current_user_id, set_current_user_id from tests.factories.user import ValidUserFactory @@ -67,3 +72,48 @@ async def test_updated_by_reflects_the_editing_user_not_the_creator(self, db): reset_current_user_id(token) assert updated.updated_by == editor_id + + +async def _budget_with_orphaned_category(db) -> BudgetModel: + """A budget with a line added then removed, leaving its + auto-created category behind (see #295's failure mode).""" + user = ValidUserFactory() + budget = await create_budget( + session=db, user_id=user["user_id"], name="Cascade", owner_id=user["customer_id"] + ) + category = await create_budget_category( + session=db, user_id=user["user_id"], budget_id=budget.id, name="Travel" + ) + line = await create_budget_line( + session=db, + user_id=user["user_id"], + budget_id=budget.id, + category_id=category.id, + description="Flight", + amount=100.0, + ) + await delete_budget_line(session=db, budget_line=line) + return budget + + +@pytest.mark.anyio +class TestDeleteBudgetCascadesOrphanedCategory: + async def test_delete_succeeds_with_categories_unloaded(self, db): + budget = await _budget_with_orphaned_category(db) + + assert await delete_budget(session=db, budget=budget) is True + + async def test_delete_succeeds_with_categories_already_loaded(self, db): + # Distinguishes cascade+passive_deletes from passive_deletes alone, + # which only fixes the unloaded case. + budget = await _budget_with_orphaned_category(db) + + result = await db.execute( + select(BudgetModel) + .where(BudgetModel.id == budget.id) + .options(selectinload(BudgetModel.categories)) + ) + loaded_budget = result.scalar_one() + assert len(list(loaded_budget.categories)) == 1 + + assert await delete_budget(session=db, budget=loaded_budget) is True