Forbid virtual type duplication (#648)

atlassian-labs · Dec 9, 2024 · ef7b74a · ef7b74a
1 parent 0d71072
commit ef7b74a
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 7 deletions.
diff --git a/lib/src/main/java/graphql/nadel/validation/NadelVirtualTypeValidation.kt b/lib/src/main/java/graphql/nadel/validation/NadelVirtualTypeValidation.kt
@@ -5,19 +5,39 @@ import graphql.nadel.definition.virtualType.hasVirtualTypeDefinition
 import graphql.nadel.engine.util.unwrapAll
 import graphql.nadel.validation.NadelTypeWrappingValidation.Rule.LHS_MUST_BE_LOOSER_OR_SAME
 import graphql.nadel.validation.NadelTypeWrappingValidation.Rule.LHS_MUST_BE_STRICTER_OR_SAME
+import graphql.nadel.validation.NadelVirtualTypeValidationContext.TraversalOutcome.DUPLICATED_BACKING_TYPE
+import graphql.nadel.validation.NadelVirtualTypeValidationContext.TraversalOutcome.SKIP
+import graphql.nadel.validation.NadelVirtualTypeValidationContext.TraversalOutcome.VISIT
 import graphql.nadel.validation.hydration.NadelHydrationValidation
 import graphql.schema.GraphQLArgument
 import graphql.schema.GraphQLFieldDefinition
 import graphql.schema.GraphQLObjectType
 
 private class NadelVirtualTypeValidationContext {
-    private val visited: MutableSet<Pair<String, String>> = mutableSetOf()
-
     /**
-     * @return true to visit the element, false to abort
+     * The previously visited elements by backing type name.
      */
-    fun visit(element: NadelServiceSchemaElement.VirtualType): Boolean {
-        return visited.add(element.overall.name to element.underlying.name)
+    private val visitedByBacking: MutableMap<String, NadelServiceSchemaElement.VirtualType> = mutableMapOf()
+
+    fun visit(element: NadelServiceSchemaElement.VirtualType): TraversalOutcome {
+        val existingTraversal = visitedByBacking[element.underlying.name]
+
+        @Suppress("CascadeIf") // Not as easy to understand
+        return if (existingTraversal == null) {
+            visitedByBacking[element.underlying.name] = element
+            VISIT
+        } else if (existingTraversal == element) {
+            SKIP
+        } else {
+            DUPLICATED_BACKING_TYPE
+        }
+    }
+
+    enum class TraversalOutcome {
+        VISIT,
+        DUPLICATED_BACKING_TYPE,
+        SKIP,
+        ;
     }
 }
 
@@ -39,8 +59,10 @@ class NadelVirtualTypeValidation internal constructor(
     private fun validate(
         schemaElement: NadelServiceSchemaElement.VirtualType,
     ): NadelSchemaValidationResult {
-        if (!visit(schemaElement)) {
-            return ok()
+        when (visit(schemaElement)) {
+            VISIT -> {}
+            DUPLICATED_BACKING_TYPE -> return NadelVirtualTypeDuplicationError(schemaElement)
+            SKIP -> return ok()
         }
 
         if (schemaElement.overall is GraphQLObjectType && schemaElement.underlying is GraphQLObjectType) {

diff --git a/lib/src/main/java/graphql/nadel/validation/NadelVirtualTypeValidationError.kt b/lib/src/main/java/graphql/nadel/validation/NadelVirtualTypeValidationError.kt
@@ -14,6 +14,15 @@ data class NadelVirtualTypeIllegalTypeError(
         get() = type.overall
 }
 
+data class NadelVirtualTypeDuplicationError(
+    val type: NadelServiceSchemaElement.VirtualType,
+) : NadelSchemaValidationError {
+    override val message: String = "Backing types cannot map to multiple virtual types"
+
+    override val subject: GraphQLNamedSchemaElement
+        get() = type.overall
+}
+
 data class NadelVirtualTypeMissingBackingFieldError(
     val type: NadelServiceSchemaElement.VirtualType,
     val virtualField: GraphQLFieldDefinition,

diff --git a/lib/src/test/kotlin/graphql/nadel/validation/NadelVirtualTypeValidationTest.kt b/lib/src/test/kotlin/graphql/nadel/validation/NadelVirtualTypeValidationTest.kt
@@ -472,6 +472,70 @@ class NadelVirtualTypeValidationTest {
         assertTrue(errors.isEmpty())
     }
 
+    @Test
+    fun `forbids duplicating backing type`() {
+        // Given
+        val fixture = makeFixture(
+            overallSchema = mapOf(
+                "serviceA" to /*language=GraphQL*/ """
+                    type Query {
+                        echo: String
+                        virtualField: DataView
+                            @hydrated(
+                                field: "data"
+                                arguments: [{name: "id", value: "1"}]
+                            )
+                    }
+                    type DataView @virtualType {
+                        id: ID
+                        string: String
+                        int: Int
+                        other: OtherDataView
+                        else: Else
+                    }
+                    type OtherDataView @virtualType {
+                        boolean: Boolean
+                        data: DataView
+                    }
+                    type Else @virtualType {
+                        boolean: Boolean
+                        data: DataView
+                    }
+                """.trimIndent(),
+                "serviceB" to /*language=GraphQL*/ """
+                    type Query {
+                      data(id: ID!): Data
+                    }
+                    type Data {
+                      id: ID
+                      string: String
+                      bool: Boolean
+                      int: Int!
+                      other: OtherData
+                      else: OtherData
+                    }
+                    type OtherData {
+                      boolean: Boolean!
+                      data: Data
+                    }
+                """.trimIndent(),
+            ),
+        )
+
+        // When
+        val errors = validate(fixture)
+
+        // Then
+        assertTrue(errors.isNotEmpty())
+
+        val duplicationError = errors
+            .asSequence()
+            .filterIsInstance<NadelVirtualTypeDuplicationError>()
+            .single()
+
+        assertTrue(duplicationError.type.underlying.name == "OtherData")
+    }
+
     private fun makeFixture(
         overallSchema: Map<String, String>,
         underlyingSchema: Map<String, String> = emptyMap(),