graphql: Do graphql query/mutation validation in the mock server. #5362
Conversation
…roduce a skipIntrospection argument inside the custom directive.
There was a problem hiding this comment.
Still need to look at e2e tests for custom logic. Will do that by midnight.
Reviewed 45 of 48 files at r1.
Reviewable status: 42 of 48 files reviewed, 15 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/custom_logic/schemas/mixed-modes.graphql, line 14 at r1 (raw file):
method: "POST" operation: "batch" graphql: "query($input: [UserInput]) { userNames(users: $input) }"
there is no input UserInput {...} here in this schema. We need that to validate body for given graphql.
Same for other types too.
graphql/resolve/resolver.go, line 738 at r1 (raw file):
body := make(map[string]interface{}) body["query"] = fconf.RemoteGqlQuery // For batch mode, the argument name is input which is a list of maps.
This is the comment that led to the discovery of this bug. 😄
I guess we should change it now to not have input.
graphql/schema/custom_http_config_test.yaml, line 11 at r1 (raw file):
type Query { getCountry(id: ID!): Country! @custom(http: {
now we have a validation that query name can't be same as one of the queries that we may generate for given types (irrespective of @remote). So, can we change it to getCountry1 or something else...
graphql/schema/custom_http_config_test.yaml, line 33 at r1 (raw file):
type Query { country(code: ID!): Country! @custom(http: {
I guess we are requiring @custom in remote schema only because schema parsing would fail if we don't have it.
graphql/schema/custom_http_config_test.yaml, line 77 at r1 (raw file):
type Mutation { addCountry(input: CountryInput!): Country! @custom(http: {
addCountry1
graphql/schema/gqlschema_test.yml, line 1154 at r1 (raw file):
method: "POST", operation: "batch" graphql: "query { getAuthor(postId: {id: $id}) }",
shouldn't we add the variable definitions for query? And it should fail if the variable definitions are not there, right?
graphql/schema/gqlschema_test.yml, line 1164 at r1 (raw file):
# - # name: "@custom directive with batch mode on field and repeating arguments for query in graphql"
I have removed this test in my validations PR.
graphql/schema/rules.go, line 1137 at r1 (raw file):
"name `%s`, it can't have a name.", typ.Name, field.Name, graphqlOpDef.Name) } // TODO - Ensure that the variables used should be already defined.
Later, we may not need to do this explicitly, if we can construct remote schema from introspection response and validate the query against that using gqlparser.
graphql/schema/rules.go, line 1167 at r1 (raw file):
if len(query.Arguments) > 0 { argCountMap := make(map[string]int) requiredFields = make(map[string]bool)
I have also changed some bits here related to required fields in my validations PR, we will have to figure out conflicts.
graphql/schema/rules.go, line 1182 at r1 (raw file):
for _, arg := range query.Arguments[0].Value.Children[0].Value.Children { argCountMap[arg.Name] = argCountMap[arg.Name] + 1 requiredFields[arg.Value.String()[1:]] = true
Just a note:
I was thinking that as for batch operations, we now have required fields parsed from body itself, so currently they would get validated correctly. But later when we are adding scalar support for both body and graphql, then there are things like """block string""" and enum which are scalars for graphql but can't be present for json body. Then we will need to change how required fields are calculated for batch from body.
graphql/schema/rules.go, line 1302 at r1 (raw file):
var skip bool if si != nil { skip, _ = strconv.ParseBool(si.Raw)
Should we also verify that si.Raw is one of true/false only, and not ignore the error?
graphql/schema/wrappers.go, line 71 at r1 (raw file):
// For the following request // graphql: "query($sinput: [SchoolInput]) { schoolNames(schools: $sinput) }" // the GraphqlBatchModeArgument would be schools, we use it to know the GraphQL variable that
When we are sending variables won't we need sinput instead of schools?
I guess this should be GraphqlBatchModeVariable instead...
graphql/schema/wrappers.go, line 777 at r1 (raw file):
if !isQueryOrMutation && graphqlArg != nil && fconf.Operation == "single" { // For batch mode, required args would have been parsed from the body above. fconf.RequiredArgs, _ = parseRequiredArgsFromGQLRequest(graphqlArg.Raw)
should we ignore this error?
graphql/schema/wrappers.go, line 797 at r1 (raw file):
qfield := queryDoc.Operations[0].SelectionSet[0].(*ast.Field) if fconf.Operation == "batch" { fconf.GraphqlBatchModeArgument = qfield.Arguments[0].Value.String()[1:]
it is equivalent to: fconf.GraphqlBatchModeArgument = queryDoc.Operations[0].VariableDefinitions[0].Variable
graphql/schema/wrappers_test.go, line 672 at r1 (raw file):
{ "parse required args for single request", "query($id: ID!) { userNames(id: $id, age: $age) }",
don't we need $age in query variable definitions?
graphql/schema/wrappers_test.go, line 773 at r1 (raw file):
require.True(t, ok) require.Equal(t, tcase.RemoteQuery, c.RemoteGqlQuery)
can we also add a check that tmpl["query"] is same as tcase.RemoteQuery along with the existing check. It would just make things more clear I guess...
There was a problem hiding this comment.
Reviewable status: 42 of 48 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/resolve/resolver.go, line 738 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
This is the comment that led to the discovery of this bug. 😄
I guess we should change it now to not haveinput.
Done. Removed
graphql/schema/custom_http_config_test.yaml, line 11 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
now we have a validation that query name can't be same as one of the queries that we may generate for given types (irrespective of
@remote). So, can we change it togetCountry1or something else...
Done.
graphql/schema/custom_http_config_test.yaml, line 33 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
I guess we are requiring
@customin remote schema only because schema parsing would fail if we don't have it.
yes that is right and I wanted to use our code for validating the query against a schema (remote schema in this case)
graphql/schema/custom_http_config_test.yaml, line 77 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
addCountry1
Done.
graphql/schema/rules.go, line 1182 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Just a note:
I was thinking that as for batch operations, we now have required fields parsed frombodyitself, so currently they would get validated correctly. But later when we are adding scalar support for both body and graphql, then there are things like"""block string"""andenumwhich are scalars for graphql but can't be present for json body. Then we will need to change how required fields are calculated for batch frombody.
Good point. May want to put that in code somewhere.
graphql/schema/wrappers.go, line 71 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
When we are sending variables won't we need
sinputinstead ofschools?
I guess this should beGraphqlBatchModeVariableinstead...
yeah you are right, the comment is wrong.
There was a problem hiding this comment.
Reviewed 2 of 48 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/custom_logic/schemas/mixed-modes.graphql, line 14 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
there is no
input UserInput {...}here in this schema. We need that to validatebodyfor givengraphql.
Same for other types too.
Actually, we won't need the variable type to be present in our schema for batched fields as we can gather the type's structure from given body. But we will need it for all other cases, as we need to do deep type checking.
graphql/schema/rules.go, line 1137 at r2 (raw file):
"name `%s`, it can't have a name.", typ.Name, field.Name, graphqlOpDef.Name) } if graphqlOpDef.VariableDefinitions != nil {
There are a few checks I think we need to do as per the new format:
- I guess ATM as we are not handling scalars, so not defining any variables itself should be an error, right? i.e., having variable definitions is mandatory as of now unless the query has no arguments.
- Then, the query arguments can use only those variables that have been defined in variable definitions.
Later, when we add query validation using schema generated from introspection, such checks would no longer be required though.
graphql/schema/rules.go, line 1144 at r2 (raw file):
return gqlerror.ErrorPosf(graphql.Position, "Type %s; Field %s; @custom directive, graphql variables must use "+ "fields defined within the type, found `%s`.", typ.Name,
This check was earlier happening in remote.go, but now it is better to do it here as we have variable definitions now.
I guess we will need to refactor remote.go to fit according to new needs.
graphql/schema/rules.go, line 1151 at r2 (raw file):
// For batch operation we already verify that body should use fields defined inside the // parent type. if !isBatchOperation {
There were a couple more checks for this case, which were happening by the same logic as for batched fields and had tests too. So, if we just create the requiredFields map from these variable definitions, then those checks should automatically happen, and we won't need these checks here. Also, we should remove the creation of requiredFields using query arguments now. But before that we need to merge master in it, as there were some changes in this file from my PR, and I doubt that they may lead to conflicts.
github-actions
bot
added
the
area/graphql
There was a problem hiding this comment.
Reviewable status: 5 of 49 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @MichaelJCompton)
graphql/e2e/custom_logic/schemas/mixed-modes.graphql, line 14 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Actually, we won't need the variable type to be present in our schema for batched fields as we can gather the type's structure from given
body. But we will need it for all other cases, as we need to do deep type checking.
Not needing this right now, will add a card for this.
graphql/schema/gqlschema_test.yml, line 1154 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
shouldn't we add the variable definitions for query? And it should fail if the variable definitions are not there, right?
Done.
graphql/schema/rules.go, line 1302 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Should we also verify that
si.Rawis one of true/false only, and not ignore the error?
Done.
graphql/schema/rules.go, line 1137 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
There are a few checks I think we need to do as per the new format:
- I guess ATM as we are not handling scalars, so not defining any variables itself should be an error, right? i.e., having variable definitions is mandatory as of now unless the query has no arguments.
- Then, the query arguments can use only those variables that have been defined in variable definitions.
Later, when we add query validation using schema generated from introspection, such checks would no longer be required though.
So I now check that required fields should have been variable definitions which should cover query/mutation/single mode. I have a separate check for batch mode. Hope that handles all the cases.
graphql/schema/rules.go, line 1151 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
There were a couple more checks for this case, which were happening by the same logic as for batched fields and had tests too. So, if we just create the
requiredFieldsmap from these variable definitions, then those checks should automatically happen, and we won't need these checks here. Also, we should remove the creation ofrequiredFieldsusing query arguments now. But before that we need to merge master in it, as there were some changes in this file from my PR, and I doubt that they may lead to conflicts.
Merged master, can you have a look again?
graphql/schema/wrappers.go, line 777 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
should we ignore this error?
Still ignoring but added a comment on why.
graphql/schema/wrappers_test.go, line 672 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
don't we need
$agein query variable definitions?
We do fixed
graphql/schema/wrappers_test.go, line 773 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
can we also add a check that
tmpl["query"]is same astcase.RemoteQueryalong with the existing check. It would just make things more clear I guess...
That is only true in the case of custom query/mutation when this function is called.
There was a problem hiding this comment.
Got some minor comments and then
Later we should add a task to refactor remote.go to validate the new format correctly for all cases. For now it will work for everything other that batch mode, but those can be improved, given we have the new format.
Reviewed 44 of 44 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/rules.go, line 1151 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Merged master, can you have a look again?
So, here now we just need to do this:
requiredFields = make(map[string]bool)
for _, vd := range graphqlOpDef.VariableDefinitions {
requiredFields[vd.Variable] = true
}
It will take care of applying these checks along with some other checks in validation no. 11.
And then we need to remove some unnecessary code as mentioned in next comment.
graphql/schema/rules.go, line 1331 at r3 (raw file):
Quoted 22 lines of code…
} else { var bodyBuilder strings.Builder comma := "," bodyBuilder.WriteString("{") for i, arg := range query.Arguments { if i == len(query.Arguments)-1 { comma = "" } bodyBuilder.WriteString(arg.Name) bodyBuilder.WriteString(":") bodyBuilder.WriteString(arg.Value.String()) bodyBuilder.WriteString(comma) } bodyBuilder.WriteString("}") _, requiredFields, err = parseBodyTemplate(bodyBuilder.String()) if err != nil { return gqlerror.ErrorPosf(graphql.Position, "Type %s; Field %s: inside graphql in @custom directive, "+ "error in parsing arguments for %s `%s`: %s.", typ.Name, field.Name, graphqlOpDef.Operation, query.Name, err.Error()) } }
This else block can be removed now, as it no longer makes sense. It is using argument names to construct requiredFields, instead we should be using names from variable definitions to construct requiredFields as mentioned above.
graphql/schema/rules.go, line 1352 at r3 (raw file):
query := graphqlOpDef.SelectionSet[0].(*ast.Field) argVal := query.Arguments[0].Value.String() vd := graphqlOpDef.VariableDefinitions.ForName(argVal[1:])
I guess we can just do:
argVal := query.Arguments[0].Value.Raw
vd := graphqlOpDef.VariableDefinitions.ForName(argVal)
It directly gives variable name instead of attaching $ to it.
|
Seems like some tests are failing because schema isn't getting updated fast enough. Will add a sleep for them as they pass locally. |
There was a problem hiding this comment.
Reviewable status: 46 of 49 files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @MichaelJCompton)
graphql/schema/rules.go, line 1151 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
So, here now we just need to do this:
requiredFields = make(map[string]bool) for _, vd := range graphqlOpDef.VariableDefinitions { requiredFields[vd.Variable] = true }It will take care of applying these checks along with some other checks in validation no. 11.
And then we need to remove some unnecessary code as mentioned in next comment.
Done.
graphql/schema/rules.go, line 1331 at r3 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
} else { var bodyBuilder strings.Builder comma := "," bodyBuilder.WriteString("{") for i, arg := range query.Arguments { if i == len(query.Arguments)-1 { comma = "" } bodyBuilder.WriteString(arg.Name) bodyBuilder.WriteString(":") bodyBuilder.WriteString(arg.Value.String()) bodyBuilder.WriteString(comma) } bodyBuilder.WriteString("}") _, requiredFields, err = parseBodyTemplate(bodyBuilder.String()) if err != nil { return gqlerror.ErrorPosf(graphql.Position, "Type %s; Field %s: inside graphql in @custom directive, "+ "error in parsing arguments for %s `%s`: %s.", typ.Name, field.Name, graphqlOpDef.Operation, query.Name, err.Error()) } }This else block can be removed now, as it no longer makes sense. It is using argument names to construct
requiredFields, instead we should be using names from variable definitions to constructrequiredFieldsas mentioned above.
Doesn't work without this because arguments are not a subset of variable definitions. There might be arguments which are not defined in variable definitions.
graphql/schema/rules.go, line 1352 at r3 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
I guess we can just do:
argVal := query.Arguments[0].Value.Raw vd := graphqlOpDef.VariableDefinitions.ForName(argVal)It directly gives variable name instead of attaching
$to it.
Done.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r4, 1 of 1 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @MichaelJCompton)
…permodeinc#5362) GraphQL variables were not being defined in the query/mutation that were being sent to the remote server because of which those requests would have failed. In this PR the mock server has been modified to validate the incoming request and populate the arguments from the variables so that the end to end tests are more real.
@abhimanyusinghgaur and I realized that GraphQL variables were not being defined in the query/mutation that were being sent to the remote server because of which those requests would have failed. I am modifying the mock server to validate the incoming request and populate the arguments from the variables so that the end to end tests are more real. We'll have to fix the code so that the generated queries/mutation define the variables that they actually use.
TODO
This change is
Docs Preview: