From fe7c839a30dba132da14405d9f9ff4b6ab998466 Mon Sep 17 00:00:00 2001 From: Ryan Schumacher Date: Fri, 4 Oct 2024 15:20:01 -0500 Subject: [PATCH] fix: Refactor static file server handler and logger (#8) Refactor the code in staticFileServerHandler.go and logger.go to improve code organization and readability. The changes include: - Move the logger related functions to a separate file, logger.go, for better separation of concerns. - Rename the StaticFilesHandler function to NewStaticFilesHandler to follow the Go convention for constructor functions. - Extract the ServeHTTP method from the StaticFilesHandler function and move it to the StaticFilesHandler struct to improve encapsulation. - Clean up the code by removing unused imports and variables. This commit improves the maintainability and readability of the codebase. --- logger.go | 22 ++++++ staticFileServerHandler.go | 116 ++++++++++++++------------------ staticFileServerHandler_test.go | 33 +++++---- 3 files changed, 93 insertions(+), 78 deletions(-) create mode 100644 logger.go diff --git a/logger.go b/logger.go new file mode 100644 index 0000000..a4c6619 --- /dev/null +++ b/logger.go @@ -0,0 +1,22 @@ +package spaserve + +import ( + "context" + "log/slog" +) + +type servespaLogger struct { + logger *slog.Logger +} + +// newLogger creates a new logger function with the given context and logger. +func newLogger(logger *slog.Logger) *servespaLogger { + return &servespaLogger{logger: logger} +} + +func (l servespaLogger) logContext(ctx context.Context, level slog.Level, msg string, attrs ...slog.Attr) { + if l.logger == nil { + return + } + l.logger.LogAttrs(ctx, level, msg, attrs...) +} diff --git a/staticFileServerHandler.go b/staticFileServerHandler.go index a3f93c2..71ada36 100644 --- a/staticFileServerHandler.go +++ b/staticFileServerHandler.go @@ -1,7 +1,6 @@ package spaserve import ( - "context" "errors" "io/fs" "log/slog" @@ -13,6 +12,14 @@ import ( "github.com/psanford/memfs" ) +type StaticFilesHandler struct { + opts staticFilesHandlerOpts + fileServer http.Handler + mfilesys *memfs.FS + logger *servespaLogger + muxErrHandler func(int, http.ResponseWriter, *http.Request) +} + type staticFilesHandlerOpts struct { ns string basePath string @@ -92,16 +99,13 @@ func WithInjectWebEnv(env any, namespace string) staticFilesHandlerFunc { // - ctx: the context // - filesys: the file system to serve files from - this will be copied to a memfs // - fn: optional functions to configure the handler (e.g. WithLogger, WithBasePath, WithMuxErrorHandler, WithInjectWebEnv) -func StaticFilesHandler(ctx context.Context, filesys fs.FS, fn ...staticFilesHandlerFunc) (http.Handler, error) { +func NewStaticFilesHandler(filesys fs.FS, fn ...staticFilesHandlerFunc) (http.Handler, error) { // process options opts := defaultStaticFilesHandlerOpts for _, f := range fn { opts = f(opts) } - logWithAttrs := newLoggerWithContext(ctx, opts.logger) - muxErrHandler := newMuxErrorHandler(opts.muxErrHandler) - var ( mfilesys *memfs.FS err error @@ -118,89 +122,69 @@ func StaticFilesHandler(ctx context.Context, filesys fs.FS, fn ...staticFilesHan // create file server fileServer := http.FileServer(http.FS(mfilesys)) + logger := newLogger(opts.logger) + + return &StaticFilesHandler{ + opts: opts, + mfilesys: mfilesys, + fileServer: fileServer, + logger: logger, + muxErrHandler: newMuxErrorHandler(opts.muxErrHandler), + }, nil +} - // return handler - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // serve index.html for root path - if r.URL.Path == "/" { - fileServer.ServeHTTP(w, r) - return - } +func (h *StaticFilesHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() - // warn if base path might be wrong - if len(opts.basePath) > 0 && r.URL.Path[:len(opts.basePath)] != opts.basePath { - logWithAttrs(slog.LevelInfo, "WARNING: base path may not be set correctly", - slog.Attr{Key: "reqPath", Value: slog.StringValue(r.URL.Path)}, - slog.Attr{Key: "basePath", Value: slog.StringValue(opts.basePath)}, - ) - } + // clean path for security and consistency + cleanedPath := path.Clean(r.URL.Path) + cleanedPath = strings.TrimPrefix(cleanedPath, h.opts.basePath) + cleanedPath = strings.TrimPrefix(cleanedPath, "/") + cleanedPath = strings.TrimSuffix(cleanedPath, "/") - // clean path for security and consistency - cleanedPath := path.Clean(r.URL.Path) - cleanedPath = strings.TrimPrefix(cleanedPath, opts.basePath) + h.logger.logContext(ctx, slog.LevelDebug, "request", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) - // open file - file, err := mfilesys.Open(cleanedPath) - if file != nil { - defer file.Close() - } + // reconstitute the path + r.URL.Path = "/" + cleanedPath - // ensure leading slash - r.URL.Path = cleanedPath - if r.URL.Path[0] != '/' { - r.URL.Path = "/" + r.URL.Path - } - - // if index.html is requested, rewrite to avoid 301 redirect - if r.URL.Path == "/index.html" { - r.URL.Path = "/" - } + // use root path for index.html + if r.URL.Path == "index.html" { + r.URL.Path = "/" + } + // handle non-root paths + if r.URL.Path != "/" { + // open file + file, err := h.mfilesys.Open(cleanedPath) + isErr := err != nil isErrNotExist := errors.Is(err, os.ErrNotExist) isFile := path.Ext(cleanedPath) != "" - - // if err != nil { - // fmt.Printf("error: %v\n", err) - // fmt.Printf("request path: %s\n", r.URL.Path) - // fmt.Printf("cleaned path: %s\n", cleanedPath) - // fs.WalkDir(mfilesys, ".", func(path string, d fs.DirEntry, err error) error { - // fmt.Printf("path: %s, d: %v, err: %v\n", path, d, err) - // return nil - // }) - // } + if file != nil { + file.Close() + } // return 500 for other errors - if err != nil && !isErrNotExist { - logWithAttrs(slog.LevelError, "could not open file", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) - muxErrHandler(http.StatusInternalServerError, w, r) + if isErr && !isErrNotExist { + h.logger.logContext(ctx, slog.LevelError, "could not open file", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) + h.muxErrHandler(http.StatusInternalServerError, w, r) return } // return 404 for actual static file requests that don't exist - if err != nil && isErrNotExist && isFile { - logWithAttrs(slog.LevelError, "could not find file", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) - muxErrHandler(http.StatusNotFound, w, r) + if isErrNotExist && isFile { + h.logger.logContext(ctx, slog.LevelDebug, "not found, static file", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) + h.muxErrHandler(http.StatusNotFound, w, r) return } // serve index.html and let SPA handle undefined routes if isErrNotExist { - logWithAttrs(slog.LevelDebug, "not found, serve index", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) + h.logger.logContext(ctx, slog.LevelDebug, "not found, serve index", slog.Attr{Key: "cleanedPath", Value: slog.StringValue(cleanedPath)}) r.URL.Path = "/" } - - fileServer.ServeHTTP(w, r) - }), nil -} - -// newLoggerWithContext creates a new logger function with the given context and logger. -func newLoggerWithContext(ctx context.Context, logger *slog.Logger) func(slog.Level, string, ...slog.Attr) { - return func(level slog.Level, msg string, attrs ...slog.Attr) { - if logger == nil { - return - } - logger.LogAttrs(ctx, level, msg, attrs...) } + + h.fileServer.ServeHTTP(w, r) } // newMuxErrorHandler creates a new error handler function with the given muxErrHandler. diff --git a/staticFileServerHandler_test.go b/staticFileServerHandler_test.go index b675111..7edcaf7 100644 --- a/staticFileServerHandler_test.go +++ b/staticFileServerHandler_test.go @@ -2,7 +2,6 @@ package spaserve import ( "bytes" - "context" "fmt" "log/slog" "net/http" @@ -14,11 +13,10 @@ import ( ) func TestStaticFilesHandler(t *testing.T) { - ctx := context.TODO() filesys := os.DirFS(path.Join("testdata", "files")) t.Run("ServeIndexHTML", func(t *testing.T) { - handler, err := StaticFilesHandler(ctx, filesys) + handler, err := NewStaticFilesHandler(filesys) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -31,10 +29,19 @@ func TestStaticFilesHandler(t *testing.T) { if w.Code != http.StatusOK { t.Errorf("Expected status code %d, but got %d", http.StatusOK, w.Code) } + + req = httptest.NewRequest(http.MethodGet, "/index.html", nil) + w = httptest.NewRecorder() + handler.ServeHTTP(w, req) + + // Assert that index.html is served + if w.Code != http.StatusMovedPermanently { + t.Errorf("Expected status code %d, but got %d", http.StatusMovedPermanently, w.Code) + } }) t.Run("ServeExistingFile", func(t *testing.T) { - handler, err := StaticFilesHandler(ctx, filesys) + handler, err := NewStaticFilesHandler(filesys) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -50,7 +57,7 @@ func TestStaticFilesHandler(t *testing.T) { }) t.Run("ServeNonExistingFile", func(t *testing.T) { - handler, err := StaticFilesHandler(ctx, filesys) + handler, err := NewStaticFilesHandler(filesys) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -66,7 +73,7 @@ func TestStaticFilesHandler(t *testing.T) { }) t.Run("ServeIndexOnNonExistingFile", func(t *testing.T) { - handler, err := StaticFilesHandler(ctx, filesys) + handler, err := NewStaticFilesHandler(filesys) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -135,7 +142,7 @@ func TestStaticFilesHandler(t *testing.T) { } } - handler, err := StaticFilesHandler(ctx, filesys, WithBasePath("/static")) + handler, err := NewStaticFilesHandler(filesys, WithBasePath("/static")) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -152,7 +159,9 @@ func TestStaticFilesHandler(t *testing.T) { t.Run("Serve with Logger", func(t *testing.T) { bufout := new(bytes.Buffer) - logger := slog.New(slog.NewJSONHandler(bufout, &slog.HandlerOptions{})) + logger := slog.New(slog.NewJSONHandler(bufout, &slog.HandlerOptions{ + Level: slog.LevelDebug, + })) // Test WithLogger function wo := WithLogger(logger) @@ -162,7 +171,7 @@ func TestStaticFilesHandler(t *testing.T) { } // Call the StaticFilesHandler function with the logger - handler, err := StaticFilesHandler(ctx, filesys, wo) + handler, err := NewStaticFilesHandler(filesys, wo) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -204,7 +213,7 @@ func TestStaticFilesHandler(t *testing.T) { t.Error("Expected mux error handler to be set, but got nil") } - handler, err := StaticFilesHandler(ctx, filesys, WithMuxErrorHandler(customErrorHandler)) + handler, err := NewStaticFilesHandler(filesys, WithMuxErrorHandler(customErrorHandler)) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -271,12 +280,12 @@ func TestStaticFilesHandler(t *testing.T) { } // Call the StaticFilesHandler function with the web environment - handler, err := StaticFilesHandler(ctx, filesys, WithInjectWebEnv(env, namespace)) + handler, err := NewStaticFilesHandler(filesys, WithInjectWebEnv(env, namespace)) if err != nil { t.Errorf("Unexpected error: %v", err) } - req := httptest.NewRequest(http.MethodGet, "/index.html", nil) + req := httptest.NewRequest(http.MethodGet, "/", nil) w := httptest.NewRecorder() handler.ServeHTTP(w, req)