From 42d17ae4bba4fa3a7d3f9d155e7d41d3a88ed79c Mon Sep 17 00:00:00 2001 From: Neur0toxine Date: Thu, 26 Sep 2024 17:36:41 +0300 Subject: [PATCH] fix data loss for handler, connection, account --- core/logger/buffer_logger_test.go | 36 ++++++++++++++++++++-- core/logger/default.go | 51 ++++++++++++++++++++++++++++--- core/logger/default_test.go | 42 +++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 7 deletions(-) diff --git a/core/logger/buffer_logger_test.go b/core/logger/buffer_logger_test.go index 9a90db7..f21ca56 100644 --- a/core/logger/buffer_logger_test.go +++ b/core/logger/buffer_logger_test.go @@ -86,18 +86,48 @@ func (l *bufferLogger) WithLazy(fields ...zapcore.Field) Logger { } // ForHandler returns a new logger that is associated with the given handler. +// This will replace "handler" field if it was set before. +// Note: chain calls like ForHandler().With().ForHandler() will DUPLICATE handler field! func (l *bufferLogger) ForHandler(handler any) Logger { - return l.clone(l.parentOrCurrent().WithLazy(zap.Any(HandlerAttr, handler))) + if l.previous != previousFieldHandler { + result := l.With(zap.Any(HandlerAttr, handler)) + result.(*bufferLogger).setPrevious(previousFieldHandler) + result.(*bufferLogger).parent = l.Logger + return result + } + result := l.clone(l.parentOrCurrent().With(zap.Any(HandlerAttr, handler))) + result.(*bufferLogger).setPrevious(previousFieldHandler) + return result } // ForConnection returns a new logger that is associated with the given connection. +// This will replace "connection" field if it was set before. +// Note: chain calls like ForConnection().With().ForConnection() will DUPLICATE connection field! func (l *bufferLogger) ForConnection(conn any) Logger { - return l.clone(l.parentOrCurrent().WithLazy(zap.Any(ConnectionAttr, conn))) + if l.previous != previousFieldConnection { + result := l.With(zap.Any(ConnectionAttr, conn)) + result.(*bufferLogger).setPrevious(previousFieldConnection) + result.(*bufferLogger).parent = l.Logger + return result + } + result := l.clone(l.parentOrCurrent().With(zap.Any(ConnectionAttr, conn))) + result.(*bufferLogger).setPrevious(previousFieldConnection) + return result } // ForAccount returns a new logger that is associated with the given account. +// This will replace "account" field if it was set before. +// Note: chain calls like ForAccount().With().ForAccount() will DUPLICATE account field! func (l *bufferLogger) ForAccount(acc any) Logger { - return l.clone(l.parentOrCurrent().WithLazy(zap.Any(AccountAttr, acc))) + if l.previous != previousFieldAccount { + result := l.With(zap.Any(AccountAttr, acc)) + result.(*bufferLogger).setPrevious(previousFieldAccount) + result.(*bufferLogger).parent = l.Logger + return result + } + result := l.clone(l.parentOrCurrent().With(zap.Any(AccountAttr, acc))) + result.(*bufferLogger).setPrevious(previousFieldAccount) + return result } // Read bytes from the logger buffer. io.Reader implementation. diff --git a/core/logger/default.go b/core/logger/default.go index 6ac3b61..ace69dc 100644 --- a/core/logger/default.go +++ b/core/logger/default.go @@ -44,10 +44,19 @@ type Logger interface { Sync() error } +type previousField uint8 + +const ( + previousFieldHandler previousField = iota + 1 + previousFieldConnection + previousFieldAccount +) + // Default is a default logger implementation. type Default struct { *zap.Logger - parent *zap.Logger + parent *zap.Logger + previous previousField } // NewDefault creates a new default logger with the given format and debug level. @@ -68,18 +77,48 @@ func (l *Default) WithLazy(fields ...zap.Field) Logger { } // ForHandler returns a new logger that is associated with the given handler. +// This will replace "handler" field if it was set before. +// Note: chain calls like ForHandler().With().ForHandler() will DUPLICATE handler field! func (l *Default) ForHandler(handler any) Logger { - return l.clone(l.parentOrCurrent().WithLazy(zap.Any(HandlerAttr, handler))) + if l.previous != previousFieldHandler { + result := l.With(zap.Any(HandlerAttr, handler)) + result.(*Default).setPrevious(previousFieldHandler) + result.(*Default).parent = l.Logger + return result + } + result := l.clone(l.parentOrCurrent().With(zap.Any(HandlerAttr, handler))) + result.(*Default).setPrevious(previousFieldHandler) + return result } // ForConnection returns a new logger that is associated with the given connection. +// This will replace "connection" field if it was set before. +// Note: chain calls like ForConnection().With().ForConnection() will DUPLICATE connection field! func (l *Default) ForConnection(conn any) Logger { - return l.clone(l.parentOrCurrent().WithLazy(zap.Any(ConnectionAttr, conn))) + if l.previous != previousFieldConnection { + result := l.With(zap.Any(ConnectionAttr, conn)) + result.(*Default).setPrevious(previousFieldConnection) + result.(*Default).parent = l.Logger + return result + } + result := l.clone(l.parentOrCurrent().With(zap.Any(ConnectionAttr, conn))) + result.(*Default).setPrevious(previousFieldConnection) + return result } // ForAccount returns a new logger that is associated with the given account. +// This will replace "account" field if it was set before. +// Note: chain calls like ForAccount().With().ForAccount() will DUPLICATE account field! func (l *Default) ForAccount(acc any) Logger { - return l.clone(l.parentOrCurrent().WithLazy(zap.Any(AccountAttr, acc))) + if l.previous != previousFieldAccount { + result := l.With(zap.Any(AccountAttr, acc)) + result.(*Default).setPrevious(previousFieldAccount) + result.(*Default).parent = l.Logger + return result + } + result := l.clone(l.parentOrCurrent().With(zap.Any(AccountAttr, acc))) + result.(*Default).setPrevious(previousFieldAccount) + return result } // clone creates a copy of the given logger. @@ -99,6 +138,10 @@ func (l *Default) parentOrCurrent() *zap.Logger { return l.Logger } +func (l *Default) setPrevious(prev previousField) { + l.previous = prev +} + // AnyZapFields converts an array of values to zap fields. func AnyZapFields(args []interface{}) []zap.Field { fields := make([]zap.Field, len(args)) diff --git a/core/logger/default_test.go b/core/logger/default_test.go index 343c9dd..7a70b47 100644 --- a/core/logger/default_test.go +++ b/core/logger/default_test.go @@ -105,6 +105,48 @@ func (s *TestDefaultSuite) TestForAccountNoDuplicate() { s.Assert().NotContains(log.String(), "acc1") } +func (s *TestDefaultSuite) TestNoDuplicatesPersistRecords() { + log := newBufferLogger() + log. + ForHandler("handler1"). + ForHandler("handler2"). + ForConnection("conn1"). + ForConnection("conn2"). + ForAccount("acc1"). + ForAccount("acc2"). + Info("test") + + s.Assert().Contains(log.String(), "handler2") + s.Assert().NotContains(log.String(), "handler1") + s.Assert().Contains(log.String(), "conn2") + s.Assert().NotContains(log.String(), "conn1") + s.Assert().Contains(log.String(), "acc2") + s.Assert().NotContains(log.String(), "acc1") +} + +// TestPersistRecordsIncompatibleWith is not a unit test, but rather a demonstration how you shouldn't use For* methods. +func (s *TestDefaultSuite) TestPersistRecordsIncompatibleWith() { + log := newBufferLogger() + log. + ForHandler("handler1"). + With(zap.Int("f1", 1)). + ForHandler("handler2"). + ForConnection("conn1"). + With(zap.Int("f2", 2)). + ForConnection("conn2"). + ForAccount("acc1"). + With(zap.Int("f3", 3)). + ForAccount("acc2"). + Info("test") + + s.Assert().Contains(log.String(), "handler2") + s.Assert().Contains(log.String(), "handler1") + s.Assert().Contains(log.String(), "conn2") + s.Assert().Contains(log.String(), "conn1") + s.Assert().Contains(log.String(), "acc2") + s.Assert().Contains(log.String(), "acc1") +} + func TestAnyZapFields(t *testing.T) { fields := AnyZapFields([]interface{}{zap.String("k0", "v0"), "ooga", "booga"}) require.Len(t, fields, 3)