Skip to content

Commit

Permalink
Incorporate feedback on automatic data loader naming
Browse files Browse the repository at this point in the history
  • Loading branch information
@jord1e
jord1e committed Mar 14, 2022
1 parent 930dccf commit 5787ef5
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 41 deletions.
8 changes: 8 additions & 0 deletions graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsDataLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,6 +33,13 @@
@Retention(RetentionPolicy.RUNTIME)
@Component
public @interface DgsDataLoader {

/**
* Used internally by {@link DataLoaderNameUtil#getDataLoaderName(Class, DgsDataLoader)}.
* <p>
* The <strong>value</strong> 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;
Expand Down
22 changes: 22 additions & 0 deletions ...rc/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsUnnamedDataLoaderOnFieldException.kt
View file
Original file line number Diff line number Diff line change
@@ -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")
42 changes: 25 additions & 17 deletions graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsDataLoaderProvider.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -44,7 +45,7 @@ import javax.annotation.PostConstruct
*/
class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) {

data class LoaderHolder<T>(val theLoader: T, val annotation: DgsDataLoader, val name: String)
private data class LoaderHolder<T>(val theLoader: T, val annotation: DgsDataLoader, val name: String)

private val batchLoaders = mutableListOf<LoaderHolder<BatchLoader<*, *>>>()
private val batchLoadersWithContext = mutableListOf<LoaderHolder<BatchLoaderWithContext<*, *>>>()
Expand All @@ -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()
Expand Down Expand Up @@ -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 <T : Any> createHolder(t: T): LoaderHolder<T> = 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)
}
}
Expand All @@ -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 <T : Any> createHolder(t: T): LoaderHolder<T> =
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)
}
}
Expand Down
18 changes: 10 additions & 8 deletions graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
68 changes: 52 additions & 16 deletions graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsDataLoaderProviderTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@

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
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
Expand Down Expand Up @@ -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()
Expand All @@ -63,15 +70,25 @@ 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<InvalidDataLoaderTypeException> { provider.findDataLoaders() }
}

@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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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<Any, Any>("DGS_DATALOADER_CLASS_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutName")
val dataLoader =
dataLoaderRegistry.getDataLoader<Any, Any>("ExampleBatchLoaderWithoutName")
Assertions.assertNotNull(dataLoader)
}

Expand Down Expand Up @@ -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<Any, Any>("DGS_DATALOADER_FIELD_com.netflix.graphql.dgs.ExampleBatchLoaderWithoutNameFromField#batchLoader")
Assertions.assertNotNull(dataLoader)

val privateDataLoader = dataLoaderRegistry.getDataLoader<Any, Any>("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"
)
}
}

Expand Down

0 comments on commit 5787ef5

Please sign in to comment.