Graphql Use new async `handleException` method #3090

lbloder · 2023-12-11T09:29:20Z

📜 Description

Use new async handleException method for compatibility with newer Graphql-Java Versions.
In order to stay compatible with both older and newer verions, the @Override annotation of the deprecated onException method has been removed. onException now calls the handleException implementation. The min supported GraphQL Version is 17, which introduced the new async method and the graphQLContext that our implementation relies on.

💡 Motivation and Context

Starting with graphql-java v21, implementing the handleException method is required and the deprecated method onException has been removed from the DataFetcherExceptionHandler interface.

Fixes #3017

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

…"onException" keep "onException" for backwards compat

github-actions · 2023-12-11T09:43:07Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	399.20 ms	448.00 ms	48.80 ms
Size	1.70 MiB	2.27 MiB	582.26 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`4e29063`	384.14 ms	447.74 ms	63.60 ms
`d429384`	397.33 ms	457.69 ms	60.36 ms
`93a76ca`	397.30 ms	455.16 ms	57.87 ms
`937879e`	400.98 ms	482.84 ms	81.86 ms
`606074f`	384.90 ms	456.90 ms	72.00 ms
`3d8bd2b`	379.45 ms	450.36 ms	70.90 ms
`2fad834`	390.07 ms	470.80 ms	80.73 ms
`b172d4e`	352.92 ms	446.50 ms	93.58 ms
`c3f503e`	360.41 ms	434.67 ms	74.27 ms
`93a76ca`	378.48 ms	451.78 ms	73.30 ms

App size

Revision	Plain	With Sentry	Diff
`4e29063`	1.72 MiB	2.29 MiB	578.38 KiB
`d429384`	1.72 MiB	2.27 MiB	558.42 KiB
`93a76ca`	1.72 MiB	2.29 MiB	576.75 KiB
`937879e`	1.72 MiB	2.27 MiB	558.42 KiB
`606074f`	1.72 MiB	2.27 MiB	558.30 KiB
`3d8bd2b`	1.72 MiB	2.29 MiB	577.53 KiB
`2fad834`	1.72 MiB	2.29 MiB	577.53 KiB
`b172d4e`	1.72 MiB	2.29 MiB	578.09 KiB
`c3f503e`	1.72 MiB	2.29 MiB	577.04 KiB
`93a76ca`	1.72 MiB	2.29 MiB	576.75 KiB

Previous results on branch: graphql-exception-handler

Startup times

Revision	Plain	With Sentry	Diff
`0417432`	422.68 ms	477.78 ms	55.10 ms
`b936993`	392.46 ms	473.14 ms	80.69 ms
`8dd20e3`	401.54 ms	542.06 ms	140.52 ms
`83cb64c`	297.31 ms	339.06 ms	41.75 ms
`351ac25`	397.96 ms	459.00 ms	61.04 ms
`5ac1966`	396.50 ms	422.35 ms	25.85 ms

App size

Revision	Plain	With Sentry	Diff
`0417432`	1.72 MiB	2.27 MiB	558.56 KiB
`b936993`	1.72 MiB	2.27 MiB	554.95 KiB
`8dd20e3`	1.72 MiB	2.27 MiB	558.21 KiB
`83cb64c`	1.72 MiB	2.27 MiB	554.95 KiB
`351ac25`	1.72 MiB	2.27 MiB	558.21 KiB
`5ac1966`	1.72 MiB	2.27 MiB	554.95 KiB

adinauer · 2023-12-18T13:23:27Z

sentry-graphql/src/main/java/io/sentry/graphql/SentryDataFetcherExceptionHandler.java

+        handleException(handlerParameters);
+
+    if (futureResult != null) {
+      return futureResult.join();


We should use .get() here combined with try/catch as well?

adinauer · 2023-12-18T13:26:01Z

sentry-graphql/src/main/java/io/sentry/graphql/SentryGenericDataFetcherExceptionHandler.java

+    try {
+      return futureResult.get();
+    } catch (InterruptedException | ExecutionException e) {
+      return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();


Suggested change

return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();

return null;

adinauer · 2023-12-18T13:26:09Z

sentry-graphql/src/main/java/io/sentry/graphql/SentryGenericDataFetcherExceptionHandler.java

+        handleException(handlerParameters);
+
+    if (futureResult == null) {
+      return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();


Suggested change

return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();

return null;

adinauer · 2023-12-18T13:26:20Z

sentry-graphql/src/main/java/io/sentry/graphql/SentryDataFetcherExceptionHandler.java

+    if (futureResult != null) {
+      return futureResult.join();
+    } else {
+      return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();


Suggested change

return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();

return null;

lbloder · 2023-12-18T15:26:50Z

sentry-graphql/src/main/java/io/sentry/graphql/SentryDataFetcherExceptionHandler.java

+  }
+
+  @SuppressWarnings("deprecation")
+  public @Nullable DataFetcherExceptionHandlerResult onException(


@adinauer this was not nullable before. Should we rather return an empty DataFetcherExceptionHandlerResult ?

Ah yeah I missed that, let's keep it @NotNull

# Conflicts: # CHANGELOG.md

…herExceptionHandler onException

sentry-graphql/src/main/java/io/sentry/graphql/SentryGenericDataFetcherExceptionHandler.java

# Conflicts: # CHANGELOG.md

lbloder and others added 2 commits December 4, 2023 15:43

WIP: initial impl of handling errors in "handleException" instead of …

0f8b455

…"onException" keep "onException" for backwards compat

Format code

ae6c65c

lbloder added 3 commits December 11, 2023 13:46

Merge branch 'main' into graphql-exception-handler

11bf275

add changelog

50c5f07

revert tests for compatibility with graphql-java 17.3

223e020

lbloder marked this pull request as ready for review December 18, 2023 13:12

lbloder requested review from adinauer, romtsn, stefanosiano and markushi as code owners December 18, 2023 13:12

lbloder removed request for markushi, romtsn and stefanosiano December 18, 2023 13:12

adinauer reviewed Dec 18, 2023

View reviewed changes

CR

4f304fd

lbloder commented Dec 18, 2023

View reviewed changes

lbloder added 3 commits December 18, 2023 16:32

Merge branch 'main' into graphql-exception-handler

133e175

# Conflicts: # CHANGELOG.md

Merge branch 'main' into graphql-exception-handler

219d9d9

# Conflicts: # CHANGELOG.md

remove @nullable annotation and return empty result in SentryDataFetc…

378e3fd

…herExceptionHandler onException

adinauer reviewed Dec 20, 2023

View reviewed changes

sentry-graphql/src/main/java/io/sentry/graphql/SentryGenericDataFetcherExceptionHandler.java Show resolved Hide resolved

adinauer approved these changes Jan 8, 2024

View reviewed changes

lbloder and others added 2 commits January 8, 2024 14:50

Merge branch 'main' into graphql-exception-handler

42939cd

# Conflicts: # CHANGELOG.md

Merge branch 'main' into graphql-exception-handler

3fdc3ce

adinauer merged commit 3ae645b into main Jan 12, 2024
20 checks passed

adinauer deleted the graphql-exception-handler branch January 12, 2024 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graphql Use new async `handleException` method #3090

Graphql Use new async `handleException` method #3090

lbloder commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 11, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: graphql-exception-handler

Startup times

App size

adinauer Dec 18, 2023

adinauer Dec 18, 2023

adinauer Dec 18, 2023

adinauer Dec 18, 2023

lbloder Dec 18, 2023

adinauer Dec 19, 2023

	return CompletableFuture.completedFuture((DataFetcherExceptionHandlerResult) null).join();
	return null;

Graphql Use new async handleException method #3090

Graphql Use new async handleException method #3090

Conversation

lbloder commented Dec 11, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Dec 11, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: graphql-exception-handler

Startup times

App size

adinauer Dec 18, 2023

Choose a reason for hiding this comment

adinauer Dec 18, 2023

Choose a reason for hiding this comment

adinauer Dec 18, 2023

Choose a reason for hiding this comment

adinauer Dec 18, 2023

Choose a reason for hiding this comment

lbloder Dec 18, 2023

Choose a reason for hiding this comment

adinauer Dec 19, 2023

Choose a reason for hiding this comment

Graphql Use new async `handleException` method #3090

Graphql Use new async `handleException` method #3090

lbloder commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 11, 2023 •

edited

Loading