fix(demo): initialize NATS adapters and add GetPubSubName function#2423
fix(demo): initialize NATS adapters and add GetPubSubName function#2423
Conversation
- Call Startup() on NATS adapters to establish connections - Add GetPubSubName function to subgraphs config to prevent nil pointer dereference - Fixes "nats client not initialized" error in mood and availability subgraphs The NATS adapters were being created but not started, leaving the client field uninitialized. This caused mutations that publish to NATS to fail with "nats client not initialized" errors.
WalkthroughThe PR adds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demo/pkg/subgraphs/subgraphs.go (1)
221-234: Good fix for adapter initialization, but consider cleanup on partial failure.The
Startup(ctx)calls correctly initialize the NATS adapters and address the "nats client not initialized" error mentioned in the PR objectives. The error handling is clear and descriptive.However, if
myNatsAdapter.Startup(ctx)fails afterdefaultAdapter.Startup(ctx)succeeds, thedefaultAdapterconnection remains open, creating a resource leak.🔎 Suggested fix to properly clean up resources on failure
if err := defaultAdapter.Startup(ctx); err != nil { return nil, fmt.Errorf("failed to start default nats adapter: %w", err) } + defer func() { + if err != nil { + _ = defaultAdapter.Shutdown(ctx) + } + }() natsPubSubByProviderID["default"] = defaultAdapter myNatsAdapter, err := natsPubsub.NewAdapter(ctx, zap.NewNop(), url, []nats.Option{}, "hostname", "test", datasource.ProviderOpts{ StreamMetricStore: rmetric.NewNoopStreamMetricStore(), }) if err != nil { return nil, fmt.Errorf("failed to create my-nats adapter: %w", err) } if err := myNatsAdapter.Startup(ctx); err != nil { return nil, fmt.Errorf("failed to start my-nats adapter: %w", err) } + defer func() { + if err != nil { + _ = myNatsAdapter.Shutdown(ctx) + } + }() natsPubSubByProviderID["my-nats"] = myNatsAdapterNote: This assumes the adapter has a
Shutdownmethod. Alternatively, you could use a named return value forerrand check it in the defer to clean up both adapters if any subsequent operation fails.
🧹 Nitpick comments (1)
demo/pkg/subgraphs/subgraphs.go (1)
83-83: Consider validating GetPubSubName is non-nil.The new
GetPubSubNamefield is used by the availability and mood subgraphs (lines 196, 200, 273, 276) to resolve PubSub names and prevent nil pointer dereferences in their implementations.Since this is demo code and
main.goalways initializes this field, the current implementation is likely acceptable. However, for defensive programming, you might consider adding a nil check in theNewfunction or providing a default implementation if the field is nil.🔎 Optional: Add defensive nil check
func New(ctx context.Context, config *Config) (*Subgraphs, error) { + if config.GetPubSubName == nil { + config.GetPubSubName = func(name string) string { return name } + } url := nats.DefaultURL if defaultSourceNameURL := os.Getenv("NATS_URL"); defaultSourceNameURL != "" { url = defaultSourceNameURL }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
demo/cmd/all/main.go(1 hunks)demo/pkg/subgraphs/subgraphs.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 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.
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2329
File: router/pkg/pubsub/datasource/subscription_event_updater.go:86-88
Timestamp: 2025-11-13T10:10:47.680Z
Learning: In router/pkg/pubsub/datasource/subscription_event_updater.go, the SetHooks method is intentionally designed to only replace hook handlers, not reconfigure timeout or semaphore settings. The timeout and semaphore fields are meant to be set once during construction via NewSubscriptionEventUpdater and remain immutable. If different timeout or concurrency settings are needed, a new updater instance should be created rather than modifying the existing one.
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
demo/cmd/all/main.godemo/pkg/subgraphs/subgraphs.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 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:
demo/cmd/all/main.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). (10)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
🔇 Additional comments (1)
demo/cmd/all/main.go (1)
40-42: LGTM! Identity function appropriately implements the GetPubSubName hook.The identity function implementation is clean and appropriate for the demo context. It ensures the
GetPubSubNamefield is always set, preventing nil pointer dereferences in the availability and mood subgraphs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2423 +/- ##
==========================================
- Coverage 61.55% 61.35% -0.20%
==========================================
Files 229 229
Lines 23814 23814
==========================================
- Hits 14658 14612 -46
- Misses 7919 7970 +51
+ Partials 1237 1232 -5 🚀 New features to boost your workflow:
|
2 participants
The NATS adapters were being created but not started, leaving the client field uninitialized. This caused mutations that publish to NATS to fail with "nats client not initialized" errors.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.