From 7b405c054d531382097a255927a71e5ea0037163 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Tue, 24 Sep 2024 11:37:50 +0200 Subject: [PATCH] Do not clear fields in POST requests (API behavior change) (#852) --- bookmarks/api/serializers.py | 38 +++------ bookmarks/services/bookmarks.py | 1 - bookmarks/tests/test_bookmarks_api.py | 85 +++++++++++++++++-- .../tests/test_bookmarks_api_permissions.py | 20 +++++ docs/src/content/docs/api.md | 29 +------ 5 files changed, 113 insertions(+), 60 deletions(-) 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]) + tag_names = validated_data.pop("tag_names", instance.tag_names) + tag_string = build_tag_string(tag_names) - # 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"]) + 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/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]) 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// -``` - -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// ``` -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: