Merge pull request #369 from mouse-reeve/activitypub-cleanup

Activitypub cleanup

bookwyrm/activitypub/__init__.py

@@ -2,11 +2,11 @@
 import inspect
 import sys
 
-from .base_activity import ActivityEncoder, Image, PublicKey, Signature
+from .base_activity import ActivityEncoder, PublicKey, Signature
 from .base_activity import Link, Mention
 from .base_activity import ActivitySerializerError
 from .base_activity import tag_formatter
-from .base_activity import image_formatter, image_attachments_formatter
+from .image import Image
 from .note import Note, GeneratedNote, Article, Comment, Review, Quotation
 from .note import Tombstone
 from .interaction import Boost, Like

bookwyrm/activitypub/base_activity.py

@@ -4,6 +4,7 @@ from json import JSONEncoder
 from uuid import uuid4
 
 from django.core.files.base import ContentFile
+from django.db import transaction
 from django.db.models.fields.related_descriptors \
         import ForwardManyToOneDescriptor, ManyToManyDescriptor, \
         ReverseManyToOneDescriptor
@@ -23,13 +24,6 @@ class ActivityEncoder(JSONEncoder):
         return o.__dict__
 
 
-@dataclass
-class Image:
-    ''' image block '''
-    url: str
-    type: str = 'Image'
-
-
 @dataclass
 class Link():
     ''' for tagging a book in a status '''
@@ -113,14 +107,15 @@ class ActivityObject:
             formatted_value = mapping.model_formatter(value)
             if isinstance(model_field, ForwardManyToOneDescriptor) and \
                     formatted_value:
-                # foreign key remote id reolver
+                # foreign key remote id reolver (work on Edition, for example)
                 fk_model = model_field.field.related_model
                 reference = resolve_foreign_key(fk_model, formatted_value)
                 mapped_fields[mapping.model_key] = reference
             elif isinstance(model_field, ManyToManyDescriptor):
+                # status mentions book/users
                 many_to_many_fields[mapping.model_key] = formatted_value
             elif isinstance(model_field, ReverseManyToOneDescriptor):
-                # attachments on statuses, for example
+                # attachments on Status, for example
                 one_to_many_fields[mapping.model_key] = formatted_value
             elif isinstance(model_field, ImageFileDescriptor):
                 # image fields need custom handling
@@ -128,37 +123,41 @@ class ActivityObject:
             else:
                 mapped_fields[mapping.model_key] = formatted_value
 
-        if instance:
-            # updating an existing model isntance
-            for k, v in mapped_fields.items():
-                setattr(instance, k, v)
-            instance.save()
-        else:
-            # creating a new model instance
-            instance = model.objects.create(**mapped_fields)
-
-        # add many-to-many fields
-        for (model_key, values) in many_to_many_fields.items():
-            getattr(instance, model_key).set(values)
-        instance.save()
-
-        # add images
-        for (model_key, value) in image_fields.items():
-            if not value:
-                continue
-            getattr(instance, model_key).save(*value, save=True)
-
-        # add one to many fields
-        for (model_key, values) in one_to_many_fields.items():
-            items = []
-            for item in values:
-                # the reference id wasn't available at creation time
-                setattr(item, instance.__class__.__name__.lower(), instance)
-                item.save()
-                items.append(item)
-            if items:
-                getattr(instance, model_key).set(items)
+        with transaction.atomic():
+            if instance:
+                # updating an existing model isntance
+                for k, v in mapped_fields.items():
+                    setattr(instance, k, v)
                 instance.save()
+            else:
+                # creating a new model instance
+                instance = model.objects.create(**mapped_fields)
+
+            # add images
+            for (model_key, value) in image_fields.items():
+                formatted_value = image_formatter(value)
+                if not formatted_value:
+                    continue
+                getattr(instance, model_key).save(*formatted_value, save=True)
+
+            for (model_key, values) in many_to_many_fields.items():
+                # mention books, mention users
+                getattr(instance, model_key).set(values)
+
+            # add one to many fields
+            for (model_key, values) in one_to_many_fields.items():
+                if values == MISSING:
+                    continue
+                model_field = getattr(instance, model_key)
+                model = model_field.model
+                for item in values:
+                    item = model.activity_serializer(**item)
+                    field_name = instance.__class__.__name__.lower()
+                    with transaction.atomic():
+                        item = item.to_model(model)
+                        setattr(item, field_name, instance)
+                        item.save()
+
         return instance
 
 
@@ -210,18 +209,18 @@ def tag_formatter(tags, tag_type):
     return items
 
 
-def image_formatter(image_json):
+def image_formatter(image_slug):
     ''' helper function to load images and format them for a model '''
-    if isinstance(image_json, list):
-        try:
-            image_json = image_json[0]
-        except IndexError:
-            return None
-
-    if not image_json or not hasattr(image_json, 'url'):
+    # when it's an inline image (User avatar/icon, Book cover), it's a json
+    # blob, but when it's an attached image, it's just a url
+    if isinstance(image_slug, dict):
+        url = image_slug.get('url')
+    elif isinstance(image_slug, str):
+        url = image_slug
+    else:
+        return None
+    if not url:
         return None
-    url = image_json.get('url')
-
     try:
         response = requests.get(url)
     except ConnectionError:
@@ -232,17 +231,3 @@ def image_formatter(image_json):
     image_name = str(uuid4()) + '.' + url.split('.')[-1]
     image_content = ContentFile(response.content)
     return [image_name, image_content]
-
-
-def image_attachments_formatter(images_json):
-    ''' deserialize a list of images '''
-    attachments = []
-    for image in images_json:
-        caption = image.get('name')
-        attachment = models.Attachment(caption=caption)
-        image_field = image_formatter(image)
-        if not image_field:
-            continue
-        attachment.image.save(*image_field, save=False)
-        attachments.append(attachment)
-    return attachments

bookwyrm/activitypub/book.py

@@ -2,42 +2,43 @@
 from dataclasses import dataclass, field
 from typing import List
 
-from .base_activity import ActivityObject, Image
+from .base_activity import ActivityObject
+from .image import Image
 
 @dataclass(init=False)
 class Book(ActivityObject):
     ''' serializes an edition or work, abstract '''
-    authors: List[str]
-    first_published_date: str
-    published_date: str
-
     title: str
-    sort_title: str
-    subtitle: str
-    description: str
+    sortTitle: str = ''
+    subtitle: str = ''
+    description: str = ''
     languages: List[str]
-    series: str
-    series_number: str
+    series: str = ''
+    seriesNumber: str = ''
     subjects: List[str]
-    subject_places: List[str]
+    subjectPlaces: List[str]
 
-    openlibrary_key: str
-    librarything_key: str
-    goodreads_key: str
+    authors: List[str]
+    firstPublishedDate: str = ''
+    publishedDate: str = ''
 
-    attachment: List[Image] = field(default_factory=lambda: [])
+    openlibraryKey: str = ''
+    librarythingKey: str = ''
+    goodreadsKey: str = ''
+
+    cover: Image = field(default_factory=lambda: {})
     type: str = 'Book'
 
 
 @dataclass(init=False)
 class Edition(Book):
     ''' Edition instance of a book object '''
-    isbn_10: str
-    isbn_13: str
-    oclc_number: str
+    isbn10: str
+    isbn13: str
+    oclcNumber: str
     asin: str
     pages: str
-    physical_format: str
+    physicalFormat: str
     publishers: List[str]
 
     work: str

bookwyrm/activitypub/image.py

@@ -0,0 +1,11 @@
+''' an image, nothing fancy '''
+from dataclasses import dataclass
+from .base_activity import ActivityObject
+
+@dataclass(init=False)
+class Image(ActivityObject):
+    ''' image block '''
+    url: str
+    name: str = ''
+    type: str = 'Image'
+    id: str = ''

bookwyrm/activitypub/note.py

@@ -2,7 +2,8 @@
 from dataclasses import dataclass, field
 from typing import Dict, List
 
-from .base_activity import ActivityObject, Image, Link
+from .base_activity import ActivityObject, Link
+from .image import Image
 
 @dataclass(init=False)
 class Tombstone(ActivityObject):

bookwyrm/activitypub/person.py

@@ -2,7 +2,8 @@
 from dataclasses import dataclass, field
 from typing import Dict
 
-from .base_activity import ActivityObject, Image, PublicKey
+from .base_activity import ActivityObject, PublicKey
+from .image import Image
 
 @dataclass(init=False)
 class Person(ActivityObject):

bookwyrm/migrations/0014_auto_20201128_0118.py

@@ -0,0 +1,17 @@
+# Generated by Django 3.0.7 on 2020-11-28 01:18
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('bookwyrm', '0013_book_origin_id'),
+    ]
+
+    operations = [
+        migrations.RenameModel(
+            old_name='Attachment',
+            new_name='Image',
+        ),
+    ]

bookwyrm/migrations/0015_auto_20201128_0349.py

@@ -0,0 +1,19 @@
+# Generated by Django 3.0.7 on 2020-11-28 03:49
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('bookwyrm', '0014_auto_20201128_0118'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='image',
+            name='status',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='attachments', to='bookwyrm.Status'),
+        ),
+    ]

bookwyrm/models/__init__.py

@@ -5,15 +5,21 @@ import sys
 from .book import Book, Work, Edition
 from .author import Author
 from .connector import Connector
-from .relationship import UserFollows, UserFollowRequest, UserBlocks
+
 from .shelf import Shelf, ShelfBook
+
 from .status import Status, GeneratedNote, Review, Comment, Quotation
-from .status import Attachment, Favorite, Boost, Notification, ReadThrough
+from .status import Favorite, Boost, Notification, ReadThrough
+from .attachment import Image
+
 from .tag import Tag
+
 from .user import User
+from .relationship import UserFollows, UserFollowRequest, UserBlocks
 from .federated_server import FederatedServer
 
 from .import_job import ImportJob, ImportItem
+
 from .site import SiteSettings, SiteInvite, PasswordReset
 
 cls_members = inspect.getmembers(sys.modules[__name__], inspect.isclass)

bookwyrm/models/attachment.py

@@ -0,0 +1,32 @@
+''' media that is posted in the app '''
+from django.db import models
+
+from bookwyrm import activitypub
+from .base_model import ActivitypubMixin
+from .base_model import ActivityMapping, BookWyrmModel
+
+
+class Attachment(ActivitypubMixin, BookWyrmModel):
+    ''' an image (or, in the future, video etc) associated with a status '''
+    status = models.ForeignKey(
+        'Status',
+        on_delete=models.CASCADE,
+        related_name='attachments',
+        null=True
+    )
+    class Meta:
+        ''' one day we'll have other types of attachments besides images '''
+        abstract = True
+
+    activity_mappings = [
+        ActivityMapping('id', 'remote_id'),
+        ActivityMapping('url', 'image'),
+        ActivityMapping('name', 'caption'),
+    ]
+
+class Image(Attachment):
+    ''' an image attachment '''
+    image = models.ImageField(upload_to='status/', null=True, blank=True)
+    caption = models.TextField(null=True, blank=True)
+
+    activity_serializer = activitypub.Image

bookwyrm/models/base_model.py

@@ -10,6 +10,7 @@ from Crypto.PublicKey import RSA
 from Crypto.Signature import pkcs1_15
 from Crypto.Hash import SHA256
 from django.db import models
+from django.db.models.fields.files import ImageFieldFile
 from django.dispatch import receiver
 
 from bookwyrm import activitypub
@@ -77,16 +78,18 @@ class ActivitypubMixin:
                 value = value.remote_id
             elif isinstance(value, datetime):
                 value = value.isoformat()
+            elif isinstance(value, ImageFieldFile):
+                value = image_formatter(value)
 
             # run the custom formatter function set in the model
-            result = mapping.activity_formatter(value)
+            formatted_value = mapping.activity_formatter(value)
             if mapping.activity_key in fields and \
                     isinstance(fields[mapping.activity_key], list):
                 # there can be two database fields that map to the same AP list
                 # this happens in status tags, which combines user and book tags
-                fields[mapping.activity_key] += result
+                fields[mapping.activity_key] += formatted_value
             else:
-                fields[mapping.activity_key] = result
+                fields[mapping.activity_key] = formatted_value
 
         if pure:
             return self.pure_activity_serializer(
@@ -270,12 +273,10 @@ def tag_formatter(items, name_field, activity_type):
     return tags
 
 
-def image_formatter(image, default_path=None):
+def image_formatter(image):
     ''' convert images into activitypub json '''
     if image and hasattr(image, 'url'):
         url = image.url
-    elif default_path:
-        url = default_path
     else:
         return None
     url = 'https://%s%s' % (DOMAIN, url)

bookwyrm/models/book.py

@@ -12,7 +12,6 @@ from bookwyrm.utils.fields import ArrayField
 
 from .base_model import ActivityMapping, BookWyrmModel
 from .base_model import ActivitypubMixin, OrderedCollectionPageMixin
-from .base_model import image_attachments_formatter
 
 class Book(ActivitypubMixin, BookWyrmModel):
     ''' a generic book, which can mean either an edition or a work '''
@@ -61,49 +60,39 @@ class Book(ActivitypubMixin, BookWyrmModel):
         ''' the activitypub serialization should be a list of author ids '''
         return [a.remote_id for a in self.authors.all()]
 
-    @property
-    def ap_parent_work(self):
-        ''' reference the work via local id not remote '''
-        return self.parent_work.remote_id
-
     activity_mappings = [
         ActivityMapping('id', 'remote_id'),
 
         ActivityMapping('authors', 'ap_authors'),
-        ActivityMapping('first_published_date', 'first_published_date'),
-        ActivityMapping('published_date', 'published_date'),
+        ActivityMapping('firstPublishedDate', 'firstpublished_date'),
+        ActivityMapping('publishedDate', 'published_date'),
 
         ActivityMapping('title', 'title'),
-        ActivityMapping('sort_title', 'sort_title'),
+        ActivityMapping('sortTitle', 'sort_title'),
         ActivityMapping('subtitle', 'subtitle'),
         ActivityMapping('description', 'description'),
         ActivityMapping('languages', 'languages'),
         ActivityMapping('series', 'series'),
-        ActivityMapping('series_number', 'series_number'),
+        ActivityMapping('seriesNumber', 'series_number'),
         ActivityMapping('subjects', 'subjects'),
-        ActivityMapping('subject_places', 'subject_places'),
+        ActivityMapping('subjectPlaces', 'subject_places'),
 
-        ActivityMapping('openlibrary_key', 'openlibrary_key'),
-        ActivityMapping('librarything_key', 'librarything_key'),
-        ActivityMapping('goodreads_key', 'goodreads_key'),
+        ActivityMapping('openlibraryKey', 'openlibrary_key'),
+        ActivityMapping('librarythingKey', 'librarything_key'),
+        ActivityMapping('goodreadsKey', 'goodreads_key'),
 
-        ActivityMapping('work', 'ap_parent_work'),
-        ActivityMapping('isbn_10', 'isbn_10'),
-        ActivityMapping('isbn_13', 'isbn_13'),
-        ActivityMapping('oclc_number', 'oclc_number'),
+        ActivityMapping('work', 'parent_work'),
+        ActivityMapping('isbn10', 'isbn_10'),
+        ActivityMapping('isbn13', 'isbn_13'),
+        ActivityMapping('oclcNumber', 'oclc_number'),
         ActivityMapping('asin', 'asin'),
         ActivityMapping('pages', 'pages'),
-        ActivityMapping('physical_format', 'physical_format'),
+        ActivityMapping('physicalFormat', 'physical_format'),
         ActivityMapping('publishers', 'publishers'),
 
         ActivityMapping('lccn', 'lccn'),
         ActivityMapping('editions', 'editions_path'),
-        ActivityMapping(
-            'attachment', 'cover',
-            # this expects an iterable and the field is just an image
-            lambda x: image_attachments_formatter([x]),
-            activitypub.image_formatter
-        ),
+        ActivityMapping('cover', 'cover'),
     ]
 
     def save(self, *args, **kwargs):

bookwyrm/models/status.py

@@ -90,7 +90,6 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel):
         ActivityMapping(
             'attachment', 'attachments',
             lambda x: image_attachments_formatter(x.all()),
-            activitypub.image_attachments_formatter
         )
     ]
 
@@ -151,17 +150,6 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel):
         return super().save(*args, **kwargs)
 
 
-class Attachment(BookWyrmModel):
-    ''' an image (or, in the future, video etc) associated with a status '''
-    status = models.ForeignKey(
-        'Status',
-        on_delete=models.CASCADE,
-        related_name='attachments'
-    )
-    image = models.ImageField(upload_to='status/', null=True, blank=True)
-    caption = models.TextField(null=True, blank=True)
-
-
 class GeneratedNote(Status):
     ''' these are app-generated messages about user activity '''
     @property

bookwyrm/models/user.py

@@ -112,11 +112,7 @@ class User(OrderedCollectionPageMixin, AbstractUser):
             activity_formatter=lambda x: {'sharedInbox': x},
             model_formatter=lambda x: x.get('sharedInbox')
         ),
-        ActivityMapping(
-            'icon', 'avatar',
-            lambda x: image_formatter(x, '/static/images/default_avi.jpg'),
-            activitypub.image_formatter
-        ),
+        ActivityMapping('icon', 'avatar'),
         ActivityMapping(
             'manuallyApprovesFollowers',
             'manually_approves_followers'

bookwyrm/tests/data/ap_quotation.json

@@ -13,14 +13,6 @@
     "sensitive": false,
     "content": "commentary",
     "type": "Quotation",
-    "attachment": [
-        {
-            "type": "Document",
-            "mediaType": "image//images/covers/2b4e4712-5a4d-4ac1-9df4-634cc9c7aff3jpg",
-            "url": "https://example.com/images/covers/2b4e4712-5a4d-4ac1-9df4-634cc9c7aff3jpg",
-            "name": "Cover of \"This Is How You Lose the Time War\""
-        }
-    ],
     "replies": {
         "id": "https://example.com/user/mouse/quotation/13/replies",
         "type": "Collection",