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

schema.parseUnionDef does not panic on invalid syntax #451

Closed
ssko1 opened this issue Apr 24, 2021 · 5 comments
Closed

schema.parseUnionDef does not panic on invalid syntax #451

ssko1 opened this issue Apr 24, 2021 · 5 comments

Comments

@ssko1
Copy link

ssko1 commented Apr 24, 2021

Overview

I was writing tests for #444 and noticed that parseUnionDef fails for a specific use case.

Current Behavior

Given the following union definition snippet

Foo = Bar Qux | Quux

And a small test snippet

func TestUnionFailCase(t *testing.T) {
	definition := "Foo = Bar Qux | Quux"

	lex := common.NewLexer(definition, false)
	lex.ConsumeWhitespace()

	func() {
		defer func() {
			if r := recover(); r == nil {
				t.Fatal("Test should panic, but didn't")
			}
		}()
		union := parseUnionDef(lex)
		fmt.Printf("union name: %q\n", union.Name)
		fmt.Printf("union typenames: %v\n", union.TypeNames)
	}()
}

The test produces an output:

=== RUN   TestUnionFailCase
union name: "Foo"
union typenames: [Bar]
    schema_internal_test.go:139: Test should panic, but didn't
--- FAIL: TestUnionFailCase (0.00s)
FAIL

Expected Behavior

parseUnionDef should panic and the test case should pass, but it does not. The syntax in the union definition snippet is invalid.

@pavelnikolov
Copy link
Member

@ssko1 Thank you very much for the unit test!

@suessflorian
Copy link
Contributor

Hey, this issue still unowned? Keen to give this a shot.

@pavelnikolov
Copy link
Member

Go for it. PRs are welcome.

@suessflorian
Copy link
Contributor

suessflorian commented Oct 2, 2021

Hey @pavelnikolov - only got a chance to look at this now and after a brief look I believe things are working as expected. I'll provide some sort of proof and then we can get this closed ay 💪

Lets go with the example with a simple schema example;

union Foo = Bar Qux | Quux

schema.parseSchema will switch on the gql keyword union and invoke parseUnionDef

func parseUnionDef(l *common.Lexer) *types.Union {
	union := &types.Union{Loc: l.Location(), Name: l.ConsumeIdent()}

	union.Directives = common.ParseDirectives(l)
	l.ConsumeToken('=')
	union.TypeNames = []string{l.ConsumeIdent()}
	for l.Peek() == '|' {
		l.ConsumeToken('|')
		union.TypeNames = append(union.TypeNames, l.ConsumeIdent())
	}

	return union
}

This function will correctly consume union Foo = Bar leaving Qux | Quux on the Lexer, returned to schema.parseSchema where identifier Qux will re-enter the switch and, as it should, hit the syntax error:

unexpected 'Qux', expecting "schema", "type", "enum", "interface", "union", "input", "scalar" or "directive"


Am I right in concluding we should leave this as is? In fact, it's exactly to spec, as GraphQL is kinda newline/whitespace agnostic (so trying to wait until a new line char is reached or such to enrich error message will not work)

@suessflorian
Copy link
Contributor

suessflorian commented Oct 2, 2021

You can verify this by playing around with the test here for example;

name: "Extend union contains type",

Just append to the union type in the test case sdl 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants