Code review of ring-core: bug fixes, doc fixes and additions, and mor…

…e consistent style.
mmcgrana · 06a1d46ecd50595f8314cdb710d75bed92de5f41 · mmcgrana committed Feb 28, 2010 · Feb 28, 2010 · 06a1d46
1 parent d65f4fc
commit 06a1d46ecd50595f8314cdb710d75bed92de5f41
@@ -1,6 +1,7 @@
 (ns ring.middleware.cookies
-  (:use clojure.contrib.def)
-  (:use clojure.contrib.java-utils))
+  "Cookie manipulation."
+  (:use [clojure.contrib.def :only (defvar-)])
+  (:require [clojure.contrib.java-utils :as ju]))
 
 (defvar- re-token #"[!#$%&'*\-+.0-9A-Z\^_`a-z\|~]+"
   "HTTP token: 1*<any CHAR except CTLs or tspecials>. See RFC2068")
@@ -74,7 +75,7 @@
 (defn- write-attr
   "Turn a name-value pair into a cookie attr string."
   [name value]
-  (str (as-str name) "=" (pr-str value)))
+  (str (ju/as-str name) "=" (pr-str value)))
 
 (defn- write-attr-map
   "Write a map of cookie attributes to a string."
@@ -99,7 +100,7 @@
   "Add a Set-Cookie header to a response if there is a :cookies key."
   [response]
   (if-let [cookies (:cookies response)]
-    (assoc-in response 
+    (assoc-in response
               [:headers "Set-Cookie"]
               (write-cookies cookies))
     response))
@@ -1,24 +1,22 @@
 (ns ring.middleware.file
+  "Static file serving."
   (:use (clojure.contrib [def :only (defvar-)]
                          [except :only (throw-if-not)])
         ring.util.response)
-  (:require (ring.util [codec :as codec]))
+  (:require [ring.util.codec :as codec]
+            [clojure.contrib.java-utils :as ju])
   (:import java.io.File))
 
-(defn- str-includes?
-  "Returns logical truth iff the given target appears in the given string"
-  [#^String target #^String string]
-  (<= 0 (.indexOf string target)))
+(defn- substring?
+  "Returns true if sub is a substring of string."
+  [sub #^String string]
+  (.contains string sub))
 
 (defn- ensure-dir
-  "Ensures that the given directory exists and returns it, throwing if it does
-  not. If the directory is given as a String, it is coerced to a Fil before
-  returning."
+  "Ensures that a given directory exists, throwing if it does not."
   [dir]
-  (let [#^File fdir (if (string? dir) (File. #^String dir) dir)]
-    (throw-if-not (.exists fdir)
-      "Directory does not exist: %s" fdir)
-    fdir))
+  (let [fdir (ju/file dir)]
+    (throw-if-not (.exists fdir) "Directory does not exist: %s" fdir)))
 
 (defvar- forbidden
   (-> (response "<html><body><h1>403 Forbidden</h1></body></html>")
@@ -28,26 +26,26 @@
 (defn- maybe-file
   "Returns the File corresponding to the given relative path within the given
   dir if it exists, or nil if no such file exists."
-  [#^File dir #^String path]
-  (let [file (File. dir path)]
+  [dir path]
+  (let [file (ju/file (str dir path))]
     (and (.exists file) (.canRead file) file)))
 
 (defn wrap-file
   "Wrap an app such that a given directory is checked for a static file
   with which to respond to the request, proxying the request to the
   wrapped app if such a file does not exist."
   [app dir]
-  (let [fdir (ensure-dir dir)]
-    (fn [req]
-      (if (#{:get :head} (:request-method req))
-        (let [uri (codec/url-decode (:uri req))]
-          (if (str-includes? ".." uri)
-            forbidden
-            (let [path (cond
-                         (.endsWith "/" uri)        (str uri "index.html")
-                         (re-find #"\.[a-z]+$" uri) uri
-                         :else                      (str uri ".html"))]
-              (if-let [file (maybe-file fdir path)]
-                (response file)
-                (app req)))))
-        (app req)))))
+  (ensure-dir dir)
+  (fn [req]
+    (if (#{:get :head} (:request-method req))
+      (let [uri (codec/url-decode (:uri req))]
+        (if (substring? ".." uri)
+          forbidden
+          (let [path (cond
+                       (.endsWith "/" uri)        (str uri "index.html")
+                       (re-find #"\.[a-z]+$" uri) uri
+                       :else                      (str uri ".html"))]
+            (if-let [file (maybe-file dir path)]
+              (response file)
+              (app req)))))
+      (app req))))
@@ -1,5 +1,6 @@
 (ns ring.middleware.file-info
-  (:use clojure.contrib.def)
+  "Augment Ring File responses."
+  (:use [clojure.contrib.def :only (defvar-)])
   (:import java.io.File))
 
 (defvar- base-mime-types
@@ -1,4 +1,5 @@
-(ns ring.middleware.keyword-params)
+(ns ring.middleware.keyword-params
+  "Convert param keys to keywords.")
 
 (defn- keyify-params [target]
   (cond
@@ -15,8 +16,8 @@
 
 (defn wrap-keyword-params
   "Middleware that converts the string-keyed :params map to one with keyword
-   keys before forwarding the request to the given handler.
-   Does not alter the maps under :*-params keys; these are left with strings."
+  keys before forwarding the request to the given handler.
+  Does not alter the maps under :*-params keys; these are left with strings."
   [handler]
   (fn [req]
     (handler (update-in req [:params] keyify-params))))
@@ -1,12 +1,12 @@
 (ns ring.middleware.multipart-params
-  (:use clojure.contrib.def
+  "Parse multipart upload into params."
+  (:use [clojure.contrib.def :only (defvar-)]
         [ring.middleware.params :only (assoc-param)])
-  (:import [org.apache.commons.fileupload
-              FileUpload
-              RequestContext]
-           [org.apache.commons.fileupload.disk
-              DiskFileItemFactory
-              DiskFileItem]))
+  (:import (org.apache.commons.fileupload
+             FileUpload RequestContext)
+           (org.apache.commons.fileupload.disk
+             DiskFileItemFactory
+             DiskFileItem)))
 
 (defn- multipart-form?
   "Does a request have a multipart form?"
@@ -1,6 +1,7 @@
 (ns ring.middleware.params
-  (:use (clojure.contrib def duck-streams str-utils))
-  (:require (ring.util [codec :as codec])))
+  "Parse form and query params."
+  (:require (clojure.contrib [str-utils :as str] [duck-streams :as duck])
+            (ring.util [codec :as codec])))
 
 (defn assoc-param
   "Associate a key with a value. If the key already exists in the map,
@@ -24,7 +25,7 @@
           (codec/url-decode (or val "") encoding))
          param-map))
     {}
-    (re-split #"&" param-string)))
+    (str/re-split #"&" param-string)))
 
 (defn- assoc-query-params
   "Parse and assoc parameters from the query string with the request."
@@ -46,7 +47,7 @@
   [request encoding]
   (merge-with merge request
     (if-let [body (and (urlencoded-form? request) (:body request))]
-      (let [params (parse-params (slurp* body) encoding)]
+      (let [params (parse-params (duck/slurp* body) encoding)]
         {:form-params params, :params params})
       {:form-params {}, :params {}})))
 
@@ -1,4 +1,5 @@
 (ns ring.middleware.session
+  "Session manipulation."
   (:use ring.middleware.cookies
         ring.middleware.session.memory))
 
@@ -1,6 +1,6 @@
 (ns ring.middleware.session.cookie
   "Encrypted cookie session storage."
-  (:use clojure.contrib.def)
+  (:use [clojure.contrib.def :only (defvar-)])
   (:require (ring.util [codec :as codec]))
   (:import java.security.SecureRandom
            (javax.crypto Cipher Mac)
@@ -1,5 +1,6 @@
 (ns ring.middleware.static
-  (:use (ring.middleware file)))
+  "Static file serving, more selective than ring.middleware.file."
+  (:use ring.middleware.file))
 
 (defn wrap-static
   "Like ring.file, but takes an additional statics, a coll of Strings that will
@@ -14,4 +15,4 @@
       (let [#^String uri (:uri req)]
         (if (some #(.startsWith uri %) statics)
           (app-with-file req)
-          (app req))))))
+          (app req))))))
@@ -1,5 +1,7 @@
 (ns ring.util.codec
-  (:import java.io.File java.net.URLDecoder
+  "Encoding and decoding utilities."
+  (:import java.io.File
+           java.net.URLDecoder
            org.apache.commons.codec.binary.Base64))
 
 (defn url-decode
@@ -1,30 +0,0 @@
-(ns ring.util.file
-  (:import java.io.File))
-
-(defn safe-path?
-  "Is a filepath safe for a particular root?"
-  [root path]
-  (.startsWith (.getCanonicalPath (File. root path))
-               (.getCanonicalPath (File. root))))
-
-(defn find-index-file
-  "Search the directory for an index file."
-  [dir]
-  (first
-    (filter
-      #(.startsWith (.toLowerCase (.getName %)) "index.")
-       (.listFiles dir))))
-
-(defn get-file
-  "Safely retrieve the correct file. See ring.util.response/static-file for an
-  explanation of options."
-  [path opts]
-  (let [file (if-let [root (:root opts)]
-               (if (safe-path? root path)
-                 (File. root path))
-               (File. path))]
-    (if (.exists file)
-      (if (.isDirectory file)
-        (if (:index-files? opts true)
-          (find-index-file file))
-        file))))
@@ -1,5 +1,6 @@
 (ns ring.util.response
-  (:require (ring.util [file :as file])))
+  "Generate and augment Ring responses."
+  (:import java.io.File))
 
 ; Ring responses
 
@@ -18,14 +19,42 @@
    :body    body
    :headers {}})
 
+(defn- safe-path?
+  "Is a filepath safe for a particular root?"
+  [root path]
+  (.startsWith (.getCanonicalPath (File. root path))
+               (.getCanonicalPath (File. root))))
+
+(defn- find-index-file
+  "Search the directory for an index file."
+  [dir]
+  (first
+    (filter
+      #(.startsWith (.toLowerCase (.getName %)) "index.")
+       (.listFiles dir))))
+
+(defn- get-file
+  "Safely retrieve the correct file. See static-file for an
+  explanation of options."
+  [path opts]
+  (let [file (if-let [root (:root opts)]
+               (if (safe-path? root path)
+                 (File. root path))
+               (File. path))]
+    (if (.exists file)
+      (if (.isDirectory file)
+        (if (:index-files? opts true)
+          (find-index-file file))
+        file))))
+
 (defn static-file
   "Returns a Ring response to serve a static file, or nil if the file does
   not exist.
   Options:
     :root         - take the filepath relative to this root path
     :index-files? - look for index.* files in directories, defaults to true"
   [filepath & [opts]]
-  (if-let [file (file/get-file filepath opts)]
+  (if-let [file (get-file filepath opts)]
     (response file)))
 
 ; Ring response augmenters
@@ -1,7 +1,7 @@
 (ns ring.middleware.file-info-test
-  (:use (clojure test)
-        (ring.middleware file-info))
-  (:import (java.io File)))
+  (:use clojure.test
+        ring.middleware.file-info)
+  (:import java.io.File))
 
 (def non-file-app (wrap-file-info (constantly {:headers {} :body "body"})))
 
@@ -1,7 +1,7 @@
 (ns ring.middleware.file-test
-  (:use (clojure test)
-        (ring.middleware file))
-  (:import (java.io File)))
+  (:use clojure.test
+        ring.middleware.file)
+  (:import java.io.File))
 
 (deftest wrap-file-no-directory
   (is (thrown-with-msg? Exception #".*Directory does not exist.*"
@@ -1,7 +1,7 @@
 (ns ring.middleware.params-test
-  (:use (clojure test)
-        (ring.middleware params))
-  (:import (java.io ByteArrayInputStream)))
+  (:use clojure.test
+        ring.middleware.params)
+  (:import java.io.ByteArrayInputStream))
 
 (defn- str-input-stream [#^String s]
   (ByteArrayInputStream. (.getBytes s)))
@@ -1,7 +1,7 @@
 (ns ring.middleware.static-test
-  (:use (clojure test)
-        (ring.middleware static))
-  (:import (java.io File)))
+  (:use clojure.test
+        ring.middleware.static)
+  (:import java.io.File))
 
 (def public-dir      (File. "test/ring/assets"))
 (def foo-html        (File. "test/ring/assets/foo.html"))
@@ -16,4 +16,4 @@
 (deftest wrap-static-smoke
   (is (= foo-html        (app-response-body "/foo.html")))
   (is (= nested-foo-html (app-response-body "/bars/foo.html")))
-  (is (= :dynamic        (app-response-body "/not/static"))))
+  (is (= :dynamic        (app-response-body "/not/static"))))