Multi ati validation (#482)

* Add validation to ensure users cannot mix index hydration * Add code to ensure there are not multiple list source input fields * Extra test
atlassian-labs · Dec 18, 2023 · 9ba372c · 9ba372c
1 parent efb2799
commit 9ba372c
Show file tree

Hide file tree

Showing 6 changed files with 563 additions and 2 deletions.
diff --git a/lib/build.gradle.kts b/lib/build.gradle.kts
@@ -31,6 +31,12 @@ dependencies {
     testImplementation("com.fasterxml.jackson.core:jackson-databind:2.15.3")
     testImplementation("org.openjdk.jmh:jmh-core:1.37")
     testImplementation("org.openjdk.jmh:jmh-generator-annprocess:1.37")
+
+    testImplementation(kotlin("test"))
+
+    testImplementation("org.junit.jupiter:junit-jupiter:5.10.1")
+    testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.10.1")
+
     testImplementation("io.kotest:kotest-runner-junit5:5.8.0")
     testImplementation("io.kotest:kotest-framework-datatest:5.8.0")
     testImplementation("io.mockk:mockk:1.13.8")

diff --git a/lib/src/main/java/graphql/nadel/engine/util/CollectionUtil.kt b/lib/src/main/java/graphql/nadel/engine/util/CollectionUtil.kt
@@ -277,3 +277,21 @@ fun <A, B : Any> Sequence<Pair<A, B?>>.filterPairSecondNotNull(): Sequence<Pair<
         }
     }
 }
+
+/**
+ * Like [List.partition] but only returns the count of each partition.
+ */
+internal fun <E> List<E>.partitionCount(predicate: (E) -> Boolean): Pair<Int, Int> {
+    var first = 0
+    var second = 0
+
+    for (element in this) {
+        if (predicate(element)) {
+            first++
+        } else {
+            second++
+        }
+    }
+
+    return first to second
+}
diff --git a/lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt b/lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt
@@ -10,6 +10,7 @@ import graphql.nadel.dsl.UnderlyingServiceHydration
 import graphql.nadel.engine.util.getFieldAt
 import graphql.nadel.engine.util.isList
 import graphql.nadel.engine.util.isNonNull
+import graphql.nadel.engine.util.partitionCount
 import graphql.nadel.engine.util.unwrapAll
 import graphql.nadel.engine.util.unwrapNonNull
 import graphql.nadel.validation.NadelSchemaValidationError.CannotRenameHydratedField
@@ -85,14 +86,73 @@ internal class NadelHydrationValidation(
             errors.addAll(outputTypeIssues)
         }
 
+        val indexHydrationErrors = limitUseOfIndexHydration(parent, overallField, hydrations)
+        val sourceFieldErrors = limitSourceField(parent, overallField, hydrations)
+
         if (hasMoreThanOneHydration) {
             val (batched, notBatched) = hydrations.partition(::isBatched)
             if (batched.isNotEmpty() && notBatched.isNotEmpty()) {
                 errors.add(NadelSchemaValidationError.HydrationsMismatch(parent, overallField))
             }
         }
 
-        return errors
+        return indexHydrationErrors + sourceFieldErrors + errors
+    }
+
+    private fun limitSourceField(
+        parent: NadelServiceSchemaElement,
+        overallField: GraphQLFieldDefinition,
+        hydrations: List<UnderlyingServiceHydration>,
+    ): List<NadelSchemaValidationError> {
+        if (hydrations.size > 1) {
+            val anyListSourceInputField = hydrations
+                .any { hydration ->
+                    val parentType = parent.underlying as GraphQLFieldsContainer
+                    hydration
+                        .arguments
+                        .asSequence()
+                        .mapNotNull { argument ->
+                            argument.remoteArgumentSource.pathToField
+                        }
+                        .any { path ->
+                            parentType.getFieldAt(path)?.type?.unwrapNonNull()?.isList == true
+                        }
+                }
+
+            if (anyListSourceInputField) {
+                val sourceFields = hydrations
+                    .flatMapTo(LinkedHashSet()) { hydration ->
+                        hydration.arguments
+                            .mapNotNull { argument ->
+                                argument.remoteArgumentSource.pathToField
+                            }
+                    }
+
+                if (sourceFields.size > 1) {
+                    return listOf(
+                        NadelSchemaValidationError.MultipleHydrationSourceInputFields(parent, overallField),
+                    )
+                }
+            }
+        }
+
+        return emptyList()
+    }
+
+    private fun limitUseOfIndexHydration(
+        parent: NadelServiceSchemaElement,
+        overallField: GraphQLFieldDefinition,
+        hydrations: List<UnderlyingServiceHydration>,
+    ): List<NadelSchemaValidationError> {
+        // todo: or maybe just don't allow polymorphic index hydration
+        val (indexCount, nonIndexCount) = hydrations.partitionCount { it.isObjectMatchByIndex }
+        if (indexCount > 0 && nonIndexCount > 0) {
+            return listOf(
+                NadelSchemaValidationError.MixedIndexHydration(parent, overallField),
+            )
+        }
+
+        return emptyList()
     }
 
     private fun isBatched(hydration: UnderlyingServiceHydration): Boolean {
@@ -283,7 +343,8 @@ internal class NadelHydrationValidation(
 
             StaticArgument -> {
                 val staticArg = remoteArgSource.staticValue
-                if (!validationUtil.isValidLiteralValue(
+                if (
+                    !validationUtil.isValidLiteralValue(
                         staticArg,
                         actorFieldArg.type,
                         overallSchema,

diff --git a/lib/src/main/java/graphql/nadel/validation/NadelSchemaValidationError.kt b/lib/src/main/java/graphql/nadel/validation/NadelSchemaValidationError.kt
@@ -386,6 +386,34 @@ sealed interface NadelSchemaValidationError {
         override val subject = overallField
     }
 
+    data class MixedIndexHydration(
+        val parentType: NadelServiceSchemaElement,
+        val overallField: GraphQLFieldDefinition,
+    ) : NadelSchemaValidationError {
+        val service: Service get() = parentType.service
+
+        override val message = run {
+            val coords = makeFieldCoordinates(parentType.overall.name, overallField.name)
+            "Field $coords uses both indexed hydration and non-indexed hydration"
+        }
+
+        override val subject = overallField
+    }
+
+    data class MultipleHydrationSourceInputFields(
+        val parentType: NadelServiceSchemaElement,
+        val overallField: GraphQLFieldDefinition,
+    ) : NadelSchemaValidationError {
+        val service: Service get() = parentType.service
+
+        override val message = run {
+            val coords = makeFieldCoordinates(parentType.overall.name, overallField.name)
+            "Field $coords uses multiple \$source fields"
+        }
+
+        override val subject = overallField
+    }
+
     data class StaticArgIsNotAssignable(
         val parentType: NadelServiceSchemaElement,
         val overallField: GraphQLFieldDefinition,

diff --git a/lib/src/test/kotlin/graphql/nadel/engine/util/CollectionUtilTest.kt b/lib/src/test/kotlin/graphql/nadel/engine/util/CollectionUtilTest.kt
@@ -0,0 +1,16 @@
+package graphql.nadel.engine.util
+
+import org.junit.jupiter.api.Test
+import kotlin.test.assertTrue
+
+class CollectionUtilTest {
+    @Test
+    fun `partitionCount counts number of values in each partition`() {
+        val partition = listOf(1, 4, 6, 8, 10)
+
+        val (first, second) = partition.partitionCount { it % 2 == 0 }
+
+        assertTrue(first == 4)
+        assertTrue(second == 1)
+    }
+}