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 for update by reference of default variable values #161

Merged
merged 4 commits into from
Jan 26, 2022

Conversation

aurelijusbanelis
Copy link
Contributor

@aurelijusbanelis aurelijusbanelis commented Aug 26, 2021

So Field "..." argument "..." of type "...!" is required but not provided. validation would always work, despite the order of the calls.

Context of the issue

# Note: there is default enum value in variables
query SomeOperation ($locale: Locale! = DE) {
	myAction(myEnum: $locale) {
		id
	}
}
query SomeOperation {
	# Note: Not providing mandatory parameter: (myEnum: Locale!)
	myAction {
		id
	}
}
  • Before fix: Missing field not caught
  • After fix: Field "myAction" argument "myEnum" of type "Locale!" is required but not provided.

Cause

VariablesInAllowedPosition rule updates value.ExpectedType.NonNull = false, resulting in ProvidedRequiredArguments rule to have argDef.Type.NonNull changed on the next call.

"VariablesInAllowedPosition" rule updates "value.ExpectedType.NonNull = false",
resulting in "ProvidedRequiredArguments" rule to have "argDef.Type.NonNull" changed on the next call.
@coveralls
Copy link

coveralls commented Aug 26, 2021

Coverage Status

Coverage increased (+0.2%) to 92.342% when pulling 76372a2 on aurelijusbanelis:by-reference into 2a3d320 on vektah:master.

Both ast.Value.ExpectedType and ast.VariableDefinition.Type are the pointers,
and it is easy to update values by accidence.

Therefore creating a copy, so changes in one validation rule (E.g. VariablesInAllowedPosition)
would not affect the behavior of the other rule (ProvidedRequiredArguments).

Not doing deep copy, so change should not have a big impact for memory or CPU even for large GraphQL applications.

Testing private methods, to not be confused by complex golang syntax.
(those are pure functions, so in case testing package is changed – refactoring should not be hard)
@aurelijusbanelis aurelijusbanelis marked this pull request as ready for review August 26, 2021 08:51
@aurelijusbanelis
Copy link
Contributor Author

May be duplicate of #158

Conflicts:
	validator/validator_test.go - include both tests
It was possible to fix the single cause,
or to prevent accidental changes in future functions.

Assuming tests would cover future changes,
so removing additional check (that could in theory impact performance).
@aurelijusbanelis
Copy link
Contributor Author

@lwc I saw you have merged #158

So I left only test cases to catch similar mistakes in the future.

@StevenACoffman StevenACoffman merged commit 25e3630 into vektah:master Jan 26, 2022
@StevenACoffman
Copy link
Collaborator

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants