1
0
mirror of https://github.com/redis/go-redis.git synced 2025-07-20 22:42:59 +03:00

refactor: remove unnecessary enabled field and IsEnabled/SetEnabled methods

- Remove enabled field from PushNotificationProcessor struct
- Remove IsEnabled() and SetEnabled() methods from processor interface
- Remove enabled parameter from NewPushNotificationProcessor()
- Update all interfaces in pool package to remove IsEnabled requirement
- Simplify processor logic - if processor exists, it works
- VoidPushNotificationProcessor handles disabled case by discarding notifications
- Update all tests to use simplified interface without enable/disable logic

Benefits:
- Simpler, cleaner interface with less complexity
- No unnecessary state management for enabled/disabled
- VoidPushNotificationProcessor pattern handles disabled case elegantly
- Reduced cognitive overhead - processors just work when set
- Eliminates redundant enabled checks throughout codebase
- More predictable behavior - set processor = it works
This commit is contained in:
Nedyalko Dyakov
2025-06-27 01:18:15 +03:00
parent fdfcf94300
commit be9b6dd6a0
7 changed files with 57 additions and 99 deletions

View File

@ -28,7 +28,6 @@ type Conn struct {
// Push notification processor for handling push notifications on this connection // Push notification processor for handling push notifications on this connection
PushNotificationProcessor interface { PushNotificationProcessor interface {
IsEnabled() bool
ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error
} }
} }

View File

@ -75,7 +75,6 @@ type Options struct {
// Push notification processor for connections // Push notification processor for connections
PushNotificationProcessor interface { PushNotificationProcessor interface {
IsEnabled() bool
ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error
} }
} }
@ -391,7 +390,7 @@ func (p *ConnPool) popIdle() (*Conn, error) {
func (p *ConnPool) Put(ctx context.Context, cn *Conn) { func (p *ConnPool) Put(ctx context.Context, cn *Conn) {
if cn.rd.Buffered() > 0 { if cn.rd.Buffered() > 0 {
// Check if this might be push notification data // Check if this might be push notification data
if cn.PushNotificationProcessor != nil && cn.PushNotificationProcessor.IsEnabled() { if cn.PushNotificationProcessor != nil {
// Try to process pending push notifications before discarding connection // Try to process pending push notifications before discarding connection
err := cn.PushNotificationProcessor.ProcessPendingNotifications(ctx, cn.rd) err := cn.PushNotificationProcessor.ProcessPendingNotifications(ctx, cn.rd)
if err != nil { if err != nil {
@ -555,7 +554,7 @@ func (p *ConnPool) isHealthyConn(cn *Conn) bool {
if err := connCheck(cn.netConn); err != nil { if err := connCheck(cn.netConn); err != nil {
// If there's unexpected data and we have push notification support, // If there's unexpected data and we have push notification support,
// it might be push notifications // it might be push notifications
if err == errUnexpectedRead && cn.PushNotificationProcessor != nil && cn.PushNotificationProcessor.IsEnabled() { if err == errUnexpectedRead && cn.PushNotificationProcessor != nil {
// Try to process any pending push notifications // Try to process any pending push notifications
ctx := context.Background() ctx := context.Background()
if procErr := cn.PushNotificationProcessor.ProcessPendingNotifications(ctx, cn.rd); procErr != nil { if procErr := cn.PushNotificationProcessor.ProcessPendingNotifications(ctx, cn.rd); procErr != nil {

View File

@ -435,18 +435,16 @@ func (c *PubSub) newMessage(reply interface{}) (interface{}, error) {
}, nil }, nil
default: default:
// Try to handle as generic push notification // Try to handle as generic push notification
if c.pushProcessor.IsEnabled() { ctx := c.getContext()
ctx := c.getContext() registry := c.pushProcessor.GetRegistry()
registry := c.pushProcessor.GetRegistry() if registry != nil {
if registry != nil { handled := registry.HandleNotification(ctx, reply)
handled := registry.HandleNotification(ctx, reply) if handled {
if handled { // Return a special message type to indicate it was handled
// Return a special message type to indicate it was handled return &PushNotificationMessage{
return &PushNotificationMessage{ Command: kind,
Command: kind, Args: reply[1:],
Args: reply[1:], }, nil
}, nil
}
} }
} }
return nil, fmt.Errorf("redis: unsupported pubsub message: %q", kind) return nil, fmt.Errorf("redis: unsupported pubsub message: %q", kind)

View File

@ -56,9 +56,7 @@ func TestConnectionPoolPushNotificationIntegration(t *testing.T) {
t.Error("Connection should have push notification processor assigned") t.Error("Connection should have push notification processor assigned")
} }
if !cn.PushNotificationProcessor.IsEnabled() { // Connection should have a processor (no need to check IsEnabled anymore)
t.Error("Connection push notification processor should be enabled")
}
// Test ProcessPendingNotifications method // Test ProcessPendingNotifications method
emptyReader := proto.NewReader(bytes.NewReader([]byte{})) emptyReader := proto.NewReader(bytes.NewReader([]byte{}))
@ -156,8 +154,9 @@ func TestConnPushNotificationMethods(t *testing.T) {
t.Error("Conn should have push notification processor") t.Error("Conn should have push notification processor")
} }
if !processor.IsEnabled() { // Processor should have a registry when enabled
t.Error("Conn push notification processor should be enabled") if processor.GetRegistry() == nil {
t.Error("Conn push notification processor should have a registry when enabled")
} }
// Test RegisterPushNotificationHandler // Test RegisterPushNotificationHandler
@ -218,8 +217,9 @@ func TestConnWithoutPushNotifications(t *testing.T) {
if processor == nil { if processor == nil {
t.Error("Conn should always have a push notification processor") t.Error("Conn should always have a push notification processor")
} }
if processor.IsEnabled() { // VoidPushNotificationProcessor should have nil registry
t.Error("Push notification processor should be disabled for RESP2") if processor.GetRegistry() != nil {
t.Error("VoidPushNotificationProcessor should have nil registry for RESP2")
} }
// Test RegisterPushNotificationHandler returns nil (no error) // Test RegisterPushNotificationHandler returns nil (no error)
@ -242,7 +242,7 @@ func TestConnWithoutPushNotifications(t *testing.T) {
// TestNewConnWithCustomProcessor tests newConn with custom processor in options. // TestNewConnWithCustomProcessor tests newConn with custom processor in options.
func TestNewConnWithCustomProcessor(t *testing.T) { func TestNewConnWithCustomProcessor(t *testing.T) {
// Create custom processor // Create custom processor
customProcessor := NewPushNotificationProcessor(true) customProcessor := NewPushNotificationProcessor()
// Create options with custom processor // Create options with custom processor
opt := &Options{ opt := &Options{
@ -377,7 +377,7 @@ func TestPushNotificationInfoStructure(t *testing.T) {
// TestConnectionPoolOptionsIntegration tests that pool options correctly include processor. // TestConnectionPoolOptionsIntegration tests that pool options correctly include processor.
func TestConnectionPoolOptionsIntegration(t *testing.T) { func TestConnectionPoolOptionsIntegration(t *testing.T) {
// Create processor // Create processor
processor := NewPushNotificationProcessor(true) processor := NewPushNotificationProcessor()
// Create options // Create options
opt := &Options{ opt := &Options{
@ -401,7 +401,7 @@ func TestConnectionPoolOptionsIntegration(t *testing.T) {
// TestProcessPendingNotificationsEdgeCases tests edge cases in ProcessPendingNotifications. // TestProcessPendingNotificationsEdgeCases tests edge cases in ProcessPendingNotifications.
func TestProcessPendingNotificationsEdgeCases(t *testing.T) { func TestProcessPendingNotificationsEdgeCases(t *testing.T) {
processor := NewPushNotificationProcessor(true) processor := NewPushNotificationProcessor()
ctx := context.Background() ctx := context.Background()
// Test with nil reader (should not panic) // Test with nil reader (should not panic)
@ -417,10 +417,10 @@ func TestProcessPendingNotificationsEdgeCases(t *testing.T) {
t.Errorf("Should not error with empty reader: %v", err) t.Errorf("Should not error with empty reader: %v", err)
} }
// Test with disabled processor // Test with void processor (simulates disabled state)
disabledProcessor := NewPushNotificationProcessor(false) voidProcessor := NewVoidPushNotificationProcessor()
err = disabledProcessor.ProcessPendingNotifications(ctx, emptyReader) err = voidProcessor.ProcessPendingNotifications(ctx, emptyReader)
if err != nil { if err != nil {
t.Errorf("Disabled processor should not error: %v", err) t.Errorf("Void processor should not error: %v", err)
} }
} }

View File

@ -106,8 +106,6 @@ func (r *PushNotificationRegistry) HasHandlers() bool {
// PushNotificationProcessorInterface defines the interface for push notification processors. // PushNotificationProcessorInterface defines the interface for push notification processors.
type PushNotificationProcessorInterface interface { type PushNotificationProcessorInterface interface {
IsEnabled() bool
SetEnabled(enabled bool)
GetRegistry() *PushNotificationRegistry GetRegistry() *PushNotificationRegistry
ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error
RegisterHandler(pushNotificationName string, handler PushNotificationHandler, protected bool) error RegisterHandler(pushNotificationName string, handler PushNotificationHandler, protected bool) error
@ -116,32 +114,15 @@ type PushNotificationProcessorInterface interface {
// PushNotificationProcessor handles the processing of push notifications from Redis. // PushNotificationProcessor handles the processing of push notifications from Redis.
type PushNotificationProcessor struct { type PushNotificationProcessor struct {
registry *PushNotificationRegistry registry *PushNotificationRegistry
enabled bool
mu sync.RWMutex // Protects enabled field
} }
// NewPushNotificationProcessor creates a new push notification processor. // NewPushNotificationProcessor creates a new push notification processor.
func NewPushNotificationProcessor(enabled bool) *PushNotificationProcessor { func NewPushNotificationProcessor() *PushNotificationProcessor {
return &PushNotificationProcessor{ return &PushNotificationProcessor{
registry: NewPushNotificationRegistry(), registry: NewPushNotificationRegistry(),
enabled: enabled,
} }
} }
// IsEnabled returns whether push notification processing is enabled.
func (p *PushNotificationProcessor) IsEnabled() bool {
p.mu.RLock()
defer p.mu.RUnlock()
return p.enabled
}
// SetEnabled enables or disables push notification processing.
func (p *PushNotificationProcessor) SetEnabled(enabled bool) {
p.mu.Lock()
defer p.mu.Unlock()
p.enabled = enabled
}
// GetRegistry returns the push notification registry. // GetRegistry returns the push notification registry.
func (p *PushNotificationProcessor) GetRegistry() *PushNotificationRegistry { func (p *PushNotificationProcessor) GetRegistry() *PushNotificationRegistry {
return p.registry return p.registry
@ -149,7 +130,7 @@ func (p *PushNotificationProcessor) GetRegistry() *PushNotificationRegistry {
// ProcessPendingNotifications checks for and processes any pending push notifications. // ProcessPendingNotifications checks for and processes any pending push notifications.
func (p *PushNotificationProcessor) ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error { func (p *PushNotificationProcessor) ProcessPendingNotifications(ctx context.Context, rd *proto.Reader) error {
if !p.IsEnabled() || !p.registry.HasHandlers() { if !p.registry.HasHandlers() {
return nil return nil
} }
@ -252,16 +233,6 @@ func NewVoidPushNotificationProcessor() *VoidPushNotificationProcessor {
return &VoidPushNotificationProcessor{} return &VoidPushNotificationProcessor{}
} }
// IsEnabled always returns false for void processor.
func (v *VoidPushNotificationProcessor) IsEnabled() bool {
return false
}
// SetEnabled is a no-op for void processor.
func (v *VoidPushNotificationProcessor) SetEnabled(enabled bool) {
// No-op: void processor is always disabled
}
// GetRegistry returns nil for void processor since it doesn't maintain handlers. // GetRegistry returns nil for void processor since it doesn't maintain handlers.
func (v *VoidPushNotificationProcessor) GetRegistry() *PushNotificationRegistry { func (v *VoidPushNotificationProcessor) GetRegistry() *PushNotificationRegistry {
return nil return nil

View File

@ -87,10 +87,10 @@ func TestPushNotificationRegistry(t *testing.T) {
func TestPushNotificationProcessor(t *testing.T) { func TestPushNotificationProcessor(t *testing.T) {
// Test the push notification processor // Test the push notification processor
processor := redis.NewPushNotificationProcessor(true) processor := redis.NewPushNotificationProcessor()
if !processor.IsEnabled() { if processor.GetRegistry() == nil {
t.Error("Processor should be enabled") t.Error("Processor should have a registry")
} }
// Test registering handlers // Test registering handlers
@ -124,10 +124,9 @@ func TestPushNotificationProcessor(t *testing.T) {
t.Error("Specific handler should have been called") t.Error("Specific handler should have been called")
} }
// Test disabling processor // Test that processor always has a registry (no enable/disable anymore)
processor.SetEnabled(false) if processor.GetRegistry() == nil {
if processor.IsEnabled() { t.Error("Processor should always have a registry")
t.Error("Processor should be disabled")
} }
} }
@ -146,8 +145,8 @@ func TestClientPushNotificationIntegration(t *testing.T) {
t.Error("Push notification processor should be initialized") t.Error("Push notification processor should be initialized")
} }
if !processor.IsEnabled() { if processor.GetRegistry() == nil {
t.Error("Push notification processor should be enabled") t.Error("Push notification processor should have a registry when enabled")
} }
// Test registering handlers through client // Test registering handlers through client
@ -187,8 +186,9 @@ func TestClientWithoutPushNotifications(t *testing.T) {
if processor == nil { if processor == nil {
t.Error("Push notification processor should never be nil") t.Error("Push notification processor should never be nil")
} }
if processor.IsEnabled() { // VoidPushNotificationProcessor should have nil registry
t.Error("Push notification processor should be disabled when PushNotifications is false") if processor.GetRegistry() != nil {
t.Error("VoidPushNotificationProcessor should have nil registry")
} }
// Registering handlers should not panic // Registering handlers should not panic
@ -215,8 +215,8 @@ func TestPushNotificationEnabledClient(t *testing.T) {
t.Error("Push notification processor should be initialized when enabled") t.Error("Push notification processor should be initialized when enabled")
} }
if !processor.IsEnabled() { if processor.GetRegistry() == nil {
t.Error("Push notification processor should be enabled") t.Error("Push notification processor should have a registry when enabled")
} }
// Test registering a handler // Test registering a handler
@ -561,10 +561,10 @@ func TestPushNotificationRegistrySpecificHandlerOnly(t *testing.T) {
func TestPushNotificationProcessorEdgeCases(t *testing.T) { func TestPushNotificationProcessorEdgeCases(t *testing.T) {
// Test processor with disabled state // Test processor with disabled state
processor := redis.NewPushNotificationProcessor(false) processor := redis.NewPushNotificationProcessor()
if processor.IsEnabled() { if processor.GetRegistry() == nil {
t.Error("Processor should be disabled") t.Error("Processor should have a registry")
} }
// Test that disabled processor doesn't process notifications // Test that disabled processor doesn't process notifications
@ -587,15 +587,14 @@ func TestPushNotificationProcessorEdgeCases(t *testing.T) {
t.Error("Handler should be called when using registry directly") t.Error("Handler should be called when using registry directly")
} }
// Test enabling processor // Test that processor always has a registry
processor.SetEnabled(true) if processor.GetRegistry() == nil {
if !processor.IsEnabled() { t.Error("Processor should always have a registry")
t.Error("Processor should be enabled after SetEnabled(true)")
} }
} }
func TestPushNotificationProcessorConvenienceMethods(t *testing.T) { func TestPushNotificationProcessorConvenienceMethods(t *testing.T) {
processor := redis.NewPushNotificationProcessor(true) processor := redis.NewPushNotificationProcessor()
// Test RegisterHandler convenience method // Test RegisterHandler convenience method
handlerCalled := false handlerCalled := false
@ -822,7 +821,7 @@ func TestPushNotificationRegistryConcurrency(t *testing.T) {
func TestPushNotificationProcessorConcurrency(t *testing.T) { func TestPushNotificationProcessorConcurrency(t *testing.T) {
// Test thread safety of the processor // Test thread safety of the processor
processor := redis.NewPushNotificationProcessor(true) processor := redis.NewPushNotificationProcessor()
numGoroutines := 5 numGoroutines := 5
numOperations := 50 numOperations := 50
@ -845,13 +844,7 @@ func TestPushNotificationProcessorConcurrency(t *testing.T) {
notification := []interface{}{command, "data"} notification := []interface{}{command, "data"}
processor.GetRegistry().HandleNotification(context.Background(), notification) processor.GetRegistry().HandleNotification(context.Background(), notification)
// Toggle processor state occasionally
if j%20 == 0 {
processor.SetEnabled(!processor.IsEnabled())
}
// Access processor state // Access processor state
processor.IsEnabled()
processor.GetRegistry() processor.GetRegistry()
} }
}(i) }(i)
@ -898,7 +891,7 @@ func TestPushNotificationClientConcurrency(t *testing.T) {
// Access processor // Access processor
processor := client.GetPushNotificationProcessor() processor := client.GetPushNotificationProcessor()
if processor != nil { if processor != nil {
processor.IsEnabled() processor.GetRegistry()
} }
} }
}(i) }(i)
@ -929,8 +922,11 @@ func TestPushNotificationConnectionHealthCheck(t *testing.T) {
// Verify push notifications are enabled // Verify push notifications are enabled
processor := client.GetPushNotificationProcessor() processor := client.GetPushNotificationProcessor()
if processor == nil || !processor.IsEnabled() { if processor == nil {
t.Fatal("Push notifications should be enabled") t.Fatal("Push notification processor should not be nil")
}
if processor.GetRegistry() == nil {
t.Fatal("Push notification registry should not be nil when enabled")
} }
// Register a handler for testing // Register a handler for testing
@ -959,11 +955,6 @@ func TestPushNotificationConnectionHealthCheck(t *testing.T) {
return return
} }
if !cn.PushNotificationProcessor.IsEnabled() {
t.Error("Push notification processor should be enabled on connection")
return
}
t.Log("✅ Connection has push notification processor correctly set") t.Log("✅ Connection has push notification processor correctly set")
t.Log("✅ Connection health check integration working correctly") t.Log("✅ Connection health check integration working correctly")
} }

View File

@ -535,7 +535,7 @@ func (c *baseClient) _process(ctx context.Context, cmd Cmder, attempt int) (bool
} }
if err := cn.WithReader(c.context(ctx), c.cmdTimeout(cmd), func(rd *proto.Reader) error { if err := cn.WithReader(c.context(ctx), c.cmdTimeout(cmd), func(rd *proto.Reader) error {
// Check for push notifications before reading the command reply // Check for push notifications before reading the command reply
if c.opt.Protocol == 3 && c.pushProcessor.IsEnabled() { if c.opt.Protocol == 3 {
if err := c.pushProcessor.ProcessPendingNotifications(ctx, rd); err != nil { if err := c.pushProcessor.ProcessPendingNotifications(ctx, rd); err != nil {
internal.Logger.Printf(ctx, "push: error processing push notifications: %v", err) internal.Logger.Printf(ctx, "push: error processing push notifications: %v", err)
} }
@ -818,7 +818,7 @@ func (c *Client) initializePushProcessor() {
c.pushProcessor = c.opt.PushNotificationProcessor c.pushProcessor = c.opt.PushNotificationProcessor
} else if c.opt.PushNotifications { } else if c.opt.PushNotifications {
// Create default processor when push notifications are enabled // Create default processor when push notifications are enabled
c.pushProcessor = NewPushNotificationProcessor(true) c.pushProcessor = NewPushNotificationProcessor()
} else { } else {
// Create void processor when push notifications are disabled // Create void processor when push notifications are disabled
c.pushProcessor = NewVoidPushNotificationProcessor() c.pushProcessor = NewVoidPushNotificationProcessor()