Change runtime wiring redefinition checks to be strict by default #3824

dondonz · 2025-02-15T09:46:15Z

The problem

As I reported in #3760 I was stumped by my code not working in prod where integration tests seemed fine. It turned out that the type wiring that I thought was brand new, was actually redefining another type in a long forgotten part of the codebase.

This was a particularly tough bug because the order that Spring for GraphQL loaded the type wirings was non-deterministic, so you never knew which type wiring you got in prod. Of course Spring for GraphQL hasn't done anything wrong here, it is hard to distinguish which of two different wirings you want to use.

I think I haven't noticed this as a problem before, because when writing small tests in this repo, the order that datafetchers (and other elements) are redefined/clobbered are predictable.

The solution

It turns out we already had a "strict mode" option to catch a number of type runtime wiring redefinitions, including type redefinition, as well as redefinitions on datafetchers, default datafetchers, scalars, and enums. However, it's not enabled by default so it's usually not switched on.

For most users, I think having strict checks for redefinition is the better default option. It is very strange to have two type runtime wirings for the same type, and one can clobber the other in an unpredictable way.

The only time we should allow more than 1 TypeRuntimeWiring definition for the same type is when adding DataFetchers for distinct child fields. By definition, we can only have one type resolver and one enum values provider in a TypeRuntimeWiring, so these must not be redefined.

This PR changes the default to strict checks set to true.

This will be a "breaking change", but it is possible to return to the previous behaviour.

dondonz · 2025-02-15T09:48:30Z

src/test/groovy/graphql/execution/incremental/DeferExecutionSupportIntegrationTest.groovy

-                .type(newTypeWiring("Page").dataFetcher("text", resolve("The page full text", 100)))
-                .type(newTypeWiring("Comment").dataFetcher("content", resolve("Full content", 100)))
-                .type(newTypeWiring("Comment").dataFetcher("author", resolve([name: "Author name"], 10)))
+                .type(newTypeWiring("Post")


Some test updates, extra datafetcher registrations should happen inside the same newTypeWiring builder

dondonz · 2025-02-15T09:48:55Z

src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy

@@ -193,15 +193,14 @@ class RuntimeWiringTest extends Specification {
        newWiring.fieldVisibility == fieldVisibility
    }

-    def "strict mode can stop certain redefinitions"() {
+    def "strict mode, on by default, can stop certain redefinitions"() {


Updating tests and adding new ones to lock in the change in default behaviour

github-actions · 2025-02-15T09:49:59Z

Test Results

311 files 311 suites 47s ⏱️
3 575 tests 3 569 ✅ 6 💤 0 ❌
3 664 runs 3 658 ✅ 6 💤 0 ❌

Results for commit 8d5436a.

♻️ This comment has been updated with latest results.

dondonz · 2025-02-15T10:08:47Z

src/main/java/graphql/schema/idl/RuntimeWiring.java

         */
+        @Deprecated(since = "2025-02-15", forRemoval = true)


Setting forRemoval = true to give clear indication this is going to be removed soon

dondonz · 2025-02-15T10:09:42Z

src/main/java/graphql/schema/idl/RuntimeWiring.java

@@ -307,6 +320,9 @@ public Builder type(TypeRuntimeWiring typeRuntimeWiring) {

            DataFetcher<?> defaultDataFetcher = typeRuntimeWiring.getDefaultDataFetcher();
            if (defaultDataFetcher != null) {
+                if (strictMode && defaultDataFetchers.containsKey(typeName)) {
+                    throw new StrictModeWiringException(format("The type %s already has a default data fetcher defined", typeName));
+                }


An edge case that wasn't previously covered: if a type wiring's default datafetcher was redefined at the runtime wiring level (and not the type wiring level), previously didn't raise an error

See test below to lock in the new behaviour

dondonz · 2025-02-15T10:10:43Z

src/test/groovy/graphql/execution/ExecutionStepInfoTest.groovy

+                .type(newTypeWiring("User")
+                        .dataFetcher("friends", friendsDF)
+                        .dataFetcher("mates", friendsDF)
+                )


Set datafetchers so it doesn't count as type redefinition

dondonz · 2025-02-15T10:11:14Z

src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy

        DataFetcher DF1 = env -> "x"
        DataFetcher DF2 = env -> "x"
        TypeResolver TR1 = env -> null
        EnumValuesProvider EVP1 = name -> null

        when:
        RuntimeWiring.newRuntimeWiring()
-                .strictMode()


Remove these lines, as the default is strictness on

dondonz · 2025-02-15T10:11:42Z

src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy

@@ -248,34 +243,91 @@ class RuntimeWiringTest extends Specification {
        e5.message == "The type Foo has already has a default data fetcher defined"
    }

-    def "overwrite default data fetchers if they are null"() {
-
+    def "strict mode, if set to off, won't stop certain redefinitions"() {


Tests to lock in the strict mode off case

dondonz · 2025-02-15T10:12:35Z

src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy


+        then:
+        def error = thrown(StrictModeWiringException)
+        error.message == "The type Foo already has a default data fetcher defined"


Covering a new edge case, where the default datafetcher is redefined at the runtime wiring level

We already had a test for when it is redefined inside the type wiring

dondonz · 2025-02-15T10:13:02Z

src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy

    }

    void cleanup() {
-        TypeRuntimeWiring.setStrictModeJvmWide(false)
+        TypeRuntimeWiring.setStrictModeJvmWide(true)


Changes to tests to reflect the new default

dondonz · 2025-02-15T10:13:18Z

src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy

-    def "strict mode can be turned on JVM wide"() {
-
-
+    def "strict mode can be turned off and on JVM wide"() {


Invert the test

…me type as long as they are not overlapping

…a non-overlapping child field

dondonz added 3 commits February 15, 2025 20:31

Change default strict mode to true and give option to turn it off

09f6a88

Add tests for new default behaviour

d28e95a

Update tests to not redefine types

bc45e3d

dondonz added this to the 23.x breaking changes milestone Feb 15, 2025

dondonz added the breaking change requires a new major version to be relased label Feb 15, 2025

dondonz commented Feb 15, 2025

View reviewed changes

Cover case where default datafetcher is redefined

e0b2c27

dondonz commented Feb 15, 2025

View reviewed changes

dondonz changed the title ~~WIP Make strict runtime wiring default~~ WIP Change runtime wiring redefinition checks to be strict by default Feb 15, 2025

dondonz changed the title ~~WIP Change runtime wiring redefinition checks to be strict by default~~ Change runtime wiring redefinition checks to be strict by default Feb 16, 2025

dondonz added 6 commits March 16, 2025 19:55

Merge branch 'master' into make-strict-runtime-wiring-default

fe1ac76

Reinstate tests, revised validation logic will handle this case

a086041

Merge branch 'master' into make-strict-runtime-wiring-default

a14fc33

Update deprecated date

2042ea4

Adjust tests to allow child datafetchers to be added again for the sa…

ee9570a

…me type as long as they are not overlapping

Allow more than 1 type runtime wiring if it is for a datafetcher for …

8d5436a

…a non-overlapping child field

bbakerman approved these changes Mar 31, 2025

View reviewed changes

dondonz merged commit cd99d17 into master Mar 31, 2025
2 checks passed

dondonz deleted the make-strict-runtime-wiring-default branch March 31, 2025 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change runtime wiring redefinition checks to be strict by default #3824

Change runtime wiring redefinition checks to be strict by default #3824

dondonz commented Feb 15, 2025 •

edited

Loading

dondonz Feb 15, 2025

dondonz Feb 15, 2025

github-actions bot commented Feb 15, 2025 •

edited

Loading

dondonz Feb 15, 2025

dondonz Feb 15, 2025

dondonz Feb 15, 2025

dondonz Feb 15, 2025

dondonz Feb 15, 2025 •

edited

Loading

dondonz Feb 15, 2025

dondonz Feb 15, 2025

dondonz Feb 15, 2025

		def "strict mode can be turned on JVM wide"() {


		def "strict mode can be turned off and on JVM wide"() {

Change runtime wiring redefinition checks to be strict by default #3824

Change runtime wiring redefinition checks to be strict by default #3824

Conversation

dondonz commented Feb 15, 2025 • edited Loading

The problem

The solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2025 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dondonz Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dondonz commented Feb 15, 2025 •

edited

Loading

github-actions bot commented Feb 15, 2025 •

edited

Loading

dondonz Feb 15, 2025 •

edited

Loading