feat: validate optional "requires" fields#2230
Conversation
This change enables validation of resolved input field set specified as an argument for "requires" directive. If this field set 'points' to a nullable fields marked as external, and if router resolves such a field set with null values with errors pointing to those values, then it will not provide such entities to the resolver of the field on which directive was specified.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughCentralizes router CLI invocation in the demo Makefile, updates scripted invocations, bumps several Go module dependencies, makes Employee.isAvailable nullable and adds isLeadAvailable, propagates a new ValidateRequiredExternalFields config flag into router planner/resolver, and adds integration tests and updated introspection fixtures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
…requires-dependencies
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
demo/pkg/subgraphs/employees/subgraph/schema.graphqls (1)
151-151: API change: isAvailable became nullable.This is a client‑visible nullability change. If intentional, ensure introspection snapshots and any client schemas are updated and tests cover null/error cases from availability.
router-tests/update-config.sh (1)
9-9: Harden script and simplify paths.Use bash strict mode and avoid redundant ../cli prefixes after cd.
Apply:
-echo "Generating config using 'wgc router compose'" -cd "../cli" || exit -pnpx tsx --env-file ../cli/.env ../cli/src/index.ts router compose -i ../demo/graph.yaml -o ../router-tests/testenv/testdata/configWithEdfs.json +set -euo pipefail +echo "Generating config using 'wgc router compose'" +cd "../cli" || exit +pnpx tsx --env-file ./.env ./src/index.ts router compose -i ../demo/graph.yaml -o ../router-tests/testenv/testdata/configWithEdfs.jsonAlso consider quoting paths if they may contain spaces.
router/pkg/config/config.schema.json (1)
2809-2813: Tighten description grammar for clarity.Minor copyedit to improve readability.
- "description": "Enables validation of resolved input field set specified as an argument for \"requires\" directive. If this field set 'points' to a nullable fields marked as external, and if router resolves such a field set with null values with errors pointing to those values, then it will not provide such entities to the resolver of the field on which directive was specified." + "description": "Validates the resolved input field set used by the \"@requires\" directive. If that field set targets nullable fields marked as @external and they resolve to null with errors for those values, the router will withhold such entities from the resolver of the field that declared @requires."demo/Makefile (2)
1-2: Nice wrapper; fix PHONY declaration and include new targets.Use .PHONY and add compose/standalone/plugin targets to avoid file/dir collisions.
-PHONY: plugin-build plugin-generate plugin-build-binary plugin-build-integration +.PHONY: generate compose-integration \ + plugin-build plugin-generate plugin-build-binary plugin-build-integration plugin-compose plugin-build-ci \ + standalone-compose standalone-integration bump-deps
7-9: compose-integration coupling.If tests require both configs, consider adding a twin target that calls update-config.sh as well, or parameterize the script.
demo/pkg/subgraphs/availability/subgraph/schema.resolvers.go (1)
35-35: Minor: avoid taking address of parameter directly.Slightly clearer to copy the param before taking address.
- return &model.Employee{ID: employeeID, IsAvailable: &isAvailable}, nil + v := isAvailable + return &model.Employee{ID: employeeID, IsAvailable: &v}, nildemo/pkg/subgraphs/employees/subgraph/model/models_gen.go (1)
107-107: *Employee.IsAvailable switched to bool — verify all resolvers set pointers.Ensure resolvers return &val (not value) and handle nil safely in any downstream logic.
router-tests/integration_test.go (4)
894-896: Make variables assertion JSON-order agnostic.Use JSONEq when variables are non-empty to avoid flaky map-key order issues.
- require.Equal(t, expectedVars[req.Query], string(req.Variables)) + exp := expectedVars[req.Query] + if exp == "" { + require.Equal(t, exp, string(req.Variables)) + } else { + require.JSONEq(t, exp, string(req.Variables)) + }
943-945: Apply the same JSONEq fix in the “enabled” subtest.- require.Equal(t, expectedVars[req.Query], string(req.Variables)) + exp := expectedVars[req.Query] + if exp == "" { + require.Equal(t, exp, string(req.Variables)) + } else { + require.JSONEq(t, exp, string(req.Variables)) + }
985-993: Relax brittle full-body equality on error message.Exact error strings tend to drift. Assert key substrings instead (or parse and assert fields) to reduce brittleness.
- require.JSONEq(t, `{ - "data":{ - "products":[ - {"isLeadAvailable":null,"upc":"consultancy"}, - {"isLeadAvailable":false,"upc":"cosmo"}, - {}]}, - "errors":[{"message":"Failed to fetch from Subgraph 'availability' at Path 'products.@.lead', Reason: failed to obtain field dependencies.","extensions":{"statusCode":200}}] - }`, res.Body) + require.Contains(t, res.Body, `"upc":"consultancy"`) + require.Contains(t, res.Body, `"isLeadAvailable":null`) + require.Contains(t, res.Body, `"upc":"cosmo"`) + require.Contains(t, res.Body, `"isLeadAvailable":false`) + require.Contains(t, res.Body, `"Failed to fetch from Subgraph 'availability'`)
919-994: Add a control case: nullable external without error should NOT be filtered.To fully cover the contract, add a subtest where Availability returns null without an error on the path; expect both representations to be forwarded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
demo/go.sumis excluded by!**/*.sumdemo/pkg/subgraphs/availability/subgraph/generated/generated.gois excluded by!**/generated/**demo/pkg/subgraphs/employees/subgraph/generated/federation.gois excluded by!**/generated/**demo/pkg/subgraphs/employees/subgraph/generated/federation.requires.gois excluded by!**/generated/**demo/pkg/subgraphs/employees/subgraph/generated/generated.gois excluded by!**/generated/**router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
demo/Makefile(2 hunks)demo/go.mod(3 hunks)demo/pkg/subgraphs/availability/subgraph/entity.resolvers.go(1 hunks)demo/pkg/subgraphs/availability/subgraph/model/models_gen.go(1 hunks)demo/pkg/subgraphs/availability/subgraph/schema.graphqls(1 hunks)demo/pkg/subgraphs/availability/subgraph/schema.resolvers.go(1 hunks)demo/pkg/subgraphs/employees/subgraph/model/models_gen.go(2 hunks)demo/pkg/subgraphs/employees/subgraph/schema.graphqls(2 hunks)router-tests/go.mod(1 hunks)router-tests/integration_test.go(1 hunks)router-tests/testdata/introspection/base-graph-schema.json(2 hunks)router-tests/testdata/introspection/feature-graph-schema.json(2 hunks)router-tests/update-config-no-edg.sh(1 hunks)router-tests/update-config.sh(1 hunks)router/core/executor.go(2 hunks)router/go.mod(1 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/go.modrouter-tests/go.mod
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/pkg/config/config.schema.json
🧬 Code graph analysis (4)
router-tests/integration_test.go (2)
router-tests/testenv/testenv.go (7)
Run(107-124)Config(286-342)ConfigJSONTemplate(81-81)SubgraphsConfig(367-380)SubgraphConfig(382-386)GraphQLRequest(1905-1913)Environment(1729-1765)router/pkg/config/config.go (2)
Config(988-1062)EngineExecutionConfiguration(371-401)
demo/pkg/subgraphs/availability/subgraph/schema.resolvers.go (2)
demo/pkg/subgraphs/availability/subgraph/model/models_gen.go (2)
Employee(5-8)Employee(10-10)demo/pkg/subgraphs/employees/subgraph/model/models_gen.go (3)
Employee(97-110)Employee(112-112)Employee(115-115)
demo/pkg/subgraphs/employees/subgraph/model/models_gen.go (1)
demo/pkg/subgraphs/availability/subgraph/model/models_gen.go (2)
Employee(5-8)Employee(10-10)
demo/pkg/subgraphs/availability/subgraph/entity.resolvers.go (2)
demo/pkg/subgraphs/availability/subgraph/model/models_gen.go (2)
Employee(5-8)Employee(10-10)demo/pkg/subgraphs/employees/subgraph/model/models_gen.go (3)
Employee(97-110)Employee(112-112)Employee(115-115)
🪛 checkmake (0.2.2)
demo/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 13-13: Target "PHONY" should be declared PHONY.
(phonydeclared)
⏰ 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). (6)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
🔇 Additional comments (22)
router-tests/go.mod (1)
30-30: LGTM!The update to
graphql-go-tools/v2pseudo-version aligns with the router module and supports the new ValidateRequiredExternalFields functionality being introduced in this PR.demo/go.mod (1)
32-32: LGTM! Standard indirect dependency additions.These new indirect dependencies (
bahlo/generic-list-go,invopop/jsonschema,mailru/easyjson,wk8/go-ordered-map/v2) are likely pulled in by the updated graphql-go-tools dependency and are appropriately marked as indirect.Also applies to: 89-89, 97-97, 148-148
router/go.mod (1)
34-34: LGTM!The pseudo-version update to
graphql-go-tools/v2properly aligns with other modules in this PR and likely includes the external field validation functionality being introduced.router-tests/update-config-no-edg.sh (1)
8-8: LGTM! Centralized CLI invocation approach.The change from the legacy
cd ../cli && pnpm wgcapproach to usingpnpx tsxwith explicit environment file sourcing improves consistency and maintainability across the build system.demo/pkg/subgraphs/availability/subgraph/schema.graphqls (1)
10-10: LGTM! Schema change supports optional external fields validation.The change from
isAvailable: Boolean!toisAvailable: Boolean(making it nullable) is consistent with the PR's objective to validate optional required fields. This aligns with the corresponding Go model changes that updatedIsAvailablefromboolto*bool.demo/pkg/subgraphs/availability/subgraph/model/models_gen.go (1)
6-7: LGTM! Model properly aligned with nullable GraphQL schema.The change from
IsAvailable booltoIsAvailable *boolwithomitemptycorrectly reflects the schema change fromBoolean!toBooleanin the GraphQL schema. This supports the PR's objective for validating optional external fields in Federation directives.router/pkg/config/config.go (1)
400-400: LGTM! Configuration field properly defined.The
ValidateRequiredExternalFieldsfield is correctly added toEngineExecutionConfigurationwith appropriate:
- Default value (
false) for backward compatibility- Environment variable binding (
ENGINE_VALIDATE_REQUIRED_EXTERNAL_FIELDS)- YAML mapping (
validate_required_external_fields)This field will enable validation of external fields referenced in GraphQL Federation
@requiresdirectives as described in the PR objectives.demo/pkg/subgraphs/employees/subgraph/schema.graphqls (1)
183-184: New derived field requires external dependency; confirm resolver behavior.isLeadAvailable depends on lead.isAvailable via @requires. Verify generated resolvers/models return the correct value and handle null lead.isAvailable consistently.
router-tests/update-config.sh (1)
9-9: Confirm tsx supports --env-file on your pinned version.Some environments use older tsx without --env-file. Please verify locally.
demo/Makefile (3)
16-20: CLI wrapper usage LGTM.
25-26: LGTM on compose + format.
39-41: LGTM on standalone compose.demo/pkg/subgraphs/availability/subgraph/entity.resolvers.go (1)
17-17: Pointer conversion is correct.Returning &availability (local var) is safe; it escapes to heap. Matches nullable schema.
router/pkg/config/testdata/config_defaults.json (1)
379-381: Approve — end-to-end wiring verified.Struct/env binding (ENGINE_VALIDATE_REQUIRED_EXTERNAL_FIELDS / yaml: validate_required_external_fields) present in router/pkg/config/config.go; schema default in router/pkg/config/config.schema.json; executor/planner consume the flag (router/core/executor.go); defaults and tests updated in router/pkg/config/testdata/* and router-tests/integration_test.go. No further changes required.
router-tests/testdata/introspection/feature-graph-schema.json (2)
15912-15923: Cosmo.isLeadAvailable — @requires present in @key types; plain declarations missing @requires; Employee.isAvailable nullability unverified
- Confirmed: isLeadAvailable has @requires(fields: "lead { isAvailable }") in @key/@implementing declarations (matches in your output at lines 176 and 183).
- Inconsistent: plain type declaration(s) show isLeadAvailable without @requires (match at line 142). Align these (add @requires where the computed field is defined or remove the duplicate plain declaration).
- Unverified: Employee.isAvailable nullability not checked — verify it's nullable in every contributing subgraph (search for "type Employee" and "isAvailable" and confirm there is no trailing '!').
15736-15739: Employee.isAvailable made nullable — verify downstream expectationsChange from NON_NULL to SCALAR Boolean looks intentional for optional requires. Ensure:
- Generated Go model uses *bool and resolvers handle nil safely.
- Any tests/fixtures expecting NON_NULL are updated (including base-graph-schema.json).
Run to double-check repo consistency:
router-tests/testdata/introspection/base-graph-schema.json (2)
15752-15755: Employee.isAvailable made nullable — verified & approvedBoth router-tests/testdata/introspection/base-graph-schema.json and router-tests/testdata/introspection/feature-graph-schema.json show Employee.isAvailable as a nullable Boolean; change matches the new validation for optional externals. Update any tests or schema snapshots that assumed Boolean!.
15912-15923: Cosmo.isLeadAvailable added — consistent with Consultancy and requires-chainVerified: both router-tests/testdata/introspection/base-graph-schema.json and router-tests/testdata/introspection/feature-graph-schema.json contain Cosmo.isLeadAvailable and Consultancy.isLeadAvailable as SCALAR Boolean; federation wiring appears in sync.
demo/pkg/subgraphs/employees/subgraph/model/models_gen.go (1)
53-56: Cosmo.IsLeadAvailable pointer looks correct and schema-aligned.Matches the new nullable field; JSON tag is consistent with other models.
router/core/executor.go (2)
89-90: Flag is correctly wired into ResolverOptions.No issues spotted.
241-243: Planner config ORs BuildFetchReasons with ValidateRequiredExternalFields — good separation from propagation.Propagation remains gated by EnableRequireFetchReasons, which is fine; just confirm this split is intentional.
router/pkg/config/testdata/config_full.json (1)
740-742: Engine flag added correctly — confirm schema/testdata naming consistencyStruct, schema and wiring are present, but testdata uses CamelCase while schema/yaml tags use snake_case — confirm the loader/validator accepts the CamelCase JSON key.
- Go struct (router/pkg/config/config.go:400): ValidateRequiredExternalFields bool
envDefault:"false" env:"ENGINE_VALIDATE_REQUIRED_EXTERNAL_FIELDS" yaml:"validate_required_external_fields".- Schema (router/pkg/config/config.schema.json): property "validate_required_external_fields" exists.
- Testdata (router/pkg/config/testdata/config_full.json, config_defaults.json): key "ValidateRequiredExternalFields" (CamelCase).
- Wiring (router/core/executor.go): the field is read and used (sets planConfig.BuildFetchReasons and planConfig.ValidateRequiredExternalFields).
Confirm the config loader/schema validator accepts the CamelCase key; if not, align names (update schema to include the CamelCase alias or change testdata to snake_case).
…requires-dependencies
…7-validate-presence-of-optional-requires-dependencies
ysmolski
changed the title
ysmolski
deleted the
yury/eng-7767-validate-presence-of-optional-requires-dependencies
branch
2 participants
This change enables validation of resolved input field set
specified as an argument for "requires" directive.
If this field set 'points' to a nullable fields marked as external,
and if router resolves such a field set with null values
with errors pointing to those values,
then it will not provide to the resolver entities that includes the field on
which the "requires" directive was specified.
Use the
validate_required_external_fieldsoption to enable the feature.Depends on wundergraph/graphql-go-tools#1297
Summary by CodeRabbit
New Features
Tests
Chores