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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception stack trace shouldn't be included in the GraphQL error message #425

Closed
valbers opened this issue Feb 6, 2023 · 1 comment
Closed

Comments

@valbers
Copy link
Collaborator

valbers commented Feb 6, 2023

Description

When a query validation error occurs regarding the variables passed in a request (e.g.: no value was provided by the client for an expected variable), a GraphQL response with an error message is sent to the client, as expected, but this error message reveals much more information than it should: it contains an exception stack trace additionally to an exception message (which is actually already a good error message to be passed along to the client).
Apart from this specific scenario of mismatching variables, I can't even think of a valid situation in which it would make sense to show a stack trace to the client, so I'd propose to remove this "feature" altogether (not only for the mismatching variable case) and at most just log the exception somewhere at the server. Another possibility would be to make what goes inside the error message directed at the client configurable (at startup): e.g.: in a development environment one could find it more comfortable to see the stack trace right at the client (as opposed to having to check up on the server logs). In that case, even in a development environment the mismatching variable validation use case still wouldn't be an "exception-throwing" case and shouldn't include any exception stack trace information in the error message. BTW the current exception message already contains all the information a client needs to understand what they did wrong.

Repro steps

  1. Modify any existing unit test that tests that an error message is returned in a GraphQL response such that it tests that the expected error message must have an exact match in the error message array sent in the response (as opposed to testing that there is an actual error message that just contains the expected error message). Example:
[<Fact>]
let ``Execute handles variables and errors on null for nested non-nulls`` () =
    let ast = parse """query q($input: TestInputObject) {
          fieldWithObjectInput(input: $input)
        }"""
    let params' : Map<string, obj> =
      Map.ofList [
        "input", upcast Map.ofList [
            "a", "foo" :> obj
            "b", upcast ["bar"]
            "c", null]]
    let actual = sync <| Executor(schema).AsyncExecute(ast, variables = params')
    match actual with
    | Direct(data, errors) ->
      // Previously: hasError "Variable '$input': in input object 'TestInputObject': in field 'c': expected value of type String but got None" errors
      hasErrorWithExactMessage "Variable '$input': in input object 'TestInputObject': in field 'c': expected value of type String but got None" errors
    | _ -> fail "Expected Direct GQResponse"
  1. Run the test

Expected behavior

Test should be successful.

Actual behavior

Test fails:

[xUnit.net 00:00:01.71]     FSharp.Data.GraphQL.Tests.VariablesTests.Execute handles variables and errors on null for nested non-nulls [FAIL]
  Fehler FSharp.Data.GraphQL.Tests.VariablesTests.Execute handles variables and errors on null for nested non-nulls [481 ms]
  Fehlermeldung:
   Expected to contain exact message 'Variable '$input': in input object 'TestInputObject': in field 'c': expected value of type String but got None', but no such message was found. Messages found: [("FSharp.Data.GraphQL.GraphQLException: Variable '$input': in input object 'TestInputObject': in field 'c': expected value of type String but got None
   at FSharp.Data.GraphQL.Values.coerceVariableValue(Boolean isNullable, InputDef typedef, VarDef vardef, Object input, String errMsg) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Values.fs:line 119
   at FSharp.Data.GraphQL.Values.coerceVariableInputObject(InputObjectDef objdef, VarDef vardef, Object input, String errMsg) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Values.fs:line 175
   at FSharp.Data.GraphQL.Values.coerceVariableValue(Boolean isNullable, InputDef typedef, VarDef vardef, Object input, String errMsg) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Values.fs:line 154
   at FSharp.Data.GraphQL.Values.coerceVariableValue(Boolean isNullable, InputDef typedef, VarDef vardef, Object input, String errMsg) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Values.fs:line 124
   at FSharp.Data.GraphQL.Values.coerceVariable(VarDef vardef, FSharpMap`2 inputs) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Values.fs:line 193
   at FSharp.Data.GraphQL.Execution.coerceVariables@655.Invoke(FSharpMap`2 acc, VarDef vardef) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Execution.fs:line 655
   at Microsoft.FSharp.Collections.ListModule.Fold[T,TState](FSharpFunc`2 folder, TState state, FSharpList`1 list) in D:\a\_work\1\s\src\FSharp.Core\list.fs:line 294
   at FSharp.Data.GraphQL.Execution.coerceVariables(FSharpList`1 variables, FSharpMap`2 vars) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Execution.fs:line 655
   at <StartupCode$FSharp-Data-GraphQL-Server>.$Executor.clo@111-4.Invoke(Unit unitVar) in /Users/valber/dev/forks/FSharp.Data.GraphQL/src/FSharp.Data.GraphQL.Server/Executor.fs:line 113
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult](AsyncActivation`1 ctxt, TResult result1, FSharpFunc`2 part2) in D:\a\_work\1\s\src\FSharp.Core\async.fs:line 508
   at <StartupCode$FSharp-Data-GraphQL-Server>.$Executor.clo@110-11.Invoke(AsyncActivation`1 ctxt)
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in D:\a\_work\1\s\src\FSharp.Core\async.fs:line 112",
  [])]
Expected: True
Actual:   False
  Stapelverfolgung:
     at Helpers.hasErrorWithExactMessage(String errMsg, IEnumerable`1 errors) in /Users/valber/dev/forks/FSharp.Data.GraphQL/tests/FSharp.Data.GraphQL.Tests/Helpers.fs:line 44
   at FSharp.Data.GraphQL.Tests.VariablesTests.Execute handles variables and errors on null for nested non-nulls() in /Users/valber/dev/forks/FSharp.Data.GraphQL/tests/FSharp.Data.GraphQL.Tests/VariablesTests.fs:line 178

Fehler!      : Fehler:     1, erfolgreich:     0, übersprungen:     0, gesamt:     1, Dauer: < 1 ms - /Users/valber/dev/forks/FSharp.Data.GraphQL/tests/FSharp.Data.GraphQL.Tests/bin/Debug/net6.0/FSharp.Data.GraphQL.Tests.dll (net6.0

Known workarounds

A possible (ugly) workaround would be to intercept the GraphQLResponse prior to sending it to the client (in the "HttpHandler's" code), then to cherry-pick only the exception message (removing everything else around it) and replace the original error message with this new one.

Related information

  • Operating system
    macOS Ventura 13.1
  • Branch
    dev
  • .NET Runtime, CoreCLR or Mono Version
    .NET 6.0
  • Performance information, links to performance testing scripts
    none
@valbers
Copy link
Collaborator Author

valbers commented Feb 7, 2023

Ok, I found out that this is already being addressed here: #418 (comment)

@valbers valbers closed this as completed Feb 7, 2023
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

No branches or pull requests

1 participant