-
Notifications
You must be signed in to change notification settings - Fork 3
Shared utilities to handle VRL expressions & some other improvements #540
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Expression type to centralize VRL expression handling, which is a great improvement for maintainability. The new type encapsulates VRL compilation and execution, removing duplicated logic from various parts of the executor. My review focuses on performance improvements for this new type, in line with the repository's style guide.
✅
|
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-0c044f54-0e2b-44ec-bfea-e82a686617ee/builder-0c044f54-0e2b-44ec-bfea-e82a686617ee0/y48xbq6zugd2ihjm2u5orc7zd",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:bddf9468aac1eed8ed60e9aafe2961dd07f839d788b19fcc1f45309bcf028c53",
"size": 1609
},
"containerimage.digest": "sha256:bddf9468aac1eed8ed60e9aafe2961dd07f839d788b19fcc1f45309bcf028c53",
"image.name": "ghcr.io/graphql-hive/router:pr-540,ghcr.io/graphql-hive/router:sha-a7217d0"
} |
| static VRL_FUNCTIONS: Lazy<Vec<Box<dyn Function>>> = Lazy::new(vrl_build_functions); | ||
| static VRL_TIMEZONE: Lazy<VrlTimeZone> = Lazy::new(VrlTimeZone::default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not do it this way, where we centralize the function building.
Yeah, right now we pass all functions that vrl provides, but if we ever want to add one made by us or filter some, for certain expressions, we would have to opt out and, we won't be able to do it without basically reverting your changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just didn't want to copy/paste the same logic again like we did 3 different places before. How do you think I should do that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a function and reuse it. If we ever want to opt-out, we either slightly refactor it or duplicate.
if you move this logic to the config crate then we’re not able to do those small adjustments in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's much better now. Thanks
If we ever want to use different set of function, we can now do:
compile_expression(expression, Some(fns))
and add pre-built functions anywhere in code, where it makes sense.
Expression type to handle VRL in one place| /// An expression that must evaluate to a boolean. If true, the label will be applied. | ||
| expression: String, | ||
| }, | ||
| Expression { expression: String }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
original comment is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted 👍
lib/executor/Cargo.toml
Outdated
| indexmap = "2.10.0" | ||
| bumpalo = "3.19.0" | ||
| once_cell = "1.21.3" | ||
| schemars = "1.0.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you sure we need schemars in executor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed 👍
2644a55 to
128b10a
Compare
128b10a to
766bd0e
Compare
4cffb36 to
c709bec
Compare
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-f7eef0ad-4ea7-46e8-ad04-461b9f8e33ef/builder-f7eef0ad-4ea7-46e8-ad04-461b9f8e33ef0/v2peugjk24d19970a7k2gnro3",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:0c9322a3636b23031dc38dca705e9acdf0fc6e4fb7ea569863c0c2853506ef6f",
"size": 1609
},
"containerimage.digest": "sha256:0c9322a3636b23031dc38dca705e9acdf0fc6e4fb7ea569863c0c2853506ef6f",
"image.name": "ghcr.io/graphql-hive/router:pr-540,ghcr.io/graphql-hive/router:sha-a9357dd"
} |
Removed `pool_idle_timeout_seconds` from `traffic_shaping` and replaced it with `pool_idle_timeout` using duration format.
046cc07 to
8650809
Compare
> [!IMPORTANT]
> Merging this pull request will create these releases
# query-planner 2.1.0 (2025-11-24)
## Features
### Directive-Based Authorization
Introducing directive-based authorization. This allows you to enforce
fine-grained access control directly from your subgraph schemas using
the `@authenticated` and `@requiresScopes` directives.
This new authorization layer runs before the query planner, ensuring
that unauthorized requests are handled efficiently without reaching your
subgraphs.
### Configuration
You can configure how the router handles unauthorized requests with two
modes:
- **`filter`** (default): Silently removes any fields the user is not
authorized to see from the query. The response will contain `null` for
the removed fields and an error in the `errors` array.
- **`reject`**: Rejects the entire GraphQL operation if it requests any
field the user is not authorized to access.
To configure this, add the following to your `router.yaml` configuration
file:
```yaml
authentication:
directives:
unauthorized:
# "filter" (default): Removes unauthorized fields from the query and returns errors.
# "reject": Rejects the entire request if any unauthorized field is requested.
mode: reject
```
If this section is omitted, the router will use `filter` mode by
default.
### JWT Scope Requirements
When using the `@requiresScopes` directive, the router expects the
user's granted scopes to be present in the JWT payload. The scopes
should be in an array of strings or a string (scopes separated by
space), within a claim named `scope`.
Here is an example of a JWT payload with the correct format:
```json
{
"sub": "user-123",
"scope": [
"read:products",
"write:reviews"
],
"iat": 1516239022
}
```
## Fixes
### Avoid extra `query` prefix for anonymous queries
When there is no variable definitions and no operation name, GraphQL
queries can be sent without the `query` prefix. For example, instead of
sending:
```diff
- query {
+ {
user(id: "1") {
name
}
}
```
# config 0.0.12 (2025-11-24)
## Features
### Breaking
Removed `pool_idle_timeout_seconds` from `traffic_shaping`, instead use
`pool_idle_timeout` with duration format.
```diff
traffic_shaping:
- pool_idle_timeout_seconds: 30
+ pool_idle_timeout: 30s
```
#540 by @ardatan
# node-addon 0.0.4 (2025-11-24)
## Fixes
### Avoid extra `query` prefix for anonymous queries
When there is no variable definitions and no operation name, GraphQL
queries can be sent without the `query` prefix. For example, instead of
sending:
```diff
- query {
+ {
user(id: "1") {
name
}
}
```
# executor 6.1.0 (2025-11-24)
## Features
### Directive-Based Authorization
Introducing directive-based authorization. This allows you to enforce
fine-grained access control directly from your subgraph schemas using
the `@authenticated` and `@requiresScopes` directives.
This new authorization layer runs before the query planner, ensuring
that unauthorized requests are handled efficiently without reaching your
subgraphs.
### Configuration
You can configure how the router handles unauthorized requests with two
modes:
- **`filter`** (default): Silently removes any fields the user is not
authorized to see from the query. The response will contain `null` for
the removed fields and an error in the `errors` array.
- **`reject`**: Rejects the entire GraphQL operation if it requests any
field the user is not authorized to access.
To configure this, add the following to your `router.yaml` configuration
file:
```yaml
authentication:
directives:
unauthorized:
# "filter" (default): Removes unauthorized fields from the query and returns errors.
# "reject": Rejects the entire request if any unauthorized field is requested.
mode: reject
```
If this section is omitted, the router will use `filter` mode by
default.
### JWT Scope Requirements
When using the `@requiresScopes` directive, the router expects the
user's granted scopes to be present in the JWT payload. The scopes
should be in an array of strings or a string (scopes separated by
space), within a claim named `scope`.
Here is an example of a JWT payload with the correct format:
```json
{
"sub": "user-123",
"scope": [
"read:products",
"write:reviews"
],
"iat": 1516239022
}
```
### Breaking
Removed `pool_idle_timeout_seconds` from `traffic_shaping`, instead use
`pool_idle_timeout` with duration format.
```diff
traffic_shaping:
- pool_idle_timeout_seconds: 30
+ pool_idle_timeout: 30s
```
#540 by @ardatan
# router 0.0.20 (2025-11-24)
## Features
- support authenticated and requiresScopes directives (#538)
### Directive-Based Authorization
Introducing directive-based authorization. This allows you to enforce
fine-grained access control directly from your subgraph schemas using
the `@authenticated` and `@requiresScopes` directives.
This new authorization layer runs before the query planner, ensuring
that unauthorized requests are handled efficiently without reaching your
subgraphs.
### Configuration
You can configure how the router handles unauthorized requests with two
modes:
- **`filter`** (default): Silently removes any fields the user is not
authorized to see from the query. The response will contain `null` for
the removed fields and an error in the `errors` array.
- **`reject`**: Rejects the entire GraphQL operation if it requests any
field the user is not authorized to access.
To configure this, add the following to your `router.yaml` configuration
file:
```yaml
authentication:
directives:
unauthorized:
# "filter" (default): Removes unauthorized fields from the query and returns errors.
# "reject": Rejects the entire request if any unauthorized field is requested.
mode: reject
```
If this section is omitted, the router will use `filter` mode by
default.
### JWT Scope Requirements
When using the `@requiresScopes` directive, the router expects the
user's granted scopes to be present in the JWT payload. The scopes
should be in an array of strings or a string (scopes separated by
space), within a claim named `scope`.
Here is an example of a JWT payload with the correct format:
```json
{
"sub": "user-123",
"scope": [
"read:products",
"write:reviews"
],
"iat": 1516239022
}
```
### Breaking
Removed `pool_idle_timeout_seconds` from `traffic_shaping`, instead use
`pool_idle_timeout` with duration format.
```diff
traffic_shaping:
- pool_idle_timeout_seconds: 30
+ pool_idle_timeout: 30s
```
#540 by @ardatan
## Fixes
### Avoid extra `query` prefix for anonymous queries
When there is no variable definitions and no operation name, GraphQL
queries can be sent without the `query` prefix. For example, instead of
sending:
```diff
- query {
+ {
user(id: "1") {
name
}
}
```
Co-authored-by: knope-bot[bot] <152252888+knope-bot[bot]@users.noreply.github.com>
Removed
pool_idle_timeout_secondsfromtraffic_shaping, instead usepool_idle_timeoutwith duration format.Related documentation changes PR -> graphql-hive/console#7207
Created some helper functions to deduplicate the code to compile and resolve VRL expressions