Fix fieldsOnDisjointTypesMustMerge flag (#4320)

apollo-ast/api/apollo-ast.api

@@ -1010,6 +1010,8 @@ public final class com/apollographql/apollo3/ast/GqlnodeKt {
 public final class com/apollographql/apollo3/ast/GqloperationdefinitionKt {
 	public static final fun rootTypeDefinition (Lcom/apollographql/apollo3/ast/GQLOperationDefinition;Lcom/apollographql/apollo3/ast/Schema;)Lcom/apollographql/apollo3/ast/GQLTypeDefinition;
 	public static final fun validate (Lcom/apollographql/apollo3/ast/GQLOperationDefinition;Lcom/apollographql/apollo3/ast/Schema;Ljava/util/Map;)Ljava/util/List;
+	public static final fun validate (Lcom/apollographql/apollo3/ast/GQLOperationDefinition;Lcom/apollographql/apollo3/ast/Schema;Ljava/util/Map;Z)Ljava/util/List;
+	public static synthetic fun validate$default (Lcom/apollographql/apollo3/ast/GQLOperationDefinition;Lcom/apollographql/apollo3/ast/Schema;Ljava/util/Map;ZILjava/lang/Object;)Ljava/util/List;
 }
 
 public final class com/apollographql/apollo3/ast/GqlstringsKt {

apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/api.kt

@@ -26,9 +26,9 @@ fun BufferedSource.toSchema(filePath: String? = null): Schema = parseAsGQLDocume
  * See [parseAsGQLDocument] and [validateAsExecutable] for more granular error reporting
  */
 @ApolloExperimental
-fun BufferedSource.toExecutableDefinitions(schema: Schema, filePath: String? = null, fieldOnDisjointTypesMustMerge: Boolean = false): List<GQLDefinition> = parseAsGQLDocument(filePath)
+fun BufferedSource.toExecutableDefinitions(schema: Schema, filePath: String? = null, fieldsOnDisjointTypesMustMerge: Boolean = true): List<GQLDefinition> = parseAsGQLDocument(filePath)
     .valueAssertNoErrors()
-    .validateAsExecutable(schema, fieldOnDisjointTypesMustMerge)
+    .validateAsExecutable(schema, fieldsOnDisjointTypesMustMerge)
     .valueAssertNoErrors()
 
 /**
@@ -87,14 +87,14 @@ fun GQLDocument.validateAsSchemaAndAddApolloDefinition(): GQLResult<Schema> {
 /**
  * Validates the given document as an executable document.
  *
- * @param fieldOnDisjointTypesMustMerge if true, the FieldsInSetCanMerge validation is not run.
+ * @param fieldsOnDisjointTypesMustMerge Whether fields with different shape are disallowed to be merged in disjoint types.
  *
  * @return  a [GQLResult] containing the operation and fragment definitions in 'value', along with any potential issues
  */
 @ApolloExperimental
-fun GQLDocument.validateAsExecutable(schema: Schema, fieldOnDisjointTypesMustMerge: Boolean): GQLResult<List<GQLDefinition>> {
+fun GQLDocument.validateAsExecutable(schema: Schema, fieldsOnDisjointTypesMustMerge: Boolean = true): GQLResult<List<GQLDefinition>> {
   val fragments = definitions.filterIsInstance<GQLFragmentDefinition>().associateBy { it.name }
-  val issues = ExecutableValidationScope(schema, fragments, fieldOnDisjointTypesMustMerge).validate(this)
+  val issues = ExecutableValidationScope(schema, fragments, fieldsOnDisjointTypesMustMerge).validate(this)
   return GQLResult(definitions, issues)
 }
 
@@ -105,7 +105,7 @@ fun GQLDocument.validateAsExecutable(schema: Schema, fieldOnDisjointTypesMustMer
 fun GQLFragmentDefinition.inferVariables(
     schema: Schema,
     fragments: Map<String, GQLFragmentDefinition>,
-    fieldOnDisjointTypesMustMerge: Boolean
-) = ExecutableValidationScope(schema, fragments, fieldOnDisjointTypesMustMerge).inferFragmentVariables(this)
+    fieldsOnDisjointTypesMustMerge: Boolean
+) = ExecutableValidationScope(schema, fragments, fieldsOnDisjointTypesMustMerge).inferFragmentVariables(this)
 
 

apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/gqloperationdefinition.kt

@@ -9,8 +9,9 @@ fun GQLOperationDefinition.rootTypeDefinition(schema: Schema) = when (operationT
   else -> null
 }
 
+@JvmOverloads
 fun GQLOperationDefinition.validate(
     schema: Schema,
     fragments: Map<String, GQLFragmentDefinition>,
-    fieldOnDisjointTypesMustMerge: Boolean,
-) = ExecutableValidationScope(schema, fragments, fieldOnDisjointTypesMustMerge).validateOperation(this)
+    fieldsOnDisjointTypesMustMerge: Boolean = true,
+) = ExecutableValidationScope(schema, fragments, fieldsOnDisjointTypesMustMerge).validateOperation(this)

apollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/internal/ExecutableValidationScope.kt

@@ -45,12 +45,12 @@ import com.apollographql.apollo3.ast.sharesPossibleTypesWith
  * @param fragmentDefinitions: all the fragments in the current compilation unit.
  * This is required to check the type conditions as well as fields merging
  *
- * @param fieldOnDisjointTypesMustMerge if true, the FieldsInSetCanMerge validation is not run.
+ * @param fieldsOnDisjointTypesMustMerge Whether fields with different shape are disallowed to be merged in disjoint types.
  */
 internal class ExecutableValidationScope(
     private val schema: Schema,
     private val fragmentDefinitions: Map<String, GQLFragmentDefinition>,
-    private val fieldOnDisjointTypesMustMerge: Boolean,
+    private val fieldsOnDisjointTypesMustMerge: Boolean,
 ) : ValidationScope {
   override val typeDefinitions = schema.typeDefinitions
   override val directiveDefinitions = schema.directiveDefinitions
@@ -510,9 +510,6 @@ internal class ExecutableValidationScope(
   }
 
   private fun fieldsInSetCanMerge(fieldsWithParent: List<FieldWithParent>) {
-    if (fieldOnDisjointTypesMustMerge) {
-      return
-    }
     fieldsWithParent.groupBy { it.field.responseName() }
         .values
         .forEach { fieldsForName ->
@@ -643,6 +640,9 @@ internal class ExecutableValidationScope(
 
   // 5.3.2 2.1
   private fun haveSameResponseShape(fieldWithParentA: FieldWithParent, fieldWithParentB: FieldWithParent): Boolean {
+    if (!fieldsOnDisjointTypesMustMerge) {
+      return true
+    }
     val fieldA = fieldWithParentA.field
     val fieldB = fieldWithParentB.field
 

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ApolloCompiler.kt

@@ -83,7 +83,7 @@ object ApolloCompiler {
     val validationResult = GQLDocument(
         definitions = definitions + incomingFragments,
         filePath = null
-    ).validateAsExecutable(options.schema, options.fieldOnDisjointTypesMustMerge)
+    ).validateAsExecutable(options.schema, options.fieldsOnDisjointTypesMustMerge)
 
     validationResult.issues.checkNoErrors()
 
@@ -161,7 +161,7 @@ object ApolloCompiler {
         codegenModels = options.codegenModels,
         generateOptionalOperationVariables = options.generateOptionalOperationVariables,
         generateDataBuilders = options.generateDataBuilders,
-        fieldOnDisjointTypesMustMerge = options.fieldOnDisjointTypesMustMerge
+        fieldsOnDisjointTypesMustMerge = options.fieldsOnDisjointTypesMustMerge
     ).build()
 
     if (debugDir != null) {

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/Options.kt

@@ -192,11 +192,16 @@ class Options(
     val requiresOptInAnnotation: String? = defaultRequiresOptInAnnotation,
 
     /**
-     * Whether to merge fields in disjoint types without the FieldsInSetCanMerge validate.
+     * Whether fields with different shape are disallowed to be merged in disjoint types.
      *
-     * If true, disable the FieldsInSetCanMerge validation.
+     * Note: setting this to `false` relaxes the standard GraphQL [FieldsInSetCanMerge](https://spec.graphql.org/draft/#FieldsInSetCanMerge()) validation which may still be
+     * run on the backend.
+     *
+     * See also [issue 4320](https://github.com/apollographql/apollo-kotlin/issues/4320)
+     *
+     * Default: true.
      */
-    val fieldOnDisjointTypesMustMerge: Boolean = defaultFieldOnDisjointTypesMustMerge
+    val fieldsOnDisjointTypesMustMerge: Boolean = defaultFieldsOnDisjointTypesMustMerge,
 ) {
 
   /**
@@ -255,7 +260,7 @@ class Options(
       addJvmOverloads: Boolean = this.addJvmOverloads,
       addTypename: String = this.addTypename,
       requiresOptInAnnotation: String? = this.requiresOptInAnnotation,
-      fieldOnDisjointTypesMustMerge: Boolean = this.fieldOnDisjointTypesMustMerge
+      fieldsOnDisjointTypesMustMerge: Boolean = this.fieldsOnDisjointTypesMustMerge
   ) = Options(
       executableFiles = executableFiles,
       schema = schema,
@@ -292,7 +297,7 @@ class Options(
       addJvmOverloads = addJvmOverloads,
       addTypename = addTypename,
       requiresOptInAnnotation = requiresOptInAnnotation,
-      fieldOnDisjointTypesMustMerge = fieldOnDisjointTypesMustMerge
+      fieldsOnDisjointTypesMustMerge = fieldsOnDisjointTypesMustMerge
   )
 
   companion object {
@@ -323,7 +328,7 @@ class Options(
     const val defaultGenerateOptionalOperationVariables = true
     const val defaultUseSchemaPackageNameForFragments = false
     const val defaultAddJvmOverloads = false
-    const val defaultFieldOnDisjointTypesMustMerge = false
+    const val defaultFieldsOnDisjointTypesMustMerge = true
   }
 }
 

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ir/IrBuilder.kt

@@ -74,7 +74,7 @@ internal class IrBuilder(
     private val codegenModels: String,
     private val generateOptionalOperationVariables: Boolean,
     private val generateDataBuilders: Boolean,
-    private val fieldOnDisjointTypesMustMerge: Boolean,
+    private val fieldsOnDisjointTypesMustMerge: Boolean,
 ) : FieldMerger {
   private val usedTypes = mutableListOf<String>()
 
@@ -455,7 +455,7 @@ internal class IrBuilder(
   private fun GQLFragmentDefinition.toIr(): IrFragmentDefinition {
     val typeDefinition = schema.typeDefinition(typeCondition.name)
 
-    val inferredVariables = inferVariables(schema, allFragmentDefinitions, fieldOnDisjointTypesMustMerge)
+    val inferredVariables = inferVariables(schema, allFragmentDefinitions, fieldsOnDisjointTypesMustMerge)
 
     val interfaceModelGroup = builder.buildFragmentInterface(
         fragmentName = name

apollo-compiler/src/test/kotlin/com/apollographql/apollo3/compiler/ValidationTest.kt

@@ -5,72 +5,54 @@ import com.apollographql.apollo3.ast.parseAsGQLDocument
 import com.apollographql.apollo3.ast.validateAsExecutable
 import com.apollographql.apollo3.ast.validateAsSchemaAndAddApolloDefinition
 import com.apollographql.apollo3.compiler.TestUtils.checkExpected
-import com.apollographql.apollo3.compiler.TestUtils.findSchema
 import com.apollographql.apollo3.compiler.TestUtils.testParametersForGraphQLFilesIn
 import okio.buffer
 import okio.source
 import org.junit.Test
-import org.junit.experimental.runners.Enclosed
 import org.junit.runner.RunWith
 import org.junit.runners.Parameterized
 import java.io.File
-import kotlin.test.assertTrue
 
 @Suppress("UNUSED_PARAMETER")
-@RunWith(Enclosed::class)
-class ValidationTest {
+@RunWith(Parameterized::class)
+class ValidationTest(name: String, private val graphQLFile: File) {
+  private val separator = "\n------------\n"
 
-  @RunWith(Parameterized::class)
-  class ValidationParamTest(name: String, private val graphQLFile: File) {
-    private val separator = "\n------------\n"
-
-    private fun List<Issue>.serialize() = joinToString(separator) {
-      "${it.severity}: ${it.javaClass.simpleName} (${it.sourceLocation.line}:${it.sourceLocation.position})\n${it.message}"
-    }
+  private fun List<Issue>.serialize() = joinToString(separator) {
+    "${it.severity}: ${it.javaClass.simpleName} (${it.sourceLocation.line}:${it.sourceLocation.position})\n${it.message}"
+  }
 
-    @Test
-    fun testValidation() = checkExpected(graphQLFile) { schema ->
-      // Don't use absolute path for filePath because it depends on the machine where the test is run
-      val parseResult = graphQLFile.source().buffer().parseAsGQLDocument(filePath = graphQLFile.name)
+  @Test
+  fun testValidation() = checkExpected(graphQLFile) { schema ->
+    // Don't use absolute path for filePath because it depends on the machine where the test is run
+    val parseResult = graphQLFile.source().buffer().parseAsGQLDocument(filePath = graphQLFile.name)
 
-      val issues = if (graphQLFile.parentFile.name == "operation" || graphQLFile.parentFile.parentFile.name == "operation") {
-        if (parseResult.issues.isNotEmpty()) {
-          parseResult.issues
-        } else {
-          parseResult.valueAssertNoErrors().validateAsExecutable(schema = schema!!, fieldOnDisjointTypesMustMerge = false).issues + if (graphQLFile.name == "capitalized_fields_disallowed.graphql") {
-            checkCapitalizedFields(parseResult.value!!.definitions)
-          } else {
-            emptyList()
-          }
-        }
+    val issues = if (graphQLFile.parentFile.name == "operation" || graphQLFile.parentFile.parentFile.name == "operation") {
+      if (parseResult.issues.isNotEmpty()) {
+        parseResult.issues
       } else {
-        if (parseResult.issues.isNotEmpty()) {
-          parseResult.issues
+        parseResult.valueAssertNoErrors().validateAsExecutable(schema = schema!!, fieldsOnDisjointTypesMustMerge = true).issues + if (graphQLFile.name == "capitalized_fields_disallowed.graphql") {
+          checkCapitalizedFields(parseResult.value!!.definitions)
         } else {
-          val schemaResult = parseResult.valueAssertNoErrors().validateAsSchemaAndAddApolloDefinition()
-          schemaResult.issues +
-              (schemaResult.value?.let { checkApolloReservedEnumValueNames(it) } ?: emptyList()) +
-              (schemaResult.value?.let { checkApolloTargetNameClashes(it) } ?: emptyList())
+          emptyList()
         }
       }
-      issues.serialize()
-    }
-
-    companion object {
-      @JvmStatic
-      @Parameterized.Parameters(name = "{0}")
-      fun data() = testParametersForGraphQLFilesIn("src/test/validation/")
+    } else {
+      if (parseResult.issues.isNotEmpty()) {
+        parseResult.issues
+      } else {
+        val schemaResult = parseResult.valueAssertNoErrors().validateAsSchemaAndAddApolloDefinition()
+        schemaResult.issues +
+            (schemaResult.value?.let { checkApolloReservedEnumValueNames(it) } ?: emptyList()) +
+            (schemaResult.value?.let { checkApolloTargetNameClashes(it) } ?: emptyList())
+      }
     }
+    issues.serialize()
   }
 
-  @Test
-  fun testFieldOnDisjointTypesMustMerge() {
-    val file = File("src/test/validation/operation/fields_in_set_can_merge/different_shapes.graphql")
-    val schema = findSchema(file.parentFile)!!
-    val parseResult = file.source().buffer().parseAsGQLDocument(filePath = file.name)
-
-    val issues = parseResult.valueAssertNoErrors().validateAsExecutable(schema = schema, fieldOnDisjointTypesMustMerge = true).issues
-
-    assertTrue(issues.isEmpty())
+  companion object {
+    @JvmStatic
+    @Parameterized.Parameters(name = "{0}")
+    fun data() = testParametersForGraphQLFilesIn("src/test/validation/")
   }
 }

apollo-gradle-plugin-external/api/apollo-gradle-plugin-external.api

@@ -69,6 +69,7 @@ public abstract interface class com/apollographql/apollo3/gradle/api/Service {
 	public abstract fun getDebugDir ()Lorg/gradle/api/file/DirectoryProperty;
 	public abstract fun getExcludes ()Lorg/gradle/api/provider/ListProperty;
 	public abstract fun getFailOnWarnings ()Lorg/gradle/api/provider/Property;
+	public abstract fun getFieldsOnDisjointTypesMustMerge ()Lorg/gradle/api/provider/Property;
 	public abstract fun getFlattenModels ()Lorg/gradle/api/provider/Property;
 	public abstract fun getGenerateApolloMetadata ()Lorg/gradle/api/provider/Property;
 	public abstract fun getGenerateAsInternal ()Lorg/gradle/api/provider/Property;

apollo-gradle-plugin-external/src/main/kotlin/com/apollographql/apollo3/gradle/api/Service.kt

@@ -548,7 +548,7 @@ interface Service {
    *
    * Default value: false
    */
-  val fieldOnDisjointTypesMustMerge: Property<Boolean>
+  val fieldsOnDisjointTypesMustMerge: Property<Boolean>
 
   /**
    * A shorthand method that configures defaults that match Apollo Android 2.x codegen

apollo-gradle-plugin-external/src/main/kotlin/com/apollographql/apollo3/gradle/internal/ApolloGenerateSourcesTask.kt

@@ -16,7 +16,7 @@ import com.apollographql.apollo3.compiler.Options.Companion.defaultAlwaysGenerat
 import com.apollographql.apollo3.compiler.Options.Companion.defaultCodegenModels
 import com.apollographql.apollo3.compiler.Options.Companion.defaultRequiresOptInAnnotation
 import com.apollographql.apollo3.compiler.Options.Companion.defaultFailOnWarnings
-import com.apollographql.apollo3.compiler.Options.Companion.defaultFieldOnDisjointTypesMustMerge
+import com.apollographql.apollo3.compiler.Options.Companion.defaultFieldsOnDisjointTypesMustMerge
 import com.apollographql.apollo3.compiler.Options.Companion.defaultGenerateAsInternal
 import com.apollographql.apollo3.compiler.Options.Companion.defaultGenerateDataBuilders
 import com.apollographql.apollo3.compiler.Options.Companion.defaultGenerateFilterNotNull
@@ -212,7 +212,7 @@ abstract class ApolloGenerateSourcesTask : DefaultTask() {
 
   @get:Input
   @get:Optional
-  abstract val fieldOnDisjointTypesMustMerge: Property<Boolean>
+  abstract val fieldsOnDisjointTypesMustMerge: Property<Boolean>
 
   @TaskAction
   fun taskAction() {
@@ -314,7 +314,7 @@ abstract class ApolloGenerateSourcesTask : DefaultTask() {
         generateOptionalOperationVariables = generateOptionalOperationVariables.getOrElse(defaultGenerateOptionalOperationVariables),
         addJvmOverloads = addJvmOverloads.getOrElse(defaultAddJvmOverloads),
         requiresOptInAnnotation = requiresOptInAnnotation.getOrElse(defaultRequiresOptInAnnotation),
-        fieldOnDisjointTypesMustMerge = fieldOnDisjointTypesMustMerge.getOrElse(defaultFieldOnDisjointTypesMustMerge)
+        fieldsOnDisjointTypesMustMerge = fieldsOnDisjointTypesMustMerge.getOrElse(defaultFieldsOnDisjointTypesMustMerge)
     )
 
     val outputCompilerMetadata = ApolloCompiler.write(options)

apollo-gradle-plugin-external/src/main/kotlin/com/apollographql/apollo3/gradle/internal/DefaultApolloExtension.kt

@@ -572,7 +572,7 @@ abstract class DefaultApolloExtension(
       task.generateOptionalOperationVariables.set(service.generateOptionalOperationVariables)
       task.languageVersion.set(service.languageVersion)
       task.requiresOptInAnnotation.set(service.requiresOptInAnnotation)
-      task.fieldOnDisjointTypesMustMerge.set(service.fieldOnDisjointTypesMustMerge)
+      task.fieldsOnDisjointTypesMustMerge.set(service.fieldsOnDisjointTypesMustMerge)
     }
   }