fix(composition): apply subscription filters on union and interface return types#2797
fix(composition): apply subscription filters on union and interface return types#2797jensneuse wants to merge 40 commits into
Conversation
…eturn types @openfed__subscriptionFilter was silently dropped during composition whenever the subscription field returned a union or an interface (federation-factory.ts gated on Kind.OBJECT_TYPE_DEFINITION and skipped without an error). The router then forwarded every event because no filter config was emitted, leaking data across tenants in customer subscriptions that fan out a union of event types. Walk every accessible concrete type for the abstract return type using the existing concreteTypeNamesByAbstractTypeName index, validate the condition against each, and emit one filter config when all walks succeed. Inaccessible members and implementers are skipped before any field-presence check. If any accessible target is missing the path or otherwise fails validation, surface a member-tagged composition error instead of silently dropping the filter. Adds composition unit tests for union and interface success, per-target error paths with exact-shape assertions, and an inaccessible-member skip case. Adds router-tests integration coverage that exercises both members of a union and both implementers of an interface end-to-end through Kafka, plus the case where an event with a __typename outside the abstract type produces an INVALID_GRAPHQL error frame instead of silently leaking. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
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 target-aware validation for ChangesSubscription-filter target-aware validation and supporting artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (74.51%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
===========================================
- Coverage 65.01% 46.35% -18.66%
===========================================
Files 573 1084 +511
Lines 71938 145677 +73739
Branches 4862 9335 +4473
===========================================
+ Hits 46767 67535 +20768
- Misses 23724 76396 +52672
- Partials 1447 1746 +299
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Required by the composition-go git-dirty-check CI step: every change in the composition TypeScript package must be reflected in composition-go/index.global.js. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
composition/tests/v1/directives/subscription-filter.test.ts (1)
459-500: Assert that only one matching config is emitted.
find(...)only proves the config exists once in the assertion path. If composition starts emitting the same subscription filter twice, these tests would still pass. Tighten the check to assert the filtered config list has exactly one entry.Suggested assertion shape
- const subscriptionField = result.fieldConfigurations.find( - (fc) => fc.typeName === SUBSCRIPTION && fc.fieldName === 'onTaskEvent', - ); - expect(subscriptionField).toStrictEqual({ + const subscriptionFields = result.fieldConfigurations.filter( + (fc) => fc.typeName === SUBSCRIPTION && fc.fieldName === 'onTaskEvent', + ); + expect(subscriptionFields).toStrictEqual([{ argumentNames: ['phoneChannelId'], fieldName: 'onTaskEvent', typeName: SUBSCRIPTION, subscriptionFilterCondition: { in: { fieldPath: ['phoneChannelId'], values: ['{{ args.phoneChannelId }}'], }, }, - }); + }]);Also applies to: 481-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/subscription-filter.test.ts` around lines 459 - 500, The test currently uses find(...) to locate the subscription field (subscriptionField) which only proves existence; change the lookup to filter(...) on result.fieldConfigurations for entries where fc.typeName === SUBSCRIPTION && fc.fieldName === 'onTaskEvent', then assert the filtered array has length 1 and assert the single element equals the expected config; update both tests referencing subscriptionField (the one for union and the one for interface) to use this filtered-list + length === 1 check before the strict equality assertion.router-tests/events/kafka_events_test.go (1)
1087-1092: Usetestenv.WSWriteJSONfor the new subscription setup writes.These new blocks still call
conn.WriteJSON, which skips the retry/backoff behavior the router-test helpers provide. Switching totestenv.WSWriteJSONwill make the union/interface subscription tests less timing-sensitive.Suggested replacement
- require.NoError(t, conn.WriteJSON(&testenv.WebSocketMessage{ + require.NoError(t, testenv.WSWriteJSON(t, conn, &testenv.WebSocketMessage{Also applies to: 1161-1166, 1234-1239, 1294-1299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/events/kafka_events_test.go` around lines 1087 - 1092, Replace direct conn.WriteJSON calls with the test helper testenv.WSWriteJSON so the retry/backoff logic is used for subscription setup; specifically, in the blocks that create the WebSocket via xEnv.InitGraphQLWebSocketConnection and then call conn.WriteJSON(&testenv.WebSocketMessage{...}) (e.g., the instance at the shown diff and the other occurrences at the ranges noted) switch those calls to testenv.WSWriteJSON(conn, t, &testenv.WebSocketMessage{...}) (keeping the same ID, Type and Payload fields) so the union/interface subscription tests use the router-test helper's timing resilience.composition/src/v1/federation/federation-factory.ts (2)
2809-2816: Add an explicitvoidreturn type to this helper.This new method relies on inference even though the repo asks for explicit TypeScript return types.
Suggested change
validateSubscriptionFilterAndGenerateConfiguration( directiveNode: ConstDirectiveNode, objectData: ObjectDefinitionData, fieldPath: string, fieldName: string, parentTypeName: string, directiveSubgraphName: string, - ) { + ): void {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/federation/federation-factory.ts` around lines 2809 - 2816, The helper method validateSubscriptionFilterAndGenerateConfiguration currently relies on inferred return type; update its signature to include an explicit TypeScript return annotation of void (i.e., add ": void" after the parameter list) so it complies with the project's rule for explicit return types, and confirm there are no returned values inside the body (or remove/adjust any return statements) to match the void annotation; reference the function validateSubscriptionFilterAndGenerateConfiguration and its parameter types (ConstDirectiveNode, ObjectDefinitionData) when making the change.
2836-2849: Consider returning concrete targets in a stable order.
concreteTypeNamesByAbstractTypeNameis aSet, so the aggregated error order here depends on insertion order. Sortingoutbytarget.namebefore returning would make member-tagged errors and the chosen first success deterministic across runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/federation/federation-factory.ts` around lines 2836 - 2849, collectSubscriptionFilterConcreteTargets currently iterates a Set (concreteTypeNamesByAbstractTypeName) and returns results in insertion order; make the return order deterministic by sorting the collected ObjectDefinitionData array (out) by the concrete type name before returning. Locate collectSubscriptionFilterConcreteTargets and, after populating out, sort it by the type name field on each ObjectDefinitionData (use the name property on the returned data) so member-tagged errors and first-success selection are stable across runs.
🤖 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/src/v1/federation/federation-factory.ts`:
- Around line 2809-2816: The helper method
validateSubscriptionFilterAndGenerateConfiguration currently relies on inferred
return type; update its signature to include an explicit TypeScript return
annotation of void (i.e., add ": void" after the parameter list) so it complies
with the project's rule for explicit return types, and confirm there are no
returned values inside the body (or remove/adjust any return statements) to
match the void annotation; reference the function
validateSubscriptionFilterAndGenerateConfiguration and its parameter types
(ConstDirectiveNode, ObjectDefinitionData) when making the change.
- Around line 2836-2849: collectSubscriptionFilterConcreteTargets currently
iterates a Set (concreteTypeNamesByAbstractTypeName) and returns results in
insertion order; make the return order deterministic by sorting the collected
ObjectDefinitionData array (out) by the concrete type name before returning.
Locate collectSubscriptionFilterConcreteTargets and, after populating out, sort
it by the type name field on each ObjectDefinitionData (use the name property on
the returned data) so member-tagged errors and first-success selection are
stable across runs.
In `@composition/tests/v1/directives/subscription-filter.test.ts`:
- Around line 459-500: The test currently uses find(...) to locate the
subscription field (subscriptionField) which only proves existence; change the
lookup to filter(...) on result.fieldConfigurations for entries where
fc.typeName === SUBSCRIPTION && fc.fieldName === 'onTaskEvent', then assert the
filtered array has length 1 and assert the single element equals the expected
config; update both tests referencing subscriptionField (the one for union and
the one for interface) to use this filtered-list + length === 1 check before the
strict equality assertion.
In `@router-tests/events/kafka_events_test.go`:
- Around line 1087-1092: Replace direct conn.WriteJSON calls with the test
helper testenv.WSWriteJSON so the retry/backoff logic is used for subscription
setup; specifically, in the blocks that create the WebSocket via
xEnv.InitGraphQLWebSocketConnection and then call
conn.WriteJSON(&testenv.WebSocketMessage{...}) (e.g., the instance at the shown
diff and the other occurrences at the ranges noted) switch those calls to
testenv.WSWriteJSON(conn, t, &testenv.WebSocketMessage{...}) (keeping the same
ID, Type and Payload fields) so the union/interface subscription tests use the
router-test helper's timing resilience.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0ec1613-f12a-4bb7-8737-408d89e66940
📒 Files selected for processing (8)
composition/src/errors/errors.tscomposition/src/v1/federation/federation-factory.tscomposition/tests/v1/directives/subscription-filter.test.tsdemo/graph.yamldemo/pkg/subgraphs/employee-events/subgraph/schema.graphqlsdemo/pkg/subgraphs/employeeupdated/subgraph/schema.graphqlsrouter-tests/events/kafka_events_test.gorouter-tests/testenv/testdata/configWithEdfs.json
…tion-losing-subscription-filter # Conflicts: # composition-go/index.global.js
Composition CI lint step (`prettier --check`) flagged formatting drift in the validator refactor. Re-applied via `pnpm --filter composition format`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ptionFilter Adds a "Supported return types" section to the directive page explaining that the filter works on object, union, and interface return types, the all-members validation rule for unions and interfaces, the @inaccessible skip behavior, and the runtime INVALID_GRAPHQL error frame for events with typenames outside the abstract type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Use filter + length===1 instead of find for fieldConfigurations lookup in composition unit tests so a duplicate-emit regression would surface. - Switch new kafka subscribe writes to testenv.WSWriteJSON for retry/backoff, matching the router-tests CLAUDE.md convention. Codex confirmed both changes against project conventions; the two other CodeRabbit nitpicks (explicit void return type, sorted concrete-type order) were rejected because nothing in the surrounding code follows those patterns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion-losing-subscription-filter
This comment has been minimized.
This comment has been minimized.
validateSubscriptionFieldCondition
…-subscription-filter' into jens/eng-9404-composition-losing-subscription-filter
validateSubscriptionFilterAndGenerateConfiguration
Summary
@openfed__subscriptionFilterwhenever the subscription field returned a union or interface (federation-factory.ts:2839gated onOBJECT_TYPE_DEFINITIONand skipped without an error). Router then forwarded every event because no filter config was emitted — see Pylon #2913 / Linear ENG-9404.concreteTypeNamesByAbstractTypeNameindex, validates the condition against each, and emits a single filter config when all walks succeed.@inaccessiblemembers and implementers are skipped before any field-presence check; per-target failures produce a member-tagged composition error.__typenameproduces anINVALID_GRAPHQLerror frame.Test plan
pnpm --filter=@wundergraph/composition test— 1043 pass, 18 skippedcd router-tests && go test -run TestKafkaEvents -count=1 ./events/...— pass in 6.3sgo test -race -run 'TestKafkaEvents/subscribe_async_with_filter$/(union|interface)_return_type' ./events/...— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Errors
Documentation
Demo
Tests