theleoborges / bouncer

A validation DSL for Clojure & Clojurescript applications
364 stars 38 forks source link

Multi-field validation #21

Closed theleoborges closed 10 years ago

theleoborges commented 10 years ago

Currently if I want to validate that :from is less than :to in a map I have to write a custom validator on the following lines:

(def subject {:from 20 :to 10})

(defn less-than [max]
  (fn [value]
    (< value max)))

(b/validate subject
            :from [[v/required (less-than (:to subject))]]
            :to   [[v/required]])

I hadn't implemented this feature so far as I didn't come up with an API I was happy with. After some thought I have some ideas that would allow the previous example to be written as:

(b/validate subject
            :from [[v/required]]
            :to   [[v/required]]
            [:from :to] [[< :message "From should be less than to"]])

This would be flexible enough to allow for other usages. For example, password confirmation could be validated as such:

(b/validate subject
            :password     [[v/required]]
            :confirmation [[v/required :pre (comp not nil? :password)]]
            [:password :confirmation] [[= :message "Passwords should match."]])

Validator functions used in multi-field validations need an arity equal to the length of keywords in the vector preceding the validators.

How does this API look to you?

mogverse commented 10 years ago

For the validate function something like [:password :confirmation] would mean a nested map having a confirmation as a key inside password.

I don't think password confirmation will be a good idea.

Is it possible for the DSL to let the user decide how the returned error keys should be named in case we don't want the same name as the key itself something ?

For example: In a web form, if the error key is that of a field I would show the error below that field using the key of the error.

but if the error is generic and not specific to one key, I would show such error somewhere else. probably at a common place other than below each fields.

How about having a macro called "as-key" to which we pass desired key and list of arguments ? what say ?

 (b/validate subject
           (as-key :password :password_field)     [[v/required]]
           :confirmation [[v/required :pre (comp not nil? :password)]]
           (as-key :pass_confirm :password :confirmation) [[= :message "Passwords should match."]])
theleoborges commented 10 years ago

Hi @amogh-talpallikar ,

Those are all valid points and I still don't have an answer to them. I've posted this mostly to get some initial feedback and ideas. Your comments and suggestions are appreciated.

The proposed change would require an update on how nested validations work, as you point out, but would keep backwards compatibility with existing validators.

Another option is to have each validator take an optional last argument which is the map being validated. That would make writing such a validator trivial:

(defn less-than [k value subject]
  (< value (k subject)))

(b/validate subject
            :from [[(partial less-than :to)]]
            :to   [[v/required]])

But in this case I'm still not convinced about the syntax. Will need to give it some more thought.

theleoborges commented 10 years ago

In fact I just played with this implementation and it works:

(defn less-than [k]
  (fn [value subject]
    (< value (k subject))))

(core/valid? {:from 20 :to 10}
             :from [v/required (less-than :to)])

;; false

(defn same-as [k]
  (fn [value subject] 
    (= value (k subject))))

(core/valid? {:password 123 :confirmation 123}
             :password [v/required (same-as :confirmation)])

;; true

The implementation itself is terribly simple. The drawback is that it breaks compatibility with existing validators - validator functions would receive a minimum of 2 arguments: the value in the key provided and the original map being validated, called the subject.

I don't have a problem with backwards compatibility in future releases if this is a feature important enough to have.

Let me know your thoughts.

mlapshin commented 10 years ago

Hi Leonardo. Our team is using bouncer to perform validations in simple web app we're working on. Now I want to implement a validation function which checks uniqueness of a value inside DB (email, login, etc). At first it looks simple, but I faced a problem: I don't have a whole map in validation fn, just the value on which validation is performed. But I need a whole map to perform query like this:

(db/q {:select "COUNT(*)"
       :from "users"
       :where (format "login = '%s' AND id <> %d" value (:id subject))

Could you push a experimental branch with two-arguments validation functions to GitHub? We are ready to test this branch inside our project.

Mike.

mogverse commented 10 years ago

Hey Mike, You can write a validator that accepts multiple arguments, first one being you value and second one being the whole map. Something like, (defvalidator check-uniqueness-db [map-key db-key whole-map] ;;return true if the its unique else return false )

and while using, (b/validate whole-map :your-key [[check-uniqueness-db :db-key whole-map :message (format "The %s already exists" "your-key" "" )]])

theleoborges commented 10 years ago

Hi @mlapshin ,

As @amogh-talpallikar pointed out, there is a workaround.

Nevertheless, I have pushed an experimental version to a new branch on github here. All validator functions receive one extra argument at the end, the original map.

All tests are passing though there are a couple missing. Also, if you're using any clojure.core functions as validators - such as nil? - you'll need to wrap them as they now need an extra argument. I've included a utility function for this, called bouncify for lack of better naming.

It looks like this:

(require '[bouncer
             [core :as core]
             [validators :as v]
             [utils :refer [bouncify]]])
(core/validate {:age -1 :year ""}
                    :name v/required
                    :year v/number
                    :age v/positive
                    :dob (bouncify (complement nil?)))

It would be a big help if you could give this branch a go and let me know what you think about the API.

Thanks

theleoborges commented 10 years ago

Hi @mlapshin ,

I'm wondering if you had a chance to try the new branch out?

I'm more and more inclined and making bouncer go in that direction for the next release but some feedback would be great before I make the decision.

Also if we do go in that direction, bouncify will be renamed to lift as it has more to do with what is actually happening.

theleoborges commented 10 years ago

Hi guys, I'm closing this issue as I haven't heard anything back yet.

Feel free to re-open in case this is still an issue.

Thanks

limist commented 9 years ago

Bumped into this issue recently; used the workaround solution above (thanks @amogh-talpallikar ), which works. If there are no plans to rollout the new API, some documentation about mult-field validation would be really helpful

OliverM commented 8 years ago

I've bumped into this too. The workaround above won't work if you've been defining common sets of validators to include in larger validation suites, as they're defined using (def {field name validation-set}), with no access to the map being validated. This means I've to copy the same validator to different locations in my project.

I've not tried the multi-field validation branch as I'm not sure what stage it's at vs. the main branch.