Migration: Fixing import issues / Speed up repeated unknown pod detection #8257

tclaus · 2021-06-23T17:59:54Z

It handles some migration issues listed in #8256, please check @cmrd-senya, @jhass, @Flaburgan .
(Import stops when posts comments or likes don't have a parent/author),
It handles #8019 (caching person fetches from remotes)

For this reason I changed the Person.find_or_fetch_by_identifier function. It now won't raise a NotFetchable exception, it just returns nil or the person object. Some callers rely on nil (which was never returned), some other rely on exception. IMHO this function should return an existing person from local store or by fetching, but never raise an exception.
Now, Person.find_or_fetch_by_identifier returns a person, or nil. Callers are adapted to this behaviour.

@cmrd-senya : I left over a failing test, which I can't fix, maybe you have on eye on this?
@jhass : can you check import - times with your archive file?

diaspora-code-review · 2021-06-23T18:00:05Z

spec/federation_callbacks_spec.rb

      diaspora_id = Fabricate.sequence(:diaspora_id)
      expect(Person).to receive(:find_or_fetch_by_identifier).with(diaspora_id)
-        .and_raise(DiasporaFederation::Discovery::DiscoveryError)
+        .and_return(nil)


Layout/MultilineMethodCallIndentation: Align .and_return with .with on line 236.

tclaus · 2021-06-23T18:38:53Z

Reminder: To enable/disable local caching on your dev machine you can toggle it (without changing configuration files) with:
$: rails dev:cache

I also added a statement to the spec helpers to clear any caches between each test.

diaspora-code-review · 2021-07-01T07:00:34Z

spec/lib/archive_importer/post_importer_spec.rb

  end
-end
+end


Layout/TrailingEmptyLines: Final newline missing.

SuperTux88

This isn't a full review yet, just a few comments about some basic stuff which probably changes a lot of the rest of the PR anyway.

SuperTux88 · 2021-07-03T02:47:55Z

app/models/person.rb

@@ -330,19 +330,31 @@ def exported_key
    serialized_public_key
  end

-  # discovery (webfinger)
+  # discovery (webfinger), returns nil if not found


As already mentioned in #8264 (comment), this method works as intended and shouldn't be changed. It's important this raises (forwards) the DiscoveryError, because multiple reasons:

It preserves the original cause of what failed. When this just returns nil, it fails later with a different error and you don't immediately see that it just couldn't fetch the person.

In most cases, raising is fine anyway, because without the person you can't continue with whatever should have been done. The archive-import is a bit of a special case, where you still want to import other stuff, and not stop the whole import process, but that problem should be solved at the import, not here.

Caching here is also not a good idea, because if a received message can't be processed because the person couldn't be fetched (because maybe the other pod was just only restarted), sidekiq retries the receive a few minutes later, if this then still has the cached result, the whole retry would be pointless. The caching makes sense for the import (if the same person is tried to be fetched only seconds apart), but again, this should be solved for the import only, not here.

Different errors are handled differently. As said above, received messages which fail because of the person not being able to be fetched are retried again a few times. With this (and all the related changes), this doesn't work anymore, because if this returns nil, and fetch_public_key in config/initializers/diaspora_federation.rb also returning nil then (instead of raising the expected DiscoveryError), the receive job fails with a PublicKeyNotFound, which isn't retried (because if a person can be fetched, because no DiscoveryError was thrown, but doesn't have a valid key for whatever reason, a retry would fail again, so no need to retry).

There are probably more things that rely on DiscoveryError being raised (and this method not being cached), this was just what immediately came to my mind.

So there needs to be a way to fix the problems of the import so they only affect the import. Maybe move more logic into the validation step and remove entities that can't be imported, so stuff doesn't happen twice? Maybe a cache that is only populated during an import?

SuperTux88 · 2021-07-03T03:11:00Z

lib/archive_importer/own_relayable_importer.rb

@@ -19,7 +21,9 @@ def fetch_parent(data)
        break entity_class::PARENT_TYPE if entity_class.const_defined?(:PARENT_TYPE)
      }
      entity = Diaspora::Federation::Mappings.model_class_for(type).find_by(guid: data.fetch(:parent_guid))
-      data[:parent] = Diaspora::Federation::Entities.related_entity(entity)
+      raise NoParentError if entity.nil?


The same question as in #8010 (comment) still exists. In theory this shouldn't happen, because in the validate step it already tries to fetch all the entities here

diaspora/lib/archive_validator/own_relayable_validator.rb

Lines 15 to 17 in f85f167

def by_guid(klass, guid)

klass.find_or_fetch_by(archive_author_diaspora_id, guid)

end

using

diaspora/lib/diaspora/federated/fetchable.rb

Lines 9 to 17 in f85f167

def find_or_fetch_by(diaspora_id, guid)

instance = find_by(guid: guid)

return instance if instance.present?

DiasporaFederation::Federation::Fetcher.fetch_public(diaspora_id, to_s, guid)

find_by(guid: guid)

rescue DiasporaFederation::Federation::Fetcher::NotFetchable

nil

end

which already returns nil when not being able to be fetched, and if the parent is nil, it's not valid:

diaspora/lib/archive_validator/relayable_validator.rb

Lines 16 to 18 in a3196a1

def validate

self.valid = parent_present?

end

which then should remove all invalid entities during the validation phase, so they aren't tried to be actually imported:

diaspora/lib/archive_validator/collection_validator.rb

Lines 9 to 13 in f85f167

collection.keep_if do |item|

subvalidator = entity_validator.new(archive_hash, item)

messages.concat(subvalidator.messages)

subvalidator.valid?

end

If this doesn't work as expected, it also means a lot of the things are done twice, which isn't intended (and could also lead to longer runtimes). If you have steps to reproduce this and can debug why entities that don't exist still end up at this point and aren't filtered out earlier already, it would be good to know and should be fixed there instead.

The part: find_by(guid: data.fetch(:parent_guid)) makes the problem here, not detected by the validator.
But we already talked that it seems to be inconvenient to fetch everything in the validation part.

tclaus · 2021-07-20T19:08:09Z

@SuperTux88 I give up. Sorry - I didn't understand the validation process in depth so that I can check all needed authors, parents and entries and fix tests and implement all as a validation.

Fixing raising NotFetchable error Fixing not found a parent on entities

Flaburgan · 2021-09-14T20:15:32Z

@SuperTux88 I give up. Sorry - I didn't understand the validation process in depth so that I can check all needed authors, parents and entries and fix tests and implement all as a validation.

If I understand correctly, you since had a call with @SuperTux88 to solve this point. So what is the status of that pull request now?

tclaus · 2021-09-15T05:35:15Z

My status is, that first idea was to push checks into the validation part, which did not succeed because of needed code changes. Another (more inportant) point was that fetching everything during validation is not what a validation process should do. After some code attempts, I implemented the smallest possible solution in this PR. To get forward and have one working branch I also rebased the upload-migration file branch to this work. Status is: it just works. (Until someone else Codes a „better“ solution).

…

Am 14.09.2021 um 22:15 schrieb Fla ***@***.***>: @SuperTux88 I give up. Sorry - I didn't understand the validation process in depth so that I can check all needed authors, parents and entries and fix tests and implement all as a validation. If I understand correctly, you since had a call with @SuperTux88 to solve this point. So what is the status of that pull request now? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

tclaus · 2021-09-18T11:02:15Z

@SuperTux88 As discussed, I have tried to make the smallest possible working solution without rebuilding the validation part. This solution gives what it should give.
With this in mind - could you check this again and possible approve (and merge) this?
It will be let the #8274 working.

…and diaspora#8017 Just test for generic NotFetchable error, which also includes the root author failing to be fetched.

SuperTux88 · 2021-09-18T17:41:51Z

I ended up merging #8010 because it had separate commits for the two different problems and also had a test for both problems. But I merged in the test from here since this was more generic (tests generic fetch problem and not only discovery errors).

Thanks for the work on this anyway since we finally found an answer for the original question in that PR 🍪

Flaburgan · 2021-09-18T17:57:55Z

Awesome to finally see this merged!

tclaus · 2021-09-18T17:59:26Z

Thanks for your analysis - to finally found a solution.

diaspora-code-review reviewed Jun 23, 2021

View reviewed changes

Flaburgan added this to the 0.8.0.0 milestone Jun 28, 2021

tclaus changed the title ~~Fixing import migration issues~~ Migration: Fixing import issues / Speed up repeated unknown pod detection Jun 28, 2021

tclaus mentioned this pull request Jun 29, 2021

Fix archive import for reshares of down pods #8017

Closed

tclaus force-pushed the 8256_fixing_migration_issues branch from e480452 to c114386 Compare July 1, 2021 07:00

diaspora-code-review reviewed Jul 1, 2021

View reviewed changes

spec/lib/archive_importer/post_importer_spec.rb Outdated

end

end

end

Copy link

diaspora-code-review Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/TrailingEmptyLines: Final newline missing.

tclaus force-pushed the 8256_fixing_migration_issues branch from c114386 to 4f663f7 Compare July 1, 2021 07:11

tclaus mentioned this pull request Jul 2, 2021

Migration: Adds user blocks to export/import profile #8258

Closed

SuperTux88 requested changes Jul 3, 2021

View reviewed changes

tclaus force-pushed the 8256_fixing_migration_issues branch from 9e2d729 to 93a776c Compare August 29, 2021 16:35

Fixing migration import errors

6f23839

Fixing raising NotFetchable error Fixing not found a parent on entities

tclaus force-pushed the 8256_fixing_migration_issues branch from 93a776c to 6f23839 Compare August 29, 2021 18:34

goobertron added ♿ accessibility 🚚 migration and removed ♿ accessibility labels Sep 12, 2021

tclaus requested a review from SuperTux88 September 18, 2021 10:59

SuperTux88 closed this in 2081f39 Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration: Fixing import issues / Speed up repeated unknown pod detection #8257

Migration: Fixing import issues / Speed up repeated unknown pod detection #8257

tclaus commented Jun 23, 2021 •

edited

diaspora-code-review Jun 23, 2021

tclaus commented Jun 23, 2021

diaspora-code-review Jul 1, 2021

SuperTux88 left a comment

SuperTux88 Jul 3, 2021

SuperTux88 Jul 3, 2021

tclaus Aug 29, 2021

tclaus commented Jul 20, 2021

Flaburgan commented Sep 14, 2021

tclaus commented Sep 15, 2021 via email

tclaus commented Sep 18, 2021

SuperTux88 commented Sep 18, 2021

Flaburgan commented Sep 18, 2021

tclaus commented Sep 18, 2021

	def by_guid(klass, guid)
	klass.find_or_fetch_by(archive_author_diaspora_id, guid)
	end

	def find_or_fetch_by(diaspora_id, guid)
	instance = find_by(guid: guid)
	return instance if instance.present?

	DiasporaFederation::Federation::Fetcher.fetch_public(diaspora_id, to_s, guid)
	find_by(guid: guid)
	rescue DiasporaFederation::Federation::Fetcher::NotFetchable
	nil
	end

	collection.keep_if do \|item\|
	subvalidator = entity_validator.new(archive_hash, item)
	messages.concat(subvalidator.messages)
	subvalidator.valid?
	end

Migration: Fixing import issues / Speed up repeated unknown pod detection #8257

Migration: Fixing import issues / Speed up repeated unknown pod detection #8257

Conversation

tclaus commented Jun 23, 2021 • edited

diaspora-code-review Jun 23, 2021

Choose a reason for hiding this comment

tclaus commented Jun 23, 2021

diaspora-code-review Jul 1, 2021

Choose a reason for hiding this comment

SuperTux88 left a comment

Choose a reason for hiding this comment

SuperTux88 Jul 3, 2021

Choose a reason for hiding this comment

SuperTux88 Jul 3, 2021

Choose a reason for hiding this comment

tclaus Aug 29, 2021

Choose a reason for hiding this comment

tclaus commented Jul 20, 2021

Flaburgan commented Sep 14, 2021

tclaus commented Sep 15, 2021 via email

tclaus commented Sep 18, 2021

SuperTux88 commented Sep 18, 2021

Flaburgan commented Sep 18, 2021

tclaus commented Sep 18, 2021

tclaus commented Jun 23, 2021 •

edited