Try fetching missing parent of relayables

* Extract post fetching logic from Reshare into
  its own module
* raise proper error message when fetching fails
* raise proper error message when parent is still missing

We can't skip fetch failures or missing parents and
still need to retry them in case we're sent the parent
later on

app/models/reshare.rb

@@ -74,35 +74,16 @@ def absolute_root
   private
 
   def after_parse
-    root_author = Webfinger.new(@root_diaspora_id).fetch
-    root_author.save! unless root_author.persisted?
-
-    return if Post.exists?(:guid => self.root_guid)
-
-    fetched_post = self.class.fetch_post(root_author, self.root_guid)
-
-    if fetched_post
-      #Why are we checking for this?
-      if root_author.diaspora_handle != fetched_post.diaspora_handle
-        raise "Diaspora ID (#{fetched_post.diaspora_handle}) in the root does not match the Diaspora ID (#{root_author.diaspora_handle}) specified in the reshare!"
+    if root.blank?
+      self.root = Diaspora::Fetcher::Single.find_or_fetch_from_remote root_guid, @root_diaspora_id do |fetched_post, author|
+        # why do we check this?
+        if fetched_post.diaspora_handle != author.diaspora_handle
+          raise Diaspora::PostNotFetchable, "Diaspora ID (#{fetched_post.diaspora_handle}) in the root does not match the Diaspora ID (#{author.diaspora_handle}) specified in the reshare!"
+        end
       end
-
-      fetched_post.save!
     end
   end
 
-  # Fetch a remote public post, used for receiving reshares of unknown posts
-  # @param [Person] author the remote post's author
-  # @param [String] guid the remote post's guid
-  # @return [Post] an unsaved remote post or false if the post was not found
-  def self.fetch_post author, guid
-    url = author.url + "/p/#{guid}.xml"
-    response = Faraday.get(url)
-    return false if response.status == 404 # Old pod, friendika
-    raise "Failed to get #{url}" unless response.success? # Other error, N/A for example
-    Diaspora::Parser.from_xml(response.body)
-  end
-
   def root_must_be_public
     if self.root && !self.root.public
       errors[:base] << "Only posts which are public may be reshared."

lib/diaspora/exceptions.rb

@@ -16,7 +16,6 @@ class AccountClosed < StandardError
   # that prevents further execution
   class NotMine < StandardError
   end
-
 
   # Received a message without having a contact
   class ContactRequiredUnlessRequest < StandardError
@@ -30,4 +29,10 @@ class RelayableObjectWithoutParent < StandardError
   # original XML message
   class AuthorXMLAuthorMismatch < StandardError
   end
+
+  # Tried to fetch a post but it was deleted, not valid
+  # or the remote end doesn't support post fetching
+  class PostNotFetchable < StandardError
+  end
+
 end

lib/diaspora/fetcher.rb

@@ -1,5 +1,6 @@
 module Diaspora
   module Fetcher
     require 'diaspora/fetcher/public'
+    require 'diaspora/fetcher/single'
   end
 end

lib/diaspora/fetcher/single.rb

@@ -0,0 +1,41 @@
+module Diaspora
+  module Fetcher
+    module Single
+      module_function
+
+      # Fetch and store a remote public post
+      # @param [String] guid the remote posts guid
+      # @param [String] author_id Diaspora ID of a user known to have the post,
+      #                 preferably the author
+      # @yield [Post, Person] If a block is given it is yielded the post
+      #                       and the author prior save
+      # @return a saved post
+      def find_or_fetch_from_remote guid, author_id
+        post = Post.where(guid: guid).first
+        return post if post
+
+        post_author = Webfinger.new(author_id).fetch
+        post_author.save! unless post_author.persisted?
+
+        if fetched_post = fetch_post(post_author, guid)
+          yield fetched_post, post_author if block_given?
+          raise Diaspora::PostNotFetchable unless fetched_post.save
+        end
+
+        fetched_post
+      end
+
+      # Fetch a remote public post, used for receiving of unknown public posts
+      # @param [Person] author the remote post's author
+      # @param [String] guid the remote post's guid
+      # @return [Post] an unsaved remote post or false if the post was not found
+      def fetch_post author, guid
+        url = author.url + "/p/#{guid}.xml"
+        response = Faraday.get(url)
+        raise Diaspora::PostNotFetchable if response.status == 404 # Old pod, Friendika, deleted
+        raise "Failed to get #{url}" unless response.success? # Other error, N/A for example
+        Diaspora::Parser.from_xml(response.body)
+      end
+    end
+  end
+end

lib/diaspora/relayable.rb

@@ -51,7 +51,8 @@ def parent_guid
     end
 
     def parent_guid= new_parent_guid
-      self.parent = parent_class.where(:guid => new_parent_guid).first
+      @parent_guid = new_parent_guid
+      self.parent = parent_class.where(guid: new_parent_guid).first
     end
 
     # @return [Array<Person>]
@@ -66,7 +67,7 @@ def subscribers(user)
     end
 
     def receive(user, person=nil)
-      comment_or_like = self.class.where(:guid => self.guid).first || self
+      comment_or_like = self.class.where(guid: self.guid).first || self
 
       # Check to make sure the signature of the comment or like comes from the person claiming to author it
       unless comment_or_like.parent_author == user.person || comment_or_like.verify_parent_author_signature
@@ -134,5 +135,17 @@ def parent
     def parent= parent
       raise NotImplementedError.new('you must override parent= in order to enable relayable on this model')
     end
+
+    # ROXML hook ensuring our own hooks are called
+    def after_parse
+      if @parent_guid
+        self.parent ||= fetch_parent(@parent_guid)
+      end
+    end
+
+    # Childs should override this to support fetching a missing parent
+    # @param guid the parents guid
+    def fetch_parent guid
+    end
   end
 end

lib/federated/relayable.rb

@@ -38,5 +38,9 @@ def parent
     def parent= parent
       self.target = parent
     end
+
+    def fetch_parent guid
+      Diaspora::Fetcher::Single.find_or_fetch_from_remote guid, diaspora_handle
+    end
   end
-end
+end

lib/postzord/receiver/public.rb

@@ -57,6 +57,7 @@ def receive_relayable
   def save_object
     @object = Diaspora::Parser::from_xml(@salmon.parsed_data)
     raise "Object is not public" if object_can_be_public_and_it_is_not?
+    raise Diaspora::RelayableObjectWithoutParent if object_must_have_parent_and_does_not?
     raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author?
     @object.save! if @object && @object.respond_to?(:save!)
     @object
@@ -88,4 +89,12 @@ def account_deletion_is_from_author
   def object_can_be_public_and_it_is_not?
     @object.respond_to?(:public) && !@object.public?
   end
+
+  def object_must_have_parent_and_does_not?
+    if @object.respond_to?(:relayable?) # comment, like
+      @object.parent.nil?
+    else
+      false
+    end
+  end
 end

spec/models/comment_spec.rb

@@ -96,6 +96,13 @@
       it 'marshals the post' do
         expect(@marshalled_comment.post).to eq(@post)
       end
+
+      it 'tries to fetch a missing parent' do
+        guid = @post.guid
+        @post.destroy
+        expect_any_instance_of(Comment).to receive(:fetch_parent).with(guid).and_return(nil)
+        Comment.from_xml(@xml)
+      end
     end
   end
 

spec/models/reshare_spec.rb

@@ -83,7 +83,7 @@
       rs1 = FactoryGirl.build(:reshare, :root=>@sm)
       rs2 = FactoryGirl.build(:reshare, :root=>rs1)
       @rs3 = FactoryGirl.build(:reshare, :root=>rs2)
-      
+
      sm = FactoryGirl.create(:status_message, :author => alice.person, :public => true)
      rs1 = FactoryGirl.create(:reshare, :root => sm)
      @of_deleted = FactoryGirl.build(:reshare, :root => rs1)
@@ -194,13 +194,13 @@
         end
 
         context "fetching post" do
-          it "doesn't error out if the post is not found" do
+          it "raises if the post is not found" do
             allow(@response).to receive(:status).and_return(404)
             expect(Faraday.default_connection).to receive(:get).and_return(@response)
 
             expect {
               Reshare.from_xml(@xml)
-            }.to_not raise_error
+            }.to raise_error(Diaspora::PostNotFetchable)
           end
 
           it "raises if there's another error receiving the post" do