Feature/graphql error handling #80

robertionut95b · 2023-11-02T14:31:42Z

Added GraphQL error handling support. Currently integrated with spring-graphql and working towards supporting more 3rd party implementations of GraphQL in Spring Framework/Boot.

Supports minimum version 1.2.2 of spring-graphql.

Features I was not able to implement or not yet supported:

Global response customization
Support for multiple 3rd party implementations of GraphQL in Spring framework:
- spring-graphql
- Netflix's DGS
- Kickstart
- SPQR
Map location and path properties in the error responses: can't yet find a generic enough way which would work for all implementations above

Added GraphQL error handling support. Currently integrated with spring-graphql and working towards supporting more 3rd party implementations of GraphQL in Spring Framework/Boot.

wimdeblauwe · 2023-11-02T20:34:59Z

...io/github/wimdeblauwe/errorhandlingspringbootstarter/AbstractErrorHandlingConfiguration.java

 import io.github.wimdeblauwe.errorhandlingspringbootstarter.mapper.HttpStatusMapper;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
 import org.springframework.context.annotation.Bean;

 public abstract class AbstractErrorHandlingConfiguration {
    @Bean
    @ConditionalOnMissingBean
-    public LoggingService loggingService(ErrorHandlingProperties properties) {
-        return new LoggingService(properties);
+    public LoggingService loggingService(ErrorHandlingProperties properties,


I would rather not have any graphql related classes in this class. It would be better to have a dedicated subclass.

I see. Do you want this Configuration class to be a separate type of ServletErrorHandlingConfiguration specific to GraphQL configuration? Would there be a requirement to mark it as optional? Such way in properties with @ConditionalOnProperty(value = "error.handling.graphql.enabled", matchIfMissing = false)?

wimdeblauwe · 2023-11-02T20:37:04Z

src/main/java/io/github/wimdeblauwe/errorhandlingspringbootstarter/LoggingService.java

@@ -27,6 +32,20 @@ public void logException(ApiErrorResponse errorResponse, Throwable exception) {
        }
    }

+    public void logException(GraphQLError graphQLError, Throwable exception) {


I would prefer a separate GraphQlLoggingService class. Common code could be moved into an abstract super class, or to helper components.

We can extract the common logic into an AbstractLoggingService class. Then, each of the concrete implementations for REST and GraphQL can declare it's custom logic for logException method depending on the context. Is this what you had in mind?

wimdeblauwe · 2023-11-02T20:37:53Z

...thub/wimdeblauwe/errorhandlingspringbootstarter/handler/AbstractGraphqlExceptionHandler.java

+import org.springframework.http.HttpStatusCode;
+
+public abstract class AbstractGraphqlExceptionHandler implements GraphqlExceptionHandler {
+    protected final HttpStatusMapper httpStatusMapper;


I guess these can be private since there are the getter methods anyway?

We can mark them as private, yes. I also mostly used the specialized "get...(Throwable exception)" methods.

wimdeblauwe · 2023-11-02T20:40:25Z

...mdeblauwe/errorhandlingspringbootstarter/handler/GqlConstraintViolationExceptionHandler.java

+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+
+public class GqlConstraintViolationExceptionHandler extends AbstractGraphqlExceptionHandler {


Lots of duplication with ConstraintViolationApiExceptionHandler. We should find a way to avoid that.

Yes here it is a fact, indeed. How would you propose to tackle this? We have multiple options:

Static Utils class to handle ConstraintViolationException -> ApiErrorResponse

Bean helper class that translates ConstraintViolationException -> ApiErrorResponse (this increases extensibility)

Specialized method (+ it's dependencies) in the super class to handle this specific behavior

wimdeblauwe · 2023-11-02T20:41:53Z

...thub/wimdeblauwe/errorhandlingspringbootstarter/handler/GqlAccessDeniedExceptionHandler.java

@@ -0,0 +1,51 @@
+package io.github.wimdeblauwe.errorhandlingspringbootstarter.handler;


Maybe we can put all GraphQL related handlers in a graphql sub-package?

Would you want the Gql handlers to be under the root package or under root/handler sub-package?

wimdeblauwe · 2023-11-02T20:45:41Z

Thank you for this PR. Nice that you have so many tests added! In general, I'd like the graphql parts to be a little more separate, so using different packages. Maybe you can add a proposal as a comment here first before you start moving things around?

ooraini · 2023-11-02T23:53:35Z

Hi, I have used both the error-handling-starter and recently started using GraphQl, so I have a two comments.

GqlErrorType looks awfully similar to org.springframework.graphql.execution.ErrorType Maybe use that instead
Not sure why GraphQlErrorMapper is needed since you will always return a 200 response

robertionut95b · 2023-11-03T07:53:13Z

Hi, I have used both the error-handling-starter and recently started using GraphQl, so I have a two comments.

GqlErrorType looks awfully similar to org.springframework.graphql.execution.ErrorType Maybe use that instead

Not sure why GraphQlErrorMapper is needed since you will always return a 200 response

Hello @ooraini,

Those are some good points, but my arguments on using them:

We can later extend the GqlErrorType to more custom values for specific use-cases, a simple off the top of my head case with incorrect queries which natively resolves to ValidationError. If the API's will allow us to furtherly customize the responses then we use more specific values. We could use BAD_REQUEST as well, but I feel like this is more oriented to invalid request responses in terms of parameters or request body.
GraphQlErrorMapper is used to aid into creating a ErrorClassification which is later injected as value in the GraphqlError. For cases where we have let's say a Validation error, we can also map this value as BAD_REQUEST, similarly to how HttpStatus behaves. I also use it in the opposite way, to translate from ErrorClassification to HttpStatus in order to ensure the existing logging functionality.

robertionut95b added 2 commits November 2, 2023 16:18

Graphql support - initial commit

847b355

Added GraphQL error handling support. Currently integrated with spring-graphql and working towards supporting more 3rd party implementations of GraphQL in Spring Framework/Boot.

Remove some unused imports

ed99722

robertionut95b mentioned this pull request Nov 2, 2023

Extend the library to handle Graphql errors #78

Closed

wimdeblauwe reviewed Nov 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/graphql error handling #80

Feature/graphql error handling #80

robertionut95b commented Nov 2, 2023 •

edited

Loading

wimdeblauwe Nov 2, 2023

robertionut95b Nov 3, 2023

wimdeblauwe Nov 2, 2023

robertionut95b Nov 3, 2023

wimdeblauwe Nov 2, 2023

robertionut95b Nov 3, 2023

wimdeblauwe Nov 2, 2023

robertionut95b Nov 3, 2023

wimdeblauwe Nov 2, 2023

robertionut95b Nov 3, 2023

wimdeblauwe commented Nov 2, 2023

ooraini commented Nov 2, 2023 •

edited

Loading

robertionut95b commented Nov 3, 2023

		@@ -0,0 +1,51 @@
		package io.github.wimdeblauwe.errorhandlingspringbootstarter.handler;

Feature/graphql error handling #80

Are you sure you want to change the base?

Feature/graphql error handling #80

Conversation

robertionut95b commented Nov 2, 2023 • edited Loading

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

wimdeblauwe commented Nov 2, 2023

ooraini commented Nov 2, 2023 • edited Loading

robertionut95b commented Nov 3, 2023

robertionut95b commented Nov 2, 2023 •

edited

Loading

ooraini commented Nov 2, 2023 •

edited

Loading