feat: send to subgraphs the "fetch_reasons" extension #2160
Conversation
Subgraphs now can see why particular fields were
requested. This data is provided with request to subgraph
in the body.extensions.whoRequestedFields path. Example:
{
"query": "{accounts {__typename ... on User {some {__typename id}}}}",
"extensions": {
"whoRequestedFields": [
{
"typeName": "User",
"fieldName": "id",
"requestedBySubgraphs": ["id-2"],
"reasonIsKey": true
}, {
"typeName": "User",
"fieldName": "some",
"requestedByUser": true
}, {
"typeName": "User",
"fieldName": "id",
"requestedByUser": true
}, {
"typeName": "Query",
"fieldName": "accounts",
"requestedByUser": true
}
]
}
}
This can be enabled via configuration of router.
Disabled by default. Always ignored for introspection queries.
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughAdds a configurable “require fetch reasons” feature: new config flag, schema entries, and defaults; wires the flag into planner and resolver options; updates factory resolver to populate fetch-reason fields and refactors federation metadata layout; adds an integration test and test config template; bumps Go and graphql-go-tools versions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
Comment |
|
❌ Internal Query Planner CI failed to run. |
Router 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: 2
🧹 Nitpick comments (4)
router/core/executor.go (1)
71-89: Add an explicit test to assert “ignored for introspection” behaviorThe feature is documented as “always ignored for introspection queries.” That guarantee should be asserted at the router level to guard against regressions in graphql-go-tools.
Proposed follow-up: add an integration test that runs a pure introspection query (e.g.,
{ __schema { types { name } } }) with the flag enabled and asserts noextensions.whoRequestedFieldsis sent to any subgraph request. If helpful, I can draft the test scaffold in router-tests/integration_test.go.router/pkg/config/testdata/config_defaults.json (1)
373-375: Defaults updated coherently; request doc tweakDefault=false is consistent with a security-conscious, opt-in rollout. Please ensure the docs and sample configuration clearly call out:
- Opt-in nature (disabled by default)
- Intended audience in subgraphs
- That introspection never carries this data
I can help open a docs PR if needed.
router-tests/integration_test.go (2)
852-854: Make assertion on extensions resilient to ordering and future fields.Comparing the entire JSON string makes the test brittle (order, extra keys like requestedBySubgraphs/reasonIsKey). Decode and assert presence instead.
Apply:
- require.Equal(t, `query($a: Int!){employee(id: $a){id}}`, req.Query) - require.Equal(t, `{"whoRequestedFields":[{"typeName":"Employee","fieldName":"id","requestedByUser":true},{"typeName":"Query","fieldName":"employee","requestedByUser":true}]}`, string(req.Extensions)) + require.Equal(t, `query($a: Int!){employee(id: $a){id}}`, req.Query) + type whfEntry struct { + TypeName string `json:"typeName"` + FieldName string `json:"fieldName"` + RequestedByUser bool `json:"requestedByUser"` + } + var ext struct { + WhoRequestedFields []whfEntry `json:"whoRequestedFields"` + } + require.NoError(t, json.Unmarshal(req.Extensions, &ext)) + require.NotEmpty(t, ext.WhoRequestedFields) + require.Contains(t, ext.WhoRequestedFields, whfEntry{TypeName: "Employee", FieldName: "id", RequestedByUser: true}) + require.Contains(t, ext.WhoRequestedFields, whfEntry{TypeName: "Query", FieldName: "employee", RequestedByUser: true})
862-867: Also assert the router does not leak whoRequestedFields to clients.The feature should only affect subgraph requests. Add a quick guard on the client response.
res := xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ Query: `query ($count:Int!) { employee(id:$count) { id } }`, Variables: json.RawMessage(`{"count":1}`), }) require.JSONEq(t, `{"data":{"employee":{"id":1}}}`, res.Body) + + // Ensure the client response does not include the subgraph-only extension. + var clientResp map[string]json.RawMessage + require.NoError(t, json.Unmarshal([]byte(res.Body), &clientResp)) + if extRaw, ok := clientResp["extensions"]; ok { + var respExt map[string]any + require.NoError(t, json.Unmarshal(extRaw, &respExt)) + _, present := respExt["whoRequestedFields"] + require.False(t, present, "router must not expose whoRequestedFields to clients") + }Optionally, consider two more small tests:
- With the flag disabled (default), assert that Employees subgraph sees no whoRequestedFields in Extensions.
- For an introspection query, assert that Employees subgraph sees no whoRequestedFields, even if the flag is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
router-tests/go.mod(1 hunks)router-tests/integration_test.go(1 hunks)router/core/executor.go(1 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 (1)
📚 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
🧬 Code graph analysis (1)
router-tests/integration_test.go (2)
router-tests/testenv/testenv.go (6)
Run(105-122)Config(284-340)SubgraphsConfig(365-378)SubgraphConfig(380-384)GraphQLRequest(1903-1911)Environment(1727-1763)router/pkg/config/config.go (2)
Config(985-1059)EngineExecutionConfiguration(370-399)
⏰ 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). (12)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
router-tests/go.mod (1)
30-30: Good: graphql-go-tools bumped and aligned with routerThe upgrade to v2.0.0-rc.221.0.20250822094201-72b8aff43390 matches the router module and is necessary to wire the new ResolverOptions flag. Also, thanks for keeping mcp-go at v0.36.0 per the prior regression learning. No action needed.
router/go.mod (1)
34-34: LGTM: router pinned to the same graphql-go-tools RC as testsThis keeps the engine/resolve API in sync with the new PropagateWhoRequestedFields option. No further changes required here.
router/core/executor.go (1)
71-89: Correct wiring of PropagateWhoRequestedFields into ResolverOptionsThe new option is passed straight from RouterEngineConfig.Execution to resolve.Resolver, which is the right place for upstream subrequest shaping. Nice and minimal change surface.
router/pkg/config/testdata/config_full.json (1)
733-734: PropagateWhoRequestedFields schema and config mapping verifiedAll checks pass:
- config.schema.json exposes
"propagate_who_requested_fields"at the expected path (line 2783).- router/pkg/config/config.go defines
ensuring the correct default, environment variable, and YAML key.PropagateWhoRequestedFields bool `envDefault:"false" env:"ENGINE_PROPAGATE_WHO_REQUESTED_FIELDS" yaml:"propagate_who_requested_fields"`No further changes needed.
router/pkg/config/config.go (1)
397-399: Configuration flag end-to-end wiring and defaults verifiedAll checks pass—no further action needed.
• router/core/executor.go (line 88):
PropagateWhoRequestedFieldsis correctly passed fromopts.RouterEngineConfig.Execution.PropagateWhoRequestedFieldsinto the executor.
• router/pkg/config/config.schema.json (line 2783): the JSON schema defines"propagate_who_requested_fields".
• router/pkg/config/testdata/config_full.json (line 733) and config_defaults.json (line 373):PropagateWhoRequestedFieldsdefaults tofalse.router/pkg/config/config.schema.json (1)
2783-2787: Schema addition aligns with Go config; good.Property name, default, and description match the Go tags and intent.
…/eng-7769-fields-authorization-for-subgraph-access
ysmolski
requested review from
Aenimus,
JivusAyrus and
wilsonrivera
as code owners
ysmolski
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composition/src/v1/federation/federation-factory.ts (1)
1464-1466: Bug: condition can never be true (size < 0)interfaceDataByTypeName.size < 0 is always false; this likely intended < 1. This prevents expected incompatibility errors from surfacing.
- if (interfaceDataByTypeName.size < 0 && !unionTypeName) { + if (interfaceDataByTypeName.size < 1 && !unionTypeName) { this.errors.push(incompatibleFederatedFieldNamedTypeError(fieldCoordinates, subgraphNamesByNamedTypeName)); continue; }
🧹 Nitpick comments (9)
composition/src/subgraph/types.ts (1)
9-21: Type-safety polish: switch Map keys to TypeNameGood incremental step. Consider aligning InternalSubgraph maps to use TypeName for consistency in a follow-up.
shared/src/router-config/graphql-configuration.ts (2)
128-129: Constructor arg order change is harmless; consider consistent orderingProperty order in object literal is irrelevant; if you want codebase consistency, prefer { typeName, fieldNames } like elsewhere.
132-134: protectedFieldNames is already an Array; optional future-proof guard• In composition/src/router-configuration/types.ts (line 91),
protectedFieldNamesis explicitly declared asArray<FieldName>, so in the current codedata.protectedFieldNamescan only ever be an array.
• If you still prefer to mirror the more defensive handling used forexternalFieldNamesand guard against a hypothetical future change to aSet, you can apply the diff below.- if (data.protectedFieldNames && data.protectedFieldNames.length > 0) { - typeField.protectedFieldNames = [...data.protectedFieldNames]; - } + if (data.protectedFieldNames) { + const protectedFieldNames = Array.isArray(data.protectedFieldNames) + ? data.protectedFieldNames + : [...data.protectedFieldNames]; + if (protectedFieldNames.length > 0) { + typeField.protectedFieldNames = protectedFieldNames; + } + }composition/src/v1/utils/constants.ts (2)
101-103: Importing PROTECTED is fine; ensure full wiringYou’ve added PROTECTED to ALL_IN_BUILT_DIRECTIVE_NAMES and defined PROTECTED_DEFINITION, but it’s not present in BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME. If PROTECTED is meant to be treated as built-in (as the set suggests), wire it there too.
export const BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME = new Map<string, DirectiveDefinitionNode>([ [DEPRECATED, DEPRECATED_DEFINITION], [EXTENDS, EXTENDS_DEFINITION], [EXTERNAL, EXTERNAL_DEFINITION], + [PROTECTED, PROTECTED_DEFINITION], [EDFS_KAFKA_PUBLISH, EDFS_KAFKA_PUBLISH_DEFINITION], [EDFS_KAFKA_SUBSCRIBE, EDFS_KAFKA_SUBSCRIBE_DEFINITION], ... ]);
662-669: Nit: comment location nameGraphQL directive locations use OBJECT, not OBJECT_DEFINITION. The code is correct; the comment isn’t.
-// directive @openfed__protected repeatable on FIELD_DEFINITION | OBJECT_DEFINITION +// directive @openfed__protected repeatable on FIELD_DEFINITION | OBJECTcomposition/tests/v1/directives/protected.test.ts (1)
35-47: Make protectedFieldNames assertions order-insensitive to avoid flakinessIf protectedFieldNames is derived from a Set, insertion order across merges might vary. Prefer arrayContaining + length or sort before compare.
Example:
- expect(configurationDataByTypeName).toStrictEqual( + const cfg = configurationDataByTypeName.get(QUERY)! + expect(cfg.protectedFieldNames).toEqual(expect.arrayContaining(['a'])) + expect(cfg.protectedFieldNames?.length).toBe(1)Apply similarly for other tests that check protectedFieldNames arrays.
router-tests/integration_test.go (2)
863-868: Optional: add a “disabled flag” control.Consider a subtest with PropagateFieldsRequestedBy = false asserting that req.Extensions is empty/omits this field to gate regressions.
853-855: Use JSONEq for Robust Comparison & Confirm “FieldsRequestedBy” Is CanonicalThe integration test should compare the parsed JSON extensions semantically, and based on a repository-wide search, the only defined extension name is
FieldsRequestedBy. There are no occurrences ofwhoRequestedFieldsin code or schema, so tests, schema, and documentation should consistently useFieldsRequestedBy. If any PR description or external docs still referencewhoRequestedFields, please update them to match the canonical name.Please apply the following optional refactor and verify any external references:
• In router-tests/integration_test.go (around line 854), replace the brittle string equality check with JSONEq:
- require.Equal(t, - `{"FieldsRequestedBy":[{"__typename":"Employee","field":"id","byUser":true},{"__typename":"Query","field":"employee","byUser":true}]}`, - string(req.Extensions), - ) + require.JSONEq(t, + `{"FieldsRequestedBy":[{"__typename":"Employee","field":"id","byUser":true},{"__typename":"Query","field":"employee","byUser":true}]}`, + string(req.Extensions), + )• Confirm that any references outside the codebase (e.g. PR description, README, design docs) use
FieldsRequestedBy, notwhoRequestedFields.composition/src/router-configuration/types.ts (1)
83-95: protectedFieldNames shape — consider Set for symmetry (optional).You expose protectedFieldNames as Array while fieldNames/externalFieldNames are Sets. Keeping it a Set would preserve uniqueness guarantees at the API boundary. If array is required downstream, convert at serialization time.
- protectedFieldNames?: Array<FieldName>; + protectedFieldNames?: Set<FieldName>;If you keep Array, ensure all writers dedupe before assign.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (27)
composition/src/router-configuration/types.ts(2 hunks)composition/src/router-configuration/utils.ts(2 hunks)composition/src/schema-building/types.ts(1 hunks)composition/src/schema-building/utils.ts(1 hunks)composition/src/subgraph/types.ts(2 hunks)composition/src/utils/string-constants.ts(2 hunks)composition/src/v1/federation/federation-factory.ts(2 hunks)composition/src/v1/normalization/directive-definition-data.ts(3 hunks)composition/src/v1/normalization/normalization-factory.ts(11 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/src/v1/normalization/walkers.ts(5 hunks)composition/src/v1/utils/constants.ts(3 hunks)composition/src/v1/utils/utils.ts(0 hunks)composition/tests/v1/directives/protected.test.ts(1 hunks)composition/tests/v1/directives/shareable.test.ts(0 hunks)composition/tests/v1/utils/utils.ts(1 hunks)connect/src/wg/cosmo/node/v1/node_pb.ts(2 hunks)proto/wg/cosmo/node/v1/node.proto(1 hunks)router-tests/integration_test.go(1 hunks)router-tests/testenv/testenv.go(1 hunks)router/core/executor.go(1 hunks)router/core/factoryresolver.go(3 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)shared/src/router-config/graphql-configuration.ts(1 hunks)
💤 Files with no reviewable changes (2)
- composition/tests/v1/directives/shareable.test.ts
- composition/src/v1/utils/utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- router/pkg/config/config.go
- router/pkg/config/testdata/config_full.json
- router/pkg/config/testdata/config_defaults.json
- router/core/executor.go
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (15)
composition/src/schema-building/utils.ts (1)
composition/src/schema-building/types.ts (2)
ParentDefinitionData(240-246)InterfaceDefinitionData(156-170)
composition/src/router-configuration/utils.ts (1)
composition/src/types/types.ts (1)
FieldName(3-3)
composition/src/schema-building/types.ts (1)
composition/src/types/types.ts (1)
FieldName(3-3)
composition/src/v1/normalization/utils.ts (2)
composition/src/utils/string-constants.ts (1)
PROTECTED(106-106)composition/src/v1/normalization/directive-definition-data.ts (1)
PROTECTED_DEFINITION_DATA(444-452)
composition/tests/v1/directives/protected.test.ts (7)
composition/tests/utils/utils.ts (3)
federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)normalizeString(19-21)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/tests/v1/utils/utils.ts (3)
versionOneRouterDefinitions(106-106)schemaQueryDefinition(84-87)baseDirectiveDefinitionsWithProtected(37-45)composition/src/router-configuration/types.ts (1)
ConfigurationData(83-95)composition/src/utils/string-constants.ts (1)
QUERY(110-110)composition/src/subgraph/types.ts (1)
Subgraph(11-15)composition/src/ast/utils.ts (1)
parse(272-274)
router-tests/integration_test.go (2)
router-tests/testenv/testenv.go (7)
Run(107-124)Config(286-342)ConfigWithProtectedJSONTemplate(95-95)SubgraphsConfig(367-380)SubgraphConfig(382-386)GraphQLRequest(1905-1913)Environment(1729-1765)router/pkg/config/config.go (2)
Config(985-1059)EngineExecutionConfiguration(370-399)
composition/src/v1/federation/federation-factory.ts (1)
composition/src/types/types.ts (1)
FieldName(3-3)
composition/src/subgraph/types.ts (3)
composition/src/types/types.ts (1)
TypeName(9-9)composition/src/router-configuration/types.ts (1)
ConfigurationData(83-95)composition/src/schema-building/types.ts (1)
ParentDefinitionData(240-246)
router/core/factoryresolver.go (2)
connect/src/wg/cosmo/node/v1/node_pb.ts (2)
TypeField(1085-1135)EntityInterfaceConfiguration(1287-1325)router/gen/proto/wg/cosmo/node/v1/node.pb.go (6)
TypeField(1491-1500)TypeField(1515-1515)TypeField(1530-1532)EntityInterfaceConfiguration(1751-1758)EntityInterfaceConfiguration(1773-1773)EntityInterfaceConfiguration(1788-1790)
composition/src/v1/utils/constants.ts (2)
composition/src/utils/string-constants.ts (3)
PROTECTED(106-106)FIELD_DEFINITION_UPPER(50-50)OBJECT_UPPER(99-99)composition/src/ast/utils.ts (2)
stringArrayToNameNodeArray(84-90)stringToNameNode(77-82)
composition/src/router-configuration/types.ts (1)
composition/src/types/types.ts (2)
FieldName(3-3)TypeName(9-9)
composition/src/v1/normalization/directive-definition-data.ts (3)
composition/src/schema-building/types.ts (2)
DirectiveDefinitionData(41-50)ArgumentData(30-34)composition/src/utils/string-constants.ts (3)
FIELD_DEFINITION_UPPER(50-50)OBJECT_UPPER(99-99)PROTECTED(106-106)composition/src/v1/utils/constants.ts (1)
PROTECTED_DEFINITION(663-668)
composition/src/v1/normalization/walkers.ts (2)
composition/src/schema-building/utils.ts (1)
isInterfaceDefinitionData(261-263)composition/src/utils/string-constants.ts (2)
EXTERNAL(42-42)PROTECTED(106-106)
composition/src/v1/normalization/normalization-factory.ts (4)
composition/src/utils/string-constants.ts (2)
PROTECTED(106-106)SHAREABLE(128-128)composition/src/utils/utils.ts (1)
generateSimpleDirective(159-164)composition/src/types/types.ts (1)
FieldName(3-3)composition/src/v1/utils/constants.ts (1)
PROTECTED_DEFINITION(663-668)
shared/src/router-config/graphql-configuration.ts (3)
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
TypeField(1085-1135)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
TypeField(1491-1500)TypeField(1515-1515)TypeField(1530-1532)router/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
TypeField(1491-1500)TypeField(1515-1515)TypeField(1530-1532)
⏰ 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). (15)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_test
🔇 Additional comments (28)
proto/wg/cosmo/node/v1/node.proto (1)
144-145: LGTM: backward-compatible field addition on TypeFieldAdding
protected_field_names = 4is wire-compatible (new repeated field, new tag). Naming is consistent with existing fields.composition/src/utils/string-constants.ts (2)
106-106: LGTM: introduce PROTECTED directive constantConstant name/value matches the established naming scheme (
openfed__...).
169-170: LGTM: include PROTECTED in INHERITABLE_DIRECTIVE_NAMESThis ensures propagation where expected.
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
1101-1118: LGTM: generated TypeField.protectedFieldNames supportThe camelCase property and FieldList entry correctly mirror proto tag 4 and keep JSON/proto compatibility.
router/pkg/config/config.schema.json (1)
2783-2787: Ignore outdated naming suggestion: the extension key is consistently “FieldsRequestedBy”The codebase, configuration schema, and integration tests all use the “FieldsRequestedBy” extension key—not “whoRequestedFields.” Since the description in
config.schema.jsonalready matches the actual implementation, no change is needed.• router/pkg/config/config.schema.json (line 2786): description uses “FieldsRequestedBy”
• router/core/executor.go (line 88): populatesExtensions["FieldsRequestedBy"]
• router/pkg/config/config.go (line 397): usespropagate_fields_requested_by→ PropagateFieldsRequestedBy
• router-tests/integration_test.go (line 854): asserts{"FieldsRequestedBy":[…]}No action required.
Likely an incorrect or invalid review comment.
composition/src/router-configuration/utils.ts (2)
2-2: Type alias import aligns types across modulesImporting FieldName ensures consistency with downstream type changes. Good.
16-16: Tighten typing to SetSwitching from Set → Set improves type clarity without runtime impact. LGTM.
composition/tests/v1/utils/utils.ts (1)
37-45: Adds openfed__protected directive for tests correctlyDefinition and locations look right; repeatable on FIELD_DEFINITION | OBJECT matches usage.
router-tests/testenv/testenv.go (1)
94-96: Embedded protected config verified and in use
- The file
router-tests/testenv/testdata/configWithProtected.jsonis present.- It is valid JSON (validated via
jq type).- The new template is referenced in
router-tests/integration_test.goat line 841:
RouterConfigJSONTemplate: testenv.ConfigWithProtectedJSONTemplate,All checks have passed—no further action required.
composition/src/schema-building/utils.ts (1)
261-263: Rename to type guard is clear and correctSwitching to isInterfaceDefinitionData as a type predicate improves readability and typing. LGTM.
composition/src/v1/utils/constants.ts (1)
516-517: Including PROTECTED in built-ins is consistent with the new directiveGood call adding PROTECTED to ALL_IN_BUILT_DIRECTIVE_NAMES.
composition/src/v1/normalization/utils.ts (1)
50-51: Directive wired correctly into normalizationAdding PROTECTED_DEFINITION_DATA to initializeDirectiveDefinitionDatas and importing PROTECTED aligns normalization with the new directive. LGTM.
Also applies to: 74-75, 409-416
composition/tests/v1/directives/protected.test.ts (1)
18-338: Solid coverage of field-, type-, extension-, and multi-subgraph casesThe scenarios comprehensively assert propagation into configuration and router schema. LGTM.
router-tests/integration_test.go (1)
841-845: Flag wiring looks correct.Enabling PropagateFieldsRequestedBy via ModifyEngineExecutionConfiguration is appropriate and scoped to this test.
composition/src/v1/normalization/directive-definition-data.ts (2)
22-32: Importing PROTECTED is consistent.Bringing PROTECTED_DEFINITION/PROTECTED into scope aligns with new directive support.
444-452: Directive definition for @openfed__protected looks good.Repeatable, locations [FIELD_DEFINITION, OBJECT], no args: matches intent.
router/core/factoryresolver.go (2)
523-529: Populate ProtectedFieldNames on TypeField — LGTM.Correctly wires through for both RootNodes and ChildNodes.
Also applies to: 533-537
516-521: Remove the unusedFederationMetaDatawrapperSince there are no consumers reading from
FederationMetaData.*, you can simplify the code by dropping the wrapper and its preallocation entirely. Update the struct literal around lines 516–521 inrouter/core/factoryresolver.goas follows:- FederationMetaData: plan.FederationMetaData{ - Keys: make([]plan.FederationFieldConfiguration, 0, len(in.Keys)), - Requires: make([]plan.FederationFieldConfiguration, 0, len(in.Requires)), - Provides: make([]plan.FederationFieldConfiguration, 0, len(in.Provides)), - },And remove any related type initialization. The code that appends to
out.Keys,out.Requires, andout.Providesremains unchanged.This eliminates unnecessary duplication and keeps the implementation lean.
composition/src/router-configuration/types.ts (2)
1-2: Typed imports are appropriate.Switching to FieldName/TypeName is consistent with the rest of the model.
76-82: Strengthen typing on RequiredFieldConfiguration.fieldName: FieldName is aligned with upstream changes.
composition/src/v1/normalization/normalization-factory.ts (3)
1263-1264: Nullish coalescing for incoming directives — nice fix.Prevents replacing an explicitly empty map.
1290-1306: protectedFieldNames plumbing verifiedI’ve confirmed that
protectedFieldNamesis properly populated by the normalization walkers and exercised by existing tests:
- In
composition/src/v1/normalization/walkers.ts(line 372), the code adds toparentData.protectedFieldNameswhen a field is marked protected:if (nf.isParentObjectProtected || directivesByDirectiveName.has(PROTECTED)) { parentData.protectedFieldNames.add(fieldName); }- Comprehensive coverage exists in
composition/tests/v1/directives/protected.test.ts, which asserts correct values inprotectedFieldNamesfor various directive scenarios.No changes needed here.
563-573: It looks like thecomposition/src/v1/utils/string-constants.tsfile doesn’t currently declare anINHERITABLE_DIRECTIVE_NAMESconstant (the grep returned no matches). Sincenormalization-factory.tsrelies on that set to strip inherited directives (including@openfed__protected), please manually verify thatINHERITABLE_DIRECTIVE_NAMESis defined incomposition/src/v1/utils/string-constants.tsand includes"openfed__protected"alongside"external"and"shareable"so inherited@protecteddirectives will be removed when emitting.composition/src/v1/normalization/walkers.ts (5)
26-26: Correct utility: isInterfaceDefinitionData — LGTMAccurate switch to the narrow type guard here.
42-42: Import PROTECTED constant — LGTMImport path and identifier look correct.
529-529: State reset symmetry — LGTMGood to reset isParentObjectProtected on ObjectTypeDefinition.leave. Please confirm it’s set on enter (likely in upsertObjectDataByNode).
555-555: State reset symmetry — LGTMSame as for definitions; good to reset on ObjectTypeExtension.leave.
366-374: Review Needed: Handle or Document @openfed__protected on Interface FieldsIt looks like the current guard in
composition/src/v1/normalization/walkers.tsonly tracks@openfed__protectedon object type fields and skips interface definitions entirely. You’ll want to either support interface-level protection or clearly document/validate that interfaces cannot carry@openfed__protected.• Verify the GraphQL directive definition for
@openfed__protected—does it includeFIELD_DEFINITIONon interfaces?
• If interfaces should be protected, move the field-levelPROTECTEDcheck outside the!isInterfaceDefinitionDatablock so that interface fields get added toparentData.protectedFieldNames.
• Otherwise, add a code comment or a validation error to make it clear that@openfed__protectedon interfaces is intentionally ignored.Proposed diff sketch:
@@ composition/src/v1/normalization/walkers.ts:366 - if (!isInterfaceDefinitionData(parentData)) { + if (!isInterfaceDefinitionData(parentData)) { nf.addInheritedDirectivesToFieldData(directivesByDirectiveName, inheritedDirectiveNames); if (directivesByDirectiveName.has(EXTERNAL)) { nf.unvalidatedExternalFieldCoords.add(`${nf.originalParentTypeName}.${fieldName}`); } - if (nf.isParentObjectProtected || directivesByDirectiveName.has(PROTECTED)) { - parentData.protectedFieldNames.add(fieldName); - } + if (nf.isParentObjectProtected) { + parentData.protectedFieldNames.add(fieldName); + } } + // Always honor field-level @openfed__protected + if (directivesByDirectiveName.has(PROTECTED)) { + parentData.protectedFieldNames.add(fieldName); + }
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/pkg/config/config.go (1)
398-400: Verify schema/docs/testdata wiring and naming consistencyConfig key/env look good; please confirm:
- config.schema.json includes engine.enable_require_fetch_reasons (not under debug).
- testdata/config_defaults|full include the key with default false and an example true.
- Docs reference both YAML key and env var.
- Executor wiring reads this flag (name differs from ResolverOptions.PropagateFetchReasons; acceptable, but consider aligning naming in a follow-up if desired).
#!/bin/bash set -euo pipefail # 1) JSON schema placement rg -nC2 '"enable_require_fetch_reasons"' router/pkg/config/config.schema.json || true # 2) Env var references rg -nC3 'ENGINE_ENABLE_REQUIRE_FETCH_REASONS' || true # 3) Usage across code (executor, options) rg -nC3 'EnableRequireFetchReasons|PropagateFetchReasons' || true # 4) Testdata coverage rg -nC2 'enable_require_fetch_reasons' router/pkg/config/testdata || true # 5) Docs coverage rg -nC2 'enable_require_fetch_reasons|ENGINE_ENABLE_REQUIRE_FETCH_REASONS' docs || true # 6) Left-over old naming (to avoid confusion) rg -nC2 'PropagateFieldsRequestedBy|propagate_fields_requested_by' || true
🧹 Nitpick comments (1)
router/pkg/config/config.go (1)
398-400: Clarify the toggle with a doc commentAdd a brief comment to make intent and scope obvious at the declaration site.
- EnableRequireFetchReasons bool `envDefault:"false" env:"ENGINE_ENABLE_REQUIRE_FETCH_REASONS" yaml:"enable_require_fetch_reasons"` + // When true, the router includes `extensions.fetch_reasons` in subgraph requests + // for fields annotated with `@openfed__requireFetchReasons`. Ignored for introspection. + EnableRequireFetchReasons bool `envDefault:"false" env:"ENGINE_ENABLE_REQUIRE_FETCH_REASONS" yaml:"enable_require_fetch_reasons"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
router-tests/integration_test.go(1 hunks)router/core/executor.go(1 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- router/core/executor.go
- router/pkg/config/config.schema.json
- router-tests/integration_test.go
⏰ 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). (15)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
The PropagateFetchReasons option of Resolver enables router to send to subgraphs an extension that includes reasons why fields where included in the request. This data is sent only for the fields marked with `@openfed__requireFetchReason` in the schema. Directive is implemented in the router repo. Router part: wundergraph/cosmo#2160
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/core/factoryresolver.go (1)
567-574: Drop nested FederationMetaData; preallocate top-level slicesRemove the
FederationMetaData: plan.FederationMetaData{…}block in theplan.DataSourceMetadatainit (router/core/factoryresolver.go:516–518) and instead initializeKeys,RequiresandProvidesdirectly with capacity:out := &plan.DataSourceMetadata{ RootNodes: make([]plan.TypeField, 0, len(in.RootNodes)), ChildNodes: make([]plan.TypeField, 0, len(in.ChildNodes)), Directives: &d, Keys: make([]plan.FederationFieldConfiguration, 0, len(in.Keys)), Requires: make([]plan.FederationFieldConfiguration, 0, len(in.Requires)), Provides: make([]plan.FederationFieldConfiguration, 0, len(in.Provides)), EntityInterfaces: make([]plan.EntityInterfaceConfiguration, 0, len(in.EntityInterfaces)), InterfaceObjects: make([]plan.EntityInterfaceConfiguration, 0, len(in.InterfaceObjects)), }This drops the vestigial nested metadata, aligns with top-level usage, and avoids extra allocations.
📜 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 (4)
router-tests/go.mod(2 hunks)router/core/executor.go(2 hunks)router/core/factoryresolver.go(4 hunks)router/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- router-tests/go.mod
- router/go.mod
- router/core/executor.go
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/factoryresolver.go (3)
connect/src/wg/cosmo/node/v1/node_pb.ts (2)
TypeField(1085-1135)EntityInterfaceConfiguration(1287-1325)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (6)
TypeField(1491-1500)TypeField(1515-1515)TypeField(1530-1532)EntityInterfaceConfiguration(1751-1758)EntityInterfaceConfiguration(1773-1773)EntityInterfaceConfiguration(1788-1790)router/gen/proto/wg/cosmo/node/v1/node.pb.go (6)
TypeField(1491-1500)TypeField(1515-1515)TypeField(1530-1532)EntityInterfaceConfiguration(1751-1758)EntityInterfaceConfiguration(1773-1773)EntityInterfaceConfiguration(1788-1790)
🔇 Additional comments (4)
router/core/factoryresolver.go (4)
596-600: Approve: InterfaceObjects mapping is correctly propagatedThe new InterfaceObjects entries in factoryresolver.go feed into execution_config_usage.go—where UsesFederationInterfaceObjects is set via
len(ds.InterfaceObjects) > 0—and no other downstream consumers were found.
523-530: Propagate fetch-reasons correctly; verify introspection skip
- Gating via
EnableRequireFetchReasonsis wired through executor (executor.go:88, 240) and covered by TestEnableRequireFetchReasons (integration_test.go:835–854).- Confirm introspection queries (e.g.
__schema,__type) omitfetch_reasonsextensions when the flag is enabled or disabled, and add or update tests to cover this behavior if absent.
590-594: Verify downstream usage of EntityInterfaces mapping
Could not find any references toEntityInterfacesin planner or executor; please manually confirm that this top-level mapping is consumed as intended.
552-557: Validate graphql-go-tools version for plan.FieldCoordinate availability
go.mod pins github.com/wundergraph/graphql-go-tools/v2 at v2.0.0-rc.225; confirm that this release defines and exports plan.FieldCoordinate in pkg/engine/plan.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/core/factoryresolver.go (1)
517-522: Preallocate top-level federation slices and remove nested init
Drop the inline FederationMetaData block in router/core/factoryresolver.go (dataSourceMetaData, lines 515–522) and instead, immediately after theout := &plan.DataSourceMetadata{…}literal add:out.Keys = make([]plan.FederationFieldConfiguration, 0, len(in.Keys)) out.Requires = make([]plan.FederationFieldConfiguration, 0, len(in.Requires)) out.Provides = make([]plan.FederationFieldConfiguration, 0, len(in.Provides)) out.EntityInterfaces = make([]plan.EntityInterfaceConfiguration, 0, len(in.EntityInterfaces)) out.InterfaceObjects = make([]plan.EntityInterfaceConfiguration, 0, len(in.InterfaceObjects))This removes the confusing, unused nested FederationMetaData initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/core/factoryresolver.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/factoryresolver.go (2)
connect/src/wg/cosmo/node/v1/node_pb.ts (2)
EntityInterfaceConfiguration(1287-1325)TypeField(1085-1135)router/gen/proto/wg/cosmo/node/v1/node.pb.go (6)
EntityInterfaceConfiguration(1751-1758)EntityInterfaceConfiguration(1773-1773)EntityInterfaceConfiguration(1788-1790)TypeField(1491-1500)TypeField(1515-1515)TypeField(1530-1532)
🔇 Additional comments (3)
router/core/factoryresolver.go (3)
569-576: LGTM on populating top-level federation fieldsAppending to out.Keys/Provides/Requires/EntityInterfaces/InterfaceObjects matches the new structure. Preallocation suggestion covered above.
Also applies to: 578-583, 585-590, 592-596, 598-602
554-561: No remaining KeyConditionCoordinate references
Ripgrep search confirms there are no leftover usages of KeyConditionCoordinate in the Go code.
530-531: FetchReasonFields correctly gated and ignored on introspection
- executor uses
EnableRequireFetchReasonsto set bothPropagateFetchReasons(router/core/executor.go:88) andBuildFetchReasons(router/core/executor.go:240)- operation_processor returns before wiring extensions when
!o.introspectionEnabledandisIntrospectionis true (router/core/operation_processor.go:576–592), so fetch_reasons is never added to introspection requests
|
This PR now exludes the addition of directive (composition) and should be easy to review. |
Co-authored-by: Jesse <endigma@mailcat.ca>
ysmolski
deleted the
yury/eng-7769-fields-authorization-for-subgraph-access
branch
Subgraphs now can now receive the "fetch_reasons" extension that
lists subgraphs and reasons why fields marked with
@openfed__requireFetchReasons directive were requested.
Example of the request to subgraph:
This can be enabled via configuration of router:
Disabled by default. Always ignored for introspection queries.
Engine part: wundergraph/graphql-go-tools#1282
Composition part is merged from this PR: #2170
Summary by CodeRabbit
Checklist