feat: add bypassIfValuesNull to openfed__subscriptionFilter#2815
Draft
feat: add bypassIfValuesNull to openfed__subscriptionFilter#2815
Conversation
Adds an optional `bypassIfValuesNull: Boolean` field to `openfed__SubscriptionFieldCondition`. When set to `true`, an `IN` condition whose templated value resolves to `nil` or GraphQL `null` evaluates as match instead of skip, so customers with optional subscription arguments can express "no filter when the argument is absent or null". Default `false`; existing behavior unchanged. The flag flows from SDL through composition validation, the proto wire format, the shared serializer, and the router's proto-to-plan mapper into the upstream graphql-go-tools runtime where the bypass is evaluated as a preflight before the event payload is consulted. Coverage: - composition: validate Boolean type, dedup, explicit `false` collapses to undefined, propagation through nested OR - shared: serializer emits proto field only when explicitly `true` - router/core: unit tests for `mapProtoFilterToPlanFilter` covering IN, OR, AND > NOT propagation - router-tests/events: end-to-end NATS tests for explicit value match, non-matching value drops, explicit null variable bypass, and declared-but-absent variable bypass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughAdds an optional boolean flag ChangesSubscription Filter Bypass Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. ✨ Finishing Touches📝 Generate docstrings
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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: 4
🧹 Nitpick comments (1)
router-tests/events/nats_subscription_filter_bypass_test.go (1)
145-151: ⚡ Quick winUse
testenv.WSWriteJSONfor subscribe frames instead of rawconn.WriteJSON.The current writes bypass the retry/deadline behavior used to stabilize websocket tests in this repo.
Suggested change
- err := conn.WriteJSON(&testenv.WebSocketMessage{ + err := testenv.WSWriteJSON(t, conn, &testenv.WebSocketMessage{ ID: "1", Type: "subscribe", Payload: []byte( `{"query":"subscription { filteredEmployeeUpdated(id: 1, filterById: 1) { id } }"}`, ), })As per coding guidelines: "Use testenv.WSReadJSON and testenv.WSWriteJSON for WebSocket reads and writes in tests instead of conn.ReadJSON and conn.WriteJSON, as these helpers include retry logic with 2-second deadlines and exponential backoff."
Also applies to: 205-211, 254-261, 302-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/events/nats_subscription_filter_bypass_test.go` around lines 145 - 151, Replace direct websocket writes that call conn.WriteJSON with the test helper testenv.WSWriteJSON to ensure retries and deadlines are applied; specifically update the subscription writes that currently invoke conn.WriteJSON (e.g., the call creating a testenv.WebSocketMessage with Type "subscribe" and ID "1") to use testenv.WSWriteJSON(conn, <message>) and apply the same replacement for the other occurrences called out (around the blocks at the other mentioned locations). Locate usages by searching for conn.WriteJSON calls that send subscribe frames and swap them to testenv.WSWriteJSON so tests benefit from the built-in 2s deadline and backoff.
🤖 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/federation/federation-factory.ts`:
- Around line 2535-2559: The parser currently lets an IN condition pass
validation when only BYPASS_IF_VALUES_NULL is present because validFieldNames
leftovers don't flip hasErrors; after parsing objectValueNode.fields for the IN
condition (the switch handling BYPASS_IF_VALUES_NULL and other keys), explicitly
validate that required keys "fieldPath" and "values" are present (check
validFieldNames or track presence of fieldPath/values), and if either is missing
set hasErrors = true and push appropriate field error messages (similar to
invalidInputFieldTypeErrorMessage) so you cannot emit an IN filter with empty
fieldPath/values even when condition.bypassIfValuesNull is true; ensure
duplicatedFieldNames/hasSeen handling remains unchanged for
BYPASS_IF_VALUES_NULL and reference the BYPASS_IF_VALUES_NULL constant,
condition.bypassIfValuesNull, validFieldNames, hasErrors, and objectValueNode to
locate where to add this check.
In `@router-tests/events/nats_subscription_filter_bypass_test.go`:
- Around line 345-359: The goroutine uses conn.ReadJSON without a read deadline
which can block forever; replace that pattern by using testenv.WSReadJSON (which
respects timeouts) instead of manual conn.ReadJSON inside the anonymous
goroutine, or at minimum set a read deadline on conn via conn.SetReadDeadline
before calling conn.ReadJSON so the goroutine unblocks when the context timeout
(context.WithTimeout) fires; update the block that creates done := make(chan
testenv.WebSocketMessage, 1) and the goroutine that calls conn.ReadJSON to use
testenv.WSReadJSON (or add conn.SetReadDeadline) so the goroutine cannot leak.
In `@router-tests/go.mod`:
- Line 32: Update the pinned version of the module go.opentelemetry.io/otel/sdk
in both the require and replace sections to at least v1.40.0: locate the require
entry referencing go.opentelemetry.io/otel/sdk (currently v1.39.0) and change it
to v1.40.0 or later, and locate the replace entry for
go.opentelemetry.io/otel/sdk (currently v1.28.0) and bump that to the same
v1.40.0+ version so the transitive grpc v1.79.3 dependency no longer pulls an
older otel/sdk.
In `@router/go.mod`:
- Line 34: Update the go.mod entries so that the module
go.opentelemetry.io/otel/sdk is pinned to v1.40.0 or later in both the require
and replace directives (replace any existing v1.28.0 pin), and ensure the grpc
dependency google.golang.org/grpc is compatible (e.g., keep or set to v1.79.3)
so the otel/sdk version satisfies grpc's minimum; modify the require line and
the corresponding replace line in go.mod to use at least v1.40.0 and verify go
mod tidy builds cleanly.
---
Nitpick comments:
In `@router-tests/events/nats_subscription_filter_bypass_test.go`:
- Around line 145-151: Replace direct websocket writes that call conn.WriteJSON
with the test helper testenv.WSWriteJSON to ensure retries and deadlines are
applied; specifically update the subscription writes that currently invoke
conn.WriteJSON (e.g., the call creating a testenv.WebSocketMessage with Type
"subscribe" and ID "1") to use testenv.WSWriteJSON(conn, <message>) and apply
the same replacement for the other occurrences called out (around the blocks at
the other mentioned locations). Locate usages by searching for conn.WriteJSON
calls that send subscribe frames and swap them to testenv.WSWriteJSON so tests
benefit from the built-in 2s deadline and backoff.
🪄 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: 23978f57-20d9-47c0-866e-314b023fb615
⛔ 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 (16)
composition-go/index.global.jscomposition/src/router-configuration/types.tscomposition/src/utils/string-constants.tscomposition/src/v1/constants/non-directive-definitions.tscomposition/src/v1/federation/federation-factory.tscomposition/tests/v1/directives/subscription-filter.test.tscomposition/tests/v1/utils/utils.tsconnect/src/wg/cosmo/node/v1/node_pb.tsproto/wg/cosmo/node/v1/node.protorouter-tests/events/nats_subscription_filter_bypass_test.gorouter-tests/go.modrouter/core/factoryresolver.gorouter/core/factoryresolver_test.gorouter/go.modshared/src/router-config/graphql-configuration.tsshared/test/subscription-filter.test.ts
Comment on lines
+2535
to
+2559
| let hasSeenBypassIfValuesNull = false; | ||
| for (const objectFieldNode of objectValueNode.fields) { | ||
| const inputFieldName = objectFieldNode.name.value; | ||
| const inputFieldPath = inputPath + `.${inputFieldName}`; | ||
| switch (inputFieldName) { | ||
| case BYPASS_IF_VALUES_NULL: { | ||
| if (hasSeenBypassIfValuesNull) { | ||
| hasErrors = true; | ||
| duplicatedFieldNames.add(BYPASS_IF_VALUES_NULL); | ||
| break; | ||
| } | ||
| hasSeenBypassIfValuesNull = true; | ||
| if (objectFieldNode.value.kind !== Kind.BOOLEAN) { | ||
| fieldErrorMessages.push( | ||
| invalidInputFieldTypeErrorMessage(inputFieldPath, BOOLEAN, kindToNodeType(objectFieldNode.value.kind)), | ||
| ); | ||
| hasErrors = true; | ||
| break; | ||
| } | ||
| // Only persist when explicitly true. Explicit false collapses to undefined so the | ||
| // serializer can emit the smallest proto for unchanged configs. | ||
| if (objectFieldNode.value.value === true) { | ||
| condition.bypassIfValuesNull = true; | ||
| } | ||
| break; |
Contributor
There was a problem hiding this comment.
Still fail IN conditions when fieldPath or values is missing.
This new optional field makes IN: { bypassIfValuesNull: true } validate successfully, because leftover entries in validFieldNames never flip hasErrors. That lets composition emit an empty fieldPath/values filter downstream.
Suggested fix
}
- if (!hasErrors) {
+ if (validFieldNames.size > 0) {
+ hasErrors = true;
+ }
+ if (!hasErrors) {
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let hasSeenBypassIfValuesNull = false; | |
| for (const objectFieldNode of objectValueNode.fields) { | |
| const inputFieldName = objectFieldNode.name.value; | |
| const inputFieldPath = inputPath + `.${inputFieldName}`; | |
| switch (inputFieldName) { | |
| case BYPASS_IF_VALUES_NULL: { | |
| if (hasSeenBypassIfValuesNull) { | |
| hasErrors = true; | |
| duplicatedFieldNames.add(BYPASS_IF_VALUES_NULL); | |
| break; | |
| } | |
| hasSeenBypassIfValuesNull = true; | |
| if (objectFieldNode.value.kind !== Kind.BOOLEAN) { | |
| fieldErrorMessages.push( | |
| invalidInputFieldTypeErrorMessage(inputFieldPath, BOOLEAN, kindToNodeType(objectFieldNode.value.kind)), | |
| ); | |
| hasErrors = true; | |
| break; | |
| } | |
| // Only persist when explicitly true. Explicit false collapses to undefined so the | |
| // serializer can emit the smallest proto for unchanged configs. | |
| if (objectFieldNode.value.value === true) { | |
| condition.bypassIfValuesNull = true; | |
| } | |
| break; | |
| let hasSeenBypassIfValuesNull = false; | |
| for (const objectFieldNode of objectValueNode.fields) { | |
| const inputFieldName = objectFieldNode.name.value; | |
| const inputFieldPath = inputPath + `.${inputFieldName}`; | |
| switch (inputFieldName) { | |
| case BYPASS_IF_VALUES_NULL: { | |
| if (hasSeenBypassIfValuesNull) { | |
| hasErrors = true; | |
| duplicatedFieldNames.add(BYPASS_IF_VALUES_NULL); | |
| break; | |
| } | |
| hasSeenBypassIfValuesNull = true; | |
| if (objectFieldNode.value.kind !== Kind.BOOLEAN) { | |
| fieldErrorMessages.push( | |
| invalidInputFieldTypeErrorMessage(inputFieldPath, BOOLEAN, kindToNodeType(objectFieldNode.value.kind)), | |
| ); | |
| hasErrors = true; | |
| break; | |
| } | |
| // Only persist when explicitly true. Explicit false collapses to undefined so the | |
| // serializer can emit the smallest proto for unchanged configs. | |
| if (objectFieldNode.value.value === true) { | |
| condition.bypassIfValuesNull = true; | |
| } | |
| break; | |
| } | |
| if (validFieldNames.size > 0) { | |
| hasErrors = true; | |
| } | |
| if (!hasErrors) { | |
| return true; | |
| } |
🤖 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 2535 -
2559, The parser currently lets an IN condition pass validation when only
BYPASS_IF_VALUES_NULL is present because validFieldNames leftovers don't flip
hasErrors; after parsing objectValueNode.fields for the IN condition (the switch
handling BYPASS_IF_VALUES_NULL and other keys), explicitly validate that
required keys "fieldPath" and "values" are present (check validFieldNames or
track presence of fieldPath/values), and if either is missing set hasErrors =
true and push appropriate field error messages (similar to
invalidInputFieldTypeErrorMessage) so you cannot emit an IN filter with empty
fieldPath/values even when condition.bypassIfValuesNull is true; ensure
duplicatedFieldNames/hasSeen handling remains unchanged for
BYPASS_IF_VALUES_NULL and reference the BYPASS_IF_VALUES_NULL constant,
condition.bypassIfValuesNull, validFieldNames, hasErrors, and objectValueNode to
locate where to add this check.
Comment on lines
+345
to
+359
| ctx, cancel := context.WithTimeout(xEnv.Context, 500*time.Millisecond) | ||
| defer cancel() | ||
| done := make(chan testenv.WebSocketMessage, 1) | ||
| go func() { | ||
| var unexpected testenv.WebSocketMessage | ||
| if err := conn.ReadJSON(&unexpected); err == nil { | ||
| done <- unexpected | ||
| } | ||
| }() | ||
| select { | ||
| case unexpected := <-done: | ||
| t.Fatalf("unexpected message arrived after filtered events: %+v", unexpected) | ||
| case <-ctx.Done(): | ||
| // no further messages — filter held. | ||
| } |
Contributor
There was a problem hiding this comment.
conn.ReadJSON without a read deadline can block indefinitely and leak the goroutine.
context.WithTimeout here does not cancel ReadJSON, so the goroutine can stay stuck on socket read and make this test flaky under slow/quiet conditions.
Suggested change
- ctx, cancel := context.WithTimeout(xEnv.Context, 500*time.Millisecond)
- defer cancel()
- done := make(chan testenv.WebSocketMessage, 1)
- go func() {
- var unexpected testenv.WebSocketMessage
- if err := conn.ReadJSON(&unexpected); err == nil {
- done <- unexpected
- }
- }()
- select {
- case unexpected := <-done:
- t.Fatalf("unexpected message arrived after filtered events: %+v", unexpected)
- case <-ctx.Done():
- // no further messages — filter held.
- }
+ require.NoError(t, conn.SetReadDeadline(time.Now().Add(500*time.Millisecond)))
+ var unexpected testenv.WebSocketMessage
+ err = conn.ReadJSON(&unexpected)
+ assert.Error(t, err, "expected timeout/no message after filtered event")As per coding guidelines: "Use manual SetReadDeadline with conn.ReadJSON only when expecting errors (e.g., websocket close after config hot reload); otherwise use testenv.WSReadJSON."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router-tests/events/nats_subscription_filter_bypass_test.go` around lines 345
- 359, The goroutine uses conn.ReadJSON without a read deadline which can block
forever; replace that pattern by using testenv.WSReadJSON (which respects
timeouts) instead of manual conn.ReadJSON inside the anonymous goroutine, or at
minimum set a read deadline on conn via conn.SetReadDeadline before calling
conn.ReadJSON so the goroutine unblocks when the context timeout
(context.WithTimeout) fires; update the block that creates done := make(chan
testenv.WebSocketMessage, 1) and the goroutine that calls conn.ReadJSON to use
testenv.WSReadJSON (or add conn.SetReadDeadline) so the goroutine cannot leak.
| github.com/wundergraph/cosmo/router v0.0.0-20260318232543-0e5fa811a191 | ||
| github.com/wundergraph/cosmo/router-plugin v0.0.0-20250808194725-de123ba1c65e | ||
| github.com/wundergraph/graphql-go-tools/v2 v2.0.0-rc.269 | ||
| github.com/wundergraph/graphql-go-tools/v2 v2.1.1-0.20260504064838-5a00844995b5 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting router-tests/go.mod security-relevant pins"
rg -n 'google\.golang\.org/grpc v1\.79\.3|go\.opentelemetry\.io/otel/sdk' router-tests/go.mod
echo
echo "Expected:"
echo "- grpc is pinned at v1.79.3"
echo "- otel/sdk should be pinned to v1.40.0 or later in both require/replace"
echo "- current output will show it is still below that floor"Repository: wundergraph/cosmo
Length of output: 568
Pin go.opentelemetry.io/otel/sdk to v1.40.0+ in both require and replace sections.
This module pins grpc v1.79.3 at line 42, which can transitively pull a vulnerable otel/sdk version. Currently, otel/sdk is set to v1.39.0 (line 34) and v1.28.0 in the replace section (line 204), both below the required v1.40.0 floor. Update both pins to v1.40.0 or later to match the security requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router-tests/go.mod` at line 32, Update the pinned version of the module
go.opentelemetry.io/otel/sdk in both the require and replace sections to at
least v1.40.0: locate the require entry referencing go.opentelemetry.io/otel/sdk
(currently v1.39.0) and change it to v1.40.0 or later, and locate the replace
entry for go.opentelemetry.io/otel/sdk (currently v1.28.0) and bump that to the
same v1.40.0+ version so the transitive grpc v1.79.3 dependency no longer pulls
an older otel/sdk.
| github.com/tidwall/sjson v1.2.5 | ||
| github.com/twmb/franz-go v1.16.1 | ||
| github.com/wundergraph/graphql-go-tools/v2 v2.0.0-rc.269 | ||
| github.com/wundergraph/graphql-go-tools/v2 v2.1.1-0.20260504064838-5a00844995b5 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting router/go.mod security-relevant pins"
rg -n 'google\.golang\.org/grpc v1\.79\.3|go\.opentelemetry\.io/otel/sdk' router/go.mod
echo
echo "Expected:"
echo "- grpc is pinned at v1.79.3"
echo "- otel/sdk should be pinned to v1.40.0 or later in both require/replace"
echo "- current output will show it is still below that floor"Repository: wundergraph/cosmo
Length of output: 562
Pin go.opentelemetry.io/otel/sdk to v1.40.0+ in both require and replace directives before merging.
This module currently resolves go.opentelemetry.io/otel/sdk to v1.39.0 in the require section and v1.28.0 via a replace directive, both below the safe floor required when using google.golang.org/grpc v1.79.3. Update both pins to v1.40.0 or later while modifying the grpc dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/go.mod` at line 34, Update the go.mod entries so that the module
go.opentelemetry.io/otel/sdk is pinned to v1.40.0 or later in both the require
and replace directives (replace any existing v1.28.0 pin), and ensure the grpc
dependency google.golang.org/grpc is compatible (e.g., keep or set to v1.79.3)
so the otel/sdk version satisfies grpc's minimum; modify the require line and
the corresponding replace line in go.mod to use at least v1.40.0 and verify go
mod tidy builds cleanly.
…ing-techniques # Conflicts: # composition-go/index.global.js # router-tests/go.mod # router-tests/go.sum # router/core/factoryresolver.go # router/go.mod # router/go.sum
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2815 +/- ##
==========================================
+ Coverage 46.24% 46.25% +0.01%
==========================================
Files 1084 1084
Lines 145014 145046 +32
Branches 9247 9253 +6
==========================================
+ Hits 67055 67092 +37
+ Misses 76179 76174 -5
Partials 1780 1780
🚀 New features to boost your workflow:
|
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/go.mod (1)
46-47:⚠️ Potential issue | 🔴 CriticalMajor: OpenTelemetry SDK version is pinned to a vulnerable version via
replace.Although
requiresetsgo.opentelemetry.io/otel/sdktov1.39.0, thereplaceblock downgrades bothgo.opentelemetry.io/otel/sdkandgo.opentelemetry.io/otel/sdk/metrictov1.28.0. This version is vulnerable to CVE-2026-24051 (path hijacking in resource detection on macOS/Darwin), which allows arbitrary code execution. Upgrade thereplacedirectives to at leastv1.40.0to mitigate this security issue.Suggested fix: bump `otel/sdk` versions in replace block to v1.40.0+
replace ( ... - go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.28.0 - go.opentelemetry.io/otel/sdk/metric => go.opentelemetry.io/otel/sdk/metric v1.28.0 + go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.40.0 + go.opentelemetry.io/otel/sdk/metric => go.opentelemetry.io/otel/sdk/metric v1.40.0 ... )Also applies to: 192-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/go.mod` around lines 46 - 47, The go.mod currently forces go.opentelemetry.io/otel/sdk and go.opentelemetry.io/otel/sdk/metric to a vulnerable v1.28.0 via a replace; update the replace directives so both packages point to at least v1.40.0 (or a newer non-vulnerable tag) to mitigate CVE-2026-24051, ensuring the require entries remain consistent with go.opentelemetry.io/otel/sdk v1.39.0 (or are bumped appropriately) so the replace no longer downgrades the SDK; locate the replace entries referencing "go.opentelemetry.io/otel/sdk" and "go.opentelemetry.io/otel/sdk/metric" and change their versions to v1.40.0+.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@router/go.mod`:
- Around line 46-47: The go.mod currently forces go.opentelemetry.io/otel/sdk
and go.opentelemetry.io/otel/sdk/metric to a vulnerable v1.28.0 via a replace;
update the replace directives so both packages point to at least v1.40.0 (or a
newer non-vulnerable tag) to mitigate CVE-2026-24051, ensuring the require
entries remain consistent with go.opentelemetry.io/otel/sdk v1.39.0 (or are
bumped appropriately) so the replace no longer downgrades the SDK; locate the
replace entries referencing "go.opentelemetry.io/otel/sdk" and
"go.opentelemetry.io/otel/sdk/metric" and change their versions to v1.40.0+.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4746faaf-90d9-4645-b3f7-dfc51993c41e
⛔ Files ignored due to path filters (3)
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/**
📒 Files selected for processing (8)
composition/src/router-configuration/types.tscomposition/src/utils/string-constants.tscomposition/src/v1/federation/federation-factory.tsconnect/src/wg/cosmo/node/v1/node_pb.tsproto/wg/cosmo/node/v1/node.protorouter-tests/go.modrouter/core/factoryresolver.gorouter/go.mod
✅ Files skipped from review due to trivial changes (3)
- composition/src/utils/string-constants.ts
- connect/src/wg/cosmo/node/v1/node_pb.ts
- router/core/factoryresolver.go
🚧 Files skipped from review as they are similar to previous changes (2)
- composition/src/router-configuration/types.ts
- router-tests/go.mod
…n composition-go bundle - Rename `WebSocketClientReadTimeout` → `WebSocketServerReadTimeout` in the new NATS bypass integration tests; the field was renamed in main. - Regenerate `composition-go/index.global.js` so the embedded TS bundle picks up the new `bypassIfValuesNull` SDL field and validator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
router-tests/events/nats_subscription_filter_bypass_test.go (1)
345-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the final read with a socket deadline (current goroutine can block indefinitely).
Line [345]-Line [359] uses
context.WithTimeout, but that does not cancelconn.ReadJSONin the goroutine. Set a read deadline and do a direct read (expecting timeout/no message), or use the helper pattern for this assertion.Suggested fix
- ctx, cancel := context.WithTimeout(xEnv.Context, 500*time.Millisecond) - defer cancel() - done := make(chan testenv.WebSocketMessage, 1) - go func() { - var unexpected testenv.WebSocketMessage - if err := conn.ReadJSON(&unexpected); err == nil { - done <- unexpected - } - }() - select { - case unexpected := <-done: - t.Fatalf("unexpected message arrived after filtered events: %+v", unexpected) - case <-ctx.Done(): - // no further messages — filter held. - } + require.NoError(t, conn.SetReadDeadline(time.Now().Add(500*time.Millisecond))) + var unexpected testenv.WebSocketMessage + err = conn.ReadJSON(&unexpected) + assert.Error(t, err, "expected timeout/no message after filtered event")As per coding guidelines: "Use manual SetReadDeadline with conn.ReadJSON only when expecting errors (e.g., websocket close after config hot reload); otherwise use testenv.WSReadJSON."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/events/nats_subscription_filter_bypass_test.go` around lines 345 - 359, The goroutine using conn.ReadJSON can block indefinitely because context.WithTimeout doesn't cancel ReadJSON; replace this pattern by either setting a socket read deadline on conn (call conn.SetReadDeadline to now+500ms) and doing a direct conn.ReadJSON into a testenv.WebSocketMessage expecting a timeout/no message, or use the existing helper testenv.WSReadJSON which handles deadlines correctly; remove the background goroutine and the context.WithTimeout usage and perform the bounded read directly (references: conn.ReadJSON, conn.SetReadDeadline, testenv.WSReadJSON, testenv.WebSocketMessage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/events/nats_subscription_filter_bypass_test.go`:
- Around line 145-152: Replace direct WebSocket writes that call
conn.WriteJSON(...) with the test helper testenv.WSWriteJSON(...) so the writes
use the built-in retry/deadline logic; locate each occurrence of conn.WriteJSON
in the file (e.g., the call that writes the subscribe message in the
subscription subtests) and change the call to testenv.WSWriteJSON(t, conn, <same
payload>) preserving the exact message payload and error handling, and repeat
the same replacement for the other occurrences mentioned (the writes around the
blocks using subscription filteredEmployeeUpdated). Ensure you import/use the
testenv helper signature (t, conn, msg) and remove any manual require.NoError
after the call if testenv.WSWriteJSON already fails the test on error.
---
Duplicate comments:
In `@router-tests/events/nats_subscription_filter_bypass_test.go`:
- Around line 345-359: The goroutine using conn.ReadJSON can block indefinitely
because context.WithTimeout doesn't cancel ReadJSON; replace this pattern by
either setting a socket read deadline on conn (call conn.SetReadDeadline to
now+500ms) and doing a direct conn.ReadJSON into a testenv.WebSocketMessage
expecting a timeout/no message, or use the existing helper testenv.WSReadJSON
which handles deadlines correctly; remove the background goroutine and the
context.WithTimeout usage and perform the bounded read directly (references:
conn.ReadJSON, conn.SetReadDeadline, testenv.WSReadJSON,
testenv.WebSocketMessage).
🪄 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: a8b4dd20-c06c-457b-98cc-faafc56cdba8
📒 Files selected for processing (2)
composition-go/index.global.jsrouter-tests/events/nats_subscription_filter_bypass_test.go
Comment on lines
+145
to
+152
| err := conn.WriteJSON(&testenv.WebSocketMessage{ | ||
| ID: "1", | ||
| Type: "subscribe", | ||
| Payload: []byte( | ||
| `{"query":"subscription { filteredEmployeeUpdated(id: 1, filterById: 1) { id } }"}`, | ||
| ), | ||
| }) | ||
| require.NoError(t, err) |
Contributor
There was a problem hiding this comment.
Replace raw websocket writes with the test helper.
conn.WriteJSON is used directly in all four subtests. Please switch these writes to testenv.WSWriteJSON(...) to keep retry/deadline behavior consistent and reduce websocket timing flakes in CI.
As per coding guidelines: "Use testenv.WSReadJSON and testenv.WSWriteJSON for WebSocket reads and writes in tests instead of conn.ReadJSON and conn.WriteJSON, as these helpers include retry logic with 2-second deadlines and exponential backoff."
Also applies to: 205-212, 254-262, 302-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router-tests/events/nats_subscription_filter_bypass_test.go` around lines 145
- 152, Replace direct WebSocket writes that call conn.WriteJSON(...) with the
test helper testenv.WSWriteJSON(...) so the writes use the built-in
retry/deadline logic; locate each occurrence of conn.WriteJSON in the file
(e.g., the call that writes the subscribe message in the subscription subtests)
and change the call to testenv.WSWriteJSON(t, conn, <same payload>) preserving
the exact message payload and error handling, and repeat the same replacement
for the other occurrences mentioned (the writes around the blocks using
subscription filteredEmployeeUpdated). Ensure you import/use the testenv helper
signature (t, conn, msg) and remove any manual require.NoError after the call if
testenv.WSWriteJSON already fails the test on error.
…test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
composition/tests/v1/directives/subscription-filter.test.ts (1)
513-540: ⚡ Quick winCover one nested non-
ORbranch too.This test proves propagation through
OR, but not through nestedAND/NOTpaths. Since the behavior is recursive, a regression in those branches would still slip through.🤖 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 513 - 540, The test only covers propagation through an OR branch; add a case verifying propagation through a nested non-OR branch (e.g., AND and/or NOT) by extending or adding a test that calls federateSubgraphsSuccess (with subgraphB and subgraphBypass3 and ROUTER_COMPATIBILITY_VERSION_ONE) and asserts fieldConfigurations.subscriptionFilterCondition contains an AND/NOT wrapper around an in clause that has bypassIfValuesNull: true; update expectations to include the nested AND/NOT structure (referring to symbols: federateSubgraphsSuccess, subgraphB, subgraphBypass3, ROUTER_COMPATIBILITY_VERSION_ONE, SUBSCRIPTION, fieldConfigurations, subscriptionFilterCondition, in, bypassIfValuesNull) so the recursive propagation through non-OR branches is validated.
🤖 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/subscription-filter.test.ts`:
- Around line 513-540: The test only covers propagation through an OR branch;
add a case verifying propagation through a nested non-OR branch (e.g., AND
and/or NOT) by extending or adding a test that calls federateSubgraphsSuccess
(with subgraphB and subgraphBypass3 and ROUTER_COMPATIBILITY_VERSION_ONE) and
asserts fieldConfigurations.subscriptionFilterCondition contains an AND/NOT
wrapper around an in clause that has bypassIfValuesNull: true; update
expectations to include the nested AND/NOT structure (referring to symbols:
federateSubgraphsSuccess, subgraphB, subgraphBypass3,
ROUTER_COMPATIBILITY_VERSION_ONE, SUBSCRIPTION, fieldConfigurations,
subscriptionFilterCondition, in, bypassIfValuesNull) so the recursive
propagation through non-OR branches is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82af321c-0bc4-46e0-932d-ee3812dd3337
📒 Files selected for processing (1)
composition/tests/v1/directives/subscription-filter.test.ts
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
1 participant
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.
Summary
Adds an optional
bypassIfValuesNull: Booleanfield toopenfed__SubscriptionFieldCondition. When set totrue, anINcondition whose templated value resolves tonilor GraphQLnullevaluates as match instead of skip — letting customers express "no filter when the argument is absent or null" for optional subscription arguments.Default is
false; existing behavior is unchanged.Linear: ENG-9357. Customer ask: Pylon #2869 (Openphone).
Customer SDL after this change:
Status: 🚧 Draft — blocked on upstream
Depends on wundergraph/graphql-go-tools#1484. The router currently consumes that PR's head commit (
5a00844995b5) viago.modversion pin.The PR is rebased on graphql-go-tools
master(post-v2.0.0), which has API changes the cosmo router has not yet absorbed. Two known breakages:plan.FieldWeight→plan.FieldCost(mechanical rename, fixed in this PR atrouter/core/factoryresolver.go).resolve.SubscriptionCloseKindandSubscriptionCloseKindNormalwere removed;SubscriptionResponseWriter.Close(SubscriptionCloseKind)becameComplete(). This affectsrouter/core/flushwriter.go,router/core/websocket.go, and therouter/pkg/pubsub/datasourcepackage (mocks +subscription_event_updater). Not addressed in this PR.CI will be red until the upstream is rebased onto a compatible base, or the cosmo router is bumped to consume
v2.0.0master.Changes
proto/wg/cosmo/node/v1/node.protooptional bool bypass_if_values_null = 3onSubscriptionFieldCondition. Wire-compatible.pnpm generate+make generate-go+composition-go go generate.composition/BYPASS_IF_VALUES_NULLconstant; injected SDL field onopenfed__SubscriptionFieldCondition; newBYPASS_IF_VALUES_NULLswitch arm invalidateSubscriptionFieldCondition(Boolean-only, dedup-checked, explicitfalsecollapses to undefined); type updated inrouter-configuration/types.ts.shared/src/router-config/graphql-configuration.tsgenerateSubscriptionFilterConditiononly emits the proto field whencondition.in.bypassIfValuesNull === true.router/core/factoryresolver.gomapProtoFilterToPlanFilterpropagatesBypassIfValuesNullontoplan.SubscriptionFieldCondition.router/go.mod,router-tests/go.modv2.1.1-0.20260504064838-5a00844995b5(PR #1484 head).Test plan
composition— 6 new tests coverbypassIfValuesNull: truepreserved,falsecollapsed, omitted, propagated through nestedOR, non-Boolean type rejected, duplicate field rejected. Full suite green.shared— 4 new tests covertrueemits,undefinedleaves unset, explicitfalseleaves unset, propagation through nestedOR. All green.router/core— newfactoryresolver_test.go: nil input,true/unset/explicit-falseon IN, propagation throughORandAND > NOT. All green.router-tests/events/nats_subscription_filter_bypass_test.go— 4 end-to-end NATS subscription cases:filterByIdmatches event idfilterByIdset to non-matching value drops events$filterByIdvariable triggers bypass$filterByIdvariable absent fromvariablesmap triggers bypassSummary by CodeRabbit
New Features
Tests