-
Notifications
You must be signed in to change notification settings - Fork 193
feat(router): option to allow all error extensions #2026
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
feat(router): option to allow all error extensions #2026
Conversation
WalkthroughA new test function was introduced to validate error propagation behavior regarding allowed extension fields in GraphQL subgraph errors, covering both wrapped and passthrough modes with different configuration flags. The dependency on Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
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.
New Tests LGTM, need tests to pass though
7d0f3cf to
36e954c
Compare
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
router-tests/error_handling_test.go(1 hunks)router-tests/go.mod(1 hunks)router/core/executor.go(1 hunks)router/go.mod(1 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/fixtures/full.yaml(2 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- router/pkg/config/testdata/config_defaults.json
🚧 Files skipped from review as they are similar to previous changes (3)
- router/go.mod
- router-tests/go.mod
- router-tests/error_handling_test.go
🧰 Additional context used
🧠 Learnings (1)
router/pkg/config/config.schema.json (2)
undefined
<retrieved_learning>
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
</retrieved_learning>
<retrieved_learning>
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-router
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
router/core/executor.go (1)
84-84: Verify extension-fields precedence and mode scopeThe new
AllowAllErrorExtensionFieldsflag is correctly wired intoresolve.ResolverOptions, but since the resolver logic lives in the externalresolvepackage, please manually verify:
- Precedence: how
AllowAllErrorExtensionFieldsinteracts withAllowedErrorExtensionFieldsat runtime.- Mode applicability: should “allow all” apply in both passthrough and wrapped modes, or be restricted to passthrough?
- Tests: add or update end-to-end tests covering combinations of both flags in each propagation mode to ensure behavior matches expectations.
router/pkg/config/testdata/config_full.json (4)
750-750: Configuration change aligns with PR objective.The mode change from "wrapped" to "pass-through" is consistent with the PR's goal of disabling error extension filtering in passthrough mode.
753-753: OmitExtensions set to true is appropriate for test data.This setting ensures that the test validates the behavior when extensions are omitted, which is a valid test scenario.
756-756: New AllowAllExtensionFields configuration looks good.The new boolean flag is properly configured and aligns with the feature implementation. Setting it to
truein test data will help validate the new functionality.
758-759: AllowedExtensionFields updated for testing.The field values have been updated from
["code"]to["field1", "field2"]which appears to be more appropriate for testing the extension field filtering functionality.router/pkg/config/fixtures/full.yaml (2)
317-317: Minor stylistic change - no functional impact.The change from double quotes to single quotes for the Redis URL is purely stylistic and doesn't affect functionality.
448-459: Comprehensive subgraph error propagation configuration added.The new configuration block provides all the necessary options for controlling error propagation behavior, including the new
allow_all_extension_fieldsflag. The configuration structure is well-organized and follows YAML best practices.router/pkg/config/config.go (2)
727-737: Environment variable tag refactoring and new field addition.The environment variable tags have been simplified by removing the redundant
SUBGRAPH_ERROR_PROPAGATION_prefix from individual fields. The newAllowAllExtensionFieldsfield is properly typed with a conservative default offalse, which is appropriate for security.
1013-1013: Environment variable prefix properly scoped.The addition of
envPrefix:"SUBGRAPH_ERROR_PROPAGATION_"to the main struct ensures that environment variables are properly scoped, complementing the tag simplification in the nested struct.
ecfc4dc to
668d5ac
Compare
668d5ac to
9643c49
Compare
c67a916 to
7eff7bf
Compare
1a566c7 to
245f417
Compare
StarpTech
left a comment
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.
LGTM
245f417 to
a7d904b
Compare
360baf3 to
ca82696
Compare
Summary by CodeRabbit
Summary by CodeRabbit
Depends on wundergraph/graphql-go-tools#1217
Checklist