Allows `@shareable` to be repeatable (to allow it on both definition and extensions) #2175

pcmanus · 2022-09-30T09:06:47Z

The rational this is explained on #2135 and in particular allows the example in this comment to work.

For backward compatibility concerns, the patch introduces a new version (federation/v2.2) of the federation spec and only make @shareable repeatable in that version. This means that nothing changes for existing subgraphs on update and you have to "manually" change the subgraph to use the new version to be able to use this. More importantly, this means that subgraph libraries that continue to generate the previous version won't break in any way. They just may want to start generating the new version at some point in the future so their users can make use of the additional flexibility afforded by this change.

On the upgrade side of things, probably stating the obvious, but if a subgraph does upgrade to use the new 2.2 version, then it needs to make sure to use a version of composition that knows it.

Regarding the code itself, marking @shareable repeatable is trivial, but more interesting changes are:

the added validation discussed on Shareable directive doesn't behave as expected with extended types #2135.
some refactoring around the code for the federation spec. Essentially, the exact definitions of the federation directives really depends on the spec version, even more so with this change, but that wasn't well abstracted by the code so far, so this patch tries to improve this a bit.

netlify · 2022-09-30T09:06:50Z

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`ca9e2f4`

codesandbox-ci · 2022-09-30T09:07:51Z

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.

pcmanus · 2022-09-30T09:13:07Z

docs/source/federated-types/sharing-types.mdx

+# This example only works with v2.2+ of the federation specification. In earlier versions, `@shareable` was
+# not marked `repeatable` and so this example would be rejected by graphQL.
+extend schema
+  @link(url: "https://specs.apollo.dev/federation/v2.2", import: ["@shareable"])


Side-note: I noticed that the document has plenty of @link, and almost all of them still link to federation/v2.0 (the one exception is the part about @composeDirective with links to federation/v2.1 because it has to). But I've been wondering if we shouldn't try to have the doc always link to the most recent spec version? I mean, we probably have to at the moment because studio composition is lagging behind, but this doesn't feel like we setup user for the best experience, so it might be worth discussing how we get to a better state. Anyway, certainly an aside...

pcmanus · 2022-09-30T09:22:38Z

subgraph-js/src/__tests__/buildSubgraphSchema.test.ts

@@ -133,12 +133,12 @@ directive @requires(fields: _FieldSet!) on FIELD_DEFINITION

 directive @provides(fields: _FieldSet!) on FIELD_DEFINITION

-directive @extends on OBJECT | INTERFACE


Note to reviewers: the reason the order of this changed is due to some code "consolidation" of the refactoring done in federationSpec.ts/federation.ts. More precisely, the completeFed1SubgraphSchema method in federation.ts used to manually list the directive, and it did so in an ever so slightly different order than how those same directives are listed in the fed2 spec. But the code now use the same list in both case (the legacyFederationDirectives list in federationSpec.ts) and that list use the existing order of the fed2 spec, but thus slightly change the order when we're "completing" a fed1 schema.

Now, we could of course avoid that order change here by recreating 2 separate lists, but I genuinely think the change of order here doesn't matter (and so is worth consolidating the code a bit). In particular, I don't think this would show up in "diffs" for users, because this here is the output of buildSubgraphSchema, and that's only used by subgraph to generate a valid schema that can be run by Apollo Server, but it is not the schema that user upload to studio in particular (use upload the "input" of buildSubgraphSchema).

clenfest

A couple of minor comments, but approved otherwise.

clenfest · 2022-10-18T13:48:39Z

composition-js/src/__tests__/compose.test.ts

+      // Note that a previous test makes sure that _not_ having @shareable on the type extension ends up failing (as `b` is
+      // not considered shareable in `subgraphA`. So succeeding here shows both that @shareable is accepted in the 2 places
+      // (definition and extension) but also that it's properly taking into account.
+      assertCompositionSuccess(result);


Did you want to assert anytyhing about the fields existing in the supergraph?

I wasn't sure there wasn't much meaningful to test about fields in the supergraph since @shareable is mostly about preventing composition in some cases, but results in nothing special in the supergraph.

clenfest · 2022-10-18T13:52:31Z

subgraph-js/src/__tests__/buildSubgraphSchema.test.ts

 directive @external(reason: String) on OBJECT | FIELD_DEFINITION

 directive @tag(name: String!) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION

+directive @extends on OBJECT | INTERFACE


Why does this ordering change? Should we try to avoid if there isn't a good reason?

See my comment made above. I explained why that happened and what is imo ok.

clenfest · 2022-10-18T13:59:29Z

internals-js/src/federation.ts

@@ -115,6 +106,7 @@ const FEDERATION_SPECIFIC_VALIDATION_RULES = [

 const FEDERATION_VALIDATION_RULES = specifiedSDLRules.filter(rule => !FEDERATION_OMITTED_VALIDATION_RULES.includes(rule)).concat(FEDERATION_SPECIFIC_VALIDATION_RULES);

+const ALL_DEFAULT_FEDERATION_DIRECTIVE_NAMES = Object.values(FederationDirectiveName) as string[];


Nit. Prefer:

const ALL_DEFAULT_FEDERATION_DIRECTIVE_NAMES: string[] = Object.values(FederationDirectiveName);

so you don't have to do any casting.

clenfest · 2022-10-18T14:07:57Z

internals-js/src/federation.ts

+  metadata: FederationMetadata,
+  errorCollector: GraphQLError[],
+) {
+  const shareableApplications: Directive<any, {}>[] = element.appliedDirectivesOf(metadata.shareableDirective());


Is there a reason you want Directive<any, {}>[] instead of Directive[]?

It was more than the former was a bit more precise than the later (making it clear there is not argument to the applications), but certainly doesn't really change anything to that method so happy to shorten it.

clenfest · 2022-10-18T14:09:45Z

internals-js/src/federation.ts

+    without: Directive<any, {}>[],
+    with: MultiMap<Extension<any>, Directive<any, {}>>,
+  };
+  const byExtensions = shareableApplications.reduce<ByExtensions>(


Oh neat! Specifying the return type of reduce in the <> is much more readable.

But allow it only if applied to different "declaration" of a type, so typically both on the main type definition as well as on some of its extensions. Fixes apollographql#2135

The `@shareable` directive was always intended to be only on object types. However, its location being `FIELD_DEFINITION`, it is syntactically allowed on interface fields, and due to an oversight of the original code, doing so had not been propertly rejected (but it had no effects whatsoever). This oversight was noticed during apollographql#2175 and a validation was added to reject a `@shareable` on interface fields. Unfortunately, it appears a number of users had started putting such `@shareable` instances, so the added validation breaks composition for those users on upgrades (the "fix" for those user is trivial, just removing `@shareable` on any interface field, it did nothing and so will have no other impact). However, sticking with the status quo is highly undesirable: continuing to allow `@shareable` on interface fields but having it do nothing whatsoever is guaranteed to create confusion, and can even be harmful if a user is falsly led to believe that it does something it does not. So this patch explores a middle-ground alternative: it gives a meaning to `@shareable` on interface fields. That meaning is to force implementations to also mark the field as `@shareable`, as this feel like the most intuitive behaviour that can be done simply. Note that this is a middle ground in the sense that a subset of the users that had started using `@shareable` on interface fields may still see composition break on upgrades, if their implementation does not "manually inherit" those `@shareable` already. But experimentally, it appears that this patch avoids breaking for almost all the cases we have access to.

`@shareable` was never meant to be applied to interface fields, but before, and since apollographql#2175, it is correctly rejected. Before that, it was syntactically accepted but was doing nothing at all, as the code was entirely ignoring. This patch document the behaviour and change.

`@shareable` was never meant to be applied to interface fields, but before, and since #2175, it is correctly rejected. Before that, it was syntactically accepted but was doing nothing at all, as the code was entirely ignoring. This patch document the behaviour and change.

pcmanus requested a review from StephenBarlow as a code owner September 30, 2022 09:06

pcmanus commented Sep 30, 2022

View reviewed changes

pcmanus force-pushed the f2135-repeatable-shareable branch from e594195 to 9117c3a Compare September 30, 2022 09:23

pcmanus self-assigned this Oct 11, 2022

clenfest approved these changes Oct 18, 2022

View reviewed changes

Sylvain Lebresne added 2 commits October 24, 2022 15:41

Allow @Shareable to be repeatable on types

0c8d38a

But allow it only if applied to different "declaration" of a type, so typically both on the main type definition as well as on some of its extensions. Fixes apollographql#2135

Additions to the documentation

ca9e2f4

pcmanus force-pushed the f2135-repeatable-shareable branch from 9117c3a to ca9e2f4 Compare October 24, 2022 13:41

pcmanus changed the base branch from main to next October 24, 2022 13:41

Changelog and review feedback

325a4f3

pcmanus merged commit bd4fae9 into apollographql:next Oct 24, 2022

This was referenced Nov 14, 2022

Loosen validation of @shareable on interface fields #2247

Closed

Shareable directive doesn't behave as expected with extended types #2135

Closed

pcmanus mentioned this pull request Nov 18, 2022

Document that @shareable on interface field is not allowed #2257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows `@shareable` to be repeatable (to allow it on both definition and extensions) #2175

Allows `@shareable` to be repeatable (to allow it on both definition and extensions) #2175

pcmanus commented Sep 30, 2022

netlify bot commented Sep 30, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 30, 2022 •

edited

Loading

pcmanus Sep 30, 2022

pcmanus Sep 30, 2022

clenfest left a comment

clenfest Oct 18, 2022

pcmanus Oct 24, 2022

clenfest Oct 18, 2022

pcmanus Oct 24, 2022

clenfest Oct 18, 2022

clenfest Oct 18, 2022

pcmanus Oct 24, 2022

clenfest Oct 18, 2022

		@@ -133,12 +133,12 @@ directive @requires(fields: _FieldSet!) on FIELD_DEFINITION

		directive @provides(fields: _FieldSet!) on FIELD_DEFINITION

		directive @extends on OBJECT \| INTERFACE

		@@ -115,6 +106,7 @@ const FEDERATION_SPECIFIC_VALIDATION_RULES = [

		const FEDERATION_VALIDATION_RULES = specifiedSDLRules.filter(rule => !FEDERATION_OMITTED_VALIDATION_RULES.includes(rule)).concat(FEDERATION_SPECIFIC_VALIDATION_RULES);

		const ALL_DEFAULT_FEDERATION_DIRECTIVE_NAMES = Object.values(FederationDirectiveName) as string[];

Allows @shareable to be repeatable (to allow it on both definition and extensions) #2175

Allows @shareable to be repeatable (to allow it on both definition and extensions) #2175

Conversation

pcmanus commented Sep 30, 2022

netlify bot commented Sep 30, 2022 • edited Loading

👷 Deploy request for apollo-federation-docs pending review.

codesandbox-ci bot commented Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clenfest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allows `@shareable` to be repeatable (to allow it on both definition and extensions) #2175

Allows `@shareable` to be repeatable (to allow it on both definition and extensions) #2175

netlify bot commented Sep 30, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 30, 2022 •

edited

Loading