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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PlatformException title parsing #2033

Merged
merged 3 commits into from
May 7, 2024

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented May 6, 2024

馃摐 Description

Parses titles that don't contain a module prefix.

馃挕 Motivation and Context

Closes #2001

馃挌 How did you test it?

Unit Tests

馃摑 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

馃敭 Next steps

@ueman Do you remember why we had the .length > 1 check? Was the assumption that there would be only module without type?

Copy link
Contributor

github-actions bot commented May 6, 2024

Android Performance metrics 馃殌

Plain With Sentry Diff
Startup time 383.38 ms 455.38 ms 72.01 ms
Size 6.33 MiB 7.29 MiB 987.27 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
33d0587 308.79 ms 370.86 ms 62.07 ms
1edf30e 334.00 ms 378.35 ms 44.35 ms
0aaa46e 313.39 ms 373.23 ms 59.85 ms
3e5078c 377.10 ms 473.68 ms 96.58 ms
afa6e2a 349.73 ms 428.48 ms 78.75 ms
1a93825 347.31 ms 424.54 ms 77.23 ms
1cdcacf 389.94 ms 463.53 ms 73.59 ms
aed5947 295.70 ms 348.18 ms 52.48 ms
6491ebd 301.74 ms 351.96 ms 50.22 ms
1c6eb5b 350.69 ms 393.86 ms 43.17 ms

App size

Revision Plain With Sentry Diff
33d0587 6.16 MiB 7.14 MiB 1007.47 KiB
1edf30e 5.94 MiB 6.96 MiB 1.02 MiB
0aaa46e 6.16 MiB 7.14 MiB 1007.32 KiB
3e5078c 6.27 MiB 7.20 MiB 959.09 KiB
afa6e2a 6.27 MiB 7.20 MiB 955.69 KiB
1a93825 6.27 MiB 7.20 MiB 956.36 KiB
1cdcacf 6.33 MiB 7.26 MiB 949.77 KiB
aed5947 5.94 MiB 6.96 MiB 1.02 MiB
6491ebd 6.15 MiB 7.13 MiB 999.96 KiB
1c6eb5b 5.94 MiB 6.92 MiB 1001.53 KiB

@denrase denrase marked this pull request as ready for review May 6, 2024 12:51
@ueman
Copy link
Collaborator

ueman commented May 6, 2024

@ueman Do you remember why we had the .length > 1 check? Was the assumption that there would be only module without type?

Maybe it's because it can be a fully qualified type? Have you tried removing the check and look which tests fail?

I.e. the module of io.sentry.SentryFlutterPlugin is io.sentry. I would assume it's something along those lines, but I don't really remember. If you figure it out, put a comment on it :D

@denrase
Copy link
Collaborator Author

denrase commented May 6, 2024

Tests pass even without the check, so I'm not sure why it was here in the first place. What would make sense is, that the module beeing there was always expected: io.sentry.SentryFlutterPlugin. Anyway, the current approach should still work for these cases and also support types without module.

Copy link
Contributor

github-actions bot commented May 6, 2024

iOS Performance metrics 馃殌

Plain With Sentry Diff
Startup time 1248.65 ms 1272.33 ms 23.67 ms
Size 8.32 MiB 9.50 MiB 1.18 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3e9fb0e 1262.49 ms 1280.65 ms 18.16 ms
a817b8f 1261.90 ms 1264.62 ms 2.73 ms
5aba417 1265.31 ms 1287.90 ms 22.59 ms
ef31c7f 1272.43 ms 1295.71 ms 23.29 ms
2d4fd8b 1207.98 ms 1232.94 ms 24.96 ms
6d7a391 1265.65 ms 1289.98 ms 24.33 ms
4ad2751 1247.67 ms 1249.20 ms 1.53 ms
cd16818 1254.78 ms 1267.76 ms 12.98 ms
333903e 1251.15 ms 1270.21 ms 19.06 ms
f79eecf 1210.25 ms 1221.65 ms 11.40 ms

App size

Revision Plain With Sentry Diff
3e9fb0e 8.15 MiB 9.12 MiB 989.77 KiB
a817b8f 8.29 MiB 9.37 MiB 1.07 MiB
5aba417 8.16 MiB 9.17 MiB 1.01 MiB
ef31c7f 8.09 MiB 9.07 MiB 1000.79 KiB
2d4fd8b 8.28 MiB 9.34 MiB 1.06 MiB
6d7a391 8.16 MiB 9.16 MiB 1.01 MiB
4ad2751 8.29 MiB 9.39 MiB 1.10 MiB
cd16818 8.28 MiB 9.33 MiB 1.05 MiB
333903e 8.10 MiB 9.16 MiB 1.06 MiB
f79eecf 8.29 MiB 9.36 MiB 1.07 MiB

@denrase denrase merged commit 9fe67d5 into main May 7, 2024
44 checks passed
@denrase denrase deleted the fix/parse-platform-exception-type-only branch May 7, 2024 07:52
@ueman
Copy link
Collaborator

ueman commented May 7, 2024

I'm sorry to comment on this so late, but I haven't come around to look at this earlier in detail. However, I think the code as changed here doesn't actually fix the problem in the linked ticket.

The given exception from the bug issue is:

PlatformException: PlatformException(getNotificationChannelsError, Unable to find resource ID #0x7f14000d, android.content.res.Resources$NotFoundException: Unable to find resource ID #0x7f14000d
	at android.content.res.ResourcesImpl.getResourceEntryName(ResourcesImpl.java:493)
	at android.content.res.Resources.getResourceEntryName(Resources.java:2441)
	at com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin.getMappedNotificationChannel(FlutterLocalNotificationsPlugin.java:170)
	at com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin.getNotificationChannels(FlutterLocalNotificationsPlugin.java:32)
	at com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin.onMethodCall(FlutterLocalNotificationsPlugin.java:399)
	at be.j$a.a(MethodChannel.java:18)
	at pd.c.l(DartMessenger.java:19)
	at pd.c.m(DartMessenger.java:42)
	at pd.c.h(Unknown Source:0)
	at pd.b.run(Unknown Source:12)
	at android.os.Handler.handleCallback(Handler.java:966)
	at android.os.Handler.dispatchMessage(Handler.java:110)
	at android.os.Looper.loopOnce(Looper.java:205)
	at android.os.Looper.loop(Looper.java:293)
	at android.app.ActivityThread.loopProcess(ActivityThread.java:9832)
	at android.app.ActivityThread.main(ActivityThread.java:9821)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:586)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1201)
, null)

Which translates to creating the PlatformException like this

final e = PlatformException(
  code: 'getNotificationChannelsError',
  message: 'Unable to find resource ID #0x7f14000d',
  details: '''android.content.res.Resources\$NotFoundException: Unable to find resource ID #0x7f14000d
	at android.content.res.ResourcesImpl.getResourceEntryName(ResourcesImpl.java:493)
	at android.content.res.Resources.getResourceEntryName(Resources.java:2441)
	at com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin.getMappedNotificationChannel(FlutterLocalNotificationsPlugin.java:170)
	at com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin.getNotificationChannels(FlutterLocalNotificationsPlugin.java:32)
	at com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin.onMethodCall(FlutterLocalNotificationsPlugin.java:399)
	at be.j\$a.a(MethodChannel.java:18)
	at pd.c.l(DartMessenger.java:19)
	at pd.c.m(DartMessenger.java:42)
	at pd.c.h(Unknown Source:0)
	at pd.b.run(Unknown Source:12)
	at android.os.Handler.handleCallback(Handler.java:966)
	at android.os.Handler.dispatchMessage(Handler.java:110)
	at android.os.Looper.loopOnce(Looper.java:205)
	at android.os.Looper.loop(Looper.java:293)
	at android.app.ActivityThread.loopProcess(ActivityThread.java:9832)
	at android.app.ActivityThread.main(ActivityThread.java:9821)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit\$MethodAndArgsCaller.run(RuntimeInit.java:586)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1201)''',
  stacktrace: null,
);

However, the new test case puts the PlatformException.toString() into the stacktrace property of the PlatformException, which doesn't make any sense.

@denrase
Copy link
Collaborator Author

denrase commented May 13, 2024

@ueman Ah, sorry about that, made the wrong assumption there.

@ueman
Copy link
Collaborator

ueman commented May 13, 2024

No worries, I created #2041 and added some more thoughts to it. Hopefully, that describes the issue better

@denrase
Copy link
Collaborator Author

denrase commented May 13, 2024

@ueman Just for clarification, I based the exception on the tests that were there:

Is this then also incorrect, as it also puts the stacktrace string into message and stacktrace properties? The order of message/details is also different in the ctor than in the test sample declarations.

@ueman
Copy link
Collaborator

ueman commented May 13, 2024

Let's discuss on the ticket :D

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.

PlatformException on Android isn't correctly parsed
3 participants