diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dd79b7..d896711 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * 2.0.next in progress * Fix #323 by supporting more than one SQL entity in `:on-conflict`. + * Fix #321 by adding `:checking` mode. Currently only detects potential problems with `IN` clauses. * 2.0.0-beta2 (for testing; 2021-04-13) * The documentation continues to be expanded and clarified in response to feedback! diff --git a/doc/getting-started.md b/doc/getting-started.md index d5ea8dc..019373f 100644 --- a/doc/getting-started.md +++ b/doc/getting-started.md @@ -297,7 +297,7 @@ HoneySQL supports quite a few [PostgreSQL extensions](postgresql.md). ## Format Options In addition to the `:quoted` and `:dialect` options described above, -`format` also accepts `:inline` and `:params`. +`format` also accepts `:checking`, `:inline`, and `:params`. The `:params` option was mentioned above and is used to specify the values of named parameters in the DSL. @@ -312,6 +312,14 @@ was wrapped in `[:inline `..`]`: * keywords and symbols become SQL keywords (uppercase, with `-` replaced by a space), * everything else is just turned into a string (by calling `str`) and added to the SQL string. +The `:checking` option defaults to `:none`. If `:checking :basic` is +specified, certain obvious errors -- such as `IN` with an empty collection -- +is treated as an error and an exception is thrown. If `:checking :strict` +is specified, certain dubious constructs -- such as `IN` with a collection +containing `NULL` values -- is also treated as an error and an exception is +thrown. It is expected that this feature will be expanded over time +to help avoid generating illegal SQL. _[New in version 2.0.next]_ + `format` accepts options as either a single hash map argument or as named arguments (alternating keys and values). If you are using Clojure 1.11 (or later) you can mix'n'match, providing some options diff --git a/src/honey/sql.cljc b/src/honey/sql.cljc index 1c70232..e46f85c 100644 --- a/src/honey/sql.cljc +++ b/src/honey/sql.cljc @@ -106,6 +106,8 @@ ;; in entities; if someone complains about this check, an option ;; can be added to format to turn this on: (def ^:private ^:dynamic *allow-suspicious-entities* false) +;; "linting" mode (:none, :basic, :strict): +(def ^:private ^:dynamic *checking* :none) ;; clause helpers @@ -929,6 +931,16 @@ (let [[sql-x & params-x] (format-expr x {:nested true}) [sql-y & params-y] (format-expr y {:nested true}) values (unwrap (first params-y) {})] + (when-not (= :none *checking*) + (when (or (and (sequential? y) (empty? y)) + (and (sequential? values) (empty? values))) + (throw (ex-info "IN () empty collection is illegal" + {:clause [in x y]}))) + (when (and (= :strict *checking*) + (or (and (sequential? y) (some nil? y)) + (and (sequential? values) (some nil? values)))) + (throw (ex-info "IN (NULL) does not match" + {:clause [in x y]})))) (if (and (= "?" sql-y) (= 1 (count params-y)) (coll? values)) (let [sql (str "(" (str/join ", " (repeat (count values) "?")) ")")] (-> [(str sql-x " " (sql-kw in) " " sql)] @@ -1228,6 +1240,9 @@ (let [dialect? (contains? opts :dialect) dialect (when dialect? (get dialects (check-dialect (:dialect opts))))] (binding [*dialect* (if dialect? dialect @default-dialect) + *checking* (if (contains? opts :checking) + (:checking opts) + :none) *clause-order* (if dialect? (if-let [f (:clause-order-fn dialect)] (f @base-clause-order) diff --git a/test/honey/sql_test.cljc b/test/honey/sql_test.cljc index 0a73eb9..489807b 100644 --- a/test/honey/sql_test.cljc +++ b/test/honey/sql_test.cljc @@ -6,7 +6,8 @@ #?(:clj [clojure.test :refer [deftest is testing]] :cljs [cljs.test :refer-macros [deftest is testing]]) [honey.sql :as sut :refer [format]] - [honey.sql.helpers :as h])) + [honey.sql.helpers :as h]) + #?(:clj (:import (clojure.lang ExceptionInfo)))) (deftest mysql-tests (is (= ["SELECT * FROM `table` WHERE `id` = ?" 1] @@ -685,9 +686,9 @@ ORDER BY id = ? DESC (format)) (is false "; not detected in entity!") (catch #?(:clj Throwable :cljs :default) e - (is (:disallowed (ex-data e)))))) + (is (:disallowed (ex-data e)))))))) ;; should not produce: ["SELECT foo, bar FROM mytable ORDER BY foo; select * from users"] - )) + (deftest issue-319-test (testing "that registering a clause is idempotent" @@ -696,3 +697,37 @@ ORDER BY id = ? DESC (sut/register-clause! :foo (constantly ["FOO"]) nil) (sut/register-clause! :foo (constantly ["FOO"]) nil) (format {:foo []})))))) + +(deftest issue-321-linting + (testing "empty IN is ignored by default" + (is (= ["WHERE x IN ()"] + (format {:where [:in :x []]}))) + (is (= ["WHERE x IN ()"] + (format {:where [:in :x :?y]} + {:params {:y []}})))) + (testing "empty IN is flagged in basic mode" + (is (thrown-with-msg? ExceptionInfo #"empty collection" + (format {:where [:in :x []]} + {:checking :basic}))) + (is (thrown-with-msg? ExceptionInfo #"empty collection" + (format {:where [:in :x :?y]} + {:params {:y []} :checking :basic})))) + (testing "IN NULL is ignored by default and basic" + (is (= ["WHERE x IN (NULL)"] + (format {:where [:in :x [nil]]}))) + (is (= ["WHERE x IN (NULL)"] + (format {:where [:in :x [nil]]} + {:checking :basic}))) + (is (= ["WHERE x IN (?)" nil] + (format {:where [:in :x :?y]} + {:params {:y [nil]}}))) + (is (= ["WHERE x IN (?)" nil] + (format {:where [:in :x :?y]} + {:params {:y [nil]} :checking :basic})))) + (testing "IN NULL is flagged in strict mode" + (is (thrown-with-msg? ExceptionInfo #"does not match" + (format {:where [:in :x [nil]]} + {:checking :strict}))) + (is (thrown-with-msg? ExceptionInfo #"does not match" + (format {:where [:in :x :?y]} + {:params {:y [nil]} :checking :strict})))))