Skip to content

Commit 5aab4be

Browse files
heiytorheiytor
committed
refactor(api): update Tag interface to use entity objects
- Change TagUpdate to accept *models.Tag instead of separate id/changes parameters - Change TagDelete to accept *models.Tag instead of id string - Update service layer to resolve tags before update/delete operations - Remove TagChanges model usage in favor of direct entity updates - Update all service methods to use new interface signatures - Add proper BSON marshaling with ObjectID conversion in TagUpdate - Update store implementation to use entity objects in transactions - Add case-insensitive name comparison check in UpdateTag - Update TagDelete to use DeleteOne instead of FindOneAndDelete - Simplify object ID conversion by reusing tag.ID in delete flow - Update all mock expectations to use entity objects - Add tenant_id validation in TagUpdate and TagDelete test cases - Update test fixtures to include complete tag objects with IDs BREAKING CHANGE: TagUpdate and TagDelete now accept *models.Tag and return only error
1 parent 7bb7e0f commit 5aab4be

File tree

6 files changed

+94
-56
lines changed

6 files changed

+94
-56
lines changed

‎api/services/tags.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package services
22

33
import (
44
"context"
5+
"strings"
56

67
"github.com/shellhub-io/shellhub/api/store"
78
"github.com/shellhub-io/shellhub/pkg/api/query"
@@ -136,7 +137,11 @@ func (s *service) UpdateTag(ctx context.Context, req *requests.UpdateTag) ([]str
136137
return conflicts, NewErrTagDuplicated(req.NewName, err)
137138
}
138139

139-
if err := s.store.TagUpdate(ctx, tag.ID, &models.TagChanges{Name: req.NewName}); err != nil {
140+
if req.NewName != "" && !strings.EqualFold(req.NewName, tag.Name) {
141+
tag.Name = req.NewName
142+
}
143+
144+
if err := s.store.TagUpdate(ctx, tag); err != nil {
140145
return nil, err
141146
}
142147

@@ -164,6 +169,6 @@ func (s *service) deleteTagCallback(req *requests.DeleteTag) store.TransactionCb
164169
}
165170
}
166171

167-
return s.store.TagDelete(ctx, tag.ID)
172+
return s.store.TagDelete(ctx, tag)
168173
}
169174
}

‎api/services/tags_test.go‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ func TestService_UpdateTag(t *testing.T) {
605605
Once()
606606
storeMock.
607607
On("TagResolve", ctx, store.TagNameResolver, "production", mock.AnythingOfType("store.QueryOption")).
608-
Return(&models.Tag{}, nil).
608+
Return(&models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "production"}, nil).
609609
Once()
610610
storeMock.
611611
On("TagConflicts", ctx, "tenant1", &models.TagConflicts{Name: "staging"}).
@@ -625,6 +625,9 @@ func TestService_UpdateTag(t *testing.T) {
625625
TenantID: "tenant1",
626626
},
627627
requiredMocks: func() {
628+
tag := &models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "production"}
629+
updatedTag := &models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "staging"}
630+
628631
storeMock.
629632
On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "tenant1").
630633
Return(&models.Namespace{}, nil).
@@ -635,14 +638,14 @@ func TestService_UpdateTag(t *testing.T) {
635638
Once()
636639
storeMock.
637640
On("TagResolve", ctx, store.TagNameResolver, "production", mock.AnythingOfType("store.QueryOption")).
638-
Return(&models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000"}, nil).
641+
Return(tag, nil).
639642
Once()
640643
storeMock.
641644
On("TagConflicts", ctx, "tenant1", &models.TagConflicts{Name: "staging"}).
642645
Return([]string{}, false, nil).
643646
Once()
644647
storeMock.
645-
On("TagUpdate", ctx, "tag_00000000-0000-4000-0000-000000000000", &models.TagChanges{Name: "staging"}).
648+
On("TagUpdate", ctx, updatedTag).
646649
Return(errors.New("error")).
647650
Once()
648651
},
@@ -659,6 +662,8 @@ func TestService_UpdateTag(t *testing.T) {
659662
TenantID: "tenant1",
660663
},
661664
requiredMocks: func() {
665+
tag := &models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "production"}
666+
662667
storeMock.
663668
On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "tenant1").
664669
Return(&models.Namespace{}, nil).
@@ -669,14 +674,18 @@ func TestService_UpdateTag(t *testing.T) {
669674
Once()
670675
storeMock.
671676
On("TagResolve", ctx, store.TagNameResolver, "production", mock.AnythingOfType("store.QueryOption")).
672-
Return(&models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000"}, nil).
677+
Return(tag, nil).
673678
Once()
674679
storeMock.
675680
On("TagConflicts", ctx, "tenant1", &models.TagConflicts{Name: "staging"}).
676681
Return([]string{}, false, nil).
677682
Once()
683+
684+
expectedTag := *tag
685+
expectedTag.Name = "staging"
686+
678687
storeMock.
679-
On("TagUpdate", ctx, "tag_00000000-0000-4000-0000-000000000000", &models.TagChanges{Name: "staging"}).
688+
On("TagUpdate", ctx, &expectedTag).
680689
Return(nil).
681690
Once()
682691
},
@@ -757,6 +766,8 @@ func TestService_DeleteTag(t *testing.T) {
757766
TenantID: "tenant1",
758767
},
759768
requiredMocks: func() {
769+
tag := &models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "production"}
770+
760771
storeMock.
761772
On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "tenant1").
762773
Return(&models.Namespace{}, nil).
@@ -767,7 +778,7 @@ func TestService_DeleteTag(t *testing.T) {
767778
Once()
768779
storeMock.
769780
On("TagResolve", ctx, store.TagNameResolver, "production", mock.AnythingOfType("store.QueryOption")).
770-
Return(&models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000"}, nil).
781+
Return(tag, nil).
771782
Once()
772783

773784
for _, target := range store.TagTargets() {
@@ -788,6 +799,8 @@ func TestService_DeleteTag(t *testing.T) {
788799
TenantID: "tenant1",
789800
},
790801
requiredMocks: func() {
802+
tag := &models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "production"}
803+
791804
storeMock.
792805
On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "tenant1").
793806
Return(&models.Namespace{}, nil).
@@ -798,7 +811,7 @@ func TestService_DeleteTag(t *testing.T) {
798811
Once()
799812
storeMock.
800813
On("TagResolve", ctx, store.TagNameResolver, "production", mock.AnythingOfType("store.QueryOption")).
801-
Return(&models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000"}, nil).
814+
Return(tag, nil).
802815
Once()
803816

804817
for _, target := range store.TagTargets() {
@@ -809,7 +822,7 @@ func TestService_DeleteTag(t *testing.T) {
809822
}
810823

811824
storeMock.
812-
On("TagDelete", ctx, "tag_00000000-0000-4000-0000-000000000000").
825+
On("TagDelete", ctx, tag).
813826
Return(errors.New("error")).
814827
Once()
815828
},
@@ -822,6 +835,8 @@ func TestService_DeleteTag(t *testing.T) {
822835
TenantID: "tenant1",
823836
},
824837
requiredMocks: func() {
838+
tag := &models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000", Name: "production"}
839+
825840
storeMock.
826841
On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "tenant1").
827842
Return(&models.Namespace{}, nil).
@@ -832,7 +847,7 @@ func TestService_DeleteTag(t *testing.T) {
832847
Once()
833848
storeMock.
834849
On("TagResolve", ctx, store.TagNameResolver, "production", mock.AnythingOfType("store.QueryOption")).
835-
Return(&models.Tag{ID: "tag_00000000-0000-4000-0000-000000000000"}, nil).
850+
Return(tag, nil).
836851
Once()
837852

838853
for _, target := range store.TagTargets() {
@@ -843,7 +858,7 @@ func TestService_DeleteTag(t *testing.T) {
843858
}
844859

845860
storeMock.
846-
On("TagDelete", ctx, "tag_00000000-0000-4000-0000-000000000000").
861+
On("TagDelete", ctx, tag).
847862
Return(nil).
848863
Once()
849864
},

‎api/store/mocks/store.go‎

Lines changed: 10 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/store/mongo/tags.go‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -139,13 +139,22 @@ func (s *Store) TagResolve(ctx context.Context, resolver store.TagResolver, valu
139139
return tag, nil
140140
}
141141

142-
func (s *Store) TagUpdate(ctx context.Context, id string, changes *models.TagChanges) error {
143-
objID, err := primitive.ObjectIDFromHex(id)
142+
func (s *Store) TagUpdate(ctx context.Context, tag *models.Tag) error {
143+
bsonBytes, err := bson.Marshal(tag)
144144
if err != nil {
145-
return err
145+
return FromMongoError(err)
146+
}
147+
148+
doc := make(bson.M)
149+
if err := bson.Unmarshal(bsonBytes, &doc); err != nil {
150+
return FromMongoError(err)
146151
}
147152

148-
r, err := s.db.Collection("tags").UpdateOne(ctx, bson.M{"_id": objID}, bson.M{"$set": changes})
153+
objID, _ := primitive.ObjectIDFromHex(tag.ID)
154+
doc["_id"] = objID
155+
156+
filter := bson.M{"_id": objID}
157+
r, err := s.db.Collection("tags").UpdateOne(ctx, filter, bson.M{"$set": doc})
149158
if err != nil {
150159
return FromMongoError(err)
151160
}
@@ -212,32 +221,34 @@ func (s *Store) TagPullFromTarget(ctx context.Context, id string, target store.T
212221
}
213222
}
214223

215-
func (s *Store) TagDelete(ctx context.Context, id string) error {
224+
func (s *Store) TagDelete(ctx context.Context, tag *models.Tag) error {
216225
session, err := s.db.Client().StartSession()
217226
if err != nil {
218227
return err
219228
}
220229
defer session.EndSession(ctx)
221230

222231
sessionCallback := func(sessCtx mongo.SessionContext) (any, error) {
223-
objID, err := primitive.ObjectIDFromHex(id)
232+
objID, err := primitive.ObjectIDFromHex(tag.ID)
224233
if err != nil {
225234
return nil, FromMongoError(err)
226235
}
227236

228-
tag := new(models.Tag)
229-
if err := s.db.Collection("tags").FindOneAndDelete(sessCtx, bson.M{"_id": objID}).Decode(tag); err != nil {
237+
r, err := s.db.Collection("tags").DeleteOne(sessCtx, bson.M{"_id": objID})
238+
if err != nil {
230239
return nil, FromMongoError(err)
231240
}
232241

233-
tagID, _ := primitive.ObjectIDFromHex(tag.ID)
242+
if r.DeletedCount < 1 {
243+
return nil, store.ErrNoDocuments
244+
}
234245

235-
if _, err := s.db.Collection("devices").UpdateMany(sessCtx, bson.M{"tenant_id": tag.TenantID}, bson.M{"$pull": bson.M{"tag_ids": tagID}}); err != nil {
246+
if _, err := s.db.Collection("devices").UpdateMany(sessCtx, bson.M{"tenant_id": tag.TenantID}, bson.M{"$pull": bson.M{"tag_ids": objID}}); err != nil {
236247
return nil, FromMongoError(err)
237248
}
238249

239250
for _, c := range []string{"public_keys", "firewall_rules"} {
240-
if _, err := s.db.Collection(c).UpdateMany(sessCtx, bson.M{"tenant_id": tag.TenantID}, bson.M{"$pull": bson.M{"filters.tag_ids": tagID}}); err != nil {
251+
if _, err := s.db.Collection(c).UpdateMany(sessCtx, bson.M{"tenant_id": tag.TenantID}, bson.M{"$pull": bson.M{"filters.tag_ids": objID}}); err != nil {
241252
return nil, FromMongoError(err)
242253
}
243254
}

‎api/store/mongo/tags_test.go‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -347,27 +347,28 @@ func TestStore_TagResolve(t *testing.T) {
347347
func TestStore_TagUpdate(t *testing.T) {
348348
cases := []struct {
349349
description string
350-
id string
351-
changes *models.TagChanges
350+
tag *models.Tag
352351
fixtures []string
353352
expected error
354353
assertChanges func(context.Context) error
355354
}{
356355
{
357-
description: "fails when tag is not found",
358-
id: "000000000000000000000000",
359-
changes: &models.TagChanges{
360-
Name: "edited-tag",
356+
description: "fails when tag is not found due to id",
357+
tag: &models.Tag{
358+
ID: "000000000000000000000000",
359+
TenantID: "00000000-0000-4000-0000-000000000000",
360+
Name: "edited-tag",
361361
},
362362
fixtures: []string{fixtureTags},
363363
expected: store.ErrNoDocuments,
364364
assertChanges: nil,
365365
},
366366
{
367367
description: "succeeds when tag is found",
368-
id: "6791d3ae04ba86e6d7a0514d",
369-
changes: &models.TagChanges{
370-
Name: "edited-tag",
368+
tag: &models.Tag{
369+
ID: "6791d3ae04ba86e6d7a0514d",
370+
TenantID: "00000000-0000-4000-0000-000000000000",
371+
Name: "edited-tag",
371372
},
372373
fixtures: []string{fixtureTags},
373374
expected: nil,
@@ -397,7 +398,7 @@ func TestStore_TagUpdate(t *testing.T) {
397398
require.NoError(tt, srv.Reset())
398399
})
399400

400-
err := s.TagUpdate(ctx, tc.id, tc.changes)
401+
err := s.TagUpdate(ctx, tc.tag)
401402
require.Equal(tt, tc.expected, err)
402403

403404
if err == nil && tc.assertChanges != nil {
@@ -541,21 +542,27 @@ func TestTagPullFromTarget(t *testing.T) {
541542
func TestStore_TagDelete(t *testing.T) {
542543
cases := []struct {
543544
description string
544-
id string
545+
tag *models.Tag
545546
fixtures []string
546547
expected error
547548
}{
548549
{
549-
description: "fails when tag is not found",
550-
id: "000000000000000000000000",
551-
fixtures: []string{fixtureTags},
552-
expected: store.ErrNoDocuments,
550+
description: "fails when tag is not found due to id",
551+
tag: &models.Tag{
552+
ID: "000000000000000000000000",
553+
TenantID: "00000000-0000-4000-0000-000000000000",
554+
},
555+
fixtures: []string{fixtureTags},
556+
expected: store.ErrNoDocuments,
553557
},
554558
{
555559
description: "succeeds when tag is found",
556-
id: "6791d3ae04ba86e6d7a0514d",
557-
fixtures: []string{fixtureTags},
558-
expected: nil,
560+
tag: &models.Tag{
561+
ID: "6791d3ae04ba86e6d7a0514d",
562+
TenantID: "00000000-0000-4000-0000-000000000000",
563+
},
564+
fixtures: []string{fixtureTags},
565+
expected: nil,
559566
},
560567
}
561568

@@ -568,14 +575,14 @@ func TestStore_TagDelete(t *testing.T) {
568575
require.NoError(tt, srv.Reset())
569576
})
570577

571-
err := s.TagDelete(ctx, tc.id)
578+
err := s.TagDelete(ctx, tc.tag)
572579
require.Equal(tt, tc.expected, err)
573580

574581
if err != nil {
575582
return
576583
}
577584

578-
objID, _ := primitive.ObjectIDFromHex(tc.id)
585+
objID, _ := primitive.ObjectIDFromHex(tc.tag.ID)
579586
count, err := db.Collection("tags").CountDocuments(ctx, bson.M{"_id": objID})
580587
require.NoError(tt, err)
581588
require.Equal(tt, int64(0), count)

0 commit comments

Comments
 (0)