Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebPush support for urgency and coalescing #213

Merged
merged 14 commits into from
Apr 13, 2021

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Apr 7, 2021

Related to #186, based off #212 :

  • support dropping earlier notifications in the same room using the Topic header
  • set the Urgency header based on the notification prio field.

@bwindels bwindels requested a review from a team April 7, 2021 12:17
@bwindels bwindels force-pushed the bwindels/webpush-urgency-and-coalescing branch from 887c58b to 589e1dc Compare April 8, 2021 13:25
docs/applications.md Outdated Show resolved Hide resolved
sygnal/webpushpushkin.py Show resolved Hide resolved
Comment on lines 360 to 361
Provide a response object that matches the API expected from pywebpush.
pywebpush expects a synchronous API, while we use an asynchronous API.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still serves as something that looks like a response to pywebpush, while actually containing the information to perform the request ourselves once pywebpush finishes. I agree the naming is not ideal, but couldn't come up with something better. Ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I see.

sygnal/webpushpushkin.py Show resolved Hide resolved
Comment on lines +202 to 212
request = webpush(
subscription_info=subscription_info,
data=data,
ttl=self.ttl,
vapid_private_key=self.vapid_private_key,
vapid_claims=vapid_claims,
requests_session=self.http_agent_wrapper,
requests_session=self.http_request_factory,
)
response = await request.execute(
self.http_agent, low_priority, topic
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does the deduplication actually take place, out of interest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this, it happens in the push gateway. If you have two messages with the same topic header, the second will replace the first (if the first hasn't been delivered yet).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, so there is a separate "push gateway" after sygnal (which I tend to think of as a, y'know, push gateway)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, webpush pipes notification to a push provider (or gateway, hard to name things correctly with so many parts to it) like FCM that will actually deliver it to the browser application, similarly to how the apns and gcm pushkins forward it to the push providers of those platforms. Every browser that supports web push needs to have a server like this that application servers can post push messages to. It's the browser-specific push provider that will drop the first message if it hasn't been deliver yet.

bwindels and others added 4 commits April 9, 2021 11:29
@bwindels bwindels requested a review from richvdh April 9, 2021 09:45
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm generally, a few nits

docs/applications.md Outdated Show resolved Hide resolved
docs/applications.md Outdated Show resolved Hide resolved
Comment on lines 360 to 361
Provide a response object that matches the API expected from pywebpush.
pywebpush expects a synchronous API, while we use an asynchronous API.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I see.

sygnal/webpushpushkin.py Outdated Show resolved Hide resolved
@@ -363,8 +365,6 @@ class HttpResponseWrapper:
in the background.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it now doesn't really happen "in the background", afaict? it's more that it is deferred until later.

How about:

To keep pywebpush happy we present it with some hardcoded values that
make its assertions pass even though the HTTP request has not yet been
made.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I forgot to update this as the async request was fired immediately before but now is deferred to the execute method 👍 Have adopted your suggestion.

@bwindels bwindels requested a review from richvdh April 9, 2021 12:41
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, sorry for the slow review!

@richvdh richvdh merged commit 8d2dff0 into master Apr 13, 2021
@richvdh richvdh deleted the bwindels/webpush-urgency-and-coalescing branch April 13, 2021 13:45
awesome-michael added a commit to Awesome-Technologies/sygnal that referenced this pull request May 6, 2021
Sygnal 0.9.3 (2021-04-22)
=========================

Features
--------

- Prevent the push key from being rejected for temporary errors and oversized payloads, add TTL logging, and support `events_only` push data flag. ([\matrix-org#212](matrix-org#212))
- WebPush: add support for Urgency and Topic header ([\matrix-org#213](matrix-org#213))

Bugfixes
--------

- Fix a long-standing bug where invalid JSON would be accepted over the HTTP interfaces. ([\matrix-org#216](matrix-org#216))
- Limit the size of requests received from HTTP clients. ([\matrix-org#220](matrix-org#220))

Updates to the Docker image
---------------------------

- Remove manually added GeoTrust Root CA certificate from docker image as Apple is no longer using it. ([\matrix-org#208](matrix-org#208))

Improved Documentation
----------------------

- Make `CONTIBUTING.md` more explicit about how to get tests passing. ([\matrix-org#188](matrix-org#188))
- Update `CONTRIBUTING.md` to specify how to run code style and type checks with Tox, and add formatting to code block samples. ([\matrix-org#193](matrix-org#193))
- Document how to work around pip installation timeout errors. Contributed by Omar Mohamed. ([\matrix-org#215](matrix-org#215))

Internal Changes
----------------

- Update Tox to run in the installed version of Python (instead of specifying Python 3.7) and to consider specific paths and folders while running checks, instead of the whole repository (potentially including unwanted files and folders, e.g. the virtual environment). ([\matrix-org#193](matrix-org#193))
- Make development dependencies available as extras. Contributed by Hillery Shay. ([\matrix-org#194](matrix-org#194))
- Update `setup.py` to specify that a minimum version of Python greater or equal to 3.7 is required. Contributed by Tawanda Moyo. ([\matrix-org#207](matrix-org#207))
- Port CI checks to Github Actions. ([\matrix-org#210](matrix-org#210), [\matrix-org#219](matrix-org#219))
- Upgrade development dependencies. Contributed by Omar Mohamed ([\matrix-org#214](matrix-org#214))
- Set up `coverage.py` to run in tox environment, and add html reports ([\matrix-org#217](matrix-org#217))

Change-Id: I14ae821ff2a1562e91fd87ce25f73baaa0b9430b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants