Push Notification ID compatibility with upcoming Firefish update

[blueset]

Describe the bug

A clear and concise description of what the bug is.

I’m a contributor to Firefish, a Misskey fork. I’m currently working on Mastodon API compatibility layer for push notifications in Firefish.

In most Misskey forks, entity IDs are in the form of strings instead of integers. This is fine with the most part of Megalodon and official upstream. However, when processing incoming push notification payloads, the notification ID is parsed as a long instead of string.

moshidon/mastodon/src/main/java/org/joinmastodon/android/model/PushNotification.java

Lines 14 to 17 in c64d6db

	public class PushNotification extends BaseModel{
	public String accessToken;
	public String preferredLocale;
	public long notificationId;

Despite that, the value is then only used once by converting it back to string in a subsequent API call.

moshidon/mastodon/src/main/java/org/joinmastodon/android/PushNotificationReceiver.java

Lines 103 to 104 in c64d6db

	PushNotification pn=AccountSessionManager.getInstance().getAccount(accountID).getPushSubscriptionManager().decryptNotification(k, p, s);
	new GetNotificationByID(pn.notificationId+"")

Parsing the notification ID as long would break on Misskey forks with Mastodon API support. I would like to suggest parsing the value as a string directly.

To reproduce

Steps to reproduce the behavior:

1. Go to an instance with dev version of Firefish fork with Mastodon push notification support (currently only s.1a23.studio, I can provide account if needed)
2. Receive push notification
3. Observe that parsing of JSON payload fails due to type mismatch at PushNotification.java:16.

Does this happen in the official app?

Does this issue also occur with the respective upstream release?
(Please test using the respective upstream-xxxxxx.apk provided in Releases or at least using the current Mastodon version from the Play Store)

Yes

In case it does, please consider filing an upstream bug report instead.
If this bug is seriously impacting your usage or you think I might want to try to fix it for Megalodon, feel free to still create this issue!

Since this is a compatibility issue with third-party server programs, it may fall out of scope of upstream project. Please let me know if it is not the case. Filing the same issue to upstream at mastodon#842 to try my luck. The same issue is also opened with Megalodon at sk22#994.

Screenshots and screen recordings

If applicable, add screenshots (and screen recordings, if possible) to help explain your problem.

N/A

Version

Moshidon version: [e.g. v1.1.4+fork.#]

v2.3.0+fork.105.moshinda-play

Crash log

If you know your way around Android development tools, please consider attaching a crash log, if possible.

N/A

[LucasGGamerM]

This is an awesome insight. I see what can be done about changing it tomorrow.

Thank you for the detailed bug report!

[FineFindus]

Since this is also a problem in the official app, have you created an issue there as well? Does the parsing of the payload fail silently, or does it crash the app? In the case of the latter, I would be pretty sure that they would accept a fix, since they have said that no matter what the server does, the app should not crash.
Also, since the official docs mention that the notification ID is a string, it might be in their interest to change it as well, or they might have some insight into why they cast it to a long.

[blueset]

@FineFindus said:
Does the parsing of the payload fail silently, or does it crash the app?

Technically the parse error throws an exception and crashes the logic. But since it’s running in the background when a push notification arrives, the crash is not visible to the user.

Looking at the example you quoted, it looks promising that upstream maintainers are willing to accommodate third-party servers. I’m filling mastodon#842 just to tryout my luck.

Thanks for the suggestion!

[blueset]

Dayum, that’s FAST!

Upstream fixed the issue at mastodon/mastodon-android@b2c797f

[LucasGGamerM]

That's grishka man for you. I am cherrypicking the commit asap

[LucasGGamerM]

Fixed with ef23734