Earlier validation that document operations exist in the schema by dondonz · Pull Request #3361 · graphql-java/graphql-java

dondonz · 2023-10-24T04:52:55Z

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

…utation query to schema mismatch""

…idate-catch_operation_to_schema_mismatch

dondonz · 2024-01-10T00:30:12Z

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 · 2024-01-23T04:41:54Z

To do: check the reference implementation graphql-js

dondonz · 2024-06-04T21:22:32Z

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 · 2024-10-29T03:30:06Z

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 · 2024-12-08T23:28:19Z

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

…idate-catch_operation_to_schema_mismatch

github-actions · 2024-12-22T00:34:03Z

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 · 2024-12-22T06:42:25Z

src/test/groovy/graphql/GraphQLTest.groovy

        then:
        result.errors.size() == 1
-        result.errors[0].class == MissingRootTypeException
+        ((ValidationError) result.errors[0]).validationErrorType == ValidationErrorType.UnknownOperation


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.

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

dondonz · 2024-12-22T06:42:48Z

src/test/groovy/graphql/validation/rules/KnownDirectivesTest.groovy

+        type Subscription {
+            field: String
+        }
+


This test needed an adjustment to be valid

dondonz · 2024-12-22T06:43:15Z

src/main/java/graphql/validation/rules/KnownOperationTypes.java

+
+    private String formatOperation(OperationDefinition.Operation operation) {
+        return StringKit.capitalize(operation.name().toLowerCase());
+    }


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

dondonz · 2024-12-22T06:44:18Z

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


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

…idate-catch_operation_to_schema_mismatch

Revert "Revert "A test to show that validation rules do not catch a m…

8f8c6d1

…utation query to schema mismatch""

dondonz added the keep-open Tells Stale Bot to keep PRs and issues open label Dec 5, 2023

Merge branch 'master' into revert-3360-revert-2741-2740-parse_and_val…

28af525

…idate-catch_operation_to_schema_mismatch

dondonz added the spec-change Tracking GraphQL specification changes label Nov 24, 2024

Merge branch 'master' into revert-3360-revert-2741-2740-parse_and_val…

f8673e6

…idate-catch_operation_to_schema_mismatch

dondonz added 3 commits December 22, 2024 17:14

Add validation at document parse time

9a69970

Make operation capitalised rather than all caps

d6be93a

Adjust tests now that validation runs earlier

36e56e0

dondonz commented Dec 22, 2024

View reviewed changes

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 added this to the 23.x breaking changes milestone Dec 22, 2024

dondonz removed the keep-open Tells Stale Bot to keep PRs and issues open label Dec 22, 2024

dondonz added 2 commits February 5, 2025 15:52

Merge branch 'master' into revert-3360-revert-2741-2740-parse_and_val…

896440c

…idate-catch_operation_to_schema_mismatch

Add extra test to demonstrate all operations are checked in a document

c467a02

andimarek approved these changes Feb 6, 2025

View reviewed changes

dondonz merged commit 3710e68 into master Feb 6, 2025
2 checks passed

dondonz deleted the revert-3360-revert-2741-2740-parse_and_validate-catch_operation_to_schema_mismatch branch February 6, 2025 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Earlier validation that document operations exist in the schema#3361

Earlier validation that document operations exist in the schema#3361
dondonz merged 8 commits intomasterfrom
revert-3360-revert-2741-2740-parse_and_validate-catch_operation_to_schema_mismatch

dondonz commented Oct 24, 2023 •

edited

Loading

Uh oh!

dondonz commented Jan 10, 2024

Uh oh!

dondonz commented Jan 23, 2024

Uh oh!

dondonz commented Jun 4, 2024

Uh oh!

dondonz commented Oct 29, 2024

Uh oh!

dondonz commented Dec 8, 2024

Uh oh!

github-actions bot commented Dec 22, 2024 •

edited

Loading

Uh oh!

dondonz Dec 22, 2024

Uh oh!

dondonz Feb 5, 2025

Uh oh!

dondonz Dec 22, 2024

Uh oh!

dondonz Dec 22, 2024 •

edited

Loading

Uh oh!

dondonz Dec 22, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

dondonz commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dondonz commented Jan 10, 2024

Uh oh!

dondonz commented Jan 23, 2024

Uh oh!

dondonz commented Jun 4, 2024

Uh oh!

dondonz commented Oct 29, 2024

Uh oh!

dondonz commented Dec 8, 2024

Uh oh!

github-actions bot commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

dondonz Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

dondonz Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

dondonz Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

dondonz Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dondonz Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dondonz commented Oct 24, 2023 •

edited

Loading

github-actions bot commented Dec 22, 2024 •

edited

Loading

dondonz Dec 22, 2024 •

edited

Loading