korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.48k stars 222 forks source link

Let prepares handle also where clauses #351

Closed gfZeng closed 8 years ago

immoh commented 8 years ago

Can you provide a failing test that this change fixes, thanks.

gfZeng commented 8 years ago

Having think that case:

I'm using Postgresql which support Enum type.

(defn sql-cast
  ":admin => 'admin'::role"
  [type val]
  (raw (format "'%s'::%s" (name val) (name type))))

;; user's role field is an enum type
(defentity user
  (prepare #(update % :role (partial sql-cast :role))))

;; insertion is ok, prepares handler this
(insert user
  (values {:role :admin
           :name "Chris"}))

;; select by role will failed
(select user
  (where {:role :admin})) 
immoh commented 8 years ago

First of all, every PR should include tests. We simply cannot add new functionality without adding tests, otherwise regression bugs are impossible to catch.

I quickly tested this in the REPL and it looks it is not this simple. The values in the map can be conditions (e.g. (where {:role [not= :admin]})) or the value can be a list instead of a map (e.g. (where (= :role :admin))) and these are not covered. Most likely there are other cases as well and I feel that this feature is probably not worth the complexity that the implementation introduces.

gfZeng commented 8 years ago

Yeah, you are right. My patch can just handler some simple case. IMO the best way is let korma support customize type cast for field.