Do not clear fields in POST requests (API behavior change) (#852)

bookmarks/api/serializers.py

@@ -49,6 +49,7 @@ class BookmarkSerializer(serializers.ModelSerializer):
             "web_archive_snapshot_url",
             "favicon_url",
             "preview_image_url",
+            "tag_names",
             "date_added",
             "date_modified",
             "website_title",
@@ -56,15 +57,9 @@ class BookmarkSerializer(serializers.ModelSerializer):
         ]
         list_serializer_class = BookmarkListSerializer
 
-    # Override optional char fields to provide default value
+    # Custom tag_names field to allow passing a list of tag names to create/update
-    title = serializers.CharField(required=False, allow_blank=True, default="")
+    tag_names = TagListField(required=False)
-    description = serializers.CharField(required=False, allow_blank=True, default="")
+    # Custom fields to return URLs for favicon and preview image
-    notes = serializers.CharField(required=False, allow_blank=True, default="")
-    is_archived = serializers.BooleanField(required=False, default=False)
-    unread = serializers.BooleanField(required=False, default=False)
-    shared = serializers.BooleanField(required=False, default=False)
-    # Override readonly tag_names property to allow passing a list of tag names to create/update
-    tag_names = TagListField(required=False, default=[])
     favicon_url = serializers.SerializerMethodField()
     preview_image_url = serializers.SerializerMethodField()
     # Add dummy website title and description fields for backwards compatibility but keep them empty
@@ -94,15 +89,9 @@ class BookmarkSerializer(serializers.ModelSerializer):
         return None
 
     def create(self, validated_data):
-        bookmark = Bookmark()
+        tag_names = validated_data.pop("tag_names", [])
-        bookmark.url = validated_data["url"]
+        tag_string = build_tag_string(tag_names)
-        bookmark.title = validated_data["title"]
+        bookmark = Bookmark(**validated_data)
-        bookmark.description = validated_data["description"]
-        bookmark.notes = validated_data["notes"]
-        bookmark.is_archived = validated_data["is_archived"]
-        bookmark.unread = validated_data["unread"]
-        bookmark.shared = validated_data["shared"]
-        tag_string = build_tag_string(validated_data["tag_names"])
 
         saved_bookmark = create_bookmark(bookmark, tag_string, self.context["user"])
         # Unless scraping is explicitly disabled, enhance bookmark with website
@@ -113,15 +102,12 @@ class BookmarkSerializer(serializers.ModelSerializer):
         return saved_bookmark
 
     def update(self, instance: Bookmark, validated_data):
-        # Update fields if they were provided in the payload
+        tag_names = validated_data.pop("tag_names", instance.tag_names)
-        for key in ["url", "title", "description", "notes", "unread", "shared"]:
+        tag_string = build_tag_string(tag_names)
-            if key in validated_data:
-                setattr(instance, key, validated_data[key])
 
-        # Use tag string from payload, or use bookmark's current tags as fallback
+        for field_name, field in self.fields.items():
-        tag_string = build_tag_string(instance.tag_names)
+            if not field.read_only and field_name in validated_data:
-        if "tag_names" in validated_data:
+                setattr(instance, field_name, validated_data[field_name])
-            tag_string = build_tag_string(validated_data["tag_names"])
 
         return update_bookmark(instance, tag_string, self.context["user"])
 

bookmarks/services/bookmarks.py

@@ -65,7 +65,6 @@ def update_bookmark(bookmark: Bookmark, tag_string, current_user: User):
     if has_url_changed:
         # Update web archive snapshot, if URL changed
         tasks.create_web_archive_snapshot(current_user, bookmark, True)
-        bookmark.save()
 
     return bookmark
 

bookmarks/tests/test_bookmarks_api.py

@@ -480,7 +480,21 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
         self.authenticate()
 
         data = {"url": "https://example.com/"}
-        self.post(reverse("bookmarks:bookmark-list"), data, status.HTTP_201_CREATED)
+        self.post(
+            reverse("bookmarks:bookmark-list") + "?disable_scraping",
+            data,
+            status.HTTP_201_CREATED,
+        )
+
+        bookmark = Bookmark.objects.get(url=data["url"])
+        self.assertEqual(data["url"], bookmark.url)
+        self.assertEqual("", bookmark.title)
+        self.assertEqual("", bookmark.description)
+        self.assertEqual("", bookmark.notes)
+        self.assertFalse(bookmark.is_archived)
+        self.assertFalse(bookmark.unread)
+        self.assertFalse(bookmark.shared)
+        self.assertBookmarkListEqual([], bookmark.tag_names)
 
     def test_create_archived_bookmark(self):
         self.authenticate()
@@ -586,6 +600,28 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
         updated_bookmark = Bookmark.objects.get(id=bookmark.id)
         self.assertEqual(updated_bookmark.url, data["url"])
 
+    def test_update_bookmark_ignores_readonly_fields(self):
+        self.authenticate()
+        bookmark = self.setup_bookmark()
+
+        data = {
+            "url": "https://example.com/updated",
+            "web_archive_snapshot_url": "test",
+            "website_title": "test",
+            "website_description": "test",
+        }
+        url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
+        self.put(url, data, expected_status_code=status.HTTP_200_OK)
+        updated_bookmark = Bookmark.objects.get(id=bookmark.id)
+        self.assertEqual(data["url"], updated_bookmark.url)
+        self.assertNotEqual(
+            data["web_archive_snapshot_url"], updated_bookmark.web_archive_snapshot_url
+        )
+        self.assertNotEqual(data["website_title"], updated_bookmark.website_title)
+        self.assertNotEqual(
+            data["website_description"], updated_bookmark.website_description
+        )
+
     def test_update_bookmark_fails_without_required_fields(self):
         self.authenticate()
         bookmark = self.setup_bookmark()
@@ -594,19 +630,24 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
         url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
         self.put(url, data, expected_status_code=status.HTTP_400_BAD_REQUEST)
 
-    def test_update_bookmark_with_minimal_payload_clears_all_fields(self):
+    def test_update_bookmark_with_minimal_payload_does_not_modify_bookmark(self):
         self.authenticate()
-        bookmark = self.setup_bookmark()
+        bookmark = self.setup_bookmark(
+            is_archived=True, unread=True, shared=True, tags=[self.setup_tag()]
+        )
 
         data = {"url": "https://example.com/"}
         url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
         self.put(url, data, expected_status_code=status.HTTP_200_OK)
         updated_bookmark = Bookmark.objects.get(id=bookmark.id)
         self.assertEqual(updated_bookmark.url, data["url"])
-        self.assertEqual(updated_bookmark.title, "")
+        self.assertEqual(updated_bookmark.title, bookmark.title)
-        self.assertEqual(updated_bookmark.description, "")
+        self.assertEqual(updated_bookmark.description, bookmark.description)
-        self.assertEqual(updated_bookmark.notes, "")
+        self.assertEqual(updated_bookmark.notes, bookmark.notes)
-        self.assertEqual(updated_bookmark.tag_names, [])
+        self.assertEqual(updated_bookmark.is_archived, bookmark.is_archived)
+        self.assertEqual(updated_bookmark.unread, bookmark.unread)
+        self.assertEqual(updated_bookmark.shared, bookmark.shared)
+        self.assertListEqual(updated_bookmark.tag_names, bookmark.tag_names)
 
     def test_update_bookmark_unread_flag(self):
         self.authenticate()
@@ -703,16 +744,42 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
         tag_names = [tag.name for tag in bookmark.tags.all()]
         self.assertListEqual(tag_names, ["updated-tag-1", "updated-tag-2"])
 
-    def test_patch_with_empty_payload_does_not_modify_bookmark(self):
+    def test_patch_ignores_readonly_fields(self):
         self.authenticate()
         bookmark = self.setup_bookmark()
 
+        data = {
+            "web_archive_snapshot_url": "test",
+            "website_title": "test",
+            "website_description": "test",
+        }
+        url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
+        self.patch(url, data, expected_status_code=status.HTTP_200_OK)
+        updated_bookmark = Bookmark.objects.get(id=bookmark.id)
+        self.assertNotEqual(
+            data["web_archive_snapshot_url"], updated_bookmark.web_archive_snapshot_url
+        )
+        self.assertNotEqual(data["website_title"], updated_bookmark.website_title)
+        self.assertNotEqual(
+            data["website_description"], updated_bookmark.website_description
+        )
+
+    def test_patch_with_empty_payload_does_not_modify_bookmark(self):
+        self.authenticate()
+        bookmark = self.setup_bookmark(
+            is_archived=True, unread=True, shared=True, tags=[self.setup_tag()]
+        )
+
         url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
         self.patch(url, {}, expected_status_code=status.HTTP_200_OK)
         updated_bookmark = Bookmark.objects.get(id=bookmark.id)
         self.assertEqual(updated_bookmark.url, bookmark.url)
         self.assertEqual(updated_bookmark.title, bookmark.title)
         self.assertEqual(updated_bookmark.description, bookmark.description)
+        self.assertEqual(updated_bookmark.notes, bookmark.notes)
+        self.assertEqual(updated_bookmark.is_archived, bookmark.is_archived)
+        self.assertEqual(updated_bookmark.unread, bookmark.unread)
+        self.assertEqual(updated_bookmark.shared, bookmark.shared)
         self.assertListEqual(updated_bookmark.tag_names, bookmark.tag_names)
 
     def test_patch_bookmark_adds_tags_from_auto_tagging(self):
@@ -919,6 +986,7 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
             {url: "https://example.com/"},
             expected_status_code=status.HTTP_404_NOT_FOUND,
         )
+        self.patch(url, expected_status_code=status.HTTP_404_NOT_FOUND)
 
         url = reverse(
             "bookmarks:bookmark-detail", args=[inaccessible_shared_bookmark.id]
@@ -928,6 +996,7 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
             {url: "https://example.com/"},
             expected_status_code=status.HTTP_404_NOT_FOUND,
         )
+        self.patch(url, expected_status_code=status.HTTP_404_NOT_FOUND)
 
         url = reverse("bookmarks:bookmark-detail", args=[inaccessible_bookmark.id])
         self.delete(url, expected_status_code=status.HTTP_404_NOT_FOUND)

bookmarks/tests/test_bookmarks_api_permissions.py

@@ -87,6 +87,16 @@ class BookmarksApiPermissionsTestCase(LinkdingApiTestCase, BookmarkFactoryMixin)
         self.authenticate()
         self.put(url, data, expected_status_code=status.HTTP_200_OK)
 
+    def test_update_bookmark_only_updates_own_bookmarks(self):
+        self.authenticate()
+
+        other_user = self.setup_user()
+        bookmark = self.setup_bookmark(user=other_user)
+        data = {"url": "https://example.com/"}
+        url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
+
+        self.put(url, data, expected_status_code=status.HTTP_404_NOT_FOUND)
+
     def test_patch_bookmark_requires_authentication(self):
         bookmark = self.setup_bookmark()
         data = {"url": "https://example.com"}
@@ -97,6 +107,16 @@ class BookmarksApiPermissionsTestCase(LinkdingApiTestCase, BookmarkFactoryMixin)
         self.authenticate()
         self.patch(url, data, expected_status_code=status.HTTP_200_OK)
 
+    def test_patch_bookmark_only_updates_own_bookmarks(self):
+        self.authenticate()
+
+        other_user = self.setup_user()
+        bookmark = self.setup_bookmark(user=other_user)
+        data = {"url": "https://example.com"}
+        url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])
+
+        self.patch(url, data, expected_status_code=status.HTTP_404_NOT_FOUND)
+
     def test_delete_bookmark_requires_authentication(self):
         bookmark = self.setup_bookmark()
         url = reverse("bookmarks:bookmark-detail", args=[bookmark.id])

docs/src/content/docs/api.md

@@ -155,34 +155,13 @@ Example payload:
 
 ```
 PUT /api/bookmarks/<id>/
-```
-
-Updates a bookmark.
-This is a full update, which requires at least a URL, and fields that are not specified are cleared or reset to their defaults.
-Tags are simply assigned using their names.
-
-Example payload:
-
-```json
-{
-  "url": "https://example.com",
-  "title": "Example title",
-  "description": "Example description",
-  "tag_names": [
-    "tag1",
-    "tag2"
-  ]
-}
-```
-
-**Patch**
-
-```
 PATCH /api/bookmarks/<id>/
 ```
 
-Updates a bookmark partially.
+Updates a bookmark.
-Allows to modify individual fields of a bookmark.
+When using `POST`, at least all required fields must be provided (currently only `url`).
+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.
 
 Example payload: