From 11ddf1c2223594eb85a2e2328c68e5644577cb06 Mon Sep 17 00:00:00 2001 From: Jordie Date: Fri, 4 Mar 2022 00:19:32 +0100 Subject: [PATCH 1/2] Automatic DgsDataLoader naming --- .../netflix/graphql/dgs/DgsDataLoader.java | 6 +- .../graphql/dgs/DgsDataFetchingEnvironment.kt | 19 ++++-- .../dgs/internal/DgsDataLoaderProvider.kt | 62 +++++++++---------- .../dgs/internal/utils/DataLoaderNameUtil.kt | 33 ++++++++++ ...xampleBatchLoaderWithoutNameFromField.java | 43 +++++++++++++ .../graphql/dgs/DgsDataLoaderProviderTest.kt | 60 +++++++++++++++++- .../dgs/ExampleBatchLoaderWithoutName.kt | 28 +++++++++ 7 files changed, 211 insertions(+), 40 deletions(-) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt create mode 100644 graphql-dgs/src/test/java/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutNameFromField.java create mode 100644 graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutName.kt diff --git a/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java b/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java index 166c95369..ff10003b6 100644 --- a/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java +++ b/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,7 +32,9 @@ @Retention(RetentionPolicy.RUNTIME) @Component public @interface DgsDataLoader { - String name(); + String GENERATE_DATA_LOADER_NAME = "NETFLIX_DGS_GENERATE_DATALOADER_NAME"; + + String name() default GENERATE_DATA_LOADER_NAME; boolean caching() default true; diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt index 7a05b46e4..85f500070 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ package com.netflix.graphql.dgs import com.netflix.graphql.dgs.context.DgsContext import com.netflix.graphql.dgs.exceptions.MultipleDataLoadersDefinedException import com.netflix.graphql.dgs.exceptions.NoDataLoaderFoundException +import com.netflix.graphql.dgs.internal.utils.DataLoaderNameUtil import graphql.GraphQLContext import graphql.cachecontrol.CacheControl import graphql.execution.ExecutionId @@ -29,7 +30,12 @@ import graphql.language.Document import graphql.language.Field import graphql.language.FragmentDefinition import graphql.language.OperationDefinition -import graphql.schema.* +import graphql.schema.DataFetchingEnvironment +import graphql.schema.DataFetchingFieldSelectionSet +import graphql.schema.GraphQLFieldDefinition +import graphql.schema.GraphQLOutputType +import graphql.schema.GraphQLSchema +import graphql.schema.GraphQLType import org.dataloader.DataLoader import org.dataloader.DataLoaderRegistry import java.util.* @@ -48,13 +54,14 @@ class DgsDataFetchingEnvironment(private val dfe: DataFetchingEnvironment) : Dat fun getDataLoader(loaderClass: Class<*>): DataLoader { val annotation = loaderClass.getAnnotation(DgsDataLoader::class.java) return if (annotation != null) { - dfe.getDataLoader(annotation.name) + dfe.getDataLoader(DataLoaderNameUtil.getDataLoaderName(loaderClass, annotation)) } else { val loaders = loaderClass.fields.filter { it.isAnnotationPresent(DgsDataLoader::class.java) } if (loaders.size > 1) throw MultipleDataLoadersDefinedException(loaderClass) - val loaderName = loaders - .firstOrNull()?.getAnnotation(DgsDataLoader::class.java)?.name - ?: throw NoDataLoaderFoundException(loaderClass) + val loaderField: java.lang.reflect.Field = loaders + .firstOrNull() ?: throw NoDataLoaderFoundException(loaderClass) + val theAnnotation = loaderField.getAnnotation(DgsDataLoader::class.java) + val loaderName = theAnnotation.name dfe.getDataLoader(loaderName) } } 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 index 4f02968bb..04896b7b0 100644 --- 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 @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,7 +22,14 @@ import com.netflix.graphql.dgs.DgsDataLoader import com.netflix.graphql.dgs.DgsDataLoaderRegistryConsumer import com.netflix.graphql.dgs.exceptions.InvalidDataLoaderTypeException import com.netflix.graphql.dgs.exceptions.UnsupportedSecuredDataLoaderException -import org.dataloader.* +import com.netflix.graphql.dgs.internal.utils.DataLoaderNameUtil +import org.dataloader.BatchLoader +import org.dataloader.BatchLoaderWithContext +import org.dataloader.DataLoader +import org.dataloader.DataLoaderOptions +import org.dataloader.DataLoaderRegistry +import org.dataloader.MappedBatchLoader +import org.dataloader.MappedBatchLoaderWithContext import org.slf4j.Logger import org.slf4j.LoggerFactory import org.springframework.aop.support.AopUtils @@ -37,10 +44,12 @@ import javax.annotation.PostConstruct */ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) { - private val batchLoaders = mutableListOf, DgsDataLoader>>() - private val batchLoadersWithContext = mutableListOf, DgsDataLoader>>() - private val mappedBatchLoaders = mutableListOf, DgsDataLoader>>() - private val mappedBatchLoadersWithContext = mutableListOf, DgsDataLoader>>() + data class LoaderHolder(val theLoader: T, val annotation: DgsDataLoader, val name: String) + + private val batchLoaders = mutableListOf>>() + private val batchLoadersWithContext = mutableListOf>>() + private val mappedBatchLoaders = mutableListOf>>() + private val mappedBatchLoadersWithContext = mutableListOf>>() fun buildRegistry(): DataLoaderRegistry { return buildRegistryWithContextSupplier { null } @@ -50,24 +59,17 @@ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) val startTime = System.currentTimeMillis() val dataLoaderRegistry = DataLoaderRegistry() - batchLoaders.forEach { dataLoaderRegistry.register(it.second.name, createDataLoader(it.first, it.second, dataLoaderRegistry)) } + batchLoaders.forEach { + dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, dataLoaderRegistry)) + } mappedBatchLoaders.forEach { - dataLoaderRegistry.register( - it.second.name, - createDataLoader(it.first, it.second, dataLoaderRegistry) - ) + dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, dataLoaderRegistry)) } batchLoadersWithContext.forEach { - dataLoaderRegistry.register( - it.second.name, - createDataLoader(it.first, it.second, contextSupplier, dataLoaderRegistry) - ) + dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, contextSupplier, dataLoaderRegistry)) } mappedBatchLoadersWithContext.forEach { - dataLoaderRegistry.register( - it.second.name, - createDataLoader(it.first, it.second, contextSupplier, dataLoaderRegistry) - ) + dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, contextSupplier, dataLoaderRegistry)) } val endTime = System.currentTimeMillis() @@ -96,15 +98,12 @@ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) val annotation = field.getAnnotation(DgsDataLoader::class.java) ReflectionUtils.makeAccessible(field) + val name: () -> String = { DataLoaderNameUtil.getDataLoaderName(field, annotation) } when (val get = field.get(dgsComponent)) { - is BatchLoader<*, *> -> - batchLoaders.add(get to annotation) - is BatchLoaderWithContext<*, *> -> - batchLoadersWithContext.add(get to annotation) - is MappedBatchLoader<*, *> -> - mappedBatchLoaders.add(get to annotation) - is MappedBatchLoaderWithContext<*, *> -> - mappedBatchLoadersWithContext.add(get to annotation) + is BatchLoader<*, *> -> batchLoaders.add(LoaderHolder(get, annotation, name())) + is BatchLoaderWithContext<*, *> -> batchLoadersWithContext.add(LoaderHolder(get, annotation, name())) + is MappedBatchLoader<*, *> -> mappedBatchLoaders.add(LoaderHolder(get, annotation, name())) + is MappedBatchLoaderWithContext<*, *> -> mappedBatchLoadersWithContext.add(LoaderHolder(get, annotation, name())) else -> throw InvalidDataLoaderTypeException(dgsComponent::class.java) } } @@ -116,15 +115,16 @@ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) dataLoaders.values.forEach { dgsComponent -> val javaClass = AopUtils.getTargetClass(dgsComponent) val annotation = javaClass.getAnnotation(DgsDataLoader::class.java) + val name: () -> String = { DataLoaderNameUtil.getDataLoaderName(javaClass, annotation) } when (dgsComponent) { is BatchLoader<*, *> -> - batchLoaders.add(Pair(dgsComponent, annotation)) + batchLoaders.add(LoaderHolder(dgsComponent, annotation, name())) is BatchLoaderWithContext<*, *> -> - batchLoadersWithContext.add(Pair(dgsComponent, annotation)) + batchLoadersWithContext.add(LoaderHolder(dgsComponent, annotation, name())) is MappedBatchLoader<*, *> -> - mappedBatchLoaders.add(Pair(dgsComponent, annotation)) + mappedBatchLoaders.add(LoaderHolder(dgsComponent, annotation, name())) is MappedBatchLoaderWithContext<*, *> -> - mappedBatchLoadersWithContext.add(Pair(dgsComponent, annotation)) + mappedBatchLoadersWithContext.add(LoaderHolder(dgsComponent, annotation, name())) else -> throw InvalidDataLoaderTypeException(dgsComponent::class.java) } } diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt new file mode 100644 index 000000000..1b0596e77 --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt @@ -0,0 +1,33 @@ +/* + * Copyright 2022 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.internal.utils + +import com.netflix.graphql.dgs.DgsDataLoader +import java.lang.reflect.Field + +object DataLoaderNameUtil { + + fun getDataLoaderName(field: Field, annotation: DgsDataLoader): String { + return if (annotation.name == DgsDataLoader.GENERATE_DATA_LOADER_NAME) + "DGS_DATALOADER_FIELD_" + field.declaringClass.name + "#" + field.name else annotation.name + } + + fun getDataLoaderName(clazz: Class<*>, annotation: DgsDataLoader): String { + return if (annotation.name == DgsDataLoader.GENERATE_DATA_LOADER_NAME) + "DGS_DATALOADER_CLASS_" + clazz.name else annotation.name + } +} diff --git a/graphql-dgs/src/test/java/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutNameFromField.java b/graphql-dgs/src/test/java/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutNameFromField.java new file mode 100644 index 000000000..c19268eeb --- /dev/null +++ b/graphql-dgs/src/test/java/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutNameFromField.java @@ -0,0 +1,43 @@ +/* + * Copyright 2022 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.*; +import java.util.concurrent.CompletableFuture; + +@DgsComponent +public class ExampleBatchLoaderWithoutNameFromField { + @DgsDataLoader + public BatchLoader batchLoader = keys -> CompletableFuture.supplyAsync(() -> { + List values = new ArrayList<>(); + values.add("a"); + values.add("b"); + values.add("c"); + return values; + }); + + @DgsDataLoader + BatchLoader privateBatchLoader = keys -> CompletableFuture.supplyAsync(() -> { + List values = new ArrayList<>(); + values.add("a"); + values.add("b"); + values.add("c"); + return values; + }); +} 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 index 1178aa290..7a1689bc5 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,7 @@ package com.netflix.graphql.dgs import com.netflix.graphql.dgs.exceptions.InvalidDataLoaderTypeException import com.netflix.graphql.dgs.internal.DgsDataLoaderProvider +import graphql.schema.DataFetchingEnvironmentImpl import io.mockk.every import io.mockk.impl.annotations.MockK import io.mockk.junit5.MockKExtension @@ -26,6 +27,7 @@ import org.dataloader.BatchLoader import org.dataloader.DataLoaderRegistry import org.junit.jupiter.api.Assertions import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows import org.junit.jupiter.api.extension.ExtendWith @@ -128,6 +130,62 @@ class DgsDataLoaderProviderTest { assertThat(loaderKeys).isEqualTo(registry.keys.toMutableList()[0]) } + @Nested + inner class UnnamedBatchLoaderTests { + @Test + fun findDataLoadersWithoutName() { + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf(Pair("helloFetcher", ExampleBatchLoaderWithoutName())) + + val provider = DgsDataLoaderProvider(applicationContextMock) + provider.findDataLoaders() + val dataLoaderRegistry = provider.buildRegistry() + Assertions.assertEquals(1, dataLoaderRegistry.dataLoaders.size) + val dataLoader = dataLoaderRegistry.getDataLoader("DGS_DATALOADER_CLASS_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutName") + Assertions.assertNotNull(dataLoader) + } + + @Test + fun findDataLoadersWithoutNameByClass() { + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns emptyMap() + val theLoader = ExampleBatchLoaderWithoutName() + Assertions.assertEquals( + DgsDataLoader.GENERATE_DATA_LOADER_NAME, + theLoader.javaClass.getAnnotation(DgsDataLoader::class.java).name + ) + every { + applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) + } returns mapOf(Pair("helloFetcher", theLoader)) + + val provider = DgsDataLoaderProvider(applicationContextMock) + provider.findDataLoaders() + val dataLoaderRegistry = provider.buildRegistry() + Assertions.assertEquals(1, dataLoaderRegistry.dataLoaders.size) + val dataLoader = DgsDataFetchingEnvironment( + DataFetchingEnvironmentImpl.newDataFetchingEnvironment() + .dataLoaderRegistry(dataLoaderRegistry).build() + ) + .getDataLoader(ExampleBatchLoaderWithoutName::class.java) + Assertions.assertNotNull(dataLoader) + } + + @Test + fun findDataLoadersFromFieldsWithoutName() { + every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf(Pair("helloFetcher", ExampleBatchLoaderWithoutNameFromField())) + + val provider = DgsDataLoaderProvider(applicationContextMock) + provider.findDataLoaders() + val dataLoaderRegistry = provider.buildRegistry() + Assertions.assertEquals(2, dataLoaderRegistry.dataLoaders.size) + val dataLoader = dataLoaderRegistry.getDataLoader("DGS_DATALOADER_FIELD_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutNameFromField#batchLoader") + Assertions.assertNotNull(dataLoader) + + val privateDataLoader = dataLoaderRegistry.getDataLoader("DGS_DATALOADER_FIELD_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutNameFromField#privateBatchLoader") + Assertions.assertNotNull(privateDataLoader) + } + } + @DgsDataLoader(name = "withRegistry") class ExampleDataLoaderWithRegistry : BatchLoader, DgsDataLoaderRegistryConsumer { diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutName.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutName.kt new file mode 100644 index 000000000..7c2b9b5d2 --- /dev/null +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ExampleBatchLoaderWithoutName.kt @@ -0,0 +1,28 @@ +/* + * Copyright 2022 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 +class ExampleBatchLoaderWithoutName : BatchLoader { + override fun load(keys: MutableList?): CompletionStage> { + return CompletableFuture.supplyAsync { mutableListOf("a", "b", "c") } + } +} From 5787ef53cc3951b7dcd6a0669aa91b91960ec033 Mon Sep 17 00:00:00 2001 From: Jordie Date: Mon, 14 Mar 2022 14:09:03 +0100 Subject: [PATCH 2/2] Incorporate feedback on automatic data loader naming --- .../netflix/graphql/dgs/DgsDataLoader.java | 8 +++ .../DgsUnnamedDataLoaderOnFieldException.kt | 22 ++++++ .../dgs/internal/DgsDataLoaderProvider.kt | 42 +++++++----- .../dgs/internal/utils/DataLoaderNameUtil.kt | 18 ++--- .../graphql/dgs/DgsDataLoaderProviderTest.kt | 68 ++++++++++++++----- 5 files changed, 117 insertions(+), 41 deletions(-) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsUnnamedDataLoaderOnFieldException.kt diff --git a/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java b/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java index ff10003b6..3f9762c67 100644 --- a/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java +++ b/graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java @@ -16,6 +16,7 @@ package com.netflix.graphql.dgs; +import com.netflix.graphql.dgs.internal.utils.DataLoaderNameUtil; import org.springframework.stereotype.Component; import java.lang.annotation.ElementType; @@ -32,6 +33,13 @@ @Retention(RetentionPolicy.RUNTIME) @Component public @interface DgsDataLoader { + + /** + * Used internally by {@link DataLoaderNameUtil#getDataLoaderName(Class, DgsDataLoader)}. + *

+ * The value of this constant may change in future versions, + * and should therefore not be relied upon. + */ String GENERATE_DATA_LOADER_NAME = "NETFLIX_DGS_GENERATE_DATALOADER_NAME"; String name() default GENERATE_DATA_LOADER_NAME; diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsUnnamedDataLoaderOnFieldException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsUnnamedDataLoaderOnFieldException.kt new file mode 100644 index 000000000..084d754d9 --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsUnnamedDataLoaderOnFieldException.kt @@ -0,0 +1,22 @@ +/* + * 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.exceptions + +import java.lang.reflect.Field + +class DgsUnnamedDataLoaderOnFieldException(field: Field) : + RuntimeException("Field `${field.name}` in class `${field.declaringClass.name}` was annotated with @DgsDataLoader, but the data loader was not given a proper name") 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 index 04896b7b0..85e828acb 100644 --- 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 @@ -20,6 +20,7 @@ import com.netflix.graphql.dgs.DataLoaderInstrumentationExtensionProvider import com.netflix.graphql.dgs.DgsComponent import com.netflix.graphql.dgs.DgsDataLoader import com.netflix.graphql.dgs.DgsDataLoaderRegistryConsumer +import com.netflix.graphql.dgs.exceptions.DgsUnnamedDataLoaderOnFieldException import com.netflix.graphql.dgs.exceptions.InvalidDataLoaderTypeException import com.netflix.graphql.dgs.exceptions.UnsupportedSecuredDataLoaderException import com.netflix.graphql.dgs.internal.utils.DataLoaderNameUtil @@ -44,7 +45,7 @@ import javax.annotation.PostConstruct */ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) { - data class LoaderHolder(val theLoader: T, val annotation: DgsDataLoader, val name: String) + private data class LoaderHolder(val theLoader: T, val annotation: DgsDataLoader, val name: String) private val batchLoaders = mutableListOf>>() private val batchLoadersWithContext = mutableListOf>>() @@ -66,10 +67,16 @@ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, dataLoaderRegistry)) } batchLoadersWithContext.forEach { - dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, contextSupplier, dataLoaderRegistry)) + dataLoaderRegistry.register( + it.name, + createDataLoader(it.theLoader, it.annotation, contextSupplier, dataLoaderRegistry) + ) } mappedBatchLoadersWithContext.forEach { - dataLoaderRegistry.register(it.name, createDataLoader(it.theLoader, it.annotation, contextSupplier, dataLoaderRegistry)) + dataLoaderRegistry.register( + it.name, + createDataLoader(it.theLoader, it.annotation, contextSupplier, dataLoaderRegistry) + ) } val endTime = System.currentTimeMillis() @@ -98,12 +105,16 @@ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) val annotation = field.getAnnotation(DgsDataLoader::class.java) ReflectionUtils.makeAccessible(field) - val name: () -> String = { DataLoaderNameUtil.getDataLoaderName(field, annotation) } + if (annotation.name == DgsDataLoader.GENERATE_DATA_LOADER_NAME) { + throw DgsUnnamedDataLoaderOnFieldException(field) + } + + fun createHolder(t: T): LoaderHolder = LoaderHolder(t, annotation, annotation.name) when (val get = field.get(dgsComponent)) { - is BatchLoader<*, *> -> batchLoaders.add(LoaderHolder(get, annotation, name())) - is BatchLoaderWithContext<*, *> -> batchLoadersWithContext.add(LoaderHolder(get, annotation, name())) - is MappedBatchLoader<*, *> -> mappedBatchLoaders.add(LoaderHolder(get, annotation, name())) - is MappedBatchLoaderWithContext<*, *> -> mappedBatchLoadersWithContext.add(LoaderHolder(get, annotation, name())) + is BatchLoader<*, *> -> batchLoaders.add(createHolder(get)) + is BatchLoaderWithContext<*, *> -> batchLoadersWithContext.add(createHolder(get)) + is MappedBatchLoader<*, *> -> mappedBatchLoaders.add(createHolder(get)) + is MappedBatchLoaderWithContext<*, *> -> mappedBatchLoadersWithContext.add(createHolder(get)) else -> throw InvalidDataLoaderTypeException(dgsComponent::class.java) } } @@ -115,16 +126,13 @@ class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) dataLoaders.values.forEach { dgsComponent -> val javaClass = AopUtils.getTargetClass(dgsComponent) val annotation = javaClass.getAnnotation(DgsDataLoader::class.java) - val name: () -> String = { DataLoaderNameUtil.getDataLoaderName(javaClass, annotation) } + fun createHolder(t: T): LoaderHolder = + LoaderHolder(t, annotation, DataLoaderNameUtil.getDataLoaderName(javaClass, annotation)) when (dgsComponent) { - is BatchLoader<*, *> -> - batchLoaders.add(LoaderHolder(dgsComponent, annotation, name())) - is BatchLoaderWithContext<*, *> -> - batchLoadersWithContext.add(LoaderHolder(dgsComponent, annotation, name())) - is MappedBatchLoader<*, *> -> - mappedBatchLoaders.add(LoaderHolder(dgsComponent, annotation, name())) - is MappedBatchLoaderWithContext<*, *> -> - mappedBatchLoadersWithContext.add(LoaderHolder(dgsComponent, annotation, name())) + is BatchLoader<*, *> -> batchLoaders.add(createHolder(dgsComponent)) + is BatchLoaderWithContext<*, *> -> batchLoadersWithContext.add(createHolder(dgsComponent)) + is MappedBatchLoader<*, *> -> mappedBatchLoaders.add(createHolder(dgsComponent)) + is MappedBatchLoaderWithContext<*, *> -> mappedBatchLoadersWithContext.add(createHolder(dgsComponent)) else -> throw InvalidDataLoaderTypeException(dgsComponent::class.java) } } diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt index 1b0596e77..4fc6a494b 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt @@ -17,17 +17,19 @@ package com.netflix.graphql.dgs.internal.utils import com.netflix.graphql.dgs.DgsDataLoader -import java.lang.reflect.Field +import com.netflix.graphql.dgs.Internal +@Internal object DataLoaderNameUtil { - fun getDataLoaderName(field: Field, annotation: DgsDataLoader): String { - return if (annotation.name == DgsDataLoader.GENERATE_DATA_LOADER_NAME) - "DGS_DATALOADER_FIELD_" + field.declaringClass.name + "#" + field.name else annotation.name - } - + /** + * When the [annotation]'s [DgsDataLoader.name] is equal to [DgsDataLoader.GENERATE_DATA_LOADER_NAME], + * the [clazz]'s [Class.getSimpleName] will be used. + * In all other cases the [DgsDataLoader.name] method will be called on [annotation]. + * + * This method does not verify that [annotation] belongs to [clazz] for performance reasons. + */ fun getDataLoaderName(clazz: Class<*>, annotation: DgsDataLoader): String { - return if (annotation.name == DgsDataLoader.GENERATE_DATA_LOADER_NAME) - "DGS_DATALOADER_CLASS_" + clazz.name else annotation.name + return if (annotation.name == DgsDataLoader.GENERATE_DATA_LOADER_NAME) clazz.simpleName else annotation.name } } 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 index 7a1689bc5..4341c2fe1 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt @@ -16,6 +16,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.internal.DgsDataLoaderProvider import graphql.schema.DataFetchingEnvironmentImpl @@ -23,6 +24,7 @@ import io.mockk.every import io.mockk.impl.annotations.MockK import io.mockk.junit5.MockKExtension import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.assertThatThrownBy import org.dataloader.BatchLoader import org.dataloader.DataLoaderRegistry import org.junit.jupiter.api.Assertions @@ -51,7 +53,12 @@ class DgsDataLoaderProviderTest { @Test fun findDataLoaders() { every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns emptyMap() - every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf(Pair("helloFetcher", ExampleBatchLoader())) + every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf( + Pair( + "helloFetcher", + ExampleBatchLoader() + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) provider.findDataLoaders() @@ -63,7 +70,12 @@ class DgsDataLoaderProviderTest { @Test fun dataLoaderInvalidType() { - every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf(Pair("helloFetcher", object {})) + every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf( + Pair( + "helloFetcher", + object {} + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) assertThrows { provider.findDataLoaders() } } @@ -71,7 +83,12 @@ class DgsDataLoaderProviderTest { @Test fun findDataLoadersFromFields() { every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns emptyMap() - every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf(Pair("helloFetcher", ExampleBatchLoaderFromField())) + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf( + Pair( + "helloFetcher", + ExampleBatchLoaderFromField() + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) provider.findDataLoaders() @@ -87,7 +104,12 @@ class DgsDataLoaderProviderTest { @Test fun findMappedDataLoaders() { every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns emptyMap() - every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf(Pair("helloFetcher", ExampleMappedBatchLoader())) + every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf( + Pair( + "helloFetcher", + ExampleMappedBatchLoader() + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) provider.findDataLoaders() @@ -100,7 +122,12 @@ class DgsDataLoaderProviderTest { @Test fun findMappedDataLoadersFromFields() { every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns emptyMap() - every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf(Pair("helloFetcher", ExampleMappedBatchLoaderFromField())) + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf( + Pair( + "helloFetcher", + ExampleMappedBatchLoaderFromField() + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) provider.findDataLoaders() @@ -135,13 +162,19 @@ class DgsDataLoaderProviderTest { @Test fun findDataLoadersWithoutName() { every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns emptyMap() - every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf(Pair("helloFetcher", ExampleBatchLoaderWithoutName())) + every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns mapOf( + Pair( + "helloFetcher", + ExampleBatchLoaderWithoutName() + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) provider.findDataLoaders() val dataLoaderRegistry = provider.buildRegistry() Assertions.assertEquals(1, dataLoaderRegistry.dataLoaders.size) - val dataLoader = dataLoaderRegistry.getDataLoader("DGS_DATALOADER_CLASS_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutName") + val dataLoader = + dataLoaderRegistry.getDataLoader("ExampleBatchLoaderWithoutName") Assertions.assertNotNull(dataLoader) } @@ -172,17 +205,20 @@ class DgsDataLoaderProviderTest { @Test fun findDataLoadersFromFieldsWithoutName() { every { applicationContextMock.getBeansWithAnnotation(DgsDataLoader::class.java) } returns emptyMap() - every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf(Pair("helloFetcher", ExampleBatchLoaderWithoutNameFromField())) + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf( + Pair( + "helloFetcher", + ExampleBatchLoaderWithoutNameFromField() + ) + ) val provider = DgsDataLoaderProvider(applicationContextMock) - provider.findDataLoaders() - val dataLoaderRegistry = provider.buildRegistry() - Assertions.assertEquals(2, dataLoaderRegistry.dataLoaders.size) - val dataLoader = dataLoaderRegistry.getDataLoader("DGS_DATALOADER_FIELD_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutNameFromField#batchLoader") - Assertions.assertNotNull(dataLoader) - - val privateDataLoader = dataLoaderRegistry.getDataLoader("DGS_DATALOADER_FIELD_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutNameFromField#privateBatchLoader") - Assertions.assertNotNull(privateDataLoader) + assertThatThrownBy { provider.findDataLoaders() } + .hasNoCause() + .isExactlyInstanceOf(DgsUnnamedDataLoaderOnFieldException::class.java) + .hasMessage( + "Field `batchLoader` in class `com.netflix.graphql.dgs.ExampleBatchLoaderWithoutNameFromField` was annotated with @DgsDataLoader, but the data loader was not given a proper name" + ) } }