Update gqlparser and gqlgen dependencies

StevenACoffman commented 1 year ago

This updates both gqlparser and gqlgen dependencies. This was prompted by an error report in gqlparser.

gqlparser also preserves comments now, so some test cases might need tweaking here.

Per @tduong2049 in https://github.com/vektah/gqlparser/issues/269

What happened?

I am using genqlient to generate GraphQL operations into Go code. It uses gqlparser to validate a provided graph before generating code. In this case, I am using a supergraph composed from multiple subgraphs via Rover CLI.

Prior to v2.5.2, gqlparser parses the supergraph and returns no errors. Upon upgrading to v2.5.2+, I get the following error when running go run github.com/Khan/genqlient: invalid schema: Argument extension for directive join__type cannot be null.

Possibly related to #258?

What did you expect?

No parsing errors when running: go run github.com/Khan/genqlient on a supergraph. However, a validation error is returned when parsing @join__type directives.

Minimal graphql.schema and models to reproduce

type AircraftManufacturer
  @join__type(graph: AIRCRAFT, key: "uuid")
{
  uuid: ID!
  name: String!
}

Signed-off-by: Steve Coffman steve@khanacademy.org

StevenACoffman commented 1 year ago

@benjaminjkraft Hmmm, I took a quick glance, and it's not obvious to me how to update the test expectations here. The tests now complain that the ast.Dump contains the comments:

              Comment: "# @genqlient\n"

csilvers commented 1 year ago

@benjaminjkraft Hmmm, I took a quick glance, and it's not obvious to me how to update the test expectations here. The tests now complain that the ast.Dump contains the comments:
              Comment: "# @genqlient\n"

Did you run UPDATE_SNAPSHOTS=1 go test ./...? It's not clear to me which tests are failing for you. Did you look at docs/CONTRIBUTING.md?

StevenACoffman commented 1 year ago

@csilvers Yeah, I updated the snapshots, but that doesn't update these particular expectations, which are reused for other purposes.

In parse_test.go TestParse it loads the fragments.graphql and fragments2.graphql files as expectations (or "golden" files) and compares that to scanning and parsing the Go files for the commented genqlient queries like:

        # @genqlient
        mutation MyMutation {
            myField
            myOtherField {
            ...MyFragment
            }
        }

The genqlient marker comment is appearing in the AST now, as gqlparser now preserves comments in the AST (for use in gqlfmt and better error reporting in gqlgen). However, if I update the corresponding fragments.graphql and fragments2.graphql files to have preceding # @genqlient comments that breaks a variety of other tests, even when updating snapshots.

For now I pushed a commit that just removes the comment lines from the ast.Dumps for the expectations, which is hacky, but clarifies where the problem is. I'm going to merge this as is and allow someone else to more elegantly solve it in a follow-up if they have the time and desire.

benjaminjkraft commented 1 year ago

The test fix seems fine, in theory it should probably remove comments from both sides of the assertion but we can fix that if/when it matters.

But I am a little worried about the changes where we now copy comments, including the # @genqlient, into the generated code (and thereby into the request to the server). It's both extra bytes on the wire (probably ok in practice, it's not like we are bothering to minify) and would mean that if your genqlient.yaml is loosely configured you could get circularity (if it tries to extract queries from the generated file). Let me take a look at fixing that.

BTW, for future reference, as I said over in the gqlparser issue, we never need to update genqlient for users to get updated deps. When the changes affect genqlient users it's nice to do so, since users who run into an issue may update genqlient and not our deps, but it doesn't block users who do find the issue from updating to fix it, so there's never a rush. (Indeed in this case the issue didn't even exist in the previous version of gqlparser we required, the user only saw it because they were on v2.5.2+ even though genqlient required only v2.5.1. Which is a great thing for some people to do so we get reports of these issues! -- but means they are perfectly capable of pulling in the fix the same way.)

benjaminjkraft commented 1 year ago

Turns out we need vektah/gqlparser#271 to make it easy to remove the comments. If that's merged as-is, we just need to update and revert the snapshots changed in this PR (keeping the parse test changes, since I didn't change ast.Dump). If folks over there want to make comments the default, we'll just need to change our formatter to remove them.

Khan / genqlient