Fix conversations API #1039
No reviewers
Labels
No labels
approved, awaiting change
broken setup
bug
cannot reproduce
configuration
documentation
duplicate
enhancement
extremely low priority
feature request
Fix it yourself
help wanted
invalid
mastodon_api
needs change/feedback
needs docs
needs tests
not a bug
not our bug
planned
pleroma_api
privacy
question
static_fe
triage
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
AkkomaGang/akkoma!1039
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Oneric/akkoma:fix-conv-api"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
See AkkomaGang/akkoma-fe#470 for reference
Mainly the pagination patch is unfinished.
However, I’m considering changing the approach. Maintaining a "proper" foreign key references for last_status_id such that we can actually use it later for displaying in results is tricky since we need to consider whether users are eligible to view the referenced post. When a new post comes in this is relatively straightforward and more or less already dealt with in the existing update logic.
Before the patch there was no logic to roll back the updated at state when a post was deleted (and in the current revision the logic is still incomplete), but this is still doable.
The real issue arises when after the field was updated a user loses the access rights via e.g. an unfollow¹. Effectively we’ll have to check user access perms at time of rendering the convo response and requery the context for a fallback if the rights were lost.
Then we also can't fully rely on post creation/deletion updates to clean up empty/dead participations and conversations.
If we need to requery this at rendering time and do extra deletion/cleanup passes anyway, we might as well simplify it and have just a
last_activity_atfield, which gets filled out with activity ids when new ones come in, but isn’t bound to the activity continuing to exist. Basically, just a timestamp-like field similar toupdated_at, just safer against spurious bumps and usable in pagiantion parameters.I’m also no longer sure whether changing the semantics of
accountsto match Mastodon is a good idea — at least not without some surveying of clients and coordination with Pleroma first. While docs claim(ed) something else, in practice this field has been the only way to access manually managed recipients in Akkoma and Pleroma, so changing it might break existing clients if any actually use the recipient management feature.Nonetheless, a commit changing this as initially proposed is done (sans adding tests) and included here. Might be removed before merge though.
The WIP patch for this is attached to this post.
[1]: currently the update logic only acts on DMs and ignores messages of other visibilities in the same thread. Whether deletes are allowed to roll back to other visibilities is not yet determined. Since we do not allow changing recipients (as in their AP IDs) in post edits this will not actually happen atm if we restrict this to only DMs.
However, (a) this seems brittle and likely to backfire in the future and (b) it’s not clear to me whether it’s actually desirable to limit this to DMs only. On IRC the desire to "follow" regular, non-DM conversations was expressed which would require lifting this limitation (for updates of existing participations, implicit creations should retain this).
94b9855c9fdc248f0739dc248f0739b114efd013b114efd013554ac9b551WIP: fix conversations APIto Fix conversations APIChanged the approach degrading the sorting field to just a funny, but more robust date as considered yesterday.
With this and some further cleanup, this should now cover all known actual bugs, pass all tests and work.
Masto-compatible
accountsresponses and smarter auto-reads are deferred to a later "enhancements and extensions" patch series.Will test this now in prod for a while before merging.
UPDATE: nvm, the custom pagination isn't working yet with min/max/since_id; apparently there were no test for that yet
UPDATE 2: now fixed, but noticed another, pre-existing issue, see next post
For future reference, the very WIP and unfinished fkey version of the pagination patch is attached.
554ac9b5515137d10288Fix conversations APIto WIP: Fix conversations APIWhile fixing the remaining pagination issue, I realised the pre-existing post-query filtering has yet another (also pre-existing) bug on edge cases: Filtering only after the query means we can end up with fewer results than allowed by the ex- or implicit
limit. That’s not great but ok'ish in itself, but crucially, pagination headers are atm also only generated after the filtering already happened. Therefore pagination headers will resume the lookup after the last unfiltered but before any already-seen but previously filtered entries. If there’s a full page worth of filtered entries (e.g. due to blocks or because all messages got deleted), it will get stuck.This is kinda another point in favour of the original proper-foreign-key approach, but as discussed before this is hard to maintain with retroactively changing access perms.
For now I intend to workaround this by moving the filtering to the view, so that pagination headers can be generated on the full results. Then there might still be empty pages in responses, but they’ll come with valid
Linkheaders the client can continue to follow to get all results.Perhaps we can reattempt the foreign-key approach in the future when we have a clearer idea of how or if non-direct posts will affect participations with future enhancements and extensions. But for now this is already a strict improvement over the prior buggy mess and much of the current work can be reused or built upon for a potential future transition to actual foreign keys.
UPDATE: now also fixed and added to the test suite
5137d10288d9bd56ea51d9bd56ea51cbf46c5beccbf46c5bec033f6b4647WIP: Fix conversations APIto Fix conversations API033f6b4647cc35ae5514Yet another pre-existing bug found during prod testing: only local messages can mark conversation as read since
create_or_bump_foris only called fromActivityPub.notify_and_stream(without regard for the activity type) which is only called from either (a)ActivityPub.(do_)createviaCommonAPI.postwhich is only used when creating local statuses, or (b) in the side-effect handling of, (for some reason) specifically onlyUpdateactivitiesThis should probably be moved to the side-effect handling of
Createi believe (assuming local activities also go through side-effect handling)Update: turns out local and remote activities have completely distinct pipelines, except for very few activity types, in specific cases partially re-using bits from the other...
cc35ae55142efe44a9072efe44a907c0485c3a71c0485c3a71fc89b2d983fc89b2d9836d14d954466d14d954464df6a370ae4df6a370ae429a4602f0429a4602f0cd12b3d8f1cd12b3d8f15ce68a83acRemote post handling is now fixed.
To avoid merge conflicts I also pulled in a notification stream+push overhaul from Pleroma, before fixing remote post handling. Though as it turns out the Pleroma commit breaks stuff so I had to rewrite everything so in the end it wouldn’t have mattered.
However, now it’s already in their and placed before the fix (and cannot be moved without conflicts). There’s on notable change and question now regarding this. It is possible to mute other users post-only (keeping notifications visible) or completely (also hiding notifications unless
with_muted=trueis passed). before the change notifications from post-only-muted users were however (usually) immediately marked as read.Now only those notifications hidden without
with_muted=trueare automatically marked as read. This intuitively makes more sense to me since notifications were explicitly exempt from the mute (and it’s not clear whether the old behaviour was intentional.Are there any objections to the change?
5ce68a83ac7ed0bb4ff832069561c2cff040a28ccff040a28c6443db213aEverything works well enough now and the patchset is already rather big, so it’s time to merge:
Enhancements left to future patchsets:
/api/v1/conversations, it is better but still sluggishlast_bumpfirst and if exists and visible use that, or by caching the prep queriesaccountssemantics differing from mastodon