Skip to content

Commit fdfcf94

Browse files
committed
feat: add VoidPushNotificationProcessor for disabled push notifications
- Add VoidPushNotificationProcessor that reads and discards push notifications - Create PushNotificationProcessorInterface for consistent behavior - Always provide a processor (real or void) instead of nil - VoidPushNotificationProcessor properly cleans RESP3 push notifications from buffer - Remove all nil checks throughout codebase for cleaner, safer code - Update tests to expect VoidPushNotificationProcessor when disabled Benefits: - Eliminates nil pointer risks throughout the codebase - Follows null object pattern for safer operation - Properly handles RESP3 push notifications even when disabled - Consistent interface regardless of push notification settings - Cleaner code without defensive nil checks everywhere
1 parent c33b157 commit fdfcf94

File tree

6 files changed

+109
-41
lines changed

6 files changed

+109
-41
lines changed

options.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ type Options struct {
230230

231231
// PushNotificationProcessor is the processor for handling push notifications.
232232
// If nil, a default processor will be created when PushNotifications is enabled.
233-
PushNotificationProcessor *PushNotificationProcessor
233+
PushNotificationProcessor PushNotificationProcessorInterface
234234
}
235235

236236
func (opt *Options) init() {

pubsub.go

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ type PubSub struct {
4040
allCh *channel
4141

4242
// Push notification processor for handling generic push notifications
43-
pushProcessor *PushNotificationProcessor
43+
pushProcessor PushNotificationProcessorInterface
4444
}
4545

4646
func (c *PubSub) init() {
@@ -49,7 +49,7 @@ func (c *PubSub) init() {
4949

5050
// SetPushNotificationProcessor sets the push notification processor for handling
5151
// generic push notifications received on this PubSub connection.
52-
func (c *PubSub) SetPushNotificationProcessor(processor *PushNotificationProcessor) {
52+
func (c *PubSub) SetPushNotificationProcessor(processor PushNotificationProcessorInterface) {
5353
c.pushProcessor = processor
5454
}
5555

@@ -435,15 +435,18 @@ func (c *PubSub) newMessage(reply interface{}) (interface{}, error) {
435435
}, nil
436436
default:
437437
// Try to handle as generic push notification
438-
if c.pushProcessor != nil && c.pushProcessor.IsEnabled() {
438+
if c.pushProcessor.IsEnabled() {
439439
ctx := c.getContext()
440-
handled := c.pushProcessor.GetRegistry().HandleNotification(ctx, reply)
441-
if handled {
442-
// Return a special message type to indicate it was handled
443-
return &PushNotificationMessage{
444-
Command: kind,
445-
Args: reply[1:],
446-
}, nil
440+
registry := c.pushProcessor.GetRegistry()
441+
if registry != nil {
442+
handled := registry.HandleNotification(ctx, reply)
443+
if handled {
444+
// Return a special message type to indicate it was handled
445+
return &PushNotificationMessage{
446+
Command: kind,
447+
Args: reply[1:],
448+
}, nil
449+
}
447450
}
448451
}
449452
return nil, fmt.Errorf("redis: unsupported pubsub message: %q", kind)

push_notification_coverage_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,10 +213,13 @@ func TestConnWithoutPushNotifications(t *testing.T) {
213213
conn := client.Conn()
214214
defer conn.Close()
215215

216-
// Test GetPushNotificationProcessor returns nil
216+
// Test GetPushNotificationProcessor returns VoidPushNotificationProcessor
217217
processor := conn.GetPushNotificationProcessor()
218-
if processor != nil {
219-
t.Error("Conn should not have push notification processor for RESP2")
218+
if processor == nil {
219+
t.Error("Conn should always have a push notification processor")
220+
}
221+
if processor.IsEnabled() {
222+
t.Error("Push notification processor should be disabled for RESP2")
220223
}
221224

222225
// Test RegisterPushNotificationHandler returns nil (no error)

push_notifications.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,15 @@ func (r *PushNotificationRegistry) HasHandlers() bool {
104104
return len(r.handlers) > 0
105105
}
106106

107+
// PushNotificationProcessorInterface defines the interface for push notification processors.
108+
type PushNotificationProcessorInterface interface {
109+
IsEnabled() bool
110+
SetEnabled(enabled bool)
111+
GetRegistry() *PushNotificationRegistry
112+
ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error
113+
RegisterHandler(pushNotificationName string, handler PushNotificationHandler, protected bool) error
114+
}
115+
107116
// PushNotificationProcessor handles the processing of push notifications from Redis.
108117
type PushNotificationProcessor struct {
109118
registry *PushNotificationRegistry
@@ -233,3 +242,62 @@ func (info *PushNotificationInfo) String() string {
233242
}
234243
return info.Name
235244
}
245+
246+
// VoidPushNotificationProcessor is a no-op processor that discards all push notifications.
247+
// Used when push notifications are disabled to avoid nil checks throughout the codebase.
248+
type VoidPushNotificationProcessor struct{}
249+
250+
// NewVoidPushNotificationProcessor creates a new void push notification processor.
251+
func NewVoidPushNotificationProcessor() *VoidPushNotificationProcessor {
252+
return &VoidPushNotificationProcessor{}
253+
}
254+
255+
// IsEnabled always returns false for void processor.
256+
func (v *VoidPushNotificationProcessor) IsEnabled() bool {
257+
return false
258+
}
259+
260+
// SetEnabled is a no-op for void processor.
261+
func (v *VoidPushNotificationProcessor) SetEnabled(enabled bool) {
262+
// No-op: void processor is always disabled
263+
}
264+
265+
// GetRegistry returns nil for void processor since it doesn't maintain handlers.
266+
func (v *VoidPushNotificationProcessor) GetRegistry() *PushNotificationRegistry {
267+
return nil
268+
}
269+
270+
// ProcessPendingNotifications reads and discards any pending push notifications.
271+
func (v *VoidPushNotificationProcessor) ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error {
272+
// Read and discard any pending push notifications to clean the buffer
273+
for {
274+
// Peek at the next reply type to see if it's a push notification
275+
replyType, err := rd.PeekReplyType()
276+
if err != nil {
277+
// No more data available or error peeking
278+
break
279+
}
280+
281+
// Check if this is a RESP3 push notification
282+
if replyType == '>' { // RespPush
283+
// Read and discard the push notification
284+
_, err := rd.ReadReply()
285+
if err != nil {
286+
internal.Logger.Printf(ctx, "push: error reading push notification to discard: %v", err)
287+
break
288+
}
289+
// Continue to check for more push notifications
290+
} else {
291+
// Not a push notification, stop processing
292+
break
293+
}
294+
}
295+
296+
return nil
297+
}
298+
299+
// RegisterHandler is a no-op for void processor, always returns nil.
300+
func (v *VoidPushNotificationProcessor) RegisterHandler(pushNotificationName string, handler PushNotificationHandler, protected bool) error {
301+
// No-op: void processor doesn't register handlers
302+
return nil
303+
}

push_notifications_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,10 +182,13 @@ func TestClientWithoutPushNotifications(t *testing.T) {
182182
})
183183
defer client.Close()
184184

185-
// Push processor should be nil
185+
// Push processor should be a VoidPushNotificationProcessor
186186
processor := client.GetPushNotificationProcessor()
187-
if processor != nil {
188-
t.Error("Push notification processor should be nil when disabled")
187+
if processor == nil {
188+
t.Error("Push notification processor should never be nil")
189+
}
190+
if processor.IsEnabled() {
191+
t.Error("Push notification processor should be disabled when PushNotifications is false")
189192
}
190193

191194
// Registering handlers should not panic

redis.go

Lines changed: 15 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ type baseClient struct {
209209
onClose func() error // hook called when client is closed
210210

211211
// Push notification processing
212-
pushProcessor *PushNotificationProcessor
212+
pushProcessor PushNotificationProcessorInterface
213213
}
214214

215215
func (c *baseClient) clone() *baseClient {
@@ -535,7 +535,7 @@ func (c *baseClient) _process(ctx context.Context, cmd Cmder, attempt int) (bool
535535
}
536536
if err := cn.WithReader(c.context(ctx), c.cmdTimeout(cmd), func(rd *proto.Reader) error {
537537
// Check for push notifications before reading the command reply
538-
if c.opt.Protocol == 3 && c.pushProcessor != nil && c.pushProcessor.IsEnabled() {
538+
if c.opt.Protocol == 3 && c.pushProcessor.IsEnabled() {
539539
if err := c.pushProcessor.ProcessPendingNotifications(ctx, rd); err != nil {
540540
internal.Logger.Printf(ctx, "push: error processing push notifications: %v", err)
541541
}
@@ -772,9 +772,7 @@ func NewClient(opt *Options) *Client {
772772
c.initializePushProcessor()
773773

774774
// Update options with the initialized push processor for connection pool
775-
if c.pushProcessor != nil {
776-
opt.PushNotificationProcessor = c.pushProcessor
777-
}
775+
opt.PushNotificationProcessor = c.pushProcessor
778776

779777
c.connPool = newConnPool(opt, c.dialHook)
780778

@@ -819,23 +817,23 @@ func (c *Client) initializePushProcessor() {
819817
if c.opt.PushNotificationProcessor != nil {
820818
c.pushProcessor = c.opt.PushNotificationProcessor
821819
} else if c.opt.PushNotifications {
822-
// Create default processor only if push notifications are enabled
820+
// Create default processor when push notifications are enabled
823821
c.pushProcessor = NewPushNotificationProcessor(true)
822+
} else {
823+
// Create void processor when push notifications are disabled
824+
c.pushProcessor = NewVoidPushNotificationProcessor()
824825
}
825826
}
826827

827828
// RegisterPushNotificationHandler registers a handler for a specific push notification name.
828829
// Returns an error if a handler is already registered for this push notification name.
829830
// If protected is true, the handler cannot be unregistered.
830831
func (c *Client) RegisterPushNotificationHandler(pushNotificationName string, handler PushNotificationHandler, protected bool) error {
831-
if c.pushProcessor != nil {
832-
return c.pushProcessor.RegisterHandler(pushNotificationName, handler, protected)
833-
}
834-
return nil
832+
return c.pushProcessor.RegisterHandler(pushNotificationName, handler, protected)
835833
}
836834

837835
// GetPushNotificationProcessor returns the push notification processor.
838-
func (c *Client) GetPushNotificationProcessor() *PushNotificationProcessor {
836+
func (c *Client) GetPushNotificationProcessor() PushNotificationProcessorInterface {
839837
return c.pushProcessor
840838
}
841839

@@ -886,10 +884,8 @@ func (c *Client) pubSub() *PubSub {
886884
}
887885
pubsub.init()
888886

889-
// Set the push notification processor if available
890-
if c.pushProcessor != nil {
891-
pubsub.SetPushNotificationProcessor(c.pushProcessor)
892-
}
887+
// Set the push notification processor
888+
pubsub.SetPushNotificationProcessor(c.pushProcessor)
893889

894890
return pubsub
895891
}
@@ -974,10 +970,8 @@ func newConn(opt *Options, connPool pool.Pooler, parentHooks *hooksMixin) *Conn
974970
c.hooksMixin = parentHooks.clone()
975971
}
976972

977-
// Set push notification processor if available in options
978-
if opt.PushNotificationProcessor != nil {
979-
c.pushProcessor = opt.PushNotificationProcessor
980-
}
973+
// Set push notification processor from options (always available now)
974+
c.pushProcessor = opt.PushNotificationProcessor
981975

982976
c.cmdable = c.Process
983977
c.statefulCmdable = c.Process
@@ -1001,14 +995,11 @@ func (c *Conn) Process(ctx context.Context, cmd Cmder) error {
1001995
// Returns an error if a handler is already registered for this push notification name.
1002996
// If protected is true, the handler cannot be unregistered.
1003997
func (c *Conn) RegisterPushNotificationHandler(pushNotificationName string, handler PushNotificationHandler, protected bool) error {
1004-
if c.pushProcessor != nil {
1005-
return c.pushProcessor.RegisterHandler(pushNotificationName, handler, protected)
1006-
}
1007-
return nil
998+
return c.pushProcessor.RegisterHandler(pushNotificationName, handler, protected)
1008999
}
10091000

10101001
// GetPushNotificationProcessor returns the push notification processor.
1011-
func (c *Conn) GetPushNotificationProcessor() *PushNotificationProcessor {
1002+
func (c *Conn) GetPushNotificationProcessor() PushNotificationProcessorInterface {
10121003
return c.pushProcessor
10131004
}
10141005

0 commit comments

Comments
 (0)