From 777e3123d6968fe4f6c5c2f4032220918c59d3de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Sat, 6 Sep 2014 23:19:57 +0200 Subject: [PATCH] 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 | 31 +++++-------------------- lib/diaspora/exceptions.rb | 7 +++++- lib/diaspora/fetcher.rb | 1 + lib/diaspora/fetcher/single.rb | 41 +++++++++++++++++++++++++++++++++ lib/diaspora/relayable.rb | 17 ++++++++++++-- lib/federated/relayable.rb | 6 ++++- lib/postzord/receiver/public.rb | 9 ++++++++ spec/models/comment_spec.rb | 7 ++++++ spec/models/reshare_spec.rb | 6 ++--- 9 files changed, 93 insertions(+), 32 deletions(-) create mode 100644 lib/diaspora/fetcher/single.rb diff --git a/app/models/reshare.rb b/app/models/reshare.rb index 5bb5326d5..33581d05e 100644 --- a/app/models/reshare.rb +++ b/app/models/reshare.rb @@ -74,35 +74,16 @@ class Reshare < Post 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." diff --git a/lib/diaspora/exceptions.rb b/lib/diaspora/exceptions.rb index eba8366e0..2a9da47a2 100644 --- a/lib/diaspora/exceptions.rb +++ b/lib/diaspora/exceptions.rb @@ -16,7 +16,6 @@ module Diaspora # that prevents further execution class NotMine < StandardError end - # Received a message without having a contact class ContactRequiredUnlessRequest < StandardError @@ -30,4 +29,10 @@ module Diaspora # 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 diff --git a/lib/diaspora/fetcher.rb b/lib/diaspora/fetcher.rb index adb82e552..a27831e2f 100644 --- a/lib/diaspora/fetcher.rb +++ b/lib/diaspora/fetcher.rb @@ -1,5 +1,6 @@ module Diaspora module Fetcher require 'diaspora/fetcher/public' + require 'diaspora/fetcher/single' end end diff --git a/lib/diaspora/fetcher/single.rb b/lib/diaspora/fetcher/single.rb new file mode 100644 index 000000000..5dfeb7f73 --- /dev/null +++ b/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 diff --git a/lib/diaspora/relayable.rb b/lib/diaspora/relayable.rb index d0db312b7..4f43ed402 100644 --- a/lib/diaspora/relayable.rb +++ b/lib/diaspora/relayable.rb @@ -51,7 +51,8 @@ module Diaspora 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] @@ -66,7 +67,7 @@ module Diaspora 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 @@ module Diaspora 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 diff --git a/lib/federated/relayable.rb b/lib/federated/relayable.rb index d852a4dcd..a6cf6bf83 100644 --- a/lib/federated/relayable.rb +++ b/lib/federated/relayable.rb @@ -38,5 +38,9 @@ module Federated 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 \ No newline at end of file +end diff --git a/lib/postzord/receiver/public.rb b/lib/postzord/receiver/public.rb index 686e9a5b3..0243b72f5 100644 --- a/lib/postzord/receiver/public.rb +++ b/lib/postzord/receiver/public.rb @@ -57,6 +57,7 @@ class Postzord::Receiver::Public < Postzord::Receiver 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 @@ class Postzord::Receiver::Public < Postzord::Receiver 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 diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index a377d142a..efc7eacdc 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -96,6 +96,13 @@ describe Comment, :type => :model do 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 diff --git a/spec/models/reshare_spec.rb b/spec/models/reshare_spec.rb index 89dfd85f6..ce05d3b6b 100644 --- a/spec/models/reshare_spec.rb +++ b/spec/models/reshare_spec.rb @@ -83,7 +83,7 @@ describe Reshare, :type => :model do 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 @@ describe Reshare, :type => :model do 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