feat(router): support costs on arguments of directives#2781
Merged
Conversation
…0-router-support-cost-on-the-arguments-of-directives
ysmolski
requested review from
a team,
Aenimus,
JivusAyrus,
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2781 +/- ##
=======================================
Coverage 46.28% 46.29%
=======================================
Files 1045 1045
Lines 139782 139820 +38
Branches 8768 8789 +21
=======================================
+ Hits 64702 64733 +31
- Misses 73328 73334 +6
- Partials 1752 1753 +1
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end support for directive-argument weights: type and proto schemas extended, normalization records active directive-argument weights per field, loaders/builders propagate them, tests and demo schema updated, and docs amended. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
composition/tests/v1/directives/cost.test.ts (1)
25-33: Add an explicit return type tofieldWeightand strongly typeoverrides.Line 25 defines a TS helper without an explicit return type; this weakens contract clarity for reused test builders.
As per coding guidelines: `**/*.{ts,tsx}`: Use explicit type annotations for function parameters and return types in TypeScript.🔧 Suggested typed helper
+interface FieldWeightExpectation { + typeName: TypeName; + fieldName: FieldName; + weight?: number; + argumentWeights: Map<string, number>; + directiveArgumentWeights: Map<string, number>; +} + -function fieldWeight(typeName: TypeName, fieldName: FieldName, overrides: Record<string, unknown> = {}) { +function fieldWeight( + typeName: TypeName, + fieldName: FieldName, + overrides: Partial<FieldWeightExpectation> = {}, +): FieldWeightExpectation { return { typeName, fieldName, argumentWeights: new Map(), directiveArgumentWeights: new Map(), ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/cost.test.ts` around lines 25 - 33, Add an explicit return type for the helper function fieldWeight and strongly type the overrides parameter: declare or reuse a FieldWeight interface (with properties typeName: TypeName, fieldName: FieldName, argumentWeights: Map<string, number>, directiveArgumentWeights: Map<string, number>) and change the function signature to return FieldWeight and accept overrides: Partial<FieldWeight> = {} so merged properties are correctly typed; update any imports/usages if needed to reference FieldWeight and ensure argumentWeights and directiveArgumentWeights are created as Map<string, number>.composition/src/v1/normalization/normalization-factory.ts (2)
2550-2591: Add explicit return type annotation.The method is missing an explicit
: voidreturn type annotation. As per coding guidelines, TypeScript functions should have explicit type annotations for return types.Suggested fix
recordDirectiveWeightOnField({ data, definitionData, directiveName, directiveNode, -}: RecordDirectiveWeightOnFieldParams) { +}: RecordDirectiveWeightOnFieldParams): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 2550 - 2591, The method recordDirectiveWeightOnField is missing an explicit return type; update its signature to declare a void return (e.g., recordDirectiveWeightOnField({...}: RecordDirectiveWeightOnFieldParams): void) so TypeScript typing rules are satisfied and no implicit any/return types remain—modify the method declaration where recordDirectiveWeightOnField is defined (references: RecordDirectiveWeightOnFieldParams, parentDefinitionDataByTypeName, getOrCreateFieldWeight).
2559-2561: Silent skip for interface types is intentional but undocumented.The method silently returns when the parent type is an interface. This aligns with the design that interface field costs are derived from concrete implementations (as noted in
costOnInterfaceFieldErrorMessage), but consider adding a brief comment explaining this design choice for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 2559 - 2561, Add a brief inline comment above the early return in normalization-factory.ts explaining that returning when parentTypeData is falsy or when parentTypeData.kind === Kind.INTERFACE_TYPE_DEFINITION is intentional because interface field costs are derived from their concrete implementations (see costOnInterfaceFieldErrorMessage), so the function silently skips interface types by design to avoid duplicate/ambiguous cost calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/security/cost-control.mdx`:
- Around line 164-166: The sentence "When specified **on a directive argument
definition** — adds cost to every field where the directive is applied and the
argument is active." uses an em dash contrary to docs style; update the sentence
to avoid the em dash by splitting or restructuring (for example: "When specified
on a directive argument definition, the directive adds cost to every field where
the directive is applied and the argument is active." or two sentences) in
docs-website/router/security/cost-control.mdx near the "Weights on Directive
Arguments" reference so it conforms to the repo guideline banning em dashes in
MDX docs.
In `@router-tests/security/costs_test.go`:
- Around line 368-369: The inline comment showing the "Actual-cost formula" is
incorrect and doesn't match the asserted value in the test for
core.CostActualHeader; update the explanatory math above the assertion in
router-tests/security/costs_test.go so it reflects the real calculation that
yields "37" (or remove the misleading formula entirely). Locate the comment near
the require.Equal(t, "37", res.Response.Header.Get(core.CostActualHeader))
assertion and correct the numbers/operators to match the computed actual cost or
replace the comment with a concise note referencing core.CostActualHeader and
the expected total.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 2550-2591: The method recordDirectiveWeightOnField is missing an
explicit return type; update its signature to declare a void return (e.g.,
recordDirectiveWeightOnField({...}: RecordDirectiveWeightOnFieldParams): void)
so TypeScript typing rules are satisfied and no implicit any/return types
remain—modify the method declaration where recordDirectiveWeightOnField is
defined (references: RecordDirectiveWeightOnFieldParams,
parentDefinitionDataByTypeName, getOrCreateFieldWeight).
- Around line 2559-2561: Add a brief inline comment above the early return in
normalization-factory.ts explaining that returning when parentTypeData is falsy
or when parentTypeData.kind === Kind.INTERFACE_TYPE_DEFINITION is intentional
because interface field costs are derived from their concrete implementations
(see costOnInterfaceFieldErrorMessage), so the function silently skips interface
types by design to avoid duplicate/ambiguous cost calculation.
In `@composition/tests/v1/directives/cost.test.ts`:
- Around line 25-33: Add an explicit return type for the helper function
fieldWeight and strongly type the overrides parameter: declare or reuse a
FieldWeight interface (with properties typeName: TypeName, fieldName: FieldName,
argumentWeights: Map<string, number>, directiveArgumentWeights: Map<string,
number>) and change the function signature to return FieldWeight and accept
overrides: Partial<FieldWeight> = {} so merged properties are correctly typed;
update any imports/usages if needed to reference FieldWeight and ensure
argumentWeights and directiveArgumentWeights are created as Map<string, number>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 076bd9c6-780f-4a65-b19a-5672b6ef81c6
⛔ Files ignored due to path filters (4)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router-tests/go.sumis excluded by!**/*.sumrouter/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
composition/src/router-configuration/types.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types.tscomposition/tests/v1/directives/cost.test.tsconnect/src/wg/cosmo/node/v1/node_pb.tsdemo/pkg/subgraphs/employees/subgraph/schema.graphqlsdocs-website/router/security/cost-control.mdxproto/wg/cosmo/node/v1/node.protorouter-tests/go.modrouter-tests/security/costs_test.gorouter-tests/testenv/testdata/config.jsonrouter/core/factoryresolver.gorouter/go.modshared/src/router-config/builder.ts
Aenimus
reviewed
Apr 23, 2026
Aenimus
reviewed
Apr 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
2553-2558: Add an explicit return type to the new helper.Annotating this as
: voidkeeps the change aligned with the repo’s TypeScript rules.As per coding guidelines, "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 2553 - 2558, The helper function recordDirectiveWeightOnField lacks an explicit return type; update its signature to declare a return type of void (e.g., change "recordDirectiveWeightOnField({ data, definitionData, directiveName, directiveNode, }: RecordDirectiveWeightOnFieldParams)" to include ": void") so it conforms to the project's TypeScript rules for explicit return types while keeping the existing parameter typings (RecordDirectiveWeightOnFieldParams) and body unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 2559-2563: The early return that skips recording directive
argument weights when parentTypeData.kind === Kind.INTERFACE_TYPE_DEFINITION
hides `@cost` directives on interface fields; update normalization-factory.ts so
this branch either raises the same composition error as handleCostDirective when
an `@cost` is present on an interface field (surface a clear rejection) or instead
implement the interface->max-resolution handling path before returning so
directive-derived weights are applied to concrete implementations; locate the
check using typeName and parentDefinitionDataByTypeName and modify that
conditional to perform the chosen behavior (throw the same error as
handleCostDirective or resolve interface->concrete weighting) rather than
silently returning.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 2553-2558: The helper function recordDirectiveWeightOnField lacks
an explicit return type; update its signature to declare a return type of void
(e.g., change "recordDirectiveWeightOnField({ data, definitionData,
directiveName, directiveNode, }: RecordDirectiveWeightOnFieldParams)" to include
": void") so it conforms to the project's TypeScript rules for explicit return
types while keeping the existing parameter typings
(RecordDirectiveWeightOnFieldParams) and body unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77910375-7136-494c-9e7b-0524df9921a7
📒 Files selected for processing (2)
composition-go/index.global.jscomposition/src/v1/normalization/normalization-factory.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
composition/tests/v1/directives/cost.test.ts (1)
541-556: Consider adding a test for omitted nullable argument without default.The tests cover explicit
nulland arguments with defaults, but there's no test for an omitted nullable argument without a default value. Per the implementation, this should also not record a weight.📝 Suggested additional test case
test('that a field with a directive whose nullable argument is omitted and has no default does not record directiveArgumentWeights', () => { const { costs } = normalizeSubgraphSuccess( { name: 'subgraph-omitted-nullable-arg', url: '', definitions: parse(` directive `@myDirective`(arg1: String `@cost`(weight: 5)) on FIELD_DEFINITION type Query { field: String! `@myDirective` } `), }, ROUTER_COMPATIBILITY_VERSION_ONE, ); expect(costs.fieldWeights.get('Query.field')).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/cost.test.ts` around lines 541 - 556, Add a new test that mirrors the existing null/default tests but verifies that omitting a nullable directive argument (with no default) also does not record directiveArgumentWeights: use normalizeSubgraphSuccess with a schema where directive `@myDirective`(arg1: String `@cost`(weight: 5)) is applied to Query.field without providing arg1, run with ROUTER_COMPATIBILITY_VERSION_ONE, and assert costs.fieldWeights.get('Query.field') is undefined; reference the existing test pattern and the helpers normalizeSubgraphSuccess and ROUTER_COMPATIBILITY_VERSION_ONE to place and name the test similarly (e.g., 'subgraph-omitted-nullable-arg').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@composition/tests/v1/directives/cost.test.ts`:
- Around line 541-556: Add a new test that mirrors the existing null/default
tests but verifies that omitting a nullable directive argument (with no default)
also does not record directiveArgumentWeights: use normalizeSubgraphSuccess with
a schema where directive `@myDirective`(arg1: String `@cost`(weight: 5)) is applied
to Query.field without providing arg1, run with
ROUTER_COMPATIBILITY_VERSION_ONE, and assert
costs.fieldWeights.get('Query.field') is undefined; reference the existing test
pattern and the helpers normalizeSubgraphSuccess and
ROUTER_COMPATIBILITY_VERSION_ONE to place and name the test similarly (e.g.,
'subgraph-omitted-nullable-arg').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe6204af-11c7-41a9-8a67-cdd1055f9872
📒 Files selected for processing (2)
composition/src/v1/normalization/normalization-factory.tscomposition/tests/v1/directives/cost.test.ts
Aenimus
reviewed
Apr 24, 2026
Aenimus
reviewed
Apr 24, 2026
Aenimus
reviewed
Apr 24, 2026
…0-router-support-cost-on-the-arguments-of-directives
Noroth
reviewed
Apr 29, 2026
…0-router-support-cost-on-the-arguments-of-directives
Noroth
approved these changes
Apr 29, 2026
endigma
approved these changes
Apr 29, 2026
ysmolski
deleted the
yury/eng-8700-router-support-cost-on-the-arguments-of-directives
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR supports cost weights on the arguments of the directives. When a customer applies such a directive with the specific non-null argument to a field, then its cost will be added to the cost of the field.
Composition now tracks weights for arguments of directives for each field. This is a static data so it goes into the config of a schema. It is needed for the engine to understand where directives' arguments with costs were used.
Router/engine uses it to take those weights into costs accounting.
If such directive was applied to the implementing type of some interface, and if the interface type is being used in the operation, then all the implementing types are taken into an account and maximum weight is picked among them.
I have added just a simple e2e test to verify that feature is working. More extended tests are implemented in the corresponding engine PR.
Example how cost from the directive could be added to the
employees' field of theEngineer` type:See documentation preview for more.
Summary by CodeRabbit
New Features
Documentation
Tests
Demo