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.

Sign up for GitHub

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

Jump to bottom

Correctly remove useless groups which "downgrade" __typename values #2778

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Correctly remove useless groups which "downgrade" __typename values #2778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c9c0f1
reproduction
trevor-scheer Sep 11, 2023
08ff48b
implement fix, update test and comments
trevor-scheer Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/itchy-spiders-cry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
"@apollo/query-planner": patch
"@apollo/gateway": patch
---

Don't preserve useless fetches which downgrade __typename from a concrete type back to its interface type.

In certain cases, the query planner was preserving some fetches which were "useless" that would rewrite __typename from its already-resolved concrete type back to its interface type. This could result in (at least) requested fields being "filtered" from the final result due to the interface's __typename in the data where the concrete type's __typename was expected.

Specifically, the solution was compute the path between newly created groups and their parents when we know that it's trivial (`[]`). Further along in the planning process, this allows to actually remove the known-useless group.

186 changes: 179 additions & 7 deletions gateway-js/src/__tests__/executeQueryPlan.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5092,15 +5092,15 @@ describe('executeQueryPlan', () => {

const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2 ]);

let operation = parseOp(`
const operation = parseOp(`
{
ts {
v
}
}
`, schema);

let queryPlan = buildPlan(operation, queryPlanner);
const queryPlan = buildPlan(operation, queryPlanner);
const expectedPlan = `
QueryPlan {
Sequence {
Expand Down Expand Up @@ -5132,7 +5132,7 @@ describe('executeQueryPlan', () => {
`;
expect(queryPlan).toMatchInlineSnapshot(expectedPlan);

let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
const response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
expect(response.data).toMatchInlineSnapshot(`
Object {
"ts": Array [
Expand Down Expand Up @@ -5229,15 +5229,15 @@ describe('executeQueryPlan', () => {

const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2, s3 ]);

let operation = parseOp(`
const operation = parseOp(`
{
ts {
v
}
}
`, schema);

let queryPlan = buildPlan(operation, queryPlanner);
const queryPlan = buildPlan(operation, queryPlanner);
const expectedPlan = `
QueryPlan {
Sequence {
Expand Down Expand Up @@ -5286,7 +5286,7 @@ describe('executeQueryPlan', () => {
`;
expect(queryPlan).toMatchInlineSnapshot(expectedPlan);

let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
const response = await executePlan(queryPlan, operation, undefined, schema, serviceMap);
expect(response.data).toMatchInlineSnapshot(`
Object {
"ts": Array [
Expand Down Expand Up @@ -5473,7 +5473,6 @@ describe('executeQueryPlan', () => {
}
`, schema);

global.console = require('console');
queryPlan = buildPlan(operation, queryPlanner);
expect(queryPlan).toMatchInlineSnapshot(`
QueryPlan {
Expand Down Expand Up @@ -5726,6 +5725,179 @@ describe('executeQueryPlan', () => {
}
`);
});

test("useless interface fetches aren't preserved when `__typename` has been resolved to its concrete type (issue #2743)", async () => {
const store = [
{
id: '1',
tField: 'field on a T',
},
];

const S1 = {
name: 'S1',
typeDefs: gql`
extend schema
@link(
url: "https://specs.apollo.dev/federation/v2.3"
import: ["@key"]
)

interface I @key(fields: "id") {
id: ID!
}

type T implements I @key(fields: "id") {
id: ID!
tField: String
}
`,
resolvers: {
I: {
__resolveReference: ({ id }: { id: string }) =>
store.find((e) => e.id === id),
__resolveType: () => 'T',
},
},
};

const S2 = {
name: 'S2',
typeDefs: gql`
extend schema
@link(
url: "https://specs.apollo.dev/federation/v2.3"
import: ["@key", "@interfaceObject"]
)

type I @key(fields: "id") @interfaceObject {
id: ID!
iField: String!
}
`,
resolvers: {
I: {
__resolveReference: ({ id }: { id: string }) =>
store.find((e) => e.id === id),
iField: () => 'field on an I',
},
},
};

const S3 = {
name: 'S3',
typeDefs: gql`
extend schema
@link(
url: "https://specs.apollo.dev/federation/v2.3"
import: ["@key", "@interfaceObject"]
)

type I @key(fields: "id") @interfaceObject {
id: ID!
}

type Query {
getIs: [I!]!
}
`,
resolvers: {
I: {
__resolveReference: ({ id }: { id: string }) =>
store.find((e) => e.id === id),
},
Query: {
getIs: () => store,
},
},
};

const { serviceMap, schema, queryPlanner } = getFederatedTestingSchema([
S1,
S2,
S3,
]);
const operation = parseOp(
`#graphql
{
getIs {
id
... on T {
iField
}
}
}
`,
schema,
);

const queryPlan = buildPlan(operation, queryPlanner);
expect(queryPlan).toMatchInlineSnapshot(`
QueryPlan {
Sequence {
Fetch(service: "S3") {
{
getIs {
__typename
id
}
}
},
Flatten(path: "getIs.@") {
Fetch(service: "S1") {
{
... on I {
__typename
id
}
} =>
{
... on I {
__typename
}
}
},
},
Flatten(path: "getIs.@") {
Fetch(service: "S2") {
{
... on I {
__typename
id
}
} =>
{
... on I {
iField
}
}
},
},
},
}
`);

const response = await executePlan(
queryPlan,
operation,
undefined,
schema,
serviceMap,
);

expect(response).toMatchInlineSnapshot(`
Object {
"data": Object {
"getIs": Array [
Object {
"iField": "field on an I",
"id": "1",
},
],
},
}
`);
});
});

describe('fields with conflicting types needing aliasing', () => {
Expand Down
39 changes: 30 additions & 9 deletions query-planner-js/src/buildPlan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
possibleRuntimeTypes,
typesCanBeMerged,
Supergraph,
sameType,
} from "@apollo/federation-internals";
import {
advanceSimultaneousPathsWithOperation,
Expand Down Expand Up @@ -2435,13 +2436,16 @@ class FetchDependencyGraph {
}

if (group.isUseless()) {
// In general, removing a group is a bit tricky because we need to deal with the fact
// that the group can have multiple parents and children and no break the "path in parent"
// in all those cases. To keep thing relatively easily, we only handle the following
// cases (other cases will remain non-optimal, but hopefully this handle all the cases
// we care about in practice):
// 1. if the group has no children. In which case we can just remove it with no ceremony.
// 2. if the group has only a single parent and we have a path to that parent.
// In general, removing a group is a bit tricky because we need to deal
// with the fact that the group can have multiple parents, and we don't
// have the "path in parent" in all cases. To keep thing relatively
// easily, we only handle the following cases (other cases will remain
// non-optimal, but hopefully this handle all the cases we care about in
// practice):
// 1. if the group has no children. In which case we can just remove it
// with no ceremony.
// 2. if the group has only a single parent and we have a path to that
// parent.
if (group.children().length === 0) {
this.remove(group);
} else {
Expand Down Expand Up @@ -4080,7 +4084,7 @@ function computeGroupsForTree(
deferContext: updatedDeferContext
};
if (conditions) {
// We have some @requires.
// We have @requires or some other dependency to create groups for.
const requireResult = handleRequires(
dependencyGraph,
edge,
Expand Down Expand Up @@ -4495,7 +4499,24 @@ function handleRequires(
subgraphName: group.subgraphName,
mergeAt: path.inResponse(),
});
newGroup.addParents(createdGroups.map((group) => ({ group })));
newGroup.addParents(
createdGroups.map((group) => ({
group,
// Usually, computing the path of our new group into the created groups
// is not entirely trivial, but there is at least the relatively common
// case where the 2 groups we look at have:
// 1) the same `mergeAt`, and
// 2) the same parentType; in that case, we can basically infer those 2
// groups apply at the same "place" and so the "path in parent" is
// empty. TODO: it should probably be possible to generalize this by
// checking the `mergeAt` plus analyzing the selection but that
// warrants some reflection...
path: sameMergeAt(group.mergeAt, newGroup.mergeAt)
&& sameType(group.parentType, newGroup.parentType)
? []
: undefined,
})),
);
addPostRequireInputs(
dependencyGraph,
path,
Expand Down