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

Introduce @requiresScopes directive in composition #2649

Merged
merged 17 commits into from
Jul 13, 2023

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented Jun 30, 2023

Effectively duplicated and adapted work from #2644

This PR introduces the new @requiresScopes directive for composition

With this change, users can now compose @requiresScopes applications from their subgraphs into a supergraph. This addition will support a future version of Apollo Router that enables authenticated access to specific types and fields via directive applications.

Since the implementation of @requiresScopes is strictly a composition and execution concern, there's no change to the query planner with this work. The execution work is well under way in Apollo Router (and won't be built at all for Gateway). So as far as this repo is concerned, only composition is concerned with @requiresScopes.

TODO:

  • changelog entry (pending some internal feedback)
  • possibly some more interesting test cases
  • pending discussion on use of custom Scope scalar for input type

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2023

🦋 Changeset detected

Latest commit: b360a78

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@apollo/query-planner Minor
@apollo/composition Minor
@apollo/federation-internals Minor
@apollo/gateway Minor
@apollo/query-graphs Minor
@apollo/subgraph Minor
apollo-federation-integration-testsuite Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 30, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

@pcmanus pcmanus left a comment

Choose a reason for hiding this comment

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

Had a look, and currently lgtm. Holding off on approving until this get out of "draft" and we've dealt with the few remaining TODO, but so far so good.

composition-js/src/__tests__/compose.test.ts Outdated Show resolved Hide resolved
gateway-js/src/__tests__/gateway/testUtils.ts Outdated Show resolved Hide resolved
@trevor-scheer trevor-scheer marked this pull request as ready for review July 7, 2023 23:47
@trevor-scheer trevor-scheer requested a review from a team as a July 7, 2023 23:47
@clenfest
Copy link
Contributor

I may have missed a conversation on this, but did we want to have two separate specs or a single spec for this?

@trevor-scheer trevor-scheer force-pushed the trevor/authenticated branch 2 times, most recently from b12d720 to b3c7dfb Compare July 12, 2023 17:22
@trevor-scheer trevor-scheer force-pushed the trevor/requiresScopes branch 2 times, most recently from edd7637 to cc41875 Compare July 12, 2023 17:51
@clenfest
Copy link
Contributor

Oh, one thing that I remembered. Could you add both directives into the directivesComposedByDefault array on line 172 of composeDirectiveManager.ts? It will prevent people from trying to manually promote those directives to the supergraph.

Base automatically changed from trevor/authenticated to next July 12, 2023 18:48
@trevor-scheer trevor-scheer force-pushed the trevor/requiresScopes branch 2 times, most recently from d914751 to 3fa6c8a Compare July 12, 2023 19:05
@clenfest
Copy link
Contributor

looks good

internals-js/src/directiveAndTypeSpecification.ts Outdated Show resolved Hide resolved
this.registerDirective(
TAG_VERSIONS.find(new FeatureVersion(0, 3))!.tagDirectiveSpec
);
this.registerSubFeature(TAG_VERSIONS.find(new FeatureVersion(0, 3))!);
Copy link
Contributor

Choose a reason for hiding this comment

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

RegisterSubFeature should probably take the whole FeatureDefinitions[] and the federation version, then it can figure out what version is needed based on the minimumFederationVersion specified. That we don't have to have the federationSpec know anything about the version relationships and can get rid of all of these if version statements.

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 disagree with the ergonomic benefit, but there's a bit to untangle underneath that I'd rather punt on.

@trevor-scheer
Copy link
Member Author

@clenfest approved this in Slack and I'd like to get this on next today so we can kick off a harness run.

@trevor-scheer trevor-scheer merged commit 4f3c3b9 into next Jul 13, 2023
@trevor-scheer trevor-scheer deleted the trevor/requiresScopes branch July 13, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants