Add DelegatingDataFetcherExceptionHandler #2355

rwinch · 2021-05-19T19:59:54Z

No description provided.

Create DelegatingDataFetcherExceptionHandler which accepts a mapping of Predicate to DataFetcherExceptionHandler. The corresponding DataFetcherHandler of the first Predicate to match is used. If no match is found, then the default handler is used.

Allows easily creating a DelegatingDataFetcherExceptionHandler mapping from the type of Exception that was thrown.

bbakerman · 2021-05-20T01:21:32Z

src/main/java/graphql/execution/DelegatingDataFetcherExceptionHandler.java

+     */
+    public void setDefaultHandler(DataFetcherExceptionHandler defaultHandler) {
+        this.defaultHandler = assertNotNull(defaultHandler, () -> "defaultHandler can't be null");
+    }


Dont make this mutable - use a builder pattern or have constructors for it

I've pushed an update to use a Builder to ensure that DelegatingDataFetcherExceptionHandler is immutable.

bbakerman · 2021-05-20T01:24:24Z

src/main/java/graphql/execution/DelegatingDataFetcherExceptionHandler.java

+     *
+     * @return the {@link DelegatingDataFetcherExceptionHandler} to use
+     */
+    public static DelegatingDataFetcherExceptionHandler fromThrowableTypeMapping(LinkedHashMap<Class<? extends Throwable>, DataFetcherExceptionHandler> types) {


A specific implementation in parameters is strange

LinkedHashMap - It should be just Map

I get that it wants to be in order - but I think the caller should decide not

Order matters and LinkedHashMap is the only Map implementation that preserves order.

I've pushed an update to use a Builder which encapsulates how the mapping and order are preserved. Internally a List<PredicateDelegateMapping> is now used such that PredicateDelegateMapping is a private class with the members Predicate<Throwable> and DataFetcherExceptionHandler.

bbakerman · 2021-05-20T01:25:04Z

src/main/java/graphql/execution/DelegatingDataFetcherExceptionHandler.java

+        return new DelegatingDataFetcherExceptionHandler(handlers);
+    }
+
+    private static <M extends Map<K, V>, K, V> M assertNotEmpty(M map, String message) {


This should go in the graphql java Assert class AND use a Supplier for the message - see that code

I've updated the code with a commit that moves this to the Assert class as described. However, with the introduction of the Builder which encapsulates the mapping in List<PredicateDelegateMapping> there is no need for it, so I removed it in an additional commit.

bbakerman · 2021-05-20T01:29:10Z

src/test/groovy/graphql/execution/DelegatingDataFetcherExceptionHandlerTest.groovy

+
+        then: 'fail assert on validation'
+        thrown AssertException
+    }


I find these types of test low value - asserts are important BUT not the key "regression" point in code base.

Do you want me to remove all the tests or just this one or...?

I've removed all tests in DelegatingDataFetcherExceptionHandlerTest that are asserting AssertException was thrown

bbakerman · 2021-05-20T01:30:21Z

src/test/groovy/graphql/execution/DelegatingDataFetcherExceptionHandlerTest.groovy

+        def actualResult = handler.onException(handlerParameters)
+
+        then: 'only the matching delegate is invoked'
+        0 * _


I am not sure this is testing anything??

I think then needs updated to be none of the delegates are invoked. This is verify that if none of the delegates match, then none of them are used (the default should be used).

I've updated then description to be accurate.

Previously then was incorrectly stated "only the matching delegate is invoked". However, the assertion correctly validated that "none of the delegates are invoked". The then description is now corrected.

Per the feedback in the code review these tests don't test `key "regression" point in code base`.

Per the review feedback it is preferred to keep this in Assert class rather than in a private method to DelegatingDataFetcherExceptionHandler.

Previously assertNotEmpty does not have a return type that preserves the type passed in. This commit returns the same type as what was asserted. This allows assertion of a List and ensuring the assigning type can still be declared as a List.

This addresses two concerns: The first is the use of LinkedHashMap for the delegates. LinkedHashMap was chosen intentionally because the order matters. However, it is strange to rely on an implementation. The Builder now encapsulates how the mapping is done (using a List and private class named PredicateDelegateMapping). The second concern it addresses is to ensure DelegatingDataFetcherExceptionHandler is immutable allowing defaultHandler to only be set in the Builder.

This is no longer required by any code in spring-graphql.

rwinch · 2021-05-20T21:07:52Z

I believe I have addressed all of your comments. I did so in a commit per comment to make it easier to follow along with what I've done. I'm happy to squash and rebase before the merge or preserve the commits.

rstoyanchev · 2021-05-21T13:06:31Z

I think having this packaged as a DataFetcherExceptionHandler implementation doesn't seem an effective way to enable sharing across multiple, independent exception handlers, which I think is the main goal, unless all exception handlers agree to adopt this particular mechanism. The only way I see to do enable sharing effectively is to have it built into the contract. Otherwise DataFetcherExceptionHandler remains nothing more than a helper, utility class for use in cases where all exception handlers are controlled by the application.

In addition, depending on how #2356 is implemented, it may change the way this here is done.

rwinch · 2021-05-21T20:48:11Z

@rstoyanchev I do agree that it will depend on the result of gh-2356, but I don't understand your other comments:

doesn't seem an effective way to enable sharing across multiple, independent exception handlers, which I think is the main goal, unless all exception handlers agree to adopt this particular mechanism

DelegatingDataFetcherExceptionHandler allows a user to assemble arbitrary DataFetcherExceptionHandler together without the delegate DataFetcherExceptionHandler needing to be aware of DelegatingDataFetcherExceptionHandler. So I'm confused why you feel that all exception handlers need to agree to adopt this particular mechanism.

The only way I see to do enable sharing effectively is to have it built into the contract.

Which contract are you referring to?

Otherwise DataFetcherExceptionHandler remains nothing more than a helper, utility class for use in cases where all exception handlers are controlled by the application.

Why is DataFetcherExceptionHandler a helper/utility class or how does DelegatingDataFetcherExceptionHandler (this PR) change DataFetcherExceptionHandler into a helper/utility class where all exceptions handlers are controlled by the application?

rstoyanchev · 2021-05-24T16:07:36Z

DelegatingDataFetcherExceptionHandler could be used to assemble arbitrary DataFetcherExceptionHandler implementations indeed but you need to write predicates to decide which handlers handle which exceptions, and that's knowledge best encapsulated within each DataFetcherExceptionHandler.

Take a scenario with two frameworks having independent DataFetcherExceptionHandler implementations. Ideally each would encapsulate the knowledge of what exceptions it handles and what it doesn't, but as the contract is currently, you're expected to handle all exceptions. An application could use predicates to decide which exceptions go to which handler, but then such predicates would be needed in every application to make the same decisions as far as I can see.

This is why I think the ability to have multiple exception handlers should be built into the very contracts of GraphQL Java or otherwise left as it is.

rwinch · 2021-05-24T18:30:01Z

I don't think that the logic of determining when to invoke a DataFetcherExceptionHandler is always best embedded within the DataFetcherExceptionHandler. For example, there might be a ForbiddenDataFetcherExceptionHandler that would be invoked differently based upon the context it is being used. In a Serlvet application ForbiddenDataFetcherExceptionHandler might be invoked when an java.security.AccessControlException is thrown. In a Spring Security application, this would happen when org.springframework.security.access.AccessDeniedException is thrown and there is no currently authenticated user.

For cases where the logic for both the decision and the handling logic should be in the same object the object can implement both DelegatingDataFetcherExceptionHandler and Predicate<Throwable>.

I think it's worth pointing out that this is only introducing a contract for a single implementation that should work for a large majority of use cases. It does not prevent additional implementations from having their own semantics.

rstoyanchev · 2021-05-24T19:23:48Z

Right, so exception handlers have to be built with DelegatingDataFetcherExceptionHandler in mind (by handling specific exceptions only or by implementing Predicate) or else they wouldn't work.

bbakerman · 2021-06-07T06:34:04Z

We intend to merge a "async" exception handler in #2371 . This will affect this code.

Please have a look at tell us of any concerns

rwinch added 2 commits May 19, 2021 14:58

Add DelegatingDataFetcherExceptionHandler.fromThrowableTypeMapping

39563e1

Allows easily creating a DelegatingDataFetcherExceptionHandler mapping from the type of Exception that was thrown.

bbakerman reviewed May 20, 2021

View reviewed changes

rwinch added 7 commits May 20, 2021 09:17

Improve "then:" Description

e3e7f7c

Previously then was incorrectly stated "only the matching delegate is invoked". However, the assertion correctly validated that "none of the delegates are invoked". The then description is now corrected.

Remove DelegatingDataFetcherExceptionHandler AssertionException Tests

0cac07b

Per the feedback in the code review these tests don't test `key "regression" point in code base`.

Assert.assertNotEmpty(Map,Supplier<String>)

0381889

Per the review feedback it is preferred to keep this in Assert class rather than in a private method to DelegatingDataFetcherExceptionHandler.

Use Assert.assertNotEmpty(Map,Supplier<String>)

ad56af1

Revert "Assert.assertNotEmpty(Map,Supplier<String>)"

1d2d972

This is no longer required by any code in spring-graphql.

bbakerman closed this Feb 1, 2022

Add DelegatingDataFetcherExceptionHandler #2355

Add DelegatingDataFetcherExceptionHandler #2355

Uh oh!

Conversation

rwinch commented May 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch commented May 20, 2021

Uh oh!

rstoyanchev commented May 21, 2021

Uh oh!

rwinch commented May 21, 2021

Uh oh!

rstoyanchev commented May 24, 2021

Uh oh!

rwinch commented May 24, 2021

Uh oh!

rstoyanchev commented May 24, 2021

Uh oh!

bbakerman commented Jun 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rwinch May 20, 2021 •

edited

Loading