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

Fix directives on fields with custom scalars #544

Merged
merged 2 commits into from
Feb 18, 2019

Conversation

enjoylife
Copy link
Contributor

@enjoylife enjoylife commented Feb 9, 2019

The bug occurs when using a custom defined scalar, an input type, and directive together.
E.g.

#schema.gql
scalar UUID
directive @direc(s: String!) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION
input CustomInput {
    stuff:[UUID]  @direc(s:"stuff")
}
// custom models
import "github.com/gofrs/uuid"
func MarshalUUID(id uuid.UUID) graphql.Marshaler{ /* ... */ }
func UnmarshalUUID(input interface{}) (uuid.UUID, error) { /* ... */ }

would generate:

// ...
// shouldn't be whole path of github.com/gofrs/...
if data, ok := tmp.([]*github.com/gofrs/uuid.UUID) ; ok {
    it.Stuff = data
} else {
    return it, fmt.Errorf(`unexpected type %T from directive, should be []*github.com/gofrs/uuid.UUID`, tmp)
}

now:

if data, ok := tmp.([]*uuid.UUID); ok {
    it.Stuff = data
} else {
    return it, fmt.Errorf(`unexpected type %T from directive, should be []*github.com/gofrs/uuid.UUID`, tmp)
}

@vektah
Copy link
Collaborator

vektah commented Feb 10, 2019

Needs a test

@enjoylife
Copy link
Contributor Author

enjoylife commented Feb 10, 2019

Added to the integration schema such that it will catch said bug during go generate.

Prior to diff running go generate with the added schema:

❯ go generate ./...
gofmt failed on generated.go: /Users/matt/Projects/gqlgen/codegen/testserver/generated.go:3856:37: expected ')', found '/' (and 5 more errors)
Generated github.com/99designs/gqlgen/codegen/testserver in 1.23s
Generated github.com/99designs/gqlgen/example/chat in 1.04s
Generated github.com/99designs/gqlgen/example/config in 1.04s
Generated github.com/99designs/gqlgen/example/dataloader in 1.33s
Generated github.com/99designs/gqlgen/example/scalars in 1.18s
Generated github.com/99designs/gqlgen/example/selection in 1.03s
Generated github.com/99designs/gqlgen/example/starwars in 1.15s
Generated github.com/99designs/gqlgen/example/todo in 1.17s
Generated github.com/99designs/gqlgen/example/type-system-extension in 1.08s
Generated github.com/99designs/gqlgen/integration in 1.24s

After diff it builds fine. @vektah I'm hoping that would cover the "test" ask? If not, I would need some guidance on what would be a good example test to follow/improve.

@enjoylife enjoylife force-pushed the fix-directive branch 3 times, most recently from 1d79b3e to 0b0e4a9 Compare February 10, 2019 05:20
…tion schema

Added to the integration schema such that the build will catch the directive bug in question.
@enjoylife
Copy link
Contributor Author

Not sure if the failure is from me force pushing or something else as it just works local.

@vektah
Copy link
Collaborator

vektah commented Feb 10, 2019

After diff it builds fine. @vektah I'm hoping that would cover the "test" ask? If not, I would need some guidance on what would be a good example test to follow/improve.

Yeah, that looks good. As long as it cant happen again without something breaking. Most of the templates are covered like this.

Not sure if the failure is from me force pushing or something else as it just works local.

Its failing because you've changed the testserver schema but havent run go generate ./... and committed the changes.

@vektah
Copy link
Collaborator

vektah commented Feb 18, 2019

I see, looks like theres some instability in the generated code. It looks unrelated to this change so Ill fix it on next.

@vektah vektah merged commit a119584 into 99designs:next Feb 18, 2019
@vektah vektah added the v0.8 For release in v0.8 label Feb 18, 2019
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
Fix directives on fields with custom scalars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.8 For release in v0.8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants