Skip to content

feat(store): role names are unique over live rows only - #1961

Merged
AmanGIT07 merged 2 commits into
mainfrom
soft-delete-role-names
Sep 30, 2026
Merged

AmanGIT07 merged 2 commits into
mainfrom
soft-delete-role-names

Conversation

@AmanGIT07

@AmanGIT07 AmanGIT07 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

What

  • The roles_org_id_name_key constraint is replaced by the partial unique index uq_roles_org_id_name_live over rows where deleted_at IS NULL.
  • The role upsert's conflict target names that index through the liveConflictTarget helper from feat(resource): soft-delete resources and keep the URN unique over live rows only #1937. Migration and query change ship together.
  • The upsert's update branch now sets updated_at.
  • Platform roles live under the zero uuid org id, so the same index covers them.

Why

Soft-deleted rows keep their values. With the plain constraint, an org that deleted a custom role could never create one with the same name again. Names are reusable once the old row is deleted. Role reads already skip deleted rows on main (#1938).

Behaviour change

None today, because nothing sets deleted_at on a role yet. Once role rows are soft-deleted, a deleted name can be reused by a new role in the same org, and the deleted row stays as history. A create with the name of a live role still updates it in place, and now moves updated_at.

Rollout

Between the migration running and the new binary starting, the old binary's ON CONFLICT (org_id, name) no longer matches an index, so role creates fail for that window. Boot only creates default roles that are missing, so an environment that already has them boots on the old binary; an empty database does not. The down migration fails if a deleted and a live role share a name by then.

Tested

  • Repository suite against Postgres 13 in Docker: a deleted name gets a new row, a reused id returns conflict, and a live duplicate updates in place and moves updated_at. The fixture has duplicate role names, so it exercises the update branch on every run.
  • With the old conflict target against the new index, every upsert fails with SQL state 42P10.
  • Migration applied, rolled back, and re-applied on a local Postgres 15.
  • golangci-lint reports no issues on the package.

SQL Safety

  • Values flow through goqu.Record{}. The conflict target and now() are constants with no caller input.
  • ToSQL() params are forwarded unchanged.
  • No ? placeholders inside quoted SQL literals.
  • No new //nolint or #nosec annotations.

@vercel

vercel Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
frontier Ready Ready Preview Sep 30, 2026 6:55am UTC

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: raystack/frontier/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 0d32d5f4-9b4c-4bf4-a935-21c55eb776a1

📥 Commits

Reviewing files that changed from the base of the PR and between 5f23fbc and 7f48580.

📒 Files selected for processing (1)
  • internal/store/postgres/postgres.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/store/postgres/postgres.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Role names can be reused within an organization after the original role is soft-deleted. The deleted role remains in the records, and a separate role is created with the reused name.
    • Upserting a role by the name of an existing active role continues to update that role.
    • Reusing the ID of a soft-deleted role still returns a conflict.

Walkthrough

The migration changes role-name uniqueness to apply only to non-deleted roles. Role upserts use the partial unique index as their conflict target. Tests cover reuse of deleted names, deleted-ID conflicts, and updates to live roles.

Changes

Role Upsert

Layer / File(s) Summary
Partial unique index
internal/store/postgres/migrations/20260929100000_roles_org_id_name_live_unique.up.sql, internal/store/postgres/migrations/20260929100000_roles_org_id_name_live_unique.down.sql
The migration adds a unique index for organization and name on non-deleted roles. The rollback restores the prior unique constraint.
Upsert conflict handling
internal/store/postgres/postgres.go, internal/store/postgres/role_repository.go, internal/store/postgres/role_repository_test.go
Upsert targets the live-row index. Tests cover name reuse after soft deletion, conflicts when reusing a deleted role’s ID, and updates to a live role.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Feature

Suggested reviewers: rohilsurana

Merge Risk: ⚪ Minimal · up to 7f485

The change allows a soft-deleted role's name to be reused and keeps current behavior for live roles. The author documents the brief window between migration and binary deploy in which role creates fail. No merge-blocking risk is evident from the supplied context.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to f166d

The live-role uniqueness rule preserves role identity and existing ownership controls. However, removing the old constraint breaks role creation by older application versions, including during rollback. Startup can also fail when default roles are missing. Deployment sequencing and schema-aware recovery need explicit coordination.

Retained concerns

  • Medium · reliability · inferred: Immediately dropping the full uniqueness constraint makes pre-change role upserts fail across organizations and platform roles sharing the database. This affects mixed-version deployment and previous-image rollback, and can abort startup when default roles are missing. Once deleted/live names overlap, the down migration also fails, constraining recovery of the role-management control plane. Existing populated environments can still boot, and current production deletion does not create the duplicate historical rows.
Security review details

Security Blast Radius

  • inferred — The compatibility failure can affect organization and platform role upserts across every application instance sharing the migrated database. Its demonstrated scope is role-management writes and missing-default-role initialization, not an unauthorized grant of existing roles.

Trust Boundaries and Controls

  • observed — Inspected public organization-role writes require organization RoleManage authorization; update/delete additionally verify role ownership. Platform-role writes require superuser authorization. The PR's changed conflict target remains scoped to organization/name and does not alter these controls.
  • inferred — Reusing a deleted name does not by itself inherit the historical role's ID-bound policies or permission tuples. The new row receives a distinct ID, while the live-conflict update preserves the existing live ID. A future soft-delete lifecycle would still need explicit cleanup of historical authorization relations.

Resilience and Maintainability Implications

  • observed — A repository upsert error stops the role service before permission-relation creation. Bootstrap skips existing predefined roles, but propagates creation failures to server startup. This contains the immediate failed write while leaving security-role provisioning unavailable on incompatible old binaries.

Hardening Proposals

  • proposed — Use a staged compatibility rollout that retains full uniqueness until old role writers are removed, then enables live-only uniqueness. Define schema-aware rollback, duplicate-history handling and interrupted-migration recovery rather than relying on image rollback alone.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Sep 30, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 36680719484

Coverage increased (+0.006%) to 52.866%

Details

  • Coverage increased (+0.006%) from the base build.
  • Patch coverage: 2 of 2 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 41316
Covered Lines: 21842
Line Coverage: 52.87%
Coverage Strength: 17.07 hits per line

💛 - Coveralls

Replace the plain unique constraint on roles (org_id, name) with a unique index over rows where deleted_at is null, and have the role upsert repeat that filter in its conflict target. The upsert's update branch now also sets updated_at.
A deleted name gets a new row, a reused id still conflicts, and a live duplicate updates in place and moves updated_at.
@AmanGIT07
AmanGIT07 force-pushed the soft-delete-role-names branch from 5f23fbc to 7f48580 Compare September 30, 2026 06:54
@AmanGIT07
AmanGIT07 merged commit 8e305d5 into main Sep 30, 2026
8 checks passed
@AmanGIT07
AmanGIT07 deleted the soft-delete-role-names branch September 30, 2026 07:00

This branch was successfully deployed

1 active deployment
Preview — 7f485807 Deployed Sep 30, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants