Add backfilling of posts #846

Open
tudbut wants to merge 22 commits from tudbut/akkoma:develop into develop
Contributor

Hey! I decided it was kinda dumb that outboxes exist everywhere yet no software uses them for their intended (or any, really) purpose.

So now I added backfilling of posts from remote instances. This only fetches a single page, and will re-fetch whenever the user's page is requested, so that it won't cause there to be only a slice of posts. :D

Hey! I decided it was kinda dumb that outboxes exist everywhere yet no software uses them for their intended (or any, really) purpose. So now I added backfilling of posts from remote instances. This only fetches a single page, and will re-fetch whenever the user's page is requested, so that it won't cause there to be only a slice of posts. :D
tudbut force-pushed develop from 2aefeeee61 to 34f78ab4cd 2024-10-27 05:49:11 +00:00 Compare

the implementation looks very sensible - i saw the pr title and thought "i'm gonna have to ask for this to be a worker" but it already is which is nice

two little things before this can be merged - first off, a changelog entry would be nice
and also, this will need some tests - current remote fetcher worker tests are over here -> https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/test/pleroma/workers/remote_fetcher_worker_test.exs

should give you a decent start - you don't need to be too extensive, but some basic input-output checks would be good i think

the implementation looks very sensible - i saw the pr title and thought "i'm gonna have to ask for this to be a worker" but it already is which is nice two little things before this can be merged - first off, a changelog entry would be nice and also, this will need some tests - current remote fetcher worker tests are over here -> https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/test/pleroma/workers/remote_fetcher_worker_test.exs should give you a decent start - you don't need to be _too_ extensive, but some basic input-output checks would be good i think
Author
Contributor

Thanks! ill go look at the tests stuff while on the train (in a few minutes).

Thanks! ill go look at the tests stuff while on the train (in a few minutes).
Oneric reviewed 2024-10-29 16:23:32 +00:00
Oneric left a comment
Owner

I haven’t had time to take a proper look but at from a glance I’m concerned about some parts of this, though feel free to point out how i missed the (obvious) justification and/or reason why this is good and safe on my quick glance

I haven’t had time to take a proper look but at from a glance I’m concerned about some parts of this, though feel free to point out how i missed the (obvious) justification and/or reason why this is good and safe on my quick glance
@ -20,3 +20,3 @@
def cast(%{"id" => object}), do: cast(object)
def cast(_), do: :error
def cast(o), do: {:error, o}
Owner

is this signature change actually safe? Why is it done?
Note this is also inconsistent with the return of a prior function clause

is this signature change actually safe? Why is it done? Note this is also inconsistent with the return of a prior function clause
Author
Contributor

this can be reverted. i experienced several errors that were not specific enough and doing this told me what the issue was. ill revert it if you want

this can be reverted. i experienced several errors that were not specific enough and doing this told me what the issue was. ill revert it if you want
Author
Contributor

reverting this in the next commit

reverting this in the next commit
tudbut marked this conversation as resolved
@ -1773,3 +1775,3 @@
end
def pin_data_from_featured_collection(%{
def activity_data_from_outbox_collection(%{
Owner

This seems to replace pinned fetches everywhere, meaning we no longer are guaranteed to get pinned posts of a user profile?

This seems to _replace_ pinned fetches everywhere, meaning we no longer are guaranteed to get pinned posts of a user profile?
Author
Contributor

pins are still fetched, using the same function. it treats an outbox the same as the featured collection

pins are still fetched, using the same function. it treats an outbox the same as the featured collection
Author
Contributor

if the naming is misleading, i can change it

if the naming is misleading, i can change it
Owner

yes, please use a more generic name, calling "fetch_outbox" to fetch the pinned collection is just as confusing as if leaving the original name and alling fetch_pins to get the outbox

yes, please use a more generic name, calling "fetch_outbox" to fetch the pinned collection is just as confusing as if leaving the original name and alling `fetch_pins` to get the outbox
tudbut marked this conversation as resolved
@ -641,0 +640,4 @@
defp handle_incoming_normalised(%{"object" => o}, options),
do: handle_incoming_normalised(o, options)
defp handle_incoming_normalised(o, _), do: {:error, {:unknown_object_class, o}}
Owner

Just processing the referenced object if we don't understand the activity is imo a bad idea and likely to break stuff: e.g. consider a malformed Delete which might now end up actually inserting the object instead of deleting it
It can also end up breaking #845 (which i also haven’t reviewed yet)

similarly the changed return signature from :error to a tuple {:error, {.., ..} needs some attention to check whether anything is affected by this

Just processing the referenced object if we don't understand the activity is imo a bad idea and likely to break stuff: e.g. consider a malformed Delete which might now end up actually inserting the object instead of deleting it It can also end up breaking #845 *(which i also haven’t reviewed yet)* similarly the changed return signature from `:error` to a tuple `{:error, {.., ..}` needs some attention to check whether anything is affected by this
Author
Contributor

the issue is that when fetching other things (e.g. pins, we get objects directly. akkoma just wraps things in an activity no matter what, so we get an activity that's inside an activity)

the issue is that when fetching other things (e.g. pins, we get objects directly. akkoma just wraps things in an activity no matter what, so we get an activity that's inside an activity)
Author
Contributor

this arises from https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/lib/pleroma/object/fetcher.ex#L150 where we wrap everything in a fake activity

we can modify said fake create activity to have some sort of indication that it is fake

this arises from https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/lib/pleroma/object/fetcher.ex#L150 where we wrap everything in a fake activity we can modify said fake create activity to have some sort of indication that it is fake
Author
Contributor

adding such an indication in the next commit

adding such an indication in the next commit
Owner

I don't like yet another "fake activity" marker (We already have one) with different semantics. Instead, the fetch logic should be changed to only wrap objects which actually need this

Both this and #845 could use a new fetchable_objects constant

I don't like yet another "fake activity" marker *(We already have one)* with different semantics. Instead, the fetch logic should be changed to only wrap objects which actually need this Both this and #845 could use a new `fetchable_objects` constant
Author
Contributor

We already have one?

Where? I couldn't find it personally, but I'm also pretty new to the codebase

> We already have one? Where? I couldn't find it personally, but I'm also pretty new to the codebase
Owner

Multiple actually:

"pleroma:fake_object_id"
"pleroma:fakeid"
"pleroma:fakecontext"

and also a "fake" => true property sometimes.

Furthermore, Create activities for fetched remote posts have a null id since a few years or so (and fixing a fallout from this keeps getting defered due to priority and time, see #712 )
The need to deal with them in al subsequent places makes things messy and imo they ideally shouldn't exist at all.
Unfortunately some of this is hard to fix due to fundamental design choices from early days (e.g. requiring the presence of a Create activity for all viewable objects).

Fortunately for what you’re trying to do here, it’s easy enough to just not create unecessary activities in the first place, avoiding all of this.

Multiple actually: ``` "pleroma:fake_object_id" "pleroma:fakeid" "pleroma:fakecontext" ``` and also a `"fake" => true` property sometimes. Furthermore, `Create` activities for fetched remote posts have a `null` id since a few years or so *(and fixing a fallout from this keeps getting defered due to priority and time, see https://akkoma.dev/AkkomaGang/akkoma/issues/712 )* The need to deal with them in al subsequent places makes things messy and imo they ideally shouldn't exist at all. Unfortunately some of this is hard to fix due to fundamental design choices from early days *(e.g. requiring the presence of a `Create` activity for all viewable objects)*. Fortunately for what you’re trying to do here, it’s easy enough to just not create unecessary activities in the first place, avoiding all of this.
Author
Contributor

Fortunately for what you’re trying to do here, it’s easy enough to just not create unecessary activities in the first place, avoiding all of this.

Wouldn't I need to completely rewrite the function around https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/lib/pleroma/object/fetcher.ex#L150 for that? Or at the very least completely bypass the function?

I can definitely switch to using "fake" => true or something. Or I can check if the ID is null

> Fortunately for what you’re trying to do here, it’s easy enough to just not create unecessary activities in the first place, avoiding all of this. Wouldn't I need to completely rewrite the function around https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/lib/pleroma/object/fetcher.ex#L150 for that? Or at the very least completely bypass the function? I can definitely switch to using "fake" => true or something. Or I can check if the ID is null
Author
Contributor

Instead, the fetch logic should be changed to only wrap objects which actually need this

I just realized this can actually be done relatively easily and will be implementing it now

> Instead, the fetch logic should be changed to only wrap objects which actually need this I just realized this can actually be done relatively easily and will be implementing it now
tudbut marked this conversation as resolved
@ -306,6 +306,8 @@ def show(%{assigns: %{user: for_user}} = conn, %{id: nickname_or_id} = params) d
def statuses(%{assigns: %{user: reading_user}} = conn, params) do
with %User{} = user <- User.get_cached_by_nickname_or_id(params.id, for: reading_user),
:visible <- User.visible_for(user, reading_user) do
ActivityPub.enqueue_outbox_fetches(user)
Owner

imho this should be limited to, or there should at least be an option to limit this to, authenticated requests only. Processing new posts entailing pulling in their whole threads, new refeenced users and their pinned (now generally latest) posts and their threads and so on can become quite costly

Also no need to enquee fetched if the viewed profile is a local user

Furthermore, there prob should be a timeout (e.g. 60 min) so we don't constantly spam outbox fetches for popular remote users

imho this should be limited to, or there should at least be an option to limit this to, authenticated requests only. Processing new posts entailing pulling in their whole threads, new refeenced users and their ~~pinned~~ *(now generally latest)* posts and their threads and so on can become quite costly Also no need to enquee fetched if the viewed profile is a local user Furthermore, there prob should be a timeout *(e.g. 60 min)* so we don't constantly spam outbox fetches for popular remote users
Author
Contributor

we do always fetch the outbox, but activities in it are still cached. a timeout may make sense either way.

local users never get an outbox address property, its only added to the user object when its a remote one. so technically we're already not fetching local outboxes. making that more explicit makes sense tho

we do always fetch the outbox, but activities in it are still cached. a timeout may make sense either way. local users never get an outbox address property, its only added to the user object when its a remote one. so technically we're already not fetching local outboxes. making that more explicit makes sense tho
Owner

we do always fetch the outbox, but activities in it are still cached.

This is insufficent, since:

  • it still spams remote servers with many requests (due to default uniqueness job constraint: 1 request per second and per remote user); this is not nice on remote servers and might aalso end up creating federation problems for us if we get timeouted due to it
  • it creates unecessary Oban job churn (yes this can matter for perf and also increases PostgreSQL bloat from deleted and not-yet-vacuumed rows; more on that in a future PR once i get to finish what i have on disk)

And also doesn’t address the limitation to authenticated users only. As I mentioned this can be costly and I don’t want unauthenticated users browsing user profiles to ever be able to create such a significant workload for the instance for profiles actual local users might never even look at. (Letting unauthenticated viewers trigger this also exacerbates the request spam issue)

> we do always fetch the outbox, but activities in it are still cached. This is insufficent, since: - it still spams remote servers with many requests *(due to default uniqueness job constraint: 1 request per second and per remote user)*; this is not nice on remote servers and might aalso end up creating federation problems for us if we get timeouted due to it - it creates unecessary Oban job churn *(yes this can matter for perf and also increases PostgreSQL bloat from deleted and not-yet-vacuumed rows; more on that in a future PR once i get to finish what i have on disk)* And also doesn’t address the limitation to authenticated users only. As I mentioned this can be costly and I don’t want unauthenticated users browsing user profiles to ever be able to create such a significant workload for the instance for profiles actual local users might never even look at. *(Letting unauthenticated viewers trigger this also exacerbates the request spam issue)*
Author
Contributor

I've made it only fetch once per minute per user, and ignore local users. If one minute is too small, I propose making this a setting, though I'm not familiar with how to do that at this point. 10 mins should also be fine to hard-code, but an hour feels a little much.

I've made it only fetch once per minute per user, and ignore local users. If one minute is too small, I propose making this a setting, though I'm not familiar with how to do that at this point. 10 mins should also be fine to hard-code, but an hour feels a little much.
Owner

The timeout mechanism seems like it should work and uses mechansims you’re familiar with
Though this much additional DB traffic to frequently update timestamp might (untested) negatively affect overall perf. Given the cooldown period is well within typicall continuous-uptime, I was thinking of just using a runtime-only hint in caches; see e.g. #762 for an example of a (new) cachex set being used for timeouts

To add the restriction for logged-in users, see e.g. lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex’s public/2 function

If this is too much, we can also adjust those bits later before merge (though it might take a while)

The timeout mechanism seems like it should work and uses mechansims you’re familiar with Though this much additional DB traffic to frequently update timestamp might *(untested)* negatively affect overall perf. Given the cooldown period is well within typicall continuous-uptime, I was thinking of just using a runtime-only hint in caches; see e.g. #762 for an example of a (new) `cachex` set being used for timeouts To add the restriction for logged-in users, see e.g. `lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex`’s `public/2` function If this is too much, we can also adjust those bits later before merge *(though it might take a while)*
Author
Contributor

I have now added the cachex bit. Ill be pushing this once i have verified it still works, and will next be looking at the logged in users restriction.

I have now added the cachex bit. Ill be pushing this once i have verified it still works, and will next be looking at the logged in users restriction.
Author
Contributor

Okay! Hope this is good now (no more when not logged in, cachex for timeouts). If so, I think that would be all in terms of what you requested, but let me know if I've missed anything!

Okay! Hope this is good now (no more when not logged in, cachex for timeouts). If so, I think that would be all in terms of what you requested, but let me know if I've missed anything!
Owner

Great! Unfortunately it will take a bit before i can take a proper look at everything again, but i posted some quick remarks on the new commits already

Great! Unfortunately it will take a bit before i can take a proper look at everything again, but i posted some quick remarks on the new commits already
@ -0,0 +6,4 @@
add_if_not_exists(:outbox, :text)
end
execute("update users set outbox = replace(inbox, 'inbox', 'outbox')")
Owner

This does not generally hold

This does not generally hold
Author
Contributor

im aware. but for migrating instances, no outboxes will be populated at first so this is kinda the only solution if you want previously fetched users to have the property

im aware. but for migrating instances, no outboxes will be populated at first so this is kinda the only solution if you want previously fetched users to have the property
Author
Contributor

any idea how to do this more accurately?

any idea how to do this more accurately?
Owner

don’t populate it and wait for the next user refresh to occur or Update activity to arrive filling outbox with the correct value

don’t populate it and wait for the next user refresh to occur or Update activity to arrive filling outbox with the correct value
Owner

(also this breaks for e.g. users named "inbox")

(also this breaks for e.g. users named "inbox")
tudbut marked this conversation as resolved
Author
Contributor

@Oneric i made some changes but i also still have a few questions about part of your feedback (feel free to take your time, but just in case you havent seen it yet)

@Oneric i made some changes but i also still have a few questions about part of your feedback (feel free to take your time, but just in case you havent seen it yet)
Author
Contributor

okay so since i have quite strong unmedicated ADHD, fixing the issues with this will probably take me a while since my motivation has disappeared now.

ill convert this to a draft for now and continue when i can either get medicated (only a few more months i think) or maybe by some miracle ill be bored/motivated enough to continue (train journeys may help here)

okay so since i have quite strong unmedicated ADHD, fixing the issues with this will probably take me a while since my motivation has disappeared now. ill convert this to a draft for now and continue when i can either get medicated (only a few more months i think) or maybe by some miracle ill be bored/motivated enough to continue (train journeys may help here)
tudbut changed title from Add backfilling of posts to WIP: Add backfilling of posts 2024-11-02 01:06:34 +00:00
Owner

Sorry if my curt comments yesterday ended up sounding harsh; I’m stretched thin on time atm and just quickly pointed out whatever seemed in need of further change

Sorry if my curt comments yesterday ended up sounding harsh; I’m stretched thin on time atm and just quickly pointed out whatever seemed in need of further change
Author
Contributor

Sorry if my curt comments yesterday ended up sounding harsh; I’m stretched thin on time atm and just quickly pointed out whatever seemed in need of further change

Oh dont worry, its not related to you. I just lose motivation after doing one project for a few days. And itll return, just takes a while

> Sorry if my curt comments yesterday ended up sounding harsh; I’m stretched thin on time atm and just quickly pointed out whatever seemed in need of further change Oh dont worry, its not related to you. I just lose motivation after doing one project for a few days. And itll return, just takes a while
tudbut reviewed 2024-11-03 20:14:27 +00:00
@ -1839,0 +1852,4 @@
{true} <-
{last_fetch > now ||
last_fetch < now - 60}
) do
Author
Contributor

I'm not too familiar with elixir yet - Is this idiomatic?

I'm not too familiar with elixir yet - Is this idiomatic?
Owner

Wrnagling the fallback for nil into a sure-to-fail NaiveDateTime will be akward using with, a small function is probably better:

defp need_outbox_refresh?(last_fetch)

defp need_outbox_refresh?(nil), do: true

defp need_outbox_refresh?(%NaiveDateTime{} = last_fetch) do
    NaiveDateTime.diff(NaiveDateTime.utc_now(), last_fetch) > Config.get!([:activitypub, :outbox_refetch_cooldown])
end

Also for the current version:

last_fetch <- last_fetch || 0
Wrnagling the fallback for `nil` into a sure-to-fail `NaiveDateTime` will be akward using `with`, a small function is probably better: ```elixir defp need_outbox_refresh?(last_fetch) defp need_outbox_refresh?(nil), do: true defp need_outbox_refresh?(%NaiveDateTime{} = last_fetch) do NaiveDateTime.diff(NaiveDateTime.utc_now(), last_fetch) > Config.get!([:activitypub, :outbox_refetch_cooldown]) end ``` Also for the current version: ```elixir last_fetch <- last_fetch || 0 ```
Owner

wrt to typical styling: just run mix format and code style will be adjusted automatically

wrt to typical styling: just run `mix format` and code style will be adjusted automatically
tudbut marked this conversation as resolved
tudbut force-pushed develop from 3e4424684f to fdaff0a70b 2024-11-03 21:43:54 +00:00 Compare
tudbut added 1 commit 2024-11-03 21:49:07 +00:00
add last_outbox_fetch to migration
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
ee1f8938fc
tudbut force-pushed develop from 488d4b7c2e to 73e587ed9b 2024-11-03 22:15:00 +00:00 Compare
tudbut force-pushed develop from 73e587ed9b to e37ffa1b1f 2024-11-03 22:17:25 +00:00 Compare
tudbut added 1 commit 2024-11-03 22:50:51 +00:00
fix last_outbox_fetch
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
718dae0f97
Author
Contributor

phew sorry for the commit spam, ill probably end up squashing all that later

phew sorry for the commit spam, ill probably end up squashing all that later
Author
Contributor

@floatingghost

Your recently added migration breaks due to trying to parse users, but at that point not all columns are set up. This won't happen in prod at this point because there's no migrations afterwards, but just as an early report, I'm putting it here.

So far i'm just moving the migration after mine to fix it for my instances, but this will cause issues in the future if left in.

00:13:25.633 [info] == Running 20240625220752 Pleroma.Repo.Migrations.MoveSigningKeys.up/0 forward
** (Postgrex.Error) ERROR 42703 (undefined_column) column u0.outbox does not exist

    query: SELECT u0."id", u0."bio", u0."raw_bio", u0."email", u0."name", u0."nickname", u0."password_hash", u0."keys", u0."ap_id", u0."avatar", u0."local", u0."follower_address", u0."following_address", u0."featured_address", u0."tags", u0."last_refreshed_at", u0."last_digest_emailed_at", u0."banner", u0."background", u0."note_count", u0."follower_count", u0."following_count", u0."is_locked", u0."is_confirmed", u0."password_reset_pending", u0."is_approved", u0."registration_reason", u0."confirmation_token", u0."default_scope", u0."domain_blocks", u0."is_active", u0."no_rich_text", u0."ap_enabled", u0."is_moderator", u0."is_admin", u0."show_role", u0."mastofe_settings", u0."uri", u0."hide_followers_count", u0."hide_follows_count", u0."hide_followers", u0."hide_follows", u0."hide_favorites", u0."email_notifications", u0."mascot", u0."emoji", u0."pleroma_settings_store", u0."fields", u0."raw_fields", u0."is_discoverable", u0."invisible", u0."allow_following_move", u0."skip_thread_containment", u0."actor_type", u0."also_known_as", u0."inbox", u0."shared_inbox", u0."outbox", u0."last_outbox_fetch", u0."last_active_at", u0."disclose_client", u0."pinned_objects", u0."is_suggested", u0."last_status_at", u0."language", u0."status_ttl_days", u0."permit_followback", u0."accepts_direct_messages_from", u0."notification_settings", u0."multi_factor_authentication_settings", u0."inserted_at", u0."updated_at" FROM "users" AS u0 WHERE (u0."local" = TRUE)

    hint: Perhaps you meant to reference the column "u0.inbox".
    (db_connection 2.7.0) lib/db_connection.ex:1509: DBConnection.prepare_declare!/4
    (db_connection 2.7.0) lib/db_connection.ex:1249: anonymous fn/4 in DBConnection.reduce/3
    (elixir 1.17.3) lib/stream.ex:1675: anonymous fn/5 in Stream.resource/3
    (elixir 1.17.3) lib/stream.ex:1891: Enumerable.Stream.do_each/4
    (elixir 1.17.3) lib/stream.ex:943: Stream.do_transform/5
    (elixir 1.17.3) lib/stream.ex:1891: Enumerable.Stream.do_each/4
    (elixir 1.17.3) lib/enum.ex:4423: Enum.each/2
    (ecto_sql 3.10.2) lib/ecto/migration/runner.ex:289: Ecto.Migration.Runner.perform_operation/3
@floatingghost Your recently added migration breaks due to trying to parse users, but at that point not all columns are set up. This won't happen in prod at this point because there's no migrations afterwards, but just as an early report, I'm putting it here. So far i'm just moving the migration after mine to fix it for my instances, but this will cause issues in the future if left in. ``` 00:13:25.633 [info] == Running 20240625220752 Pleroma.Repo.Migrations.MoveSigningKeys.up/0 forward ** (Postgrex.Error) ERROR 42703 (undefined_column) column u0.outbox does not exist query: SELECT u0."id", u0."bio", u0."raw_bio", u0."email", u0."name", u0."nickname", u0."password_hash", u0."keys", u0."ap_id", u0."avatar", u0."local", u0."follower_address", u0."following_address", u0."featured_address", u0."tags", u0."last_refreshed_at", u0."last_digest_emailed_at", u0."banner", u0."background", u0."note_count", u0."follower_count", u0."following_count", u0."is_locked", u0."is_confirmed", u0."password_reset_pending", u0."is_approved", u0."registration_reason", u0."confirmation_token", u0."default_scope", u0."domain_blocks", u0."is_active", u0."no_rich_text", u0."ap_enabled", u0."is_moderator", u0."is_admin", u0."show_role", u0."mastofe_settings", u0."uri", u0."hide_followers_count", u0."hide_follows_count", u0."hide_followers", u0."hide_follows", u0."hide_favorites", u0."email_notifications", u0."mascot", u0."emoji", u0."pleroma_settings_store", u0."fields", u0."raw_fields", u0."is_discoverable", u0."invisible", u0."allow_following_move", u0."skip_thread_containment", u0."actor_type", u0."also_known_as", u0."inbox", u0."shared_inbox", u0."outbox", u0."last_outbox_fetch", u0."last_active_at", u0."disclose_client", u0."pinned_objects", u0."is_suggested", u0."last_status_at", u0."language", u0."status_ttl_days", u0."permit_followback", u0."accepts_direct_messages_from", u0."notification_settings", u0."multi_factor_authentication_settings", u0."inserted_at", u0."updated_at" FROM "users" AS u0 WHERE (u0."local" = TRUE) hint: Perhaps you meant to reference the column "u0.inbox". (db_connection 2.7.0) lib/db_connection.ex:1509: DBConnection.prepare_declare!/4 (db_connection 2.7.0) lib/db_connection.ex:1249: anonymous fn/4 in DBConnection.reduce/3 (elixir 1.17.3) lib/stream.ex:1675: anonymous fn/5 in Stream.resource/3 (elixir 1.17.3) lib/stream.ex:1891: Enumerable.Stream.do_each/4 (elixir 1.17.3) lib/stream.ex:943: Stream.do_transform/5 (elixir 1.17.3) lib/stream.ex:1891: Enumerable.Stream.do_each/4 (elixir 1.17.3) lib/enum.ex:4423: Enum.each/2 (ecto_sql 3.10.2) lib/ecto/migration/runner.ex:289: Ecto.Migration.Runner.perform_operation/3 ```
Owner

recently added migration breaks due to trying to parse users, but at that point not all columns are set up.

whoops, good catch!
I think we can avoid parsing the Suer struct at all here by just selecting the fields we need in the initial query

> recently added migration breaks due to trying to parse users, but at that point not all columns are set up. whoops, good catch! I think we can avoid parsing the `Suer` struct at all here by just `select`ing the fields we need in the initial query
tudbut added 1 commit 2024-11-26 22:08:31 +00:00
Merge branch 'develop' into develop
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
4e5e947f26
tudbut added 1 commit 2024-11-26 22:16:56 +00:00
better control flow to check outbox refetch
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
ee0370089b
Author
Contributor

recently added migration breaks due to trying to parse users, but at that point not all columns are set up.

whoops, good catch!
I think we can avoid parsing the Suer struct at all here by just selecting the fields we need in the initial query

this is still happening and makes developing noticeably harder

> > recently added migration breaks due to trying to parse users, but at that point not all columns are set up. > > whoops, good catch! > I think we can avoid parsing the `Suer` struct at all here by just `select`ing the fields we need in the initial query > this is still happening and makes developing noticeably harder
tudbut added 2 commits 2024-12-03 20:19:13 +00:00
actually add config value
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
273ac2c380
Owner

this is still happening and makes developing noticeably harder

The following patch should fix it; you can insert it at the start:

Fix SigningKey migration patch
From d7e0005edc46f652652cdecb95f2c5c651183377 Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Sun, 1 Dec 2024 00:00:00 +0100
Subject: [PATCH] Make SigningKey data migration future-proof

Bug originally discovered by tudbut
---
 .../migrations/20240625220752_move_signing_keys.exs | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/priv/repo/migrations/20240625220752_move_signing_keys.exs b/priv/repo/migrations/20240625220752_move_signing_keys.exs
index 9104b7c29..f5569ce09 100644
--- a/priv/repo/migrations/20240625220752_move_signing_keys.exs
+++ b/priv/repo/migrations/20240625220752_move_signing_keys.exs
@@ -8,13 +8,14 @@ def up do
     # we do not handle remote users here!
     # because we want to store a key id -> user id mapping, and we don't
     # currently store key ids for remote users...
-    query =
-      from(u in User)
-      |> where(local: true)
-
-    Repo.stream(query, timeout: :infinity)
+    # Also this MUST use select, else the migration will fail in future installs with new user fields!
+    from(u in Pleroma.User,
+      where: u.local == true,
+      select: {u.id, u.keys, u.ap_id}
+    )
+    |> Repo.stream(timeout: :infinity)
     |> Enum.each(fn
-      %User{id: user_id, keys: private_key, local: true, ap_id: ap_id} ->
+      {user_id, private_key, ap_id} ->
         IO.puts("Migrating user #{user_id}")
         # we can precompute the public key here...
         # we do use it on every user view which makes it a bit of a dos attack vector
-- 
2.39.5
> this is still happening and makes developing noticeably harder The following patch should fix it; you can insert it at the start: <details> <summary>Fix SigningKey migration patch</summary> ```patch From d7e0005edc46f652652cdecb95f2c5c651183377 Mon Sep 17 00:00:00 2001 From: Oneric <oneric@oneric.stub> Date: Sun, 1 Dec 2024 00:00:00 +0100 Subject: [PATCH] Make SigningKey data migration future-proof Bug originally discovered by tudbut --- .../migrations/20240625220752_move_signing_keys.exs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/priv/repo/migrations/20240625220752_move_signing_keys.exs b/priv/repo/migrations/20240625220752_move_signing_keys.exs index 9104b7c29..f5569ce09 100644 --- a/priv/repo/migrations/20240625220752_move_signing_keys.exs +++ b/priv/repo/migrations/20240625220752_move_signing_keys.exs @@ -8,13 +8,14 @@ def up do # we do not handle remote users here! # because we want to store a key id -> user id mapping, and we don't # currently store key ids for remote users... - query = - from(u in User) - |> where(local: true) - - Repo.stream(query, timeout: :infinity) + # Also this MUST use select, else the migration will fail in future installs with new user fields! + from(u in Pleroma.User, + where: u.local == true, + select: {u.id, u.keys, u.ap_id} + ) + |> Repo.stream(timeout: :infinity) |> Enum.each(fn - %User{id: user_id, keys: private_key, local: true, ap_id: ap_id} -> + {user_id, private_key, ap_id} -> IO.puts("Migrating user #{user_id}") # we can precompute the public key here... # we do use it on every user view which makes it a bit of a dos attack vector -- 2.39.5 ``` </details>
Author
Contributor

should i add that into the PR or will it be pushed to develop soon? id just merge from that if so

should i add that into the PR or will it be pushed to develop soon? id just merge from that if so
Owner

I do not know when it will be pushed to develop, but since you can easily drop the patch again later if needed with git rebase -i develop i’d personally just add it here for now

I do not know when it will be pushed to develop, but since you can easily drop the patch again later if needed with `git rebase -i develop` i’d personally just add it here for now
Owner

@tudbut: the data migration fix was pushed yesterday

@tudbut: the data migration fix was pushed yesterday
tudbut added 1 commit 2024-12-09 20:54:21 +00:00
Merge branch 'develop' into develop
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
e98555f767
tudbut added 1 commit 2025-01-05 05:23:38 +00:00
dont reverse the outbox when fetching it
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
6d9fda9a24
tudbut added 2 commits 2025-01-17 19:27:11 +00:00
only backfetch when logged in
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
ci/woodpecker/pr/docs Pipeline is pending approval
ci/woodpecker/pr/lint Pipeline is pending approval
ci/woodpecker/pr/test Pipeline is pending approval
92e9ca0faf
tudbut changed title from WIP: Add backfilling of posts to Add backfilling of posts 2025-01-19 02:03:38 +00:00
Oneric reviewed 2025-01-21 23:14:45 +00:00
@ -1857,0 +1868,4 @@
@timer,
outbox_address,
true,
ttl: Config.get!([:activitypub, :outbox_refetch_cooldown])
Owner

hmm.. i imagine some will want to disable this completely and intuitively a cooldown of :infinity makes sense for this. As is :infinity won't prevent fetches though as at least one fetch will happen before there’s a cache entry. Perhaps check for :infinity before the cache look up

hmm.. i imagine some will want to disable this completely and intuitively a cooldown of `:infinity` makes sense for this. As is `:infinity` won't prevent fetches though as at least one fetch will happen before there’s a cache entry. Perhaps check for `:infinity` before the cache look up
Author
Contributor

good idea, will do!

good idea, will do!
tudbut marked this conversation as resolved
@ -1857,0 +1889,4 @@
def enqueue_outbox_fetches(%{local: false} = user) do
make_user_from_ap_id(user.ap_id)
:ok
Owner

Technically AP spec requires all actors to provide at least an inbox and outbox and this should never happen (well, .. except for old, by now unreachable, pre-AP users). If an implementation decided to omit outbox though due to having little relevance in Server2Server interaction, this would end up refetching the user each time the account statuses are queried — including duplicates for simultaneous API calls. That’s not nice on remote servers

While still not ideal (no dedupe yet), the common approach in the codebase is to use User.get_or_fetch_by_ap_id. It will refresh the user with a built in cooldown *(by default 1 day, can be changed iwth maximum_age: parameter

Technically AP spec requires all actors to provide at least an inbox *and* outbox and this should never happen *(well, .. except for old, by now unreachable, pre-AP users)*. If an implementation decided to omit `outbox` though due to having little relevance in Server2Server interaction, this would end up refetching the user each time the account statuses are queried — including duplicates for simultaneous API calls. That’s not nice on remote servers While still not ideal *(no dedupe *yet*)*, the common approach in the codebase is to use `User.get_or_fetch_by_ap_id`. It will refresh the user with a built in cooldown *(by default 1 day, can be changed iwth `maximum_age: ` parameter
Author
Contributor

good catch! luckily an easy fix :D
will do tomorrow

good catch! luckily an easy fix :D will do tomorrow
tudbut marked this conversation as resolved
@ -309,0 +312,4 @@
_ ->
ActivityPub.enqueue_outbox_fetches(user)
end
Owner

style nit: this can be simplified to an if without else

style nit: this can be simplified to an `if` without `else`
Author
Contributor

how did i miss that! probably forgot that if statements exist or something :P

how did i miss that! probably forgot that if statements exist or something :P
tudbut marked this conversation as resolved
Oneric reviewed 2025-02-10 20:30:20 +00:00
Oneric left a comment
Owner

Most comments are just informational remarks or minor nits.
The biggest remaining hurdle appears to be avoiding repetitive errors from trying to reinsert activities without performance impact; not sure what the best course of action is.

I was originally also concerned about whether fetch-specific security checks after retrieval hold up when dealing with activities since so far the code only had to handle terminal objects, but it seems like it’s fine.

Most comments are just informational remarks or minor nits. The biggest remaining hurdle appears to be avoiding repetitive errors from trying to reinsert activities without performance impact; not sure what the best course of action is. I was originally also concerned about whether fetch-specific security checks after retrieval hold up when dealing with activities since so far the code only had to handle terminal objects, but it *seems* like it’s fine.
@ -373,2 +373,3 @@
authorized_fetch_mode: false,
max_collection_objects: 50
max_collection_objects: 50,
outbox_refetch_cooldown: 5 * 60
Owner

note: 5min still seems too short to me as a default, but ultimately this is up to floati to decide

note: 5min still seems too short to me as a default, but ultimately this is up to floati to decide
Author
Contributor

ill do 30 mins as default

ill do 30 mins as default
tudbut marked this conversation as resolved
@ -1589,2 +1589,3 @@
featured_address = data["featured"]
{:ok, pinned_objects} = fetch_and_prepare_featured_from_ap_id(featured_address)
{:ok, pinned_objects} = fetch_and_prepare_collection_from_ap_id(featured_address)
outbox_address = data["outbox"]
Owner

note: the preëxisting {:ok, _} = pattern match here causes the process to crash if an error occurs while fetching the collection, which is undesireable if the process also did anything other than fetching this collection

I think i fixed this already either in #862 or some not-yet published patches building further on top of it (but it’s hard to keep track of so many intedependend patch trees), so prepare for merge conflicts possibly showing up here

note: the preëxisting `{:ok, _} = ` pattern match here causes the process to crash if an error occurs while fetching the collection, which is undesireable if the process also did anything other than fetching this collection I *think* i fixed this already either in #862 or some not-yet published patches building further on top of it *(but it’s hard to keep track of so many intedependend patch trees)*, so prepare for merge conflicts possibly showing up here
Author
Contributor

thanks, but i dont fear merge conflicts anymore :p

thanks, but i dont fear merge conflicts anymore :p
@ -1857,0 +1879,4 @@
})
a ->
Logger.debug("Not refetching outbox (TTL not reached)")
Owner

a is unsused creating a compiler warning (this should be the same in your elixir version).
Instead of a variable use an underscore _ as a generic placeholder

`a` is unsused creating a compiler warning *(this should be the same in your elixir version)*. Instead of a variable use an underscore `_` as a generic placeholder
Author
Contributor

Yeah i was using it for debug output but then removed that.

Yeah i was using it for debug output but then removed that.
tudbut marked this conversation as resolved
@ -308,1 +308,4 @@
:visible <- User.visible_for(user, reading_user) do
case reading_user do
nil -> {}
Owner

This will be replaced with an if anyway but for future reference:

mix format:
If one branch uses multiple lines, all branches must break condition and body by a newline

diff --git a/lib/pleroma/web/mastodon_api/controllers/account_controller.ex b/lib/pleroma/web/mastodon_api/controllers/account_controller.ex
index 8a9b419f9..cf98d91bf 100644
--- a/lib/pleroma/web/mastodon_api/controllers/account_controller.ex
+++ b/lib/pleroma/web/mastodon_api/controllers/account_controller.ex
@@ -306,9 +306,9 @@ def show(%{assigns: %{user: for_user}} = conn, %{id: nickname_or_id} = params) d
   def statuses(%{assigns: %{user: reading_user}} = conn, params) do
     with %User{} = user <- User.get_cached_by_nickname_or_id(params.id, for: reading_user),
          :visible <- User.visible_for(user, reading_user) do
-
       case reading_user do
-        nil -> {}
+        nil ->
+          {}

         _ ->
           ActivityPub.enqueue_outbox_fetches(user)
This will be replaced with an `if` anyway but for future reference: **`mix format`**: If one branch uses multiple lines, all branches must break condition and body by a newline ```diff diff --git a/lib/pleroma/web/mastodon_api/controllers/account_controller.ex b/lib/pleroma/web/mastodon_api/controllers/account_controller.ex index 8a9b419f9..cf98d91bf 100644 --- a/lib/pleroma/web/mastodon_api/controllers/account_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/account_controller.ex @@ -306,9 +306,9 @@ def show(%{assigns: %{user: for_user}} = conn, %{id: nickname_or_id} = params) d def statuses(%{assigns: %{user: reading_user}} = conn, params) do with %User{} = user <- User.get_cached_by_nickname_or_id(params.id, for: reading_user), :visible <- User.visible_for(user, reading_user) do - case reading_user do - nil -> {} + nil -> + {} _ -> ActivityPub.enqueue_outbox_fetches(user) ```
Author
Contributor

replaced by if !is_nil

replaced by if !is_nil
tudbut marked this conversation as resolved
@ -13,0 +18,4 @@
Enum.each(outbox, fn {ap_id, _} ->
if is_nil(Object.get_cached_by_ap_id(ap_id)) do
Fetcher.fetch_object_from_id(ap_id, depth: 1)
end
Owner

Fetcher.fetch_object_from_id already checks whether the object is already locally known, no need to check again here.
Neither checks whether an activity of this AP id is already known though which will likely blow up the amount of already known error logs. But activity lookups aren't cached and thus costly atm (most lookups also don't directly use the id but its object property anyway)...

`Fetcher.fetch_object_from_id` already checks whether the object is already locally known, no need to check again here. Neither checks whether an *activity* of this AP id is already known though which will likely blow up the amount of `already known` error logs. But activity lookups aren't cached and thus costly atm *(most lookups also don't directly use the `id` but its `object` property anyway)*...
Author
Contributor

awesome, thank you. simplified to remove conditional

awesome, thank you. simplified to remove conditional
tudbut marked this conversation as resolved
@ -0,0 +3,4 @@
def up do
alter table(:users) do
add_if_not_exists(:outbox, :text)
Owner

This is likely the default anyway, but for consistency with the definition in user.ex it’d be good to define the nil defaut

This is likely the default anyway, but for consistency with the definition in `user.ex` it’d be good to define the `nil` defaut
Author
Contributor

i took example from users_add_inboxes: image

and it doesnt seem like an argument specifying default exists for the function

i took example from users_add_inboxes: ![image](/attachments/3ab6f1cf-a708-4f4b-97c6-7e7ce39e6b04) and it doesnt seem like an argument specifying default exists for the function
Author
Contributor

found default parameter in documentation :D

found default parameter in documentation :D
tudbut marked this conversation as resolved
@ -0,0 +7,4 @@
file # libmagic
ffmpeg
];
}
Owner

If this was just added because it shows up as a new file you can add shell.nix to a user-side .gitignore

If this was just added because it shows up as a new file you can add shell.nix to a [user-side .gitignore](https://git-scm.com/docs/git-config#Documentation/git-config.txt-coreexcludesFile)
Author
Contributor

i do think it makes sense to have it, but its obviously external to the PR's subject. ill remove it if desired

i do think it makes sense to have it, but its obviously external to the PR's subject. ill remove it if desired
Owner

Also 3 tests are failing, make sure to run mix test || mix test --failed and fix any errors:

Running ExUnit with seed: 222752, max_cases: 32
Excluding tags: [:federated, :erratic]

     warning: Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id/1 is undefined or private. Did you mean:

           * fetch_and_prepare_collection_from_ap_id/1
           * fetch_and_prepare_user_from_ap_id/1
           * fetch_and_prepare_user_from_ap_id/2

     │
 389 │     {:ok, data} = ActivityPub.fetch_and_prepare_featured_from_ap_id(featured_url)
     │                               ~
     │
     └─ test/pleroma/web/activity_pub/activity_pub_test.exs:389:31: Pleroma.Web.ActivityPub.ActivityPubTest."test fetches user featured collection using the first property"/1
     └─ test/pleroma/web/activity_pub/activity_pub_test.exs:423:30: Pleroma.Web.ActivityPub.ActivityPubTest."test fetches user featured when it has string IDs"/1

      warning: Pleroma.Web.ActivityPub.ActivityPub.pin_data_from_featured_collection/1 is undefined or private
      │
 2667 │              ActivityPub.pin_data_from_featured_collection(%{
      │                          ~
      │
      └─ test/pleroma/web/activity_pub/activity_pub_test.exs:2667:26: Pleroma.Web.ActivityPub.ActivityPubTest."test pin_data_from_featured_collection will ignore unsupported values"/1



  1) test pin_data_from_featured_collection will ignore unsupported values (Pleroma.Web.ActivityPub.ActivityPubTest)
     test/pleroma/web/activity_pub/activity_pub_test.exs:2665
     ** (UndefinedFunctionError) function Pleroma.Web.ActivityPub.ActivityPub.pin_data_from_featured_collection/1 is undefined or private
     code: ActivityPub.pin_data_from_featured_collection(%{
     stacktrace:
       (pleroma 3.14.0-738-g678f8c53-tmp+test) Pleroma.Web.ActivityPub.ActivityPub.pin_data_from_featured_collection(%{"first" => "https://social.example/users/alice/collections/featured?page=true", "type" => "CollectionThatIsNotRealAndCannotHurtMe"})
       test/pleroma/web/activity_pub/activity_pub_test.exs:2667: (test)



  2) test fetches user featured collection using the first property (Pleroma.Web.ActivityPub.ActivityPubTest)
     test/pleroma/web/activity_pub/activity_pub_test.exs:355
     ** (UndefinedFunctionError) function Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id/1 is undefined or private. Did you mean:

           * fetch_and_prepare_collection_from_ap_id/1
           * fetch_and_prepare_user_from_ap_id/1
           * fetch_and_prepare_user_from_ap_id/2
     
     code: {:ok, data} = ActivityPub.fetch_and_prepare_featured_from_ap_id(featured_url)
     stacktrace:
       (pleroma 3.14.0-738-g678f8c53-tmp+test) Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id("https://friendica.example.com/featured/raha")
       test/pleroma/web/activity_pub/activity_pub_test.exs:389: (test)



  3) test fetches user featured when it has string IDs (Pleroma.Web.ActivityPub.ActivityPubTest)
     test/pleroma/web/activity_pub/activity_pub_test.exs:393
     ** (UndefinedFunctionError) function Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id/1 is undefined or private. Did you mean:

           * fetch_and_prepare_collection_from_ap_id/1
           * fetch_and_prepare_user_from_ap_id/1
           * fetch_and_prepare_user_from_ap_id/2
     
     code: {:ok, %{}} = ActivityPub.fetch_and_prepare_featured_from_ap_id(featured_url)
     stacktrace:
       (pleroma 3.14.0-738-g678f8c53-tmp+test) Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id("https://example.com/users/alisaie/collections/featured")
       test/pleroma/web/activity_pub/activity_pub_test.exs:423: (test)


Finished in 2.5 seconds (0.00s async, 2.5s sync)
3 tests, 3 failures
Also 3 tests are failing, make sure to run `mix test || mix test --failed` and fix any errors: ``` Running ExUnit with seed: 222752, max_cases: 32 Excluding tags: [:federated, :erratic] warning: Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id/1 is undefined or private. Did you mean: * fetch_and_prepare_collection_from_ap_id/1 * fetch_and_prepare_user_from_ap_id/1 * fetch_and_prepare_user_from_ap_id/2 │ 389 │ {:ok, data} = ActivityPub.fetch_and_prepare_featured_from_ap_id(featured_url) │ ~ │ └─ test/pleroma/web/activity_pub/activity_pub_test.exs:389:31: Pleroma.Web.ActivityPub.ActivityPubTest."test fetches user featured collection using the first property"/1 └─ test/pleroma/web/activity_pub/activity_pub_test.exs:423:30: Pleroma.Web.ActivityPub.ActivityPubTest."test fetches user featured when it has string IDs"/1 warning: Pleroma.Web.ActivityPub.ActivityPub.pin_data_from_featured_collection/1 is undefined or private │ 2667 │ ActivityPub.pin_data_from_featured_collection(%{ │ ~ │ └─ test/pleroma/web/activity_pub/activity_pub_test.exs:2667:26: Pleroma.Web.ActivityPub.ActivityPubTest."test pin_data_from_featured_collection will ignore unsupported values"/1 1) test pin_data_from_featured_collection will ignore unsupported values (Pleroma.Web.ActivityPub.ActivityPubTest) test/pleroma/web/activity_pub/activity_pub_test.exs:2665 ** (UndefinedFunctionError) function Pleroma.Web.ActivityPub.ActivityPub.pin_data_from_featured_collection/1 is undefined or private code: ActivityPub.pin_data_from_featured_collection(%{ stacktrace: (pleroma 3.14.0-738-g678f8c53-tmp+test) Pleroma.Web.ActivityPub.ActivityPub.pin_data_from_featured_collection(%{"first" => "https://social.example/users/alice/collections/featured?page=true", "type" => "CollectionThatIsNotRealAndCannotHurtMe"}) test/pleroma/web/activity_pub/activity_pub_test.exs:2667: (test) 2) test fetches user featured collection using the first property (Pleroma.Web.ActivityPub.ActivityPubTest) test/pleroma/web/activity_pub/activity_pub_test.exs:355 ** (UndefinedFunctionError) function Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id/1 is undefined or private. Did you mean: * fetch_and_prepare_collection_from_ap_id/1 * fetch_and_prepare_user_from_ap_id/1 * fetch_and_prepare_user_from_ap_id/2 code: {:ok, data} = ActivityPub.fetch_and_prepare_featured_from_ap_id(featured_url) stacktrace: (pleroma 3.14.0-738-g678f8c53-tmp+test) Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id("https://friendica.example.com/featured/raha") test/pleroma/web/activity_pub/activity_pub_test.exs:389: (test) 3) test fetches user featured when it has string IDs (Pleroma.Web.ActivityPub.ActivityPubTest) test/pleroma/web/activity_pub/activity_pub_test.exs:393 ** (UndefinedFunctionError) function Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id/1 is undefined or private. Did you mean: * fetch_and_prepare_collection_from_ap_id/1 * fetch_and_prepare_user_from_ap_id/1 * fetch_and_prepare_user_from_ap_id/2 code: {:ok, %{}} = ActivityPub.fetch_and_prepare_featured_from_ap_id(featured_url) stacktrace: (pleroma 3.14.0-738-g678f8c53-tmp+test) Pleroma.Web.ActivityPub.ActivityPub.fetch_and_prepare_featured_from_ap_id("https://example.com/users/alisaie/collections/featured") test/pleroma/web/activity_pub/activity_pub_test.exs:423: (test) Finished in 2.5 seconds (0.00s async, 2.5s sync) 3 tests, 3 failures ```
Author
Contributor

aside: i love this collection name in the tests image

aside: i love this collection name in the tests ![image](/attachments/7bdea396-74be-4bbd-ab49-c78716d1644b)
5.2 KiB
Author
Contributor

okay so i just re-discovered why i didnt use the tests: i get about a hundred failures for irrelevant reasons like these URLs or somehow broken MRF policies

image
image
image

ill push without testing it myself again

okay so i just re-discovered why i didnt use the tests: i get about a hundred failures for irrelevant reasons like these URLs or somehow broken MRF policies ![image](/attachments/d97ed1db-e5a9-434d-9d56-795315f0908b) ![image](/attachments/f960d19f-093d-426d-89ea-2a4a621b0001) ![image](/attachments/7daa6ce1-c334-4b6e-8c21-89d1ab603448) ill push without testing it myself again
tudbut added 1 commit 2025-02-11 22:56:06 +00:00
Author
Contributor

this push includes a mix format which changed a few files that aren't related. i had been avoiding this before by restoring such files on every format but i hope its fine to just commit them

this push includes a `mix format` which changed a few files that aren't related. i had been avoiding this before by restoring such files on every format but i hope its fine to just commit them
Owner

re-discovered why i didnt use the tests: i get about a hundred failures

this looks like the test env isn’t applied for some reason. Are you manually overriding MIX_ENV while running mix test || mix test --failed?

this push includes a mix format which changed a few files that aren't related. i had been avoiding this before by restoring such files on every format but i hope its fine to just commit them

Please do not reformat any preëxisting code unrelated to the PR. Everything should already be mix format clean. If you’re using an older elixir version the format guidelines might differ from what our canonical dev version used in CI expects; changing it will just cause CI failures later

i love this collection name in the tests
CollectionThatIsNotRealAndCannotHurtMe

my favourite kind of collection ^^

> re-discovered why i didnt use the tests: i get about a hundred failures this looks like the test env isn’t applied for some reason. Are you manually overriding `MIX_ENV` while running `mix test || mix test --failed`? > this push includes a mix format which changed a few files that aren't related. i had been avoiding this before by restoring such files on every format but i hope its fine to just commit them Please do not reformat any preëxisting code unrelated to the PR. Everything should already be `mix format` clean. If you’re using an older elixir version the format guidelines might differ from what our canonical dev version used in CI expects; changing it will just cause CI failures later > i love this collection name in the tests > `CollectionThatIsNotRealAndCannotHurtMe` my favourite kind of collection ^^
Owner

Thinking a bit about it, I believe the best was to deal with the issue of constraint errors from reprocessing outbox activities is to remember the last processed outbox element and abort further insertion when it’s encountered again. With this check added, also querying the database to check for any pre-existing (e.g. from federation) activity shouldn't be too costly anymore.

We should not remember just any last activity from the user which e.g. arrived via federation, since then we might miss entries inbetween which were not federated and the ordering might be different. Only from outbox fetches.

This leaves the issue of whether to add the activity check to the outbox fetching specifically or to all fetches in Object.Fetcher; not sure

Thinking a bit about it, I believe the best was to deal with the issue of constraint errors from reprocessing outbox activities is to remember the last processed outbox element and abort further insertion when it’s encountered again. With this check added, also querying the database to check for any pre-existing (e.g. from federation) activity shouldn't be too costly anymore. We should _not_ remember just any last activity from the user which e.g. arrived via federation, since then we might miss entries inbetween which were not federated and the ordering might be different. Only from outbox fetches. This leaves the issue of whether to add the activity check to the outbox fetching specifically or to all fetches in `Object.Fetcher`; not sure
Oneric added the
needs change/feedback
label 2025-05-09 21:50:17 +00:00
This pull request has changes conflicting with the target branch.
  • config/config.exs
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u develop:tudbut-develop
git checkout tudbut-develop
Sign in to join this conversation.
No description provided.