Repository: sissbruecker/linkding
Pull request #852 (2024-09-24 09:32:21)

Do not clear fields in POST requests (API behavior change)
Previously, `POST` requests to the API would reset fields that are not provided in the request to their default values. While documented, the behavior was sort of accidental and potentially dangerous when new fields get added. Thus a small behavior change to not modify fields that are not part of the request.


Patch:
diff --git a/bookmarks/api/serializers.py b/bookmarks/api/serializers.py
index 88efa26..37a7bb0 100644
--- a/bookmarks/api/serializers.py
+++ b/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
-    title = serializers.CharField(required=False, allow_blank=True, default="")
-    description = serializers.CharField(required=False, allow_blank=True, default="")
-    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=[])
+    # Custom tag_names field to allow passing a list of tag names to create/update
+    tag_names = TagListField(required=False)
+    # Custom fields to return URLs for favicon and preview image
     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()
-        bookmark.url = validated_data["url"]
-        bookmark.title = validated_data["title"]
-        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"])
+        tag_names = validated_data.pop("tag_names", [])
+        tag_string = build_tag_string(tag_names)
+        bookmark = Bookmark(**validated_data)
 
         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
-        for key in ["url", "title", "description", "notes", "unread", "shared"]:
-            if key in validated_data:
-                setattr(instance, key, validated_data[key])
-
-        # Use tag string from payload, or use bookmark's current tags as fallback
-        tag_string = build_tag_string(instance.tag_names)
-        if "tag_names" in validated_data:
-            tag_string = build_tag_string(validated_data["tag_names"])
+        tag_names = validated_data.pop("tag_names", instance.tag_names)
+        tag_string = build_tag_string(tag_names)
+
+        for field_name, field in self.fields.items():
+            if not field.read_only and field_name in validated_data:
+                setattr(instance, field_name, validated_data[field_name])
 
         return update_bookmark(instance, tag_string, self.context["user"])
 
diff --git a/bookmarks/services/bookmarks.py b/bookmarks/services/bookmarks.py
index 034404f..9bc9ea4 100644
--- a/bookmarks/services/bookmarks.py
+++ b/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
 
diff --git a/docs/src/content/docs/api.md b/docs/src/content/docs/api.md
index 7a12bad..f23fe33 100644
--- a/docs/src/content/docs/api.md
+++ b/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.
-Allows to modify individual fields of a bookmark.
+Updates 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:


Test patch:
diff --git a/bookmarks/tests/test_bookmarks_api.py b/bookmarks/tests/test_bookmarks_api.py
index 8794b8d..4bbe5a2 100644
--- a/bookmarks/tests/test_bookmarks_api.py
+++ b/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.description, "")
-        self.assertEqual(updated_bookmark.notes, "")
-        self.assertEqual(updated_bookmark.tag_names, [])
+        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_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)
diff --git a/bookmarks/tests/test_bookmarks_api_permissions.py b/bookmarks/tests/test_bookmarks_api_permissions.py
index 23830a1..2602c87 100644
--- a/bookmarks/tests/test_bookmarks_api_permissions.py
+++ b/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])