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] Support variadic $(...) function #6456

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Dec 13, 2024

When it takes a single argument, the $(...) function (aka ExprPath syntax, introduced in #5994), is useful for embedding literal values into selection syntax.

This PR generalizes the $(...) function to take one or more arguments, where the first argument that evaluates to Some(JSON) instead of None is returned, allowing default expressions like $(possiblyEmptyList->first, {}) or $($args.possibly.missing, null). A benefit of this syntax is that it's insensitive to whether the first expression failed because $args.possibly was missing or because $args.possibly.missing was missing. The whole expression evaluated (in the apply_to_path sense) to None, for whatever reason, so the $(...) function moves on to the next option.

Passing more than two arguments is also possible, as in $(maybe.this, maybe.that, false).

Like many -> functions, the $(...) syntax evaluates its arguments lazily, so any arguments passed after the one returned will remain fully unevaluated. This makes it possible to use a (hypothetical) ->withError(message) method to report errors only when an expression is evaluated:

$(possiblyEmptyList->first, $(null)->withError("list was empty"))

It may also be worth noting that methods like ->match can evaluate to None if they do not have an infallible [@, ...] catch-all case, or if the matched value expression evaluates to None. In principle, you could use the $(...) function to provide a catch-all default, like so:

$(kind->match(
  ["book", "Book"],
  ["movie", "Film"],
), "Product")

The ->match method will normally produce a runtime ApplyToError when it exhausts all its cases, but the $(...) function hides those errors as long as one of its options evaluates to Some(JSON). If they all fail (i.e. evaluate to None), then all the original runtime ApplyToErrors will be returned.

@benjamn benjamn self-assigned this Dec 13, 2024
@benjamn benjamn requested a review from a team as a code owner December 13, 2024 21:48
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Dec 13, 2024

❌ Docs Preview Failed

Error

Error: ENOSPC: no space left on device, mkdir '/tmp/librarian/remote-sources/apollographql/router/benjamn/JSONSelection-variadic-ExprPath'

@router-perf
Copy link

router-perf bot commented Dec 13, 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

}
// As long as the expressions keep evaluating to None, keep
// accumulating errors in all_errors.
all_errors.extend(expr_errors);
Copy link
Contributor

Choose a reason for hiding this comment

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

so if all subexpressions in $(a, b, c), we'd see four errors? (one for each subexpression plus the No $(...) expression evaluated successfully)

is there value in the errors for each subexpression? seems like the last error gives you all the information you need, right?

Copy link
Member Author

@benjamn benjamn Dec 16, 2024

Choose a reason for hiding this comment

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

I think there's some value in the subexpression errors because they're the ones that will tell you which property specifically was missing, if the expression could fail in multiple ways.

Since there's a sort of hierarchical structure to the errors, maybe it doesn't make sense to lump them all into one array, but instead store the subexpression errors as children of the final error somehow? Then you'd dig into those errors only if you needed to explain the final error in more detail.

@benjamn benjamn force-pushed the benjamn/JSONSelection-ArrowMethod-refactor branch from 8f4c23d to 62775ad Compare December 16, 2024 18:23
@benjamn benjamn force-pushed the benjamn/JSONSelection-variadic-ExprPath branch from 909e897 to e5eedde Compare December 16, 2024 18:24
benjamn added a commit that referenced this pull request Dec 16, 2024
@benjamn benjamn force-pushed the benjamn/JSONSelection-variadic-ExprPath branch from e5eedde to 16d0306 Compare December 16, 2024 18:48
Base automatically changed from benjamn/JSONSelection-ArrowMethod-refactor to next December 16, 2024 19:09
@benjamn benjamn force-pushed the benjamn/JSONSelection-variadic-ExprPath branch from 16d0306 to dfeb558 Compare December 16, 2024 19:12
@apollo-bot2
Copy link

Detected SAST Vulnerabilities

🔴 Potential Security Issues Found

If you are seeing this message, it means that the security scanning tool that Apollo uses to test our source has identified a potential security issue in code introduced or updated via your branch. Information about what was detected and steps to move forward are below.

If the scanner detected a legitimate issue, please take action to correct it prior to merging this PR. The action required will vary based on the detection. If the detection is a false positive, please follow the steps below to resolve the issue.

Issues Detected

Message Caused CI Failure? Link Path Signature
Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. Yes Link apollo- router/src/protocols/websocket.rs rules.providers.semgrep.security.ja vascript.lang.security.detect- insecure-websocket
Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. Yes Link apollo- router/src/protocols/websocket.rs rules.providers.semgrep.security.ja vascript.lang.security.detect- insecure-websocket

False Positive Resolution Process

The easiest way to resolve a false positive is to add a comment containing nosemgrep to the code that triggered the detection. You can add the comment directly to the line triggering the detection or on the line immediately above it. For example:

#!/usr/bin/env python3

def my_function():
    my_code_generating_detection() # nosemgrep

What if I can't add a line comment?

If you can't add a comment because the detection is generated by a file that does not support comments (like JSON), you can use Semgrep's .semgrepignore file. For more information about how to create an exclusion via .semgrepignore, refer to the Semgrep documentation.


Getting Help

The Apollo Security team is available to assist in resolving this issue. Please tag us on this PR using @apollographql/security if you need assistance!


How do I know I fixed this correctly?

Once you have resolved the issue completely, this message will disappear! If you're still seeing this message, there is more to do prior to merging.

benjamn added a commit that referenced this pull request Dec 16, 2024
This allows us to postpone PR #6456 for further consideration.
dylan-apollo pushed a commit that referenced this pull request Dec 20, 2024
This allows us to postpone PR #6456 for further consideration.
dylan-apollo pushed a commit that referenced this pull request Dec 20, 2024
This allows us to postpone PR #6456 for further consideration.
dylan-apollo pushed a commit that referenced this pull request Dec 20, 2024
This allows us to postpone PR #6456 for further consideration.
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.

4 participants