fix: query string handling in static file middleware #591
Conversation
- can be removed once rgrinberg/opium#297 has been merged
There was a problem hiding this comment.
Pull request overview
This PR introduces a custom static file middleware to fix query string handling until an upstream fix in opium (PR #297) is merged. The implementation strips query parameters from file paths before serving static assets, allowing cache-busting URLs like /style.css?v=123 to work correctly.
Key Changes
- Implements a custom Body module with file serving logic copied from Opium
- Adds query string stripping logic in the file read handler
- Replaces the default Opium static file middleware with the custom implementation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pool/web/middleware/middleware_static.ml | New custom middleware that strips query strings from static file requests before serving them |
| pool/web/middleware/middleware.ml | Exposes the new Static middleware module |
| pool/routes/routes.ml | Replaces Middleware.static_file() with CustomMiddleware.Static.middleware() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| module Body = struct | ||
| include Rock.Body | ||
|
|
||
| let log_src = Logs.Src.create "opium.body.of_file" | ||
|
|
||
| module Log = (val Logs.src_log log_src : Logs.LOG) | ||
|
|
||
| exception Isnt_a_file | ||
|
|
||
| let of_file fname = | ||
| let open Lwt.Syntax in | ||
| (* TODO: allow buffer size to be configurable *) | ||
| let bufsize = 4096 in | ||
| Lwt.catch | ||
| (fun () -> | ||
| let* s = Lwt_unix.stat fname in | ||
| let* () = | ||
| if Unix.(s.st_kind <> S_REG) then Lwt.fail Isnt_a_file else Lwt.return_unit | ||
| in | ||
| let* ic = | ||
| Lwt_io.open_file | ||
| ~buffer:(Lwt_bytes.create bufsize) | ||
| ~flags:[ Unix.O_RDONLY ] | ||
| ~mode:Lwt_io.input | ||
| fname | ||
| in | ||
| let+ size = Lwt_io.length ic in | ||
| let stream = | ||
| Lwt_stream.from (fun () -> | ||
| Lwt.catch | ||
| (fun () -> | ||
| let+ b = Lwt_io.read ~count:bufsize ic in | ||
| match b with | ||
| | "" -> None | ||
| | buf -> Some buf) | ||
| (fun exn -> | ||
| Log.warn (fun m -> | ||
| m "Error while reading file %s. %s" fname (Printexc.to_string exn)); | ||
| Lwt.return_none)) | ||
| in | ||
| Lwt.on_success (Lwt_stream.closed stream) (fun () -> | ||
| Lwt.async (fun () -> Lwt_io.close ic)); | ||
| Some (of_stream ~length:size stream)) | ||
| (fun e -> | ||
| match[@warning "-4"] e with | ||
| | Isnt_a_file | Unix.Unix_error (Unix.ENOENT, _, _) -> Lwt.return None | ||
| | exn -> | ||
| Logs.err (fun m -> | ||
| m "Unknown error while serving file %s. %s" fname (Printexc.to_string exn)); | ||
| Lwt.fail exn) | ||
| ;; | ||
| end |
There was a problem hiding this comment.
The Body module duplicates Opium's body handling logic. While this is likely intentional as a workaround, consider adding a comment at the module level explaining that this is copied/adapted from Opium and should be removed when the upstream fix is merged. This will help future maintainers understand why this duplication exists.
| let default_etag ~local_path fname = | ||
| let fpath = Filename.concat local_path fname in | ||
| let* exists = Lwt_unix.file_exists fpath in | ||
| if exists | ||
| then | ||
| let* stat = Lwt_unix.stat fpath in | ||
| let hash = | ||
| Marshal.to_string stat.Unix.st_mtime [] | ||
| |> Digestif.MD5.digest_string | ||
| |> Digestif.MD5.to_raw_string | ||
| |> Base64.encode_exn | ||
| in | ||
| Lwt.return_some hash | ||
| else Lwt.return_none | ||
| ;; |
There was a problem hiding this comment.
The etag_of_fname function receives a filename without query string processing, but it should also strip query strings to ensure consistent etag generation. When the middleware processes a request with query parameters (e.g., "/style.css?v=123"), the read function strips the query string before serving the file, but the etag_of_fname will still receive the original filename with query string. This could lead to etag cache misses or inconsistencies.
| let m ~local_path ?uri_prefix ?headers ?(etag_of_fname = default_etag ~local_path) () = | ||
| let read fname = | ||
| let chop_query_string s = | ||
| match String.index_opt s '?' with | ||
| | Some idx -> String.sub s 0 idx | ||
| | None -> s | ||
| in |
There was a problem hiding this comment.
The chop_query_string function is defined as a nested function inside the read function, which means it's recreated on every request. Consider extracting this helper function to the module level to avoid unnecessary allocations and improve performance, especially since this middleware processes all static file requests.
| let m ~local_path ?uri_prefix ?headers ?(etag_of_fname = default_etag ~local_path) () = | |
| let read fname = | |
| let chop_query_string s = | |
| match String.index_opt s '?' with | |
| | Some idx -> String.sub s 0 idx | |
| | None -> s | |
| in | |
| let chop_query_string s = | |
| match String.index_opt s '?' with | |
| | Some idx -> String.sub s 0 idx | |
| | None -> s | |
| ;; | |
| let m ~local_path ?uri_prefix ?headers ?(etag_of_fname = default_etag ~local_path) () = | |
| let read fname = |
| let m ~local_path ?uri_prefix ?headers ?(etag_of_fname = default_etag ~local_path) () = | ||
| let read fname = | ||
| let chop_query_string s = | ||
| match String.index_opt s '?' with | ||
| | Some idx -> String.sub s 0 idx | ||
| | None -> s | ||
| in | ||
| let fname = fname |> chop_query_string in | ||
| let* body = Body.of_file (Filename.concat local_path fname) in | ||
| match body with | ||
| | None -> Lwt.return (Error `Not_found) | ||
| | Some body -> Lwt.return (Ok body) | ||
| in | ||
| Opium.Middleware.static ~read ?uri_prefix ?headers ~etag_of_fname () | ||
| ;; | ||
|
|
||
| let middleware = | ||
| let local_path = | ||
| Option.value (Sihl.Configuration.read_string "PUBLIC_DIR") ~default:"./public" | ||
| in | ||
| let internal_uri_prefix = | ||
| Option.value (Sihl.Configuration.read_string "PUBLIC_URI_PREFIX") ~default:"/assets" | ||
| in | ||
| m ~local_path ~uri_prefix:internal_uri_prefix | ||
| ;; |
There was a problem hiding this comment.
The new static file middleware lacks test coverage for the query string handling fix, which is the main purpose of this PR. Consider adding tests that verify static files can be served correctly when the request includes query parameters (e.g., "/style.css?v=123" should serve the same file as "/style.css"), especially since the repository has comprehensive test coverage for other middleware and components.
| let m ~local_path ?uri_prefix ?headers ?(etag_of_fname = default_etag ~local_path) () = | ||
| let read fname = | ||
| let chop_query_string s = | ||
| match String.index_opt s '?' with | ||
| | Some idx -> String.sub s 0 idx | ||
| | None -> s | ||
| in | ||
| let fname = fname |> chop_query_string in | ||
| let* body = Body.of_file (Filename.concat local_path fname) in | ||
| match body with | ||
| | None -> Lwt.return (Error `Not_found) | ||
| | Some body -> Lwt.return (Ok body) | ||
| in | ||
| Opium.Middleware.static ~read ?uri_prefix ?headers ~etag_of_fname () | ||
| ;; |
There was a problem hiding this comment.
The m function and its parameters lack documentation explaining its purpose and usage. Given that this is a workaround for an upstream issue, it would be helpful to document why this custom implementation exists, what problem it solves (query string handling), and that it's intended to be temporary until opium PR #297 is merged.
|
Upstream MR has been merged |
should the following PR (rgrinberg/opium#297) not be merged soon this PR serves to fix the error in the Pool Tool directly.
The Middleware should however be removed as soon as a new Version of Opium is released