Skip to content

perf(core): add TwoRelationsQueries.replaceWithDelta(); switch org-user PUT to use it#8820

Merged
simeng-li merged 2 commits into
masterfrom
simeng-log-13474-perf-c-rewrite-tworelationsqueriesreplace-as-a-delta-cte
May 19, 2026
Merged

perf(core): add TwoRelationsQueries.replaceWithDelta(); switch org-user PUT to use it#8820
simeng-li merged 2 commits into
masterfrom
simeng-log-13474-perf-c-rewrite-tworelationsqueriesreplace-as-a-delta-cte

Conversation

@simeng-li
Copy link
Copy Markdown
Contributor

@simeng-li simeng-li commented May 15, 2026

Summary

Refs LOG-13474. Third task in the Phase 0.5 performance milestone. Stacked on #8818 (Perf-A — the organization_user_relations (tenant_id, user_id) index) which the new method's delta CTE depends on at scale.

What changed

  • packages/core/src/utils/RelationQueries.ts — adds a new opt-in method replaceWithDelta(schema1Id, schema2Ids) on TwoRelationsQueries. Computes added/removed id sets in a single CTE statement (Postgres supports INSERT and DELETE in WITH) and returns { added, removed }. Preserves the select ... for update row lock on the Schema1 entity that replace() already takes. The existing replace() method is left untouched.
  • packages/core/src/routes/organization/user/index.ts — switches PUT /organizations/:id/users to call replaceWithDelta() instead of replace(). This is the only call site that migrates in this PR.
  • packages/core/src/utils/RelationQueries.test.ts — new file. 3 unit tests using a slonik mock pool that captures every query: verifies the row lock fires first, the delta CTE structure (with graph + returning + array_agg), and the empty-result return shape.
  • packages/integration-tests/src/tests/api/organization/organization-user.test.ts — 5 new integration cases under PUT /organizations/:id/users delta semantics: no-op PUT, partial-overlap, empty body, no-overlap-from-empty, and a role-preservation regression test that fails on master and passes after this change.
  • .changeset/perf-relation-queries-replace-delta.md@logto/corepatch.

Expected result

  • PUT /organizations/:id/users with a no-op body produces zero row writes against organization_user_relations (master: ~2N).
  • A one-row delta on an N-row membership produces exactly one row write.
  • Members whose membership survives a PUT keep their role assignments. Previously, the DELETE WHERE org_id = X in replace() cascaded through organization_role_user_relations and silently dropped every member's roles on every PUT — even no-op PUTs. New behavior is strictly more correct.
  • API response shape is unchanged (204 No Content). replaceWithDelta's return value is discarded by this caller; it is wired up for downstream consumption by Phase 1 (LOG-13462).
  • The other nine TwoRelationsQueries subclasses (SSO connector relations, the four application_user_consent_* tables, organization_role_scope_relations, organization_role_resource_scope_relations, organization_invitation_role_relations, organization_jit_roles) continue to use replace() and see no behavior change.

Reviewer notes

  • replaceWithDelta was kept as a separate method rather than rewriting replace() in place because of the cascade-preservation behavior difference. Of the ten TwoRelationsQueries subclasses, only two sit upstream of FK cascades (organization_user_relations and organization_application_relations), and only one of those (organization_user_relations) is the membership-set hot path. Opt-in migration bounds the blast radius to call sites that explicitly want the new semantics, and lets reviewers reason about each migration on its own.
  • The NOT IN form for the deleted CTE was rewritten to NOT EXISTS after a self-review surfaced its NULL-fragility. next_set.id from unnest(varchar[]) can't be NULL in normal operation, but the rewrite is null-safe and a touch more index-friendly.
  • The unit-test mock pool is hand-rolled because slonik's createMockPool only exposes a query override, not transaction. The mock records every query (including the ones inside pool.transaction(cb)), which is sufficient for asserting structural invariants without locking in line-by-line SQL.

Testing

Unit tests, integration tests

Checklist

  • .changeset
  • unit tests
  • integration tests
  • necessary TSDoc comments

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 15, 2026

COMPARE TO master

Total Size Diff 📉 -3.74 KB

Diff by File
Name Diff
.changeset/console-audit-logs-time-picker.md 📉 -434 Bytes
.changeset/perf-list-org-users-lateral.md 📉 -1.05 KB
.changeset/perf-relation-queries-replace-delta.md 📉 -1.36 KB
packages/console/src/components/AuditLogTable/index.tsx 📉 -181 Bytes
packages/core/src/libraries/protected-app.test.ts 📉 -1.54 KB
packages/core/src/libraries/protected-app.ts 📉 -116 Bytes
packages/core/src/queries/application-secrets.ts 📉 -950 Bytes
packages/core/src/queries/organization/user-relations.ts 📉 -766 Bytes
packages/core/src/routes/applications/application-secret.test.ts 📉 -5.95 KB
packages/core/src/routes/applications/application-secret.ts 📉 -1.75 KB
packages/core/src/routes/applications/application.ts 📉 -44 Bytes
packages/core/src/routes/organization/user/index.ts 📉 -96 Bytes
packages/core/src/utils/RelationQueries.test.ts 📉 -3.86 KB
packages/core/src/utils/RelationQueries.ts 📉 -2.89 KB
packages/integration-tests/src/tests/api/organization/organization-user.test.ts 📉 -1.99 KB
packages/phrases/src/locales/ar/errors/application.ts 📉 -80 Bytes
packages/phrases/src/locales/de/errors/application.ts 📉 -97 Bytes
packages/phrases/src/locales/en/errors/application.ts 📉 -69 Bytes
packages/phrases/src/locales/es/errors/application.ts 📉 -88 Bytes
packages/phrases/src/locales/fr/errors/application.ts 📉 -94 Bytes
packages/phrases/src/locales/it/errors/application.ts 📉 -98 Bytes
packages/phrases/src/locales/ja/errors/application.ts 📉 -113 Bytes
packages/phrases/src/locales/ko/errors/application.ts 📉 -96 Bytes
packages/phrases/src/locales/pl-pl/errors/application.ts 📉 -89 Bytes
packages/phrases/src/locales/pt-br/errors/application.ts 📉 -83 Bytes
packages/phrases/src/locales/pt-pt/errors/application.ts 📉 -84 Bytes
packages/phrases/src/locales/ru/errors/application.ts 📉 -125 Bytes
packages/phrases/src/locales/th/errors/application.ts 📉 -94 Bytes
packages/phrases/src/locales/tr-tr/errors/application.ts 📉 -84 Bytes
packages/phrases/src/locales/zh-cn/errors/application.ts 📉 -71 Bytes
packages/phrases/src/locales/zh-hk/errors/application.ts 📉 -71 Bytes
packages/phrases/src/locales/zh-tw/errors/application.ts 📉 -71 Bytes
packages/schemas/src/consts/application.ts 📉 -62 Bytes
packages/schemas/src/consts/index.ts 📉 -34 Bytes

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces an opt-in, delta-based replacement method for 2-table relation sets to reduce write amplification and avoid unintended FK-cascade side effects, and migrates PUT /organizations/:id/users to use it.

Changes:

  • Added TwoRelationsQueries.replaceWithDelta() that computes added/removed relation IDs via a single Postgres CTE (INSERT/DELETE) and returns { added, removed }.
  • Switched PUT /organizations/:id/users to use replaceWithDelta() to avoid rewriting large membership sets and to preserve role relations for unchanged members.
  • Added unit tests for the new delta SQL shape and integration tests covering delta semantics + role-preservation regression.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/core/src/utils/RelationQueries.ts Adds replaceWithDelta() transactional delta CTE implementation returning added/removed sets.
packages/core/src/routes/organization/user/index.ts Migrates org membership PUT endpoint to use replaceWithDelta().
packages/core/src/utils/RelationQueries.test.ts Adds unit tests asserting row-lock ordering and the delta CTE structure/return shape.
packages/integration-tests/src/tests/api/organization/organization-user.test.ts Adds integration coverage for PUT delta semantics and role-preservation regression.
.changeset/perf-relation-queries-replace-delta.md Documents the new method and the migrated call site as a @logto/core minor change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/core/src/routes/organization/user/index.ts
Comment thread packages/core/src/utils/RelationQueries.test.ts
Comment thread packages/integration-tests/src/tests/api/organization/organization-user.test.ts Outdated
@simeng-li simeng-li force-pushed the simeng-log-13472-perf-a-add-index-on-organization_user_relations-tenant_id branch from 44a9e9e to 33f35a6 Compare May 15, 2026 09:34
@simeng-li simeng-li force-pushed the simeng-log-13474-perf-c-rewrite-tworelationsqueriesreplace-as-a-delta-cte branch from 8a90c2d to a43c4ed Compare May 15, 2026 09:40
@github-actions github-actions Bot added size/l and removed size/l labels May 15, 2026
Base automatically changed from simeng-log-13472-perf-a-add-index-on-organization_user_relations-tenant_id to master May 18, 2026 02:35
@github-actions github-actions Bot added size/l and removed size/l labels May 18, 2026
…er PUT to use it

TwoRelationsQueries.replace() runs DELETE-all + bulk INSERT in a
transaction, rewriting O(N) rows on every call regardless of the actual
delta size. At 10k+ memberships that became a meaningful share of PUT
request latency, and at the FK-cascade fan-out level it silently
dropped dependent rows on every call (e.g. wiping every member's role
assignments on every PUT /organizations/:id/users).

This change adds an opt-in sibling method, replaceWithDelta(), that
computes the added/removed sets in a single CTE statement and writes
only the rows that change. A no-op call writes zero rows; a one-row
delta writes one row. It returns { added, removed } so consumers (e.g.
LOG-13462's webhook payload work) can use the delta without a
re-query. replace() is unchanged and continues to back the other nine
TwoRelationsQueries subclasses, none of which need the new behavior.

Only one caller migrates in this PR: PUT /organizations/:id/users.
That route's membership set is the one that grows to 10k+ in
production, and it sits upstream of the cascade to
organization_role_user_relations — so the migration also fixes the
silent role-dropping bug. An integration test guards the
role-preservation behavior; unit tests cover the new SQL shape and
the preserved select ... for update lock.

Refs LOG-13474
@simeng-li simeng-li force-pushed the simeng-log-13474-perf-c-rewrite-tworelationsqueriesreplace-as-a-delta-cte branch from a43c4ed to 54638b5 Compare May 18, 2026 02:37
@github-actions github-actions Bot added size/l and removed size/l labels May 18, 2026
@gao-sun gao-sun self-requested a review May 19, 2026 05:30
Comment thread .changeset/perf-relation-queries-replace-delta.md Outdated
change version bump to patch
Copilot AI review requested due to automatic review settings May 19, 2026 06:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread .changeset/perf-relation-queries-replace-delta.md
@github-actions github-actions Bot added size/l and removed size/l labels May 19, 2026
@simeng-li simeng-li enabled auto-merge (squash) May 19, 2026 06:16
@simeng-li simeng-li merged commit 26c8c3f into master May 19, 2026
38 checks passed
@simeng-li simeng-li deleted the simeng-log-13474-perf-c-rewrite-tworelationsqueriesreplace-as-a-delta-cte branch May 19, 2026 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

4 participants