Detect and throw exception for dataloaders with same name (#1652)

* Detect and throw exception for dataloaders with same name * Removed distinctBy for dataloaders, that should not be necessary. There was an issue in a test that actually produced two beans of the same dataloader --------- Co-authored-by: pbakker <[email protected]>
Netflix · Oct 5, 2023 · d42476e · d42476e
1 parent ed5ef68
commit d42476e
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 14 deletions.
diff --git a/.../src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt b/.../src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt
@@ -932,13 +932,6 @@ class MicrometerServletSmokeTest {
             return CustomDataFetchingExceptionHandler()
         }
 
-        @Bean
-        open fun reverserDataLoader(
-            @Qualifier("dataLoaderTaskExecutor") executor: Executor
-        ): ExampleImplementation.ReverseStringDataLoader {
-            return ExampleImplementation.ReverseStringDataLoader(executor)
-        }
-
         @Bean
         open fun dataLoaderTaskExecutor(): Executor {
             val executor = ThreadPoolTaskExecutor()

diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsDataLoaderProvider.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsDataLoaderProvider.kt
@@ -16,14 +16,10 @@
 
 package com.netflix.graphql.dgs.internal
 
-import com.netflix.graphql.dgs.DataLoaderInstrumentationExtensionProvider
-import com.netflix.graphql.dgs.DgsComponent
-import com.netflix.graphql.dgs.DgsDataLoader
-import com.netflix.graphql.dgs.DgsDataLoaderOptionsProvider
-import com.netflix.graphql.dgs.DgsDataLoaderRegistryConsumer
-import com.netflix.graphql.dgs.DgsDispatchPredicate
+import com.netflix.graphql.dgs.*
 import com.netflix.graphql.dgs.exceptions.DgsUnnamedDataLoaderOnFieldException
 import com.netflix.graphql.dgs.exceptions.InvalidDataLoaderTypeException
+import com.netflix.graphql.dgs.exceptions.MultipleDataLoadersDefinedException
 import com.netflix.graphql.dgs.exceptions.UnsupportedSecuredDataLoaderException
 import com.netflix.graphql.dgs.internal.utils.DataLoaderNameUtil
 import jakarta.annotation.PostConstruct
@@ -59,6 +55,8 @@ class DgsDataLoaderProvider(
     private val mappedBatchLoaders = mutableListOf<LoaderHolder<MappedBatchLoader<*, *>>>()
     private val mappedBatchLoadersWithContext = mutableListOf<LoaderHolder<MappedBatchLoaderWithContext<*, *>>>()
 
+    private val loaderMap = mutableMapOf<String, String>()
+
     fun buildRegistry(): DataLoaderRegistry {
         return buildRegistryWithContextSupplier { null }
     }
@@ -87,7 +85,8 @@ class DgsDataLoaderProvider(
     }
 
     private fun addDataLoaderFields() {
-        applicationContext.getBeansWithAnnotation(DgsComponent::class.java).values.forEach { dgsComponent ->
+        val dataLoaders = applicationContext.getBeansWithAnnotation(DgsComponent::class.java)
+        dataLoaders.values.forEach { dgsComponent ->
             val javaClass = AopUtils.getTargetClass(dgsComponent)
 
             javaClass.declaredFields.asSequence().filter { it.isAnnotationPresent(DgsDataLoader::class.java) }
@@ -230,6 +229,11 @@ class DgsDataLoaderProvider(
             is MappedBatchLoaderWithContext<*, *> -> createDataLoader(holder.theLoader, holder.annotation, holder.name, contextSupplier, registry, extensionProviders)
             else -> throw IllegalArgumentException("Data loader ${holder.name} has unknown type")
         }
+        // detect and throw an exception if multiple data loaders use the same name
+        if (registry.keys.contains(holder.name)) {
+            throw MultipleDataLoadersDefinedException(holder.theLoader.javaClass)
+        }
+
         if (holder.dispatchPredicate == null) {
             registry.register(holder.name, loader)
         } else {

diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt
@@ -18,6 +18,7 @@ package com.netflix.graphql.dgs
 
 import com.netflix.graphql.dgs.exceptions.DgsUnnamedDataLoaderOnFieldException
 import com.netflix.graphql.dgs.exceptions.InvalidDataLoaderTypeException
+import com.netflix.graphql.dgs.exceptions.MultipleDataLoadersDefinedException
 import com.netflix.graphql.dgs.internal.DgsDataLoaderProvider
 import graphql.schema.DataFetchingEnvironmentImpl
 import org.assertj.core.api.Assertions.assertThat
@@ -65,6 +66,16 @@ class DgsDataLoaderProviderTest {
         }
     }
 
+    @Test
+    fun detectDuplicateDataLoaders() {
+        applicationContextRunner.withBean(ExampleBatchLoader::class.java).withBean(ExampleDuplicateBatchLoader::class.java).run { context ->
+            val provider = context.getBean(DgsDataLoaderProvider::class.java)
+            assertThrows<MultipleDataLoadersDefinedException> {
+                provider.buildRegistry()
+            }
+        }
+    }
+
     @Test
     fun dataLoaderInvalidType() {
         @DgsDataLoader

diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ExampleDuplicateBatchLoader.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ExampleDuplicateBatchLoader.kt
@@ -0,0 +1,28 @@
+/*
+ * Copyright 2021 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.graphql.dgs
+
+import org.dataloader.BatchLoader
+import java.util.concurrent.CompletableFuture
+import java.util.concurrent.CompletionStage
+
+@DgsDataLoader(name = "exampleLoader")
+class ExampleDuplicateBatchLoader : BatchLoader<String, String> {
+    override fun load(keys: MutableList<String>?): CompletionStage<MutableList<String>> {
+        return CompletableFuture.supplyAsync { mutableListOf("a", "b", "c") }
+    }
+}