feat: transactions and rls rules#2426
Conversation
e73ede8 to
0878464
Compare
82a508e to
08e8681
Compare
|
@cursoragent fix my comments |
|
All changes are pushed. Here's a summary of what was fixed for each PR comment:
|
Implements Row-Level Security (RLS) policies and SQL transaction management. Row-Level Security (RLS): - Database function get_current_organization_id() reads app.current_organization_id from session - RLS policies on tables with organization_id filter rows automatically - Schema uses organizationRLSPolicy() helper to enable RLS per table SQL Transactions: - Domain Layer (@domain/shared): SqlClient interface for database operations - Platform Layer (@platform/db-postgres): SqlClientLive with automatic RLS context - App Layer (apps/*): Boundaries provide SqlClientLive with organization context Usage patterns: - Repositories use sqlClient.query() for single operations - Use cases use sqlClient.transaction() for multi-step operations - Routes provide SqlClientLive(client, organizationId) for RLS enforcement Key behaviors: - Every transaction sets app.current_organization_id session variable - Nested transactions share connection (pass-through proxy) - Domain errors propagate; failures trigger automatic rollback
08e8681 to
fee22ce
Compare
- Return Effect from validateApiKey instead of Promise (restores original pattern) - Replace raw SQL queries with Drizzle ORM in api-keys.test.ts - Replace raw SQL queries with Drizzle ORM in create-test-app.ts - Remove unused withRls method from in-memory-postgres.ts Co-authored-by: Gerard <gerard@latitude.so>
|
@cursoragent review |
|
Now I have a complete picture. Let me compile the review. Here is my review of the PR: OverallStrong architectural direction. The Critical1.
|
| Severity | Count | Key items |
|---|---|---|
| Critical | 4 | exists() bug, wrong org on CLI key, test swallowing errors, silent cross-tenant delete |
| Warning | 4 | Missing transactions, unscoped invites query, raw SQL in test, default org hiding bugs |
| Nit | 4 | Redundant error wrapping, unsafe cast, style inconsistency, role check |
Critical fixes: - Fix subscription-repository exists() always returning true (checked array not element) - Fix exchangeCliSession creating API key with wrong org (pass activeOrganizationId) - Fix complete-auth-intent test swallowing errors (assert expected behavior) - Fix softDelete returning 204 for cross-tenant delete (now returns NotFoundError) - Fix findPendingInvitesByOrganizationId to filter by org via JSONB query - Preserve domain errors through transaction rollback (DomainErrorWrapper) Cleanup: - Replace raw SQL with Drizzle ORM in projects.test.ts - Remove redundant mapError in auth-intent-repository - Fix unsafe type cast in membership-repository (use Effect.map) - Standardize datasets.functions.ts to Effect.provide style - Fix seeds/run.ts passing undefined to SqlClientLive - Add role existence check to createRlsMiddleware in tests Co-authored-by: Gerard <gerard@latitude.so>
Rewrites SqlClientLive to use closure-scoped transaction tracking with a promise-bridge pattern. The inner effect now runs in the parent fiber (via Effect.exit) instead of Effect.runPromiseExit, so all provided services (repositories, cache invalidators, etc.) remain available inside the transaction scope. Key changes: - SqlClientLive tracks activeTx via closure variable shared by query() and transaction() methods on the same instance - transaction() uses promise bridge: Drizzle callback signals tx ready, awaits effect completion, then commits or rolls back - query() checks activeTx at call time, reusing the transaction connection when one is active - Repositories captured at boundary still participate in transactions because query() reads activeTx dynamically, not at capture time Multi-step use cases now properly wrapped in sqlClient.transaction(): - createProjectUseCase: existsByName + existsBySlug + save - updateProjectUseCase: findById + existsByName + save - revokeApiKeyUseCase: findById + save + cache invalidation - changePlan: findActive + revokeBySubscription + saveMany + save Co-authored-by: Gerard <gerard@latitude.so>
Introduces ChSqlClient as the ClickHouse counterpart to SqlClient: - ChSqlClient domain interface in @domain/shared (pass-through transaction, direct query execution) - ChSqlClientLive implementation in @platform/db-clickhouse - DatasetRowRepositoryLive migrated from closure-captured client to ChSqlClient service pattern (Layer.effect + yield* ChSqlClient) Adds withPostgres and withClickHouse helpers that bundle repository layers with their database client in a single call: - Uses Layer.provideMerge so the SqlClient/ChSqlClient service is available both to the repo layers AND to the outer effect (needed for use-case-level sqlClient.transaction() calls) - Repos sharing the same helper call share the same client instance Boundary callers updated from: Effect.provide(RepoLive), Effect.provide(SqlClientLive(client, orgId)), To: Effect.provide(withPostgres(client, orgId, RepoLive)), Co-authored-by: Gerard <gerard@latitude.so>
|
@cursoragent lockfile is stale. Pnpm install and commit the updated lock file |
The implementation skips set_config for the system/admin org context (added in 9256be6), but the test still expected 1 executed statement. Update the assertion to expect 0 statements. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…x detection - Add `createInMemoryPostgres` / `closeInMemoryPostgres` to testkit using PGlite so tests can run without a real Postgres instance - Export from testkit index; add `@electric-sql/pglite` and `drizzle-orm` as deps to testkit - Add `vitest.config.ts` to `apps/web` wiring up the vitest preset - Harden `SqlClientLive`: track `txOpening` flag so concurrent `transaction()` calls on the same instance fail fast via `Effect.die` instead of silently corrupting connections; add tests for both the error and the sequential-OK path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…viders Previously withPostgres and withClickHouse returned a Layer, requiring callers to wrap every call site with Effect.provide(). They also accepted (client, orgId, ...layers) which was hard to type correctly with variadic layer arguments. New signature: (layer, client, orgId?) => pipe-compatible function via Effect.provide internally. Call sites can now use effect.pipe(withPostgres(...)) directly without the outer Effect.provide wrapper. - Rename withClickHouse to the same shape as withPostgres - Migrate all call sites in apps/web and apps/api to the new signature - Consolidate auth functions to withPostgres instead of multiple Effect.provide(RepositoryLive) + Effect.provide(SqlClientLive) chains - Switch exchangeCliSession from adminClient to scoped postgresClient since API key creation is org-scoped Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
67e53d2 to
066db53
Compare
* feat: transactions and rls rules Implements Row-Level Security (RLS) policies and SQL transaction management. Row-Level Security (RLS): - Database function get_current_organization_id() reads app.current_organization_id from session - RLS policies on tables with organization_id filter rows automatically - Schema uses organizationRLSPolicy() helper to enable RLS per table SQL Transactions: - Domain Layer (@domain/shared): SqlClient interface for database operations - Platform Layer (@platform/db-postgres): SqlClientLive with automatic RLS context - App Layer (apps/*): Boundaries provide SqlClientLive with organization context Usage patterns: - Repositories use sqlClient.query() for single operations - Use cases use sqlClient.transaction() for multi-step operations - Routes provide SqlClientLive(client, organizationId) for RLS enforcement Key behaviors: - Every transaction sets app.current_organization_id session variable - Nested transactions share connection (pass-through proxy) - Domain errors propagate; failures trigger automatic rollback * fix: address PR review comments - Return Effect from validateApiKey instead of Promise (restores original pattern) - Replace raw SQL queries with Drizzle ORM in api-keys.test.ts - Replace raw SQL queries with Drizzle ORM in create-test-app.ts - Remove unused withRls method from in-memory-postgres.ts Co-authored-by: Gerard <gerard@latitude.so> * fix: address review findings across the stack Critical fixes: - Fix subscription-repository exists() always returning true (checked array not element) - Fix exchangeCliSession creating API key with wrong org (pass activeOrganizationId) - Fix complete-auth-intent test swallowing errors (assert expected behavior) - Fix softDelete returning 204 for cross-tenant delete (now returns NotFoundError) - Fix findPendingInvitesByOrganizationId to filter by org via JSONB query - Preserve domain errors through transaction rollback (DomainErrorWrapper) Cleanup: - Replace raw SQL with Drizzle ORM in projects.test.ts - Remove redundant mapError in auth-intent-repository - Fix unsafe type cast in membership-repository (use Effect.map) - Standardize datasets.functions.ts to Effect.provide style - Fix seeds/run.ts passing undefined to SqlClientLive - Add role existence check to createRlsMiddleware in tests Co-authored-by: Gerard <gerard@latitude.so> * fix: enable atomic transactions in multi-step use cases Rewrites SqlClientLive to use closure-scoped transaction tracking with a promise-bridge pattern. The inner effect now runs in the parent fiber (via Effect.exit) instead of Effect.runPromiseExit, so all provided services (repositories, cache invalidators, etc.) remain available inside the transaction scope. Key changes: - SqlClientLive tracks activeTx via closure variable shared by query() and transaction() methods on the same instance - transaction() uses promise bridge: Drizzle callback signals tx ready, awaits effect completion, then commits or rolls back - query() checks activeTx at call time, reusing the transaction connection when one is active - Repositories captured at boundary still participate in transactions because query() reads activeTx dynamically, not at capture time Multi-step use cases now properly wrapped in sqlClient.transaction(): - createProjectUseCase: existsByName + existsBySlug + save - updateProjectUseCase: findById + existsByName + save - revokeApiKeyUseCase: findById + save + cache invalidation - changePlan: findActive + revokeBySubscription + saveMany + save Co-authored-by: Gerard <gerard@latitude.so> * feat: add ChSqlClient and withPostgres/withClickHouse helpers Introduces ChSqlClient as the ClickHouse counterpart to SqlClient: - ChSqlClient domain interface in @domain/shared (pass-through transaction, direct query execution) - ChSqlClientLive implementation in @platform/db-clickhouse - DatasetRowRepositoryLive migrated from closure-captured client to ChSqlClient service pattern (Layer.effect + yield* ChSqlClient) Adds withPostgres and withClickHouse helpers that bundle repository layers with their database client in a single call: - Uses Layer.provideMerge so the SqlClient/ChSqlClient service is available both to the repo layers AND to the outer effect (needed for use-case-level sqlClient.transaction() calls) - Repos sharing the same helper call share the same client instance Boundary callers updated from: Effect.provide(RepoLive), Effect.provide(SqlClientLive(client, orgId)), To: Effect.provide(withPostgres(client, orgId, RepoLive)), Co-authored-by: Gerard <gerard@latitude.so> * add tests for the postgres sql client * update lockfile * fix signup/login * added comment, do not set rls if admin context * test(db-postgres): fix sql-client test for system org RLS skip The implementation skips set_config for the system/admin org context (added in 9256be6), but the test still expected 1 executed statement. Update the assertion to expect 0 statements. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(testkit): add PGlite in-memory Postgres adapter and concurrent tx detection - Add `createInMemoryPostgres` / `closeInMemoryPostgres` to testkit using PGlite so tests can run without a real Postgres instance - Export from testkit index; add `@electric-sql/pglite` and `drizzle-orm` as deps to testkit - Add `vitest.config.ts` to `apps/web` wiring up the vitest preset - Harden `SqlClientLive`: track `txOpening` flag so concurrent `transaction()` calls on the same instance fail fast via `Effect.die` instead of silently corrupting connections; add tests for both the error and the sequential-OK path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: make withPostgres/withClickHouse return pipe-compatible providers Previously withPostgres and withClickHouse returned a Layer, requiring callers to wrap every call site with Effect.provide(). They also accepted (client, orgId, ...layers) which was hard to type correctly with variadic layer arguments. New signature: (layer, client, orgId?) => pipe-compatible function via Effect.provide internally. Call sites can now use effect.pipe(withPostgres(...)) directly without the outer Effect.provide wrapper. - Rename withClickHouse to the same shape as withPostgres - Migrate all call sites in apps/web and apps/api to the new signature - Consolidate auth functions to withPostgres instead of multiple Effect.provide(RepositoryLive) + Effect.provide(SqlClientLive) chains - Switch exchangeCliSession from adminClient to scoped postgresClient since API key creation is org-scoped Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat: migrate ch remaining repos to new pattern --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>


Summary
Implements Row-Level Security (RLS) policies and SQL transaction management across the entire stack, from database schema to API boundaries.
SQL Transactions
All sql operations are handled by the SqlClient service. SqlClient abstracts the underlying sql platform as well as the RLS enforcement.
Platforms provide SqlService implementations and app boundaries inject them in the use cases that need them.
@domain/shared):SqlClientinterface—platform-agnostic contract@platform/db-postgres):SqlClientLiveimplementation with RLSapps/*): ProvidesSqlClientLivelayer with organization contextUsage in Code
Repositories use
sqlClient.query()for single operations:.query checks if in transaction, if not it opens one and enforces RLS, if yes it's a pass-through proxy.
Use cases wrap multi-step operations in
sqlClient.transaction():.transaction checks if in transaction, if not it opens one with RLS enforced and updates the context so that any underlying effects pull from this transaction rather than the global sql client. If in transaction, it's a pass-through proxy.
Boundary routes provide the
SqlClientlayer with organization context:Boundaries provide the sql client implementation with proper context. Most of the times this means a postgres client + a tenant id (the ord id). For non-rls ops simply pass the admin postgres client. If you pass the regular postgres client without org id, rls will fail on any rls-protected underlying operation.
Key Behaviors
app.current_organization_idsession variableFiles Changed
packages/domain/shared/src/sql-client.ts— Domain SqlClient interfacepackages/platform/db-postgres/src/sql-client.ts— RLS-enabled transaction implementationpackages/platform/db-postgres/src/schemaHelpers.ts— RLS policy helperpackages/platform/db-postgres/drizzle/*— Migration files for RLS functionapps/*/routes/*.ts— Updated routes to provide SqlClient layerapps/web/src/domains/*/*.functions.ts— Server functions with RLS contextpackages/**/repositories/*.ts— Repositories using sqlClient.query()packages/**/use-cases/*.ts— Use cases using sqlClient.transaction()