Repository: graph-gophers/graphql-go (go)

Issue:
Descriptions not parsed properly
**What version of the lib are you using?**
3e8838

**What did you do?**
Switched from comments (using `#`) to descriptions (using `"`, `"""`). Described some types but did not some others.

**What did you expect to see?**
When types are introspected, descriptions are properly assigned.

**What did you see instead?**
Non-described types have assigned description of the latest described types.

**To reproduce**
Run [this](https://gist.github.com/michaljemala/c6d2d38652d70b4ed563313f74092876) GraphQL server and introspect the types using
```
curl -XPOST -d '{"query":"{described: __type(name: \"Query\") {description} non-described: __type(name: \"NonDescribed\"){description}}"}' localhost:8080/query

{
  "data": {
    "described": {
      "description":" This is a described type. "
    },
    "nondescribed":{
        "description":" This is a described type. "
    }
  }
}
```
But you should see:
```
{
  "data": {
    "described": {
      "description":" This is a described type. "
    },
    "nondescribed":{
        "description":""
    }
  }
}
```


Pull request description:
Fix parsing of descriptions
Fixes #315.

Patch:
diff --git a/internal/common/lexer.go b/internal/common/lexer.go
index 8b3176c..9cc7e54 100644
--- a/internal/common/lexer.go
+++ b/internal/common/lexer.go
@@ -1,6 +1,7 @@
 package common
 
 import (
+	"bytes"
 	"fmt"
 	"strconv"
 	"strings"
@@ -14,7 +15,7 @@ type syntaxError string
 type Lexer struct {
 	sc                    *scanner.Scanner
 	next                  rune
-	descComment           string
+	comment               bytes.Buffer
 	useStringDescriptions bool
 }
 
@@ -58,9 +59,7 @@ func (l *Lexer) Peek() rune {
 // The description is available from `DescComment()`, and will be reset every time `ConsumeWhitespace()` is
 // executed unless l.useStringDescriptions is set.
 func (l *Lexer) ConsumeWhitespace() {
-	if !l.useStringDescriptions {
-		l.descComment = ""
-	}
+	l.comment.Reset()
 	for {
 		l.next = l.sc.Scan()
 
@@ -79,7 +78,6 @@ func (l *Lexer) ConsumeWhitespace() {
 			// A comment can contain any Unicode code point except `LineTerminator` so a comment always
 			// consists of all code points starting with the '#' character up to but not including the
 			// line terminator.
-
 			l.consumeComment()
 			continue
 		}
@@ -95,22 +93,20 @@ func (l *Lexer) ConsumeWhitespace() {
 // If a description is found, consume any following comments as well
 //
 // http://facebook.github.io/graphql/June2018/#sec-Descriptions
-func (l *Lexer) consumeDescription() bool {
+func (l *Lexer) consumeDescription() string {
 	// If the next token is not a string, we don't consume it
-	if l.next == scanner.String {
-		// a triple quote string is an empty "string" followed by an open quote due to the way the parser treats strings as one token
-		l.descComment = ""
-		tokenText := l.sc.TokenText()
-		if l.sc.Peek() == '"' {
-			// Consume the third quote
-			l.next = l.sc.Next()
-			l.consumeTripleQuoteComment()
-		} else {
-			l.consumeStringComment(tokenText)
-		}
-		return true
+	if l.next != scanner.String {
+		return ""
+	}
+	// Triple quote string is an empty "string" followed by an open quote due to the way the parser treats strings as one token
+	var desc string
+	if l.sc.Peek() == '"' {
+		desc = l.consumeTripleQuoteComment()
+	} else {
+		desc = l.consumeStringComment()
 	}
-	return false
+	l.ConsumeWhitespace()
+	return desc
 }
 
 func (l *Lexer) ConsumeIdent() string {
@@ -147,12 +143,12 @@ func (l *Lexer) ConsumeToken(expected rune) {
 }
 
 func (l *Lexer) DescComment() string {
+	comment := l.comment.String()
+	desc := l.consumeDescription()
 	if l.useStringDescriptions {
-		if l.consumeDescription() {
-			l.ConsumeWhitespace()
-		}
+		return desc
 	}
-	return l.descComment
+	return comment
 }
 
 func (l *Lexer) SyntaxError(message string) {
@@ -166,12 +162,13 @@ func (l *Lexer) Location() errors.Location {
 	}
 }
 
-func (l *Lexer) consumeTripleQuoteComment() {
+func (l *Lexer) consumeTripleQuoteComment() string {
+	l.next = l.sc.Next()
 	if l.next != '"' {
 		panic("consumeTripleQuoteComment used in wrong context: no third quote?")
 	}
 
-	var comment string
+	var buf bytes.Buffer
 	var numQuotes int
 	for {
 		l.next = l.sc.Next()
@@ -180,24 +177,27 @@ func (l *Lexer) consumeTripleQuoteComment() {
 		} else {
 			numQuotes = 0
 		}
-		comment += string(l.next)
+		buf.WriteRune(l.next)
 		if numQuotes == 3 || l.next == scanner.EOF {
 			break
 		}
 	}
-	l.descComment += strings.TrimSpace(comment[:len(comment)-numQuotes])
+	val := buf.String()
+	val = val[:len(val)-numQuotes]
+	val = strings.TrimSpace(val)
+	return val
 }
 
-func (l *Lexer) consumeStringComment(str string) {
-	value, err := strconv.Unquote(str)
+func (l *Lexer) consumeStringComment() string {
+	val, err := strconv.Unquote(l.sc.TokenText())
 	if err != nil {
 		panic(err)
 	}
-	l.descComment += value
+	return val
 }
 
 // consumeComment consumes all characters from `#` to the first encountered line terminator.
-// The characters are appended to `l.descComment`.
+// The characters are appended to `l.comment`.
 func (l *Lexer) consumeComment() {
 	if l.next != '#' {
 		panic("consumeComment used in wrong context")
@@ -208,9 +208,8 @@ func (l *Lexer) consumeComment() {
 		l.sc.Next()
 	}
 
-	if l.descComment != "" && !l.useStringDescriptions {
-		// TODO: use a bytes.Buffer or strings.Builder instead of this.
-		l.descComment += "\n"
+	if l.comment.Len() > 0 {
+		l.comment.WriteRune('\n')
 	}
 
 	for {
@@ -218,10 +217,6 @@ func (l *Lexer) consumeComment() {
 		if next == '\r' || next == '\n' || next == scanner.EOF {
 			break
 		}
-
-		if !l.useStringDescriptions {
-			// TODO: use a bytes.Buffer or strings.Build instead of this.
-			l.descComment += string(next)
-		}
+		l.comment.WriteRune(next)
 	}
 }


Test patch:
diff --git a/internal/schema/schema_test.go b/internal/schema/schema_test.go
index e1239c2..0352a9e 100644
--- a/internal/schema/schema_test.go
+++ b/internal/schema/schema_test.go
@@ -1,113 +1,185 @@
 package schema_test
 
 import (
+	"fmt"
 	"testing"
 
-	"github.com/graph-gophers/graphql-go/errors"
 	"github.com/graph-gophers/graphql-go/internal/schema"
 )
 
-type parseTestCase struct {
-	description string
-	sdl         string
-	expected    *schema.Schema
-	err         error
-}
-
-var parseTests = []parseTestCase{{
-	description: "Parses interface definition",
-	sdl:         "interface Greeting { message: String! }",
-	expected: &schema.Schema{
-		Types: map[string]schema.NamedType{
-			"Greeting": &schema.Interface{
-				Name:   "Greeting",
-				Fields: []*schema.Field{{Name: "message"}},
-			}},
-	}}, {
-	description: "Parses type with description string",
-	sdl: `
-	"Single line description."
-	type Type {
-		field: String
-	}`,
-	expected: &schema.Schema{
-		Types: map[string]schema.NamedType{
-			"Type": &schema.Object{
-				Name: "Type",
-				Desc: "Single line description.",
-			}},
-	}}, {
-	description: "Parses type with multi-line description string",
-	sdl: `
-	"""
-	Multi-line description.
-	"""
-	type Type {
-		field: String
-	}`,
-	expected: &schema.Schema{
-		Types: map[string]schema.NamedType{
-			"Type": &schema.Object{
-				Name: "Type",
-				Desc: "Multi-line description.",
-			}},
-	}}, {
-	description: "Parses type with multi-line description and ignores comments",
-	sdl: `
-	"""
-	Multi-line description with ignored comments.
-	"""
-	# This comment should be ignored.
-	type Type {
-		field: String
-	}`,
-	expected: &schema.Schema{
-		Types: map[string]schema.NamedType{
-			"Type": &schema.Object{
-				Name: "Type",
-				Desc: "Multi-line description with ignored comments.",
-			}},
-	}},
-}
-
 func TestParse(t *testing.T) {
-	setup := func(t *testing.T) *schema.Schema {
-		t.Helper()
-		return schema.New()
-	}
-
-	for _, test := range parseTests {
-		t.Run(test.description, func(t *testing.T) {
-			t.Skip("TODO: add support for descriptions")
-			schema := setup(t)
-
-			err := schema.Parse(test.sdl, false)
-			if err != nil {
-				t.Fatal(err)
+	for _, test := range []struct {
+		name                  string
+		sdl                   string
+		useStringDescriptions bool
+		validateError         func(err error) error
+		validateSchema        func(s *schema.Schema) error
+	}{
+		{
+			name: "Parses interface definition",
+			sdl:  "interface Greeting { message: String! }",
+			validateSchema: func(s *schema.Schema) error {
+				const typeName = "Greeting"
+				typ, ok := s.Types[typeName].(*schema.Interface)
+				if !ok {
+					return fmt.Errorf("interface %q not found", typeName)
+				}
+				if want, have := 1, len(typ.Fields); want != have {
+					return fmt.Errorf("invalid number of fields: want %d, have %d", want, have)
+				}
+				const fieldName = "message"
+				if typ.Fields[0].Name != fieldName {
+					return fmt.Errorf("field %q not found", fieldName)
+				}
+				return nil
+			},
+		},
+		{
+			name: "Parses implementing type without providing required fields",
+			sdl:  `
+			interface Greeting { 
+				message: String!
+			} 
+			type Welcome implements Greeting {
+			}`,
+			validateError: func(err error) error {
+				if err == nil {
+					return fmt.Errorf("want error, have <nil>")
+				}
+				if want, have := `graphql: interface "Greeting" expects field "message" but "Welcome" does not provide it`, err.Error(); want != have {
+					return fmt.Errorf("unexpected error: want %q, have %q", want, have)
+				}
+				return nil
+			},
+		},
+		{
+			name: "Parses type with description string",
+			sdl: `
+			"Single line description."
+			type Type { 
+				field: String
+			}`,
+			useStringDescriptions: true,
+			validateSchema: func(s *schema.Schema) error {
+				const typeName = "Type"
+				typ, ok := s.Types[typeName].(*schema.Object)
+				if !ok {
+					return fmt.Errorf("type %q not found", typeName)
+				}
+				if want, have := "Single line description.", typ.Description(); want != have {
+					return fmt.Errorf("invalid description: want %q, have %q", want, have)
+				}
+				return nil
+			},
+		},
+		{
+			name: "Parses type with multi-line description string",
+			sdl: `
+			"""
+			Multi-line description.
+			"""
+			type Type {
+				field: String
+			}`,
+			useStringDescriptions: true,
+			validateSchema: func(s *schema.Schema) error {
+				const typeName = "Type"
+				typ, ok := s.Types[typeName].(*schema.Object)
+				if !ok {
+					return fmt.Errorf("type %q not found", typeName)
+				}
+				if want, have := "Multi-line description.", typ.Description(); want != have {
+					return fmt.Errorf("invalid description: want %q, have %q", want, have)
+				}
+				return nil
+			},
+		},
+		{
+			name: "Parses type with multi-line description and ignores comments",
+			sdl: `
+			"""
+			Multi-line description with ignored comments.
+			"""
+			# This comment should be ignored.
+			type Type {
+				field: String
+			}`,
+			useStringDescriptions: true,
+			validateSchema: func(s *schema.Schema) error {
+				const typeName = "Type"
+				typ, ok := s.Types[typeName].(*schema.Object)
+				if !ok {
+					return fmt.Errorf("type %q not found", typeName)
+				}
+				if want, have := "Multi-line description with ignored comments.", typ.Description(); want != have {
+					return fmt.Errorf("invalid description: want %q, have %q", want, have)
+				}
+				return nil
+			},
+		},
+		{
+			name: "Description is correctly parsed for non-described types",
+			sdl: `
+			"Some description."
+			scalar MyInt 
+			type Type { 
+				field: String
+			}`,
+			useStringDescriptions: true,
+			validateSchema: func(s *schema.Schema) error {
+				typ, ok := s.Types["Type"]
+				if !ok {
+					return fmt.Errorf("type %q not found", "Type")
+				}
+				if want, have := "", typ.Description(); want != have {
+					return fmt.Errorf("description does not match: want %q, have %q ", want, have)
+				}
+				return nil
+			},
+		},
+		{
+			name: "Multi-line comment is correctly parsed",
+			sdl: `
+			# Multi-line
+			# comment.
+			" This description should be ignored. "
+			scalar MyInt 
+			type Type { 
+				field: String
+			}`,
+			validateSchema: func(s *schema.Schema) error {
+				typ, ok := s.Types["MyInt"]
+				if !ok {
+					return fmt.Errorf("scalar %q not found", "MyInt")
+				}
+				if want, have := "Multi-line\ncomment.", typ.Description(); want != have {
+					return fmt.Errorf("description does not match: want %q, have %q ", want, have)
+				}
+				typ, ok = s.Types["Type"]
+				if !ok {
+					return fmt.Errorf("type %q not found", "Type")
+				}
+				if want, have := "", typ.Description(); want != have {
+					return fmt.Errorf("description does not match: want %q, have %q ", want, have)
+				}
+				return nil
+			},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			s := schema.New()
+			if err := s.Parse(test.sdl, test.useStringDescriptions); err != nil {
+				if test.validateError == nil {
+					t.Fatal(err)
+				}
+				if err := test.validateError(err); err != nil {
+					t.Fatal(err)
+				}
 			}
-
-			// TODO: verify schema is the same as expected.
-		})
-	}
-}
-
-func TestInvalidInterfaceImpl(t *testing.T) {
-	var tests = []parseTestCase{{
-		description: "Parses type Welcome that implements interface Greeting without providing required fields",
-		sdl:         "interface Greeting { message: String! } type Welcome implements Greeting {}",
-		err:         errors.Errorf(`interface "Greeting" expects field "message" but "Welcome" does not provide it`),
-	}}
-
-	setup := func(t *testing.T) *schema.Schema {
-		t.Helper()
-		return schema.New()
-	}
-	for _, test := range tests {
-		t.Run(test.description, func(t *testing.T) {
-			schema := setup(t)
-			err := schema.Parse(test.sdl, false)
-			if err == nil || err.Error() != test.err.Error() {
-				t.Fatal(err)
+			if test.validateSchema != nil {
+				if err := test.validateSchema(s); err != nil {
+					t.Fatal(err)
+				}
 			}
 		})
 	}