From 5824d9eeef63596d6dc0277c79805d29d8982428 Mon Sep 17 00:00:00 2001
From: Miikka Koskinen <miikka@metosin.fi>
Date: Fri, 26 Feb 2021 08:14:01 +0200
Subject: [PATCH] Make the not-found-handler fix backwards compatible

PR #471 aimed to fix issue #464. However, the change was slightly
backwards-incompatible, since it made the file and resource handlers use
the default 404 handler when mounted outside of the router. The previous
behavior was to return nil in that case.

This patch restores the previous behavior and clarifies that `:path`
option can be used only when the file/resource handler is mounted
outside of a router.
---
 modules/reitit-ring/src/reitit/ring.cljc |  8 +++--
 test/cljc/reitit/ring_test.cljc          | 40 ++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/modules/reitit-ring/src/reitit/ring.cljc b/modules/reitit-ring/src/reitit/ring.cljc
index 0c6460e8..0e513847 100644
--- 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
index c1d15d06..52dd938b 100644
--- 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