Clarify entity fetcher contract #192

lorensr · 2024-11-13T23:58:33Z

Should return either a single instance or null (technically can return a List with one item in it and the framework will unwrap, but not mentioning that as I wouldn't recommend it)
Can fetch data

srinivasankavitha · 2024-11-14T00:26:19Z

docs/federation.md

+
+In this case, we're not doing any data fetching: our `Show` instance only has an `id` field, and we implement a `Show.reviews` datafetcher in the [next section](#providing-data-with-a-data-fetcher). 
+
+However, we could instead fetch data in the entity fetcher. If our DGS served multiple fields, and they all came from the same data source, we could fetch them all at once in the entity fetcher instead of writing separate datafetchers for each field:


I would leave this out from the docs - don't think we want to necessarily recommend this as a pattern.

So is it an anti-pattern to build the whole entity object? Should we always return the partial entity (only id field)?
If it is, maybe it makes sense to mentioned that it is not recommended.

IMO it's a valid thing to do, and sometimes the better thing to do, and many won't realize it's an option unless we say it is. Maybe can change/add language around it to say only use it in this circumstance, or "here is why you often don't want to do it?"

Ok, I guess if it's providing multiple federated fields - so would be good to highlight that.

+1 that this is a valid pattern

pkhamutou · 2024-11-14T10:27:03Z

docs/federation.md

-The DGS framework does most of the heavy lifting, and all we have to do is provide the following:
+The Reviews DGS needs to implement an entity fetcher to handle this query.
+
+- **Entity fetcher**: A method annotated with [`@DgsEntityFetcher`](https://javadoc.io/doc/com.netflix.graphql.dgs/graphql-dgs/latest/com/netflix/graphql/dgs/DgsEntityFetcher.html) that takes a key and returns a single instance of the entity or null.


what are the implications of returning the entity or null? which one is preferred?

I think it also makes sense to mention that the entity resolver must return some value for every key.

pkhamutou · 2024-11-14T10:28:48Z

docs/federation.md

+
+```java
+@DgsEntityFetcher(name = "Show")
+public Show movie(Map<String, Object> values, DataFetchingEnvironment env) {


it should be CompletableFuture<Show>

Why? I don't think it needs to be?

doesn't dataLoader.load(showId) return a Future?

pkhamutou · 2024-11-14T10:29:16Z

docs/federation.md

 }
 ```

+If there is no such Show with the given id in the database, the entity fetcher should return `null`.


null or new Show(showId)?

Maybe explain a bit further what this means for the federated request. E.g. will this show as an error

pkhamutou · 2024-11-14T10:31:52Z

docs/federation.md

+
+In this case, we're not doing any data fetching: our `Show` instance only has an `id` field, and we implement a `Show.reviews` datafetcher in the [next section](#providing-data-with-a-data-fetcher). 
+
+However, we could instead fetch data in the entity fetcher. If our DGS served multiple fields, and they all came from the same data source, we could fetch them all at once in the entity fetcher instead of writing separate datafetchers for each field:


So is it an anti-pattern to build the whole entity object? Should we always return the partial entity (only id field)?
If it is, maybe it makes sense to mentioned that it is not recommended.

paulbakker · 2024-11-14T20:20:01Z

docs/federation.md

 ### Testing a Federated Query

-You can always manually test federated queries by running the gateway and your DGS locally. 
+You can always manually test federated queries by running the gateway and your DGS locally.


I think we should flip this text around and focus on testing without running a gateway, so:

Write a test

Test by sending a federated query directly to the DGS (e.g. using graphiql)

Run your gateway locally

paulbakker

Thanks for updating this. The changes look good, but I left some comments for further improvements.

loren added 3 commits November 13, 2024 16:57

Autoformat

7f54fce

Clarify entity fetcher contract

6ddf35e

Formatting

e65009d

lorensr requested a review from srinivasankavitha November 13, 2024 23:58

srinivasankavitha requested changes Nov 14, 2024

View reviewed changes

pkhamutou reviewed Nov 14, 2024

View reviewed changes

paulbakker reviewed Nov 14, 2024

View reviewed changes

paulbakker requested changes Nov 14, 2024

View reviewed changes


		In this case, we're not doing any data fetching: our `Show` instance only has an `id` field, and we implement a `Show.reviews` datafetcher in the [next section](#providing-data-with-a-data-fetcher).

		However, we could instead fetch data in the entity fetcher. If our DGS served multiple fields, and they all came from the same data source, we could fetch them all at once in the entity fetcher instead of writing separate datafetchers for each field:

Uh oh!

Clarify entity fetcher contract #192

Are you sure you want to change the base?

Clarify entity fetcher contract #192

Uh oh!

Conversation

lorensr commented Nov 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulbakker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants