nautilus/gateway

Input string with escaped double quote results in a Gateway error

pkosiec opened this issue · 5 comments

Hi,
I'm using Nautilus to aggregate a few GraphQL APIs. In one of the GraphQL APIs I have a mutation where user provides JSON string. In the string there are escaped double quotes.

Schema:

input ResourceInput {
    input: ResourceInputData
}

input ResourceInputData {
    parameters: String
    (...)
}

createResource(in: ResourceInput): Resource!

This is the mutation I'm doing:

mutation CreateResource {
  createResource(in: {
    input: {
      parameters: "{\"input1\": \"foo\", \"input2\": 2, \"input3\": { \"nested\": true }}"
    }
  }) {
    id
  }
}

When I'm doing such a mutation directly to my service (which uses GQLGen), it works fine. However, when I do such mutation via Gateway, I get:

{
  "data": {},
  "errors": [
    {
      "extensions": {
        "code": "GRAPHQL_PARSE_FAILED"
      },
      "message": "Expected :, found String"
    }
  ]
}

In logs I can just see:

WARNING Network Error: Expected :, found String

Which is apparently returned here:

log.Warn("Network Error: ", err)
.

Looks like this is a problem with marshaling/unmarshaling query parameters. Even the query with just a single escape of double quote results in a problem:

Input: parameters: "foo\""

Result:

{
  "data": {},
  "errors": [
    {
      "extensions": {
        "code": "GRAPHQL_PARSE_FAILED"
      },
      "message": "Expected Name, found <Invalid>"
    }
  ]
}

Do you have an idea where exactly the problem occurs? Thanks a lot!

Hello there ,
I just wanted to let you know that I started to investigate this issue.

Apparently it's a little bit deeper than I initially thought. I successfully reproduced the issue with a unit test case for https://github.com/nautilus/graphql printer:
pkosiec/graphql@144e309

--- FAIL: TestPrintQuery (0.01s)
    printer_test.go:731: 
                Error Trace:    printer_test.go:731
                Error:          Not equal: 
                                expected: "{\n  hello(json: \"{\\\"foo\\\": \\\"bar\\\"}\")\n}\n"
                                actual  : "{\n  hello(json: \"{\"foo\": \"bar\"}\")\n}\n"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,3 +1,3 @@
                                 {
                                -  hello(json: "{\"foo\": \"bar\"}")
                                +  hello(json: "{"foo": "bar"}")
                                 }
                Test:           TestPrintQuery
FAIL

Even when I bumped the dependency (replaced outdated github.com/carted/graphql@v0.7.6 with github.com/graphql-go/graphql latest master commit) the problem still persists. So it's definitely a bug in https://github.com/graphql-go/graphql/blob/master/language/printer/printer.go.

Tomorrow I will continue my investigation and hopefully I'll come up with a fix.

As I wrote above, the problem was down in the carted/graphql (some old fork of graphql-go/graphql), but it was still there in the graphql-go/graphql itself.

So it was a bit deeper in the dependency tree:

nautilus/gateway -> nautilus/graphql -> graphql-go/graphql

I reported the issue and submitted PR with fix:

Issue: graphql-go/graphql#586,
PR: graphql-go/graphql#587

Once the contribution is approved, I will create a PR with dependencies bump in both nautilus/graphql and github.com/nautilus/gateway repositories. If you would like to @AlecAivazis, then I can also include a new test case for that in the nautilus/graphql.

Cheers!

Thank you so much for hunting this down @pkosiec!

Adding a test to the nautilus/graphql package that covers this would be awesome. I would like to eventually move away from relying on that package to print queries, i just haven't found the time.

No problem, @AlecAivazis 🙂 I will add the test case in the same pull request where I will bump the graphql-go dependency. Hopefully that will happen very soon - it depends on how fast the guys from graphql-go approve & merge my PR. Fingers crossed it won't take long 🤞

Hi @AlecAivazis,
It took a bit of time to have the PR merged on the original graphql/graphql-go repository. However, today it was finally merged. That's why I created a PR in nautilus/graphql repo for bumping the dependency. As discussed, I also added a simple test case for the issue.

Please take a look: nautilus/graphql#18

Cheers!