From 3685f3f66b9df71e32adbceb9c6d612dc1e0234b Mon Sep 17 00:00:00 2001
From: Ben Kraft <ben@benkraft.org>
Date: Wed, 18 May 2022 15:21:30 -0500
Subject: [PATCH] Validate against a case gqlparser doesn't catch (#197)

While writing tests at some point I came across an invalid query that
gqlparser doesn't catch, and which causes a panic for us.  Now we
validate for it and return a nice error instead of panicing.

Fixes #176.

Test plan: make check
---
 generate/generate.go                          | 32 ++++++++++++++++---
 .../testdata/errors/NoMutationType.graphql    |  1 +
 .../errors/NoMutationType.schema.graphql      |  1 +
 generate/testdata/errors/NoQueryType.graphql  |  1 +
 .../errors/NoQueryType.schema.graphql         |  1 +
 .../TestGenerateErrors-NoMutationType-graphql |  1 +
 .../TestGenerateErrors-NoQueryType-graphql    |  1 +
 7 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 generate/testdata/errors/NoMutationType.graphql
 create mode 100644 generate/testdata/errors/NoMutationType.schema.graphql
 create mode 100644 generate/testdata/errors/NoQueryType.graphql
 create mode 100644 generate/testdata/errors/NoQueryType.schema.graphql
 create mode 100644 generate/testdata/snapshots/TestGenerateErrors-NoMutationType-graphql
 create mode 100644 generate/testdata/snapshots/TestGenerateErrors-NoQueryType-graphql

diff --git a/generate/generate.go b/generate/generate.go
index 1ab34dad..6af6cf1d 100644
--- a/generate/generate.go
+++ b/generate/generate.go
@@ -224,16 +224,40 @@ func (g *generator) preprocessQueryDocument(doc *ast.QueryDocument) {
 	validator.Walk(g.schema, doc, &observers)
 }
 
+// validateOperation checks for a few classes of operations that gqlparser
+// considers valid but we don't allow, and returns an error if this operation
+// is invalid for genqlient's purposes.
+func (g *generator) validateOperation(op *ast.OperationDefinition) error {
+	opType, err := g.baseTypeForOperation(op.Operation)
+	switch {
+	case err != nil:
+		// (e.g. operation has subscriptions, which we don't support)
+		return err
+	case opType == nil:
+		// gqlparser should err here, but doesn't [1], so we err to prevent
+		// panics later.
+		// TODO(benkraft): Remove once gqlparser is fixed.
+		// [1] https://github.com/vektah/gqlparser/issues/221
+		return errorf(op.Position, "schema has no %v type", op.Operation)
+	}
+
+	if op.Name == "" {
+		return errorf(op.Position, "operations must have operation-names")
+	} else if goKeywords[op.Name] {
+		return errorf(op.Position, "operation name must not be a go keyword")
+	}
+
+	return nil
+}
+
 // addOperation adds to g.Operations the information needed to generate a
 // genqlient entrypoint function for the given operation.  It also adds to
 // g.typeMap any types referenced by the operation, except for types belonging
 // to named fragments, which are added separately by Generate via
 // convertFragment.
 func (g *generator) addOperation(op *ast.OperationDefinition) error {
-	if op.Name == "" {
-		return errorf(op.Position, "operations must have operation-names")
-	} else if goKeywords[op.Name] {
-		return errorf(op.Position, "operation name must not be a go keyword")
+	if err := g.validateOperation(op); err != nil {
+		return err
 	}
 
 	queryDoc := &ast.QueryDocument{
diff --git a/generate/testdata/errors/NoMutationType.graphql b/generate/testdata/errors/NoMutationType.graphql
new file mode 100644
index 00000000..682e969a
--- /dev/null
+++ b/generate/testdata/errors/NoMutationType.graphql
@@ -0,0 +1 @@
+mutation M { g }
diff --git a/generate/testdata/errors/NoMutationType.schema.graphql b/generate/testdata/errors/NoMutationType.schema.graphql
new file mode 100644
index 00000000..75cab938
--- /dev/null
+++ b/generate/testdata/errors/NoMutationType.schema.graphql
@@ -0,0 +1 @@
+type Query { f: String }
diff --git a/generate/testdata/errors/NoQueryType.graphql b/generate/testdata/errors/NoQueryType.graphql
new file mode 100644
index 00000000..c6ce7398
--- /dev/null
+++ b/generate/testdata/errors/NoQueryType.graphql
@@ -0,0 +1 @@
+query Q { f }
diff --git a/generate/testdata/errors/NoQueryType.schema.graphql b/generate/testdata/errors/NoQueryType.schema.graphql
new file mode 100644
index 00000000..8b8f8b92
--- /dev/null
+++ b/generate/testdata/errors/NoQueryType.schema.graphql
@@ -0,0 +1 @@
+type T { f: String }
diff --git a/generate/testdata/snapshots/TestGenerateErrors-NoMutationType-graphql b/generate/testdata/snapshots/TestGenerateErrors-NoMutationType-graphql
new file mode 100644
index 00000000..2c40a67d
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateErrors-NoMutationType-graphql
@@ -0,0 +1 @@
+testdata/errors/NoMutationType.graphql:1: schema has no mutation type
diff --git a/generate/testdata/snapshots/TestGenerateErrors-NoQueryType-graphql b/generate/testdata/snapshots/TestGenerateErrors-NoQueryType-graphql
new file mode 100644
index 00000000..aa00a565
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateErrors-NoQueryType-graphql
@@ -0,0 +1 @@
+testdata/errors/NoQueryType.graphql:1: schema has no query type