Summary merging Protocol/Web receive: change receive to take Object

for #529

activitypub.py

@@ -722,7 +722,8 @@ def inbox(protocol=None, domain=None):
         followee_url = redirect_unwrap(util.get_url(activity, 'object'))
         activity.setdefault('url', f'{follower_url}#followed-{followee_url}')
 
-    return ActivityPub.receive(activity.get('id'), as2=redirect_unwrap(activity))
+    obj = Object(id=activity.get('id'), as2=redirect_unwrap(activity))
+    return ActivityPub.receive(obj)
 
 
 @app.get(f'/ap/<any({",".join(PROTOCOLS)}):protocol>/<regex("{common.DOMAIN_RE}"):domain>/<any(followers,following):collection>')

models.py

@@ -15,7 +15,11 @@ from granary import as1, as2, bluesky, microformats2
 from oauth_dropins.webutil import util
 from oauth_dropins.webutil.appengine_info import DEBUG
 from oauth_dropins.webutil.flask_util import error
-from oauth_dropins.webutil.models import ComputedJsonProperty, JsonProperty, StringIdModel
+from oauth_dropins.webutil.models import (
+    ComputedJsonProperty,
+    JsonProperty,
+    StringIdModel,
+)
 from oauth_dropins.webutil.util import json_dumps, json_loads
 
 import common
@@ -481,8 +485,8 @@ class Object(StringIdModel):
         """Returns an Object with the given property values.
 
         If a matching Object doesn't exist in the datastore, creates it first.
-
+        Only populates non-False/empty property values in props into the object.
-        Only populates non-False/empty property values.
+        Also populates the :attr:`new` and :attr:`changed` properties.
 
         Returns:
           :class:`Object`
@@ -496,7 +500,10 @@ class Object(StringIdModel):
             obj.new = True
 
         obj.clear()
-        obj.populate(**{k: v for k, v in props.items() if v})
+        obj.populate(**{
+            k: v for k, v in props.items()
+            if v and not isinstance(getattr(Object, k), ndb.ComputedProperty)
+        })
         if not obj.new:
             obj.changed = obj.activity_changed(orig_as1)
 
@@ -508,7 +515,7 @@ class Object(StringIdModel):
         for prop in 'as2', 'bsky', 'mf2':
             val = getattr(self, prop, None)
             if val:
-                logger.warning(f'Wiping out {prop}: {json_dumps(val, indent=2)}')
+                logger.warning(f'Wiping out existing {prop}: {json_dumps(val, indent=2)}')
             setattr(self, prop, None)
 
     def as_as2(self):

protocol.py

@@ -305,12 +305,14 @@ class Protocol:
         raise NotImplementedError()
 
     @classmethod
-    def receive(cls, id, **props):
+    def receive(cls, obj):
         """Handles an incoming activity.
 
+        If obj's key is unset, obj.as1's id field is used. If both are unset,
+        raises :class:`werkzeug.exceptions.BadRequest`.
+
         Args:
-          id: str, activity id
+          obj: :class:`Object`
-          props: property values to populate into the :class:`Object`
 
         Returns:
           (response body, HTTP status code) tuple for Flask response
@@ -318,42 +320,47 @@ class Protocol:
         Raises:
           :class:`werkzeug.HTTPException` if the request is invalid
         """
+        # check some invariants
         logger.info(f'From {cls.__name__}')
         assert cls != Protocol
+        assert isinstance(obj, Object), obj
+        logger.info(f'Got {obj.key.id()} AS1: {json_dumps(obj.as1, indent=2)}')
 
+        if not obj.as1:
+            error('No object data provided')
+
+        id = obj.key.id()
+        if not id:
+            id = obj.as1.get('id')
             if not id:
                 error('No id provided')
-        elif util.domain_from_link(id) in common.DOMAINS:
+            obj.key = ndb.Key(Object, id)
+
+        # block intra-BF ids
+        if util.domain_from_link(id) in common.DOMAINS:
             error(f'{id} is on a Bridgy Fed domain, which is not supported')
+        for field in 'id', 'actor', 'author', 'attributedTo':
+            val = as1.get_object(obj.as1, field).get('id')
+            if util.domain_from_link(val) in common.DOMAINS:
+                error(f'{field} {val} is on Bridgy Fed, which is not supported')
 
         # short circuit if we've already seen this activity id.
         # (don't do this for bare objects since we need to check further down
         # whether they've been updated since we saw them last.)
-        obj = Object(id=id, **props)
+        if obj.as1.get('objectType') == 'activity':
-        if not obj.as1 or obj.as1.get('objectType') == 'activity':
             with seen_ids_lock:
                 already_seen = id in seen_ids
                 seen_ids[id] = True
                 if already_seen or Object.get_by_id(id):
                     msg = f'Already handled this activity {id}'
                     logger.info(msg)
-                    return msg, 200
+                    return msg, 204
 
-        # block intra-BF ids
+        # write Object to datastore
-        if obj.as1:
+        obj = Object.get_or_create(id, **obj.to_dict())
-            for field in 'id', 'actor', 'author', 'attributedTo':
-                val = as1.get_object(obj.as1, field).get('id')
-                if util.domain_from_link(val) in common.DOMAINS:
-                    error(f'{field} {val} is on Bridgy Fed, which is not supported')
 
-        # create real Object
-        #
         # if this is a post, ie not an activity, wrap it in a create or update
         obj = cls._handle_bare_object(obj)
-        if not obj:
-            obj = Object.get_or_insert(id)
-            obj.clear()
-            obj.populate(source_protocol=cls.LABEL, **props)
 
         obj_actor = as1.get_owner(obj.as1)
         if obj_actor:
@@ -366,8 +373,8 @@ class Protocol:
             if inner_actor:
                 add(obj.users, cls.key_for(inner_actor))
 
+        obj.source_protocol = cls.LABEL
         obj.put()
-        logger.info(f'Got AS1: {json_dumps(obj.as1, indent=2)}')
 
         if obj.type not in SUPPORTED_TYPES:
             error(f'Sorry, {obj.type} activities are not supported yet.', status=501)
@@ -456,7 +463,7 @@ class Protocol:
                 obj.our_as1 = {**obj.as1, 'object': inner_obj.as1}
 
         if obj.type == 'follow':
-            cls.accept_follow(obj)
+            cls._accept_follow(obj)
 
         # deliver to each target
         cls._deliver(obj)
@@ -477,7 +484,7 @@ class Protocol:
         return 'OK'
 
     @classmethod
-    def accept_follow(cls, obj):
+    def _accept_follow(cls, obj):
         """Replies to a follow with an accept.
 
         Args:
@@ -564,20 +571,13 @@ class Protocol:
         now = util.now().isoformat()
 
         if obj.type not in ('note', 'article', 'comment'):
-            return
+            return obj
 
         # this is a raw post; wrap it in a create or update activity
-        if obj.new is None and obj.changed is None:
-            # check if we've seen this object, and if it's changed since then
-            existing = Object.get_by_id(obj.key.id())
-            obj.new = existing is None
-            obj.changed = existing and obj.activity_changed(existing.as1)
-
         if obj.changed:
             logger.info(f'Content has changed from last time at {obj.updated}! Redelivering to all inboxes')
             id = f'{obj.key.id()}#bridgy-fed-update-{now}'
             logger.info(f'Wrapping in update activity {id}')
-            obj.put()
             update_as1 = {
                 'objectType': 'activity',
                 'verb': 'update',
@@ -601,7 +601,6 @@ class Protocol:
             logger.info(f'New Object {obj.key.id()}')
             id = f'{obj.key.id()}#bridgy-fed-create'
             logger.info(f'Wrapping in post activity {id}')
-            obj.put()
             create_as1 = {
                 'objectType': 'activity',
                 'verb': 'post',
@@ -611,9 +610,8 @@ class Protocol:
                 'published': now,
             }
             source_protocol = obj.source_protocol
-            obj = Object.get_or_insert(id)
+            obj = Object.get_or_create(id, our_as1=create_as1,
-            obj.populate(our_as1=create_as1, source_protocol=source_protocol)
+                                       source_protocol=source_protocol)
-
         else:
             error(f'{obj.key.id()} is unchanged, nothing to do', status=204)
 
@@ -635,7 +633,7 @@ class Protocol:
         if not targets:
             obj.status = 'ignored'
             obj.put()
-            return 'No targets', 204
+            error('No targets', status=204)
 
         sorted_targets = sorted(targets.items(), key=lambda t: t[0].uri)
         obj.populate(
@@ -803,7 +801,7 @@ class Protocol:
             datastore after a successful remote fetch.
           kwargs: passed through to :meth:`fetch()`
 
-        Returns: :class:`Object` or None if it isn't in the datastore and remote
+        Returns: :class:`Object`, or None if it isn't in the datastore and remote
           is False
 
         Raises:

tests/test_protocol.py

@@ -18,18 +18,9 @@ from ui import UIProtocol
 from web import Web
 from werkzeug.exceptions import BadRequest
 
-from .test_activitypub import ACTOR, REPLY, REPLY_OBJECT
+from .test_activitypub import ACTOR
 from .test_web import ACTOR_HTML
 
-REPLY = {
-    **REPLY,
-    'actor': ACTOR,
-    'object': {
-        **REPLY['object'],
-        'attributedTo': ACTOR,
-    },
-}
-
 
 class ProtocolTest(TestCase):
 
@@ -342,7 +333,6 @@ class ProtocolReceiveTest(TestCase):
                                  type='post',
                                  labels=['user', 'activity', 'feed'],
                                  users=[g.user.key, self.alice.key, self.bob.key],
-                                 source_protocol=None,
                                  )
 
         self.assertEqual([(obj, 'shared:target')], Fake.sent)
@@ -414,7 +404,6 @@ class ProtocolReceiveTest(TestCase):
                                  type='update',
                                  labels=['user', 'activity'],
                                  users=[g.user.key],
-                                 source_protocol=None,
                                  )
 
         self.assertEqual([(obj, 'shared:target')], Fake.sent)
@@ -492,7 +481,6 @@ class ProtocolReceiveTest(TestCase):
                                  type='post',
                                  labels=['user', 'activity', 'notification'],
                                  users=[self.alice.key, self.bob.key],
-                                 source_protocol=None,
                                  )
 
         self.assertEqual([(obj, 'fake:post:target')], Fake.sent)
@@ -673,6 +661,7 @@ class ProtocolReceiveTest(TestCase):
 #             LIKE,
 #         ]
 
+        # with self.assertRaises(NoContent):
 #         got = self.client.post('/_ah/queue/webmention', data={
 #             'source': 'https://user.com/like',
 #             'target': 'https://fed.brid.gy/',
@@ -823,6 +812,7 @@ class ProtocolReceiveTest(TestCase):
 #             'source': 'https://user.com/repost',
 #             'target': 'https://fed.brid.gy/',
 #         })
+        # with self.assertRaises(NoContent):
 #         self.assertEqual(204, got.status_code)
 #         mock_post.assert_not_called()
 
@@ -1080,26 +1070,20 @@ class ProtocolReceiveTest(TestCase):
 
     def test_receive_from_bridgy_fed_fails(self):
         with self.assertRaises(BadRequest):
-            Fake.receive('https://fed.brid.gy/r/foo', as2=REPLY)
+            Fake.receive({
+                'id': 'https://fed.brid.gy/r/foo',
+            })
 
         self.assertIsNone(Object.get_by_id('https://fed.brid.gy/r/foo'))
 
         with self.assertRaises(BadRequest):
-            Fake.receive('foo', as2={
+            Fake.receive({
-                **REPLY,
+                'id': 'fake:foo',
-                'id': 'https://web.brid.gy/r/foo',
-            })
-
-        self.assertIsNone(Object.get_by_id('foo'))
-        self.assertIsNone(Object.get_by_id('https://web.brid.gy/r/foo'))
-
-        with self.assertRaises(BadRequest):
-            Fake.receive(REPLY['id'], as2={
-                **REPLY,
                 'actor': 'https://ap.brid.gy/user.com',
             })
 
-        self.assertIsNone(Object.get_by_id(REPLY['id']))
+        self.assertIsNone(Object.get_by_id('foo'))
+        self.assertIsNone(Object.get_by_id('https://ap.brid.gy/user.com'))
 
     # def test_skip_same_domain_target(self):
     #     TODO

tests/testutil.py

@@ -92,11 +92,9 @@ class Fake(User, protocol.Protocol):
         return 'shared:target' if shared else f'{obj.key.id()}:target'
 
     @classmethod
-    def receive(cls, id_or_data, **kwargs):
+    def receive(cls, our_as1):
-        if isinstance(id_or_data, dict):
+        assert isinstance(our_as1, dict)
-            return super().receive(id_or_data['id'], our_as1=id_or_data, **kwargs)
+        return super().receive(Object(id=our_as1['id'], our_as1=our_as1))
-        else:
-            return super().receive(id_or_data, **kwargs)
 
 
 # used in TestCase.make_user() to reuse keys across Users since they're

web.py

@@ -523,7 +523,7 @@ def webmention_task():
     if not obj.mf2 and obj.type != 'delete':
         error(f'No microformats2 found in {source}', status=304)
     elif obj.mf2:
-        # set actor to user
+        # default actor to user
         props = obj.mf2['properties']
         author_urls = microformats2.get_string_urls(props.get('author', []))
         if author_urls and not g.user.is_web_url(author_urls[0]):