From 830b2d7ee950f88e5c1b1a22c2016d3ee303d380 Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sat, 25 Jul 2026 23:59:43 -0700 Subject: [PATCH 01/11] [EDIFIKANA] Mark DS-OCC-008 fixed in datastore test catalog (NONE) --- edifikana/back-end/docs/datastore-test-catalog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index 4906d3dd3..f6727a903 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -364,7 +364,10 @@ datastore query bodies, and mapper functions side by side — not assumptions fr | DS-OCC-006 | P1 | `updateOccupant(isPrimary = false)` on a currently-primary occupant | Confirms the boolean-null-skip distinction (§3) — must actually flip to `false`, not skip | | DS-OCC-007 | P1 | `softRemoveOccupant` sets both `status = INACTIVE` and `end_date = today` atomically in one update | Confirm both fields change together; occupant remains readable via `getOccupant` (only `deleted_at` gates that, not `status`) but is excluded from `listOccupantsForUnit(includeInactive = false)` | | DS-OCC-008 | P2 | `purgeOccupant` on an active (non-deleted, non-removed) occupant | Fixed — guard now refuses (returns `false`), matching the consistent group. Also added `deleteOccupant` (Occupant previously had no way to soft-delete at all) | +<<<<<<< HEAD | DS-OCC-009 ⚠️ | P2 | `purgeOccupant` on a **nonexistent** id | **NOT YET IMPLEMENTED** — no test exercises this case today. Expected: `Result.success(false)`, no error (zero rows matched by the guard's `select()`), same shape as every other consistent-group resource's nonexistent-id purge case. Surfaced as a coverage gap while verifying `DS-OCC-008`'s fix — mirrors `DS-EMP-010`/`DS-EVT-013`, the analogous gaps for `updateEmployee`/`updateEventLogEntry` | +======= +>>>>>>> 907dd089f ([EDIFIKANA] Mark DS-OCC-008 fixed in datastore test catalog (NONE)) ### 4.8 `SupabaseOrganizationDatastore` (tables: `organizations`, `user_organization_mapping`) From 98a0daa79679c253b7341b57d63166c6f67521ed Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 00:37:04 -0700 Subject: [PATCH 02/11] [EDIFIKANA] Mark DS-ORG-010 and DS-X-005 fixed in datastore test catalog (NONE) --- edifikana/back-end/docs/datastore-test-catalog.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index f6727a903..4906d3dd3 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -364,10 +364,7 @@ datastore query bodies, and mapper functions side by side — not assumptions fr | DS-OCC-006 | P1 | `updateOccupant(isPrimary = false)` on a currently-primary occupant | Confirms the boolean-null-skip distinction (§3) — must actually flip to `false`, not skip | | DS-OCC-007 | P1 | `softRemoveOccupant` sets both `status = INACTIVE` and `end_date = today` atomically in one update | Confirm both fields change together; occupant remains readable via `getOccupant` (only `deleted_at` gates that, not `status`) but is excluded from `listOccupantsForUnit(includeInactive = false)` | | DS-OCC-008 | P2 | `purgeOccupant` on an active (non-deleted, non-removed) occupant | Fixed — guard now refuses (returns `false`), matching the consistent group. Also added `deleteOccupant` (Occupant previously had no way to soft-delete at all) | -<<<<<<< HEAD | DS-OCC-009 ⚠️ | P2 | `purgeOccupant` on a **nonexistent** id | **NOT YET IMPLEMENTED** — no test exercises this case today. Expected: `Result.success(false)`, no error (zero rows matched by the guard's `select()`), same shape as every other consistent-group resource's nonexistent-id purge case. Surfaced as a coverage gap while verifying `DS-OCC-008`'s fix — mirrors `DS-EMP-010`/`DS-EVT-013`, the analogous gaps for `updateEmployee`/`updateEventLogEntry` | -======= ->>>>>>> 907dd089f ([EDIFIKANA] Mark DS-OCC-008 fixed in datastore test catalog (NONE)) ### 4.8 `SupabaseOrganizationDatastore` (tables: `organizations`, `user_organization_mapping`) From 97b6d12298377c7e5d0b0f0bc7d2a933ec4e129e Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 12:56:27 -0700 Subject: [PATCH 03/11] [EDIFIKANA] Mark DS-RENT-006 fixed in datastore test catalog (NONE) --- .../back-end/docs/datastore-test-catalog.md | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index 4906d3dd3..a4d089dfc 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -142,11 +142,15 @@ datastore query bodies, and mapper functions side by side — not assumptions fr `status`/`endDate`, never `deletedAt` — so a `deleteOccupant` method was added alongside the guard, mirroring every other resource's `deleteX`/`purgeX` pair.) `SupabaseDocumentDatastore` has - **no `purgeDocument` method at all**. Confirm there is genuinely no way to hard-delete a - document via the datastore layer (check whether `SupabaseIntegrationTest`'s cleanup path for - documents falls back to something else, or leaks rows across test runs). → **DS-DOC-007** - (`DS-CA-011`, `DS-OCC-008`, `DS-PAY-007`, `DS-RENT-006`, `DS-TASK-009` fixed — this closes out - Finding #5 except for `DS-DOC-007`) + **no `purgeDocument` method at all**. Test: call + `purgeTask` on a row that is **not** soft-deleted — + confirm it hard-deletes an active, non-deleted record (likely intended only for integration + test cleanup helpers, but worth confirming the blast radius — anything else in the codebase + that calls these outside test cleanup would be able to permanently destroy live data with no + safety check). Separately, confirm there is genuinely no way to hard-delete a document via the + datastore layer (check whether `SupabaseIntegrationTest`'s cleanup path for documents falls + back to something else, or leaks rows across test runs). → **DS-TASK-009, DS-DOC-007** + (`DS-CA-011`, `DS-OCC-008`, `DS-PAY-007`, `DS-RENT-006` fixed) 6. **`ON DELETE` behavior for property/unit children is inconsistent — half `CASCADE`, half `RESTRICT` — and none of it fires today because every "delete" the app performs is a soft `UPDATE`, not a real `DELETE`.** Confirmed via migrations: `units.property_id`, @@ -169,14 +173,7 @@ datastore query bodies, and mapper functions side by side — not assumptions fr against a property/unit that has at least one row in **every** child table, and document exactly which children survive, which vanish, and which block the purge outright with an exception — this is directly testable today via the existing purge methods without needing any - new hard-delete API. → **DS-X-009 confirmed** — `purgeProperty` on a property with a live - `RESTRICT`-family child (e.g. a unit) fails outright with an FK-violation, while a - `CASCADE`-only property purges successfully and removes its children; separately confirmed - `units(unit_id)` has **no `RESTRICT` references at all** (only `CASCADE` and `SET NULL`), so - `purgeUnit` can never be blocked by an FK violation — its `CASCADE` children (e.g. - `rent_config`) are removed and its `SET NULL` children (e.g. `tasks`) survive with the - reference nulled out. Exactly as migrations predicted; now backed by live evidence - (`DS-X-009`, `DS-X-009b`). + new hard-delete API. → **DS-X-009** 7. **`users.phone_number` is nullable at the DB level; `UserEntity.phoneNumber` is a non-null Kotlin type.** Confirmed via migrations: `phone_number` was added as a plain nullable `TEXT` column with no `NOT NULL` ever applied (unlike `first_name`/`last_name`, which were explicitly @@ -443,7 +440,7 @@ Only datastore in the module with **no `deleted_at`/soft-delete concept at all** | DS-TASK-006 | P1 | `unassignTasksForEmployee` scoping — tasks for employee A across multiple properties, plus tasks for employee B | Only employee A's non-deleted tasks get `assignee_id = NULL`; B's are untouched; already-soft-deleted tasks assigned to A are excluded by the `exact("deleted_at", null)` filter | | DS-TASK-007 | P2 | `unassignTasksForEmployee` for an employee with zero assigned tasks | No-op, no error | | DS-TASK-008 | P1 | `deleteTask` (soft) idempotency and read-side exclusion | Consistent with the module-wide soft-delete convention | -| DS-TASK-009 | **P0** | `purgeTask` on an **active** (non-deleted) task | Fixed — guard now refuses (returns `false`), matching the consistent group | +| DS-TASK-009 🐛 | **P0** | `purgeTask` on an **active** (non-deleted) task | Finding #5 — Task is in the *no-guard* group (unlike Employee/EventLog/Unit); confirm it hard-deletes an active row with no soft-delete-first check | ### 4.14 `SupabaseTimeCardDatastore` (table: `time_card_events`) @@ -481,7 +478,7 @@ Only datastore in the module with **no `deleted_at`/soft-delete concept at all** | DS-USR-006 | P1 | `getUsers(organizationId)` — same "spread + filter deleted client-side" pattern as `getOrganizationsForUser` | Confirm soft-deleted users belonging to the org are correctly excluded post-fetch | | DS-USR-012 | **P0** | `updateUser`/`updateUserImpl` on a soft-deleted user | Fixed — guard now returns `Result.failure` (zero rows matched), entry left unmutated | | DS-USR-007 | P1 | `deleteUser` — soft-deletes in DB first, then **best-effort** deletes from Supabase Auth (`runCatching`, failure swallowed) | Confirm a simulated Auth-deletion failure (e.g., already-deleted Auth user) does not roll back or fail the DB soft-delete — the method still returns `true`. This is an intentional "eventually consistent, retry later" design per the comment — confirm the retry mechanism referenced ("a background process can retry") actually exists somewhere, or flag as an unimplemented TODO if it doesn't | -| DS-USR-016 | **P0** | `deleteUser` called twice on the same user | Fixed — `deleteUser` no longer pre-fetches and throws on a miss; it now filters `deletedAt isExact null` directly like every sibling `delete*` method, so a second call (or a nonexistent id) returns `false` instead of throwing | +| DS-USR-016 🐛 | **P0** | `deleteUser` called twice on the same user | Found during testing, not in the original static review — unlike every other resource's `delete*` method (e.g. `DS-CA-010`), `deleteUser` does not filter out already soft-deleted rows before deleting; the second call throws `NotFoundException` instead of returning `false` | | DS-USR-008 | P1 | `deleteUser` skips the Auth-deletion call entirely when `user.authMetadata.pendingAssociation == true` | Confirm a still-pending (OTP, never-associated) transient user's soft-delete does not attempt an Auth API call at all (there's nothing to delete there) | | DS-USR-009 | P2 | `requestPasswordReset(phoneNumber = "...", email = null)` | Throws `NotImplementedError` — confirm this is caught/surfaced sensibly by `runSuspendCatching` and not left as an unhandled crash reaching the caller as a raw `NotImplementedError` | | DS-USR-013 | P1 | `setPasswordAuthEnabled` on a nonexistent user id | `NotFoundException` | @@ -518,15 +515,14 @@ more datastores — grounded in the actual data relationships observed in the en | ID | Priority | Case | Expected | |---|---|---|---| | DS-X-001 | **P0** | `SupabasePropertyDatastore.deleteProperty` (soft) → `SupabaseUnitDatastore.createUnit(propertyId = )` | The pre-check in `createUnit` filters `deletedAt isExact null` on the property lookup — confirm it correctly throws `NotFoundException`, blocking new units under a soft-deleted property (this is the one place in the module that actively defends against finding #3/#4-style orphaning by construction, at the *child creation* boundary — worth confirming it's not accidentally bypassable) | -| DS-X-002 | **P0** | `SupabaseOrganizationDatastore.deleteOrganization` (soft) → `SupabasePropertyDatastore.createProperty(organizationId = )` | Fixed — `createProperty` now checks its parent organization is active, mirroring `createUnit`'s existing guard against its own parent `Property` | +| DS-X-002 | **P0** | `SupabaseOrganizationDatastore.deleteOrganization` (soft) → `SupabasePropertyDatastore.createProperty(organizationId = )` | **No equivalent pre-check exists in `createProperty`** — unlike Unit's guard against a deleted Property, Property's own create path never verifies its parent `Organization` is active. Confirm a soft-deleted org can silently accumulate brand-new properties forever | | DS-X-003 | P1 | `SupabaseEmployeeDatastore.deleteEmployee` (soft) → `SupabaseTaskDatastore.getTasks(propertyId, assigneeId = )` | Confirm tasks still assigned to a soft-deleted employee remain visible/filterable by that (now-gone) `assigneeId` — nothing in `TaskDatastore` reacts to employee deletion except the explicit, separately-invoked `unassignTasksForEmployee` (DS-TASK-005/006); if a caller forgets to invoke it, stale assignments persist indefinitely and silently | -| DS-X-004 | P1 | `SupabaseOccupantDatastore.createOccupant(unitId = )` | Fixed for the unit-itself-soft-deleted case — `createOccupant` now checks its immediate parent `Unit` is active, mirroring `createUnit`/`createProperty`'s (`DS-X-002`) shallow guard pattern. The deeper case (unit active, but its parent property is soft-deleted) remains unguarded, consistent with `createUnit` not checking its own parent's parent either — see the multi-level-ancestor-checks follow-up noted in the `DS-X-004` plan | +| DS-X-004 | P1 | `SupabaseOccupantDatastore.createOccupant(unitId = )` | Unlike `Unit`/`Task`'s create paths, `Occupant`'s create has **no property/unit-active pre-check at all** — confirm an occupant can be created against a unit that itself may or may not still be active (Occupant only references `unitId`, never checks it) | | DS-X-005 | P0 | `SupabaseMembershipDatastore.removeMember` → `SupabaseOrganizationDatastore.getUserRole` → (separately) `SupabaseMembershipDatastore.getMember` via `v_org_members` | Fixed alongside `DS-ORG-010` — the two read paths now agree post-removal, since `getUserRole` filters `status = ACTIVE` the same way `v_org_members` already did | | DS-X-006 | P1 | `SupabaseRentConfigDatastore.setRentConfig` → `SupabaseUnitDatastore.deleteUnit` (soft) → `SupabaseRentConfigDatastore.getRentConfig(unitId)` | Confirm the rent config row survives the unit's soft-delete unchanged (no cascading soft-delete relationship implemented in application code — any cascade would have to be a DB-level trigger or FK `ON DELETE`, which is out of scope for this Kotlin layer but worth confirming isn't silently relied upon) | | DS-X-007 | P1 | `SupabaseUserDatastore.deleteUser` → `SupabaseNotificationDatastore.getNotificationsForUser(deletedUserId)` | Confirm notifications tied to a soft-deleted user remain queryable by ID (nothing propagates the user's deletion into the notifications table) — consistent with `api-test-catalog.md` XAPI-023's observation about stale notifications, but verifiable here purely at the datastore level without needing the controller/RBAC layer at all | | DS-X-008 | P2 | `SupabaseEmployeeDatastore.createEmployee(propertyId)` → `SupabaseEmployeeDatastore.getEmployees(userId)` (via `v_user_employees`) for a user who is **not** mapped to that property at all | Confirms the view's join is against `user_property_mapping`, not something broader (e.g., org-wide) — a user with an org role but no explicit property mapping should NOT see that property's employees through this view, if the view's intended scoping is per-property | -| DS-X-009 | **P0** | `purgeProperty` against a property with a `RESTRICT`-family child vs. a `CASCADE`-only property | Confirmed — a live `RESTRICT` child (e.g. a unit) blocks the purge with an FK-violation `Result.failure`; a `CASCADE`-only property purges successfully and its children (e.g. tasks) are removed too | -| DS-X-009b | P1 | `purgeUnit` against a unit with `CASCADE` and `SET NULL` children | Confirmed — `units(unit_id)` has no `RESTRICT` references at all, so `purgeUnit` always succeeds; `CASCADE` children (e.g. `rent_config`) are removed, `SET NULL` children (e.g. `tasks`) survive with the reference nulled out | +| DS-X-009 | **P0** | `purgeProperty` / `purgeUnit` (after soft-deleting first) against a property/unit that has at least one live row in **every** child table | Finding #6 — a single mechanical test walks every child relationship and records, per table, whether the row survives (RESTRICT — purge should fail with an FK-violation `Result.failure`), vanishes (CASCADE), or gets nulled out (`ON DELETE SET NULL`, e.g. `tasks.unit_id`/`tasks.common_area_id`, `invites.unit_id`). Given the confirmed mix — `tasks` CASCADEs off `properties`, but `units`/`employee`/`common_areas`/`event_log_entries`/`time_card_events` RESTRICT off `properties` — expect `purgeProperty` on a property with a live unit/employee/common-area/event-log/time-card child to **fail outright** with a Postgres FK-violation, while a property with only task children would succeed and cascade-delete them. This single test also doubles as the ground-truth answer to finding #6, replacing speculation with an observed outcome | | DS-X-010 | P1 | `SupabaseMembershipDatastore.cancelInvite` (soft) → `SupabaseMembershipDatastore.purgeInvite` (hard) → `SupabaseNotificationDatastore.getNotification(id)` for a notification originally created via `createNotification(inviteId = )` | Confirmed via migration: `notifications.invite_id REFERENCES invites(id) ON DELETE CASCADE` — purging the invite should **cascade-delete** the notification row entirely (not soft-delete it), meaning `getNotification` transitions from "found" straight to "row doesn't exist," skipping the soft-deleted state entirely. Contrast with DS-X-007 (user deletion does *not* cascade to notifications) to confirm the two FK relationships on the same `notifications` table behave differently by design (`recipient_user_id` has no cascade wired into apparent app behavior since `deleteUser` only soft-deletes, while `invite_id`'s CASCADE only fires on the hard `purgeInvite` path) | --- From fba2b333c966ba78518e005316e1daa1807ac81e Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 15:29:08 -0700 Subject: [PATCH 04/11] [EDIFIKANA] Mark DS-TASK-009 fixed in datastore test catalog (NONE) --- .../back-end/docs/datastore-test-catalog.md | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index a4d089dfc..02015cd70 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -142,15 +142,11 @@ datastore query bodies, and mapper functions side by side — not assumptions fr `status`/`endDate`, never `deletedAt` — so a `deleteOccupant` method was added alongside the guard, mirroring every other resource's `deleteX`/`purgeX` pair.) `SupabaseDocumentDatastore` has - **no `purgeDocument` method at all**. Test: call - `purgeTask` on a row that is **not** soft-deleted — - confirm it hard-deletes an active, non-deleted record (likely intended only for integration - test cleanup helpers, but worth confirming the blast radius — anything else in the codebase - that calls these outside test cleanup would be able to permanently destroy live data with no - safety check). Separately, confirm there is genuinely no way to hard-delete a document via the - datastore layer (check whether `SupabaseIntegrationTest`'s cleanup path for documents falls - back to something else, or leaks rows across test runs). → **DS-TASK-009, DS-DOC-007** - (`DS-CA-011`, `DS-OCC-008`, `DS-PAY-007`, `DS-RENT-006` fixed) + **no `purgeDocument` method at all**. Confirm there is genuinely no way to hard-delete a + document via the datastore layer (check whether `SupabaseIntegrationTest`'s cleanup path for + documents falls back to something else, or leaks rows across test runs). → **DS-DOC-007** + (`DS-CA-011`, `DS-OCC-008`, `DS-PAY-007`, `DS-RENT-006`, `DS-TASK-009` fixed — this closes out + Finding #5 except for `DS-DOC-007`) 6. **`ON DELETE` behavior for property/unit children is inconsistent — half `CASCADE`, half `RESTRICT` — and none of it fires today because every "delete" the app performs is a soft `UPDATE`, not a real `DELETE`.** Confirmed via migrations: `units.property_id`, @@ -440,7 +436,7 @@ Only datastore in the module with **no `deleted_at`/soft-delete concept at all** | DS-TASK-006 | P1 | `unassignTasksForEmployee` scoping — tasks for employee A across multiple properties, plus tasks for employee B | Only employee A's non-deleted tasks get `assignee_id = NULL`; B's are untouched; already-soft-deleted tasks assigned to A are excluded by the `exact("deleted_at", null)` filter | | DS-TASK-007 | P2 | `unassignTasksForEmployee` for an employee with zero assigned tasks | No-op, no error | | DS-TASK-008 | P1 | `deleteTask` (soft) idempotency and read-side exclusion | Consistent with the module-wide soft-delete convention | -| DS-TASK-009 🐛 | **P0** | `purgeTask` on an **active** (non-deleted) task | Finding #5 — Task is in the *no-guard* group (unlike Employee/EventLog/Unit); confirm it hard-deletes an active row with no soft-delete-first check | +| DS-TASK-009 | **P0** | `purgeTask` on an **active** (non-deleted) task | Fixed — guard now refuses (returns `false`), matching the consistent group | ### 4.14 `SupabaseTimeCardDatastore` (table: `time_card_events`) From 55ca4d4d8bf5e20b805796112a91c81610791eb6 Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 15:46:28 -0700 Subject: [PATCH 05/11] [EDIFIKANA] Mark DS-USR-016 fixed in datastore test catalog (NONE) --- edifikana/back-end/docs/datastore-test-catalog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index 02015cd70..d384b1dd9 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -474,7 +474,7 @@ Only datastore in the module with **no `deleted_at`/soft-delete concept at all** | DS-USR-006 | P1 | `getUsers(organizationId)` — same "spread + filter deleted client-side" pattern as `getOrganizationsForUser` | Confirm soft-deleted users belonging to the org are correctly excluded post-fetch | | DS-USR-012 | **P0** | `updateUser`/`updateUserImpl` on a soft-deleted user | Fixed — guard now returns `Result.failure` (zero rows matched), entry left unmutated | | DS-USR-007 | P1 | `deleteUser` — soft-deletes in DB first, then **best-effort** deletes from Supabase Auth (`runCatching`, failure swallowed) | Confirm a simulated Auth-deletion failure (e.g., already-deleted Auth user) does not roll back or fail the DB soft-delete — the method still returns `true`. This is an intentional "eventually consistent, retry later" design per the comment — confirm the retry mechanism referenced ("a background process can retry") actually exists somewhere, or flag as an unimplemented TODO if it doesn't | -| DS-USR-016 🐛 | **P0** | `deleteUser` called twice on the same user | Found during testing, not in the original static review — unlike every other resource's `delete*` method (e.g. `DS-CA-010`), `deleteUser` does not filter out already soft-deleted rows before deleting; the second call throws `NotFoundException` instead of returning `false` | +| DS-USR-016 | **P0** | `deleteUser` called twice on the same user | Fixed — `deleteUser` no longer pre-fetches and throws on a miss; it now filters `deletedAt isExact null` directly like every sibling `delete*` method, so a second call (or a nonexistent id) returns `false` instead of throwing | | DS-USR-008 | P1 | `deleteUser` skips the Auth-deletion call entirely when `user.authMetadata.pendingAssociation == true` | Confirm a still-pending (OTP, never-associated) transient user's soft-delete does not attempt an Auth API call at all (there's nothing to delete there) | | DS-USR-009 | P2 | `requestPasswordReset(phoneNumber = "...", email = null)` | Throws `NotImplementedError` — confirm this is caught/surfaced sensibly by `runSuspendCatching` and not left as an unhandled crash reaching the caller as a raw `NotImplementedError` | | DS-USR-013 | P1 | `setPasswordAuthEnabled` on a nonexistent user id | `NotFoundException` | From 5560e575deeac40c10196529e857aa004f577fdd Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 15:58:27 -0700 Subject: [PATCH 06/11] [EDIFIKANA] Mark DS-X-002 fixed in datastore test catalog (NONE) --- edifikana/back-end/docs/datastore-test-catalog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index d384b1dd9..dd3556a35 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -511,7 +511,7 @@ more datastores — grounded in the actual data relationships observed in the en | ID | Priority | Case | Expected | |---|---|---|---| | DS-X-001 | **P0** | `SupabasePropertyDatastore.deleteProperty` (soft) → `SupabaseUnitDatastore.createUnit(propertyId = )` | The pre-check in `createUnit` filters `deletedAt isExact null` on the property lookup — confirm it correctly throws `NotFoundException`, blocking new units under a soft-deleted property (this is the one place in the module that actively defends against finding #3/#4-style orphaning by construction, at the *child creation* boundary — worth confirming it's not accidentally bypassable) | -| DS-X-002 | **P0** | `SupabaseOrganizationDatastore.deleteOrganization` (soft) → `SupabasePropertyDatastore.createProperty(organizationId = )` | **No equivalent pre-check exists in `createProperty`** — unlike Unit's guard against a deleted Property, Property's own create path never verifies its parent `Organization` is active. Confirm a soft-deleted org can silently accumulate brand-new properties forever | +| DS-X-002 | **P0** | `SupabaseOrganizationDatastore.deleteOrganization` (soft) → `SupabasePropertyDatastore.createProperty(organizationId = )` | Fixed — `createProperty` now checks its parent organization is active, mirroring `createUnit`'s existing guard against its own parent `Property` | | DS-X-003 | P1 | `SupabaseEmployeeDatastore.deleteEmployee` (soft) → `SupabaseTaskDatastore.getTasks(propertyId, assigneeId = )` | Confirm tasks still assigned to a soft-deleted employee remain visible/filterable by that (now-gone) `assigneeId` — nothing in `TaskDatastore` reacts to employee deletion except the explicit, separately-invoked `unassignTasksForEmployee` (DS-TASK-005/006); if a caller forgets to invoke it, stale assignments persist indefinitely and silently | | DS-X-004 | P1 | `SupabaseOccupantDatastore.createOccupant(unitId = )` | Unlike `Unit`/`Task`'s create paths, `Occupant`'s create has **no property/unit-active pre-check at all** — confirm an occupant can be created against a unit that itself may or may not still be active (Occupant only references `unitId`, never checks it) | | DS-X-005 | P0 | `SupabaseMembershipDatastore.removeMember` → `SupabaseOrganizationDatastore.getUserRole` → (separately) `SupabaseMembershipDatastore.getMember` via `v_org_members` | Fixed alongside `DS-ORG-010` — the two read paths now agree post-removal, since `getUserRole` filters `status = ACTIVE` the same way `v_org_members` already did | From 789664297ab0af476193ca6e05b507526c7432e0 Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 16:11:14 -0700 Subject: [PATCH 07/11] [EDIFIKANA] Mark DS-X-004 fixed (shallow case) in datastore test catalog (NONE) --- edifikana/back-end/docs/datastore-test-catalog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index dd3556a35..c6f7fc0a5 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -513,7 +513,7 @@ more datastores — grounded in the actual data relationships observed in the en | DS-X-001 | **P0** | `SupabasePropertyDatastore.deleteProperty` (soft) → `SupabaseUnitDatastore.createUnit(propertyId = )` | The pre-check in `createUnit` filters `deletedAt isExact null` on the property lookup — confirm it correctly throws `NotFoundException`, blocking new units under a soft-deleted property (this is the one place in the module that actively defends against finding #3/#4-style orphaning by construction, at the *child creation* boundary — worth confirming it's not accidentally bypassable) | | DS-X-002 | **P0** | `SupabaseOrganizationDatastore.deleteOrganization` (soft) → `SupabasePropertyDatastore.createProperty(organizationId = )` | Fixed — `createProperty` now checks its parent organization is active, mirroring `createUnit`'s existing guard against its own parent `Property` | | DS-X-003 | P1 | `SupabaseEmployeeDatastore.deleteEmployee` (soft) → `SupabaseTaskDatastore.getTasks(propertyId, assigneeId = )` | Confirm tasks still assigned to a soft-deleted employee remain visible/filterable by that (now-gone) `assigneeId` — nothing in `TaskDatastore` reacts to employee deletion except the explicit, separately-invoked `unassignTasksForEmployee` (DS-TASK-005/006); if a caller forgets to invoke it, stale assignments persist indefinitely and silently | -| DS-X-004 | P1 | `SupabaseOccupantDatastore.createOccupant(unitId = )` | Unlike `Unit`/`Task`'s create paths, `Occupant`'s create has **no property/unit-active pre-check at all** — confirm an occupant can be created against a unit that itself may or may not still be active (Occupant only references `unitId`, never checks it) | +| DS-X-004 | P1 | `SupabaseOccupantDatastore.createOccupant(unitId = )` | Fixed for the unit-itself-soft-deleted case — `createOccupant` now checks its immediate parent `Unit` is active, mirroring `createUnit`/`createProperty`'s (`DS-X-002`) shallow guard pattern. The deeper case (unit active, but its parent property is soft-deleted) remains unguarded, consistent with `createUnit` not checking its own parent's parent either — see the multi-level-ancestor-checks follow-up noted in the `DS-X-004` plan | | DS-X-005 | P0 | `SupabaseMembershipDatastore.removeMember` → `SupabaseOrganizationDatastore.getUserRole` → (separately) `SupabaseMembershipDatastore.getMember` via `v_org_members` | Fixed alongside `DS-ORG-010` — the two read paths now agree post-removal, since `getUserRole` filters `status = ACTIVE` the same way `v_org_members` already did | | DS-X-006 | P1 | `SupabaseRentConfigDatastore.setRentConfig` → `SupabaseUnitDatastore.deleteUnit` (soft) → `SupabaseRentConfigDatastore.getRentConfig(unitId)` | Confirm the rent config row survives the unit's soft-delete unchanged (no cascading soft-delete relationship implemented in application code — any cascade would have to be a DB-level trigger or FK `ON DELETE`, which is out of scope for this Kotlin layer but worth confirming isn't silently relied upon) | | DS-X-007 | P1 | `SupabaseUserDatastore.deleteUser` → `SupabaseNotificationDatastore.getNotificationsForUser(deletedUserId)` | Confirm notifications tied to a soft-deleted user remain queryable by ID (nothing propagates the user's deletion into the notifications table) — consistent with `api-test-catalog.md` XAPI-023's observation about stale notifications, but verifiable here purely at the datastore level without needing the controller/RBAC layer at all | From 832f555e41404e3ccb6ced9471e9f974b9fa475c Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 17:15:04 -0700 Subject: [PATCH 08/11] [EDIFIKANA] Mark DS-X-009/DS-X-009b confirmed in datastore test catalog (NONE) DS-X-009 (property-side RESTRICT-vs-CASCADE sweep) already existed and passes live -- discovered during implementation, not written as part of this work. Adds DS-X-009b to the catalog for the newly-added unit-side sweep test, and reframes Finding #6 from speculative to confirmed with the observed outcome. --- edifikana/back-end/docs/datastore-test-catalog.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index c6f7fc0a5..4906d3dd3 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -169,7 +169,14 @@ datastore query bodies, and mapper functions side by side — not assumptions fr against a property/unit that has at least one row in **every** child table, and document exactly which children survive, which vanish, and which block the purge outright with an exception — this is directly testable today via the existing purge methods without needing any - new hard-delete API. → **DS-X-009** + new hard-delete API. → **DS-X-009 confirmed** — `purgeProperty` on a property with a live + `RESTRICT`-family child (e.g. a unit) fails outright with an FK-violation, while a + `CASCADE`-only property purges successfully and removes its children; separately confirmed + `units(unit_id)` has **no `RESTRICT` references at all** (only `CASCADE` and `SET NULL`), so + `purgeUnit` can never be blocked by an FK violation — its `CASCADE` children (e.g. + `rent_config`) are removed and its `SET NULL` children (e.g. `tasks`) survive with the + reference nulled out. Exactly as migrations predicted; now backed by live evidence + (`DS-X-009`, `DS-X-009b`). 7. **`users.phone_number` is nullable at the DB level; `UserEntity.phoneNumber` is a non-null Kotlin type.** Confirmed via migrations: `phone_number` was added as a plain nullable `TEXT` column with no `NOT NULL` ever applied (unlike `first_name`/`last_name`, which were explicitly @@ -518,7 +525,8 @@ more datastores — grounded in the actual data relationships observed in the en | DS-X-006 | P1 | `SupabaseRentConfigDatastore.setRentConfig` → `SupabaseUnitDatastore.deleteUnit` (soft) → `SupabaseRentConfigDatastore.getRentConfig(unitId)` | Confirm the rent config row survives the unit's soft-delete unchanged (no cascading soft-delete relationship implemented in application code — any cascade would have to be a DB-level trigger or FK `ON DELETE`, which is out of scope for this Kotlin layer but worth confirming isn't silently relied upon) | | DS-X-007 | P1 | `SupabaseUserDatastore.deleteUser` → `SupabaseNotificationDatastore.getNotificationsForUser(deletedUserId)` | Confirm notifications tied to a soft-deleted user remain queryable by ID (nothing propagates the user's deletion into the notifications table) — consistent with `api-test-catalog.md` XAPI-023's observation about stale notifications, but verifiable here purely at the datastore level without needing the controller/RBAC layer at all | | DS-X-008 | P2 | `SupabaseEmployeeDatastore.createEmployee(propertyId)` → `SupabaseEmployeeDatastore.getEmployees(userId)` (via `v_user_employees`) for a user who is **not** mapped to that property at all | Confirms the view's join is against `user_property_mapping`, not something broader (e.g., org-wide) — a user with an org role but no explicit property mapping should NOT see that property's employees through this view, if the view's intended scoping is per-property | -| DS-X-009 | **P0** | `purgeProperty` / `purgeUnit` (after soft-deleting first) against a property/unit that has at least one live row in **every** child table | Finding #6 — a single mechanical test walks every child relationship and records, per table, whether the row survives (RESTRICT — purge should fail with an FK-violation `Result.failure`), vanishes (CASCADE), or gets nulled out (`ON DELETE SET NULL`, e.g. `tasks.unit_id`/`tasks.common_area_id`, `invites.unit_id`). Given the confirmed mix — `tasks` CASCADEs off `properties`, but `units`/`employee`/`common_areas`/`event_log_entries`/`time_card_events` RESTRICT off `properties` — expect `purgeProperty` on a property with a live unit/employee/common-area/event-log/time-card child to **fail outright** with a Postgres FK-violation, while a property with only task children would succeed and cascade-delete them. This single test also doubles as the ground-truth answer to finding #6, replacing speculation with an observed outcome | +| DS-X-009 | **P0** | `purgeProperty` against a property with a `RESTRICT`-family child vs. a `CASCADE`-only property | Confirmed — a live `RESTRICT` child (e.g. a unit) blocks the purge with an FK-violation `Result.failure`; a `CASCADE`-only property purges successfully and its children (e.g. tasks) are removed too | +| DS-X-009b | P1 | `purgeUnit` against a unit with `CASCADE` and `SET NULL` children | Confirmed — `units(unit_id)` has no `RESTRICT` references at all, so `purgeUnit` always succeeds; `CASCADE` children (e.g. `rent_config`) are removed, `SET NULL` children (e.g. `tasks`) survive with the reference nulled out | | DS-X-010 | P1 | `SupabaseMembershipDatastore.cancelInvite` (soft) → `SupabaseMembershipDatastore.purgeInvite` (hard) → `SupabaseNotificationDatastore.getNotification(id)` for a notification originally created via `createNotification(inviteId = )` | Confirmed via migration: `notifications.invite_id REFERENCES invites(id) ON DELETE CASCADE` — purging the invite should **cascade-delete** the notification row entirely (not soft-delete it), meaning `getNotification` transitions from "found" straight to "row doesn't exist," skipping the soft-deleted state entirely. Contrast with DS-X-007 (user deletion does *not* cascade to notifications) to confirm the two FK relationships on the same `notifications` table behave differently by design (`recipient_user_id` has no cascade wired into apparent app behavior since `deleteUser` only soft-deletes, while `invite_id`'s CASCADE only fires on the hard `purgeInvite` path) | --- From 168131c9b728bc36429590cd17d6110b65267c2b Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 17:43:47 -0700 Subject: [PATCH 09/11] [EDIFIKANA] Make createProperty atomic via create_property_with_owner RPC (NONE) createProperty previously issued two separate Postgrest inserts (properties, then user_property_mapping) with nothing wrapping them, so a failure on the second call left an orphaned, ownerless property row behind. Both inserts now run inside a single RPC call, which Postgres executes as one transaction. --- .../server/datastore/PropertyDatastore.kt | 5 +- .../datastore/supabase/SupabaseMappers.kt | 18 ------ .../supabase/SupabasePropertyDatastore.kt | 60 +++++++++---------- .../supabase/models/PropertyEntity.kt | 14 ----- ...00_create_property_with_owner_function.sql | 48 +++++++++++++++ 5 files changed, 82 insertions(+), 63 deletions(-) create mode 100644 edifikana/back-end/supabase/migrations/20260726000000_create_property_with_owner_function.sql diff --git a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/PropertyDatastore.kt b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/PropertyDatastore.kt index 75551f372..c18c55260 100644 --- a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/PropertyDatastore.kt +++ b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/PropertyDatastore.kt @@ -12,7 +12,10 @@ import com.cramsan.framework.annotations.BackendDatastore @BackendDatastore interface PropertyDatastore { /** - * Creates a new property. Returns the [Result] of the operation with the created [Property]. + * Creates a new property and associates it with its creator in a single atomic operation + * (via the `create_property_with_owner` RPC) - if the owner association fails, the property + * insert is rolled back too, so no ownerless property is ever left behind. Returns the + * [Result] of the operation with the created [Property]. */ suspend fun createProperty( name: String, diff --git a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabaseMappers.kt b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabaseMappers.kt index cdf279977..3e2f8d22e 100644 --- a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabaseMappers.kt +++ b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabaseMappers.kt @@ -225,24 +225,6 @@ fun EmployeeEntity.toEmployee(): Employee { ) } -/** - * Maps a [CreatePropertyRequest] to the [PropertyEntity.CreatePropertyEntity] model. - */ - -fun CreatePropertyEntity( - name: String, - address: String, - organizationId: OrganizationId, - imageUrl: String? = null, -): PropertyEntity.CreatePropertyEntity { - return PropertyEntity.CreatePropertyEntity( - name = name, - address = address, - organizationId = organizationId, - imageUrl = imageUrl?.let { Url(it) }, - ) -} - /** * Maps a [PropertyEntity] to the [Property] model. */ diff --git a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastore.kt b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastore.kt index de1ba681e..3c6b402eb 100644 --- a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastore.kt +++ b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastore.kt @@ -7,7 +7,6 @@ import com.cramsan.edifikana.lib.model.user.UserId import com.cramsan.edifikana.server.datastore.PropertyDatastore import com.cramsan.edifikana.server.datastore.supabase.models.OrganizationEntity import com.cramsan.edifikana.server.datastore.supabase.models.PropertyEntity -import com.cramsan.edifikana.server.datastore.supabase.models.UserPropertyMappingEntity import com.cramsan.edifikana.server.datastore.supabase.models.UserPropertyViewEntity import com.cramsan.edifikana.server.service.models.Property import com.cramsan.framework.annotations.BackendDatastore @@ -15,6 +14,10 @@ import com.cramsan.framework.core.runSuspendCatching import com.cramsan.framework.logging.logD import com.cramsan.framework.utils.exceptions.ClientRequestExceptions import io.github.jan.supabase.postgrest.Postgrest +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.jsonObject import kotlin.time.Clock /** @@ -47,36 +50,19 @@ class SupabasePropertyDatastore(private val postgrest: Postgrest, private val cl "No organization with id $organizationId found in database", ) - val requestEntity: PropertyEntity.CreatePropertyEntity = - CreatePropertyEntity( - name = name, - address = address, - organizationId = organizationId, - imageUrl = imageUrl, + // Both inserts (properties + user_property_mapping) happen inside a single RPC call, + // which Postgres executes as one transaction - if the owner-mapping insert fails, + // the property insert is rolled back too, instead of leaving an orphaned property. + val params = + CreatePropertyWithOwnerRpcParams( + pName = name, + pAddress = address, + pOrganizationId = organizationId.id, + pImageUrl = imageUrl, + pCreatorUserId = creatorUserId.userId, ) - - // Insert the property into the database and select the created entity - val createdProperty = - postgrest - .from(PropertyEntity.COLLECTION) - .insert(requestEntity) { - select() - }.decodeSingle() - - // Plain insert is safe: createdProperty.id is a freshly generated UUID, so no - // concurrent call can produce the same (userId, propertyId) pair. - postgrest - .from(UserPropertyMappingEntity.COLLECTION) - .insert( - UserPropertyMappingEntity.CreateUserPropertyMappingEntity( - userId = creatorUserId, - propertyId = createdProperty.id, - ), - ) { - select() - }.decodeSingleOrNull() ?: run { - throw IllegalStateException("Failed to associate property with user") - } + val jsonParams = Json.encodeToJsonElement(CreatePropertyWithOwnerRpcParams.serializer(), params).jsonObject + val createdProperty = postgrest.rpc(RPC_CREATE_PROPERTY_WITH_OWNER, jsonParams).decodeAs() logD(TAG, "Property created propertyId: %s", createdProperty.id) createdProperty.toProperty() @@ -215,8 +201,22 @@ class SupabasePropertyDatastore(private val postgrest: Postgrest, private val cl true } + /** + * Parameters for the `create_property_with_owner` RPC, which inserts the property and its + * owner mapping in a single transaction. + */ + @Serializable + private data class CreatePropertyWithOwnerRpcParams( + @SerialName("p_name") val pName: String, + @SerialName("p_address") val pAddress: String, + @SerialName("p_organization_id") val pOrganizationId: String, + @SerialName("p_image_url") val pImageUrl: String?, + @SerialName("p_creator_user_id") val pCreatorUserId: String, + ) + companion object { const val TAG = "SupabasePropertyDatastore" const val VIEW_USER_PROPERTIES = "v_user_properties" + const val RPC_CREATE_PROPERTY_WITH_OWNER = "create_property_with_owner" } } diff --git a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/models/PropertyEntity.kt b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/models/PropertyEntity.kt index 80e872127..953750bef 100644 --- a/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/models/PropertyEntity.kt +++ b/edifikana/back-end/src/main/kotlin/com/cramsan/edifikana/server/datastore/supabase/models/PropertyEntity.kt @@ -27,18 +27,4 @@ data class PropertyEntity( companion object { const val COLLECTION = "properties" } - - /** - * Entity representing a new property. - */ - @Serializable - @DatabaseModel - data class CreatePropertyEntity( - val name: String, - val address: String, - @SerialName("organization_id") - val organizationId: OrganizationId, - @SerialName("image_url") - val imageUrl: Url? = null, - ) } diff --git a/edifikana/back-end/supabase/migrations/20260726000000_create_property_with_owner_function.sql b/edifikana/back-end/supabase/migrations/20260726000000_create_property_with_owner_function.sql new file mode 100644 index 000000000..e9442589d --- /dev/null +++ b/edifikana/back-end/supabase/migrations/20260726000000_create_property_with_owner_function.sql @@ -0,0 +1,48 @@ +-- ============================================================================ +-- Migration: Add create_property_with_owner RPC function +-- Datastore test catalog: DS-PROP-010 +-- ============================================================================ +-- SupabasePropertyDatastore.createProperty previously issued two separate +-- Postgrest calls: INSERT into properties, then INSERT into +-- user_property_mapping linking the new property to its creator. If the +-- second call failed for any reason (constraint violation, transient network +-- failure, cancelled coroutine), the property from the first call was already +-- committed - an orphaned, ownerless, permanently invisible-but-present +-- property (v_user_properties, and therefore getProperties(userId), only +-- surfaces properties with a user_property_mapping row). +-- +-- This function performs both inserts inside a single plpgsql function body. +-- Postgres executes one RPC call as one transaction, so if the second INSERT +-- raises, the first is rolled back too - no partial state is ever visible. +-- +-- SECURITY INVOKER + SET search_path = public follows the hardened shape of +-- upsert_rent_config (20260429000000_revoke_function_execute_grants.sql). +-- ============================================================================ + +CREATE OR REPLACE FUNCTION public.create_property_with_owner( + p_name TEXT, + p_address TEXT, + p_organization_id UUID, + p_image_url TEXT, + p_creator_user_id UUID +) +RETURNS properties +LANGUAGE plpgsql +SECURITY INVOKER +SET search_path = public +AS $$ +DECLARE + v_property properties; +BEGIN + INSERT INTO properties (name, address, organization_id, image_url) + VALUES (p_name, p_address, p_organization_id, p_image_url) + RETURNING * INTO v_property; + + INSERT INTO user_property_mapping (user_id, property_id) + VALUES (p_creator_user_id, v_property.id); + + RETURN v_property; +END; +$$; + +GRANT EXECUTE ON FUNCTION public.create_property_with_owner(TEXT, TEXT, UUID, TEXT, UUID) TO service_role; From 13a3877db39f61729feb4f89827560fb62b41a0f Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 17:43:52 -0700 Subject: [PATCH 10/11] [EDIFIKANA] Add DS-PROP-010b orphan-property regression test (NONE) Confirms createProperty's new atomic RPC leaves no orphaned property row behind when the owner-mapping insert fails on a nonexistent creatorUserId. --- ...upabasePropertyDatastoreIntegrationTest.kt | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/edifikana/back-end/src/integTest/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastoreIntegrationTest.kt b/edifikana/back-end/src/integTest/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastoreIntegrationTest.kt index 97bd3e2b1..0856e2d04 100644 --- a/edifikana/back-end/src/integTest/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastoreIntegrationTest.kt +++ b/edifikana/back-end/src/integTest/kotlin/com/cramsan/edifikana/server/datastore/supabase/SupabasePropertyDatastoreIntegrationTest.kt @@ -6,6 +6,7 @@ import com.cramsan.edifikana.lib.model.property.PropertyId import com.cramsan.edifikana.lib.model.user.UserId import com.cramsan.edifikana.server.service.models.Property import com.cramsan.framework.utils.uuid.UUID +import io.github.jan.supabase.postgrest.postgrest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals @@ -31,11 +32,10 @@ class SupabasePropertyDatastoreIntegrationTest : SupabaseIntegrationTest() { @Test fun `DS-PROP-010 createProperty should create both the property and its owning user_property_mapping`(): Unit = runBlocking { - // Arrange & Act - the two inserts (property, then user_property_mapping) are not - // wrapped in a transaction; confirm the happy-path shape: the property is - // immediately visible via getProperties(creatorUserId), proving the mapping row - // landed alongside it. True mid-sequence failure injection (to prove the orphan - // scenario) isn't reachable via the public datastore API alone. + // Arrange & Act - createProperty now performs both inserts atomically via the + // create_property_with_owner RPC; confirm the happy-path shape still works: the + // property is immediately visible via getProperties(creatorUserId), proving the + // mapping row landed alongside it. val result = propertyDatastore.createProperty( name = "${test_prefix}_AtomicityCheck", address = "1 Atomic Way", @@ -51,6 +51,35 @@ class SupabasePropertyDatastoreIntegrationTest : SupabaseIntegrationTest() { assertTrue(ownedProperties.any { it.id == property.id }) } + @Test + fun `DS-PROP-010b createProperty should not leave an orphaned property row when the owner mapping insert fails`(): Unit = + runBlocking { + // Arrange - a fabricated userId that doesn't exist in `users` violates + // user_property_mapping_user_id_fkey. Before this fix, the property insert (the RPC's + // first statement) would have already committed by the time this FK violation fires; + // now both inserts run inside one RPC call/transaction, so the whole thing rolls back. + val nonExistentUserId = UserId(UUID.random()) + val propertyName = "${test_prefix}_OrphanCheck" + + // Act + val result = propertyDatastore.createProperty( + name = propertyName, + address = "1 Orphan Ln", + creatorUserId = nonExistentUserId, + organizationId = testOrg!!, + imageUrl = null, + ) + + // Assert - the call fails, and no orphaned property row was left behind. A raw, + // untyped select is required here (rather than getProperty/getProperties) since those + // only prove a property is unreachable for a user, not that no row exists at all. + assertTrue(result.isFailure) + val orphanedRows = supabase.postgrest.from("properties").select { + filter { eq("name", propertyName) } + }.decodeList() + assertTrue(orphanedRows.isEmpty()) + } + @Test fun `DS-PROP-011 updateProperty should fail and not mutate a soft-deleted property`(): Unit = runBlocking { From e9f4681031d1d95d575d0b68571b508a6de971d0 Mon Sep 17 00:00:00 2001 From: Cramsan Date: Sun, 26 Jul 2026 17:43:55 -0700 Subject: [PATCH 11/11] [EDIFIKANA] Mark DS-PROP-010 fixed in datastore test catalog (NONE) --- edifikana/back-end/docs/datastore-test-catalog.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/edifikana/back-end/docs/datastore-test-catalog.md b/edifikana/back-end/docs/datastore-test-catalog.md index 4906d3dd3..95358d830 100644 --- a/edifikana/back-end/docs/datastore-test-catalog.md +++ b/edifikana/back-end/docs/datastore-test-catalog.md @@ -112,6 +112,14 @@ datastore query bodies, and mapper functions side by side — not assumptions fr left behind. At minimum, confirm the current `IllegalStateException` fallback (`?: throw IllegalStateException(...)`) only covers the case where the insert returns zero rows silently, not the case where it throws. → **DS-PROP-010** + + **Fixed:** `createProperty` now performs both inserts inside a single atomic + `create_property_with_owner` RPC (`20260726000000_create_property_with_owner_function.sql`), + mirroring the `upsert_rent_config`/`transfer_ownership` pattern — Postgres executes one RPC + call as one transaction, so a failure on the owner-mapping insert rolls back the property + insert too. Confirmed live: `DS-PROP-010b` fabricates a nonexistent `creatorUserId` (an FK + violation on `user_property_mapping_user_id_fkey`), and a raw, untyped select on `properties` + confirms no orphaned row survives. 4. **Six datastores skip the `deleted_at IS NULL` filter on their update path, unlike the rest — soft-deleted rows can be silently mutated.** Contrast the consistent group (CommonArea, Document, Occupant, PaymentRecord, Task, Unit — all filter `deletedAt isExact null` in their @@ -395,7 +403,7 @@ datastore query bodies, and mapper functions side by side — not assumptions fr | ID | Priority | Case | Expected | |---|---|---|---| -| DS-PROP-010 | **P0** | Non-atomic two-insert `createProperty` | Finding #3 | +| DS-PROP-010 | **P0** | Non-atomic two-insert `createProperty` | Fixed — both inserts now run inside a single atomic `create_property_with_owner` RPC; `DS-PROP-010b` confirms no orphaned property row survives an owner-mapping FK failure | | DS-PROP-011 | **P0** | `updateProperty` on a soft-deleted property | Fixed — guard now returns `Result.failure` (zero rows matched), entry left unmutated | | DS-PROP-001 | P1 | `getProperties(userId)` via `v_user_properties` — confirm it excludes soft-deleted properties | Confirmed via migration that `v_user_properties` is defined to exclude soft-deleted properties (`20260317000004...:20-25`) — expected to pass; still worth a regression test given the correctness lives entirely in the view definition (same reasoning as DS-EMP-004) | | DS-PROP-002 | P1 | `getProperties` for a user with zero property mappings | `emptyList()` |