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

Add federation metadata extension methods and attributes #3958

Merged
merged 14 commits into from
Jun 8, 2024

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jun 7, 2024

Part 2 for #3921

Does not include the directive definitions

  • Add documentation
  • Scope extension methods to the proper types
  • Add url links to Apollo docs

@Shane32 Shane32 requested a review from gao-artur June 7, 2024 15:56
@Shane32 Shane32 self-assigned this Jun 7, 2024
@gao-artur
Copy link
Contributor

A lot of work for analyzers 🥴

@Shane32
Copy link
Member Author

Shane32 commented Jun 7, 2024

I've got some ideas....

@Shane32 Shane32 changed the title Add federation metadata extension methods and attributes [WIP] Add federation metadata extension methods and attributes Jun 7, 2024
@Shane32 Shane32 added this to the 8.0 milestone Jun 7, 2024
@Shane32
Copy link
Member Author

Shane32 commented Jun 7, 2024

Ok the extension methods and attributes should all be scoped to the appropriate targets.

@Shane32
Copy link
Member Author

Shane32 commented Jun 7, 2024

Have not yet added url links within the xml comments

@Shane32 Shane32 changed the title [WIP] Add federation metadata extension methods and attributes Add federation metadata extension methods and attributes Jun 7, 2024
@Shane32 Shane32 requested a review from gao-artur June 7, 2024 18:08
@Shane32 Shane32 added enhancement New feature or request federation Relates to GraphQL Federation labels Jun 7, 2024
/// <remarks>
/// See <see href="https://www.apollographql.com/docs/federation/federated-types/federated-directives#key"/>.
/// </remarks>
public static TMetadataWriter Key<TMetadataWriter>(this TMetadataWriter graphType, string[] fields, bool resolvable = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This extension is useful to define compound or nested field keys. But I guess a single field as a key should be the most frequently used. Then need another extension based on IFieldMetadataWriter

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, FieldBuilder implements IMetadataWriter, so it works for all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it doesn't implement IComplexGraphType so it doesn't work...

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 don't follow you. @key is only valid on an object or interface, which is what IComplexGraphType is.

directive @key(fields: FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE

https://www.apollographql.com/docs/federation/federated-types/federated-directives#key

Copy link
Member Author

@Shane32 Shane32 Jun 7, 2024

Choose a reason for hiding this comment

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

As is implemented now, these methods simply add directives to the schema. They are not 'intelligent'; for example, you can't put [Key] on a field and have it automatically add the proper directive to the graph type. While that's possible, it's not the current design.

So for @key, you can do @key(fields: "id name pricing { id }") by specifying this.Key("id name pricing { id }") on the graph type.

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 see. Then, it can be enchantment in 8.1 if you prefer.

Yes let’s just get a workable version pushed out

Or at least merged in so we can review what further enhancements we want to make

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw key on interface is only supported sine v2.3. Adding it now will require adding version detection/configuration + validation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we need to work on that (not yet in #3921 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally, I'm planning to have the proper directives added based on the version. Schema validation will already match up the applied directives to configured directives. So it will catch this error if it's the wrong version.

Copy link
Member Author

Choose a reason for hiding this comment

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

having one target IObjectGraphType and the other target IInterfaceGraphType

Sounds better than implementing an analyzer to prevent incorrect usage. We can probably use the [AllowedOn<>], but I'd prefer strongly typed API over static analysis.

I’ll make the change

Done

@Shane32 Shane32 merged commit 68a726c into develop Jun 8, 2024
11 checks passed
@Shane32 Shane32 deleted the fed2_metadata branch June 8, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request federation Relates to GraphQL Federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants