Skip to content

Commit

Permalink
Revert "Cmr 4580 fix parameter validation new (#385)"
Browse files Browse the repository at this point in the history
This reverts commit 1aef221.
  • Loading branch information
chris-durbin committed Dec 8, 2017
1 parent 758d792 commit 7ab919e
Show file tree
Hide file tree
Showing 10 changed files with 10 additions and 128 deletions.
9 changes: 0 additions & 9 deletions common-app-lib/src/cmr/common_app/services/search/params.clj
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,6 @@
(not (and (string? value) (= "" (string/trim value)))))]
(into {} (filter (comp not-empty-string? second) params))))

(defn sanitize-without-removing-empty-params
"Manipulates the parameters to make them easier to process"
[params]
(-> params
u/map-keys->kebab-case
(update-in [:sort-key] #(when % (if (sequential? %)
(map sanitize-sort-key %)
(sanitize-sort-key %))))))

(defn sanitize-params
"Manipulates the parameters to make them easier to process"
[params]
Expand Down
9 changes: 0 additions & 9 deletions common-lib/src/cmr/common/util.clj
Original file line number Diff line number Diff line change
Expand Up @@ -276,15 +276,6 @@
m
m))

(defn select-blank-keys
"Keep the keys with blank values in a map."
[m]
(select-keys m
(for [[k v] m
:when (or (nil? v)
(and (string? v) (string/blank? v)))]
k)))

(defn inflate-nil-keys
"Occupy nil values with a given default value."
[m filler]
Expand Down
9 changes: 1 addition & 8 deletions common-lib/test/cmr/common/test/util.clj
Original file line number Diff line number Diff line change
Expand Up @@ -741,13 +741,6 @@
; We don't want an ExecutionException, we want to get the null pointer exception
(is false "Fast-map threw ExecutionException and should throw NullPointerException")))))))

(deftest select-blank-keys
(testing "select-blank-keys function"
(is (= {:a "" :b " " :c nil}
(util/select-blank-keys {:a "" :b " " :c nil :d "a" :e [1 2 3]})))
(is (= {}
(util/select-blank-keys nil)))))

(deftest max-compare-test
(util/are3
[coll expected-max]
Expand Down Expand Up @@ -787,7 +780,7 @@
(is (= (util/safe-lowercase true) (str/lower-case true)))
(is (= (util/safe-lowercase "StRing") (str/lower-case "StRing")))
(is (= (util/safe-lowercase nil) nil)))
(testing "safe-uppercase"
(testing "safe-upperrcase"
(is (= (util/safe-uppercase false) (str/upper-case false)))
(is (= (util/safe-uppercase true) (str/upper-case true)))
(is (= (util/safe-uppercase "StRing") (str/upper-case "StRing")))
Expand Down
36 changes: 4 additions & 32 deletions search-app/src/cmr/search/routes.clj
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
routes that apply to both (e.g., robots.txt)."
(:require
[clojure.java.io :as io]
[clojure.string :as string]
[clojure.string :as str]
[cmr.acl.core :as acl]
[cmr.common.api.errors :as errors]
[cmr.common-app.api.request-context-user-augmenter :as context-augmenter]
Expand All @@ -29,9 +29,9 @@
[query-str]
(when query-str
(let [query-str (-> query-str
(string/replace #"%5B" "[")
(string/replace #"%5D" "]")
(string/replace #"\[\]" ""))]
(str/replace #"%5B" "[")
(str/replace #"%5D" "]")
(str/replace #"\[\]" ""))]
(last (some #(re-find % query-str)
[#"(^|&)(.*?)=.*?&\2\["
#"(^|&)(.*?)\[.*?&\2="])))))
Expand Down Expand Up @@ -62,33 +62,6 @@
:body-copy body
:body (java.io.ByteArrayInputStream. (.getBytes body)))))))

(defn add-equal-sign-to-params-in-list
"Add = sign to each parameter in the param-list if it is a string
and it's not blank and it doesn't already contain = sign"
[param-list]
(map (fn [param]
(if (and (string? param)
(not (string/blank? param))
(not (string/includes? param "=")))
(str param "=")
param))
param-list))

(defn parameter-without-equal-sign-handler
"Add = sign to each param in the query-string that doesn't contain the = sign so that it could make it
to the params without nil value and get validated."
[handler]
(fn [request]
(let [query-string (some-> (:query-string request)
(string/split #"&")
add-equal-sign-to-params-in-list)
query-string (if (sequential? query-string)
(string/join "&" query-string)
query-string)]
(handler (assoc
request
:query-string query-string)))))

(defn default-error-format
"Determine the format that errors should be returned in based on the request URI."
[{:keys [uri]} _e]
Expand Down Expand Up @@ -131,5 +104,4 @@
(cmr-context/build-request-context-handler system)
common-routes/pretty-print-response-handler
params/wrap-params
parameter-without-equal-sign-handler
copy-of-body-handler))
Original file line number Diff line number Diff line change
Expand Up @@ -714,14 +714,6 @@
concept-type safe-params (parameter-validations concept-type) type-errors))
params)

(defn validate-empty-parameters
"Validates parameters with no values. Only need to validate if the parameter name is valid.
Returns parameters if validation was successful so it can be chained with other calls."
[concept-type params]
(cpv/validate-parameters
concept-type params [cpv/unrecognized-params-validation])
params)

(defn validate-standard-query-parameters
"Validates the query parameters passed in with an AQL or JSON search.
Throws exceptions to send to the user. Returns parameters if validation
Expand Down
18 changes: 0 additions & 18 deletions search-app/src/cmr/search/services/query_service.clj
Original file line number Diff line number Diff line change
Expand Up @@ -144,31 +144,13 @@
[params]
(or (:tag-data params) (:tag_data params)))

(defn- validate-empty-parameters
"validate only the empty valued parameters"
[context concept-type params]
(let [empty-params
(some-> params
u/select-blank-keys
common-params/sanitize-without-removing-empty-params
;; empty equator crossing start date and end date are not useful
;; towards the final replacement of equator-crossing-date.
(dissoc :equator-crossing-start-date :equator-crossing-end-date))]
(some->> empty-params
lp/replace-parameter-aliases
(lp/process-legacy-multi-params-conditions concept-type)
(lp/replace-science-keywords-or-option concept-type)
(psn/replace-provider-short-names context)
(pv/validate-empty-parameters concept-type))))

(defn- create-and-validate-concepts-query-parameters
([context concept-type params]
(->> params
(make-concepts-tag-data)
(create-and-validate-concepts-query-parameters
context concept-type params)))
([context concept-type params tag-data]
(validate-empty-parameters context concept-type params)
(->> params
common-params/sanitize-params
(add-tag-data-to-params tag-data)
Expand Down
18 changes: 3 additions & 15 deletions search-app/test/cmr/search/test/routes.clj
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
(ns cmr.search.test.routes
(:require
[clojure.test :refer :all]
[cmr.common.util :refer [are3]]
[cmr.search.routes :as r])
(:require [clojure.test :refer :all]
[cmr.search.routes :as r])
(:use ring.mock.request))

(def ^:private web (#'cmr.search.routes/build-routes
Expand All @@ -21,14 +19,4 @@
"foo=1&options[foo][pattern]=true" nil
"foo[]=1&options[foo][pattern]=true" nil
"foo=1&bar_foo[baz]=2" nil
"foo[baz]=1&bar_foo=2" nil)))

(deftest add-equal-sign-to-params-in-list
(testing "add-equal-sign-to-params-in-list adds = to each non-blank string param that doesn't contain ="
(are3 [old-param-list new-param-list]
(is (= new-param-list
(r/add-equal-sign-to-params-in-list old-param-list)))
"testing empty list case"
[""] [""]
"testing non-empty list case"
["abc" "123" 456 " "] ["abc=" "123=" 456 " "])))
"foo[baz]=1&bar_foo=2" nil)))
Original file line number Diff line number Diff line change
Expand Up @@ -404,22 +404,6 @@
(let [{:keys [status errors]} (search/find-refs :collection {"temporal[]" "2010-13-12T12:00:00,"})]
(is (= 400 status))
(is (re-find #"temporal start datetime is invalid: \[2010-13-12T12:00:00\] is not a valid datetime" (first errors)))))
(testing "search by empty invalid parameter."
(let [{:keys [status errors]} (search/find-concepts-json :collection {"tempora" ""})]
(is (= 400 status))
(is (= "Parameter [tempora] was not recognized." (first errors)))))
(testing "search by nil invalid parameter."
(let [{:keys [status errors]} (search/find-concepts-json :collection {"tempora" nil})]
(is (= 400 status))
(is (= "Parameter [tempora] was not recognized." (first errors)))))
(testing "search by empty valid parameter."
(let [{:keys [status errors]} (search/find-concepts-json :collection {"temporal" ""})]
(is (= 200 status))
(is (= nil (first errors)))))
(testing "search by nil valid parameter."
(let [{:keys [status errors]} (search/find-concepts-json :collection {"temporal" nil})]
(is (= 200 status))
(is (= nil (first errors)))))
(testing "periodic temporal search that produces empty search ranges."
(let [{:keys [status errors]} (search/find-refs :collection
{"temporal[]" "2016-05-10T08:18:16Z,2016-05-10T08:34:31Z,131,131"})]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,19 +319,9 @@
(testing "invalid include-facets"
(is (= {:errors ["Parameter include_facets must take value of true, false, or v2, but was [foo]"] :status 400}
(search/find-refs :collection {:include-facets "foo"})))
(is (= {:errors ["Parameter [invalid_param] was not recognized."] :status 400}
(search/find-refs :collection {:include-facets "v2" :invalid-param ""})))
(is (= {:errors ["Parameter [invalid_param] was not recognized."] :status 400}
(search/find-refs :collection {:include-facets "v2" :invalid-param nil})))
(is (= {:errors ["Parameter [include_facets] was not recognized."] :status 400}
(search/find-refs :granule {:include-facets true}))))

(testing "valid include-facets"
(is (= 200
(:status (search/find-concepts-json :collection {:include-facets "v2" :concept-id ""}))))
(is (= 200
(:status (search/find-concepts-json :collection {:include-facets "v2" :concept-id nil})))))

(testing "retreving all facets in different formats"
(let [expected-facets [{:field "data_center"
:value-counts [["Larc" 3] ["Dist" 1] ["GSFC" 1] ["Proc" 1]]}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,10 +319,9 @@
",0.7" [gran4 gran5]
"-70.0,31.0" [gran1 gran2 gran4 gran5]
"-70.0,120.0" [gran1 gran2 gran3 gran4 gran5]
;; Empty and nil cloud cover is allowed.
;; Empty cloud cover is allowed.
;; It is as if no cloud cover parameter is present and will find everything.
"" [gran1 gran2 gran3 gran4 gran5 gran6]
nil [gran1 gran2 gran3 gran4 gran5 gran6]))
"" [gran1 gran2 gran3 gran4 gran5 gran6]))

(testing "search by cloud-cover with min value greater than max value"
(let [min-value 30.0
Expand Down

0 comments on commit 7ab919e

Please sign in to comment.