Skip to content

Commit

Permalink
Don't ever add @Shareable to subscription fields during schema upgrad…
Browse files Browse the repository at this point in the history
…ing (#3094)

It could be valid to have two fields of the same that are not shareable
if one of them is being overridden. Don't add @sharable to subscription
fields and just let it be a composition error if neither are overrides.
As @Shareable is not allowed on a subscription field, they should never
exist there in any case.
  • Loading branch information
clenfest authored Jul 24, 2024
1 parent 4658fea commit 5f4bb16
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 1 deletion.
5 changes: 5 additions & 0 deletions .changeset/nice-dogs-walk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/federation-internals": patch
---

When auto-upgrading schemas from fed1, never add @shareable on subscription fields.
40 changes: 40 additions & 0 deletions internals-js/src/__tests__/schemaUpgrader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -325,3 +325,43 @@ test("fully upgrades a schema with no @link directive", () => {
}`
);
});

test("don't add @shareable to subscriptions", () => {
const subgraph1 = buildSubgraph(
"subgraph1",
"",
`#graphql
type Query {
hello: String
}
type Subscription {
update: String!
}
`
);

const subgraph2 = buildSubgraph(
"subgraph2",
"",
`#graphql
type Query {
hello: String
}
type Subscription {
update: String!
}
`
);
const subgraphs = new Subgraphs();
subgraphs.add(subgraph1);
subgraphs.add(subgraph2);
const result = upgradeSubgraphsIfNecessary(subgraphs);

expect(printSchema(result.subgraphs!.get("subgraph1")!.schema!)).not.toContain('update: String! @shareable');
expect(printSchema(result.subgraphs!.get("subgraph2")!.schema!)).not.toContain('update: String! @shareable');

expect(result.subgraphs!.get("subgraph1")!.schema.type('Subscription')?.appliedDirectivesOf('@shareable').length).toBe(0);
expect(result.subgraphs!.get("subgraph2")!.schema.type('Subscription')?.appliedDirectivesOf('@shareable').length).toBe(0);
});
5 changes: 4 additions & 1 deletion internals-js/src/schemaUpgrader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,10 @@ class SchemaUpgrader {
// - to every "value type" (in the fed1 sense of non-root type and non-entity) if it is used in any other subgraphs
// - to any (non-external) field of an entity/root-type that is not a key field and if another subgraphs resolve it (fully or partially through @provides)
for (const type of this.schema.objectTypes()) {
if (type.hasAppliedDirective(keyDirective) || type.isRootType()) {
if(type.isSubscriptionRootType()) {
continue;
}
if (type.hasAppliedDirective(keyDirective) || (type.isRootType())) {
for (const field of type.fields()) {
// To know if the field is a "key" field which doesn't need shareable, we rely on whether the field is shareable in the original
// schema (the fed1 version), because as fed1 schema will have no @shareable, the key fields will effectively be the only field
Expand Down

0 comments on commit 5f4bb16

Please sign in to comment.