REVIEW: Support merging tags through the API

.pr-review/BODY.md

@@ -0,0 +1,58 @@
+# Support merging tags through the API
+
+Tag merge has been available in the web UI since v1.43.0 (#1175), but the tags API only
+supports list, retrieve, create, and delete. A scripted consolidation therefore has to
+hand-roll it: PATCH every affected bookmark's `tag_names` one at a time, then delete the
+tag that is left behind. That is not atomic, and it leaves an orphaned tag if the caller
+stops halfway or forgets the cleanup — which the web UI's merge never does.
+
+This exposes the existing merge as a detail action on the target tag, in the same spirit as
+#1411 exposing tag delete.
+
+```
+POST /api/tags/<id>/merge/
+
+{ "merge_tag_ids": [2, 3] }
+```
+
+Returns `204 No Content`.
+
+## Design decisions
+
+**Detail action on the target, not `POST /api/tags/merge/`.** The tags API addresses tags by
+ID everywhere else, so the target belongs in the path like it does for retrieve and delete.
+It also follows the convention the bookmarks API already sets for non-CRUD operations on a
+single resource — `POST /api/bookmarks/<id>/archive/` — rather than introducing a
+collection-level pseudo-resource. The practical benefit is that the target goes through
+`get_object()`, so a tag belonging to another user 404s through exactly the same mechanism
+as retrieve and delete, with nothing new to get wrong.
+
+**Merge tags are named by ID.** Mixing an ID in the path with names in the body would be
+incoherent, and IDs avoid the question of what a name that does not resolve should mean.
+`TagMergeForm` matches names case-insensitively because it is fed by the tag autocomplete;
+an API client that has names already has the IDs from `GET /api/tags/`.
+
+**Unknown or foreign merge tag IDs are a `400`, not a `404`.** The addressed resource — the
+target tag — does exist; the payload is what is invalid, and the response names the offending
+IDs. Merge tags are resolved through `get_queryset()`, so another user's tag is rejected as
+non-existent rather than merged, and ownership has one enforcement point rather than two.
+
+**`204 No Content`,** matching `archive`/`unarchive` and tag delete. Nothing about the target
+tag's own representation changes. Returning a count of retagged bookmarks would be useful for
+tooling — happy to add that if you would prefer it, but it seemed out of scope for parity with
+the web UI.
+
+## Shared transaction
+
+The `transaction.atomic()` block moves out of `bookmarks/views/tags.py` into
+`bookmarks.services.tags.merge_tags()` unchanged — same queries, same order — so the view and
+the API action cannot drift. The view keeps its success message and its existing tests, which
+pass untouched.
+
+## Tests
+
+Seven tests in `bookmarks/tests/test_tags_api.py`, covering the merge itself, the
+`exclude(tags=target_tag)` branch where a bookmark already carries the target tag, a merge tag
+that does not exist, the target listed among the merge tags, a missing/empty `merge_tag_ids`,
+and both cross-user directions (foreign target 404s, foreign merge tag 400s and is left
+untouched).

bookmarks/api/routes.py

@@ -6,6 +6,7 @@
 from django.http import Http404, StreamingHttpResponse
 from rest_framework import mixins, status, viewsets
 from rest_framework.decorators import action
+from rest_framework.exceptions import ValidationError
 from rest_framework.permissions import AllowAny
 from rest_framework.response import Response
 from rest_framework.routers import DefaultRouter, SimpleRouter
@@ -15,6 +16,7 @@
     BookmarkAssetSerializer,
     BookmarkBundleSerializer,
     BookmarkSerializer,
+    TagMergeSerializer,
     TagSerializer,
     UserProfileSerializer,
 )
@@ -26,7 +28,14 @@
     Tag,
     User,
 )
-from bookmarks.services import assets, auto_tagging, bookmarks, bundles, website_loader
+from bookmarks.services import (
+    assets,
+    auto_tagging,
+    bookmarks,
+    bundles,
+    tags,
+    website_loader,
+)
 from bookmarks.type_defs import HttpRequest
 from bookmarks.views import access
 
@@ -263,6 +272,37 @@ def get_queryset(self):
     def get_serializer_context(self):
         return {"user": self.request.user}
 
+    @action(methods=["post"], detail=True)
+    def merge(self, request: HttpRequest, pk):
+        target_tag = self.get_object()
+
+        serializer = TagMergeSerializer(data=request.data)
+        serializer.is_valid(raise_exception=True)
+        merge_tag_ids = set(serializer.validated_data["merge_tag_ids"])
+
+        if target_tag.id in merge_tag_ids:
+            raise ValidationError(
+                {"merge_tag_ids": ["The target tag cannot be selected for merging."]}
+            )
+
+        # Resolve through the queryset so that tags of other users are rejected
+        # the same way as tags that do not exist
+        merge_tags = list(self.get_queryset().filter(id__in=merge_tag_ids))
+        missing_tag_ids = merge_tag_ids - {tag.id for tag in merge_tags}
+        if missing_tag_ids:
+            raise ValidationError(
+                {
+                    "merge_tag_ids": [
+                        f"Tag with ID {tag_id} does not exist."
+                        for tag_id in sorted(missing_tag_ids)
+                    ]
+                }
+            )
+
+        tags.merge_tags(target_tag, merge_tags)
+
+        return Response(status=status.HTTP_204_NO_CONTENT)
+
 
 class UserViewSet(viewsets.GenericViewSet):
     @action(methods=["get"], detail=False)

bookmarks/api/serializers.py

@@ -203,6 +203,12 @@ def create(self, validated_data):
         return get_or_create_tag(validated_data["name"], self.context["user"])
 
 
+class TagMergeSerializer(serializers.Serializer):
+    merge_tag_ids = serializers.ListField(
+        child=serializers.IntegerField(), allow_empty=False
+    )
+
+
 class UserProfileSerializer(serializers.ModelSerializer):
     class Meta:
         model = UserProfile

bookmarks/services/tags.py

@@ -2,9 +2,10 @@
 import operator
 
 from django.contrib.auth.models import User
+from django.db import transaction
 from django.utils import timezone
 
-from bookmarks.models import Tag
+from bookmarks.models import Bookmark, Tag
 from bookmarks.utils import unique
 
 logger = logging.getLogger(__name__)
@@ -34,3 +35,33 @@ def get_or_create_tag(name: str, user: User):
         )
         logger.error(message)
         return first_tag
+
+
+def merge_tags(target_tag: Tag, merge_tags: list[Tag]):
+    with transaction.atomic():
+        BookmarkTag = Bookmark.tags.through
+
+        # Get all bookmarks that have any of the merge tags, but do not
+        # already have the target tag
+        bookmark_ids = list(
+            Bookmark.objects.filter(tags__in=merge_tags)
+            .exclude(tags=target_tag)
+            .values_list("id", flat=True)
+            .distinct()
+        )
+
+        # Create new relationships to the target tag
+        new_relationships = [
+            BookmarkTag(tag_id=target_tag.id, bookmark_id=bookmark_id)
+            for bookmark_id in bookmark_ids
+        ]
+
+        if new_relationships:
+            BookmarkTag.objects.bulk_create(new_relationships)
+
+        # Bulk delete all relationships for merge tags
+        merge_tag_ids = [tag.id for tag in merge_tags]
+        BookmarkTag.objects.filter(tag_id__in=merge_tag_ids).delete()
+
+        # Delete the merged tags
+        Tag.objects.filter(id__in=merge_tag_ids).delete()

bookmarks/tests/test_tags_api.py

@@ -39,3 +39,133 @@ def test_can_not_delete_tag_of_other_user(self):
         self.delete(url, expected_status_code=status.HTTP_404_NOT_FOUND)
 
         self.assertTrue(Tag.objects.filter(id=tag.id).exists())
+
+    def test_merge_tags(self):
+        self.authenticate()
+
+        target_tag = self.setup_tag(name="target_tag")
+        merge_tag1 = self.setup_tag(name="merge_tag1")
+        merge_tag2 = self.setup_tag(name="merge_tag2")
+        other_tag = self.setup_tag(name="other_tag")
+
+        bookmark1 = self.setup_bookmark(tags=[merge_tag1])
+        bookmark2 = self.setup_bookmark(tags=[merge_tag2])
+        bookmark3 = self.setup_bookmark(tags=[merge_tag1, merge_tag2])
+        bookmark4 = self.setup_bookmark(tags=[merge_tag1, other_tag])
+        untouched_bookmark = self.setup_bookmark(tags=[other_tag])
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": [merge_tag1.id, merge_tag2.id]},
+            expected_status_code=status.HTTP_204_NO_CONTENT,
+        )
+
+        self.assertTrue(Tag.objects.filter(id=target_tag.id).exists())
+        self.assertFalse(Tag.objects.filter(id=merge_tag1.id).exists())
+        self.assertFalse(Tag.objects.filter(id=merge_tag2.id).exists())
+
+        self.assertListEqual(list(bookmark1.tags.all()), [target_tag])
+        self.assertListEqual(list(bookmark2.tags.all()), [target_tag])
+        self.assertListEqual(list(bookmark3.tags.all()), [target_tag])
+        self.assertCountEqual(list(bookmark4.tags.all()), [target_tag, other_tag])
+        self.assertListEqual(list(untouched_bookmark.tags.all()), [other_tag])
+
+    def test_merge_tags_keeps_single_relationship_when_bookmark_has_target_tag(self):
+        self.authenticate()
+
+        target_tag = self.setup_tag(name="target_tag")
+        merge_tag = self.setup_tag(name="merge_tag")
+
+        bookmark = self.setup_bookmark(tags=[merge_tag, target_tag])
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": [merge_tag.id]},
+            expected_status_code=status.HTTP_204_NO_CONTENT,
+        )
+
+        self.assertFalse(Tag.objects.filter(id=merge_tag.id).exists())
+        self.assertListEqual(list(bookmark.tags.all()), [target_tag])
+
+    def test_merge_tags_requires_merge_tag_ids(self):
+        self.authenticate()
+
+        target_tag = self.setup_tag(name="target_tag")
+        merge_tag = self.setup_tag(name="merge_tag")
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {},
+            expected_status_code=status.HTTP_400_BAD_REQUEST,
+        )
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": []},
+            expected_status_code=status.HTTP_400_BAD_REQUEST,
+        )
+
+        self.assertTrue(Tag.objects.filter(id=merge_tag.id).exists())
+
+    def test_can_not_merge_target_tag_into_itself(self):
+        self.authenticate()
+
+        target_tag = self.setup_tag(name="target_tag")
+        merge_tag = self.setup_tag(name="merge_tag")
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": [merge_tag.id, target_tag.id]},
+            expected_status_code=status.HTTP_400_BAD_REQUEST,
+        )
+
+        self.assertTrue(Tag.objects.filter(id=target_tag.id).exists())
+        self.assertTrue(Tag.objects.filter(id=merge_tag.id).exists())
+
+    def test_can_not_merge_tag_that_does_not_exist(self):
+        self.authenticate()
+
+        target_tag = self.setup_tag(name="target_tag")
+        merge_tag = self.setup_tag(name="merge_tag")
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": [merge_tag.id, merge_tag.id + 1000]},
+            expected_status_code=status.HTTP_400_BAD_REQUEST,
+        )
+
+        self.assertTrue(Tag.objects.filter(id=merge_tag.id).exists())
+
+    def test_can_not_merge_into_tag_of_other_user(self):
+        self.authenticate()
+
+        other_user = self.setup_user()
+        target_tag = self.setup_tag(name="target_tag", user=other_user)
+        merge_tag = self.setup_tag(name="merge_tag")
+        bookmark = self.setup_bookmark(tags=[merge_tag])
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": [merge_tag.id]},
+            expected_status_code=status.HTTP_404_NOT_FOUND,
+        )
+
+        self.assertTrue(Tag.objects.filter(id=target_tag.id).exists())
+        self.assertTrue(Tag.objects.filter(id=merge_tag.id).exists())
+        self.assertListEqual(list(bookmark.tags.all()), [merge_tag])
+
+    def test_can_not_merge_tag_of_other_user(self):
+        self.authenticate()
+
+        other_user = self.setup_user()
+        target_tag = self.setup_tag(name="target_tag")
+        merge_tag = self.setup_tag(name="merge_tag", user=other_user)
+        bookmark = self.setup_bookmark(tags=[merge_tag], user=other_user)
+
+        self.post(
+            reverse("linkding:tag-merge", kwargs={"pk": target_tag.id}),
+            {"merge_tag_ids": [merge_tag.id]},
+            expected_status_code=status.HTTP_400_BAD_REQUEST,
+        )
+
+        self.assertTrue(Tag.objects.filter(id=merge_tag.id).exists())
+        self.assertListEqual(list(bookmark.tags.all()), [merge_tag])

bookmarks/views/tags.py

@@ -1,14 +1,14 @@
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required
 from django.core.paginator import Paginator
-from django.db import transaction
 from django.db.models import Count
 from django.http import HttpResponseRedirect
 from django.shortcuts import get_object_or_404, render
 from django.urls import reverse
 
 from bookmarks.forms import TagForm, TagMergeForm
-from bookmarks.models import Bookmark, Tag
+from bookmarks.models import Tag
+from bookmarks.services import tags
 from bookmarks.type_defs import HttpRequest
 from bookmarks.utils import redirect_with_query
 from bookmarks.views import turbo
@@ -118,40 +118,14 @@ def tag_merge(request: HttpRequest):
         if form.is_valid():
             target_tag = form.cleaned_data["target_tag"]
             merge_tags = form.cleaned_data["merge_tags"]
+            tag_names = [tag.name for tag in merge_tags]
 
-            with transaction.atomic():
-                BookmarkTag = Bookmark.tags.through
+            tags.merge_tags(target_tag, merge_tags)
 
-                # Get all bookmarks that have any of the merge tags, but do not
-                # already have the target tag
-                bookmark_ids = list(
-                    Bookmark.objects.filter(tags__in=merge_tags)
-                    .exclude(tags=target_tag)
-                    .values_list("id", flat=True)
-                    .distinct()
-                )
-
-                # Create new relationships to the target tag
-                new_relationships = [
-                    BookmarkTag(tag_id=target_tag.id, bookmark_id=bookmark_id)
-                    for bookmark_id in bookmark_ids
-                ]
-
-                if new_relationships:
-                    BookmarkTag.objects.bulk_create(new_relationships)
-
-                # Bulk delete all relationships for merge tags
-                merge_tag_ids = [tag.id for tag in merge_tags]
-                BookmarkTag.objects.filter(tag_id__in=merge_tag_ids).delete()
-
-                # Delete the merged tags
-                tag_names = [tag.name for tag in merge_tags]
-                Tag.objects.filter(id__in=merge_tag_ids).delete()
-
-                messages.success(
-                    request,
-                    f'Successfully merged {len(merge_tags)} tags ({", ".join(tag_names)}) into "{target_tag.name}".',
-                )
+            messages.success(
+                request,
+                f'Successfully merged {len(merge_tags)} tags ({", ".join(tag_names)}) into "{target_tag.name}".',
+            )
 
             return HttpResponseRedirect(reverse("linkding:tags.index"))
         else: