Skip to content

Earlier validation that document operations exist in the schema #3361

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
Merged
merged 8 commits into from
Feb 6, 2025
Merged

Earlier validation that document operations exist in the schema #3361

merged 8 commits into from
Feb 6, 2025

Conversation

dondonz
Copy link
Member

@dondonz dondonz commented Oct 24, 2023 •
edited
Loading

Update: Dec 2024

This PR brings forward validation to check an operation exists in the schema.

Previously, this was checked at execution time. Now it will be checked at documentation validation time.

The motivation for this change was a change in the specification, which is currently in RFC2 and is about to be merged in. See graphql/graphql-spec#955

We also had folks from the community request this. Originally from #2741 and #2740.

I've aligned names with the reference implementation graphql/graphql-js@50d555c

@dondonz dondonz added the keep-open Tells Stale Bot to keep PRs and issues open label Dec 5, 2023
@dondonz
Copy link
Member Author

dondonz commented Jan 10, 2024

To chat about later - this problem would always be caught at the execution step, and I think execution is a more natural place to check this sort of problem

@dondonz
Copy link
Member Author

dondonz commented Jan 23, 2024

To do: check the reference implementation graphql-js

@dondonz
Copy link
Member Author

dondonz commented Jun 4, 2024

Interesting, this is a gap in the specification, which our .NET implementation friends have also found.

This is a proposal to formalise that mutation and subscription operations exist in the schema before execution.

Issue link: graphql/graphql-spec#1097
Spec proposal link: graphql/graphql-spec#1098

@dondonz
Copy link
Member Author

dondonz commented Oct 29, 2024

Coming back to this PR, a new spec change is landing soon which adds validation for this failing test case, where an operation doesn't exist in the schema.

This PR has all the changes: graphql/graphql-spec#955
This is another PR for context, but all changes were merged into the first one graphql/graphql-spec#1098

@dondonz dondonz added the spec-change Tracking GraphQL specification changes label Nov 24, 2024
@dondonz
Copy link
Member Author

dondonz commented Dec 8, 2024

3 days ago this RFC was promoted to the RFC 2 Draft stage graphql/graphql-spec#955

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 22, 2024 •
edited
Loading

Test Results

  306 files  ±0    306 suites  ±0   46s ⏱️ -1s
3 486 tests +3  3 481 ✅ +4  5 💤 ±0  0 ❌  - 1 
3 575 runs  +3  3 570 ✅ +4  5 💤 ±0  0 ❌  - 1 

Results for commit c467a02. ± Comparison against base commit d370d0b.

This pull request removes 151 and adds 132 tests. Note that renamed tests count towards both. 
	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.GraphQLTest ‑ null mutation type does not throw an npe but returns and error
graphql.ParseAndValidateTest ‑ known operation validation rule checks all operations in document
graphql.ParseAndValidateTest ‑ validation error raised if mutation operation does not exist in schema
graphql.ParseAndValidateTest ‑ validation error raised if subscription operation does not exist in schema
graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@3bfa9e44>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@167722e7>
graphql.ScalarsIDTest ‑ parseValue allows any object via String.valueOf <java.lang.Object@8d2c950>
graphql.ScalarsIDTest ‑ serialize allows any object via String.valueOf <java.lang.Object@40609006>
graphql.ScalarsIntTest ‑ parseValue throws exception for invalid input <java.lang.Object@11d5599e>
graphql.ScalarsIntTest ‑ serialize throws exception for invalid input <java.lang.Object@70ff3cce>
…

♻️ This comment has been updated with latest results.

dondonz
dondonz commented Dec 22, 2024
View reviewed changes
src/test/groovy/graphql/GraphQLTest.groovy
@@ -370,7 +370,7 @@ class GraphQLTest extends Specification {

then:
result.errors.size() == 1
result.errors[0].class == MissingRootTypeException
((ValidationError) result.errors[0]).validationErrorType == ValidationErrorType.UnknownOperation
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to discuss this

Previously, the same sort of problem would be caught at execution time, throwing a MissingRootTypeException if the operation could not be executed. Given that I've now moved forward validation, it's very unlikely the old exception will ever be triggered. However for safety I'd like to be defensive and leave in the old code. See SchemaUtil and getOperationRootType.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline - will keep in the old error in case, to be conservative

dondonz
dondonz commented Dec 22, 2024
View reviewed changes
src/test/groovy/graphql/validation/rules/KnownDirectivesTest.groovy
type Subscription {
field: String
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This test needed an adjustment to be valid

dondonz
dondonz commented Dec 22, 2024
View reviewed changes
src/main/java/graphql/validation/rules/KnownOperationTypes.java

private String formatOperation(OperationDefinition.Operation operation) {
return StringKit.capitalize(operation.name().toLowerCase());
}
Copy link
Member Author

@dondonz dondonz Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Our Operation enums are ALL CAPS and I thought that was too shouty

dondonz
dondonz commented Dec 22, 2024
View reviewed changes
src/main/java/graphql/validation/rules/KnownOperationTypes.java
addError(UnknownOperation, operationDefinition.getSourceLocation(), message);
} else if (documentOperation == OperationDefinition.Operation.QUERY
&& graphQLSchema.getQueryType() == null) {
// This is unlikely to happen, as a validated GraphQLSchema must have a Query type by definition
Copy link
Member Author

Choose a reason for hiding this comment

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

A valid GraphQLSchema should have already been checked for a query type, I am being defensive here

@dondonz dondonz changed the title A test to show that validation rules do not catch a mutation query to schema mismatch Earlier validation that document operations exist in the schema Dec 22, 2024
@dondonz dondonz added this to the 23.x breaking changes milestone Dec 22, 2024
@dondonz dondonz removed the keep-open Tells Stale Bot to keep PRs and issues open label Dec 22, 2024
@dondonz dondonz merged commit 3710e68 into master Feb 6, 2025
2 checks passed
@dondonz dondonz deleted the revert-3360-revert-2741-2740-parse_and_validate-catch_operation_to_schema_mismatch branch February 6, 2025 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andimarek andimarek andimarek approved these changes

Assignees
No one assigned
Labels
spec-change Tracking GraphQL specification changes
Projects
None yet
Milestone
23.0 breaking changes
Development

Successfully merging this pull request may close these issues.
2 participants
@dondonz @andimarek