Netflix/dgs-framework

bug: scalar coercing exceptions don't use typed graphql error

Meemaw opened this issue · 8 comments

Meemaw commented

Expected behavior

TypedGraphQLError is used consistently across all error surfaces.

Actual behavior

When using custom scalar, coercing exceptions (e.g CoercingParseLiteralException) don't use the TypedGraphQLError and provide very bare-bones errors.

This was improved in 8.6.0 for some validation errors, but custom scalar coercing was missed.

Steps to reproduce

Note: A test case would be highly appreciated, but we understand that's not always possible

srinivasankavitha commented

Thanks for flagging this. We'll fix this behavior in the next release or two .

mrvaruntandon commented

@Meemaw, could you please the steps to reproduce this issue?

mrvaruntandon commented

@srinivasankavitha is this issue open for contribution?

srinivasankavitha commented

Yes, this has not been addressed yet.

mrvaruntandon commented

Hello @srinivasankavitha,

I tried to create a project to understand this issue.

I created a query returning a custom scaler. While returning a value for the query, the serialize() method of Coercing interface from my custom implementation was invoked. When I passed a value that would throw CoercingSerializeException. The response for such a query was:

{
  "errors": [
    {
      "message": "Can't serialize value (/ip) : Invalid IPv4 address",
      "path": [
        "ip"
      ],
      "extensions": {
        "errorType": "UNAVAILABLE",
        "errorDetail": "SERVICE_ERROR"
      }
    }
  ],
  "data": {
    "ip": null
  }
}

I did not see any logs in my console telling me that CoercingSerializeException has happened.

Could you tell me if the above description successfully reproduces this issue?

Also, could share what should be the expected response here and how is it related to TypedGraphQLError class?

mrvaruntandon commented

@Meemaw, could you please the steps to reproduce this issue?

@Meemaw could you please check the way I have tried to reproduce this issue? Also, could you tell me what would be the expected behaviour here?

srinivasankavitha commented

@mrvaruntandon - I believe you have the correct repro of the issue. The ask is to fill out the errorType/errorDetail to reflect something closer to the exception rather than our generic UNAVAILABLE error. The logic to handle translating graphql-java error to a TypedGraphQlError is here: https://github.com/Netflix/dgs-framework/blob/master/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt#L29, so we just need to handle the CoercingSerializeException here.

mrvaruntandon commented

Hey @srinivasankavitha,

Thank you for your guidance! I have tried to resolve this issue via #1970. Could you please take a look and provide your comments.

Thanks in advance!