Change standard validator behaviour w.r.t. nil/blank handling

[lorddoig]

With regard to the following example on this page.

(ns my.app
  (:require [validateur.validation :refer [validation-set presence-of format-of]]))

(let [v (validation-set
         (presence-of :user-name)
         (format-of :user-name
                    :format #"^[^\s]*$"
                    :message "may not contain whitespace"))]
  (v {:user-name "99 bananas"}))
;= {:user-name #{"may not contain whitespace"}}

This gives the impression that presence and format are separate validations when, in reality, the format-of test is also going to check for presence meaning both validations are pushing "can't be blank." into the errors set. This isn't noticeable in this example because of the use of sets: it only shows up once.

However, if one wishes to make this change to the above example:

    (presence-of :user-name
                 :message "Your username can't be blank.")

then this happens:

(v {:user-name ""})
; => {:user-name  #{"Your username can't be blank." "can't be blank."}}

I honestly just spent the best part of 2 hours debugging this, before I realised I was debugging the wrong functions.

I'm scrolling through the source here and it seems like pretty much every -of function has an allow-blank or allow-nil option that defaults to false and has it's own, baked in default for blank-message of "can't be blank." So if you wish to combine them with a non-default blank-message you either have to a) set allow-blank to true on all but one (which both obscures intent and raises the probability of a refactoring disaster); or b) duplicate your desired blank message across them all.

The -of methods returning function closures limits the options for solving this a bit - the only thing I can think of is making validation-set a macro that looks for presence-of calls and rewrites the other functions as necessary, but this seems uber-kludgy. Sorry for being all problems and no solutions.

[michaelklishin]

I think we should simply remove the blank value checking from other validators. Unfortunately, this is problematic for nil as other validators would trip up on nil values and validators do not terminate
function invocation chain.

[lorddoig]

I've been thinking about this for my own purposes, and aside from the macro idea, what if the -of functions returned metadata on their errors? Something like

{:blank-msgs {:user-name "can't be blank."}}

then validation-set could take the first blank message per field and filter out the rest. If validators depend on each other (e.g. for nil, as you mention) but can't coordinate with each other then it seems to me like some kind of intelligent dispatch/result aggregation is inevitable.

Or what if the non-nil-compatible validators swapped in a sane zero-value (e.g. a blank string) when faced with operating on nil? Then the checks could be removed as you suggest.

Let me know if any of this interests you and if I get anywhere I'll put a PR together.

[michaelklishin]

I'm not inclined to modify validation-set as it is really easy to reason about right now and wouldn't be if we make it depend on what individual validators may return.

The problem with non-presence validators not verifying nil values is significant, so I'm growing convinced they should reject nil values, treat blank strings as regular strings, and maybe use protocols to make the behaviour more sensible. We'll have to make the next version 3.0 then.

[michaelklishin]

To be more specific, validate-length-of should treat a nil argument as invalid but use an error message that makes sense for that validator ("should be of length …") instead of doing part of the work of presence-of.

What do you say?

[lorddoig]

I'm not sure, to be honest. Imagining a slightly contrived web use-case with a validation-set comprising, say, presence-of/length-of/format-of/exclusion-of, wouldn't this approach create the potential for showing things like the following to the user, given an empty form input:

• Username
  • can't be blank.
  • must be of length x-y.
  • must contain only letters, numbers, and periods.
  • whatever exclusion-of says

Maybe it's just my opinion but I don't think this is good for UX: "can't be blank." makes the rest superfluous. It's the kind of thing I'm used to seeing on ancient java-servlet-based webapps for multinationals/government.

This is a contrived example that can be mostly be reduced to a single format-of, but the general problem is that because the validators are black boxes spitting out a contextless set of strings then if one wishes to create the kind of UX I describe, the only options are to override every validators message-fn or filter the set of errors by comparing hard-coded strings hand-duplicated from the source files.

Then again maybe this library isn't made for me?

I don't mean to be overly critical - the above is meant purely constructively. I'm very grateful for the work you guys put in.

[michaelklishin]

The only suggestion I have is to make standard validators besides presence-of ignore nil values entirely. But that may be confusing to some, too.

[michaelklishin]

@lorddoig I perhaps haven't made this clear but I'd be happy to review a pull request that does the changes per my comment above (your ideas are welcomed, too). I have little time to dedicate to Validateur but I agree that the current behaviour is not great and we should change it and slap a 3.0.0 label on the library after that.

[lorddoig]

@michaelklishin sorry for the 3+ month delay. I'm using this library again and have a couple of ideas - I figure applying the validation functions monadically should allow us to return one error at a time for any given field, in the order the rules are defined, without reworking the existing code too much. I think the current behaviour could also be retained with this approach, and users could choose which they want.

Monads and I are not the best of mates, but if I find the time I'll give it a go.

[michaelklishin]

@lorddoig thanks! :)