From 2ecbd709294f475fb7c238b637a832205b7f8e68 Mon Sep 17 00:00:00 2001 From: bbaker Date: Thu, 5 Jun 2025 14:55:50 +1000 Subject: [PATCH 01/15] Adding errorprone support --- build.gradle | 70 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/build.gradle b/build.gradle index b6124e744..bbe590a63 100644 --- a/build.gradle +++ b/build.gradle @@ -1,5 +1,7 @@ import java.text.SimpleDateFormat - +import org.jetbrains.kotlin.gradle.dsl.JvmTarget +import org.jetbrains.kotlin.gradle.dsl.KotlinVersion +import net.ltgt.gradle.errorprone.CheckSeverity plugins { id 'java' @@ -12,11 +14,28 @@ plugins { id "io.github.gradle-nexus.publish-plugin" version "2.0.0" id "groovy" id "me.champeau.jmh" version "0.7.3" + id "net.ltgt.errorprone" version '4.2.0' + // + // Kotlin just for tests - not production code + id 'org.jetbrains.kotlin.jvm' version '2.1.21' } java { toolchain { - languageVersion = JavaLanguageVersion.of(11) + languageVersion = JavaLanguageVersion.of(17) // build on 17 - release on 11 + } +} + +kotlin { + compilerOptions { + apiVersion = KotlinVersion.KOTLIN_2_0 + languageVersion = KotlinVersion.KOTLIN_2_0 + jvmTarget = JvmTarget.JVM_11 + javaParameters = true + freeCompilerArgs = [ + '-Xemit-jvm-type-annotations', + '-Xjspecify-annotations=strict', + ] } } @@ -97,19 +116,15 @@ jar { attributes('Automatic-Module-Name': 'com.graphqljava') } } -tasks.withType(GroovyCompile) { - // Options when compiling Java using the Groovy plugin. - // (Groovy itself defaults to UTF-8 for Groovy code) - options.encoding = 'UTF-8' - groovyOptions.forkOptions.memoryMaximumSize = "4g" -} + dependencies { - implementation 'org.antlr:antlr4-runtime:' + antlrVersion api 'com.graphql-java:java-dataloader:5.0.0' api 'org.reactivestreams:reactive-streams:' + reactiveStreamsVersion api "org.jspecify:jspecify:1.0.0" - antlr 'org.antlr:antlr4:' + antlrVersion + + implementation 'org.antlr:antlr4-runtime:' + antlrVersion implementation 'com.google.guava:guava:' + guavaVersion + testImplementation group: 'junit', name: 'junit', version: '4.13.2' testImplementation 'org.spockframework:spock-core:2.3-groovy-4.0' testImplementation 'net.bytebuddy:byte-buddy:1.17.5' @@ -129,9 +144,17 @@ dependencies { testImplementation 'org.testng:testng:7.11.0' // use for reactive streams test inheritance testImplementation "com.tngtech.archunit:archunit-junit5:1.4.1" + antlr 'org.antlr:antlr4:' + antlrVersion + // this is needed for the idea jmh plugin to work correctly jmh 'org.openjdk.jmh:jmh-core:1.37' jmh 'org.openjdk.jmh:jmh-generator-annprocess:1.37' + + errorprone 'com.uber.nullaway:nullaway:0.12.6' + errorprone 'com.google.errorprone:error_prone_core:2.37.0' + + // just tests - no Kotlin otherwise + testCompileOnly 'org.jetbrains.kotlin:kotlin-stdlib-jdk8' } shadowJar { @@ -218,6 +241,33 @@ compileJava { source file("build/generated-src"), sourceSets.main.java } +tasks.withType(GroovyCompile) { + // Options when compiling Java using the Groovy plugin. + // (Groovy itself defaults to UTF-8 for Groovy code) + options.encoding = 'UTF-8' + groovyOptions.forkOptions.memoryMaximumSize = "4g" +} + +tasks.withType(JavaCompile) { + options.release = 11 + options.errorprone { + disableAllChecks = true + check("NullAway", CheckSeverity.ERROR) + // + // end state has us with this config turned on - eg all classes + // + //option("NullAway:AnnotatedPackages", "graphql") + option("NullAway:OnlyNullMarked", "true") + option("NullAway:JSpecifyMode", "true") + } + // Include to disable NullAway on test code + if (name.toLowerCase().contains("test")) { + options.errorprone { + disable("NullAway") + } + } +} + generateGrammarSource { includes = ['Graphql.g4'] maxHeapSize = "64m" From 6439f97f212bb5d9f936fe7dfd00725fd210b09f Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Thu, 5 Jun 2025 18:25:18 +1000 Subject: [PATCH 02/15] Update build.gradle --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index bbe590a63..5bd521389 100644 --- a/build.gradle +++ b/build.gradle @@ -22,7 +22,7 @@ plugins { java { toolchain { - languageVersion = JavaLanguageVersion.of(17) // build on 17 - release on 11 + languageVersion = JavaLanguageVersion.of(21) // build on 21 - release on 11 } } From 08278776299dcad0d48aa201d2f046b54bffee11 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Thu, 12 Jun 2025 17:03:54 +1000 Subject: [PATCH 03/15] nullable types --- src/main/java/graphql/Assert.java | 3 --- .../PerLevelDataLoaderDispatchStrategy.java | 11 ++++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/main/java/graphql/Assert.java b/src/main/java/graphql/Assert.java index 90af6820b..872e19c05 100644 --- a/src/main/java/graphql/Assert.java +++ b/src/main/java/graphql/Assert.java @@ -1,7 +1,5 @@ package graphql; -import org.jspecify.annotations.NullMarked; - import java.util.Collection; import java.util.function.Supplier; import java.util.regex.Pattern; @@ -10,7 +8,6 @@ @SuppressWarnings("TypeParameterUnusedInFormals") @Internal -@NullMarked public class Assert { public static T assertNotNullWithNPE(T object, Supplier msg) { diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index ee80d4aa6..b3c681e5e 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -296,7 +296,8 @@ public void deferredOnFieldValue(String resultKey, FieldValueInfo fieldValueInfo CallStack callStack = getCallStack(parameters); boolean ready = callStack.lock.callLocked(() -> { callStack.deferredFragmentRootFieldsFetched.add(fieldValueInfo); - return callStack.deferredFragmentRootFieldsFetched.size() == parameters.getDeferredCallContext().getFields(); + DeferredCallContext deferredCallContext = Assert.assertNotNull(parameters.getDeferredCallContext()); + return callStack.deferredFragmentRootFieldsFetched.size() == deferredCallContext.getFields(); }); if (ready) { int curLevel = parameters.getPath().getLevel(); @@ -358,7 +359,7 @@ private void onFieldValuesInfoDispatchIfNeeded(List fieldValueIn // // thread safety: called with callStack.lock // - private Integer handleSubSelectionFetched(List fieldValueInfos, int subSelectionLevel, CallStack + private @Nullable Integer handleSubSelectionFetched(List fieldValueInfos, int subSelectionLevel, CallStack callStack) { callStack.increaseHappenedOnFieldValueCalls(subSelectionLevel); int expectedOnObjectCalls = getObjectCountForList(fieldValueInfos); @@ -426,7 +427,7 @@ private boolean dispatchIfNeeded(int level, CallStack callStack) { // // thread safety: called with callStack.lock // - private Integer getHighestReadyLevel(int startFrom, CallStack callStack) { + private @Nullable Integer getHighestReadyLevel(int startFrom, CallStack callStack) { int curLevel = callStack.highestReadyLevel; while (true) { if (!checkLevelImpl(curLevel + 1, callStack)) { @@ -499,7 +500,7 @@ void dispatch(int level, CallStack callStack) { } - public void dispatchDLCFImpl(Set resultPathsToDispatch, Integer level, CallStack callStack) { + private void dispatchDLCFImpl(Set resultPathsToDispatch, @Nullable Integer level, CallStack callStack) { // filter out all DataLoaderCFS that are matching the fields we want to dispatch List relevantResultPathWithDataLoader = new ArrayList<>(); @@ -570,7 +571,7 @@ public void run() { callStack.batchWindowOfDelayedDataLoaderToDispatch.clear(); callStack.batchWindowOpen = false; }); - dispatchDLCFImpl(resultPathToDispatch.get(), null, callStack); + dispatchDLCFImpl(Assert.assertNotNull(resultPathToDispatch.get()), null, callStack); } } From 13f89aa47ea0f4c5ac698c2f51ec617a11089a5e Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Thu, 12 Jun 2025 17:14:20 +1000 Subject: [PATCH 04/15] nullable types --- .../dataloader/PerLevelDataLoaderDispatchStrategy.java | 4 ++-- .../java/graphql/schema/DataFetchingEnvironmentImpl.java | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index 2b23d211d..56525bb5f 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -305,8 +305,8 @@ public void deferredOnFieldValue(String resultKey, FieldValueInfo fieldValueInfo CallStack callStack = getCallStack(parameters); boolean ready = callStack.lock.callLocked(() -> { callStack.deferredFragmentRootFieldsFetched.add(fieldValueInfo); - DeferredCallContext deferredCallContext = Assert.assertNotNull(parameters.getDeferredCallContext()); - return callStack.deferredFragmentRootFieldsFetched.size() == deferredCallContext.getFields(); + AlternativeCallContext alternativeCallContext = Assert.assertNotNull(parameters.getDeferredCallContext()); + return callStack.deferredFragmentRootFieldsFetched.size() == alternativeCallContext.getFields(); }); if (ready) { int curLevel = parameters.getPath().getLevel(); diff --git a/src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java b/src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java index b15e63949..9447f9cc9 100644 --- a/src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java +++ b/src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java @@ -22,6 +22,7 @@ import org.dataloader.DataLoader; import org.dataloader.DataLoaderRegistry; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.NullUnmarked; import org.jspecify.annotations.Nullable; import java.util.List; @@ -33,11 +34,15 @@ @Internal @NullMarked public class DataFetchingEnvironmentImpl implements DataFetchingEnvironment { + @Nullable private final Object source; private final Supplier> arguments; + @Nullable private final Object context; private final GraphQLContext graphQLContext; + @Nullable private final Object localContext; + @Nullable private final Object root; private final GraphQLFieldDefinition fieldDefinition; private final MergedField mergedField; @@ -265,6 +270,7 @@ public String toString() { '}'; } + @NullUnmarked public static class Builder { private Object source; From 94efefc998abe0b0db9405155ba9f3452ab04c11 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Thu, 12 Jun 2025 20:15:00 +1000 Subject: [PATCH 05/15] fix tests --- .../groovy/graphql/util/AnonymizerTest.groovy | 79 +++++++++---------- .../rules/ExecutableDefinitionsTest.groovy | 10 +-- 2 files changed, 44 insertions(+), 45 deletions(-) diff --git a/src/test/groovy/graphql/util/AnonymizerTest.groovy b/src/test/groovy/graphql/util/AnonymizerTest.groovy index 48f5ecde4..6865879f5 100644 --- a/src/test/groovy/graphql/util/AnonymizerTest.groovy +++ b/src/test/groovy/graphql/util/AnonymizerTest.groovy @@ -722,46 +722,45 @@ type Object1 { .print(result) then: - newSchema == """\ - schema @Directive1(argument1 : "stringValue1"){ - query: Object1 - } - - directive @Directive1(argument1: String! = "stringValue4") repeatable on SCHEMA | SCALAR | OBJECT | FIELD_DEFINITION | ARGUMENT_DEFINITION | INTERFACE | UNION | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION - - "Marks the field, argument, input field or enum value as deprecated" - directive @deprecated( - "The reason for the deprecation" - reason: String! = "No longer supported" - ) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION - - interface Interface1 @Directive1(argument1 : "stringValue12") { - field2: String - field3: Enum1 - } - - union Union1 @Directive1(argument1 : "stringValue21") = Object2 - - type Object1 @Directive1(argument1 : "stringValue8") { - field1: Interface1 @Directive1(argument1 : "stringValue9") - field4: Union1 @deprecated - } - - type Object2 implements Interface1 { - field2: String - field3: Enum1 - field5(argument2: InputObject1): String - } - - enum Enum1 @Directive1(argument1 : "stringValue15") { - EnumValue1 @Directive1(argument1 : "stringValue18") - EnumValue2 - } - - input InputObject1 @Directive1(argument1 : "stringValue24") { - inputField1: Int @Directive1(argument1 : "stringValue27") - } - """.stripIndent() + newSchema == """schema @Directive1(argument1 : "stringValue1"){ + query: Object1 +} + +directive @Directive1(argument1: String! = "stringValue4") repeatable on SCHEMA | SCALAR | OBJECT | FIELD_DEFINITION | ARGUMENT_DEFINITION | INTERFACE | UNION | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION + +"Marks the field, argument, input field or enum value as deprecated" +directive @deprecated( + "The reason for the deprecation" + reason: String! = "No longer supported" + ) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION + +interface Interface1 @Directive1(argument1 : "stringValue12") { + field2: String + field3: Enum1 +} + +union Union1 @Directive1(argument1 : "stringValue21") = Object2 + +type Object1 @Directive1(argument1 : "stringValue8") { + field1: Interface1 @Directive1(argument1 : "stringValue9") + field4: Union1 @deprecated +} + +type Object2 implements Interface1 { + field2: String + field3: Enum1 + field5(argument2: InputObject1): String +} + +enum Enum1 @Directive1(argument1 : "stringValue15") { + EnumValue1 @Directive1(argument1 : "stringValue18") + EnumValue2 +} + +input InputObject1 @Directive1(argument1 : "stringValue24") { + inputField1: Int @Directive1(argument1 : "stringValue27") +} +""" } def "query with directives"() { diff --git a/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy b/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy index 4b0e27848..b124ff900 100644 --- a/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy +++ b/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy @@ -68,10 +68,10 @@ class ExecutableDefinitionsTest extends Specification { !validationErrors.empty validationErrors.size() == 2 validationErrors[0].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[0].locations == [new SourceLocation(8, 1)] + validationErrors[0].locations == [new SourceLocation(8, 3)] validationErrors[0].message == "Validation error (NonExecutableDefinition) : Type 'Cow' definition is not executable" validationErrors[1].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[1].locations == [new SourceLocation(12, 1)] + validationErrors[1].locations == [new SourceLocation(12, 3)] validationErrors[1].message == "Validation error (NonExecutableDefinition) : Type 'Dog' definition is not executable" } @@ -92,10 +92,10 @@ class ExecutableDefinitionsTest extends Specification { !validationErrors.empty validationErrors.size() == 2 validationErrors[0].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[0].locations == [new SourceLocation(2, 1)] + validationErrors[0].locations == [new SourceLocation(2, 3)] validationErrors[0].message == "Validation error (NonExecutableDefinition) : Schema definition is not executable" validationErrors[1].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[1].locations == [new SourceLocation(6, 1)] + validationErrors[1].locations == [new SourceLocation(6, 3)] validationErrors[1].message == "Validation error (NonExecutableDefinition) : Type 'QueryRoot' definition is not executable" } @@ -128,7 +128,7 @@ class ExecutableDefinitionsTest extends Specification { !validationErrors.empty validationErrors.size() == 1 validationErrors[0].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[0].locations == [new SourceLocation(2, 1)] + validationErrors[0].locations == [new SourceLocation(2, 3)] validationErrors[0].message == "Validation error (NonExecutableDefinition) : Directive 'nope' definition is not executable" } From d9de9b4260eba08d018a1e874c44392112570604 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Fri, 13 Jun 2025 04:58:15 +1000 Subject: [PATCH 06/15] add custom contract annotation --- build.gradle | 6 ++-- src/main/java/graphql/Assert.java | 32 ++++++++++++++----- src/main/java/graphql/Contract.java | 26 +++++++++++++++ .../PerLevelDataLoaderDispatchStrategy.java | 4 +-- 4 files changed, 56 insertions(+), 12 deletions(-) create mode 100644 src/main/java/graphql/Contract.java diff --git a/build.gradle b/build.gradle index 28fd34ea1..a468bb697 100644 --- a/build.gradle +++ b/build.gradle @@ -1,7 +1,8 @@ -import java.text.SimpleDateFormat +import net.ltgt.gradle.errorprone.CheckSeverity import org.jetbrains.kotlin.gradle.dsl.JvmTarget import org.jetbrains.kotlin.gradle.dsl.KotlinVersion -import net.ltgt.gradle.errorprone.CheckSeverity + +import java.text.SimpleDateFormat plugins { id 'java' @@ -257,6 +258,7 @@ tasks.withType(JavaCompile) { // end state has us with this config turned on - eg all classes // //option("NullAway:AnnotatedPackages", "graphql") + option("NullAway:CustomContractAnnotations", "graphql.Contract") option("NullAway:OnlyNullMarked", "true") option("NullAway:JSpecifyMode", "true") } diff --git a/src/main/java/graphql/Assert.java b/src/main/java/graphql/Assert.java index 872e19c05..399cc8c1e 100644 --- a/src/main/java/graphql/Assert.java +++ b/src/main/java/graphql/Assert.java @@ -1,5 +1,8 @@ package graphql; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + import java.util.Collection; import java.util.function.Supplier; import java.util.regex.Pattern; @@ -8,6 +11,7 @@ @SuppressWarnings("TypeParameterUnusedInFormals") @Internal +@NullMarked public class Assert { public static T assertNotNullWithNPE(T object, Supplier msg) { @@ -17,42 +21,48 @@ public static T assertNotNullWithNPE(T object, Supplier msg) { throw new NullPointerException(msg.get()); } - public static T assertNotNull(T object) { + @Contract("null -> fail") + public static T assertNotNull(@Nullable T object) { if (object != null) { return object; } return throwAssert("Object required to be not null"); } - public static T assertNotNull(T object, Supplier msg) { + @Contract("null,_ -> fail") + public static T assertNotNull(@Nullable T object, Supplier msg) { if (object != null) { return object; } return throwAssert(msg.get()); } - public static T assertNotNull(T object, String constantMsg) { + @Contract("null,_ -> fail") + public static T assertNotNull(@Nullable T object, String constantMsg) { if (object != null) { return object; } return throwAssert(constantMsg); } - public static T assertNotNull(T object, String msgFmt, Object arg1) { + @Contract("null,_,_ -> fail") + public static T assertNotNull(@Nullable T object, String msgFmt, Object arg1) { if (object != null) { return object; } return throwAssert(msgFmt, arg1); } - public static T assertNotNull(T object, String msgFmt, Object arg1, Object arg2) { + @Contract("null,_,_,_ -> fail") + public static T assertNotNull(@Nullable T object, String msgFmt, Object arg1, Object arg2) { if (object != null) { return object; } return throwAssert(msgFmt, arg1, arg2); } - public static T assertNotNull(T object, String msgFmt, Object arg1, Object arg2, Object arg3) { + @Contract("null,_,_,_,_ -> fail") + public static T assertNotNull(@Nullable T object, String msgFmt, Object arg1, Object arg2, Object arg3) { if (object != null) { return object; } @@ -60,28 +70,33 @@ public static T assertNotNull(T object, String msgFmt, Object arg1, Object a } - public static void assertNull(T object, Supplier msg) { + @Contract("!null,_ -> fail") + public static void assertNull(@Nullable T object, Supplier msg) { if (object == null) { return; } throwAssert(msg.get()); } - public static void assertNull(T object) { + @Contract("!null -> fail") + public static void assertNull(@Nullable Object object) { if (object == null) { return; } throwAssert("Object required to be null"); } + @Contract("-> fail") public static T assertNeverCalled() { return throwAssert("Should never been called"); } + @Contract("_,_-> fail") public static T assertShouldNeverHappen(String format, Object... args) { return throwAssert("Internal error: should never happen: %s", format(format, args)); } + @Contract("-> fail") public static T assertShouldNeverHappen() { return throwAssert("Internal error: should never happen"); } @@ -93,6 +108,7 @@ public static Collection assertNotEmpty(Collection collection) { return collection; } + // @Contract("null,_-> fail") public static Collection assertNotEmpty(Collection collection, Supplier msg) { if (collection == null || collection.isEmpty()) { throwAssert(msg.get()); diff --git a/src/main/java/graphql/Contract.java b/src/main/java/graphql/Contract.java new file mode 100644 index 000000000..b2261de79 --- /dev/null +++ b/src/main/java/graphql/Contract.java @@ -0,0 +1,26 @@ +package graphql; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +/** + * Custom contract annotation used for jspecify and NullAway checks. + * + * This is the same as Spring does: we don't want any additional dependencies, therefore we define our own Contract annotation. + * + * @see Spring Framework Contract + * @see org.jetbrains.annotations.Contract + * @see + * NullAway custom contract annotations + */ +@Documented +@Target(ElementType.METHOD) +public @interface Contract { + + /** + * Describing the contract between call arguments and the returned value. + */ + String value() default ""; + +} diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index 56525bb5f..e63223815 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -305,8 +305,8 @@ public void deferredOnFieldValue(String resultKey, FieldValueInfo fieldValueInfo CallStack callStack = getCallStack(parameters); boolean ready = callStack.lock.callLocked(() -> { callStack.deferredFragmentRootFieldsFetched.add(fieldValueInfo); - AlternativeCallContext alternativeCallContext = Assert.assertNotNull(parameters.getDeferredCallContext()); - return callStack.deferredFragmentRootFieldsFetched.size() == alternativeCallContext.getFields(); + Assert.assertNotNull(parameters.getDeferredCallContext()); + return callStack.deferredFragmentRootFieldsFetched.size() == parameters.getDeferredCallContext().getFields(); }); if (ready) { int curLevel = parameters.getPath().getLevel(); From 6fcc7675d2dbad402429a40721b879337f296da2 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Fri, 13 Jun 2025 09:56:26 +1000 Subject: [PATCH 07/15] make test fail faster --- .../Issue1178DataLoaderDispatchTest.groovy | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy index 27f884f2c..436491842 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy @@ -6,10 +6,11 @@ import graphql.schema.DataFetcher import graphql.schema.DataFetchingEnvironment import graphql.schema.StaticDataFetcher import graphql.schema.idl.RuntimeWiring +import org.awaitility.Awaitility import org.dataloader.BatchLoader -import org.dataloader.DataLoader import org.dataloader.DataLoaderFactory import org.dataloader.DataLoaderRegistry +import spock.lang.RepeatUntilFailure import spock.lang.Specification import java.util.concurrent.CompletableFuture @@ -22,6 +23,7 @@ class Issue1178DataLoaderDispatchTest extends Specification { public static final int NUM_OF_REPS = 100 + @RepeatUntilFailure(maxAttempts = 20) def "shouldn't dispatch twice in multithreaded env"() { setup: def sdl = """ @@ -67,10 +69,10 @@ class Issue1178DataLoaderDispatchTest extends Specification { def wiring = RuntimeWiring.newRuntimeWiring() .type(newTypeWiring("Query") - .dataFetcher("getTodos", new StaticDataFetcher([[id: '1'], [id: '2'], [id: '3'], [id: '4'], [id: '5']]))) + .dataFetcher("getTodos", new StaticDataFetcher([[id: '1'], [id: '2'], [id: '3'], [id: '4'], [id: '5']]))) .type(newTypeWiring("Todo") - .dataFetcher("related", relatedDf) - .dataFetcher("related2", relatedDf2)) + .dataFetcher("related", relatedDf) + .dataFetcher("related2", relatedDf2)) .build() @@ -80,7 +82,7 @@ class Issue1178DataLoaderDispatchTest extends Specification { then: "execution shouldn't error" for (int i = 0; i < NUM_OF_REPS; i++) { - def result = graphql.execute(ExecutionInput.newExecutionInput() + def ei = ExecutionInput.newExecutionInput() .graphQLContext([(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_CHAINING): enableDataLoaderChaining]) .dataLoaderRegistry(dataLoaderRegistry) .query(""" @@ -115,8 +117,10 @@ class Issue1178DataLoaderDispatchTest extends Specification { } } } - }""").build()) - assert result.errors.empty + }""").build() + def resultCF = graphql.executeAsync(ei) + Awaitility.await().until { resultCF.isDone() } + assert resultCF.get().errors.empty } where: enableDataLoaderChaining << [true, false] From 971f2e4b3bb29f3a892600f0d83ce4ff13c36cf4 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Fri, 13 Jun 2025 10:44:39 +1000 Subject: [PATCH 08/15] groovy compatibility --- build.gradle | 3 +++ 1 file changed, 3 insertions(+) diff --git a/build.gradle b/build.gradle index a468bb697..7e929c0c9 100644 --- a/build.gradle +++ b/build.gradle @@ -246,6 +246,8 @@ tasks.withType(GroovyCompile) { // Options when compiling Java using the Groovy plugin. // (Groovy itself defaults to UTF-8 for Groovy code) options.encoding = 'UTF-8' + sourceCompatibility = '11' + targetCompatibility = '11' groovyOptions.forkOptions.memoryMaximumSize = "4g" } @@ -429,3 +431,4 @@ tasks.withType(GenerateModuleMetadata) { test { useJUnitPlatform() } + From 1d5d4f6b0691d97936561e77d1b5d2b4dcbc13e7 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Sat, 14 Jun 2025 21:38:32 +1000 Subject: [PATCH 09/15] fix concurrent problem in dispatching strategy improve test --- .../PerLevelDataLoaderDispatchStrategy.java | 5 ++++- .../Issue1178DataLoaderDispatchTest.groovy | 19 ++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index e63223815..68da3a352 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -513,7 +513,10 @@ private void dispatchDLCFImpl(Set resultPathsToDispatch, @Nullable Integ // filter out all DataLoaderCFS that are matching the fields we want to dispatch List relevantResultPathWithDataLoader = new ArrayList<>(); - for (ResultPathWithDataLoader resultPathWithDataLoader : callStack.allResultPathWithDataLoader) { + // we need to copy the list because the callStack.allResultPathWithDataLoader can be modified concurrently + // while iterating over it + ArrayList resultPathWithDataLoaders = new ArrayList<>(callStack.allResultPathWithDataLoader); + for (ResultPathWithDataLoader resultPathWithDataLoader : resultPathWithDataLoaders) { if (resultPathsToDispatch.contains(resultPathWithDataLoader.resultPath)) { relevantResultPathWithDataLoader.add(resultPathWithDataLoader); } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy index 436491842..300c3e937 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/Issue1178DataLoaderDispatchTest.groovy @@ -21,9 +21,8 @@ import static graphql.schema.idl.TypeRuntimeWiring.newTypeWiring class Issue1178DataLoaderDispatchTest extends Specification { - public static final int NUM_OF_REPS = 100 - @RepeatUntilFailure(maxAttempts = 20) + @RepeatUntilFailure(maxAttempts = 100) def "shouldn't dispatch twice in multithreaded env"() { setup: def sdl = """ @@ -81,11 +80,10 @@ class Issue1178DataLoaderDispatchTest extends Specification { .build() then: "execution shouldn't error" - for (int i = 0; i < NUM_OF_REPS; i++) { - def ei = ExecutionInput.newExecutionInput() - .graphQLContext([(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_CHAINING): enableDataLoaderChaining]) - .dataLoaderRegistry(dataLoaderRegistry) - .query(""" + def ei = ExecutionInput.newExecutionInput() + .graphQLContext([(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_CHAINING): enableDataLoaderChaining]) + .dataLoaderRegistry(dataLoaderRegistry) + .query(""" query { getTodos { __typename id related { id __typename @@ -118,10 +116,9 @@ class Issue1178DataLoaderDispatchTest extends Specification { } } }""").build() - def resultCF = graphql.executeAsync(ei) - Awaitility.await().until { resultCF.isDone() } - assert resultCF.get().errors.empty - } + def resultCF = graphql.executeAsync(ei) + Awaitility.await().until { resultCF.isDone() } + assert resultCF.get().errors.empty where: enableDataLoaderChaining << [true, false] From f2bf6f3b649284584101189854d1d1d404eee7a8 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Sat, 14 Jun 2025 22:32:48 +1000 Subject: [PATCH 10/15] run tests on 11 and 17 too --- build.gradle | 17 ++++++++++++++ .../rules/ExecutableDefinitionsTest.groovy | 23 ++++++++++--------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/build.gradle b/build.gradle index 7e929c0c9..ee2551909 100644 --- a/build.gradle +++ b/build.gradle @@ -432,3 +432,20 @@ test { useJUnitPlatform() } +tasks.register('testWithJava17', Test) { + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(17) + } + useJUnitPlatform() +} +tasks.register('testWithJava11', Test) { + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(11) + } + useJUnitPlatform() +} +test.dependsOn testWithJava17 +test.dependsOn testWithJava11 + + + diff --git a/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy b/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy index b124ff900..4002b14ab 100644 --- a/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy +++ b/src/test/groovy/graphql/validation/rules/ExecutableDefinitionsTest.groovy @@ -6,6 +6,7 @@ import graphql.validation.SpecValidationSchema import graphql.validation.ValidationError import graphql.validation.ValidationErrorType import graphql.validation.Validator +import org.codehaus.groovy.runtime.StringGroovyMethods import spock.lang.Specification class ExecutableDefinitionsTest extends Specification { @@ -46,7 +47,7 @@ class ExecutableDefinitionsTest extends Specification { } def 'Executable Definitions with type definition'() { - def query = """ + def query = StringGroovyMethods.stripIndent(""" query Foo { dog { name @@ -60,7 +61,7 @@ class ExecutableDefinitionsTest extends Specification { extend type Dog { color: String } - """.stripIndent() + """) when: def validationErrors = validate(query) @@ -68,15 +69,15 @@ class ExecutableDefinitionsTest extends Specification { !validationErrors.empty validationErrors.size() == 2 validationErrors[0].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[0].locations == [new SourceLocation(8, 3)] + validationErrors[0].locations == [new SourceLocation(8, 1)] validationErrors[0].message == "Validation error (NonExecutableDefinition) : Type 'Cow' definition is not executable" validationErrors[1].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[1].locations == [new SourceLocation(12, 3)] + validationErrors[1].locations == [new SourceLocation(12, 1)] validationErrors[1].message == "Validation error (NonExecutableDefinition) : Type 'Dog' definition is not executable" } def 'Executable Definitions with schema definition'() { - def query = """ + def query = StringGroovyMethods.stripIndent(""" schema { query: QueryRoot } @@ -84,7 +85,7 @@ class ExecutableDefinitionsTest extends Specification { type QueryRoot { test: String } - """.stripIndent() + """) when: def validationErrors = validate(query) @@ -92,10 +93,10 @@ class ExecutableDefinitionsTest extends Specification { !validationErrors.empty validationErrors.size() == 2 validationErrors[0].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[0].locations == [new SourceLocation(2, 3)] + validationErrors[0].locations == [new SourceLocation(2, 1)] validationErrors[0].message == "Validation error (NonExecutableDefinition) : Schema definition is not executable" validationErrors[1].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[1].locations == [new SourceLocation(6, 3)] + validationErrors[1].locations == [new SourceLocation(6, 1)] validationErrors[1].message == "Validation error (NonExecutableDefinition) : Type 'QueryRoot' definition is not executable" } @@ -117,9 +118,9 @@ class ExecutableDefinitionsTest extends Specification { } def 'Executable Definitions with no directive definition'() { - def query = """ + def query = StringGroovyMethods.stripIndent(""" directive @nope on INPUT_OBJECT - """.stripIndent() + """) when: def document = new Parser().parseDocument(query) def validationErrors = new Validator().validateDocument(SpecValidationSchema.specValidationSchema, document, Locale.ENGLISH) @@ -128,7 +129,7 @@ class ExecutableDefinitionsTest extends Specification { !validationErrors.empty validationErrors.size() == 1 validationErrors[0].validationErrorType == ValidationErrorType.NonExecutableDefinition - validationErrors[0].locations == [new SourceLocation(2, 3)] + validationErrors[0].locations == [new SourceLocation(2, 1)] validationErrors[0].message == "Validation error (NonExecutableDefinition) : Directive 'nope' definition is not executable" } From 0cece8db1cb178ae992ffd69168427fdbca0be27 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Sat, 14 Jun 2025 22:41:09 +1000 Subject: [PATCH 11/15] use java 21 to run gradle --- .github/workflows/master.yml | 4 ++-- .github/workflows/pull_request.yml | 4 ++-- .github/workflows/release.yml | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index dcb8b0e15..3de1aaade 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -17,10 +17,10 @@ jobs: steps: - uses: actions/checkout@v4 - uses: gradle/actions/wrapper-validation@v4 - - name: Set up JDK 11 + - name: Set up JDK 21 uses: actions/setup-java@v4 with: - java-version: '11' + java-version: '21' distribution: 'corretto' - name: build test and publish run: ./gradlew assemble && ./gradlew check --info && ./gradlew publishToSonatype closeAndReleaseSonatypeStagingRepository -x check --info --stacktrace diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 30d202f66..1300df6a1 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -21,10 +21,10 @@ jobs: steps: - uses: actions/checkout@v4 - uses: gradle/actions/wrapper-validation@v4 - - name: Set up JDK 11 + - name: Set up JDK 21 uses: actions/setup-java@v4 with: - java-version: '11' + java-version: '21' distribution: 'corretto' - name: build and test run: ./gradlew assemble && ./gradlew check --info --stacktrace diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2d630314c..d48e8cb66 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,10 +19,10 @@ jobs: steps: - uses: actions/checkout@v4 - uses: gradle/actions/wrapper-validation@v4 - - name: Set up JDK 11 + - name: Set up JDK 21 uses: actions/setup-java@v4 with: - java-version: '11' + java-version: '21' distribution: 'corretto' - name: build test and publish run: ./gradlew assemble && ./gradlew check --info && ./gradlew publishToSonatype closeAndReleaseSonatypeStagingRepository -x check --info --stacktrace From 29c2c37268ea34b7a49c7fa2c4e11bdb5d5aca1f Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Sat, 14 Jun 2025 22:47:10 +1000 Subject: [PATCH 12/15] tests reporting --- build.gradle | 57 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/build.gradle b/build.gradle index ee2551909..200fd95c6 100644 --- a/build.gradle +++ b/build.gradle @@ -307,6 +307,7 @@ artifacts { List failedTests = [] test { + useJUnitPlatform() testLogging { events "FAILED", "SKIPPED" exceptionFormat = "FULL" @@ -319,6 +320,43 @@ test { } } +tasks.register('testWithJava17', Test) { + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(17) + } + useJUnitPlatform() + testLogging { + events "FAILED", "SKIPPED" + exceptionFormat = "FULL" + } + + afterTest { TestDescriptor descriptor, TestResult result -> + if (result.getFailedTestCount() > 0) { + failedTests.add(descriptor) + } + } + +} +tasks.register('testWithJava11', Test) { + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(11) + } + useJUnitPlatform() + testLogging { + events "FAILED", "SKIPPED" + exceptionFormat = "FULL" + } + + afterTest { TestDescriptor descriptor, TestResult result -> + if (result.getFailedTestCount() > 0) { + failedTests.add(descriptor) + } + } +} +test.dependsOn testWithJava17 +test.dependsOn testWithJava11 + + /* * The gradle.buildFinished callback is deprecated BUT there does not seem to be a decent alternative in gradle 7 * So progress over perfection here @@ -428,24 +466,5 @@ tasks.withType(GenerateModuleMetadata) { enabled = false } -test { - useJUnitPlatform() -} - -tasks.register('testWithJava17', Test) { - javaLauncher = javaToolchains.launcherFor { - languageVersion = JavaLanguageVersion.of(17) - } - useJUnitPlatform() -} -tasks.register('testWithJava11', Test) { - javaLauncher = javaToolchains.launcherFor { - languageVersion = JavaLanguageVersion.of(11) - } - useJUnitPlatform() -} -test.dependsOn testWithJava17 -test.dependsOn testWithJava11 - From 4b5751e15c5ef50678956b225b06e07722e7d9ba Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Sat, 14 Jun 2025 23:04:19 +1000 Subject: [PATCH 13/15] error reporting --- .github/workflows/master.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 3de1aaade..d252aa945 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -28,4 +28,7 @@ jobs: uses: EnricoMi/publish-unit-test-result-action@v2.20.0 if: always() with: - files: '**/build/test-results/test/TEST-*.xml' + files: | + **/build/test-results/test/TEST-*.xml + **/build/test-results/testWithJava17/TEST-*.xml + **/build/test-results/testWithJava21/TEST-*.xml From d46e24766375b99b8e866f23de4cb9145b5d3cb7 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Sat, 14 Jun 2025 23:04:36 +1000 Subject: [PATCH 14/15] error reporting --- .github/workflows/master.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index d252aa945..d4da18ac6 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -30,5 +30,5 @@ jobs: with: files: | **/build/test-results/test/TEST-*.xml + **/build/test-results/testWithJava11/TEST-*.xml **/build/test-results/testWithJava17/TEST-*.xml - **/build/test-results/testWithJava21/TEST-*.xml From a05a319f4c46be58984129837fd765e9abb5bc75 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Tue, 17 Jun 2025 19:28:49 +1000 Subject: [PATCH 15/15] Ignore test when run on Java version > 11 --- src/main/java/graphql/Contract.java | 1 + .../graphql/schema/fetching/LambdaFetchingSupportTest.groovy | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/main/java/graphql/Contract.java b/src/main/java/graphql/Contract.java index b2261de79..93ba900bf 100644 --- a/src/main/java/graphql/Contract.java +++ b/src/main/java/graphql/Contract.java @@ -16,6 +16,7 @@ */ @Documented @Target(ElementType.METHOD) +@Internal public @interface Contract { /** diff --git a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy index 7dd5e4cb9..9ad4df39e 100644 --- a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy +++ b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy @@ -4,6 +4,7 @@ import graphql.Scalars import graphql.schema.GraphQLFieldDefinition import graphql.schema.PropertyDataFetcher import graphql.util.javac.DynamicJavacSupport +import spock.lang.IgnoreIf import spock.lang.Specification class LambdaFetchingSupportTest extends Specification { @@ -150,6 +151,7 @@ class LambdaFetchingSupportTest extends Specification { return GraphQLFieldDefinition.newFieldDefinition().name(fldName).type(Scalars.GraphQLString).build() } + @IgnoreIf({ System.getProperty("java.version").split('\\.')[0] as Integer > 11 }) def "different class loaders induce certain behaviours"() { String sourceCode = ''' package com.dynamic;