Peter Sanchez: 1 Fixing bug where removing a tag will remove it across multiple bookmarks, listings and short links. 5 files changed, 131 insertions(+), 9 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.code.netlandish.com/~netlandish/links-dev/patches/200/mbox | git am -3Learn more about email & git
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
Applied. To git@git.code.netlandish.com:~netlandish/links c334752..babd2b9 master -> master