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.
jrschumacher · Oct 4, 2024 · fe7c839 · fe7c839
1 parent b5d1de7
commit fe7c839
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 78 deletions.
diff --git a/logger.go 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
@@ -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
@@ -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)