Ensure tag names don't contain spaces (#184)

bookmarks/api/serializers.py

@@ -41,14 +41,14 @@ class BookmarkSerializer(serializers.ModelSerializer):
         bookmark.url = validated_data['url']
         bookmark.title = validated_data['title']
         bookmark.description = validated_data['description']
-        tag_string = build_tag_string(validated_data['tag_names'], ' ')
+        tag_string = build_tag_string(validated_data['tag_names'])
         return create_bookmark(bookmark, tag_string, self.context['user'])
 
     def update(self, instance: Bookmark, validated_data):
         instance.url = validated_data['url']
         instance.title = validated_data['title']
         instance.description = validated_data['description']
-        tag_string = build_tag_string(validated_data['tag_names'], ' ')
+        tag_string = build_tag_string(validated_data['tag_names'])
         return update_bookmark(instance, tag_string, self.context['user'])
 
 

bookmarks/models.py

@@ -20,11 +20,19 @@ class Tag(models.Model):
         return self.name
 
 
+def sanitize_tag_name(tag_name: str):
+    # strip leading/trailing spaces
+    # replace inner spaces with replacement char
+    return tag_name.strip().replace(' ', '-')
+
+
 def parse_tag_string(tag_string: str, delimiter: str = ','):
     if not tag_string:
         return []
     names = tag_string.strip().split(delimiter)
-    names = [name.strip() for name in names if name]
+    # remove empty names, sanitize remaining names
+    names = [sanitize_tag_name(name) for name in names if name]
+    # remove duplicates
     names = unique(names, str.lower)
     names.sort(key=str.lower)
 

bookmarks/services/bookmarks.py

@@ -90,7 +90,7 @@ def delete_bookmarks(bookmark_ids: [Union[int, str]], current_user: User):
 def tag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_user: User):
     sanitized_bookmark_ids = _sanitize_id_list(bookmark_ids)
     bookmarks = Bookmark.objects.filter(owner=current_user, id__in=sanitized_bookmark_ids)
-    tag_names = parse_tag_string(tag_string, ' ')
+    tag_names = parse_tag_string(tag_string)
     tags = get_or_create_tags(tag_names, current_user)
 
     for bookmark in bookmarks:
@@ -103,7 +103,7 @@ def tag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_user
 def untag_bookmarks(bookmark_ids: [Union[int, str]], tag_string: str, current_user: User):
     sanitized_bookmark_ids = _sanitize_id_list(bookmark_ids)
     bookmarks = Bookmark.objects.filter(owner=current_user, id__in=sanitized_bookmark_ids)
-    tag_names = parse_tag_string(tag_string, ' ')
+    tag_names = parse_tag_string(tag_string)
     tags = get_or_create_tags(tag_names, current_user)
 
     for bookmark in bookmarks:
@@ -125,7 +125,7 @@ def _update_website_metadata(bookmark: Bookmark):
 
 
 def _update_bookmark_tags(bookmark: Bookmark, tag_string: str, user: User):
-    tag_names = parse_tag_string(tag_string, ' ')
+    tag_names = parse_tag_string(tag_string)
     tags = get_or_create_tags(tag_names, user)
     bookmark.tags.set(tags)
 

bookmarks/tests/test_bookmarks_api.py

@@ -60,6 +60,18 @@ class BookmarksApiTestCase(LinkdingApiTestCase, BookmarkFactoryMixin):
         self.assertEqual(bookmark.tags.filter(name=data['tag_names'][0]).count(), 1)
         self.assertEqual(bookmark.tags.filter(name=data['tag_names'][1]).count(), 1)
 
+    def test_create_bookmark_replaces_whitespace_in_tag_names(self):
+        data = {
+            'url': 'https://example.com/',
+            'title': 'Test title',
+            'description': 'Test description',
+            'tag_names': ['tag 1', 'tag 2']
+        }
+        self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)
+        bookmark = Bookmark.objects.get(url=data['url'])
+        tag_names = [tag.name for tag in bookmark.tags.all()]
+        self.assertListEqual(tag_names, ['tag-1', 'tag-2'])
+
     def test_create_bookmark_minimal_payload(self):
         data = {'url': 'https://example.com/'}
         self.post(reverse('bookmarks:bookmark-list'), data, status.HTTP_201_CREATED)

bookmarks/tests/test_bookmarks_service.py

@@ -21,7 +21,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
     def test_create_should_create_web_archive_snapshot(self):
         with patch.object(tasks, 'create_web_archive_snapshot') as mock_create_web_archive_snapshot:
             bookmark_data = Bookmark(url='https://example.com')
-            bookmark = create_bookmark(bookmark_data, 'tag1 tag2', self.user)
+            bookmark = create_bookmark(bookmark_data, 'tag1,tag2', self.user)
 
             mock_create_web_archive_snapshot.assert_called_once_with(bookmark.id, False)
 
@@ -29,7 +29,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         with patch.object(tasks, 'create_web_archive_snapshot') as mock_create_web_archive_snapshot:
             bookmark = self.setup_bookmark()
             bookmark.url = 'https://example.com/updated'
-            update_bookmark(bookmark, 'tag1 tag2', self.user)
+            update_bookmark(bookmark, 'tag1,tag2', self.user)
 
             mock_create_web_archive_snapshot.assert_called_once_with(bookmark.id, True)
 
@@ -37,7 +37,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         with patch.object(tasks, 'create_web_archive_snapshot') as mock_create_web_archive_snapshot:
             bookmark = self.setup_bookmark()
             bookmark.title = 'updated title'
-            update_bookmark(bookmark, 'tag1 tag2', self.user)
+            update_bookmark(bookmark, 'tag1,tag2', self.user)
 
             mock_create_web_archive_snapshot.assert_not_called()
 
@@ -216,7 +216,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         tag1 = self.setup_tag()
         tag2 = self.setup_tag()
 
-        tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name} {tag2.name}',
+        tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name},{tag2.name}',
                       self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
@@ -232,7 +232,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         bookmark2 = self.setup_bookmark()
         bookmark3 = self.setup_bookmark()
 
-        tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], 'tag1 tag2', self.get_or_create_test_user())
+        tag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], 'tag1,tag2', self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
         bookmark2.refresh_from_db()
@@ -257,7 +257,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         tag1 = self.setup_tag()
         tag2 = self.setup_tag()
 
-        tag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name} {tag2.name}', self.get_or_create_test_user())
+        tag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
         bookmark2.refresh_from_db()
@@ -275,7 +275,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         tag1 = self.setup_tag()
         tag2 = self.setup_tag()
 
-        tag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name} {tag2.name}',
+        tag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name},{tag2.name}',
                       self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
@@ -293,7 +293,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         tag1 = self.setup_tag()
         tag2 = self.setup_tag()
 
-        tag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name} {tag2.name}',
+        tag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name},{tag2.name}',
                       self.get_or_create_test_user())
 
         self.assertCountEqual(bookmark1.tags.all(), [tag1, tag2])
@@ -307,7 +307,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         bookmark2 = self.setup_bookmark(tags=[tag1, tag2])
         bookmark3 = self.setup_bookmark(tags=[tag1, tag2])
 
-        untag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name} {tag2.name}',
+        untag_bookmarks([bookmark1.id, bookmark2.id, bookmark3.id], f'{tag1.name},{tag2.name}',
                         self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
@@ -325,7 +325,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         bookmark2 = self.setup_bookmark(tags=[tag1, tag2])
         bookmark3 = self.setup_bookmark(tags=[tag1, tag2])
 
-        untag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name} {tag2.name}', self.get_or_create_test_user())
+        untag_bookmarks([bookmark1.id, bookmark3.id], f'{tag1.name},{tag2.name}', self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
         bookmark2.refresh_from_db()
@@ -343,7 +343,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         bookmark2 = self.setup_bookmark(tags=[tag1, tag2])
         inaccessible_bookmark = self.setup_bookmark(user=other_user, tags=[tag1, tag2])
 
-        untag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name} {tag2.name}',
+        untag_bookmarks([bookmark1.id, bookmark2.id, inaccessible_bookmark.id], f'{tag1.name},{tag2.name}',
                         self.get_or_create_test_user())
 
         bookmark1.refresh_from_db()
@@ -361,7 +361,7 @@ class BookmarkServiceTestCase(TestCase, BookmarkFactoryMixin):
         bookmark2 = self.setup_bookmark(tags=[tag1, tag2])
         bookmark3 = self.setup_bookmark(tags=[tag1, tag2])
 
-        untag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name} {tag2.name}',
+        untag_bookmarks([str(bookmark1.id), bookmark2.id, str(bookmark3.id)], f'{tag1.name},{tag2.name}',
                         self.get_or_create_test_user())
 
         self.assertCountEqual(bookmark1.tags.all(), [])

bookmarks/tests/test_importer.py

@@ -2,6 +2,7 @@ from unittest.mock import patch
 
 from django.test import TestCase
 
+from bookmarks.models import Tag
 from bookmarks.services import tasks
 from bookmarks.services.importer import import_netscape_html
 from bookmarks.tests.helpers import BookmarkFactoryMixin, disable_logging
@@ -20,6 +21,18 @@ class ImporterTestCase(TestCase, BookmarkFactoryMixin):
 </DL><p>
         '''
 
+    def test_replace_whitespace_in_tag_names(self):
+        test_html = self.create_import_html(f'''
+            <DT><A HREF="https://example.com" ADD_DATE="1616337559" PRIVATE="0" TOREAD="0" TAGS="tag 1, tag 2, tag 3">Example.com</A>
+            <DD>Example.com
+        ''')
+        import_netscape_html(test_html, self.get_or_create_test_user())
+
+        tags = Tag.objects.all()
+        tag_names = [tag.name for tag in tags]
+
+        self.assertListEqual(tag_names, ['tag-1', 'tag-2', 'tag-3'])
+
     @disable_logging
     def test_validate_empty_or_missing_bookmark_url(self):
         test_html = self.create_import_html(f'''

bookmarks/tests/test_tags_model.py

@@ -25,3 +25,9 @@ class TagTestCase(TestCase):
     def test_parse_tag_string_deduplicates_tag_names(self):
         self.assertEqual(len(parse_tag_string('book,book,Book,BOOK')), 1)
 
+    def test_parse_tag_string_handles_duplicate_separators(self):
+        self.assertCountEqual(parse_tag_string('book,,movie,,,album'), ['album', 'book', 'movie'])
+
+    def test_parse_tag_string_replaces_whitespace_within_names(self):
+        self.assertCountEqual(parse_tag_string('travel guide, book recommendations'),
+                              ['travel-guide', 'book-recommendations'])

bookmarks/views/bookmarks.py

@@ -68,6 +68,12 @@ def generate_return_url(base_url, page, query_string):
     return urllib.parse.quote_plus(return_url)
 
 
+def convert_tag_string(tag_string: str):
+    # Tag strings coming from inputs are space-separated, however services.bookmarks functions expect comma-separated
+    # strings
+    return tag_string.replace(' ', ',')
+
+
 @login_required
 def new(request):
     initial_url = request.GET.get('url')
@@ -78,7 +84,8 @@ def new(request):
         auto_close = form.data['auto_close']
         if form.is_valid():
             current_user = request.user
-            create_bookmark(form.save(commit=False), form.data['tag_string'], current_user)
+            tag_string = convert_tag_string(form.data['tag_string'])
+            create_bookmark(form.save(commit=False), tag_string, current_user)
             if auto_close:
                 return HttpResponseRedirect(reverse('bookmarks:close'))
             else:
@@ -107,7 +114,8 @@ def edit(request, bookmark_id: int):
         form = BookmarkForm(request.POST, instance=bookmark)
         return_url = form.data['return_url']
         if form.is_valid():
-            update_bookmark(form.save(commit=False), form.data['tag_string'], request.user)
+            tag_string = convert_tag_string(form.data['tag_string'])
+            update_bookmark(form.save(commit=False), tag_string, request.user)
             return HttpResponseRedirect(return_url)
     else:
         return_url = request.GET.get('return_url')
@@ -166,10 +174,10 @@ def bulk_edit(request):
     if 'bulk_delete' in request.POST:
         delete_bookmarks(bookmark_ids, request.user)
     if 'bulk_tag' in request.POST:
-        tag_string = request.POST['bulk_tag_string']
+        tag_string = convert_tag_string(request.POST['bulk_tag_string'])
         tag_bookmarks(bookmark_ids, tag_string, request.user)
     if 'bulk_untag' in request.POST:
-        tag_string = request.POST['bulk_tag_string']
+        tag_string = convert_tag_string(request.POST['bulk_tag_string'])
         untag_bookmarks(bookmark_ids, tag_string, request.user)
 
     return_url = request.GET.get('return_url')