-
Notifications
You must be signed in to change notification settings - Fork 25
fix(demand-control): support proxy mode
#1608
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds support for proxy mode to the demand control functionality, expanding test coverage to ensure the demand control plugin works in both proxy and supergraph gateway modes.
- Refactored test setup to support testing both
proxyandsupergraphmodes - Added null guard for
subgraphparameter inuseDemandControlplugin - Updated test assertions to handle formatting differences between modes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/runtime/tests/demand-control.test.ts | Refactored tests to run in both proxy and supergraph modes, extracted common gateway creation logic |
| packages/runtime/src/plugins/useDemandControl.ts | Added null check for subgraph parameter to support proxy mode where subgraph may be undefined |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/gateway |
2.1.11-alpha-6307c1f7db5f80db9bac8da7831a2aef3c9dedcc |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
2.0.16-alpha-6307c1f7db5f80db9bac8da7831a2aef3c9dedcc |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
2.0.11-alpha-6307c1f7db5f80db9bac8da7831a2aef3c9dedcc |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-opentelemetry |
1.0.13-alpha-6307c1f7db5f80db9bac8da7831a2aef3c9dedcc |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
2.0.14-alpha-6307c1f7db5f80db9bac8da7831a2aef3c9dedcc |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
2.1.10-alpha-6307c1f7db5f80db9bac8da7831a2aef3c9dedcc |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
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.
There seems to be no test that verifies that maxCost option works in proxy mode. From looking at the code this would not work.
There is also no documentation on the caveats in proxy mode, e.g. "User has to provide the schema on their own using schema option.", I don't understand what this means.
fde8802 to
6307c1f
Compare
|
I updated @n1ru4l the description to clarify the situation. |
Ref GW-489
In proxy mode, if there is no
schemaprovided in the configuration, the gateway sends an introspection query to the endpoint to fetch the schema.This introspection query is made through
onSubgraphExecutebecause technically it is still a GraphQL request going outside.Demand Control plugin uses
subgraphinonSubgraphExecute's payload, and it fails because it is missing at this point(it hasn't been fetched and set yet). This is the first issue which has been fixed by checkingsubgraphinonSubgraphExecutehook here; https://github.com/graphql-hive/gateway/pull/1608/files#diff-c101c1adf0b03c53dc73424f7f837be04f5c98798305c2d3ac0504c230752df1R73But that opens a new discussion about the typing of
subgraphinonSubgraphExecute. Should it beoptionalinstead of beingrequired?The introspection result doesn't have directive definitions, so
@costand@listSizedefinitions are missing on the fields, types and arguments. So the only option to benefit from directives is to pass aschemato the configuration as we do in tests; https://github.com/graphql-hive/gateway/pull/1608/files#diff-0892aaa1d262ea809386f7653bbb2b01ef5bd02b1e3dbb1d3b294a36e31ce137R32Other than that, all other options of the demand control plugin work as expected.
The next steps should be
subgraphoptional in the type of the payload ofonSubgraphExecutehook.proxymode with directives@auth) and@rateLimitetc.