Skip to content

Commit 534f5c2

Browse files
pcmanuspcmanus
committed
Expands over-eager merging of field fix to handle @defer consistently
The previously committed [apollographql#2713](apollographql#2713) fixed an issue introduced by [apollographql#2387](apollographql#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [apollographql#2713](apollographql#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [apollographql#2387](apollographql#2387).
1 parent 2d81036 commit 534f5c2

3 files changed

Lines changed: 348 additions & 22 deletions

File tree

‎.changeset/sixty-walls-chew.md‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
"@apollo/federation-internals": patch
3+
---
4+
5+
Expands over-eager merging of field fix to handle `@defer` consistently
6+
7+
The previously committed [#2713](https://github.com/apollographql/federation/pull/2713) fixed an issue introduced by
8+
[#2387](https://github.com/apollographql/federation/pull/2387), ensuring that querying the same field with different
9+
directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly
10+
differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments
11+
where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query:
12+
```graphq
13+
query Test($skipField: Boolean!) {
14+
x {
15+
... on X @defer {
16+
a
17+
}
18+
... on X @defer {
19+
b
20+
}
21+
}
22+
}
23+
```
24+
the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following
25+
[#2713](https://github.com/apollographql/federation/pull/2713), query:
26+
```graphq
27+
query Test($skipField: Boolean!) {
28+
x @defer {
29+
a
30+
}
31+
x @defer {
32+
b
33+
}
34+
}
35+
```
36+
_will_ merge both `x @defer`, resulting in a single deferred section.
37+
38+
This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections,
39+
consistently with both 1) the case of fragments and 2) the behaviour prior to
40+
[#2387](https://github.com/apollographql/federation/pull/2387).
41+

‎internals-js/src/__tests__/operations.test.ts‎

Lines changed: 300 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2476,6 +2476,8 @@ describe('basic operations', () => {
24762476
b1: Int
24772477
b2: T
24782478
}
2479+
2480+
directive @customSkip(if: Boolean!, label: String!) on FIELD | INLINE_FRAGMENT
24792481
`);
24802482

24812483
const operation = parseOperation(schema, `
@@ -2521,29 +2523,309 @@ describe('basic operations', () => {
25212523
]);
25222524
})
25232525

2524-
test('fields are keyed on both name and directive applications', () => {
2525-
const operation = operationFromDocument(schema, gql`
2526-
query Test($skipIf: Boolean!) {
2527-
t {
2528-
v1
2526+
describe('same field merging', () => {
2527+
test('do merge when same field and no directive', () => {
2528+
const operation = operationFromDocument(schema, gql`
2529+
query Test {
2530+
t {
2531+
v1
2532+
}
2533+
t {
2534+
v2
2535+
}
25292536
}
2530-
t @skip(if: $skipIf) {
2531-
v2
2537+
`);
2538+
2539+
expect(operation.toString()).toMatchString(`
2540+
query Test {
2541+
t {
2542+
v1
2543+
v2
2544+
}
25322545
}
2533-
}
2534-
`);
2546+
`);
2547+
});
25352548

2536-
expect(operation.toString()).toMatchString(`
2537-
query Test($skipIf: Boolean!) {
2538-
t {
2539-
v1
2549+
test('do merge when both have the _same_ directive', () => {
2550+
const operation = operationFromDocument(schema, gql`
2551+
query Test($skipIf: Boolean!) {
2552+
t @skip(if: $skipIf) {
2553+
v1
2554+
}
2555+
t @skip(if: $skipIf) {
2556+
v2
2557+
}
25402558
}
2541-
t @skip(if: $skipIf) {
2542-
v2
2559+
`);
2560+
2561+
expect(operation.toString()).toMatchString(`
2562+
query Test($skipIf: Boolean!) {
2563+
t @skip(if: $skipIf) {
2564+
v1
2565+
v2
2566+
}
25432567
}
2544-
}
2545-
`);
2546-
})
2568+
`);
2569+
});
2570+
2571+
test('do merge when both have the _same_ directive, even if argument order differs', () => {
2572+
const operation = operationFromDocument(schema, gql`
2573+
query Test($skipIf: Boolean!) {
2574+
t @customSkip(if: $skipIf, label: "foo") {
2575+
v1
2576+
}
2577+
t @customSkip(label: "foo", if: $skipIf) {
2578+
v2
2579+
}
2580+
}
2581+
`);
2582+
2583+
expect(operation.toString()).toMatchString(`
2584+
query Test($skipIf: Boolean!) {
2585+
t @customSkip(if: $skipIf, label: "foo") {
2586+
v1
2587+
v2
2588+
}
2589+
}
2590+
`);
2591+
});
2592+
2593+
test('do not merge when one has a directive and the other do not', () => {
2594+
const operation = operationFromDocument(schema, gql`
2595+
query Test($skipIf: Boolean!) {
2596+
t {
2597+
v1
2598+
}
2599+
t @skip(if: $skipIf) {
2600+
v2
2601+
}
2602+
}
2603+
`);
2604+
2605+
expect(operation.toString()).toMatchString(`
2606+
query Test($skipIf: Boolean!) {
2607+
t {
2608+
v1
2609+
}
2610+
t @skip(if: $skipIf) {
2611+
v2
2612+
}
2613+
}
2614+
`);
2615+
});
2616+
2617+
test('do not merge when both have _differing_ directives', () => {
2618+
const operation = operationFromDocument(schema, gql`
2619+
query Test($skip1: Boolean!, $skip2: Boolean!) {
2620+
t @skip(if: $skip1) {
2621+
v1
2622+
}
2623+
t @skip(if: $skip2) {
2624+
v2
2625+
}
2626+
}
2627+
`);
2628+
2629+
expect(operation.toString()).toMatchString(`
2630+
query Test($skip1: Boolean!, $skip2: Boolean!) {
2631+
t @skip(if: $skip1) {
2632+
v1
2633+
}
2634+
t @skip(if: $skip2) {
2635+
v2
2636+
}
2637+
}
2638+
`);
2639+
});
2640+
2641+
test('do not merge @defer directive, even if applied the same way', () => {
2642+
const operation = operationFromDocument(schema, gql`
2643+
query Test {
2644+
t @defer {
2645+
v1
2646+
}
2647+
t @defer {
2648+
v2
2649+
}
2650+
}
2651+
`);
2652+
2653+
expect(operation.toString()).toMatchString(`
2654+
query Test {
2655+
t @defer {
2656+
v1
2657+
}
2658+
t @defer {
2659+
v2
2660+
}
2661+
}
2662+
`);
2663+
});
2664+
});
2665+
2666+
describe('same fragment merging', () => {
2667+
test('do merge when same fragment and no directive', () => {
2668+
const operation = operationFromDocument(schema, gql`
2669+
query Test {
2670+
t {
2671+
... on T {
2672+
v1
2673+
}
2674+
... on T {
2675+
v2
2676+
}
2677+
}
2678+
}
2679+
`);
2680+
2681+
expect(operation.toString()).toMatchString(`
2682+
query Test {
2683+
t {
2684+
... on T {
2685+
v1
2686+
v2
2687+
}
2688+
}
2689+
}
2690+
`);
2691+
});
2692+
2693+
test('do merge when both have the _same_ directive', () => {
2694+
const operation = operationFromDocument(schema, gql`
2695+
query Test($skipIf: Boolean!) {
2696+
t {
2697+
... on T @skip(if: $skipIf) {
2698+
v1
2699+
}
2700+
... on T @skip(if: $skipIf) {
2701+
v2
2702+
}
2703+
}
2704+
}
2705+
`);
2706+
2707+
expect(operation.toString()).toMatchString(`
2708+
query Test($skipIf: Boolean!) {
2709+
t {
2710+
... on T @skip(if: $skipIf) {
2711+
v1
2712+
v2
2713+
}
2714+
}
2715+
}
2716+
`);
2717+
});
2718+
2719+
test('do merge when both have the _same_ directive, even if argument order differs', () => {
2720+
const operation = operationFromDocument(schema, gql`
2721+
query Test($skipIf: Boolean!) {
2722+
t {
2723+
... on T @customSkip(if: $skipIf, label: "foo") {
2724+
v1
2725+
}
2726+
... on T @customSkip(label: "foo", if: $skipIf) {
2727+
v2
2728+
}
2729+
}
2730+
}
2731+
`);
2732+
2733+
expect(operation.toString()).toMatchString(`
2734+
query Test($skipIf: Boolean!) {
2735+
t {
2736+
... on T @customSkip(if: $skipIf, label: "foo") {
2737+
v1
2738+
v2
2739+
}
2740+
}
2741+
}
2742+
`);
2743+
});
2744+
2745+
test('do not merge when one has a directive and the other do not', () => {
2746+
const operation = operationFromDocument(schema, gql`
2747+
query Test($skipIf: Boolean!) {
2748+
t {
2749+
... on T {
2750+
v1
2751+
}
2752+
... on T @skip(if: $skipIf) {
2753+
v2
2754+
}
2755+
}
2756+
}
2757+
`);
2758+
2759+
expect(operation.toString()).toMatchString(`
2760+
query Test($skipIf: Boolean!) {
2761+
t {
2762+
... on T {
2763+
v1
2764+
}
2765+
... on T @skip(if: $skipIf) {
2766+
v2
2767+
}
2768+
}
2769+
}
2770+
`);
2771+
});
2772+
2773+
test('do not merge when both have _differing_ directives', () => {
2774+
const operation = operationFromDocument(schema, gql`
2775+
query Test($skip1: Boolean!, $skip2: Boolean!) {
2776+
t {
2777+
... on T @skip(if: $skip1) {
2778+
v1
2779+
}
2780+
... on T @skip(if: $skip2) {
2781+
v2
2782+
}
2783+
}
2784+
}
2785+
`);
2786+
2787+
expect(operation.toString()).toMatchString(`
2788+
query Test($skip1: Boolean!, $skip2: Boolean!) {
2789+
t {
2790+
... on T @skip(if: $skip1) {
2791+
v1
2792+
}
2793+
... on T @skip(if: $skip2) {
2794+
v2
2795+
}
2796+
}
2797+
}
2798+
`);
2799+
});
2800+
2801+
test('do not merge @defer directive, even if applied the same way', () => {
2802+
const operation = operationFromDocument(schema, gql`
2803+
query Test {
2804+
t {
2805+
... on T @defer {
2806+
v1
2807+
}
2808+
... on T @defer {
2809+
v2
2810+
}
2811+
}
2812+
}
2813+
`);
2814+
2815+
expect(operation.toString()).toMatchString(`
2816+
query Test {
2817+
t {
2818+
... on T @defer {
2819+
v1
2820+
}
2821+
... on T @defer {
2822+
v2
2823+
}
2824+
}
2825+
}
2826+
`);
2827+
});
2828+
});
25472829
});
25482830

25492831
describe('MutableSelectionSet', () => {

0 commit comments

Comments
 (0)