check for empty where clauses fix #413

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changes
 
+* 2.3.next in progress
+  * Address [#413](https://github.com/seancorfield/honeysql/issues/413) by flagging a lack of `WHERE` clause for `DELETE`, `DELETE FROM`, and `UPDATE` when `:checking :basic` (or `:checking :strict`).
+
 * 2.3.911 -- 2022-07-29
   * Address [#418](https://github.com/seancorfield/honeysql/issues/418) by documenting a potential "gotcha" with multi-column `IN` expressions (a change from HoneySQL 1.x).
   * Fix [#416](https://github.com/seancorfield/honeysql/issues/416) via PR [#417](https://github.com/seancorfield/honeysql/issues/417) from [@corasaurus-hex](https://github.com/corasaurus-hex) -- using the internal default state for the integrity assertion.

doc/options.md

@@ -42,15 +42,18 @@ Added in 2.2.858.
 
 ## `:checking`
 
-The `:checking` option defaults to `:none`. If `:checking :basic` is
-specified, certain obvious errors -- such as `IN` with an empty collection
-or `SELECT` with an empty list of columns --
-are 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 -- are also treated as an error and an exception is
-thrown. It is expected that this feature will be expanded over time
+The `:checking` option defaults to `:none`.
+If `:checking :basic` is specified, certain obvious errors
+are treated as an error and an exception is thrown.
+If `:checking :strict` is specified, certain dubious constructs are 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.
 
+Here are the checks for each level:
+* `:basic` -- `DELETE` and `DELETE FROM` without a `WHERE` clause; `IN` with an empty collection; `SELECT` with an empty list of columns; `UPDATE` without a `WHERE` clause.
+* `:strict` -- (all the `:basic` checks plus) `IN` with a collection containing `NULL` values (since this will not match rows).
+
 ## `:dialect`
 
 If `:dialect` is provided, `:quoted` will default to `true` for this call. You can still specify `:quoted false` to turn that back off.

src/honey/sql.cljc

@@ -926,6 +926,17 @@
       (into [(str/join sqls)] params))
     [s]))
 
+(defn- check-where
+  "Given a formatter function, performs a pre-flight check that there is
+  a non-empty where clause if at least basic checking is enabled."
+  [formatter]
+  (fn [k xs]
+    (when-not (= :none *checking*)
+      (when-not (seq (:where *dsl*))
+        (throw (ex-info (str (sql-kw k) " without a non-empty WHERE clause is dangerous")
+                        {:clause k :where (:where *dsl*)}))))
+    (formatter k xs)))
+
 (def ^:private base-clause-order
   "The (base) order for known clauses. Can have items added and removed.
 
@@ -985,9 +996,9 @@
          :into            #'format-select-into
          :bulk-collect-into #'format-select-into
          :insert-into     #'format-insert
-         :update          #'format-selector
-         :delete          #'format-selects
-         :delete-from     #'format-selector
+         :update          (check-where #'format-selector)
+         :delete          (check-where #'format-selects)
+         :delete-from     (check-where #'format-selector)
          :truncate        #'format-selector
          :columns         #'format-columns
          :set             #'format-set-exprs

test/honey/sql_test.cljc

@@ -796,7 +796,21 @@ ORDER BY id = ? DESC
                                   {:checking :strict})))
     (is (thrown-with-msg? ExceptionInfo #"does not match"
                           (format {:where [:in :x :?y]}
-                                  {:params {:y [nil]} :checking :strict})))))
+                                  {:params {:y [nil]} :checking :strict}))))
+  (testing "empty WHERE clauses ignored with none"
+    (is (= ["DELETE FROM foo"]
+           (format {:delete-from :foo})))
+    (is (= ["DELETE foo"]
+           (format {:delete :foo})))
+    (is (= ["UPDATE foo SET x = ?" 1]
+           (format {:update :foo :set {:x 1}}))))
+  (testing "empty WHERE clauses flagged in basic mode"
+    (is (thrown-with-msg? ExceptionInfo #"without a non-empty"
+                          (format {:delete-from :foo} {:checking :basic})))
+    (is (thrown-with-msg? ExceptionInfo #"without a non-empty"
+                          (format {:delete :foo} {:checking :basic})))
+    (is (thrown-with-msg? ExceptionInfo #"without a non-empty"
+                          (format {:update :foo :set {:x 1}} {:checking :basic})))))
 
 (deftest quoting-:%-syntax
   (testing "quoting of expressions in functions shouldn't depend on syntax"