Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 76 additions & 2 deletions core/resource/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,82 @@ func (s Service) List(ctx context.Context, flt Filter) ([]Resource, error) {
return s.repository.List(ctx, flt)
}

func (s Service) Update(ctx context.Context, resource Resource) (Resource, error) {
return s.repository.Update(ctx, resource)
// Update persists a resource change and reconciles its #project / #owner
// SpiceDB relations. Previously Update only touched title/metadata in the DB
// and never reconciled relations, so moving a resource to a new project or
// reassigning its owner silently left the old #project/#owner tuples in place
// (and never wrote the new ones). Name and namespace stay immutable here; the
// URN tracks the (possibly new) project name.
func (s Service) Update(ctx context.Context, res Resource) (Resource, error) {
existing, err := s.repository.GetByID(ctx, res.ID)
if err != nil {
return Resource{}, err
}

principalID := res.PrincipalID
principalType := res.PrincipalType
// PAT → resolve to underlying user, mirroring Create
if principalType == schema.PATPrincipal {
sub, err := s.resolvePATUser(ctx, principalID)
if err != nil {
return Resource{}, fmt.Errorf("resolving PAT principal: %w", err)
}
principalID = sub.ID
principalType = sub.Namespace
}
// no owner supplied → keep the current one
if strings.TrimSpace(principalID) == "" {
principalID = existing.PrincipalID
principalType = existing.PrincipalType
}

resourceProject, err := s.projectService.Get(ctx, res.ProjectID)
if err != nil {
return Resource{}, fmt.Errorf("failed to get project: %w", err)
}

updated, err := s.repository.Update(ctx, Resource{
ID: existing.ID,
URN: existing.CreateURN(resourceProject.Name),
Name: existing.Name,
Title: res.Title,
ProjectID: resourceProject.ID,
NamespaceID: existing.NamespaceID,
PrincipalID: principalID,
PrincipalType: principalType,
Metadata: res.Metadata,
})
if err != nil {
return Resource{}, err
}

// reconcile the project grant if the resource moved projects
if existing.ProjectID != updated.ProjectID {
if err = s.relationService.Delete(ctx, relation.Relation{
Object: relation.Object{ID: updated.ID, Namespace: updated.NamespaceID},
RelationName: schema.ProjectRelationName,
}); err != nil && !errors.Is(err, relation.ErrNotExist) {
return Resource{}, err
}
if err = s.AddProjectToResource(ctx, updated.ProjectID, updated); err != nil {
return Resource{}, err
}
}

// reconcile the owner grant if the owner changed
if existing.PrincipalID != updated.PrincipalID || existing.PrincipalType != updated.PrincipalType {
if err = s.relationService.Delete(ctx, relation.Relation{
Object: relation.Object{ID: updated.ID, Namespace: updated.NamespaceID},
RelationName: schema.OwnerRelationName,
}); err != nil && !errors.Is(err, relation.ErrNotExist) {
return Resource{}, err
}
if err = s.AddResourceOwner(ctx, updated); err != nil {
return Resource{}, err
}
}

return updated, nil
}

func (s Service) AddProjectToResource(ctx context.Context, projectID string, res Resource) error {
Expand Down
49 changes: 44 additions & 5 deletions core/resource/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -468,15 +468,54 @@ func TestList(t *testing.T) {
}

func TestUpdate(t *testing.T) {
t.Run("delegates to repository", func(t *testing.T) {
repo, _, _, _, _, _, _, _, svc := newTestService(t)
res := resource.Resource{ID: "r1", Title: "updated"}
repo.EXPECT().Update(mock.Anything, res).Return(res, nil)
proj := project.Project{ID: uuid.New().String(), Name: "proj"}

got, err := svc.Update(context.Background(), res)
t.Run("metadata-only update does not touch relations", func(t *testing.T) {
repo, _, _, _, projectSvc, _, _, _, svc := newTestService(t)
existing := resource.Resource{
ID: "r1", Name: "res", NamespaceID: "resource/item",
ProjectID: proj.ID, PrincipalID: "u1", PrincipalType: schema.UserPrincipal,
}
repo.EXPECT().GetByID(mock.Anything, "r1").Return(existing, nil)
projectSvc.EXPECT().Get(mock.Anything, proj.ID).Return(proj, nil)
// owner unchanged, project unchanged -> repository update only, no relation calls
updated := existing
updated.Title = "updated"
repo.EXPECT().Update(mock.Anything, mock.Anything).Return(updated, nil)

got, err := svc.Update(context.Background(), resource.Resource{
ID: "r1", Title: "updated", ProjectID: proj.ID,
PrincipalID: "u1", PrincipalType: schema.UserPrincipal,
})
assert.NoError(t, err)
assert.Equal(t, "updated", got.Title)
})

t.Run("moving project reconciles the #project relation", func(t *testing.T) {
repo, _, relationSvc, _, projectSvc, _, _, _, svc := newTestService(t)
newProj := project.Project{ID: uuid.New().String(), Name: "new-proj"}
existing := resource.Resource{
ID: "r1", Name: "res", NamespaceID: "resource/item",
ProjectID: proj.ID, PrincipalID: "u1", PrincipalType: schema.UserPrincipal,
}
updated := existing
updated.ProjectID = newProj.ID
repo.EXPECT().GetByID(mock.Anything, "r1").Return(existing, nil)
projectSvc.EXPECT().Get(mock.Anything, newProj.ID).Return(newProj, nil)
repo.EXPECT().Update(mock.Anything, mock.Anything).Return(updated, nil)
// old #project tuple removed, new one written
relationSvc.EXPECT().Delete(mock.Anything, relation.Relation{
Object: relation.Object{ID: "r1", Namespace: "resource/item"},
RelationName: schema.ProjectRelationName,
}).Return(nil)
relationSvc.EXPECT().Create(mock.Anything, mock.Anything).Return(relation.Relation{}, nil)

_, err := svc.Update(context.Background(), resource.Resource{
ID: "r1", ProjectID: newProj.ID,
PrincipalID: "u1", PrincipalType: schema.UserPrincipal,
})
assert.NoError(t, err)
})
}

func TestDelete(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions internal/store/postgres/resource_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,12 @@ func (r ResourceRepository) Update(ctx context.Context, res resource.Resource) (
}
query, params, err := dialect.Update(TABLE_RESOURCES).Set(
goqu.Record{
"title": res.Title,
"metadata": marshaledMetadata,
"title": res.Title,
"metadata": marshaledMetadata,
"urn": res.URN,
"project_id": res.ProjectID,
"principal_id": res.PrincipalID,
"principal_type": res.PrincipalType,
Comment on lines +193 to +194

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Inconsistent NULL handling for principal_id/principal_type.

Create wraps principal_id and principal_type in sql.NullString (lines 51-52) to store NULL when empty, but Update writes them as raw strings. If the service passes empty strings, the DB will store "" instead of NULL, breaking consistency with Create behavior and potentially causing filter/query issues.

🛡️ Proposed fix to match Create's NULL handling
+	principalID := sql.NullString{String: res.PrincipalID, Valid: res.PrincipalID != ""}
+	principalType := sql.NullString{String: res.PrincipalType, Valid: res.PrincipalType != ""}
+
 	query, params, err := dialect.Update(TABLE_RESOURCES).Set(
 		goqu.Record{
 			"title":          res.Title,
 			"metadata":       marshaledMetadata,
 			"urn":            res.URN,
 			"project_id":     res.ProjectID,
-			"principal_id":   res.PrincipalID,
-			"principal_type": res.PrincipalType,
+			"principal_id":   principalID,
+			"principal_type": principalType,
 		},
 	).Where(goqu.Ex{"id": res.ID}).Returning(&ResourceCols{}).ToSQL()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"principal_id": res.PrincipalID,
"principal_type": res.PrincipalType,
principalID := sql.NullString{String: res.PrincipalID, Valid: res.PrincipalID != ""}
principalType := sql.NullString{String: res.PrincipalType, Valid: res.PrincipalType != ""}
query, params, err := dialect.Update(TABLE_RESOURCES).Set(
goqu.Record{
"title": res.Title,
"metadata": marshaledMetadata,
"urn": res.URN,
"project_id": res.ProjectID,
"principal_id": principalID,
"principal_type": principalType,
},
).Where(goqu.Ex{"id": res.ID}).Returning(&ResourceCols{}).ToSQL()

},
).Where(goqu.Ex{"id": res.ID}).Returning(&ResourceCols{}).ToSQL()
if err != nil {
Expand Down
11 changes: 7 additions & 4 deletions internal/store/postgres/resource_repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,13 @@ func (s *ResourceRepositoryTestSuite) TestUpdate() {
Description: "should update a resource",
ResourceID: s.resources[0].ID,
ResourceToUpdate: resource.Resource{
ID: s.resources[0].ID,
Name: "resource-1",
ProjectID: s.resources[0].ProjectID,
NamespaceID: s.resources[0].NamespaceID,
ID: s.resources[0].ID,
URN: "resource-1-urn",
Name: "resource-1",
ProjectID: s.resources[0].ProjectID,
NamespaceID: s.resources[0].NamespaceID,
PrincipalID: s.resources[0].PrincipalID,
PrincipalType: s.resources[0].PrincipalType,
},
ExpectedResource: resource.Resource{
ID: s.resources[0].ID,
Expand Down
62 changes: 62 additions & 0 deletions test/e2e/regression/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2894,6 +2894,68 @@ func (s *APIRegressionTestSuite) TestOrganizationRoleDeleteInUse() {
s.Assert().False(roleHasPermTuples())
}

// TestProjectResourceUpdateReconcile asserts that moving a resource to another
// project reconciles its #project SpiceDB relation: the new project tuple is
// written and the old one is removed (gap #1661.7).
func (s *APIRegressionTestSuite) TestProjectResourceUpdateReconcile() {
ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie)

createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{
Body: &frontierv1beta1.OrganizationRequestBody{Name: "org-resource-reconcile"},
}))
s.Require().NoError(err)
orgID := createOrgResp.Msg.GetOrganization().GetId()

projA, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{
Body: &frontierv1beta1.ProjectRequestBody{Name: "resource-reconcile-proj-a", OrgId: orgID},
}))
s.Require().NoError(err)
projB, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{
Body: &frontierv1beta1.ProjectRequestBody{Name: "resource-reconcile-proj-b", OrgId: orgID},
}))
s.Require().NoError(err)

createResourceResp, err := s.testBench.Client.CreateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectResourceRequest{
ProjectId: projA.Msg.GetProject().GetId(),
Body: &frontierv1beta1.ResourceRequestBody{
Name: "reconcile-res",
Namespace: computeOrderNamespace,
},
}))
s.Require().NoError(err)
resourceID := createResourceResp.Msg.GetResource().GetId()

projectSubjectOf := func() string {
resp, err := s.testBench.AdminClient.ListRelations(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListRelationsRequest{
Object: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID),
}))
s.Require().NoError(err)
for _, rel := range resp.Msg.GetRelations() {
if rel.GetRelation() == schema.ProjectRelationName {
return rel.GetSubject()
}
}
return ""
}

// before the move the resource points at project A
s.Assert().Equal(schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, projA.Msg.GetProject().GetId()), projectSubjectOf())

// move the resource to project B
_, err = s.testBench.Client.UpdateProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.UpdateProjectResourceRequest{
Id: resourceID,
ProjectId: projB.Msg.GetProject().GetId(),
Body: &frontierv1beta1.ResourceRequestBody{
Name: "reconcile-res",
Namespace: computeOrderNamespace,
},
}))
s.Require().NoError(err)

// the #project relation now points at project B, and only B (old tuple gone)
s.Assert().Equal(schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, projB.Msg.GetProject().GetId()), projectSubjectOf())
}

func TestEndToEndAPIRegressionTestSuite(t *testing.T) {
suite.Run(t, new(APIRegressionTestSuite))
}
Loading