Add trivial field to DgsData / DgsQuery annotations for marking trivial data fetcher methods #1955
Conversation
| @Target({ElementType.METHOD, ElementType.TYPE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Inherited | ||
| public @interface Trivial { } |
There was a problem hiding this comment.
DgsTrivial?
I'm torn on this, as I never liked the prefix, but it's on nearly all of the interfaces. And with the spring-graphql integration, I feel like the inconsistency might add some initial readability confusion.
There was a problem hiding this comment.
Yeah I understand that concern, I ended up revisiting just adding a new field to the existing annotations based on Paul's feedback, and was able to make that work, so I'm scrapping this new annotation anyway.
| @Target({ElementType.METHOD, ElementType.TYPE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Inherited | ||
| public @interface Trivial { } |
There was a problem hiding this comment.
Have you considered making it a new argument for @DgsData? As long as it has a default value, that would be backward compatible as well I think?
There was a problem hiding this comment.
I recall considering that approach, but I couldn't think of a clean way to make DgsQuery work, although I guess I could add it to DgsQuery as well and annotate the field with @AliasFor to make that work. Having multiple DgsData annotations via @DgsData.List is also another thing I'd have to make sure works correctly. I can experiment with that and see if it works.
There was a problem hiding this comment.
@paulbakker okay, I've updated the PR to just use fields on the existing DgsData / DgsQuery annotations, and added test cases for the various scenarios.
There was a problem hiding this comment.
Very nice! I think I prefer this approach, WDYT?
There was a problem hiding this comment.
Yeah, I prefer this over adding a new annotation.
| @@ -39,16 +43,31 @@ class MethodDataFetcherFactory( | |||
| private val resolvers = ArgumentResolverComposite(argumentResolvers) | |||
|
|
|||
| fun createDataFetcher(bean: Any, method: Method): DataFetcher<Any?> { | |||
| if (isTrivial(method) || isTrivial(bean.javaClass)) { | |||
There was a problem hiding this comment.
Wondering if we should allow this on class level. It's probably unlikely that it's adding a lot of convenience, while it would be easy to have an unintended side-effect on datafetchers added later to an existing class.
There was a problem hiding this comment.
I can just remove it for now then.
Add a "trivial" boolean field to DgsData and DgsQuery annotations which allows signaling to DGS that the generated DataFetcher should implement TrivialDataFetcher. This can benefit data fetcher methods that don't do any I/O, since they can be handled specially by instrumentation.
kilink
force-pushed
the
trivial-data-fetcher
branch
from
21848ff
to
20c8e57
Compare
kilink
changed the title
| @@ -38,17 +43,44 @@ class MethodDataFetcherFactory( | |||
|
|
|||
| private val resolvers = ArgumentResolverComposite(argumentResolvers) | |||
|
|
|||
| fun createDataFetcher(bean: Any, method: Method): DataFetcher<Any?> { | |||
There was a problem hiding this comment.
Is it possible to add a test case with a trivial data fetcher usage?
There was a problem hiding this comment.
I had written tests but forgot to check them in. I've updated the PR.
kilink
force-pushed
the
trivial-data-fetcher
branch
from
0f2b66f
to
2e5d85e
Compare
kilink
force-pushed
the
trivial-data-fetcher
branch
from
2e5d85e
to
e80ed48
Compare
Add ability to mark data fetcher methods as trivial
Add a "trivial" boolean field to DgsData and DgsQuery annotations which allows signaling to DGS
that the generated DataFetcher should implement TrivialDataFetcher. This can benefit data fetcher
methods that don't do any I/O, since they can be handled specially by instrumentation.