Refactor and address #127

lib/builder_web.ml

@@ -302,11 +302,7 @@ let routes ~datadir ~cachedir ~configdir =
     |> string_of_html |> Dream.html |> Lwt_result.ok
   in
 
-  let redirect_latest req =
+  let redirect_latest req ~job_name ~platform ~artifact =
-    let job_name = Dream.param req "job" in
-    let platform = Dream.query req "platform" in
-    (* FIXME *)
-    let artifact = begin[@alert "-deprecated"] Dream.path req |> String.concat "/" end in
     (Dream.sql req (Model.job_id job_name) >>= Model.not_found >>= fun job_id ->
      Dream.sql req (Model.latest_successful_build_uuid job_id platform))
     >>= Model.not_found
@@ -316,13 +312,32 @@ let routes ~datadir ~cachedir ~configdir =
     |> Lwt_result.ok
   in
 
+  let redirect_latest req =
+    let job_name = Dream.param req "job" in
+    let platform = Dream.query req "platform" in
+    let artifact =
+      (* FIXME Dream.path deprecated *)
+      let path = begin[@alert "-deprecated"] Dream.path req end in
+      if path = [] then
+        "" (* redirect without trailing slash *)
+      else
+        "/" ^ (List.map Uri.pct_encode path |> String.concat "/")
+    in
+    redirect_latest req ~job_name ~platform ~artifact
+
+  and redirect_latest_no_slash req =
+    let job_name = Dream.param req "job" in
+    let platform = Dream.query req "platform" in
+    redirect_latest req ~job_name ~platform ~artifact:""
+  in
+
   let redirect_main_binary req =
     let job_name = Dream.param req "job"
     and build = Dream.param req "build" in
     get_uuid build >>= fun uuid ->
     Dream.sql req (main_binary_of_uuid uuid) >>= fun main_binary ->
-    let filepath = main_binary.Builder_db.filepath in
+    let artifact = `File main_binary.Builder_db.filepath in
-    Link.Job_build_f.make ~job_name ~build:uuid ~filepath ()
+    Link.Job_build_artifact.make ~job_name ~build:uuid ~artifact ()
     |> Dream.redirect req
     |> Lwt_result.ok
   in
@@ -597,6 +612,7 @@ let routes ~datadir ~cachedir ~configdir =
     `Get, "/job/:job/build", (w redirect_parent);
     `Get, "/job/:job/failed", (w job_with_failed);
     `Get, "/job/:job/build/latest/**", (w redirect_latest);
+    `Get, "/job/:job/build/latest", (w redirect_latest_no_slash);
     `Get, "/job/:job/build/:build", (w job_build);
     `Get, "/job/:job/build/:build/f/**", (w job_build_file);
     `Get, "/job/:job/build/:build/main-binary", (w redirect_main_binary);
@@ -619,7 +635,8 @@ let to_dream_route = function
 let to_dream_routes l = List.map to_dream_route l
 
 let routeprefix_ignorelist_when_removing_trailing_slash = [
-  "/job/:job/build/:build/f"
+  "/job/:job/build/:build/f";
+  "/job/:job/build/latest";
 ]
 
 module Middleware = struct

lib/link.ml

@@ -52,15 +52,16 @@ end
 module Job_build_artifact = struct
 
   let encode_artifact = function
-    | `Main_binary -> "main-binary"
+    | `Main_binary -> "/main-binary"
-    | `Viz_treemap -> "viztreemap"
+    | `Viz_treemap -> "/viztreemap"
-    | `Viz_dependencies -> "vizdependencies"
+    | `Viz_dependencies -> "/vizdependencies"
-    | `Script -> "script"
+    | `Script -> "/script"
-    | `Console -> "console"
+    | `Console -> "/console"
-    | `All_targz -> "all.tar.gz"
+    | `All_targz -> "/all.tar.gz"
+    | `File f -> "/f/" ^ Fmt.to_to_string fpath_url_pp f
 
   let make_from_string ~job_name ~build ~artifact () =
-    Fmt.str "/job/%a/build/%a/%s"
+    Fmt.str "/job/%a/build/%a%s"
       pctencode job_name
       Uuidm.pp build
       artifact
@@ -71,16 +72,6 @@ module Job_build_artifact = struct
 
 end
 
-module Job_build_f = struct
-
-  let make ~job_name ~build ~filepath () =
-    Fmt.str "/job/%a/build/%a/f/%a"
-      pctencode job_name
-      Uuidm.pp build
-      fpath_url_pp filepath
-
-end
-
 module Compare_builds = struct
 
   let make ~left ~right () =

lib/views.ml

@@ -191,10 +191,10 @@ let artifact
     ~file:{ Builder_db.filepath; localpath = _; sha256; size }
   =
   let artifact_link =
-    Link.Job_build_f.make
+    Link.Job_build_artifact.make
       ~job_name
       ~build:build.Builder_db.Build.uuid
-      ~filepath ()
+      ~artifact:(`File filepath) ()
   in
   [
     H.a ~a:H.[a_href artifact_link] [
@@ -476,10 +476,10 @@ module Job_build = struct
       let (`Hex sha256_hex) = Hex.of_cstruct file.sha256 in
       [
         H.dt [
-          H.a ~a:H.[a_href @@ Link.Job_build_f.make
+          H.a ~a:H.[a_href @@ Link.Job_build_artifact.make
                       ~job_name
                       ~build:build_uuid
-                      ~filepath:file.filepath ()
+                      ~artifact:(`File file.filepath) ()
                    ]
             [H.code [txtf "%a" Fpath.pp file.filepath]] ];
         H.dd ([

test/router.ml

@@ -110,7 +110,7 @@ let () =
         test_case "Link.Job_build_artifact.make_from_string" `Quick begin
           let job_name = "test" in
           let build = Uuidm.v `V4 in
-          let artifact = "all.tar.gz" in
+          let artifact = "" in
           test_link `GET @@
           Builder_web.Link.Job_build_artifact.make_from_string
             ~job_name ~build ~artifact ()
@@ -123,10 +123,11 @@ let () =
           `Script;
           `Console;
           `All_targz;
+          `File Fpath.(v "some" / "path");
         ]
         |> List.map (fun artifact ->
             let descr =
-              Fmt.str "Link..artifact..: %s" @@
+              Fmt.str "Job_build_artifact.make: %s" @@
               Builder_web.Link.Job_build_artifact.encode_artifact artifact
             in
             test_case descr `Quick begin
@@ -134,13 +135,6 @@ let () =
             end
           )
       ) @ Alcotest.[
-        test_case "Link.Job_build_f.make" `Quick begin
-          let job_name = "test" in
-          let build = Uuidm.v `V4 in
-          let filepath = Fpath.(v "some" / "path") in
-          test_link `GET @@
-          Builder_web.Link.Job_build_f.make ~job_name ~build ~filepath ()
-        end;
         test_case "Link.Compare_builds.make" `Quick begin
           let left = Uuidm.v `V4 in
           let right = Uuidm.v `V4 in
@@ -152,4 +146,12 @@ let () =
           Builder_web.Link.Failed_builds.make ~count:2 ~start:1 () 
         end;
       ];
+    (* this doesn't actually test the redirects, unfortunately *)
+    "Latest", List.map (fun p -> Alcotest.(test_case ("…"^p) `Quick begin
+        test_link `GET @@ "/job/test/build/latest" ^ p end))
+      [
+        "/f/bin/unikernel.hvt";
+        "/";
+        "";
+      ]
   ]