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.

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

[JSONSelection] Remove rest: * { a b } star selection syntax #6185

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Oct 22, 2024

This PR removes the under-documented JSONSelection StarSelection syntax.

The StarSelection syntax provided a way to capture all unselected properties of an input object, which could then be given a name by providing an alias (e.g. rest: *) or further sub-selected by appending a SubSelection (e.g. rest: * { id } to select just the ids).

Another use case for the * syntax was to make it easier to handle dynamic dictionaries in GraphQL, which does not provide a way to select dynamic object keys:

booksByISBN: books { * { title author }}

Supposing books is a dictionary whose keys are ISBN strings and whose values are objects, this selection grabs the title and author properties from each of those objects while preserving the ISBN keys and dictionary structure.

Unfortunately, this syntax has interacted poorly with other JSONSelection functionality, including features we want to build. In particular, capturing only unselected fields with the * selection makes its behavior sensitive to the removal of other NamedSelection fields, which violates guiding principle #3.

Fortunately, we have new tools to address the use cases StarSelection syntax was originally designed to solve. In particular, we can use -> methods to inspect available data and perform arbitrary processing of JSON dictionaries:

# If you need a complete list of keys/values available from
# the current input object, the ->entries method can help
keys: $->entries.key
values: $->entries.value

# Imagine ->mapValues preserves object keys while binding
# each value to @. Note: this method is still hypothetical.
booksByISBN: books->mapValues(@ { title author })

Given these alternatives, I'm confident we can remove the special * syntax without eliminating any use cases, though this is definitely a breaking change for developers who were previously using the * syntax. Please comment below if this removal is disruptive to you.

@benjamn benjamn self-assigned this Oct 22, 2024
@benjamn benjamn requested review from nicholascioli and a team as code owners October 22, 2024 19:53
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Oct 22, 2024

✅ Docs Preview Ready

No new or changed pages found.

@router-perf
Copy link

router-perf bot commented Oct 22, 2024

CI performance tests

  • connectors-const - Connectors stress test that runs with a constant number of users
  • 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

Copy link
Contributor

@lennyburdette lennyburdette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for doing this! i'm a fan of you deleting my code 😁

@benjamn benjamn merged commit 82e262b into next Oct 23, 2024
12 of 14 checks passed
@benjamn benjamn deleted the benjamn/JSONSelection-remove-StarSelection branch October 23, 2024 17:01
benjamn added a commit that referenced this pull request Oct 23, 2024
PR #6185 removed support for the following syntax for preserving dynamic
keys of dictionary objects while mapping their values:

```graphql
booksByISBN: books { * { title author }}
```

As suggested in the description of PR #6185, this pattern can be
replaced with a `->mapValues` method call:

```graphql
booksByISBN: books->mapValues(@ { title author })
```

This PR tentatively implements that `->mapValues` method, filing it in
the `future::` namespace to indicate it hasn't been shipped yet.
benjamn added a commit that referenced this pull request Oct 23, 2024
PR #6185 removed support for the following syntax for preserving dynamic
keys of dictionary objects while mapping their values:
```graphql
booksByISBN: books { * { title author }}
```

As suggested in the description of PR #6185, this pattern can be
replaced with a `->mapValues` method call:
```graphql
booksByISBN: books->mapValues(@ { title author })
```

This PR tentatively implements that `->mapValues` method, filing it away
in the `future::` namespace to indicate it hasn't been shipped yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants