Provide consistent support for dynamically registered DataFetchers #1770

ivanasen · 2024-01-08T22:15:53Z

Pull request checklist

Please read our contributor guide
Consider creating a discussion on the discussion forum
first
Make sure the PR doesn't introduce backward compatibility issues
Make sure to have sufficient test cases

Pull Request type

Changes in this PR

Implemented a new way of using @DgsCodeRegistry in which a DgsCodeRegistryBuilder is injected instead. This way, the registered DataFetchers are processed by DataFetcherResultProcessors and the behavior is more consistent with the annotation-based approach.

Link to discussion

Alternatives considered

Haven't considered alternatives.

Implemented a new way of using `@DgsCodeRegistry` in which a `DgsCodeRegistryBuilder` is injected instead. This way, the registered DataFetchers are processed by DataFetcherResultProcessors and the behaviour is more consistent with the annotation-based approach. [Link to discussion](Netflix#1768)

srinivasankavitha

Thanks for the PR contribution. Changes look good, just had a few comments.

graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsSchemaProvider.kt

srinivasankavitha · 2024-01-09T15:43:38Z

graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsSchemaProviderTest.kt

@@ -477,29 +481,66 @@ internal class DgsSchemaProviderTest {
                val coordinates = FieldCoordinates.coordinates("Query", "myField")
                return codeRegistryBuilder.dataFetcher(coordinates, df)
            }
+
+            // Result should be processed by DataFetcherResultProcessors
+            @DgsCodeRegistry


Would you be able to also update our docs to use the DgsCodeRegistryBuilder? Much appreciated.

Actually, where are the docs 😄 ?

graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsCodeRegistryBuilder.java

ivanasen · 2024-01-09T19:40:05Z

I was looking at the code today in DgsSchemaProvider.kt and I actually think that in order to really be consistent we need to not account for only the DataFetcherResultProcessors in the new DgsCodeRegistryBuilder . In order for it to be really consistent, shouldn't we account for most of the logic in DgsSchemaProvider::registerDataFetcher? Also, I shouldn't be duplicating logic here. Registering DataFetchers through annotations and dynamically should use common logic. So in that case, we should refactor out the DgsSchemaProvider::registerDataFetcher (or most parts of it) in some common place which can be invoked from both DgsSchemaProvider and DgsCodeRegistryBuilder.

I won't have time to make this change in the next 2-3 days, but will most likely get to it in the weekend

srinivasankavitha · 2024-01-10T18:07:40Z

I was looking at the code today in DgsSchemaProvider.kt and I actually think that in order to really be consistent we need to not account for only the DataFetcherResultProcessors in the new DgsCodeRegistryBuilder . In order for it to be really consistent, shouldn't we account for most of the logic in DgsSchemaProvider::registerDataFetcher? Also, I shouldn't be duplicating logic here. Registering DataFetchers through annotations and dynamically should use common logic. So in that case, we should refactor out the DgsSchemaProvider::registerDataFetcher (or most parts of it) in some common place which can be invoked from both DgsSchemaProvider and DgsCodeRegistryBuilder.

I won't have time to make this change in the next 2-3 days, but will most likely get to it in the weekend

Ok. I don't mind the current solution in any case. It's really not much duplication.

ivanasen · 2024-01-15T21:06:08Z

I was looking at the code today in DgsSchemaProvider.kt and I actually think that in order to really be consistent we need to not account for only the DataFetcherResultProcessors in the new DgsCodeRegistryBuilder . In order for it to be really consistent, shouldn't we account for most of the logic in DgsSchemaProvider::registerDataFetcher? Also, I shouldn't be duplicating logic here. Registering DataFetchers through annotations and dynamically should use common logic. So in that case, we should refactor out the DgsSchemaProvider::registerDataFetcher (or most parts of it) in some common place which can be invoked from both DgsSchemaProvider and DgsCodeRegistryBuilder.
I won't have time to make this change in the next 2-3 days, but will most likely get to it in the weekend

Ok. I don't mind the current solution in any case. It's really not much duplication.

Ok, if I'm gonna make more changes better make them in pieces. For this commit, I propose we just handle this.

srinivasankavitha · 2024-01-16T16:44:10Z

I was looking at the code today in DgsSchemaProvider.kt and I actually think that in order to really be consistent we need to not account for only the DataFetcherResultProcessors in the new DgsCodeRegistryBuilder . In order for it to be really consistent, shouldn't we account for most of the logic in DgsSchemaProvider::registerDataFetcher? Also, I shouldn't be duplicating logic here. Registering DataFetchers through annotations and dynamically should use common logic. So in that case, we should refactor out the DgsSchemaProvider::registerDataFetcher (or most parts of it) in some common place which can be invoked from both DgsSchemaProvider and DgsCodeRegistryBuilder.
I won't have time to make this change in the next 2-3 days, but will most likely get to it in the weekend

Ok. I don't mind the current solution in any case. It's really not much duplication.

Ok, if I'm gonna make more changes better make them in pieces. For this commit, I propose we just handle this.

Sounds good. @kilink - any objections to merging this PR?

graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsCodeRegistryBuilder.kt

kilink · 2024-01-17T18:25:19Z

I was looking at the code today in DgsSchemaProvider.kt and I actually think that in order to really be consistent we need to not account for only the DataFetcherResultProcessors in the new DgsCodeRegistryBuilder . In order for it to be really consistent, shouldn't we account for most of the logic in DgsSchemaProvider::registerDataFetcher? Also, I shouldn't be duplicating logic here. Registering DataFetchers through annotations and dynamically should use common logic. So in that case, we should refactor out the DgsSchemaProvider::registerDataFetcher (or most parts of it) in some common place which can be invoked from both DgsSchemaProvider and DgsCodeRegistryBuilder.
I won't have time to make this change in the next 2-3 days, but will most likely get to it in the weekend

Ok. I don't mind the current solution in any case. It's really not much duplication.

Ok, if I'm gonna make more changes better make them in pieces. For this commit, I propose we just handle this.

Sounds good. @kilink - any objections to merging this PR?

LGTM, just had 2 minor comments about nullability of parameters.

ivanasen · 2024-01-20T17:57:06Z

Thanks, I updated the PR with the suggested changes. I haven't received an answer to this question though

srinivasankavitha · 2024-01-22T18:27:34Z

Thanks, I updated the PR with the suggested changes. I haven't received an answer to this question though

Think we decided to keep things as is for now. We can do a refactor in a separate PR in the future, but I'm also fine leaving things the way they are for now.

The branch has some conflicts. If you could resolve them when you have a chance, we'll go ahead and merge the PR. Thanks for all the work on the PR!

ivanasen added 3 commits January 9, 2024 00:11

Fix lint errors

e5b200d

Fix lint errors

abfdbcb

srinivasankavitha requested changes Jan 9, 2024

View reviewed changes

kilink reviewed Jan 9, 2024

View reviewed changes

graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsCodeRegistryBuilder.java Outdated Show resolved Hide resolved

Merge branch 'master' into master

eac2155

ivanasen and others added 2 commits January 15, 2024 23:00

Remove code duplications in DgsCodeRegistryBuilder

6d82c15

Merge branch 'master' into master

ec8fd77

kilink reviewed Jan 17, 2024

View reviewed changes

graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsCodeRegistryBuilder.kt Outdated Show resolved Hide resolved

kilink reviewed Jan 17, 2024

View reviewed changes

graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsCodeRegistryBuilder.kt Outdated Show resolved Hide resolved

Make params non-nullable

687277f

ivanasen and others added 5 commits January 23, 2024 10:26

Merge branch 'master' into master

ca29594

Fix lint error

55a2112

Merge branch 'master' into master

1bf2901

Fix merge conflict

d4b62ff

Fix tests

7eee501

srinivasankavitha merged commit 0b0f41d into Netflix:master Jan 29, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide consistent support for dynamically registered DataFetchers #1770

Provide consistent support for dynamically registered DataFetchers #1770

ivanasen commented Jan 8, 2024

srinivasankavitha left a comment

srinivasankavitha Jan 9, 2024

ivanasen Jan 9, 2024

ivanasen Jan 15, 2024

ivanasen commented Jan 9, 2024 •

edited

Loading

srinivasankavitha commented Jan 10, 2024

ivanasen commented Jan 15, 2024

srinivasankavitha commented Jan 16, 2024

kilink commented Jan 17, 2024

ivanasen commented Jan 20, 2024 •

edited

Loading

srinivasankavitha commented Jan 22, 2024 •

edited

Loading

Provide consistent support for dynamically registered DataFetchers #1770

Provide consistent support for dynamically registered DataFetchers #1770

Conversation

ivanasen commented Jan 8, 2024

Pull request checklist

Pull Request type

Changes in this PR

Alternatives considered

srinivasankavitha left a comment

Choose a reason for hiding this comment

srinivasankavitha Jan 9, 2024

Choose a reason for hiding this comment

ivanasen Jan 9, 2024

Choose a reason for hiding this comment

ivanasen Jan 15, 2024

Choose a reason for hiding this comment

ivanasen commented Jan 9, 2024 • edited Loading

srinivasankavitha commented Jan 10, 2024

ivanasen commented Jan 15, 2024

srinivasankavitha commented Jan 16, 2024

kilink commented Jan 17, 2024

ivanasen commented Jan 20, 2024 • edited Loading

srinivasankavitha commented Jan 22, 2024 • edited Loading

ivanasen commented Jan 9, 2024 •

edited

Loading

ivanasen commented Jan 20, 2024 •

edited

Loading

srinivasankavitha commented Jan 22, 2024 •

edited

Loading