handling of nil argument in merge-where about honeysql

[ddellacosta]

I think that if you want to propose a fix for this it should address the semantics of all helpers vs. just merge-where, as any one of them can presumably generate badly formed SQL; as it stands, I think the assumption is that it's on the library user to ensure their SQL is properly formed.

@jkk and other contributors, curious what you think--I'm inclined to close this one.

[MichaelBlume]

I could see just saying that all the helpers should raise if called with nil, apart from select, delete-from, etc.

[schmir]

Well, I do think this is all just an result of the current implementation (which IMHO is confusing).

Look at these:

user> (merge-where [:= :foo 1])
{:where [:= :foo 1]}
user> (merge-where [:= :foo 1] nil)
{:where [:and [:= :foo 1] nil]}
user> (merge-where {} nil nil)
{:where [:and nil nil]}
user> (merge-where {} nil)
{}
user> (merge-where)
{:where [:and]}
user> (merge-where {})
{:where [:and]}

[schmir]

I propose merge-where should require at least one argument (the map) and multiple optional arguments.
A nil map should be handled like an empty map. That would be much saner.

[ddellacosta]

My point is, you can do the same thing with every helper in the library:

user=> (require '[honeysql.helpers :as hh] '[honeysql.core :as hsql] :reload)
nil
user=> (hh/merge-where nil [:= :foo "bar"])
{:where [:and nil [:= :foo "bar"]]}
user=> (hh/where nil [:= :foo "bar"])
{:where [:and nil [:= :foo "bar"]]}
user=> (hh/join nil [:foo :f] [:= :f.a :b.a])
{:join (nil [:foo :f] [:= :f.a :b.a])}
user=> (hh/merge-join nil [:foo :f] [:= :f.a :b.a])
{:join (nil [:foo :f] [:= :f.a :b.a])}

;; ...etc

Perhaps it is confusing that merge-where works the way you are highlighting, but it's not unique. As it is now, all the helper functions assume you are feeding them valid data--garbage in, garbage out. It doesn't make sense to me to simply change merge-where's behavior without a more comprehensive discussion of how HoneySQL's helpers work and what their deficiencies are, and what an ideal semantics would be--and based on the lack of other related issues in the repo's issue tracker I would argue this is really not much of a problem for most users of the library at the moment.

[schmir]

I think part of the problem is that the documentation is rather sparse for those helpers. It's not even clear what is "valid data". I'm not even sure now if the first argument has to be a map. It looks like that is not required, but the documentation doesn't show an example of merge-where being called with anything other than a map as first argument.

[seancorfield]

Would an optional library of specs help here?

[seancorfield]

Now that I've gone through all the issues and PRs and made the first pass over the README to address some of the low-hanging fruit (adding more examples/clarifications), I'd be happy to update the documentation to make this clearer but I'm not quite sure what is needed/expected here -- the examples show the natural way to use the helper DSL and I'm not sure where they are misleading.

For example, these are equivalent:

(-> (select :*) (from :foo) (where [:= :a 1]))
(-> (from :foo) (where [:= :a 1]) (select :*))
(-> (where [:= :a 1]) (select :*) (from :foo))

(the README specifically says order of composition doesn't matter)

[seancorfield]

HoneySQL 0.9.4 improves this situation:

user=> (merge-where [:= :foo 1])
{:where [:= :foo 1]}
user=> (merge-where [:= :foo 1] nil)
{:where [:and [:= :foo 1] nil]}
user=> (merge-where {} nil nil)
{:where [:and nil nil]}
user=>  (merge-where {} nil)
{}
user=>  (merge-where)
{}
user=> (merge-where {})
{}

I'll take another look at this in 0.9.5.

[seancorfield]

In the absence of feedback, and given the improvements in the docs and the handling in 0.9.4, I'm closing this. Feel free to open new issues with more specific feedback and actionable responses.

[p-himik]

I just stumbled upon this in regular merge. Using just one nil seems to be OK but two nils break things.
So

(where {}
       (when x [:= :x x]))

works. And

(where {}
       (when x [:= :x x])
       (when y [:= :y y]))

doesn't because it produces WHERE ().
By seeing examples with a single usage of when I assumed that using multiple such statements would be fine.

[p-himik]

@seancorfield Should I create a separate issue for where and multiple nil arguments?

[seancorfield]

Please create a new issue with the repro case.