Skip to content

feat(store): policy, relation, and resource reads skip soft-deleted rows #1936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
whoAbhishekSah wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(store): policy, relation, and resource reads skip soft-deleted rows #1936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
141a5ae
feat(store): policy, relation, and resource reads skip soft-deleted rows
whoAbhishekSah Sep 18, 2026
6d0ae34
fix(store): a resource create that reuses a taken id returns conflict
whoAbhishekSah Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions internal/store/postgres/policy_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (r PolicyRepository) buildListQuery() *goqu.SelectDataset {
"p.principal_type",
"p.role_id",
"p.grant_relation",
).From(goqu.T(TABLE_POLICIES).As("p"))
).From(goqu.T(TABLE_POLICIES).As("p")).Where(live("p"))
}

func (r PolicyRepository) Get(ctx context.Context, id string) (policy.Policy, error) {
Expand Down Expand Up @@ -181,7 +181,7 @@ func (r PolicyRepository) List(ctx context.Context, flt policy.Filter) ([]policy

func (r PolicyRepository) Count(ctx context.Context, flt policy.Filter) (int64, error) {
var count int64
stmt := dialect.Select(goqu.COUNT(goqu.Star()).As("count")).From(goqu.T(TABLE_POLICIES).As("p"))
stmt := dialect.Select(goqu.COUNT(goqu.Star()).As("count")).From(goqu.T(TABLE_POLICIES).As("p")).Where(live("p"))
stmt = applyListFilter(stmt, flt)

query, params, err := stmt.ToSQL()
Expand Down Expand Up @@ -292,7 +292,7 @@ func (r PolicyRepository) Update(ctx context.Context, toUpdate policy.Policy) (s
"updated_at": goqu.L("now()"),
}).Where(goqu.Ex{
"id": toUpdate.ID,
}).Returning("id", "updated_at").ToSQL()
}, live(TABLE_POLICIES)).Returning("id", "updated_at").ToSQL()
if err != nil {
return "", fmt.Errorf("%w: %s", errQuery, err)
}
Expand Down Expand Up @@ -461,7 +461,7 @@ func (r PolicyRepository) GroupMemberCount(ctx context.Context, groupIDs []strin
if len(groupIDs) == 0 {
return nil, policy.ErrInvalidID
}
stmt := dialect.From("policies").
stmt := fromLive(TABLE_POLICIES).
Select(goqu.I("resource_id").As("id"), goqu.COUNT(goqu.DISTINCT(goqu.I("principal_id"))).As("count")).
Where(goqu.Ex{
"resource_type": schema.GroupNamespace,
Expand Down Expand Up @@ -498,7 +498,7 @@ func (r PolicyRepository) ProjectMemberCount(ctx context.Context, projectIDs []s
if len(projectIDs) == 0 {
return nil, policy.ErrInvalidID
}
stmt := dialect.From("policies").
stmt := fromLive(TABLE_POLICIES).
Select(goqu.I("resource_id").As("id"), goqu.COUNT(goqu.DISTINCT(goqu.I("principal_id"))).As("count")).
Where(goqu.Ex{
"resource_type": schema.ProjectNamespace,
Expand Down Expand Up @@ -534,7 +534,7 @@ func (r PolicyRepository) OrgMemberCount(ctx context.Context, id string) (policy
if len(id) == 0 {
return policy.MemberCount{}, policy.ErrInvalidID
}
stmt := dialect.From("policies").
stmt := fromLive(TABLE_POLICIES).
Select(goqu.I("resource_id").As("id"), goqu.COUNT(goqu.DISTINCT(goqu.I("principal_id"))).As("count")).
Where(goqu.Ex{
"resource_type": schema.OrganizationNamespace,
Expand Down Expand Up @@ -600,7 +600,7 @@ func (r PolicyRepository) buildPolicyAuditRecord(ctx context.Context, tx *sqlx.T
// getPolicyByConstraint fetches a policy by unique constraint fields
// Returns the policy and true if found, empty policy and false if not found
func (r PolicyRepository) getPolicyByConstraint(ctx context.Context, pol policy.Policy) (Policy, bool) {
query, params, _ := dialect.From(TABLE_POLICIES).
query, params, _ := fromLive(TABLE_POLICIES).
Select("id", "resource_type", "resource_id", "principal_id", "principal_type", "role_id").
Where(goqu.Ex{
"role_id": pol.RoleID,
Expand All @@ -625,19 +625,19 @@ func (r PolicyRepository) getResourceInfo(ctx context.Context, tx *sqlx.Tx, reso
switch resourceType {
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).
Comment on lines 626 to +640

@AmanGIT07 AmanGIT07 Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Select("org_id", "title").
Where(goqu.Ex{"id": resourceID}).
ToSQL()
Expand Down
31 changes: 31 additions & 0 deletions internal/store/postgres/policy_repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,3 +535,34 @@ func (s *PolicyRepositoryTestSuite) TestOrgMemberCount() {
})
}
}

func (s *PolicyRepositoryTestSuite) TestSkipsSoftDeletedPolicies() {
deleted := s.policies[0]
flt := policy.Filter{PrincipalID: s.userID}
before, err := s.repository.List(s.ctx, flt)
s.Require().NoError(err)
countBefore, err := s.repository.Count(s.ctx, flt)
s.Require().NoError(err)

_, err = s.client.ExecContext(s.ctx, "UPDATE policies SET deleted_at = now() WHERE id = $1", deleted.ID)
if err != nil {
s.T().Fatal(err)
}

_, err = s.repository.Get(s.ctx, deleted.ID)
s.Assert().ErrorIs(err, policy.ErrNotExist)

got, err := s.repository.List(s.ctx, flt)
s.Assert().NoError(err)
s.Assert().Len(got, len(before)-1)
for _, p := range got {
s.Assert().NotEqual(deleted.ID, p.ID)
}

count, err := s.repository.Count(s.ctx, flt)
s.Assert().NoError(err)
s.Assert().Equal(countBefore-1, count)

_, err = s.repository.Update(s.ctx, deleted)
s.Assert().ErrorIs(err, policy.ErrNotExist)
}
8 changes: 4 additions & 4 deletions internal/store/postgres/relation_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (r RelationRepository) Upsert(ctx context.Context, relationToCreate relatio
}

func (r RelationRepository) List(ctx context.Context, flt relation.Filter) ([]relation.Relation, error) {
stmt := dialect.Select(&relationCols{}).From(TABLE_RELATIONS)
stmt := fromLive(TABLE_RELATIONS).Select(&relationCols{})
if flt.Subject.ID != "" {
stmt = stmt.Where(goqu.Ex{
"subject_id": flt.Subject.ID,
Expand Down Expand Up @@ -101,7 +101,7 @@ func (r RelationRepository) Get(ctx context.Context, id string) (relation.Relati
return relation.Relation{}, relation.ErrInvalidID
}

query, params, err := dialect.Select(&relationCols{}).From(TABLE_RELATIONS).
query, params, err := fromLive(TABLE_RELATIONS).Select(&relationCols{}).
Where(goqu.Ex{
"id": id,
}).ToSQL()
Expand Down Expand Up @@ -166,7 +166,7 @@ func (r RelationRepository) DeleteByID(ctx context.Context, id string) error {

func (r RelationRepository) GetByFields(ctx context.Context, rel relation.Relation) ([]relation.Relation, error) {
var fetchedRelations []Relation
stmt := dialect.Select(&relationCols{}).From(TABLE_RELATIONS)
stmt := fromLive(TABLE_RELATIONS).Select(&relationCols{})
if rel.Object.ID != "" {
stmt = stmt.Where(goqu.Ex{
"object_id": rel.Object.ID,
Expand Down Expand Up @@ -230,7 +230,7 @@ func (r RelationRepository) ListByFields(ctx context.Context, rel relation.Relat
if len(rel.Object.ID) != 0 {
exprs = append(exprs, goqu.Ex{"object_id": rel.Object.ID})
}
query, params, err := dialect.Select(&relationCols{}).From(TABLE_RELATIONS).Where(exprs...).ToSQL()
query, params, err := fromLive(TABLE_RELATIONS).Select(&relationCols{}).Where(exprs...).ToSQL()
if err != nil {
return nil, fmt.Errorf("%w: %s", errQuery, err)
}
Expand Down
23 changes: 23 additions & 0 deletions internal/store/postgres/relation_repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,3 +323,26 @@ func (s *RelationRepositoryTestSuite) TestDeleteByID() {
func TestRelationRepository(t *testing.T) {
suite.Run(t, new(RelationRepositoryTestSuite))
}

func (s *RelationRepositoryTestSuite) TestSkipsSoftDeletedRelations() {
deleted := s.relations[0]
_, err := s.client.ExecContext(s.ctx, "UPDATE relations SET deleted_at = now() WHERE id = $1", deleted.ID)
if err != nil {
s.T().Fatal(err)
}

_, 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})

@AmanGIT07 AmanGIT07 Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

s.Assert().NoError(err)
for _, r := range got {
s.Assert().NotEqual(deleted.ID, r.ID)
}

byFields, err := s.repository.GetByFields(s.ctx, deleted)
s.Assert().NoError(err)
for _, r := range byFields {
s.Assert().NotEqual(deleted.ID, r.ID)
}
}
10 changes: 6 additions & 4 deletions internal/store/postgres/resource_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ func (r ResourceRepository) Create(ctx context.Context, res resource.Resource) (
return resource.Resource{}, fmt.Errorf("%w: %w", err, resource.ErrInvalidDetail)
case errors.Is(err, ErrInvalidTextRepresentation):
return resource.Resource{}, fmt.Errorf("%w: %w", err, resource.ErrInvalidUUID)
case errors.Is(err, ErrDuplicateKey):
return resource.Resource{}, resource.ErrConflict
default:
return resource.Resource{}, err
}
Expand All @@ -101,7 +103,7 @@ func (r ResourceRepository) Create(ctx context.Context, res resource.Resource) (
func (r ResourceRepository) List(ctx context.Context, flt resource.Filter) ([]resource.Resource, error) {
var fetchedResources []Resource

sqlStatement := dialect.From(TABLE_RESOURCES)
sqlStatement := fromLive(TABLE_RESOURCES)
if flt.ProjectID != "" {
sqlStatement = sqlStatement.Where(goqu.Ex{"project_id": flt.ProjectID})
}
Expand Down Expand Up @@ -149,7 +151,7 @@ func (r ResourceRepository) GetByID(ctx context.Context, id string) (resource.Re
return resource.Resource{}, resource.ErrInvalidID
}

query, params, err := dialect.From(TABLE_RESOURCES).Where(goqu.Ex{
query, params, err := fromLive(TABLE_RESOURCES).Where(goqu.Ex{
Comment thread
coderabbitai[bot] marked this conversation as resolved.
"id": id,
}).ToSQL()
if err != nil {
Expand Down Expand Up @@ -189,7 +191,7 @@ func (r ResourceRepository) Update(ctx context.Context, res resource.Resource) (
"metadata": marshaledMetadata,
"updated_at": goqu.L("now()"),
},
).Where(goqu.Ex{"id": res.ID}).Returning(&ResourceCols{}).ToSQL()
).Where(goqu.Ex{"id": res.ID}, live(TABLE_RESOURCES)).Returning(&ResourceCols{}).ToSQL()
if err != nil {
return resource.Resource{}, fmt.Errorf("%w: %s", errQuery, err)
}
Expand Down Expand Up @@ -221,7 +223,7 @@ func (r ResourceRepository) GetByURN(ctx context.Context, urn string) (resource.
return resource.Resource{}, resource.ErrInvalidURN
}

query, params, err := dialect.Select(&ResourceCols{}).From(TABLE_RESOURCES).Where(
query, params, err := fromLive(TABLE_RESOURCES).Select(&ResourceCols{}).Where(
goqu.Ex{
"urn": urn,
}).ToSQL()
Expand Down
37 changes: 37 additions & 0 deletions internal/store/postgres/resource_repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,3 +411,40 @@ func (s *ResourceRepositoryTestSuite) TestUpdate() {
func TestResourceRepository(t *testing.T) {
suite.Run(t, new(ResourceRepositoryTestSuite))
}

func (s *ResourceRepositoryTestSuite) TestSkipsSoftDeletedResources() {
deleted := s.resources[0]
_, err := s.client.ExecContext(s.ctx, "UPDATE resources SET deleted_at = now() WHERE id = $1", deleted.ID)
if err != nil {
s.T().Fatal(err)
}

_, err = s.repository.GetByID(s.ctx, deleted.ID)
s.Assert().ErrorIs(err, resource.ErrNotExist)

_, 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})

@AmanGIT07 AmanGIT07 Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

s.Assert().NoError(err)
for _, r := range got {
s.Assert().NotEqual(deleted.ID, r.ID)
}

updated := deleted
updated.Title = "changed"
_, err = s.repository.Update(s.ctx, updated)
s.Assert().ErrorIs(err, resource.ErrNotExist)

// the id of a deleted row is still taken; a create that reuses it with a new URN must conflict
_, err = s.repository.Create(s.ctx, resource.Resource{
ID: deleted.ID,
URN: "urn-reusing-a-deleted-id",
Name: "reused-id",
ProjectID: deleted.ProjectID,
NamespaceID: deleted.NamespaceID,
PrincipalID: deleted.PrincipalID,
PrincipalType: deleted.PrincipalType,
})
s.Assert().ErrorIs(err, resource.ErrConflict)
}
Loading