Add aioapns version of APNS #696

pomali · 2023-11-02T15:39:25Z

Since version python 3.10+ is not supported by apns2 (because of dependency on hyper) we add aioapns version of sending APNs notifications.

We add installation extra [APNS_ASYNC] that installs aioapns and use new version of service if aioapns is installed. Tests are also conditional on installed modules/version of python.

Related to

Add Python 3.10 support and classifiers #623 (is blocked by apns2)
State of the maintenance #685
Incompatible with Python 3.10 #622
does not work with python 3.10 #675

Since version python 3.10+ is not supported by `apns2` (because of dependency on `hyper`) we add `aioapns` version of sending APNs notifications. We add installation extra `[APNS_ASYNC]` that installs aioapns and use new version of service if `aioapns` is installed. Tests are also conditional on installed modules/version of python.

pomali · 2023-11-02T15:43:01Z

There are some (maybe) controversial decisions, feel free to discuss them

choice of aioapns - it looks like only library with active development
supporting both apns2 and aioapns - because we don't want to break older code
deciding on which version to use in models.py - sure we can move it to apns.py and move apns2 code to apns_legacy.py

sivasankarankb · 2023-12-08T07:55:59Z

@pomali You have left print() statements in the code. See apns_async.py.

codecov · 2023-12-08T08:16:43Z

Codecov Report

Attention: Patch coverage is 11.20690% with 103 lines in your changes are missing coverage. Please review.

Project coverage is 64.66%. Comparing base (0f79181) to head (7500caa).

❗ Current head 7500caa differs from pull request most recent head f8c192a

Please upload reports for the commit f8c192a to get more accurate results.

Files	Patch %	Lines
push_notifications/apns_async.py	4.62%	103 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
- Coverage   70.39%   64.66%   -5.73%     
==========================================
  Files          27       28       +1     
  Lines        1101     1214     +113     
  Branches      180      199      +19     
==========================================
+ Hits          775      785      +10     
- Misses        288      391     +103     
  Partials       38       38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

itayAmza · 2024-01-25T15:20:44Z

@pomali, any progress for this one?
I had the apns2 0.7.2 requires PyJWT<2.0.0,>=1.4.0, but you have pyjwt 2.1.0 which is incompatible issue, and pointing to your branch (as a temporary solution) completely solves it.

pomali · 2024-01-25T15:29:37Z

I'm not sure what's missing, maybe someone should review it? And then there is code coverage check failing, but most of failing lines are comments, so I am not sure how important it is.

Is there anyone who could point me in right direction?

@itayAmza glad that it helped :)

itayAmza · 2024-01-25T15:36:25Z

@jamaalscarlett following this one can you help us figure out who can help review this PR?

50-Course · 2024-02-15T15:53:04Z

hopefully, I get to have a look into both PRs, tonight. Thank you for the notification @itayAmza

jamaalscarlett · 2024-03-11T17:00:52Z

I can review the PR, but I have never used APNS. Has someone actually run this with an ios/macos app?
Regarding the support for both apns2 and aioapns, I think we should add a deprecation warning with this release then completely remove apns2 with the next release.

pomali · 2024-03-11T17:57:33Z

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

ssyberg · 2024-03-13T14:19:42Z

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

I just tried this and got this error when trying to send a test message via apns:

ModuleNotFoundError at /admin/push_notifications/apnsdevice/
No module named 'apns2'
Request Method:	POST
Request URL:	https://local.a24-vod:8060/admin/push_notifications/apnsdevice/
Django Version:	4.2
Exception Type:	ModuleNotFoundError
Exception Value:	
No module named 'apns2'

kupsum · 2024-03-17T09:07:27Z

push_notifications/apns_async.py

+		collapse_id: str = None,
+		aps_kwargs: dict = {},
+		message_kwargs: dict = {},
+		notification_request_kwargs: dict = {},


Since notification_request_kwargs argument has a default value and it's content is changed in the function logic, the default state can change during execution. If any previous call has set values expiration, priority or collapse_id, these are persisted to any next call and if the next call does not have these values or has the value set to None, the values are still passed to sent NotificationRequest. A simple test case to demonstrate this is to send first message with expiration and second without expiration set. The value from first call is passed to second call NotificationRequest.

Solved it by creating copy of notification_request_kwargs, but if you consider it code smell I could rework it to having default argument None and replacing them in function with {}. Added your test (test_apns_send_messages_different_priority)

kupsum · 2024-03-17T09:11:37Z

There is a possible bug with default argument pollution. Below test case can demonstrate the issue. I've also added a review comment about this.

	@mock.patch("push_notifications.apns_async.APNs", autospec=True)
	def test_apns_send_messages_different_priority(self, mock_apns):
		self._create_devices(["abc", "def"])
		device_1 = APNSDevice.objects.get(registration_id="abc")
		device_2 = APNSDevice.objects.get(registration_id="def")

		device_1.send_message("Hello world 1", expiration=time.time() + 1, priority=5, collapse_id="1")
		args_1, _ = mock_apns.return_value.send_notification.call_args

		device_2.send_message("Hello world 2")
		args_2, _ = mock_apns.return_value.send_notification.call_args

		req = args_1[0]
		self.assertEqual(req.device_token, "abc")
		self.assertEqual(req.message["aps"]["alert"], "Hello world 1")
		self.assertAlmostEqual(req.time_to_live, 1, places=-1)
		self.assertEqual(req.priority, 5)
		self.assertEqual(req.collapse_key, "1")

		reg_2 = args_2[0]
		self.assertEqual(reg_2.device_token, "def")
		self.assertEqual(reg_2.message["aps"]["alert"], "Hello world 2")
		self.assertIsNone(reg_2.time_to_live, "No time to live should be specified")
		self.assertIsNone(reg_2.priority, "No priority should be specified")
		self.assertIsNone(reg_2.collapse_key, "No collapse key should be specified")

pomali · 2024-03-18T07:46:31Z

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

I just tried this and got this error when trying to send a test message via apns:
ModuleNotFoundError at /admin/push_notifications/apnsdevice/
No module named 'apns2'
Request Method:	POST
Request URL:	https://local.a24-vod:8060/admin/push_notifications/apnsdevice/
Django Version:	4.2
Exception Type:	ModuleNotFoundError
Exception Value:	
No module named 'apns2'

do you have aioapns installed? because that is condition on which we use the new code

added mention to README

pip install django-push-notifications[APNS_ASYNC]

pomali · 2024-03-18T10:55:43Z

There is a possible bug with default argument pollution. Below test case can demonstrate the issue. I've also added a review comment about this.

Thanks for catching this, I added your test and fixed the issue.

for more information, see https://pre-commit.ci

kmasuhr · 2024-03-18T10:59:22Z

I think it is worth to add python 3.10 as testing target in CI, since it should enable support for version 3.10

django-push-notifications/.github/workflows/test.yml

Line 12 in 820262e

python-version: ['3.6', '3.7', '3.8', '3.9']

ssyberg · 2024-03-21T17:54:30Z

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

I just tried this and got this error when trying to send a test message via apns:
ModuleNotFoundError at /admin/push_notifications/apnsdevice/
No module named 'apns2'
Request Method:	POST
Request URL:	https://local.a24-vod:8060/admin/push_notifications/apnsdevice/
Django Version:	4.2
Exception Type:	ModuleNotFoundError
Exception Value:	
No module named 'apns2'
do you have aioapns installed? because that is condition on which we use the new code

added mention to README
pip install django-push-notifications[APNS_ASYNC]

I'm pretty sure I did when I tested it, but wouldn't that be handled by pip/piptools?

50-Course · 2024-03-22T09:37:02Z

Hello @kmasuhr,

I have just updated this PR to bump the version of python being used in CI.
However, it would seem one of its transitive dependencies is not compatible with Python 3.10 - precisely, the hyper==0.7.0
HTTP Client.

Given that the hyper library is not actively maintained, I would like to propose that we replace it with a maintained alternative.
I am open to suggestions, but I would recommend using httpx as a good replacement.

pomali · 2024-03-26T23:31:23Z

I'm pretty sure I did when I tested it, but wouldn't that be handled by pip/piptools?

@ssyberg it should be handled by pip, but it's optional dependency, so maybe it didn't get installed?

rib3 · 2024-03-29T20:28:02Z

I have just updated this PR to bump the version of python being used in CI. However, it would seem one of its transitive dependencies is not compatible with Python 3.10 - precisely, the hyper==0.7.0 HTTP Client.

Given that the hyper library is not actively maintained, I would like to propose that we replace it with a maintained alternative. I am open to suggestions, but I would recommend using httpx as a good replacement.

hyper is a dependency of apns2.
This PR adds support for aioapns as an alternative apple push notification library.
(because of the problems with apns2/hyper)

There are some github issues with additional details / background linked in the PR description.

ssyberg · 2024-04-08T13:39:18Z

I have just updated this PR to bump the version of python being used in CI. However, it would seem one of its transitive dependencies is not compatible with Python 3.10 - precisely, the hyper==0.7.0 HTTP Client.
Given that the hyper library is not actively maintained, I would like to propose that we replace it with a maintained alternative. I am open to suggestions, but I would recommend using httpx as a good replacement.

hyper is a dependency of apns2. This PR adds support for aioapns as an alternative apple push notification library. (because of the problems with apns2/hyper)

There are some github issues with additional details / background linked in the PR description.

If I'm understanding correctly, the inclusion of hyper in this PR is only to stay backwards compatible with apns2 for people who don't want to upgrade to using aioapns in which case they need to stay <3.10 (which if they are using apns2 I think is implied). IMO this could be noted in the docs but shouldn't block merge, a lot of people are waiting for this to get merged and it's blocking dev for anyone on later version setups.

kupsum

A few comments when trying to install this in Python 3.11 system

kupsum · 2024-04-13T10:25:13Z

.github/workflows/test.yml

@@ -9,7 +9,7 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        python-version: ['3.6', '3.7', '3.8', '3.9']
+        python-version: ['3.6', '3.7', '3.8', '3.9', '3.10']


Could you consider adding 3.11 too?

Suggested change

python-version: ['3.6', '3.7', '3.8', '3.9', '3.10']

python-version: ['3.6', '3.7', '3.8', '3.9', '3.10', '3.11']

accepted change

kupsum · 2024-04-13T16:44:43Z

setup.cfg

@@ -22,6 +22,7 @@ classifiers =
 	Programming Language :: Python :: 3.7
 	Programming Language :: Python :: 3.8
 	Programming Language :: Python :: 3.9
+	Programming Language :: Python :: 3.11


Could you consider adding Python 3.11? Either way, Python 3.10 was missing

Suggested change

Programming Language :: Python :: 3.11

Programming Language :: Python :: 3.10

Programming Language :: Python :: 3.11

accepted change

kupsum · 2024-04-13T16:50:51Z

tox.ini

@@ -41,6 +45,8 @@ deps =
    dj32: Django>=3.2,<3.3
    dj40: Django>=4.0,<4.0.5
 	dj405: Django>=4.0.5,<4.1
+	dj42: Django>=4.2,<4.3
+	dj42: aioapns>=3.1,<3.2


Maybe it would make send to have apns2 for Python 3.9 or less and aioapns Python 3.10+

Suggested change

dj42: aioapns>=3.1,<3.2

py{36,37,38,39}: apns2

py{310,311}: aioapns>=3.1,<3.2

updated deps, removed apns2 from default leaving it only in py{36,37,38,39}

kupsum · 2024-04-13T16:59:34Z

tests/test_apns_push_payload.py

+	from apns2.client import NotificationPriority
+	from push_notifications.apns import _apns_send
+	from push_notifications.exceptions import APNSUnsupportedPriority
+except AttributeError:


apns2 and aioapns are not supported together with conflict

The conflict is caused by: aioapns 3.1 depends on pyjwt>=2.0.0 apns2 0.7.2 depends on PyJWT<2.0.0 and >=1.4.0

If apns2 is not installed, ModuleNotFoundError is raised.

Suggested change

except AttributeError:

expect (AttributeError, ModuleNotFoundError):

accepted change

kupsum · 2024-04-13T16:59:38Z

tests/test_apns_models.py

-class APNSModelTestCase(TestCase):
+	from push_notifications.exceptions import APNSError
+	from push_notifications.models import APNSDevice
+except AttributeError:


apns2 and aioapns are not supported together with conflict

The conflict is caused by: aioapns 3.1 depends on pyjwt>=2.0.0 apns2 0.7.2 depends on PyJWT<2.0.0 and >=1.4.0

If apns2 is not installed, ModuleNotFoundError is raised.

Suggested change

except AttributeError:

expect (AttributeError, ModuleNotFoundError):

accepted change

kupsum · 2024-04-13T17:00:49Z

tox.ini

+    py{38,39,310}-dj{40,405}
+	py311-dj42


Django 4.2 supports still Python 3.8

Suggested change

py{38,39,310}-dj{40,405}

py311-dj42

py{38,39,310,311}-dj{40,405,42}

accepted change

(fix missing space)

…ge, add ruff formatting config

to conform with Core metadata specification https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use

50-Course · 2024-05-23T10:51:09Z

push_notifications/apns_async.py


 from . import models
 from .conf import get_manager
 from .exceptions import APNSServerError

+ErrFunc = Optional[Callable[[NotificationRequest, NotificationResult], Awaitable[None]]]


Out of office right now, Awaitable is that some form of Future result?

#Update: Interesting,

https://docs.python.org/3/library/typing.html#annotating-callable-objects

I have always took a different approach to this!

50-Course · 2024-05-23T10:58:33Z

push_notifications/apns_async.py

+			notification_request_kwargs_out["time_to_live"] = expiration - int(
+				time.time()
+			)


ruff botched this formatting in here

50-Course · 2024-05-23T10:59:55Z

push_notifications/apns_async.py

👍🏼 looks good to me, can we have a test for this?

pomali requested a review from jamaalscarlett November 7, 2023 18:16

remove print statements

b9647e4

itayAmza mentioned this pull request Jan 25, 2024

apns2 0.7.2 requires PyJWT<2.0.0,>=1.4.0, but you have pyjwt 2.1.0 which is incompatible. Pr0Ger/PyAPNs2#133

Open

VaughnDV approved these changes Feb 2, 2024

View reviewed changes

ssyberg mentioned this pull request Mar 13, 2024

hyper dependency is no longer maintained Pr0Ger/PyAPNs2#126

Open

kupsum reviewed Mar 17, 2024

View reviewed changes

document APNS_ASYNC installation in README

5c55ba0

pomali added 3 commits March 18, 2024 10:57

Merge branch 'master' into master

63be384

fix issue with overwriting default, add test case

9242485

fix some flake8 issues, improve readme

f8f6bc5

[pre-commit.ci] auto fixes from pre-commit.com hooks

92c425c

for more information, see https://pre-commit.ci

50-Course added 2 commits March 22, 2024 09:55

ci(python-deps): support python 3.10 as testing target on CI

3fdbabf

bump tox python target version to py310

698df8f

kupsum reviewed Apr 13, 2024

View reviewed changes

ssyberg mentioned this pull request Apr 24, 2024

Incompatible with Python 3.10 #622

Open

macro1 mentioned this pull request May 6, 2024

Migrate from apns2 to aioapns #721

Draft

pomali added 7 commits May 22, 2024 00:48

Update test.yml

9a139bd

Update test.yml

b81f3a0

(fix missing space)

Add fixes from PR suggestions, update tested environments

82ab061

Merge branch 'master' into master

de6ed14

add types

bcad217

add err_func to enable processing errors from aioapns.APNs.send_messa…

22dcc9f

…ge, add ruff formatting config

fix extras APNS_ASYNC -> apns-async

7500caa

to conform with Core metadata specification https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use

50-Course reviewed May 23, 2024

View reviewed changes

push_notifications/apns_async.py

Copy link

Member

50-Course May 23, 2024

Choose a reason for hiding this comment

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

👍🏼 looks good to me, can we have a test for this?

fix NotificationResult import, add test for err_func param

f8c192a

	Programming Language :: Python :: 3.11
	Programming Language :: Python :: 3.10
	Programming Language :: Python :: 3.11

	dj42: aioapns>=3.1,<3.2
	py{36,37,38,39}: apns2
	py{310,311}: aioapns>=3.1,<3.2

	except AttributeError:
	expect (AttributeError, ModuleNotFoundError):

	py{38,39,310}-dj{40,405}
	py311-dj42
	py{38,39,310,311}-dj{40,405,42}

Add aioapns version of APNS #696

Are you sure you want to change the base?

Add aioapns version of APNS #696

Conversation

pomali commented Nov 2, 2023

pomali commented Nov 2, 2023

sivasankarankb commented Dec 8, 2023

codecov bot commented Dec 8, 2023 • edited

Codecov Report

itayAmza commented Jan 25, 2024

pomali commented Jan 25, 2024

itayAmza commented Jan 25, 2024

50-Course commented Feb 15, 2024

jamaalscarlett commented Mar 11, 2024

pomali commented Mar 11, 2024

ssyberg commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kupsum commented Mar 17, 2024

pomali commented Mar 18, 2024

pomali commented Mar 18, 2024

kmasuhr commented Mar 18, 2024

ssyberg commented Mar 21, 2024

50-Course commented Mar 22, 2024

pomali commented Mar 26, 2024 • edited

rib3 commented Mar 29, 2024

ssyberg commented Apr 8, 2024

kupsum left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

50-Course May 23, 2024 • edited

Choose a reason for hiding this comment

50-Course May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 8, 2023 •

edited

pomali commented Mar 26, 2024 •

edited

kupsum left a comment •

edited

50-Course May 23, 2024 •

edited

50-Course May 23, 2024 •

edited