diff --git a/pkg/gofr/container/container.go b/pkg/gofr/container/container.go index 2f8719d7ae..cded0a2c73 100644 --- a/pkg/gofr/container/container.go +++ b/pkg/gofr/container/container.go @@ -434,11 +434,45 @@ func (c *Container) GetAppVersion() string { return c.appVersion } +// GetPublisher returns the pub/sub client, or nil when none is usable. +// +// Same filter, same reason as GetSubscriber below -- a handler calling +// ctx.GetPublisher().Publish(...) against a typed-nil client hits the identical +// nil receiver, it just surfaces on a request instead of at startup. +// +// To be precise about what this does and does not buy: an unconditional +// ctx.GetPublisher().Publish(...) still panics, now on a nil interface rather +// than a nil receiver. What the filter fixes is every caller that DOES check, +// whose `!= nil` guard the typed nil used to walk straight through. The +// constructors no longer put a typed nil in the field at all, so this is +// defense in depth rather than the only line of it. func (c *Container) GetPublisher() pubsub.Publisher { + if isNil(c.PubSub) { + return nil + } + return c.PubSub } +// GetSubscriber returns the pub/sub client, or nil when none is usable. +// +// The nil check is isNil rather than a plain comparison because the pub/sub +// constructors assign the result of google.New or kafka.New straight into the +// interface, and those return a TYPED nil when they reject an incomplete +// config. A typed nil is not equal to nil, so returning c.PubSub unfiltered let +// every caller's own nil guard pass and then call a method on a nil receiver. +// Filtering here fixes all of them at once, the worst being App.Subscribe: its +// `GetSubscriber() == nil` guard admitted the typed nil, registered the +// subscription, and handleSubscription then called Subscribe on the nil +// receiver. That call sits outside the recover it installs around the handler, +// and errgroup does not recover either -- x/sync v0.23.0 says so in +// errgroup.go, "It is tempting to propagate panics from f() [...]" -- so the +// panic killed the process at startup rather than surfacing on a request. func (c *Container) GetSubscriber() pubsub.Subscriber { + if isNil(c.PubSub) { + return nil + } + return c.PubSub } @@ -542,7 +576,12 @@ func (c *Container) createKafkaPubSub(conf config.Config) { pubsubBrokers := strings.Split(conf.Get("PUBSUB_BROKER"), ",") - c.PubSub = kafka.New(&kafka.Config{ + // Assigned only when a real client came back. kafka.New returns a bare nil *kafkaClient when it + // rejects the config, and assigning that straight into the pubsub.Client interface is what + // creates the typed nil in the first place: a non-nil interface holding a nil pointer, which + // every `c.PubSub != nil` check in the codebase admits. The getters filter it as defense in + // depth, but they cannot help a direct reader of the exported field. + if client := kafka.New(&kafka.Config{ Brokers: pubsubBrokers, Partition: partition, ConsumerGroupID: conf.Get("CONSUMER_ID"), @@ -555,14 +594,20 @@ func (c *Container) createKafkaPubSub(conf config.Config) { SASLUser: conf.Get("KAFKA_SASL_USERNAME"), SASLPassword: conf.Get("KAFKA_SASL_PASSWORD"), TLS: tlsConf, - }, c.Logger, c.metricsManager) + }, c.Logger, c.metricsManager); client != nil { + c.PubSub = client + } } func (c *Container) createGooglePubSub(conf config.Config) { - c.PubSub = google.New(google.Config{ + // See createKafkaPubSub: google.New returns a typed nil for an incomplete config, and only a + // real client may reach the exported field. + if client := google.New(google.Config{ ProjectID: conf.Get("GOOGLE_PROJECT_ID"), SubscriptionName: conf.Get("GOOGLE_SUBSCRIPTION_NAME"), - }, c.Logger, c.metricsManager) + }, c.Logger, c.metricsManager); client != nil { + c.PubSub = client + } } func (c *Container) createRedisPubSub(conf config.Config) { diff --git a/pkg/gofr/container/health.go b/pkg/gofr/container/health.go index 15d098d25a..2304070dd2 100644 --- a/pkg/gofr/container/health.go +++ b/pkg/gofr/container/health.go @@ -198,7 +198,18 @@ func (c *Container) checkPrimaryDatasources(ctx context.Context, collector *heal }) } - if c.PubSub != nil { + // isNil, not a plain != nil: the pub/sub constructors assign the result of + // google.New or kafka.New straight into this interface, and those return a + // TYPED nil when they reject an incomplete config. A typed nil in an interface + // is not equal to nil, so a plain check admits it and the Health call below + // runs on a nil receiver. SQL and Redis above already use isNil for the same + // reason; this guard was the odd one out. + // + // runCheck recovers, so this was never a crash here -- it surfaced as a + // "pubsub": DOWN entry reading "health check panicked", which put the whole + // app into DEGRADED over a dependency it does not have. The crash is on the + // GetSubscriber path, where App.Subscribe's errgroup has no recover. + if !isNil(c.PubSub) { runCheck(wg, collector, pubsubKey, func() { health := c.PubSub.Health() collector.record(pubsubKey, health, health.Status == datasource.StatusDown) @@ -342,12 +353,32 @@ func (c *Container) appHealth(healthMap map[string]any, downCount int) { } } +// isNil reports whether i is absent: either an unset interface, or an interface +// holding a nil pointer. +// +// The Kind check is not optional. reflect.Value.IsNil PANICS on a value whose +// kind cannot be nil, and a datasource field can legitimately hold one: +// App.AddPubSub, App.AddMongo and the rest take an interface, so a caller whose +// implementation has value receivers can hand over a struct rather than a +// pointer -- ordinary Go, and nothing in the signature discourages it. The +// unguarded IsNil panicked on that, and Container.Close reaches it from the +// shutdown goroutine in startShutdownHandler, which has no recover. +// +// Anything not nillable is present by definition, so it reports false. func isNil(i any) bool { - // Get the value of the interface val := reflect.ValueOf(i) + if !val.IsValid() { + return true + } - // If the interface is not assigned or is nil, return true - return !val.IsValid() || val.IsNil() + //nolint:exhaustive // the default is the point: every kind that cannot be nil is present. + switch val.Kind() { + case reflect.Chan, reflect.Func, reflect.Interface, + reflect.Map, reflect.Pointer, reflect.Slice, reflect.UnsafePointer: + return val.IsNil() + default: + return false + } } // stalledHealth is the body returned while a previous round's checks are still outstanding. It diff --git a/pkg/gofr/container/pubsub_health_nil_test.go b/pkg/gofr/container/pubsub_health_nil_test.go new file mode 100644 index 0000000000..5bccbc4e60 --- /dev/null +++ b/pkg/gofr/container/pubsub_health_nil_test.go @@ -0,0 +1,232 @@ +package container + +import ( + "context" + "testing" + "unsafe" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "gofr.dev/pkg/gofr/config" + "gofr.dev/pkg/gofr/datasource" + "gofr.dev/pkg/gofr/datasource/pubsub" + "gofr.dev/pkg/gofr/logging" +) + +// nilReceiverClient reproduces the shape the real constructors produce: a +// pubsub.Client whose methods are reached through a nil pointer. +// +// google.New and kafka.New return a typed nil when they reject an incomplete +// config -- (*googleClient)(nil), (*kafkaClient)(nil) -- and the createXPubSub +// helpers assign that straight into the interface. A typed nil is not equal to +// nil, so every plain "!= nil" guard downstream passes and then calls a method on +// a nil receiver. +// +// Health dereferences the receiver for the same reason googleClient.Health does +// (it reads a config field), so a guard that stops filtering is a panic here +// rather than a silently different result. Building the state directly keeps +// these tests independent of what google.New happens to validate. +type nilReceiverClient struct { + status string +} + +func (c *nilReceiverClient) Health() datasource.Health { + return datasource.Health{Status: c.status} +} + +func (*nilReceiverClient) Publish(context.Context, string, []byte) error { return nil } + +func (*nilReceiverClient) Subscribe(context.Context, string) (*pubsub.Message, error) { + return nil, nil +} + +func (*nilReceiverClient) CreateTopic(context.Context, string) error { return nil } + +func (*nilReceiverClient) DeleteTopic(context.Context, string) error { return nil } + +func (*nilReceiverClient) Query(context.Context, string, ...any) ([]byte, error) { return nil, nil } + +func (*nilReceiverClient) Close() error { return nil } + +// typedNilPubSubContainer builds the state every test below needs: a container +// whose PubSub interface holds a typed nil. +func typedNilPubSubContainer(t *testing.T) *Container { + t.Helper() + + var client *nilReceiverClient + + c := &Container{} + c.PubSub = client + + // Stated as a plain comparison, not require.NotNil: that one reflects, and + // would report this very value as nil -- which is the whole premise. + if c.PubSub == nil { + t.Fatal("the premise: a typed nil assigned into the interface must not compare equal to nil") + } + + return c +} + +// assertPlainNil asserts the way callers actually test the value. +// +// assert.Nil is deliberately NOT used: it reflects, so it reports a typed nil as +// nil and would pass whether or not the filter exists. App.Subscribe writes +// `GetSubscriber() == nil`, so the test has to make the same plain comparison to +// mean anything -- which is what this helper exists to say, so the next reader +// does not "fix" the assertion back. +func assertPlainNil[T any](t *testing.T, got T, msgAndArgs ...any) { + t.Helper() + + //nolint:testifylint // a reflective nil check cannot fail here; see the doc comment + assert.True(t, any(got) == nil, msgAndArgs...) +} + +// TestHealthOmitsTypedNilPubSub pins the guard in checkPrimaryDatasources, which +// called Health() on the nil receiver. +// +// The assertion is on the health map, not on NotPanics. runCheck already recovers +// in its deferred func, so the panic never escaped -- it was converted into a pubsub +// entry reading "health check panicked", which is the visible defect: an app with +// no usable pub/sub reported a DOWN dependency, and any aggregator watching the +// endpoint saw a degraded service with a stack fragment for a reason. With the +// guard the key is absent, which is what "no pub/sub configured" has always +// looked like. A NotPanics assertion here would pass either way and prove +// nothing. +func TestHealthOmitsTypedNilPubSub(t *testing.T) { + c := typedNilPubSubContainer(t) + + health, ok := c.Health(context.Background()).(map[string]any) + require.True(t, ok, "Health returns a map") + + assert.NotContains(t, health, pubsubKey, + "a typed nil pub/sub must not be reported as a dependency at all") +} + +// TestGetSubscriberFiltersTypedNil pins the more damaging one. +// +// App.Subscribe guards with `GetSubscriber() == nil` and then hands the client to +// a goroutine in an errgroup that has no recover, so the nil-receiver panic there +// killed the process at startup rather than on a request. Filtering in +// GetSubscriber fixes every caller at once, so this is the assertion that keeps +// them all honest. +func TestGetSubscriberFiltersTypedNil(t *testing.T) { + c := typedNilPubSubContainer(t) + + assertPlainNil(t, c.GetSubscriber(), + "a typed nil must not escape GetSubscriber: callers compare it against nil") +} + +// TestGetPublisherFiltersTypedNil is the same guarantee on the publishing side. +// +// The two getters return the same field, so leaving one unfiltered would be an +// asymmetry a reader has to guess about. A handler writing +// ctx.GetPublisher().Publish(...) against a typed-nil client hits the identical +// nil receiver; it just surfaces on a request rather than at startup. +func TestGetPublisherFiltersTypedNil(t *testing.T) { + c := typedNilPubSubContainer(t) + + assertPlainNil(t, c.GetPublisher(), + "a typed nil must not escape GetPublisher either") +} + +// TestIsNilHandlesNonNillableKinds pins that isNil does not panic on a value +// whose kind cannot be nil. +// +// reflect.Value.IsNil panics for a struct, a string, an int and so on, and a +// datasource field can legitimately hold one: App.AddPubSub and friends take an +// interface, so an implementation with value receivers can be handed over as a +// struct rather than a pointer. GoFr writes such implementations itself -- +// sqlMockDB's methods are on a value receiver -- it just happens to pass them by +// address. Before the Kind check, isNil panicked on every one of those; in +// Container.Close that reached the shutdown goroutine, which has no recover. +func TestIsNilHandlesNonNillableKinds(t *testing.T) { + type valueImpl struct{ name string } + + present := []any{ + valueImpl{name: "struct value"}, + "a string", + 42, + [2]int{1, 2}, + } + + for _, v := range present { + assert.NotPanics(t, func() { _ = isNil(v) }, "%T must not panic", v) + assert.False(t, isNil(v), "%T is present, not nil", v) + } + + // Every nillable kind keeps its meaning. Listed exhaustively rather than testing the pointer + // alone: the Kind switch enumerates them, so a kind dropped from it would otherwise fall through + // to "not nillable, therefore present" silently, which is the wrong answer in the safe-looking + // direction. + var ( + nilPtr *valueImpl + nilMap map[string]int + nilSlice []int + nilChan chan int + nilFunc func() + nilIface error + nilUnsaf unsafe.Pointer + ) + + absent := []any{nilPtr, nilMap, nilSlice, nilChan, nilFunc, nilIface, nilUnsaf} + for _, v := range absent { + assert.True(t, isNil(v), "a nil %T is absent", v) + } + + assert.True(t, isNil(nil), "an unset interface is absent") + + presentNillable := []any{ + &valueImpl{}, + map[string]int{}, + []int{}, + make(chan int), + func() {}, + } + for _, v := range presentNillable { + assert.False(t, isNil(v), "a non-nil %T is present", v) + } +} + +// TestRejectedPubSubConfigLeavesTheExportedFieldNil pins the root cause rather than the symptom. +// +// The getters filter a typed nil, but Container.PubSub is exported: ctx.Container.PubSub, and any +// future internal caller, reads it directly and gets no filtering at all. kafka.New and google.New +// both return a bare nil of their concrete type when they reject a config, and assigning that +// straight into the pubsub.Client interface is what manufactures the typed nil in the first place. +// +// Assigning only a real client means the bad value never exists, and the getters stay as defense in +// depth rather than as the only thing standing between a misconfiguration and a nil receiver. +// +// The plain == nil comparison is the point: assert.Nil reflects, so it would pass on a typed nil and +// this test would prove nothing. +func TestRejectedPubSubConfigLeavesTheExportedFieldNil(t *testing.T) { + tests := []struct { + desc string + create func(c *Container, conf config.Config) + conf map[string]string + }{ + { + desc: "google with no project id", + create: (*Container).createGooglePubSub, + conf: map[string]string{"PUBSUB_BACKEND": "GOOGLE"}, + }, + { + desc: "kafka with no broker", + create: (*Container).createKafkaPubSub, + conf: map[string]string{"PUBSUB_BACKEND": "KAFKA", "PUBSUB_BROKER": ""}, + }, + } + + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + c := &Container{Logger: logging.NewMockLogger(logging.ERROR)} + + tc.create(c, config.NewMockConfig(tc.conf)) + + if c.PubSub != nil { + t.Errorf("a rejected config must leave PubSub plainly nil, got a %T in the interface", c.PubSub) + } + }) + } +}