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.

Sign up for GitHub

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

Jump to bottom

Fix Input Value Validation #330

Merged
merged 1 commit into from
May 13, 2019
Merged

Fix Input Value Validation #330

merged 1 commit into from
May 13, 2019

Conversation

dackroyd
Copy link
Contributor

@dackroyd dackroyd commented May 9, 2019

Validating Input values supplied via variable input are supplied with the correct input structure and values. This includes ensuring that enum embedded within input values are validated as well

@dackroyd
Copy link
Contributor Author

dackroyd commented May 9, 2019

Related to #325

@dackroyd
Copy link
Contributor Author

Please keep this on hold - I've realised that nesting Input types don't validate correctly, which I'll look at fixing

@dackroyd dackroyd force-pushed the bug/fix-input-validation branch from 542d5de to b5ef288 Compare May 10, 2019 09:17
dackroyd
dackroyd commented May 10, 2019
View reviewed changes
internal/validation/testdata/tests.json
@@ -1752,17 +1873,7 @@
"vars": {
"colors": "BROWN"
},
"errors": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case was actually an example where input coercion should have allowed the value, so expecting an error here was actually incorrect.

dackroyd
dackroyd commented May 10, 2019
View reviewed changes
internal/validation/validation.go
@@ -193,7 +193,8 @@ func validateValue(c *opContext, v *common.InputValue, val interface{}, t common
}
vv, ok := val.([]interface{})
if !ok {
c.addErr(v.Loc, "VariablesOfCorrectType", "Variable \"%s\" has invalid type %T.\nExpected type \"%s\", found %v.", v.Name.Name, val, t, val)
// Input coercion rules allow single items without wrapping array
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolves the issue with input coercion for lists where a single value is supplied in variables, without a wrapping list

@dackroyd dackroyd force-pushed the bug/fix-input-validation branch from b5ef288 to 0bbc9de Compare May 12, 2019 23:29
@dackroyd
Fix Input Value Validation
3572ff4 
Validating Input values supplied via variable input are supplied with
 the correct input structure and values. This includes ensuring that
 enum embedded within input values are validated as well
@dackroyd dackroyd force-pushed the bug/fix-input-validation branch from 0bbc9de to 3572ff4 Compare May 12, 2019 23:34
@pavelnikolov pavelnikolov merged commit 158e7b8 into graph-gophers:master May 13, 2019
@pavelnikolov
Copy link
Member

Thank you!

@marcoazn89
Copy link

marcoazn89 commented May 14, 2019 •
edited
Loading

Sorry if this is me not understanding Graphql but it seems that this line will trigger an error when the input is not there https://github.com/graph-gophers/graphql-go/pull/330/files#diff-bd40f10a5bef7a0945987430ac7762bdR223 which makes sense when the inputs are required. However, it should not do that if the input is optional and not present.

For example, it is failing if I don't pass a home.

input DogInput {
    dogId: ID
    dogName: String
    home: String
}

Let me know if this makes sense!

@pavelnikolov
Copy link
Member

@marcoazn89 thanks for reporting this. Do you have an example to reproduce an error?

@dackroyd dackroyd deleted the bug/fix-input-validation branch October 4, 2019 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavelnikolov pavelnikolov pavelnikolov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dackroyd @pavelnikolov @marcoazn89