Prevent duplicates when editing

bookmarks/api/serializers.py

@@ -111,6 +111,24 @@ def update(self, instance: Bookmark, validated_data):
 
         return update_bookmark(instance, tag_string, self.context["user"])
 
+    def validate(self, attrs):
+        # When creating a bookmark, the service logic prevents duplicate URLs by
+        # updating the existing bookmark instead. When editing a bookmark,
+        # there is no assumption that it would update a different bookmark if
+        # the URL is a duplicate, so raise a validation error in that case.
+        if self.instance and "url" in attrs:
+            is_duplicate = (
+                Bookmark.objects.filter(owner=self.instance.owner, url=attrs["url"])
+                .exclude(pk=self.instance.pk)
+                .exists()
+            )
+            if is_duplicate:
+                raise serializers.ValidationError(
+                    {"url": "A bookmark with this URL already exists."}
+                )
+
+        return attrs
+
 
 class TagSerializer(serializers.ModelSerializer):
     class Meta:

bookmarks/models.py

@@ -168,21 +168,23 @@ def has_notes(self):
             self.instance and self.instance.notes
         )
 
-    def clean(self):
-        cleaned_data = super().clean()
-        url = cleaned_data.get("url")
-
-        if self.instance.pk and url:
-            # Ensure there is no existing Bookmark with the same URL
-            existing_bookmark = (
-                Bookmark.objects.filter(url=url, owner=self.instance.owner)
+    def clean_url(self):
+        # When creating a bookmark, the service logic prevents duplicate URLs by
+        # updating the existing bookmark instead, which is also communicated in
+        # the form's UI. When editing a bookmark, there is no assumption that
+        # it would update a different bookmark if the URL is a duplicate, so
+        # raise a validation error in that case.
+        url = self.cleaned_data["url"]
+        if self.instance.pk:
+            is_duplicate = (
+                Bookmark.objects.filter(owner=self.instance.owner, url=url)
                 .exclude(pk=self.instance.pk)
-                .first()
+                .exists()
             )
-            if existing_bookmark:
-                self.add_error("url", "A bookmark with this URL already exists.")
+            if is_duplicate:
+                raise forms.ValidationError("A bookmark with this URL already exists.")
 
-        return cleaned_data
+        return url
 
 
 class BookmarkSearch:

bookmarks/tests/test_bookmark_edit_view.py

@@ -141,17 +141,32 @@ def test_should_prefill_bookmark_form_fields(self):
             html,
         )
 
-    def test_should_prevent_changing_url_to_existing_url_for_same_owner(self):
+    def test_should_prevent_duplicate_urls(self):
         edited_bookmark = self.setup_bookmark(url="http://example.com/edited")
         existing_bookmark = self.setup_bookmark(url="http://example.com/existing")
+        other_user_bookmark = self.setup_bookmark(
+            url="http://example.com/other-user", user=User.objects.create_user("other")
+        )
 
-        form_data = self.create_form_data(
-            {"id": edited_bookmark.id, "url": existing_bookmark.url}
+        # if the URL isn't modified it's not a duplicate
+        form_data = self.create_form_data({"url": edited_bookmark.url})
+        response = self.client.post(
+            reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data
         )
+        self.assertEqual(response.status_code, 302)
+
+        # if the URL is already bookmarked by another user, it's not a duplicate
+        form_data = self.create_form_data({"url": other_user_bookmark.url})
         response = self.client.post(
             reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data
         )
+        self.assertEqual(response.status_code, 302)
 
+        # if the URL is already bookmarked by the same user, it's a duplicate
+        form_data = self.create_form_data({"url": existing_bookmark.url})
+        response = self.client.post(
+            reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data
+        )
         self.assertEqual(response.status_code, 422)
         self.assertInHTML(
             "<li>A bookmark with this URL already exists.</li>",
@@ -160,23 +175,6 @@ def test_should_prevent_changing_url_to_existing_url_for_same_owner(self):
         edited_bookmark.refresh_from_db()
         self.assertNotEqual(edited_bookmark.url, existing_bookmark.url)
 
-    def test_should_allow_changing_url_to_existing_url_for_different_owner(self):
-        edited_bookmark = self.setup_bookmark(url="http://example.com/edited")
-        existing_bookmark = self.setup_bookmark(
-            url="http://example.com/existing", user=User.objects.create_user("other")
-        )
-
-        form_data = self.create_form_data(
-            {"id": edited_bookmark.id, "url": existing_bookmark.url}
-        )
-        response = self.client.post(
-            reverse("bookmarks:edit", args=[edited_bookmark.id]), form_data
-        )
-
-        self.assertEqual(response.status_code, 302)
-        edited_bookmark.refresh_from_db()
-        self.assertEqual(edited_bookmark.url, existing_bookmark.url)
-
     def test_should_redirect_to_return_url(self):
         bookmark = self.setup_bookmark()
         form_data = self.create_form_data()

bookmarks/tests/test_bookmarks_api.py

@@ -685,6 +685,29 @@ def test_update_bookmark_adds_tags_from_auto_tagging(self):
         updated_bookmark = Bookmark.objects.get(id=bookmark.id)
         self.assertCountEqual(updated_bookmark.tags.all(), [tag1, tag2])
 
+    def test_update_bookmark_should_prevent_duplicate_urls(self):
+        self.authenticate()
+        edited_bookmark = self.setup_bookmark(url="https://example.com/edited")
+        existing_bookmark = self.setup_bookmark(url="https://example.com/existing")
+        other_user_bookmark = self.setup_bookmark(
+            url="https://example.com/other", user=self.setup_user()
+        )
+
+        # if the URL isn't modified it's not a duplicate
+        data = {"url": edited_bookmark.url}
+        url = reverse("bookmarks:bookmark-detail", args=[edited_bookmark.id])
+        self.put(url, data, expected_status_code=status.HTTP_200_OK)
+
+        # if the URL is already bookmarked by another user, it's not a duplicate
+        data = {"url": other_user_bookmark.url}
+        url = reverse("bookmarks:bookmark-detail", args=[edited_bookmark.id])
+        self.put(url, data, expected_status_code=status.HTTP_200_OK)
+
+        # if the URL is already bookmarked by the same user, it's a duplicate
+        data = {"url": existing_bookmark.url}
+        url = reverse("bookmarks:bookmark-detail", args=[edited_bookmark.id])
+        self.put(url, data, expected_status_code=status.HTTP_400_BAD_REQUEST)
+
     def test_patch_bookmark(self):
         self.authenticate()
         bookmark = self.setup_bookmark()

docs/src/content/docs/api.md

@@ -127,11 +127,9 @@ POST /api/bookmarks/
 Creates a new bookmark. Tags are simply assigned using their names. Including
 `is_archived: true` saves a bookmark directly to the archive.
 
-If the title and description are not provided or empty, the application automatically tries to scrape them from the bookmarked website. This behavior can be disabled by adding the `disable_scraping` query parameter to the API request. If you have an application where you want to keep using scraped metadata, but also allow users to leave the title or description empty, you should:
+If the provided URL is already bookmarked, this silently updates the existing bookmark instead of creating a new one. If you are implementing a user interface, consider notifying users about this behavior. You can use the `/check` endpoint to check if a URL is already bookmarked and at the same time get the existing bookmark data. This behavior may change in the future to return an error instead.
 
-- Fetch the scraped title and description using the `/check` endpoint.
-- Prefill the title and description fields in your app with the fetched values and allow users to clear those values.
-- Add the `disable_scraping` query parameter to prevent the API from adding them back again.
+If the title and description are not provided or empty, the application automatically tries to scrape them from the bookmarked website. This behavior can be disabled by adding the `disable_scraping` query parameter to the API request.
 
 Example payload:
 
@@ -164,6 +162,8 @@ When using `PATCH`, only the fields that should be updated need to be provided.
 Regardless which method is used, any field that is not provided is not modified.
 Tags are simply assigned using their names.
 
+If the provided URL is already bookmarked this returns an error.
+
 Example payload:
 
 ```json