Add support for limited Announce Activity #15776
Conversation
There was a problem hiding this comment.
While the feature would be somewhat useful, this current implementation has several performance issues.
Furthermore, I'm uncomfortable with the whole audience processing logic being duplicated with Create.
Finally, I am not too sure mentions for reblogs are handled in a way that makes sense across the codebase.
| @@ -2,28 +2,23 @@ | |||
|
|
|||
| class ActivityPub::Activity::Announce < ActivityPub::Activity | |||
| def perform | |||
| dereference_object! | |||
There was a problem hiding this comment.
dereference_object! unconditionally dereferences the object if it's an URL. This change will cause any cross-instance boost to re-fetch known Announced statuses.
Also, it would make sense to not dereference a status if it's not related to local activity (though the reblog_of_local_status? check would have to be changed to first check if the URI is possibly local and not do status_from_uri).
There was a problem hiding this comment.
Changed to check if url points to local before reblog_of_local_status? accesses the database. After passing these checks, I now run dereference_object!.
| original_status = status_from_object | ||
|
|
||
| return reject_payload! if original_status.nil? || !announceable?(original_status) | ||
| @original_status = status_from_object |
There was a problem hiding this comment.
If the object is an URI, the earlier call to dereference_object! fails and the object isn't already known, a second (wasteful) attempt at dereferencing the status will be made by status_from_object.
If dereference_object! has been successful, status_from_object will perform a wasteful database query to find the status that is already loaded in @object.
There was a problem hiding this comment.
Changed to record the success or failure of dereference and use the acquired object if the dereference is successful.
| delivered_to_account = Account.find(@options[:delivered_to_account_id]) | ||
|
|
||
| @status.mentions.create(account: delivered_to_account, silent: true) | ||
| @status.update(visibility: :limited) if @status.direct_visibility? |
There was a problem hiding this comment.
Not a big deal, but not useful here as reblogs can't have direct visibility.
There was a problem hiding this comment.
Moved duplicate code for Create and Announce to one place.
|
This PR allows you to receive boosts for circle members in the circle function. The ability to deliver limited annouce will be implemented separately. We have also made a change to allow dereferenced Announce Activities delivered by group actors. This allows the group server to deliver posts that are exclusive to group participants. I haven't tested it enough yet, so I'll verify it over the weekend. |
noellabo
force-pushed
the
add-support-for-limited-announce-activity
branch
from
23f219a
to
6388cce
Compare
noellabo
force-pushed
the
add-support-for-limited-announce-activity
branch
from
bb05b46
to
c5aa094
Compare
|
Changed to use the object fetched by dereference and solved the performance problem. The code related to the group will be reverted and will be handled by another PR. Please review. |
There was a problem hiding this comment.
Not marked as accepted as I would like another set of eyes on this PR that might have subtle security implications.
I have not found anything fishy, though, and the change seems good to me.
I'm not too sure about process_audience and postprocess_audience_and_deliver being in ActivityPub::Activity because they rely on rely specific params, status and mentions instance variables and are really specific to status creation, but I still think it's better than duplicating them. Maybe a concern?
| @@ -63,6 +71,6 @@ def requested_through_relay? | |||
| end | |||
|
|
|||
| def reblog_of_local_status? | |||
| status_from_uri(object_uri)&.account&.local? | |||
| ActivityPub::TagManager.instance.local_uri?(object_uri) && status_from_uri(object_uri)&.account&.local? | |||
There was a problem hiding this comment.
Not directly related as far as I understand, but a good change nonetheless as it avoids a database query in the case the status isn't a local one.
|
I moved the methods related to process_audience to ProcessAudience concern. |
noellabo
force-pushed
the
add-support-for-limited-announce-activity
branch
from
a7caeb3
to
8866397
Compare
This reverts commit 6388ccee5bc5facce36d2b46d1dd0061d95889fb.
noellabo
force-pushed
the
add-support-for-limited-announce-activity
branch
from
8866397
to
80f6e79
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
The evaluation has not progressed, but I think that it will be reviewed when the group function is used. Stay open. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request will remain closed and will be pulled again when the need arises. |
For Announce Activity, we'll add support for Bearcaps dereference, limited visibility, and direct delivery of actors inbox.