Repository: jaywink/federation

Issue:
Finish up status message photos support
Every found `<photo>` should be linked to the current entity as a child entity.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b9b2bda..c80ac67 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,16 @@
 # Changelog
 
+## [unreleased]
+
+### Backwards incompatible changes
+* `Image` no longer has a `text` attribute. It is replaced by `raw_content`, the same attribute as `Post` and `Comment` have. Unlike the latter two, `Image.raw_content` is not mandatory.
+
+### Added
+* Entities can now have a children. These can be accessed using the `_children` list. Acceptable children depends on the entity. Currently, `Post`, `Comment` and `Profile` can have children of entity type `Image`. Child types are validated in the `.validate()` entity method call.
+
+### Fixed
+* Diaspora protocol `message_to_objects` method (called through inbound high level methods) now correctly parses Diaspora `<photo>` elements and creates `Image` entities from them. If they are children of status messages, they will be available through the `Post._children` list.
+
 ## [0.8.2] - 2016-10-23
 
 ### Fixed
diff --git a/federation/entities/base.py b/federation/entities/base.py
index 0e1534f..71d441b 100644
--- a/federation/entities/base.py
+++ b/federation/entities/base.py
@@ -12,6 +12,8 @@ __all__ = (
 
 class BaseEntity(object):
     _required = []
+    _children = []
+    _allowed_children = ()
 
     def __init__(self, *args, **kwargs):
         self._required = []
@@ -29,6 +31,7 @@ class BaseEntity(object):
         1) Check `_required` have been given
         2) Make sure all attrs in required have a non-empty value
         3) Loop through attributes and call their `validate_<attr>` methods, if any.
+        4) Validate allowed children
         """
         attributes = []
         validates = []
@@ -43,6 +46,7 @@ class BaseEntity(object):
         self._validate_empty_attributes(attributes)
         self._validate_required(attributes)
         self._validate_attributes(validates)
+        self._validate_children()
 
     def _validate_required(self, attributes):
         """Ensure required attributes are present."""
@@ -67,6 +71,15 @@ class BaseEntity(object):
                     "Attribute %s cannot be None or an empty string since it is required." % attr
                 )
 
+    def _validate_children(self):
+        """Check that the children we have are allowed here."""
+        for child in self._children:
+            if child.__class__ not in self._allowed_children:
+                raise ValueError(
+                    "Child %s is not allowed as a children for this %s type entity." % (
+                        child, self.__class__
+                    )
+                )
 
 class GUIDMixin(BaseEntity):
     guid = ""
@@ -138,18 +151,17 @@ class RawContentMixin(BaseEntity):
         return set({word.strip("#") for word in self.raw_content.split() if word.startswith("#")})
 
 
-class Post(RawContentMixin, GUIDMixin, HandleMixin, PublicMixin, CreatedAtMixin, BaseEntity):
-    """Reflects a post, status message, etc, which will be composed from the message or to the message."""
-    provider_display_name = ""
-    location = ""
-    photos = []
+class OptionalRawContentMixin(RawContentMixin):
+    """A version of the RawContentMixin where `raw_content` is not required."""
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self._required.remove("raw_content")
 
 
-class Image(GUIDMixin, HandleMixin, PublicMixin, CreatedAtMixin, BaseEntity):
+class Image(GUIDMixin, HandleMixin, PublicMixin, OptionalRawContentMixin, CreatedAtMixin, BaseEntity):
     """Reflects a single image, possibly linked to another object."""
     remote_path = ""
     remote_name = ""
-    text = ""
     linked_type = ""
     linked_guid = ""
     height = 0
@@ -160,6 +172,15 @@ class Image(GUIDMixin, HandleMixin, PublicMixin, CreatedAtMixin, BaseEntity):
         self._required += ["remote_path", "remote_name"]
 
 
+class Post(RawContentMixin, GUIDMixin, HandleMixin, PublicMixin, CreatedAtMixin, BaseEntity):
+    """Reflects a post, status message, etc, which will be composed from the message or to the message."""
+    provider_display_name = ""
+    location = ""
+    photos = []
+
+    _allowed_children = (Image,)
+
+
 class ParticipationMixin(TargetGUIDMixin):
     """Reflects a participation to something."""
     target_guid = ""
@@ -183,6 +204,8 @@ class Comment(RawContentMixin, GUIDMixin, ParticipationMixin, CreatedAtMixin, Ha
     """Represents a comment, linked to another object."""
     participation = "comment"
 
+    _allowed_children = (Image,)
+
 
 class Reaction(GUIDMixin, ParticipationMixin, CreatedAtMixin, HandleMixin):
     """Represents a reaction to another object, for example a like."""
@@ -243,6 +266,8 @@ class Profile(CreatedAtMixin, HandleMixin, RawContentMixin, PublicMixin, GUIDMix
     tag_list = []
     public_key = ""
 
+    _allowed_children = (Image,)
+
     def validate_email(self):
         if self.email:
             validator = Email()
diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py
index d9fbe6a..84b5b4c 100644
--- a/federation/entities/diaspora/mappers.py
+++ b/federation/entities/diaspora/mappers.py
@@ -30,6 +30,10 @@ DATETIME_KEYS = [
     "created_at",
 ]
 
+INTEGER_KEYS = [
+    "height",
+    "width",
+]
 
 def xml_children_as_dict(node):
     """Turn the children of node <xml> into a dict, keyed by tag name.
@@ -39,32 +43,57 @@ def xml_children_as_dict(node):
     return dict((e.tag, e.text) for e in node)
 
 
-def message_to_objects(message):
-    """Takes in a message extracted by a protocol and maps it to entities."""
-    doc = etree.fromstring(message)
+def element_to_objects(tree):
+    """Transform an Element tree to a list of entities recursively.
+
+    Possible child entities are added to each entity `_children` list.
+
+    :param tree: Element
+    :returns: list of entities
+    """
     entities = []
-    for element in doc.iter():
+    for element in tree:
         cls = MAPPINGS.get(element.tag, None)
-        if cls:
-            attrs = xml_children_as_dict(element)
-            transformed = transform_attributes(attrs)
-            if hasattr(cls, "fill_extra_attributes"):
-                transformed = cls.fill_extra_attributes(transformed)
-            entity = cls(**transformed)
-            try:
-                entity.validate()
-                entities.append(entity)
-            except ValueError as ex:
-                logger.error("Failed to validate entity %s: %s", entity, ex, extra={
-                    "attrs": attrs,
-                    "transformed": transformed,
-                })
-                continue
-            if cls == DiasporaRequest:
-                # We support sharing/following separately, so also generate base Relationship for the following part
-                transformed.update({"relationship": "following"})
-                entity = Relationship(**transformed)
-                entities.append(entity)
+        if not cls:
+            continue
+
+        attrs = xml_children_as_dict(element)
+        transformed = transform_attributes(attrs)
+        if hasattr(cls, "fill_extra_attributes"):
+            transformed = cls.fill_extra_attributes(transformed)
+        entity = cls(**transformed)
+        try:
+            entity.validate()
+        except ValueError as ex:
+            logger.error("Failed to validate entity %s: %s", entity, ex, extra={
+                "attrs": attrs,
+                "transformed": transformed,
+            })
+            continue
+        # Do child elements
+        entity._children = element_to_objects(element)
+        # Add to entities list
+        entities.append(entity)
+        if cls == DiasporaRequest:
+            # We support sharing/following separately, so also generate base Relationship for the following part
+            transformed.update({"relationship": "following"})
+            relationship = Relationship(**transformed)
+            entities.append(relationship)
+    return entities
+
+
+def message_to_objects(message):
+    """Takes in a message extracted by a protocol and maps it to entities.
+
+    :param message: XML payload
+    :type message: str
+    :returns: list of entities
+    """
+    doc = etree.fromstring(message)
+    if doc[0].tag == "post":
+        # Skip the top <post> element if it exists
+        doc = doc[0]
+    entities = element_to_objects(doc)
     return entities
 
 
@@ -102,10 +131,19 @@ def transform_attributes(attrs):
             transformed["public"] = True if value == "true" else False
         elif key == "target_type":
             transformed["entity_type"] = DiasporaRetraction.entity_type_from_remote(value)
+        elif key == "remote_photo_path":
+            transformed["remote_path"] = value
+        elif key == "remote_photo_name":
+            transformed["remote_name"] = value
+        elif key == "status_message_guid":
+            transformed["linked_guid"] = value
+            transformed["linked_type"] = "Post"
         elif key in BOOLEAN_KEYS:
             transformed[key] = True if value == "true" else False
         elif key in DATETIME_KEYS:
             transformed[key] = datetime.strptime(value, "%Y-%m-%d %H:%M:%S %Z")
+        elif key in INTEGER_KEYS:
+            transformed[key] = int(value)
         else:
             transformed[key] = value or ""
     return transformed


Test patch:
diff --git a/federation/tests/entities/diaspora/test_mappers.py b/federation/tests/entities/diaspora/test_mappers.py
index 0a5aa32..c98c661 100644
--- a/federation/tests/entities/diaspora/test_mappers.py
+++ b/federation/tests/entities/diaspora/test_mappers.py
@@ -4,7 +4,9 @@ from unittest.mock import patch
 
 import pytest
 
-from federation.entities.base import Comment, Post, Reaction, Relationship, Profile, Retraction
+from federation.entities.base import (
+    Comment, Post, Reaction, Relationship, Profile, Retraction, Image
+)
 from federation.entities.diaspora.entities import (
     DiasporaPost, DiasporaComment, DiasporaLike, DiasporaRequest,
     DiasporaProfile, DiasporaRetraction
@@ -12,8 +14,8 @@ from federation.entities.diaspora.entities import (
 from federation.entities.diaspora.mappers import message_to_objects, get_outbound_entity
 from federation.tests.fixtures.payloads import (
     DIASPORA_POST_SIMPLE, DIASPORA_POST_COMMENT, DIASPORA_POST_LIKE,
-    DIASPORA_REQUEST, DIASPORA_PROFILE, DIASPORA_POST_INVALID, DIASPORA_RETRACTION
-)
+    DIASPORA_REQUEST, DIASPORA_PROFILE, DIASPORA_POST_INVALID, DIASPORA_RETRACTION,
+    DIASPORA_POST_WITH_PHOTOS)
 
 
 def mock_fill(attributes):
@@ -35,6 +37,38 @@ class TestDiasporaEntityMappersReceive(object):
         assert post.created_at == datetime(2011, 7, 20, 1, 36, 7)
         assert post.provider_display_name == "Socialhome"
 
+    def test_message_to_objects_post_with_photos(self):
+        entities = message_to_objects(DIASPORA_POST_WITH_PHOTOS)
+        assert len(entities) == 1
+        post = entities[0]
+        assert isinstance(post, DiasporaPost)
+        photo = post._children[0]
+        assert isinstance(photo, Image)
+        assert photo.remote_path == "https://alice.diaspora.example.org/uploads/images/"
+        assert photo.remote_name == "1234.jpg"
+        assert photo.raw_content == None
+        assert photo.linked_type == "Post"
+        assert photo.linked_guid == "((guidguidguidguidguidguidguid))"
+        assert photo.height == 120
+        assert photo.width == 120
+        assert photo.guid == "((guidguidguidguidguidguidguif))"
+        assert photo.handle == "alice@alice.diaspora.example.org"
+        assert photo.public == False
+        assert photo.created_at == datetime(2011, 7, 20, 1, 36, 7)
+        photo = post._children[1]
+        assert isinstance(photo, Image)
+        assert photo.remote_path == "https://alice.diaspora.example.org/uploads/images/"
+        assert photo.remote_name == "12345.jpg"
+        assert photo.raw_content == "foobar"
+        assert photo.linked_type == "Post"
+        assert photo.linked_guid == "((guidguidguidguidguidguidguid))"
+        assert photo.height == 120
+        assert photo.width == 120
+        assert photo.guid == "((guidguidguidguidguidguidguig))"
+        assert photo.handle == "alice@alice.diaspora.example.org"
+        assert photo.public == False
+        assert photo.created_at == datetime(2011, 7, 20, 1, 36, 7)
+
     def test_message_to_objects_comment(self):
         entities = message_to_objects(DIASPORA_POST_COMMENT)
         assert len(entities) == 1
diff --git a/federation/tests/entities/test_base.py b/federation/tests/entities/test_base.py
index be48eaf..f4b769f 100644
--- a/federation/tests/entities/test_base.py
+++ b/federation/tests/entities/test_base.py
@@ -25,6 +25,28 @@ class TestBaseEntityCallsValidateMethods(object):
         post.validate()
         assert post.validate_location.call_count == 1
 
+    def test_entity_calls_main_validate_methods(self):
+        post = PostFactory()
+        post._validate_required = Mock()
+        post._validate_attributes = Mock()
+        post._validate_empty_attributes = Mock()
+        post._validate_children = Mock()
+        post.validate()
+        assert post._validate_required.call_count == 1
+        assert post._validate_attributes.call_count == 1
+        assert post._validate_empty_attributes.call_count == 1
+        assert post._validate_children.call_count == 1
+
+    def test_validate_children(self):
+        post = PostFactory()
+        image = Image()
+        profile = Profile()
+        post._children = [image]
+        post._validate_children()
+        post._children = [profile]
+        with pytest.raises(ValueError):
+            post._validate_children()
+
 
 class TestGUIDMixinValidate(object):
     def test_validate_guid_raises_on_low_length(self):
diff --git a/federation/tests/fixtures/payloads.py b/federation/tests/fixtures/payloads.py
index 5bb64d6..15b0888 100644
--- a/federation/tests/fixtures/payloads.py
+++ b/federation/tests/fixtures/payloads.py
@@ -42,6 +42,46 @@ DIASPORA_POST_SIMPLE = """<XML>
     </XML>
 """
 
+
+DIASPORA_POST_WITH_PHOTOS = """<XML>
+      <post>
+        <status_message>
+          <raw_message>((status message))</raw_message>
+          <guid>((guidguidguidguidguidguidguid))</guid>
+          <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
+          <public>false</public>
+          <created_at>2011-07-20 01:36:07 UTC</created_at>
+          <provider_display_name>Socialhome</provider_display_name>
+          <photo>
+            <guid>((guidguidguidguidguidguidguif))</guid>
+            <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
+            <public>false</public>
+            <created_at>2011-07-20 01:36:07 UTC</created_at>
+            <remote_photo_path>https://alice.diaspora.example.org/uploads/images/</remote_photo_path>
+            <remote_photo_name>1234.jpg</remote_photo_name>
+            <text/>
+            <status_message_guid>((guidguidguidguidguidguidguid))</status_message_guid>
+            <height>120</height>
+            <width>120</width>
+          </photo>
+          <photo>
+            <guid>((guidguidguidguidguidguidguig))</guid>
+            <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
+            <public>false</public>
+            <created_at>2011-07-20 01:36:07 UTC</created_at>
+            <remote_photo_path>https://alice.diaspora.example.org/uploads/images/</remote_photo_path>
+            <remote_photo_name>12345.jpg</remote_photo_name>
+            <text>foobar</text>
+            <status_message_guid>((guidguidguidguidguidguidguid))</status_message_guid>
+            <height>120</height>
+            <width>120</width>
+          </photo>
+        </status_message>
+      </post>
+    </XML>
+"""
+
+
 DIASPORA_POST_INVALID = """<XML>
       <post>
         <status_message>