Implement Push Notifications #1842
Conversation
| ); | ||
|
|
||
| -- Pusher IDs must be unique for a given user. | ||
| CREATE UNIQUE INDEX IF NOT EXISTS pusher_localpart_id_idx ON pusher_pushers(localpart, pusher_id); |
There was a problem hiding this comment.
pusher_id isn't defined in the schema.
There was a problem hiding this comment.
Thanks @S7evinK it was a work in progress, I just pushed the final piece :)
There was a problem hiding this comment.
Sorry, I should have waited a bit. :) (was a bit bored, and push was also on my list)
You can mark a PR as "WIP" if you're not done yet, btw. :)
| ` | ||
|
|
||
| const selectPushersByLocalpartSQL = "" + | ||
| "SELECT pusher_id, display_name, last_seen_ts, ip, user_agent FROM pusher_pushers WHERE localpart = $1 AND pusher_id != $2" |
There was a problem hiding this comment.
Same as above, pusher_id doesn't seem to exist.
There was a problem hiding this comment.
This is a good start overall. It looks like some state refactor work has leaked through into this PR @neilalexander - any idea?
It would be nice to try to get this into a mergable state now to avoid having to land a massively scary change all at once.
clientapi/routing/pusher.go
Outdated
| return jsonerror.InternalServerError() | ||
| } | ||
| } else if body.Kind == "" { | ||
| if targetPusher == nil { |
There was a problem hiding this comment.
It's impossible for this to be hit.
| ctx context.Context, session_id int64, | ||
| pushkey, kind, appid, appdisplayname, devicedisplayname, profiletag, lang, data, localpart string, | ||
| ) error { | ||
| return d.pushers.insertPusher(ctx, nil, session_id, pushkey, kind, appid, appdisplayname, devicedisplayname, profiletag, lang, data, localpart) |
There was a problem hiding this comment.
All writes need to use d.writer else it can race with other writes causing database is locked errors on sqlite.
| ) error { | ||
| stmt := sqlutil.TxStmt(txn, s.insertPusherStmt) | ||
| _, err := stmt.ExecContext(ctx, localpart, session_id, pushkey, kind, appid, appdisplayname, devicedisplayname, profiletag, lang, data) | ||
| logrus.Debugf("🥳 Created pusher %d", session_id) |
There was a problem hiding this comment.
Whilst the emoji are cute, we don't use them anywhere else. For consistency, please keep to the boring log lines :)
| lang TEXT, | ||
| data TEXT, | ||
|
|
||
| UNIQUE (app_id, pushkey, localpart) |
There was a problem hiding this comment.
If this is the unique key then I'm surprised to see selecting pushers and updating pushers use WHERE localpart = $1 AND pushkey = $2.
| ) error { | ||
| return sqlutil.WithTransaction(d.db, func(txn *sql.Tx) error { | ||
| if err := d.pushers.deletePusher(ctx, txn, appid, pushkey, localpart); err != sql.ErrNoRows { | ||
| return err |
There was a problem hiding this comment.
This will fire if err == nil e.g on success, which is presumably not what you want. It's a bit of a weird way of doing this. I would just have an extra if statement for clarity e.g:
err := d.pushers.deletePusher(ctx, txn, appid, pushkey, localpart)
if err == nil || err == sql.ErrNoRows {
return nil
}
return err
kegsay
added
the
stale
|
Please re-open when you have brought this back up-to-date. |
|
Hi @kegsay, we've updated the branch but don't have the permission to reopen this pull request. |
|
Please make a new pull request - I can't seem to reopen this as this branch was either force pushed or recreated. |
|
According to the solution found in How to reopen a pull-request after a force-push? We've restored original branch by $ git push -f origin bf92734:implement-push-notifications (SHA of last commit from https://github.com/matrix-org/dendrite/pull/1842/commits). @kegsay I think you should be able to reopen PR now (SHA from step 1 and 2 are different and we can use just instructions from gist instead of comment). |
PiotrKozimor
force-pushed
the
implement-push-notifications
branch
from
bf92734
to
ba3114e
Compare
|
Dendrite in sytest is failing with |
kegsay
removed
the
stale
|
I am almost there :D I have two failing test in sytest - one is this flakey |
|
Thanks for working on this! I'm switching from XMPP to Matrix and thought I'd go for Dendrite. Once this is in, I'm guessing this would work with Sygnal. Is the longterm plan to also port Sygnal/Push gateway to Dendrite/Go? |
|
I was playing around with this and managed to get push notifications working between Element Webs. Element Android background processing still doesn't work, but it does show notifications when the app is active, so that might be on the phone side. Steps I had to take
Edit I just realized that the actual pushing isn't implemented in this PR yet (the |
|
@PiotrKozimor thanks for the confirmation. I'd love to see this, as I think phone notifications would be the only thing stopping me from replacing XMPP. I'm currently waiting for review on #2014 (the SSO support). Is there anything I can help with on this PR while that's in queue? |
|
@tommie your help would be more than appreciated. I think we should focus on implementing notifications with default rules - |
See tommie@abce199, it implements the Element Android even requires a specific rule ID: However, they seem happy even without the PUT endpoints. Notably, we need the rules both in
To summarize the current holes:
Let me know if I missed some area. What are you planning on working on (if anything) in the near term? I'm new to Matrix, but am comfortable with Go and distributed systems in general. I don't see any problem with me working on any of these, but there's obviously some dependencies, and I don't know how big the room membership and push rules evaluation tasks are. |
|
I will get back to notifications in 3-4 weeks - that's the current plan. It would be awesome if you could work on it in the meantime.
We also have another big hole - do we want to push notification for user that are already syncing? If yes, then we need presence module - #1879. Documentation is not very informative on this topic, maybe we need to check synapse? |
They are, though. As "account data", which is read by Element, see link in my previous comment. That JSON ends up in
Ok, then it sounds like I should just continue on anything you mentioned for now. :) |
|
I got a first notification on Elements Android with tommie@4ff4502.
The next step is push rules evaluation. |
|
Initial pushrules evaluator in tommie@e741b84.
The next step should be unit tests for the pushrules package. After that would be user-provided pushrules. Will pick that up later. |
|
OK, I've made quite a few significant changes here, the biggest being that there's no longer a push server — the new code is now folded into the User API. This fits in with our new plans for component scaling. I've also fixed various bugs, like with the JetStream consumers, the SQL table creation and detecting when the user doesn't have existing push rules. I'm running this on |
…c API with stream positions, instead of consuming directly
|
Awesome. Thanks! |
|
Could not wait for the next release ;) |
|
Hmm in |
|
Wow. That's the most awaited PR. |
|
Hi everyone, https://github.com/globekeeper/dendrite/commits/main (v0.8.1 with 2 modifications of mine) runs on dendrite.stg.globekeeper.com in polylith mode with clustered nats and CockroachDB as a database. Feel free to try it out! |
|
Does this mean we should be getting push notifications on mobile now (Element Android)? If we're not, should I file a bug report? |
|
Yes you should be able. Beside Element, there is hydrogen from your web browser with WebPush Notification (both works well on my dendrite) Element Android I use together with my own https://ntfy.sh instance to receive the notification over UnifiedPush instatt of FCM (over google's server). Yes you should create issues / bug reports, if it does not work |
If you used the |
|
@S7evinK Ah, I did use that! I will track that PR. |
Added support for Push Notifications.according to Client-Server Push Notifications API, resolves #611.
This PR adds a new "Pusher Database" based on the Pusher Model.
Design thoughts 🤔
clientapior in a separate component?clientapior in a separate component?What's left to do ❓
Tests 🔧
tests/14account/01change-password.pl:
tests/30rooms/60version_upgrade.pl:
tests/61push/01message-pushed.pl:
tests/61push/02add_rules.pl:
tests/61push/03_unread_count.pl:
tests/61push/05_set_actions.pl:
tests/61push/06_get_pusher.pl:
tests/61push/06_push_rules_in_sync.pl:
tests/61push/07_set_enabled.pl:
tests/61push/08_rejected_pushers.pl:
tests/61push/09_notifications_api.pl:
tests/61push/80torture.pl:
Pull Request Checklist ✅
sytest-whitelistas specified in docs/sytest.mdSigned-off-by:
Dan Peleg <[email protected]>