[go: nahoru, domu]
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

fix(federation): test directives on fragment spreads #5785

Merged
merged 7 commits into from
Sep 2, 2024
Merged

fix(federation): test directives on fragment spreads #5785

merged 7 commits into from
Sep 2, 2024

Conversation

goto-bus-stop
Copy link
Member
@goto-bus-stop goto-bus-stop commented Aug 7, 2024 •
edited
Loading

In JS, we supported a functionality that does not make sense and is invalid per GraphQL spec. @include cannot be used on a named fragment definition. See definition of include and skip.

directive @skip(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
directive @include(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

Support for this was removed in the Rust port. This removes the test.

A related misfeature is migrating directives from a named fragment spread to an inline fragment. This happens during expansion:

directive @namedonly on FRAGMENT_SPREAD

query {
  f {
    ...named @namedonly
  }
}
fragment named on A {
  field
}

Becomes:

query {
  f {
    ... on A @namedonly {
      field
    }
  }
}

@namedonly cannot be used on INLINE_FRAGMENT, so this is invalid.
However, we do rely on this feature for @skip and @include, and potentially other directives.

As a compromise fix, this PR raises an error when a directive on a named fragment spread cannot also be used on an inline fragment. This supports @skip and @include and provides a more helpful error for unsupported cases. In the future we may consider maintaining named fragments if they have directives but this would require a major refactor.

@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Aug 7, 2024

@goto-bus-stop, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@router-perf
Copy link
router-perf bot commented Aug 7, 2024

CI performance tests

  • const - Basic stress test that runs with a constant number of users
  • demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
  • demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
  • enhanced-signature - Enhanced signature enabled
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • extended-reference-mode - Extended reference mode enabled
  • large-request - Stress test with a 1 MB request payload
  • no-tracing - Basic stress test, no tracing
  • reload - Reload test over a long period of time at a constant rate of users
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • step-local-metrics - Field stats that are generated from the router rather than FTV1
  • step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
  • step - Basic stress test that steps up the number of users over time
  • xlarge-request - Stress test with 10 MB request payload
  • xxlarge-request - Stress test with 100 MB request payload

@goto-bus-stop goto-bus-stop enabled auto-merge (squash) August 7, 2024 13:58
@goto-bus-stop goto-bus-stop marked this pull request as draft August 7, 2024 14:48
auto-merge was automatically disabled August 7, 2024 14:48

Pull request was converted to draft

@goto-bus-stop goto-bus-stop changed the title chore(federation): remove incorrect test fix(federation): test custom directives on fragment spreads Aug 26, 2024
@goto-bus-stop goto-bus-stop changed the title fix(federation): test custom directives on fragment spreads fix(federation): test directives on fragment spreads Aug 26, 2024
@goto-bus-stop goto-bus-stop marked this pull request as ready for review August 27, 2024 13:55
goto-bus-stop added a commit that referenced this pull request Aug 27, 2024
@goto-bus-stop
fix(federation): treat invalid subgraph operations as internal errors
78c9198 
Currently, if we produce an invalid subgraph operation, it looks exactly
the same as a validation error from the input query. This makes issues
harder to debug.

This PR turns validation errors at the end of planning into internal
errors, so we know where they come from, and so end users do not get
confused about what a validation error means. The new error explicitly
mentions that it's a bug in the query planner that should be reported to
Apollo.

It's not possible to add a test for this, as it only triggers for bugs.
I manually tested it with the bug that's fixed in #5785:

```
Details: Query planning produced an invalid subgraph operation.
Error: fragSpreadOnly directive is not supported for INLINE_FRAGMENT location
```
@goto-bus-stop goto-bus-stop mentioned this pull request Aug 27, 2024
Merged
dariuszkuc pushed a commit that referenced this pull request Aug 29, 2024
@goto-bus-stop @dariuszkuc
fix(federation): treat invalid subgraph operations as internal errors
5cfb62e 
Currently, if we produce an invalid subgraph operation, it looks exactly
the same as a validation error from the input query. This makes issues
harder to debug.

This PR turns validation errors at the end of planning into internal
errors, so we know where they come from, and so end users do not get
confused about what a validation error means. The new error explicitly
mentions that it's a bug in the query planner that should be reported to
Apollo.

It's not possible to add a test for this, as it only triggers for bugs.
I manually tested it with the bug that's fixed in #5785:

```
Details: Query planning produced an invalid subgraph operation.
Error: fragSpreadOnly directive is not supported for INLINE_FRAGMENT location
```
@lrlna lrlna enabled auto-merge (squash) September 2, 2024 12:23
@lrlna lrlna merged commit 863d0bc into dev Sep 2, 2024
13 checks passed
@lrlna lrlna deleted the renee/remove-incorrect-test branch September 2, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariuszkuc dariuszkuc dariuszkuc left review comments

@lrlna lrlna lrlna approved these changes

@sachindshinde sachindshinde Awaiting requested review from sachindshinde sachindshinde is a code owner

@SimonSapin SimonSapin Awaiting requested review from SimonSapin SimonSapin is a code owner

@TylerBloom TylerBloom Awaiting requested review from TylerBloom TylerBloom is a code owner

@duckki duckki Awaiting requested review from duckki duckki is a code owner

Assignees

@goto-bus-stop goto-bus-stop

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@goto-bus-stop @lrlna @dariuszkuc