Skip to content

Commit e60d30f

Browse files
ibetitsmikeibetitsmike
committed
Removed nil checks when fetching RequestLogger
Refactored RequestLoggerContext to RequestLogger Refactored provisionerjobs_internal tests to inject MockRequestLogger Added Mocks for RequestLogger
1 parent aa3167d commit e60d30f

File tree

8 files changed

+114
-39
lines changed

8 files changed

+114
-39
lines changed

‎coderd/httpmw/logger.go

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
3535
slog.F("start", start),
3636
)
3737

38-
logContext := NewRequestLoggerContext(httplog, r.Method, start)
38+
logContext := NewRequestLogger(httplog, r.Method, start)
3939

40-
ctx := context.WithValue(r.Context(), logContextKey{}, logContext)
40+
ctx := WithRequestLogger(r.Context(), logContext)
4141

4242
next.ServeHTTP(sw, r.WithContext(ctx))
4343

@@ -63,27 +63,32 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
6363
}
6464
}
6565

66-
type RequestLoggerContext struct {
66+
type RequestLogger interface {
67+
WithFields(fields ...slog.Field)
68+
WriteLog(ctx context.Context, status int)
69+
}
70+
71+
type RequestContextLogger struct {
6772
log slog.Logger
6873
written bool
6974
message string
7075
start time.Time
7176
}
7277

73-
func NewRequestLoggerContext(log slog.Logger, message string, start time.Time) *RequestLoggerContext {
74-
return &RequestLoggerContext{
78+
func NewRequestLogger(log slog.Logger, message string, start time.Time) RequestLogger {
79+
return &RequestContextLogger{
7580
log: log,
7681
written: false,
7782
message: message,
7883
start: start,
7984
}
8085
}
8186

82-
func (c *RequestLoggerContext) WithFields(fields ...slog.Field) {
87+
func (c *RequestContextLogger) WithFields(fields ...slog.Field) {
8388
c.log = c.log.With(fields...)
8489
}
8590

86-
func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) {
91+
func (c *RequestContextLogger) WriteLog(ctx context.Context, status int) {
8792
if c.written {
8893
return
8994
}
@@ -107,9 +112,13 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) {
107112

108113
type logContextKey struct{}
109114

110-
func RequestLoggerFromContext(ctx context.Context) *RequestLoggerContext {
115+
func WithRequestLogger(ctx context.Context, rl RequestLogger) context.Context {
116+
return context.WithValue(ctx, logContextKey{}, rl)
117+
}
118+
119+
func RequestLoggerFromContext(ctx context.Context) RequestLogger {
111120
val := ctx.Value(logContextKey{})
112-
if logCtx, ok := val.(*RequestLoggerContext); ok {
121+
if logCtx, ok := val.(RequestLogger); ok {
113122
return logCtx
114123
}
115124
return nil

‎coderd/httpmw/logger_internal_test.go

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@ import (
1212
"github.com/coder/coder/v2/coderd/tracing"
1313
)
1414

15-
func TestRequestLoggerContext_WriteLog(t *testing.T) {
15+
func TestRequestLogger_WriteLog(t *testing.T) {
1616
t.Parallel()
1717
ctx := context.Background()
1818

1919
testLogger := slogtest.Make(t, nil)
2020

21-
logCtx := NewRequestLoggerContext(testLogger, "GET", time.Now())
21+
logCtx := NewRequestLogger(testLogger, "GET", time.Now())
2222

2323
// Add custom fields
2424
logCtx.WithFields(
@@ -28,9 +28,13 @@ func TestRequestLoggerContext_WriteLog(t *testing.T) {
2828
// Write log for 200 status
2929
logCtx.WriteLog(ctx, http.StatusOK)
3030

31-
if !logCtx.written {
32-
t.Error("expected log to be written once")
31+
if logCtx != nil {
32+
requestCtxLog, ok := logCtx.(*RequestContextLogger)
33+
if ok && !requestCtxLog.written {
34+
t.Error("expected log to be written once")
35+
}
3336
}
37+
3438
// Attempt to write again (should be skipped).
3539
// If the error log entry gets written,
3640
// slogtest will fail the test.
@@ -66,7 +70,10 @@ func TestLoggerMiddleware(t *testing.T) {
6670

6771
logCtx := RequestLoggerFromContext(context.Background())
6872
// Verify that the log was written
69-
if logCtx != nil && !logCtx.written {
70-
t.Error("expected log to be written exactly once")
73+
if logCtx != nil {
74+
requestCtxLog, ok := logCtx.(*RequestContextLogger)
75+
if ok && !requestCtxLog.written {
76+
t.Error("expected log to be written once")
77+
}
7178
}
7279
}

‎coderd/httpmw/mocks/mock_request_logger.go

Lines changed: 70 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/inboxnotifications.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -220,10 +220,7 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request)
220220
defer encoder.Close(websocket.StatusNormalClosure)
221221

222222
// Log the request immediately instead of after it completes.
223-
requestLogger := httpmw.RequestLoggerFromContext(ctx)
224-
if requestLogger != nil {
225-
requestLogger.WriteLog(ctx, http.StatusAccepted)
226-
}
223+
httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted)
227224

228225
for {
229226
select {

‎coderd/provisionerjobs.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -555,10 +555,7 @@ func (f *logFollower) follow() {
555555
}
556556

557557
// Log the request immediately instead of after it completes.
558-
requestLogger := httpmw.RequestLoggerFromContext(f.ctx)
559-
if requestLogger != nil {
560-
requestLogger.WriteLog(f.ctx, http.StatusAccepted)
561-
}
558+
httpmw.RequestLoggerFromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted)
562559

563560
// no need to wait if the job is done
564561
if f.complete {

‎coderd/provisionerjobs_internal_test.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import (
1919
"github.com/coder/coder/v2/coderd/database/dbmock"
2020
"github.com/coder/coder/v2/coderd/database/dbtime"
2121
"github.com/coder/coder/v2/coderd/database/pubsub"
22+
"github.com/coder/coder/v2/coderd/httpmw"
23+
"github.com/coder/coder/v2/coderd/httpmw/mocks"
2224
"github.com/coder/coder/v2/codersdk"
2325
"github.com/coder/coder/v2/provisionersdk"
2426
"github.com/coder/coder/v2/testutil"
@@ -305,11 +307,16 @@ func Test_logFollower_EndOfLogs(t *testing.T) {
305307
JobStatus: database.ProvisionerJobStatusRunning,
306308
}
307309

310+
mockLogger := mocks.NewMockRequestLogger(ctrl)
311+
mockLogger.EXPECT().WriteLog(gomock.Any(), http.StatusAccepted).Times(1)
312+
ctx = httpmw.WithRequestLogger(ctx, mockLogger)
313+
308314
// we need an HTTP server to get a websocket
309315
srv := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
310316
uut := newLogFollower(ctx, logger, mDB, ps, rw, r, job, 0)
311317
uut.follow()
312318
}))
319+
313320
defer srv.Close()
314321

315322
// job was incomplete when we create the logFollower, and still incomplete when it queries

‎coderd/workspaceagents.go

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -556,10 +556,7 @@ func (api *API) workspaceAgentLogs(rw http.ResponseWriter, r *http.Request) {
556556
defer t.Stop()
557557

558558
// Log the request immediately instead of after it completes.
559-
requestLogger := httpmw.RequestLoggerFromContext(ctx)
560-
if requestLogger != nil {
561-
requestLogger.WriteLog(ctx, http.StatusAccepted)
562-
}
559+
httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted)
563560

564561
go func() {
565562
defer func() {
@@ -935,10 +932,7 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) {
935932
defer encoder.Close(websocket.StatusGoingAway)
936933

937934
// Log the request immediately instead of after it completes.
938-
requestLogger := httpmw.RequestLoggerFromContext(ctx)
939-
if requestLogger != nil {
940-
requestLogger.WriteLog(ctx, http.StatusAccepted)
941-
}
935+
httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted)
942936

943937
go func(ctx context.Context) {
944938
// TODO(mafredri): Is this too frequent? Use separate ping disconnect timeout?
@@ -1328,10 +1322,7 @@ func (api *API) watchWorkspaceAgentMetadata(
13281322
defer sendTicker.Stop()
13291323

13301324
// Log the request immediately instead of after it completes.
1331-
requestLogger := httpmw.RequestLoggerFromContext(ctx)
1332-
if requestLogger != nil {
1333-
requestLogger.WriteLog(ctx, http.StatusAccepted)
1334-
}
1325+
httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted)
13351326

13361327
// Send initial metadata.
13371328
sendMetadata()

‎enterprise/coderd/provisionerdaemons.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -378,10 +378,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request)
378378
})
379379

380380
// Log the request immediately instead of after it completes.
381-
requestLogger := httpmw.RequestLoggerFromContext(ctx)
382-
if requestLogger != nil {
383-
requestLogger.WriteLog(ctx, http.StatusAccepted)
384-
}
381+
httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted)
385382

386383
err = server.Serve(ctx, session)
387384
srvCancel()

0 commit comments

Comments
 (0)