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()` | 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 { 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;