feat(store): policy, relation, and resource reads skip soft-deleted rows - #1936
whoAbhishekSah wants to merge 2 commits into
Conversation
Every read in the three repositories goes through fromLive, or adds the live filter directly where the query aliases the table. The policy repository's small lookups of organization, project, and group titles for audit records use fromLive too. The two updates keyed by id add the filter as well. The ON CONFLICT upserts are unchanged. They move with the unique constraints in a later change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: raystack/frontier/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 SummarySummary by CodeRabbit
WalkthroughPostgreSQL policy, relation, and resource repositories now use live-row filters for reads and applicable updates. Tests verify that soft-deleted records are excluded and return not-exist or conflict errors where applicable. ChangesRepository live-row filtering
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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. Comment |
Coverage Report for CI Build 35560452772Coverage increased (+0.2%) to 50.472%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: afc335bf-b2e4-4815-9d2b-9a3083fff1eb
📒 Files selected for processing (6)
internal/store/postgres/policy_repository.gointernal/store/postgres/policy_repository_test.gointernal/store/postgres/relation_repository.gointernal/store/postgres/relation_repository_test.gointernal/store/postgres/resource_repository.gointernal/store/postgres/resource_repository_test.go
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
|
Tested live on a local server built from this branch (main with #1931 merged). Fixtures: a user with an org viewer policy, a project viewer policy, and a platform admin relation, plus two custom resources under a live project. Ran every affected RPC before and after setting
Write paths against a hidden row, worth noting for the phased plan:
No behaviour change today since no row has |
The create upsert targets urn only. An insert that reuses an existing id with a different urn fails on the primary key and fell through to a raw duplicate key error. It now maps to resource.ErrConflict, which the handler already turns into a conflict response. With live-only reads the id of a soft-deleted row is invisible to the pre-check, so this path is easier to reach.
| SELECT id FROM ` + TABLE_POLICIES + ` | ||
| WHERE resource_id = $2 | ||
| AND resource_type = $3 | ||
| AND role_id = $4 | ||
| ORDER BY id | ||
| FOR UPDATE |
There was a problem hiding this comment.
The locked CTE still counts soft-deleted rows, so a deleted owner policy keeps the guard from firing and the last live owner can be removed. I checked with a test: one live owner plus one soft-deleted owner, and the guard deletes the live one. Can we add AND deleted_at IS NULL to the CTE and cover it with a test that soft-deletes the second holder?
| case schema.OrganizationNamespace: | ||
| orgID = resourceID | ||
| orgQuery, orgParams, _ := dialect.From(TABLE_ORGANIZATIONS). | ||
| orgQuery, orgParams, _ := fromLive(TABLE_ORGANIZATIONS). | ||
| Select("title"). | ||
| Where(goqu.Ex{"id": resourceID}). | ||
| ToSQL() | ||
| _ = tx.QueryRowContext(ctx, orgQuery, orgParams...).Scan(&resourceName) | ||
| case schema.ProjectNamespace: | ||
| projQuery, projParams, _ := dialect.From(TABLE_PROJECTS). | ||
| projQuery, projParams, _ := fromLive(TABLE_PROJECTS). | ||
| Select("org_id", "title"). | ||
| Where(goqu.Ex{"id": resourceID}). | ||
| ToSQL() | ||
| _ = tx.QueryRowContext(ctx, projQuery, projParams...).Scan(&orgID, &resourceName) | ||
| case schema.GroupNamespace: | ||
| grpQuery, grpParams, _ := dialect.From(TABLE_GROUPS). | ||
| grpQuery, grpParams, _ := fromLive(TABLE_GROUPS). |
There was a problem hiding this comment.
These lookups only fill the org id and title on the audit record. Once projects and groups are soft-deleted, a policy removed under one of them gets no org id and drops out of the org's audit view. The org title lookup in the audit insert reads all rows today. Can we keep these three on the plain From so the trail stays attributed?
| if len(flt.RolePermissions) > 0 { | ||
| // Join the roles table to keep only policies whose role grants at | ||
| // least one of the listed permission names. | ||
| stmt = stmt. | ||
| Join( | ||
| goqu.T(TABLE_ROLES).As("r"), | ||
| goqu.On(goqu.I("r.id").Eq(goqu.I("p.role_id"))), | ||
| ). | ||
| Where(goqu.Func( | ||
| "jsonb_exists_any", | ||
| goqu.I("r.permissions"), | ||
| pq.Array(flt.RolePermissions), | ||
| )) | ||
| } |
There was a problem hiding this comment.
The role join doesn't have deleted_at filter, will this be part of role repository change? If not, we can add it here
| _, err = s.repository.GetByURN(s.ctx, deleted.URN) | ||
| s.Assert().ErrorIs(err, resource.ErrNotExist) | ||
|
|
||
| got, err := s.repository.List(s.ctx, resource.Filter{ProjectID: deleted.ProjectID}) |
There was a problem hiding this comment.
This project has only one resource, and we just soft-deleted it. So got is empty here and the loop below never runs. The test only proves that List did not fail. Can we check the count instead, like the policy test does? For example, call List with no filter and expect len(s.resources)-1 rows.
| _, err = s.repository.Get(s.ctx, deleted.ID) | ||
| s.Assert().ErrorIs(err, relation.ErrNotExist) | ||
|
|
||
| got, err := s.repository.List(s.ctx, relation.Filter{Subject: deleted.Subject, Object: deleted.Object}) |
There was a problem hiding this comment.
Only one relation matches this subject and object, and we just soft-deleted it. So got is empty and the loop never runs. The same happens for byFields below. Can we check the count instead? For example, call List with no filter and expect one row less than we started with.
|
About RemovePlatformUser and DeleteRelation returning success but doing nothing on a hidden row, from your testing note: is that fix meant for this PR, or is it planned for later? If later, which change will pick it up? |
3 participants
Second of three PRs that make the identity repositories skip soft-deleted rows. Follows #1931.
What
fromLive, or addslive("p")directly where the query aliases the table.fromLivetoo.livefilter.ON CONFLICTupserts are unchanged. They move together with the unique constraints in a later PR.Behaviour change
None today. No row has
deleted_atset yet.Tested
One new test per repository: soft-delete a seeded row, then check every get, list, count, and update skips it. Store package passes with
-race.