Merge pull request metosin#473 from metosin/feature/finish-not-found

Make the not-found-handler fix backwards compatible
cheolhee · Mar 2, 2021 · 6f2e181 · 6f2e181
2 parents 0c93cfb + b6d9707
commit 6f2e181
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 4 deletions.
diff --git a/doc/ring/static.md b/doc/ring/static.md
@@ -52,7 +52,7 @@ A better way to serve files from conflicting paths, e.g. `"/*"`, is to serve the
 | -----------------|-------------|
 | :parameter         | optional name of the wildcard parameter, defaults to unnamed keyword `:`
 | :root              | optional resource root, defaults to `\"public\"`
-| :path              | optional path to mount the handler to. Works only if mounted outside of a router.
+| :path              | path to mount the handler to. Required when mounted outside of a router, does not work inside a router.
 | :loader            | optional class loader to resolve the resources
 | :index-files       | optional vector of index-files to look in a resource directory, defaults to `[\"index.html\"]`
 | :not-found-handler | optional handler function to use if the requested resource is missing (404 Not Found)

diff --git a/modules/reitit-ring/src/reitit/ring.cljc b/modules/reitit-ring/src/reitit/ring.cljc
@@ -195,7 +195,9 @@
                         root "public"
                         index-files ["index.html"]
                         paths (constantly nil)
-                        not-found-handler (constantly {:status 404, :body "", :headers {}})}}]
+                        not-found-handler (if path
+                                            (constantly nil)
+                                            (constantly {:status 404, :body "", :headers {}}))}}]
      (let [options {:root root
                     :loader loader
                     :index-files? false
@@ -239,7 +241,7 @@
      | -------------------|-------------|
      | :parameter         | optional name of the wildcard parameter, defaults to unnamed keyword `:`
      | :root              | optional resource root, defaults to `\"public\"`
-     | :path              | optional path to mount the handler to. Works only if mounted outside of a router.
+     | :path              | path to mount the handler to. Required when mounted outside of a router, does not work inside a router.
      | :loader            | optional class loader to resolve the resources
      | :index-files       | optional vector of index-files to look in a resource directory, defaults to `[\"index.html\"]`
      | :not-found-handler | optional handler function to use if the requested resource is missing (404 Not Found)"
@@ -256,7 +258,7 @@
      | -------------------|-------------|
      | :parameter         | optional name of the wildcard parameter, defaults to unnamed keyword `:`
      | :root              | optional resource root, defaults to `\"public\"`
-     | :path              | optional path to mount the handler to. Works only if mounted outside of a router.
+     | :path              | path to mount the handler to. Required when mounted outside of a router, does not work inside a router.
      | :loader            | optional class loader to resolve the resources
      | :index-files       | optional vector of index-files to look in a resource directory, defaults to `[\"index.html\"]`
      | :not-found-handler | optional handler function to use if the requested resource is missing (404 Not Found)"

diff --git a/test/cljc/reitit/ring_test.cljc b/test/cljc/reitit/ring_test.cljc
@@ -618,6 +618,46 @@
                      (is (get-in @result [:headers "Last-Modified"]))
                      (is (= "<xml><hello>file</hello></xml>\n" (slurp (:body @result))))))))))))))
 
+
+#?(:clj
+  (deftest file-resource-handler-not-found-test
+    (let [redirect (fn [uri] {:status 302, :body "", :headers {"Location" uri}})
+          request (fn [uri] {:uri uri, :request-method :get})
+          not-found-handler (fn [_] {:status 404, :body "not-found-handler"})]
+
+       (doseq [[name create] [["resource-handler" ring/create-resource-handler]
+                              ["file-handler" #(ring/create-file-handler (assoc % :root "dev-resources/public"))]]]
+         (testing (str "for " name)
+           (testing "inside a router"
+             (let [create-app (fn [handler]
+                                (ring/ring-handler
+                                 (ring/router
+                                  ["/files/*" handler])))]
+               (testing "not-found-handler not set"
+                 (let [app (create-app (create nil))]
+                   (is (nil? (app (request "/not-found"))))
+                   (is (= "" (:body (app (request "/files/not-found")))))))
+
+               (testing "not-found-handler set"
+                 (let [app (create-app (create {:not-found-handler not-found-handler}))]
+                   (is (nil? (app (request "/not-found"))))
+                   (is (= "not-found-handler" (:body (app (request "/files/not-found")))))))))
+
+           (testing "outside a router"
+             (let [create-app (fn [handler]
+                                (ring/ring-handler
+                                 (ring/router [])
+                                 handler))]
+               (testing "not-found-handler not set"
+                 (let [app (create-app (create {:path "/files"}))]
+                   (is (nil? (app (request "/not-found"))))
+                   (is (nil? (app (request "/files/not-found"))))))
+
+               (testing "not-found-handler set"
+                 (let [app (create-app (create {:path "/files" :not-found-handler not-found-handler}))]
+                   (is (nil? (app (request "/not-found"))))
+                   (is (= "not-found-handler" (:body (app (request "/files/not-found"))))))))))))))
+
 (deftest router-available-in-default-branch
   (testing "1-arity"
     ((ring/ring-handler