Add experimental server-side notification grouping

[ClearlyClaire]

This PR adds a group_key attribute on the Notification model that is filled when creating the notification and is used to allow efficient pagination, fetching no more than one notification per group from the database.

It also adds an experimental API:

• GET /api/v2_alpha/notifications
• GET /api/v2_alpha/notifications/:group_key
• POST /api/v2_alpha/notifications/:group_key/dismiss

This relies on a recursive CTE, which is currently not possible to write with stock ActiveRecord, and therefore relies on a couple monkey-patches:

• a monkey-patch to ActiveRecord::QueryMethods to add support for recursive CTEs through a with_recursive method, inspired by the one from @lorint (Recursive + 7.1 vlado/activerecord-cte#16 (comment)), and submitted in Rails: Add support for recursive CTEs in ActiveRecord rails/rails#51601
• a monkey-patch to Arel::Visitors::ToSql to address an issue (Arel union with "limit" or "order by" generates invalid SQL rails/rails#40181) that caused arel to output incorrect SQL for our recursive CTE because of the use of LIMIT, now merged upstream: Fixes/union select parentheses rails/rails#51549

These patches allow to write the CTE in a pretty readable fashion, and allowing the use of already-defined scopes. This can be used to build an API that returns a page of notifications with no more than one notification per group, and still returns a good number of groups, instead of having to call the notification API repeatedly to possibly get more than one group worth of notifications.

Caveats

• while performance should be really good in most cases, large amounts of notifications within few groups will cause large amounts of rows to be filtered. This may cause performance issues. But it may also work well in practice. Needs more investigation.
• because of the monkey-patches, this introduces a fairly high maintenance burden.
  That being said, one of the two patches has been merged upstream, and the other one is under consideration.
  EDIT: both patches have been adopted upstream, so we won't have a long-term maintenance burden
• notification marker ID: we need to return the most recent notification of a page

[ClearlyClaire]

While this PR is still exploratory, this approach is promising. The Arel patch has been accepted into ActiveRecord, so the only monkey-patch left is the recursive CTE one.

On that topic, @lorint, I've used the code you published here to validate the approach, but I have to double-check whether you are ok for this to be included in Mastodon (AGPLv3) or Rails (MIT). Furthermore, are you interested in trying to get this feature merged upstream yourself, or do you prefer I try to do it?

[ClearlyClaire]

I think this is ready for review, with the understanding that the API is experimental for now (hence the /api/v2_alpha namespace) and will likely change before v4.3.0, so marked it as such.

cc @mjankowski if you can have a quick look at the implementation.

[mjankowski]

Some questions before I get into any detailed code review...

• Is the idea here that the /v1/notifications* endpoints continue to exist as-is, and these /v2/... are opt-in versions for getting grouped notification data? so depending on client need they can either stick with the existing one notification at a time approach or switch to new grouped approach?
• I assume the intention with the namespace is to deploy this in sort of an alpha/preview mode and let clients test against live deployed api, and iterate from there?
• Is there any existing client impl done in tandem to use for guidance here, or is this sort of a first pass at the API level in advance of that? It would be useful to see some (even crude) mocks or wireframes of the expected changing UX to contemplate.

[ClearlyClaire]

• Is the idea here that the /v1/notifications* endpoints continue to exist as-is, and these /v2/... are opt-in versions for getting grouped notification data? so depending on client need they can either stick with the existing one notification at a time approach or switch to new grouped approach?

Yes. We might discontinue /api/v1/notifications eventually, but this is not planned for now.

• I assume the intention with the namespace is to deploy this in sort of an alpha/preview mode and let clients test against live deployed api, and iterate from there?

Yes.

• Is there any existing client impl done in tandem to use for guidance here, or is this sort of a first pass at the API level in advance of that? It would be useful to see some (even crude) mocks or wireframes of the expected changing UX to contemplate.

The UI work has not started yet.

[mjankowski]

Great, noted on API preview expectations and migration path.

The lack of UI work makes it sort of challenging to review the approach, since there's not a use case or design to support it seems a little speculative, rather than emerging from a need of a feature? If we're doing alpha/preview just to put actual endpoints up for clients to experiment with ... could we literally just hardcode some JSON showing the proposed new response, and get feedback from client devs? Or if there's internal WIP for the web client, leave this in draft until that's further along?

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[renchap]

I am fine with merging this.

The database side should not change now, and the API is marked as experimental, so I do not expect problems deploying it.

We can change the API in further PRs until we mark it as stable if needed, and having it merged will make working on the frontend implementation much easier and less prone to rebase issues.

outdated

[ClearlyClaire]

@mjankowski I marked your conversations as resolved in order to move forward, but there's a couple refactors that may still be worth exploring:

• moving the notification_group_key logic from NotifyService to the Notification class
• refactor pagination tests to use named variables (but this requires fighting rubocop)