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

feat(Apollo): Add support for @provides and @requires directive. #7503

Merged
merged 18 commits into from
Mar 8, 2021

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Mar 2, 2021

Fixes GRAPHQL-1053.
This PR adds support for @provides and @requires directive for Apollo federation according to the [federation specs]

@provides

@provides directive is used on a field that tells the Gateway to return a specific fieldSet from the base type while fetching the field. For example:

type Review @key(fields: "id") {
  product: Product @provides(fields: "name price")
}

extend type Product @key(fields: "upc") {
  upc: String @external
  name: String @external
  price: Int @external
}

while fetching Review.product from the reviews service, and if the name or price is also queried, the Gateway will fetch these from the review service itself. This means that the review service also resolves these fields, even though both fields are @external.

@requires

@requires field is used on a field to annotate the fieldSet of the base type. It is used to develop a query plan where the required fields may not be needed by the client, but the service may need additional information from other services. For example:

extend type User @key(fields: "id") {
  id: ID! @external
  email: String @external
  reviews: [Review] @requires(fields: "email")
}

when the Gateway fetches user.reviews from the review service, the Gateway will provide user.email from the User service and provide it as an argument to the _entities query.

(https://www.apollographql.com/docs/federation/federation-spec/).


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 2, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Looking good, will review fully when finished.

Reviewed 7 of 7 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)


graphql/schema/gqlschema.go, line 577 at r1 (raw file):

directiveLocationMap

also add a nil entry for the new directives in this map


graphql/schema/rules.go, line 2115 at r1 (raw file):

typ.Directives.ForName(apolloKeyDirective)

stale :)

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)


graphql/schema/gqlschema.go, line 577 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
directiveLocationMap

also add a nil entry for the new directives in this map

Done.


graphql/schema/rules.go, line 2115 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
typ.Directives.ForName(apolloKeyDirective)

stale :)

Done.

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @pawanrawal)

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Please add a detailed commit description explaining what the feature does.

Reviewed 5 of 7 files at r1, 2 of 3 files at r2, 6 of 6 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel)


graphql/schema/gqlschema.go, line 1611 at r3 (raw file):

// Returns true if the field is of type which can be summed. Eg: int, int64, float
func isSummable(fld *ast.FieldDefinition, defn *ast.Definition, providesTypeMap map[string]bool) bool {
	if hasExternal(fld) && !isKeyField(fld, defn) && !providesTypeMap[fld.Name] {

can we define an fn for this as it is being used at multiple places


graphql/schema/gqlschema.go, line 2224 at r3 (raw file):

		// Ignore Fields with @external directives and excluding those which are present
		// as an argument in @key directive
		if hasExternal(fld) && !isKeyField(fld, defn) && !providesTypeMap[fld.Name] {

use the fn defined above

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 12 of 14 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)


graphql/schema/gqlschema.go, line 1611 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can we define an fn for this as it is being used at multiple places

Done.


graphql/schema/gqlschema.go, line 2224 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

use the fn defined above

Done.

@minhaj-shakeel minhaj-shakeel merged commit 02e5170 into master Mar 8, 2021
@minhaj-shakeel minhaj-shakeel deleted the minhaj/require-provide branch March 8, 2021 06:19
aman-bansal pushed a commit that referenced this pull request Mar 9, 2021
Fixes GRAPHQL-1053.
This PR adds support for @provides and @requires directive for Apollo federation according to the [federation specs]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants