Skip to content

Commit 8cdd1f0

Browse files
Revert "fix(query-planner): Fix bug where subgraph jump key fields are fetched from the wrong group" (#3300)
Reverts #3293 , as we haven't finished fully testing whether the solution is sound (and we don't want to block any pending releases).
1 parent af6cba8 commit 8cdd1f0

File tree

3 files changed

+2
-148
lines changed

3 files changed

+2
-148
lines changed

.changeset/afraid-singers-arrive.md

Lines changed: 0 additions & 5 deletions
This file was deleted.

query-planner-js/src/__tests__/buildPlan.test.ts

Lines changed: 0 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -3162,126 +3162,6 @@ describe('@requires', () => {
31623162
}
31633163
`);
31643164
});
3165-
3166-
it('avoids selecting inapplicable @key from parent fetch group', () => {
3167-
// Previously, there was an issue where a subgraph jump inserted due to a
3168-
// @requires field would try (as an optimization) to collect its locally
3169-
// satisfiable key from the parent, but the parent may not have that locally
3170-
// satisfiable key. We now explicitly check for this, falling back to the
3171-
// current node if needed (which should be guaranteed to have it).
3172-
const subgraph1 = {
3173-
name: 'A',
3174-
typeDefs: gql`
3175-
type Query {
3176-
t: T
3177-
}
3178-
type T @key(fields: "id1") {
3179-
id1: ID!
3180-
}
3181-
`,
3182-
};
3183-
3184-
const subgraph2 = {
3185-
name: 'B',
3186-
typeDefs: gql`
3187-
type T @key(fields: "id2") @key(fields: "id1") {
3188-
id1: ID!
3189-
id2: ID!
3190-
x: Int @external
3191-
req: Int @requires(fields: "x")
3192-
}
3193-
`,
3194-
};
3195-
3196-
const subgraph3 = {
3197-
name: 'C',
3198-
typeDefs: gql`
3199-
type T @key(fields: "id2") {
3200-
id2: ID!
3201-
x: Int
3202-
}
3203-
`,
3204-
};
3205-
3206-
const [api, queryPlanner] = composeAndCreatePlanner(
3207-
subgraph1,
3208-
subgraph2,
3209-
subgraph3,
3210-
);
3211-
const operation = operationFromDocument(
3212-
api,
3213-
gql`
3214-
{
3215-
t {
3216-
req
3217-
}
3218-
}
3219-
`,
3220-
);
3221-
3222-
const plan = queryPlanner.buildQueryPlan(operation);
3223-
expect(plan).toMatchInlineSnapshot(`
3224-
QueryPlan {
3225-
Sequence {
3226-
Fetch(service: "A") {
3227-
{
3228-
t {
3229-
__typename
3230-
id1
3231-
}
3232-
}
3233-
},
3234-
Flatten(path: "t") {
3235-
Fetch(service: "B") {
3236-
{
3237-
... on T {
3238-
__typename
3239-
id1
3240-
}
3241-
} =>
3242-
{
3243-
... on T {
3244-
__typename
3245-
id2
3246-
}
3247-
}
3248-
},
3249-
},
3250-
Flatten(path: "t") {
3251-
Fetch(service: "C") {
3252-
{
3253-
... on T {
3254-
__typename
3255-
id2
3256-
}
3257-
} =>
3258-
{
3259-
... on T {
3260-
x
3261-
}
3262-
}
3263-
},
3264-
},
3265-
Flatten(path: "t") {
3266-
Fetch(service: "B") {
3267-
{
3268-
... on T {
3269-
__typename
3270-
x
3271-
id2
3272-
}
3273-
} =>
3274-
{
3275-
... on T {
3276-
req
3277-
}
3278-
}
3279-
},
3280-
},
3281-
},
3282-
}
3283-
`);
3284-
});
32853165
});
32863166

32873167
describe('fetch operation names', () => {

query-planner-js/src/buildPlan.ts

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4814,34 +4814,13 @@ function createPostRequiresGroup(
48144814
// we need the path here, so this will have to do for now, and if this ever breaks in practice, we'll at least have an example to
48154815
// guide us toward improving/fixing.
48164816
assert(parent.path, `Missing path-in-parent for @requires on ${edge} with group ${group} and parent ${parent}`);
4817-
let requirePath = path.forParentOfGroup(parent.path, parent.group.parentType.schema());
4818-
let preRequireGroup = parent.group;
4819-
4820-
// The `postRequireGroup` needs a key. This can come from `group` (and code
4821-
// in `canSatisfyConditions()` guarantees such a locally satisfiable key
4822-
// exists in `group`), but it can also potentially come from `parent.group`,
4823-
// and previous code had (wrongfully) always assumed it could.
4824-
//
4825-
// To keep this previous optimization, we now explicitly check whether the
4826-
// known locally satisfiable key can be rebased in `parent.group`, and we
4827-
// fall back to `group` if it doesn't.
4828-
const keyCondition = getLocallySatisfiableKey(dependencyGraph.federatedQueryGraph, edge.head);
4829-
assert(keyCondition, () => `Due to @requires, validation should have required a key to be present for ${edge}`);
4830-
if (!keyCondition.canRebaseOn(typeAtPath(preRequireGroup.selection.parentType, requirePath.inGroup()))) {
4831-
requirePath = path;
4832-
preRequireGroup = group;
4833-
// It's possible we didn't add `group` as a parent previously, so we do so
4834-
// here similarly to how `handleRequiresTree()` specifies it.
4835-
postRequireGroup.addParent({ group, path: [] });
4836-
}
4837-
48384817
addPostRequireInputs(
48394818
dependencyGraph,
4840-
requirePath,
4819+
path.forParentOfGroup(parent.path, parent.group.parentType.schema()),
48414820
entityType,
48424821
edge,
48434822
context,
4844-
preRequireGroup,
4823+
parent.group,
48454824
postRequireGroup,
48464825
);
48474826
return {

0 commit comments

Comments
 (0)