Skip to content

Conversation

@BentiGorlich
Copy link
Member

@BentiGorlich BentiGorlich commented Oct 7, 2024

  • Save activities in the database so that they can be called via URL
  • Add unit tests for the JSON used in outward federation
  • Add functional tests for inbound federation
    • tests with our own JSON
    • tests with lemmy's JSON

Closes #772

@BentiGorlich BentiGorlich added enhancement New feature or request activitypub ActivityPub related issues backend Backend related issues and pull requests labels Oct 7, 2024
@BentiGorlich BentiGorlich added this to the v1.8.0 milestone Oct 7, 2024
@BentiGorlich BentiGorlich self-assigned this Oct 7, 2024
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from c444eeb to 57ec00a Compare October 7, 2024 19:14
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from cbcd898 to 49bdeaa Compare October 9, 2024 15:14
@melroy89

This comment was marked as outdated.

@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 49bdeaa to 5c5f342 Compare November 19, 2024 22:24
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from ef0cc27 to e108c22 Compare November 24, 2024 11:14
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from e108c22 to 4d6b1ec Compare December 8, 2024 23:43
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 4d6b1ec to 0ad5211 Compare December 10, 2024 11:06
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 0ad5211 to 3ce355a Compare January 7, 2025 12:02
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 3ce355a to 5729ac9 Compare January 7, 2025 12:49
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 5729ac9 to 1bc2da1 Compare January 7, 2025 13:05
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 1bc2da1 to ec9516b Compare January 7, 2025 14:32
@BentiGorlich BentiGorlich linked an issue Jan 13, 2025 that may be closed by this pull request
@BentiGorlich BentiGorlich changed the base branch from dev/new_features to main January 14, 2025 15:31
@melroy89
Copy link
Member

melroy89 commented May 26, 2025

  • add announcing of incoming reports to local magazines to all remote instances with moderators (although that might deserve it own issue and PR, what do you think @melroy89 ?)

That sounds indeed something for a separate PR. Especially if you want to limit the scope a bit of this PR in order to get it merged faster.

Personally I'm always in favor of smaller PRs, merging more, but smaller amount of changes (if possible). Like small iterations. Which also has the benefit from retrieving feedback earlier and more often, as well as leaner to adapt to new requests or changes in requirements. But I digress :D

@BentiGorlich
Copy link
Member Author

Alright, then we'll do that in a different PR and I will make an issue about it

@BentiGorlich
Copy link
Member Author

BentiGorlich commented May 28, 2025

@melroy89 one maybe open todo could be to make the new tests thread safe, but I was really trying and did the setup the way I though should work with paratest, but I really really struggled and gave up in the end. Problem is that the CI duration is now up to 20 minutes again (from the usual 13 minutes).
I don't know how much you know about parallelization, but maybe it would help if you take a look at it.
Btw. the changes to the settings repo for example were only done to fix a parallelization issue

Time savings on my machine (even though paratest threw errors): 2m 8s -> 1m 1s (only 4 threads like GH Actions use)

@BentiGorlich
Copy link
Member Author

BentiGorlich commented May 28, 2025

And as a hint I get these type of errors when running the new AP tests with paratest:

  1. App\Tests\Functional\ActivityPub\Inbox\FlagHandlerTest::testFlagRemoteEntryCommentInRemoteMagazine
    Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException: An exception occurred while executing a query: SQLSTATE[23503]: Foreign key violation: 7 ERROR: insert or update on table "report" violates foreign key constraint "fk_c42f778427ee0e60"
    DETAIL: Key (reporting_id)=(92) is not present in table "user".
  2. App\Tests\Functional\ActivityPub\Inbox\DislikeHandlerTest::testUndoDislikeRemoteEntryInRemoteMagazine
    Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException: An exception occurred while executing a query: SQLSTATE[23503]: Foreign key violation: 7 ERROR: insert or update on table "moderator" violates foreign key constraint "fk_6a30b268a76ed395"
    DETAIL: Key (user_id)=(56) is not present in table "user".

My guess is that somewhere there could happen an implicit COMMIT call which breaks the transaction isolation of the tests, but I have no idea where this could happen

tested with:

php vendor/bin/paratest tests/Functional/ --group ActivityPub -p 4

@melroy89
Copy link
Member

Are you aware of the setup function is not behaving the same as in phpunit due to the nature of paratest, the setup will be called multiple times for each process started by paratest. You could workaround this behavior, see: https://github.com/paratestphp/paratest?tab=readme-ov-file#initial-setup-for-all-tests

I'm not saying this is the root cause though, but important to know.

@BentiGorlich
Copy link
Member Author

Yeah I know about that. The way I did the setUp methods everywhere should not be a problem I think...

@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 186451e to 27dbc4e Compare July 6, 2025 17:21
Whenever an actor makes an activity (like, announce, etc.) we now save it in our db, so the url we pass to other activity pub servers is valid and can return the correct json.
All the factory and wrapper classes now return an `Activity` entity that can be converted to json using the new `ActivityJsonBuilder`

# Conflicts:
#	src/MessageHandler/ActivityPub/Inbox/FollowHandler.php
#	src/MessageHandler/ActivityPub/Outbox/AnnounceLikeHandler.php
#	src/MessageHandler/ActivityPub/Outbox/FollowHandler.php
- Remove the static nature of the `SettingsManager` and the `MentionManager`
  - If the environment is not in `test` then the `SettingsManager` will still use a static cache, but all static methods were replace with instance methods
  - All static methods from the `MentionManager` were replaced with instance methods
- Remove some Doctrine cascade removes (removing a message thread with the entity manager resulted in all participating users being purged)
- Handle pin announces in the message handler themselves instead of the pin event
- pass the incoming activities through to the messages in specific cases
- create `Activity` entities for remote payloads under specific circumstances (so we can announce the original payload)
- improve webfinger exception message
- Add functional ActivityPub testing. This is very complicated and requires a lot of code. Sadly it is also not thread-safe for a reason I could not find out, yet. In every setup method we create actors for different domains, so we can generate some activities, webfinger jsons, and everything we need to mock the activities needed to test the current part of the AP code. Each message handler is tested in its own test class
- Add tests for the `FlagHandler`
- Add tests for undoing like (and fix double wrapping the activity)
@BentiGorlich BentiGorlich force-pushed the new/dereferencable-activities branch from 27dbc4e to 7d73358 Compare July 9, 2025 13:55
@BentiGorlich BentiGorlich marked this pull request as ready for review July 16, 2025 11:23
@BentiGorlich
Copy link
Member Author

I have finally marked this as ready for review. I have it running on gehirneimer.de
It is of course a whole tone of code that is new. A lot of it are new snapshot tests, showing almost all of our federation jsons.

This issue is blocking basically all ActivityPub modifications, and it fixes some very important issues.

@melroy89
Copy link
Member

  • Save activities in the database so that they can be called via URL

    • Add unit tests for the JSON used in outward federation

    • Add functional tests for inbound federation

      • tests with our own JSON
      • tests with lemmy's JSON

Closes #772

Where the lemmy tests also done?

@BentiGorlich
Copy link
Member Author

Nope I didn't add any lemmy testing to the suite. But the mbin tests are a start and this PR is big enough and took long enough as it is...

@melroy89
Copy link
Member

melroy89 commented Jul 16, 2025

No doubt indeed, this pr is big enough.

@BentiGorlich BentiGorlich merged commit 473ec72 into main Jul 16, 2025
7 checks passed
@BentiGorlich BentiGorlich deleted the new/dereferencable-activities branch July 16, 2025 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

activitypub ActivityPub related issues backend Backend related issues and pull requests enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrapped Create activity and its actor have different origins Like activity ID is not dereferencable

3 participants