Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Use class name by default for @DgsDataLoader#name, and add some inline methods to DgsDataFetchingEnvironment #861

Closed
jord1e opened this issue Feb 5, 2022 · 5 comments
Labels
enhancement New feature or request Need more detail This issue needs more detail, or will be closed otherwise. stale

Comments

@jord1e
Copy link
Contributor

jord1e commented Feb 5, 2022

Please read our contributor guide before
creating an issue. Also consider discussing your idea on
the discussion forum first.

Describe the Feature Request

The the annotated class's name as the name field of the @DgsDataLoader annotation by default
When annotating methods use something like SimpleClassName.methodName

Describe Preferred Solution

Using

@DgsDataLoader
class PersonByIdDataLoader : MappedBatchLoader<String, Try<Person>>{
    override fun load(keys: Set<String>): CompletionStage<Map<String, Try<Person>>> {
        TODO("Not yet implemented")
    }
}

We can then do:

val dl = dfe.getDataLoader<String, Try<Person>>(PersonByIdDataLoader::class.java)
// or
val dl = dfe.getDataLoader<String, Try<Person>>("PersonByIdDataLoader")

We may also wish to add some extra (inline-reified) methods to DgsDataFetchingEnvironment, for example:

inline fun <K, V, reified T : MappedBatchLoader<K, V>> DgsDataFetchingEnvironment.getMappedBatchLoader(): DataLoader<K, V> {
    return this.getDataLoader(T::class.java)
}
inline fun <K, V, reified T : BatchLoader<K, V>> DgsDataFetchingEnvironment.getBatchLoader(): DataLoader<K, V> {
    return this.getDataLoader(T::class.java)
}

Which gives us compile-time type checking:
incorrect:
afbeelding

correct:
afbeelding

Describe Alternatives

For the annotation proposal: keep it as is

For the methods:

inline fun <K, V, reified T> DgsDataFetchingEnvironment.getDataLoader(): DataLoader<K, V> {
    return this.getDataLoader(T::class.java)
}

would work instead of 4 seperate functions, but this does not do any type checking, it merely avoids having to use ::class.java:
afbeelding

Contributing

Would be willing to contribute a PR, or two separate ones.

@jord1e jord1e added the enhancement New feature or request label Feb 5, 2022
@srinivasankavitha
Copy link
Contributor

@jord1e - What exactly is the issue? This should work fine in your case, no?

val dataloader1 = dfe.getDataLoader<String, Try<Person>>(PersonByIdDataLoader::class.java)

The above should anyway handle the case where the data loader is a mapped data loader. Why do you need the second one shown in your example, i.e.

val dataloader2 = dfe.getMappedDataLoader<String, Try<Person>>()

@srinivasankavitha srinivasankavitha added the Need more detail This issue needs more detail, or will be closed otherwise. label Feb 27, 2022
@jord1e
Copy link
Contributor Author

jord1e commented Mar 3, 2022

What exactly is the issue? This should work fine in your case, no?

Yes, it works, but it does not perform type-checking at compile time. Adding these utility methods gives library consumers to implement compile-time type-checking.

In the first example PersonByIdDataLoader is a MappedBatchLoader<String, Try<Person>>.
Because I am calling getMappedBatchLoader<String, Person, PersonByIdDataLoader>() in the example, the two first types are checked against the PersonByIdDataLoader, because the class returns Try<Person> we get a compilation error.

This is handy for when a return type or dataloader generic parameters changes types.

The four methods will be supplementary to the existing getDataLoader method.

@paulbakker
Copy link
Collaborator

paulbakker commented May 2, 2022

This is merged in #904 correct? Or is there more to it?

Can this issue be closed?

@github-actions github-actions bot added the stale label May 3, 2023
@Emily
Copy link
Contributor

Emily commented Dec 7, 2023

Appears to have been solved by #904.

@Emily Emily closed this as completed Dec 7, 2023
@jord1e
Copy link
Contributor Author

jord1e commented Dec 7, 2023

It has indeed, I forgot to reply to Paul, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Need more detail This issue needs more detail, or will be closed otherwise. stale
Projects
None yet
Development

No branches or pull requests

4 participants