Fixes: https://todo.code.netlandish.com/~netlandish/links/118
Changelog-fixed: Tag removal bug where tags are incorrectly removed from
multiple objects.
---
This is an embarassing bug to have missed. Added tests as well for these
cases
api/api_test.go | 113 ++++++++++++++++++++++++++++++++++
api/graph/schema.resolvers.go | 6 +-
models/tag_link_shorts.go | 7 ++-
models/tag_links.go | 7 ++-
models/tag_listing.go | 7 ++-
5 files changed, 131 insertions(+), 9 deletions(-)
diff --git a/api/api_test.go b/api/api_test.go
index 465c019..d098661 100644
--- a/api/api_test.go
+++ b/api/api_test.go
@@ -3313,4 +3313,117 @@ func TestAPI(t *testing.T) {
c.Error(err)
c.Equal("gqlclient: server failure: Organization not found.", err.Error())
})
+
+ t.Run("tag removal isolation", func(t *testing.T) {
+ c := require.New(t)
+ type AddLinkResponse struct {
+ Link models.OrgLink `json:"addLink"`
+ }
+ type UpdateLinkResponse struct {
+ Link models.OrgLink `json:"updateLink"`
+ }
+
+ addLinkQuery := `mutation AddLink($title: String!, $url: String!, $description: String,
+ $visibility: LinkVisibility!, $unread: Boolean!, $starred: Boolean!,
+ $archive: Boolean! $slug: String!, $tags: String) {
+ addLink(input: {
+ title: $title,
+ url: $url,
+ description: $description,
+ visibility: $visibility,
+ unread: $unread,
+ starred: $starred,
+ archive: $archive,
+ orgSlug: $slug,
+ tags: $tags}) {
+ id
+ hash
+ }
+ }`
+
+ var linkA AddLinkResponse
+ opA := gqlclient.NewOperation(addLinkQuery)
+ opA.Var("title", "Link A isolation")
+ opA.Var("url", "https://example.com/link-a-isolation")
+ opA.Var("description", "")
+ opA.Var("visibility", models.OrgLinkVisibilityPublic)
+ opA.Var("tags", "isolation-common, isolation-unique-a")
+ opA.Var("slug", "personal-org")
+ opA.Var("unread", false)
+ opA.Var("starred", false)
+ opA.Var("archive", false)
+ err := links.Execute(ctx, opA, &linkA)
+ c.NoError(err)
+ c.NotZero(linkA.Link.ID)
+
+ tagLinksA, err := models.GetTagLinks(dbCtx,
+ &database.FilterOptions{Filter: sq.Expr("org_link_id = ?", linkA.Link.ID)})
+ c.NoError(err)
+ c.Equal(2, len(tagLinksA), "Link A should have 2 tags after creation")
+
+ var linkB AddLinkResponse
+ opB := gqlclient.NewOperation(addLinkQuery)
+ opB.Var("title", "Link B isolation")
+ opB.Var("url", "https://example.com/link-b-isolation")
+ opB.Var("description", "")
+ opB.Var("visibility", models.OrgLinkVisibilityPublic)
+ opB.Var("tags", "isolation-common, isolation-unique-b")
+ opB.Var("slug", "personal-org")
+ opB.Var("unread", false)
+ opB.Var("starred", false)
+ opB.Var("archive", false)
+ err = links.Execute(ctx, opB, &linkB)
+ c.NoError(err)
+ c.NotZero(linkB.Link.ID)
+
+ tagLinksB, err := models.GetTagLinks(dbCtx,
+ &database.FilterOptions{Filter: sq.Expr("org_link_id = ?", linkB.Link.ID)})
+ c.NoError(err)
+ c.Equal(2, len(tagLinksB), "Link B should have 2 tags after creation")
+
+ updateLinkQuery := `mutation UpdateLink($hash: String!, $title: String!,
+ $url: String!, $visibility: LinkVisibility!,
+ $tags: String) {
+ updateLink(input: {
+ title: $title,
+ hash: $hash,
+ url: $url,
+ visibility: $visibility,
+ tags: $tags,
+ }) {id, hash}
+ }`
+
+ var updatedA UpdateLinkResponse
+ opUpdate := gqlclient.NewOperation(updateLinkQuery)
+ opUpdate.Var("hash", linkA.Link.Hash)
+ opUpdate.Var("title", "Link A isolation")
+ opUpdate.Var("url", "https://example.com/link-a-isolation")
+ opUpdate.Var("visibility", models.OrgLinkVisibilityPublic)
+ opUpdate.Var("tags", "isolation-unique-a")
+ err = links.Execute(ctx, opUpdate, &updatedA)
+ c.NoError(err)
+
+ tagLinksAAfter, err := models.GetTagLinks(dbCtx,
+ &database.FilterOptions{Filter: sq.Expr("org_link_id = ?", linkA.Link.ID)})
+ c.NoError(err)
+ c.Equal(1, len(tagLinksAAfter), "Link A should have 1 tag after update")
+
+ tagLinksB, err = models.GetTagLinks(dbCtx,
+ &database.FilterOptions{Filter: sq.Expr("org_link_id = ?", linkB.Link.ID)})
+ c.NoError(err)
+ c.Equal(2, len(tagLinksB), "Link B should still have 2 tags after updating Link A")
+
+ tagIDs := make([]int, 0)
+ for _, tl := range tagLinksB {
+ tagIDs = append(tagIDs, tl.TagID)
+ }
+ tags, err := models.GetTags(dbCtx, &database.FilterOptions{Filter: sq.Eq{"t.id": tagIDs}})
+ c.NoError(err)
+ tagSlugs := make(map[string]bool)
+ for _, tag := range tags {
+ tagSlugs[tag.Slug] = true
+ }
+ c.True(tagSlugs["isolation-common"], "Link B should still have 'isolation-common' tag")
+ c.True(tagSlugs["isolation-unique-b"], "Link B should still have 'isolation-unique-b' tag")
+ })
}
diff --git a/api/graph/schema.resolvers.go b/api/graph/schema.resolvers.go
index 3c28046..74e3a84 100644
--- a/api/graph/schema.resolvers.go
+++ b/api/graph/schema.resolvers.go
@@ -781,7 +781,7 @@ func (r *mutationResolver) UpdateLink(ctx context.Context, input *model.UpdateLi
}
if len(tagIDsToRemove) > 0 {
- err = models.RemoveTagsFromLink(ctx, tagIDsToRemove)
+ err = models.RemoveTagsFromLink(ctx, orgLink.ID, tagIDsToRemove)
if err != nil {
return nil, err
}
@@ -2578,7 +2578,7 @@ func (r *mutationResolver) UpdateLinkShort(ctx context.Context, input *model.Upd
}
if len(tagIDsToRemove) > 0 {
- err = models.RemoveTagsFromLinkShort(ctx, tagIDsToRemove)
+ err = models.RemoveTagsFromLinkShort(ctx, linkShort.ID, tagIDsToRemove)
if err != nil {
return nil, err
}
@@ -3413,7 +3413,7 @@ func (r *mutationResolver) UpdateListing(ctx context.Context, input *model.Updat
}
if len(tagIDsToRemove) > 0 {
- err = models.RemoveTagsFromListing(ctx, tagIDsToRemove)
+ err = models.RemoveTagsFromListing(ctx, listing.ID, tagIDsToRemove)
if err != nil {
return nil, err
}
diff --git a/models/tag_link_shorts.go b/models/tag_link_shorts.go
index 6530864..4b60411 100644
--- a/models/tag_link_shorts.go
+++ b/models/tag_link_shorts.go
@@ -87,14 +87,17 @@ func CreateBatchTagLinkShorts(ctx context.Context, linkShortID int, tagIDs []int
return err
}
-func RemoveTagsFromLinkShort(ctx context.Context, tagIDs []int) error {
+func RemoveTagsFromLinkShort(ctx context.Context, linkShortID int, tagIDs []int) error {
if len(tagIDs) == 0 {
return fmt.Errorf("No Tag IDs were given")
}
err := database.WithTx(ctx, nil, func(tx *sql.Tx) error {
_, err := sq.
Delete("tag_link_shorts").
- Where(sq.Eq{"tag_id": tagIDs}).
+ Where(sq.And{
+ sq.Eq{"link_short_id": linkShortID},
+ sq.Eq{"tag_id": tagIDs},
+ }).
PlaceholderFormat(database.GetPlaceholderFormat()).
RunWith(tx).
ExecContext(ctx)
diff --git a/models/tag_links.go b/models/tag_links.go
index 9c6a634..81c4a1a 100644
--- a/models/tag_links.go
+++ b/models/tag_links.go
@@ -83,14 +83,17 @@ func CreateBatchTagLinks(ctx context.Context, linkID int, tagIDs []int) error {
return err
}
-func RemoveTagsFromLink(ctx context.Context, tagIDs []int) error {
+func RemoveTagsFromLink(ctx context.Context, linkID int, tagIDs []int) error {
if len(tagIDs) == 0 {
return fmt.Errorf("No Tag IDs were given")
}
err := database.WithTx(ctx, nil, func(tx *sql.Tx) error {
_, err := sq.
Delete("tag_links").
- Where(sq.Eq{"tag_id": tagIDs}).
+ Where(sq.And{
+ sq.Eq{"org_link_id": linkID},
+ sq.Eq{"tag_id": tagIDs},
+ }).
PlaceholderFormat(database.GetPlaceholderFormat()).
RunWith(tx).
ExecContext(ctx)
diff --git a/models/tag_listing.go b/models/tag_listing.go
index a0f05b3..df89dc0 100644
--- a/models/tag_listing.go
+++ b/models/tag_listing.go
@@ -87,14 +87,17 @@ func CreateBatchTagListings(ctx context.Context, linkShortID int, tagIDs []int)
return err
}
-func RemoveTagsFromListing(ctx context.Context, tagIDs []int) error {
+func RemoveTagsFromListing(ctx context.Context, listingID int, tagIDs []int) error {
if len(tagIDs) == 0 {
return fmt.Errorf("No Tag IDs were given")
}
err := database.WithTx(ctx, nil, func(tx *sql.Tx) error {
_, err := sq.
Delete("tag_listings").
- Where(sq.Eq{"tag_id": tagIDs}).
+ Where(sq.And{
+ sq.Eq{"listing_id": listingID},
+ sq.Eq{"tag_id": tagIDs},
+ }).
PlaceholderFormat(database.GetPlaceholderFormat()).
RunWith(tx).
ExecContext(ctx)
--
2.49.1