From 2b359631a1fabe7800806ae2b830a32f8bace57e Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 5 Sep 2023 17:31:32 -0700 Subject: [PATCH] lint: Enable errcheck, fix failures (#1345) This enables linting with errcheck on the repository. Exclusions were added for functions that are known to never fail, e.g. all Write methods on Zap's buffer.Buffer. In attempting to enable exclusions for these functions, I discovered and fixed a typo in the golangci.yml.
Complete list of issues fixed ``` encoder_test.go:44:18: Error return value is not checked (errcheck) encoder_test.go:55:18: Error return value is not checked (errcheck) error.go:64:19: Error return value of `arr.AppendObject` is not checked (errcheck) http_handler.go:83:13: Error return value of `enc.Encode` is not checked (errcheck) http_handler.go:88:14: Error return value of `enc.Encode` is not checked (errcheck) http_handler.go:92:13: Error return value of `enc.Encode` is not checked (errcheck) http_handler.go:95:13: Error return value of `enc.Encode` is not checked (errcheck) http_handler_test.go:170:24: Error return value of `res.Body.Close` is not checked (errcheck) logger.go:377:24: Error return value of `log.errorOutput.Sync` is not checked (errcheck) sink.go:69:17: Error return value of `sr.RegisterSink` is not checked (errcheck) stacktrace_ext_test.go:178:13: Error return value of `os.MkdirAll` is not checked (errcheck) writer.go:65:11: Error return value of `c.Close` is not checked (errcheck) writer_test.go:94:11: Error return value of `os.Remove` is not checked (errcheck) writer_test.go:258:9: Error return value of `w.Write` is not checked (errcheck) zapcore/buffered_write_syncer_bench_test.go:43:15: Error return value of `w.Stop` is not checked (errcheck) zapcore/buffered_write_syncer_bench_test.go:47:12: Error return value of `w.Write` is not checked (errcheck) zapcore/buffered_write_syncer_test.go:104:11: Error return value of `ws.Write` is not checked (errcheck) zapcore/core.go:107:9: Error return value of `c.Sync` is not checked (errcheck) zapcore/core_test.go:151:13: Error return value of `core.Write` is not checked (errcheck) zapcore/encoder_test.go:288:17: Error return value of `enc.AddArray` is not checked (errcheck) zapcore/encoder_test.go:316:17: Error return value of `enc.AddArray` is not checked (errcheck) zapcore/encoder_test.go:723:14: Error return value of `mem.AddArray` is not checked (errcheck) zapcore/entry.go:245:23: Error return value of `ce.ErrorOutput.Sync` is not checked (errcheck) zapcore/entry.go:257:22: Error return value of `ce.ErrorOutput.Sync` is not checked (errcheck) zapcore/error.go:101:19: Error return value of `arr.AppendObject` is not checked (errcheck) zapcore/json_encoder_bench_test.go:34:16: Error return value of `enc.AddObject` is not checked (errcheck) zapcore/json_encoder_bench_test.go:98:16: Error return value of `json.Marshal` is not checked (errcheck) zapcore/json_encoder_impl_test.go:252:16: Error return value of `e.AddObject` is not checked (errcheck) zapcore/json_encoder_impl_test.go:260:16: Error return value of `e.AddObject` is not checked (errcheck) zapcore/json_encoder_impl_test.go:268:16: Error return value of `e.AddObject` is not checked (errcheck) zapcore/json_encoder_impl_test.go:292:13: Error return value of `e.AddArray` is not checked (errcheck) zapcore/json_encoder_impl_test.go:423:21: Error return value of `arr.AppendObject` is not checked (errcheck) zapcore/json_encoder_impl_test.go:431:21: Error return value of `arr.AppendObject` is not checked (errcheck) zapcore/json_encoder_impl_test.go:533:20: Error return value of `arr.AppendObject` is not checked (errcheck) zapcore/level_test.go:162:16: Error return value of `l.MarshalText` is not checked (errcheck) zapcore/memory_encoder_test.go:221:16: Error return value of `e.AddObject` is not checked (errcheck) zapcore/memory_encoder_test.go:235:16: Error return value of `e.AddObject` is not checked (errcheck) zapcore/memory_encoder_test.go:288:54: Error return value of `e.AppendReflected` is not checked (errcheck) zapcore/memory_encoder_test.go:294:18: Error return value of `e.AppendArray` is not checked (errcheck) zapcore/memory_encoder_test.go:305:18: Error return value of `e.AppendArray` is not checked (errcheck) zapcore/memory_encoder_test.go:306:24: Error return value of `inner.AppendObject` is not checked (errcheck) zapcore/memory_encoder_test.go:321:18: Error return value of `e.AppendArray` is not checked (errcheck) zapcore/memory_encoder_test.go:322:24: Error return value of `inner.AppendObject` is not checked (errcheck) zapcore/tee_test.go:123:13: Error return value of `tee.Write` is not checked (errcheck) zapcore/write_syncer_bench_test.go:40:12: Error return value of `w.Write` is not checked (errcheck) zapcore/write_syncer_bench_test.go:54:12: Error return value of `w.Write` is not checked (errcheck) zapcore/write_syncer_bench_test.go:67:15: Error return value of `w.Stop` is not checked (errcheck) zapcore/write_syncer_bench_test.go:71:12: Error return value of `w.Write` is not checked (errcheck) zapcore/write_syncer_bench_test.go:86:12: Error return value of `w.Write` is not checked (errcheck) zapcore/write_syncer_test.go:73:9: Error return value of `ws.Sync` is not checked (errcheck) zaptest/observer/observer_test.go:176:15: Error return value of `logger.Write` is not checked (errcheck) ```
--- .golangci.yml | 28 ++++++++++- benchmarks/scenario_bench_test.go | 18 +++---- encoder_test.go | 4 +- error.go | 5 +- error_test.go | 36 ++++++++++++++ http_handler.go | 19 ++++--- http_handler_test.go | 29 ++++++++++- logger.go | 2 +- sink.go | 5 +- stacktrace_ext_test.go | 4 +- writer.go | 2 +- writer_test.go | 4 +- zapcore/buffered_write_syncer_bench_test.go | 8 ++- zapcore/buffered_write_syncer_test.go | 3 +- zapcore/core.go | 6 +-- zapcore/core_test.go | 2 +- zapcore/encoder_test.go | 9 ++-- zapcore/entry.go | 4 +- zapcore/entry_ext_test.go | 55 +++++++++++++++++++++ zapcore/error.go | 5 +- zapcore/error_test.go | 47 ++++++++++++++++++ zapcore/json_encoder_bench_test.go | 9 +++- zapcore/json_encoder_impl_test.go | 19 ++++--- zapcore/level_test.go | 2 +- zapcore/memory_encoder_test.go | 28 ++++++----- zapcore/tee_test.go | 2 +- zapcore/write_syncer_bench_test.go | 21 ++++++-- zapcore/write_syncer_test.go | 2 +- zaptest/observer/observer_test.go | 2 +- 29 files changed, 309 insertions(+), 71 deletions(-) create mode 100644 zapcore/entry_ext_test.go diff --git a/.golangci.yml b/.golangci.yml index 154951275..fbc6df790 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -9,7 +9,7 @@ linters: disable-all: true enable: # golangci-lint defaults: - # - errcheck TODO: enable errcheck + - errcheck - gosimple - govet - ineffassign @@ -21,7 +21,7 @@ linters: - nolintlint # lints nolint directives - revive -linter-settings: +linters-settings: govet: # These govet checks are disabled by default, but they're useful. enable: @@ -30,6 +30,24 @@ linter-settings: - sortslice - unusedwrite + errcheck: + exclude-functions: + # These methods can not fail. + # They operate on an in-memory buffer. + - (*go.uber.org/zap/buffer.Buffer).Write + - (*go.uber.org/zap/buffer.Buffer).WriteByte + - (*go.uber.org/zap/buffer.Buffer).WriteString + + - (*go.uber.org/zap/zapio.Writer).Close + - (*go.uber.org/zap/zapio.Writer).Sync + - (*go.uber.org/zap/zapio.Writer).Write + # Write to zapio.Writer cannot fail, + # so io.WriteString on it cannot fail. + - io.WriteString(*go.uber.org/zap/zapio.Writer) + + # Writing a plain string to a fmt.State cannot fail. + - io.WriteString(fmt.State) + issues: # Print all issues reported by all linters. max-issues-per-linter: 0 @@ -51,3 +69,9 @@ issues: # for foo() { } - linters: [revive] text: 'empty-block: this block is empty, you can remove it' + + # Ignore logger.Sync() errcheck failures in example_test.go + # since those are intended to be uncomplicated examples. + - linters: [errcheck] + path: example_test.go + text: 'Error return value of `logger.Sync` is not checked' diff --git a/benchmarks/scenario_bench_test.go b/benchmarks/scenario_bench_test.go index b41b1b50e..4b867fa91 100644 --- a/benchmarks/scenario_bench_test.go +++ b/benchmarks/scenario_bench_test.go @@ -104,7 +104,6 @@ func BenchmarkDisabledWithoutFields(b *testing.B) { } }) }) - } func BenchmarkDisabledAccumulatedContext(b *testing.B) { @@ -183,7 +182,6 @@ func BenchmarkDisabledAccumulatedContext(b *testing.B) { } }) }) - } func BenchmarkDisabledAddingFields(b *testing.B) { @@ -253,7 +251,6 @@ func BenchmarkDisabledAddingFields(b *testing.B) { } }) }) - } func BenchmarkWithoutFields(b *testing.B) { @@ -323,7 +320,9 @@ func BenchmarkWithoutFields(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - logger.Log(getMessage(0), getMessage(1)) + if err := logger.Log(getMessage(0), getMessage(1)); err != nil { + b.Fatal(err) + } } }) }) @@ -401,7 +400,6 @@ func BenchmarkWithoutFields(b *testing.B) { } }) }) - } func BenchmarkAccumulatedContext(b *testing.B) { @@ -471,7 +469,9 @@ func BenchmarkAccumulatedContext(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - logger.Log(getMessage(0), getMessage(1)) + if err := logger.Log(getMessage(0), getMessage(1)); err != nil { + b.Fatal(err) + } } }) }) @@ -531,7 +531,6 @@ func BenchmarkAccumulatedContext(b *testing.B) { } }) }) - } func BenchmarkAddingFields(b *testing.B) { @@ -592,7 +591,9 @@ func BenchmarkAddingFields(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - logger.Log(fakeSugarFields()...) + if err := logger.Log(fakeSugarFields()...); err != nil { + b.Fatal(err) + } } }) }) @@ -643,5 +644,4 @@ func BenchmarkAddingFields(b *testing.B) { } }) }) - } diff --git a/encoder_test.go b/encoder_test.go index f6be665b1..b71eb654b 100644 --- a/encoder_test.go +++ b/encoder_test.go @@ -41,7 +41,7 @@ func TestRegisterEncoder(t *testing.T) { func TestDuplicateRegisterEncoder(t *testing.T) { testEncoders(func() { - RegisterEncoder("foo", newNilEncoder) + assert.NoError(t, RegisterEncoder("foo", newNilEncoder), "expected to be able to register the encoder foo") assert.Error(t, RegisterEncoder("foo", newNilEncoder), "expected an error when registering an encoder with the same name twice") }) } @@ -52,7 +52,7 @@ func TestRegisterEncoderNoName(t *testing.T) { func TestNewEncoder(t *testing.T) { testEncoders(func() { - RegisterEncoder("foo", newNilEncoder) + assert.NoError(t, RegisterEncoder("foo", newNilEncoder), "expected to be able to register the encoder foo") encoder, err := newEncoder("foo", zapcore.EncoderConfig{}) assert.NoError(t, err, "could not create an encoder for the registered name foo") assert.Nil(t, encoder, "the encoder from newNilEncoder is not nil") diff --git a/error.go b/error.go index 38cb768de..45f7b838d 100644 --- a/error.go +++ b/error.go @@ -61,9 +61,12 @@ func (errs errArray) MarshalLogArray(arr zapcore.ArrayEncoder) error { // allocating, pool the wrapper type. elem := _errArrayElemPool.Get() elem.error = errs[i] - arr.AppendObject(elem) + err := arr.AppendObject(elem) elem.error = nil _errArrayElemPool.Put(elem) + if err != nil { + return err + } } return nil } diff --git a/error_test.go b/error_test.go index 64dab191c..4bfa370d2 100644 --- a/error_test.go +++ b/error_test.go @@ -95,3 +95,39 @@ func TestErrorsArraysHandleRichErrors(t *testing.T) { require.True(t, ok, "Expected serialized error to be a map, got %T.", serialized) assert.Equal(t, "egad", errMap["error"], "Unexpected standard error string.") } + +func TestErrArrayBrokenEncoder(t *testing.T) { + t.Parallel() + + failWith := errors.New("great sadness") + err := (brokenArrayObjectEncoder{ + Err: failWith, + ObjectEncoder: zapcore.NewMapObjectEncoder(), + }).AddArray("errors", errArray{ + errors.New("foo"), + errors.New("bar"), + }) + require.Error(t, err, "Expected error from broken encoder.") + assert.ErrorIs(t, err, failWith, "Unexpected error.") +} + +// brokenArrayObjectEncoder is an ObjectEncoder +// that builds a broken ArrayEncoder. +type brokenArrayObjectEncoder struct { + zapcore.ObjectEncoder + zapcore.ArrayEncoder + + Err error // error to return +} + +func (enc brokenArrayObjectEncoder) AddArray(key string, marshaler zapcore.ArrayMarshaler) error { + return enc.ObjectEncoder.AddArray(key, + zapcore.ArrayMarshalerFunc(func(ae zapcore.ArrayEncoder) error { + enc.ArrayEncoder = ae + return marshaler.MarshalLogArray(enc) + })) +} + +func (enc brokenArrayObjectEncoder) AppendObject(zapcore.ObjectMarshaler) error { + return enc.Err +} diff --git a/http_handler.go b/http_handler.go index 632b6831a..2be8f6515 100644 --- a/http_handler.go +++ b/http_handler.go @@ -69,6 +69,13 @@ import ( // // curl -X PUT localhost:8080/log/level -H "Content-Type: application/json" -d '{"level":"debug"}' func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if err := lvl.serveHTTP(w, r); err != nil { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "internal error: %v", err) + } +} + +func (lvl AtomicLevel) serveHTTP(w http.ResponseWriter, r *http.Request) error { type errorResponse struct { Error string `json:"error"` } @@ -80,19 +87,20 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodGet: - enc.Encode(payload{Level: lvl.Level()}) + return enc.Encode(payload{Level: lvl.Level()}) + case http.MethodPut: requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r) if err != nil { w.WriteHeader(http.StatusBadRequest) - enc.Encode(errorResponse{Error: err.Error()}) - return + return enc.Encode(errorResponse{Error: err.Error()}) } lvl.SetLevel(requestedLvl) - enc.Encode(payload{Level: lvl.Level()}) + return enc.Encode(payload{Level: lvl.Level()}) + default: w.WriteHeader(http.StatusMethodNotAllowed) - enc.Encode(errorResponse{ + return enc.Encode(errorResponse{ Error: "Only GET and PUT are supported.", }) } @@ -129,5 +137,4 @@ func decodePutJSON(body io.Reader) (zapcore.Level, error) { return 0, errors.New("must specify logging level") } return *pld.Level, nil - } diff --git a/http_handler_test.go b/http_handler_test.go index 9fa9c64c1..9da3dc7b5 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -22,6 +22,7 @@ package zap_test import ( "encoding/json" + "errors" "net/http" "net/http/httptest" "strings" @@ -167,7 +168,9 @@ func TestAtomicLevelServeHTTP(t *testing.T) { res, err := http.DefaultClient.Do(req) require.NoError(t, err, "Error making %s request.", req.Method) - defer res.Body.Close() + defer func() { + assert.NoError(t, res.Body.Close(), "Error closing response body.") + }() require.Equal(t, tt.expectedCode, res.StatusCode, "Unexpected status code.") if tt.expectedCode != http.StatusOK { @@ -188,3 +191,27 @@ func TestAtomicLevelServeHTTP(t *testing.T) { }) } } + +func TestAtomicLevelServeHTTPBrokenWriter(t *testing.T) { + t.Parallel() + + lvl := zap.NewAtomicLevel() + + request, err := http.NewRequest(http.MethodGet, "http://localhost:1234/log/level", nil) + require.NoError(t, err, "Error constructing request.") + + recorder := httptest.NewRecorder() + lvl.ServeHTTP(&brokenHTTPResponseWriter{ + ResponseWriter: recorder, + }, request) + + assert.Equal(t, http.StatusInternalServerError, recorder.Code, "Unexpected status code.") +} + +type brokenHTTPResponseWriter struct { + http.ResponseWriter +} + +func (w *brokenHTTPResponseWriter) Write([]byte) (int, error) { + return 0, errors.New("great sadness") +} diff --git a/logger.go b/logger.go index 32d737276..e9b49cc9f 100644 --- a/logger.go +++ b/logger.go @@ -374,7 +374,7 @@ func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { if stack.Count() == 0 { if log.addCaller { fmt.Fprintf(log.errorOutput, "%v Logger.check error: failed to get caller\n", ent.Time.UTC()) - log.errorOutput.Sync() + _ = log.errorOutput.Sync() } return ce } diff --git a/sink.go b/sink.go index 478c9a10f..499772a00 100644 --- a/sink.go +++ b/sink.go @@ -66,7 +66,8 @@ func newSinkRegistry() *sinkRegistry { factories: make(map[string]func(*url.URL) (Sink, error)), openFile: os.OpenFile, } - sr.RegisterSink(schemeFile, sr.newFileSinkFromURL) + // Infallible operation: the registry is empty, so we can't have a conflict. + _ = sr.RegisterSink(schemeFile, sr.newFileSinkFromURL) return sr } @@ -154,7 +155,7 @@ func (sr *sinkRegistry) newFileSinkFromPath(path string) (Sink, error) { case "stderr": return nopCloserSink{os.Stderr}, nil } - return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0666) + return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666) } func normalizeScheme(s string) (string, error) { diff --git a/stacktrace_ext_test.go b/stacktrace_ext_test.go index 71f098333..9f018aa9b 100644 --- a/stacktrace_ext_test.go +++ b/stacktrace_ext_test.go @@ -97,7 +97,7 @@ func TestStacktraceFiltersVendorZap(t *testing.T) { testDir := filepath.Join(goPath, "src/go.uber.org/zap_test/") vendorDir := filepath.Join(testDir, "vendor") - require.NoError(t, os.MkdirAll(testDir, 0777), "Failed to create source director") + require.NoError(t, os.MkdirAll(testDir, 0o777), "Failed to create source director") curFile := getSelfFilename(t) setupSymlink(t, curFile, filepath.Join(testDir, curFile)) @@ -175,7 +175,7 @@ func getSelfFilename(t *testing.T) string { func setupSymlink(t *testing.T, src, dst string) { // Make sure the destination directory exists. - os.MkdirAll(filepath.Dir(dst), 0777) + require.NoError(t, os.MkdirAll(filepath.Dir(dst), 0o777)) // Get absolute path of the source for the symlink, otherwise we can create a symlink // that uses relative paths. diff --git a/writer.go b/writer.go index 625b06760..06768c679 100644 --- a/writer.go +++ b/writer.go @@ -62,7 +62,7 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) { closers := make([]io.Closer, 0, len(paths)) closeAll := func() { for _, c := range closers { - c.Close() + _ = c.Close() } } diff --git a/writer_test.go b/writer_test.go index b74345567..20e00b74b 100644 --- a/writer_test.go +++ b/writer_test.go @@ -91,7 +91,6 @@ func TestOpen(t *testing.T) { } assert.True(t, fileExists(tempName)) - os.Remove(tempName) } func TestOpenPathsNotFound(t *testing.T) { @@ -255,7 +254,8 @@ func TestOpenWithErroringSinkFactory(t *testing.T) { func TestCombineWriteSyncers(t *testing.T) { tw := &testWriter{"test", t} w := CombineWriteSyncers(tw) - w.Write([]byte("test")) + _, err := w.Write([]byte("test")) + assert.NoError(t, err, "Unexpected write error.") } func fileExists(name string) bool { diff --git a/zapcore/buffered_write_syncer_bench_test.go b/zapcore/buffered_write_syncer_bench_test.go index 1e3db59f1..56ad5f2c6 100644 --- a/zapcore/buffered_write_syncer_bench_test.go +++ b/zapcore/buffered_write_syncer_bench_test.go @@ -40,11 +40,15 @@ func BenchmarkBufferedWriteSyncer(b *testing.B) { w := &BufferedWriteSyncer{ WS: AddSync(file), } - defer w.Stop() + defer func() { + assert.NoError(b, w.Stop(), "failed to stop buffered write syncer") + }() b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) diff --git a/zapcore/buffered_write_syncer_test.go b/zapcore/buffered_write_syncer_test.go index 8a36ad69d..d0f6037af 100644 --- a/zapcore/buffered_write_syncer_test.go +++ b/zapcore/buffered_write_syncer_test.go @@ -101,7 +101,8 @@ func TestBufferWriter(t *testing.T) { n, err := ws.Write([]byte("foo")) require.NoError(t, err, "Unexpected error writing to WriteSyncer.") require.Equal(t, 3, n, "Wrote an unexpected number of bytes.") - ws.Write([]byte("foo")) + _, err = ws.Write([]byte("foo")) + assert.Error(t, err, "Expected error writing to WriteSyncer.") assert.Error(t, ws.Stop(), "Expected stop to fail.") }) diff --git a/zapcore/core.go b/zapcore/core.go index 9dfd64051..776e93f6f 100644 --- a/zapcore/core.go +++ b/zapcore/core.go @@ -102,9 +102,9 @@ func (c *ioCore) Write(ent Entry, fields []Field) error { return err } if ent.Level > ErrorLevel { - // Since we may be crashing the program, sync the output. Ignore Sync - // errors, pending a clean solution to issue #370. - c.Sync() + // Since we may be crashing the program, sync the output. + // Ignore Sync errors, pending a clean solution to issue #370. + _ = c.Sync() } return nil } diff --git a/zapcore/core_test.go b/zapcore/core_test.go index 1311097cc..e3186311a 100644 --- a/zapcore/core_test.go +++ b/zapcore/core_test.go @@ -148,7 +148,7 @@ func TestIOCoreSyncsOutput(t *testing.T) { DebugLevel, ) - core.Write(tt.entry, nil) + assert.NoError(t, core.Write(tt.entry, nil), "Unexpected error writing entry.") assert.Equal(t, tt.shouldSync, sink.Called(), "Incorrect Sync behavior.") } } diff --git a/zapcore/encoder_test.go b/zapcore/encoder_test.go index c0dbc5bb1..9b8142f5d 100644 --- a/zapcore/encoder_test.go +++ b/zapcore/encoder_test.go @@ -285,10 +285,11 @@ func TestEncoderConfiguration(t *testing.T) { }, extra: func(enc Encoder) { enc.AddTime("extra", _epoch) - enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { + err := enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { enc.AppendTime(_epoch) return nil })) + assert.NoError(t, err) }, expectedJSON: `{"L":"info","T":"1970-01-01 00:00:00 +0000 UTC","N":"main","C":"foo.go:42","F":"foo.Foo","M":"hello","extra":"1970-01-01 00:00:00 +0000 UTC","extras":["1970-01-01 00:00:00 +0000 UTC"],"S":"fake-stack"}` + "\n", expectedConsole: "1970-01-01 00:00:00 +0000 UTC\tinfo\tmain\tfoo.go:42\tfoo.Foo\thello\t" + // plain-text preamble @@ -313,10 +314,11 @@ func TestEncoderConfiguration(t *testing.T) { }, extra: func(enc Encoder) { enc.AddDuration("extra", time.Second) - enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { + err := enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { enc.AppendDuration(time.Minute) return nil })) + assert.NoError(t, err) }, expectedJSON: `{"L":"info","T":0,"N":"main","C":"foo.go:42","F":"foo.Foo","M":"hello","extra":"1s","extras":["1m0s"],"S":"fake-stack"}` + "\n", expectedConsole: "0\tinfo\tmain\tfoo.go:42\tfoo.Foo\thello\t" + // preamble @@ -720,10 +722,11 @@ func TestNameEncoders(t *testing.T) { func assertAppended(t testing.TB, expected interface{}, f func(ArrayEncoder), msgAndArgs ...interface{}) { mem := NewMapObjectEncoder() - mem.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error { + err := mem.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error { f(arr) return nil })) + assert.NoError(t, err, msgAndArgs...) arr := mem.Fields["k"].([]interface{}) require.Equal(t, 1, len(arr), "Expected to append exactly one element to array.") assert.Equal(t, expected, arr[0], msgAndArgs...) diff --git a/zapcore/entry.go b/zapcore/entry.go index 059844f92..459a5d7ce 100644 --- a/zapcore/entry.go +++ b/zapcore/entry.go @@ -242,7 +242,7 @@ func (ce *CheckedEntry) Write(fields ...Field) { // CheckedEntry is being used after it was returned to the pool, // the message may be an amalgamation from multiple call sites. fmt.Fprintf(ce.ErrorOutput, "%v Unsafe CheckedEntry re-use near Entry %+v.\n", ce.Time, ce.Entry) - ce.ErrorOutput.Sync() + _ = ce.ErrorOutput.Sync() // ignore error } return } @@ -254,7 +254,7 @@ func (ce *CheckedEntry) Write(fields ...Field) { } if err != nil && ce.ErrorOutput != nil { fmt.Fprintf(ce.ErrorOutput, "%v write error: %v\n", ce.Time, err) - ce.ErrorOutput.Sync() + _ = ce.ErrorOutput.Sync() // ignore error } hook := ce.after diff --git a/zapcore/entry_ext_test.go b/zapcore/entry_ext_test.go new file mode 100644 index 000000000..fd1a05cb5 --- /dev/null +++ b/zapcore/entry_ext_test.go @@ -0,0 +1,55 @@ +// Copyright (c) 2023 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package zapcore_test + +import ( + "bytes" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest" +) + +func TestCheckedEntryIllegalReuse(t *testing.T) { + t.Parallel() + + var errOut bytes.Buffer + + testCore := zaptest.NewLogger(t).Core() + ce := testCore.Check(zapcore.Entry{ + Level: zapcore.InfoLevel, + Time: time.Now(), + Message: "hello", + }, nil) + ce.ErrorOutput = zapcore.AddSync(&errOut) + + // The first write should succeed. + ce.Write(zap.String("k", "v"), zap.Int("n", 42)) + assert.Empty(t, errOut.String(), "Expected no errors on first write.") + + // The second write should fail. + ce.Write(zap.String("foo", "bar"), zap.Int("x", 1)) + assert.Contains(t, errOut.String(), "Unsafe CheckedEntry re-use near Entry", + "Expected error logged on second write.") +} diff --git a/zapcore/error.go b/zapcore/error.go index c67dd71df..c40df1326 100644 --- a/zapcore/error.go +++ b/zapcore/error.go @@ -98,8 +98,11 @@ func (errs errArray) MarshalLogArray(arr ArrayEncoder) error { } el := newErrArrayElem(errs[i]) - arr.AppendObject(el) + err := arr.AppendObject(el) el.Free() + if err != nil { + return err + } } return nil } diff --git a/zapcore/error_test.go b/zapcore/error_test.go index d8263abc1..c5d61b040 100644 --- a/zapcore/error_test.go +++ b/zapcore/error_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/assert" "go.uber.org/multierr" + "go.uber.org/zap/zapcore" . "go.uber.org/zap/zapcore" ) @@ -161,3 +162,49 @@ func TestRichErrorSupport(t *testing.T) { f.AddTo(enc) assert.Equal(t, "failed: egad", enc.Fields["k"], "Unexpected basic error message.") } + +func TestErrArrayBrokenEncoder(t *testing.T) { + t.Parallel() + + f := Field{ + Key: "foo", + Type: ErrorType, + Interface: multierr.Combine( + errors.New("foo"), + errors.New("bar"), + ), + } + + failWith := errors.New("great sadness") + enc := NewMapObjectEncoder() + f.AddTo(brokenArrayObjectEncoder{ + Err: failWith, + ObjectEncoder: enc, + }) + + // Failure to add the field to the encoder + // causes the error to be added as a string field. + assert.Equal(t, "great sadness", enc.Fields["fooError"], + "Unexpected error message.") +} + +// brokenArrayObjectEncoder is an ObjectEncoder +// that builds a broken ArrayEncoder. +type brokenArrayObjectEncoder struct { + ObjectEncoder + ArrayEncoder + + Err error // error to return +} + +func (enc brokenArrayObjectEncoder) AddArray(key string, marshaler ArrayMarshaler) error { + return enc.ObjectEncoder.AddArray(key, + ArrayMarshalerFunc(func(ae ArrayEncoder) error { + enc.ArrayEncoder = ae + return marshaler.MarshalLogArray(enc) + })) +} + +func (enc brokenArrayObjectEncoder) AppendObject(zapcore.ObjectMarshaler) error { + return enc.Err +} diff --git a/zapcore/json_encoder_bench_test.go b/zapcore/json_encoder_bench_test.go index bcb5a0133..ef8001993 100644 --- a/zapcore/json_encoder_bench_test.go +++ b/zapcore/json_encoder_bench_test.go @@ -31,10 +31,13 @@ import ( func BenchmarkJSONLogMarshalerFunc(b *testing.B) { for i := 0; i < b.N; i++ { enc := NewJSONEncoder(testEncoderConfig()) - enc.AddObject("nested", ObjectMarshalerFunc(func(enc ObjectEncoder) error { + err := enc.AddObject("nested", ObjectMarshalerFunc(func(enc ObjectEncoder) error { enc.AddInt64("i", int64(i)) return nil })) + if err != nil { + b.Fatal(err) + } } } @@ -95,7 +98,9 @@ func BenchmarkStandardJSON(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - json.Marshal(record) + if _, err := json.Marshal(record); err != nil { + b.Fatal(err) + } } }) } diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go index fde241f56..5fc7b4db2 100644 --- a/zapcore/json_encoder_impl_test.go +++ b/zapcore/json_encoder_impl_test.go @@ -249,7 +249,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { desc: "object (no nested namespace)", expected: `"obj":{"obj-out":"obj-outside-namespace"},"not-obj":"should-be-outside-obj"`, f: func(e Encoder) { - e.AddObject("obj", maybeNamespace{false}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{false})) e.AddString("not-obj", "should-be-outside-obj") }, }, @@ -257,7 +257,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { desc: "object (with nested namespace)", expected: `"obj":{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"not-obj":"should-be-outside-obj"`, f: func(e Encoder) { - e.AddObject("obj", maybeNamespace{true}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{true})) e.AddString("not-obj", "should-be-outside-obj") }, }, @@ -265,7 +265,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { desc: "multiple open namespaces", expected: `"k":{"foo":1,"middle":{"foo":2,"inner":{"foo":3}}}`, f: func(e Encoder) { - e.AddObject("k", ObjectMarshalerFunc(func(enc ObjectEncoder) error { + err := e.AddObject("k", ObjectMarshalerFunc(func(enc ObjectEncoder) error { e.AddInt("foo", 1) e.OpenNamespace("middle") e.AddInt("foo", 2) @@ -273,6 +273,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { e.AddInt("foo", 3) return nil })) + assert.NoError(t, err) }, }, } @@ -289,10 +290,11 @@ func TestJSONEncoderTimeFormats(t *testing.T) { f := func(e Encoder) { e.AddTime("k", date) - e.AddArray("a", ArrayMarshalerFunc(func(enc ArrayEncoder) error { + err := e.AddArray("a", ArrayMarshalerFunc(func(enc ArrayEncoder) error { enc.AppendTime(date) return nil })) + assert.NoError(t, err) } tests := []struct { desc string @@ -420,7 +422,7 @@ func TestJSONEncoderArrays(t *testing.T) { desc: "object (no nested namespace) then string", expected: `[{"obj-out":"obj-outside-namespace"},"should-be-outside-obj",{"obj-out":"obj-outside-namespace"},"should-be-outside-obj"]`, f: func(arr ArrayEncoder) { - arr.AppendObject(maybeNamespace{false}) + assert.NoError(t, arr.AppendObject(maybeNamespace{false})) arr.AppendString("should-be-outside-obj") }, }, @@ -428,7 +430,7 @@ func TestJSONEncoderArrays(t *testing.T) { desc: "object (with nested namespace) then string", expected: `[{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj",{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj"]`, f: func(arr ArrayEncoder) { - arr.AppendObject(maybeNamespace{true}) + assert.NoError(t, arr.AppendObject(maybeNamespace{true})) arr.AppendString("should-be-outside-obj") }, }, @@ -530,10 +532,13 @@ type turducken struct{} func (t turducken) MarshalLogObject(enc ObjectEncoder) error { return enc.AddArray("ducks", ArrayMarshalerFunc(func(arr ArrayEncoder) error { for i := 0; i < 2; i++ { - arr.AppendObject(ObjectMarshalerFunc(func(inner ObjectEncoder) error { + err := arr.AppendObject(ObjectMarshalerFunc(func(inner ObjectEncoder) error { inner.AddString("in", "chicken") return nil })) + if err != nil { + return err + } } return nil })) diff --git a/zapcore/level_test.go b/zapcore/level_test.go index ab97c98d0..d8eb96292 100644 --- a/zapcore/level_test.go +++ b/zapcore/level_test.go @@ -159,7 +159,7 @@ func TestLevelNils(t *testing.T) { }, "Level(nil).String() should panic") assert.Panics(t, func() { - l.MarshalText() + _, _ = l.MarshalText() // should panic }, "Expected to panic when marshalling a nil level.") err := l.UnmarshalText([]byte("debug")) diff --git a/zapcore/memory_encoder_test.go b/zapcore/memory_encoder_test.go index 052bdb0c6..d5f215fb6 100644 --- a/zapcore/memory_encoder_test.go +++ b/zapcore/memory_encoder_test.go @@ -218,7 +218,7 @@ func TestMapObjectEncoderAdd(t *testing.T) { desc: "object (no nested namespace) then string", f: func(e ObjectEncoder) { e.OpenNamespace("k") - e.AddObject("obj", maybeNamespace{false}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{false})) e.AddString("not-obj", "should-be-outside-obj") }, expected: map[string]interface{}{ @@ -232,7 +232,7 @@ func TestMapObjectEncoderAdd(t *testing.T) { desc: "object (with nested namespace) then string", f: func(e ObjectEncoder) { e.OpenNamespace("k") - e.AddObject("obj", maybeNamespace{true}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{true})) e.AddString("not-obj", "should-be-outside-obj") }, expected: map[string]interface{}{ @@ -255,6 +255,7 @@ func TestMapObjectEncoderAdd(t *testing.T) { }) } } + func TestSliceArrayEncoderAppend(t *testing.T) { tests := []struct { desc string @@ -284,29 +285,33 @@ func TestSliceArrayEncoderAppend(t *testing.T) { {"AppendUint8", func(e ArrayEncoder) { e.AppendUint8(42) }, uint8(42)}, {"AppendUintptr", func(e ArrayEncoder) { e.AppendUintptr(42) }, uintptr(42)}, { - desc: "AppendReflected", - f: func(e ArrayEncoder) { e.AppendReflected(map[string]interface{}{"foo": 5}) }, + desc: "AppendReflected", + f: func(e ArrayEncoder) { + assert.NoError(t, e.AppendReflected(map[string]interface{}{"foo": 5})) + }, expected: map[string]interface{}{"foo": 5}, }, { desc: "AppendArray (arrays of arrays)", f: func(e ArrayEncoder) { - e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + err := e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { inner.AppendBool(true) inner.AppendBool(false) return nil })) + assert.NoError(t, err) }, expected: []interface{}{true, false}, }, { desc: "object (no nested namespace) then string", f: func(e ArrayEncoder) { - e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { - inner.AppendObject(maybeNamespace{false}) + err := e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + err := inner.AppendObject(maybeNamespace{false}) inner.AppendString("should-be-outside-obj") - return nil + return err })) + assert.NoError(t, err) }, expected: []interface{}{ map[string]interface{}{ @@ -318,11 +323,12 @@ func TestSliceArrayEncoderAppend(t *testing.T) { { desc: "object (with nested namespace) then string", f: func(e ArrayEncoder) { - e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { - inner.AppendObject(maybeNamespace{true}) + err := e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + err := inner.AppendObject(maybeNamespace{true}) inner.AppendString("should-be-outside-obj") - return nil + return err })) + assert.NoError(t, err) }, expected: []interface{}{ map[string]interface{}{ diff --git a/zapcore/tee_test.go b/zapcore/tee_test.go index b2b9c9dc0..f6be13316 100644 --- a/zapcore/tee_test.go +++ b/zapcore/tee_test.go @@ -120,7 +120,7 @@ func TestTeeWrite(t *testing.T) { debugEntry := Entry{Level: DebugLevel, Message: "log-at-debug"} warnEntry := Entry{Level: WarnLevel, Message: "log-at-warn"} for _, ent := range []Entry{debugEntry, warnEntry} { - tee.Write(ent, nil) + assert.NoError(t, tee.Write(ent, nil)) } for _, logs := range []*observer.ObservedLogs{debugLogs, warnLogs} { diff --git a/zapcore/write_syncer_bench_test.go b/zapcore/write_syncer_bench_test.go index db6ec4523..90ae47558 100644 --- a/zapcore/write_syncer_bench_test.go +++ b/zapcore/write_syncer_bench_test.go @@ -24,6 +24,7 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap/internal/ztest" ) @@ -37,7 +38,9 @@ func BenchmarkMultiWriteSyncer(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) @@ -51,7 +54,9 @@ func BenchmarkMultiWriteSyncer(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) @@ -64,11 +69,15 @@ func BenchmarkMultiWriteSyncer(b *testing.B) { &ztest.Discarder{}, ), } - defer w.Stop() + defer func() { + assert.NoError(b, w.Stop(), "Unexpected error stopping buffered write syncer.") + }() b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) @@ -83,7 +92,9 @@ func BenchmarkWriteSyncer(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) diff --git a/zapcore/write_syncer_test.go b/zapcore/write_syncer_test.go index 4748be7f5..c0c2698e9 100644 --- a/zapcore/write_syncer_test.go +++ b/zapcore/write_syncer_test.go @@ -70,7 +70,7 @@ func TestNewMultiWriteSyncerWorksForSingleWriter(t *testing.T) { ws := NewMultiWriteSyncer(w) assert.Equal(t, w, ws, "Expected NewMultiWriteSyncer to return the same WriteSyncer object for a single argument.") - ws.Sync() + assert.NoError(t, ws.Sync(), "Expected Sync to succeed.") assert.True(t, w.Called(), "Expected Sync to be called on the created WriteSyncer") } diff --git a/zaptest/observer/observer_test.go b/zaptest/observer/observer_test.go index 2a901b1ce..0a57a0f32 100644 --- a/zaptest/observer/observer_test.go +++ b/zaptest/observer/observer_test.go @@ -173,7 +173,7 @@ func TestFilters(t *testing.T) { logger, sink := New(zap.InfoLevel) for _, log := range logs { - logger.Write(log.Entry, log.Context) + assert.NoError(t, logger.Write(log.Entry, log.Context), "Unexpected error writing log entry.") } tests := []struct {