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

chore: rfc foundation for @openfed__prerequisite #1429

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Aenimus
Copy link
Member

@Aenimus Aenimus commented Dec 10, 2024

Motivation and Context

Checklist

directive with the `resolvable` argument either omitted or explicitly set to true.

### The fields input
The `fields` input is type `openfed__InputSet!` (henceforth simply `InputSet`).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced the name of this input is best. Some other potential ideas:

  • MappedFieldSet
  • MapSet
  • InputToFieldSet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that InputToFieldSet is too verbose. I personally like MappedFieldSet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you can make it clear that it's specific for openfed__ResolveEntityInput by calling it ResolveFieldSet

```graphql
type Mutation {
updateEmail(
id: ID! @openfed__prerequisite(resolveEntity: { typeName: "User", fields: "id" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding correctly, you have to provide the key fields, and then the rest of the fields get fetched and provided for you by the Router? Do the non-ID fields get stripped out of the composed schema? Otherwise I imagine this would be confusing and people would try passing in these fields.

I'm not totally following the use case — if I want to update the email, why would I fetch the email from the User subgraph? Don't I want to overwrite that value with a new email?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to update an email on a user. How do you know that user you're trying to update with the input is a valid user?

Cosmo will produce an error and the operation will not be committed.

### Potential discussion points
- Multiple entities (batching)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very important. Is this API intended to only work for mutations or is this usable for queries as well?

This means that entity reference is valid and the operation cannot proceed.

### Entity Representation Call returns null
If the entity representation call is unsuccessful, null will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null will be returned by what / to what? In general, I think a better strategy is to pass the error to the subgraph and allow it to read the error and determine how to proceed. Some errors on some fields will be fail open, and others will be fail closed. IMO we should allow the "client" to decide how to handle failures.

Copy link
Member Author

@Aenimus Aenimus Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the entity resolver cannot match an entity for the key(s) provided, it returns null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it matches, but some of the fields return errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. This is an entity representation call like any other. We just provide indirect access to it through the same subgraph.

Copy link
Member Author

@Aenimus Aenimus Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some confusion here. This directive validates an entity. It does not perform across-subgraph calls. You provide the primary key fields and check whether it matches an entity.

The functionality that I think you're describing is something else. This RFC is currently on the sidebench while thinking about what and how we'd like to address injection/inter-subgraph resolutions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. If you do intend to work on this more it would be helpful to add a couple examples showing the actual requests the Router would make to each subgraph including the request body to better understand the call patterns and intent.

Copy link

github-actions bot commented Jan 3, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants