Conditional WHERE clauses are hard to write and read

[p-himik]

A single conditional clause

If I have just one conditional clause, I have to write

(cond-> (cond-> {:select [:x]
                 :from   [:t]}
          filter-by-x?
          (assoc :where [:< :x 5])))

or something like

(require '[honey.sql.helpers :as hh])

(-> {:select [:x]
     :from   [:t]}
    (hh/where (when filter-by-x? [:< :x 5])))

IMO it would be better if it were possible to write

{:select [:x]
 :from   [:t]
 :where  (when filter-by-x? [:< :x 5])}

But it's not possible - format throws:

Execution error (ExceptionInfo) at honey.sql/format-dsl (sql.cljc:1254).
These SQL clauses are unknown or have nil values: :where

Multiple conditional clauses

With more than one condition, I'm practically forced to use the helper function because otherwise I'd have to figure out myself when I need [:and ...]:

(require '[honey.sql.helpers :as hh])

(cond-> {:select [:x]
         :from   [:t]}
  filter-by-x? (hh/where [:< :x 5])
  filter-by-y? (hh/where [:> :y 0]))

I think it would be better to be able to write

{:select [:x]
 :from   [:t]
 :where  [:and
          (when filter-by-x? [:< :x 5])
          (when filter-by-y? [:> :y 0])]}

But if none of the conditions is there, format throws:

Execution error (ExceptionInfo) at honey.sql/format-expr (sql.cljc:1595).
no operands found for :and

[p-himik]

The issue with multiple clauses is exacerbated when you have clauses that should follow WHERE in SQL.
Compare the naturally flowing

{:select   [[[:array_agg :x]]]
 :from     [:t]
 :where    [:and
            (when filter-by-x? [:< :x 5])
            (when filter-by-y? [:> :y 0])]
 :group-by [:y]
 :order-by [:y]}

to what I have to write currently:

(cond-> {:select   [[[:array_agg :x]]]
         :from     [:t]
         :group-by [:y]
         :order-by [:y]}
  filter-by-x? (hh/where [:< :x 5])
  filter-by-y? (hh/where [:> :y 0]))

[seancorfield]

It's done as a safety measure, so you don't accidentally select your entire table and delete or update it.

I think at one point, it used to allow it and that caught someone out badly, so now you have to be explicit.

[p-himik]

Hmm, not sure I would've done the same TBH. We don't have (infinite-range) instead of (range), after all (although I imagine would reason that we should).

But if you think it's a reasonable trade-off in the case of HoneySQL, how about having a flag that allows that?

Alternatively, now that I have slept for a bit, I realize that :where [] and :where [:and [:< :y 0]] are both valid (while :where [:and nil nil] still isn't) so a macro like this would help me:

(defmacro cond-where [& clauses]
  (assert (even? (count clauses)))
  `(let [where# (cond-> [:and]
                  ~@(mapcat (fn [[cond expr]]
                              [cond `(conj ~expr)])
                            (partition 2 clauses)))]
     (if (= (count where#) 1)
       []
       where#)))

Of course I can just copy it across my projects. But it feels that the need that this issue outlines should be a common one, so maybe the macro or something like it is worth introducing it into HoneySQL?

[p-himik]

Another 5c (keep noticing stuff only because I'm rewriting a decently sized code base from v1 to v2): dynamic :where from a result of map.
Something like:

{:select [:*]
 :from   [:t]
 :where  (into [:and]
               (map cond->sql-cond)
               conditions)}

Of course, it's easy to wrap that into in an (if (seq conditions) ... []). But forget to do it once and you'll have a run time error sooner or later.
And I see how what you mentioned is a similar "you'll have a run time issue sooner or later" problem. But in that case it's relying on one's attention+SQL knowledge whereas here it's relying on attention+SQL knowledge+HoneySQL knowledge.

[p-himik]

Oh, maybe this is what you were talking about: #413
If so, then I can see how having it as an optional check makes sense. But this issue is different as it describes a failure, not a deliberate check.

[seancorfield]

While you can argue for [:and] to collapse to true essentially, [:or] would need to collapse to false -- both of them already ignore nil arguments to make conditional DSL construction easier.

But you could do [:and true (when ...) (when ...)] and [:or false (when ...) (when ...)] and similarly (into [:and true] conditions) and (into [:or false] conditions).

I could special case :and and :or to handle that collapsing. :where [:and (when ...)] would then work for the single, optional condition case if you'd rather do that than cond-> and assoc :where ...?

[seancorfield]

I've updated develop to collapse [:and] to true and [:or] to false -- is that sufficient to address this?

[p-himik]

I would say so, yes!