Validate `conn-opts` passed to wcar

vise890 commented 9 years ago

I just spent a considerable amount of time debugging my app because the conn-opts I was passing to wcar was:

{:host "123.123.123.123"}

instead of

{:spec {:host "123.123.123"}}

Now, after rereading the docstring for wcar it feels like a very silly mistake. However, I feel like I could have gotten a much better error if I got a message saying that the options I was passing in were malformed.

If I were to implement it, would you be open to merging a PR that validates conn-opts with something like schema? Any thoughts?

ptaoussanis commented 9 years ago

Hi Martino,

I'm not keen on picking up a Schema dependency, but adding some kind of manual validation would be welcome. Since :host is optional, what specifically did you have in mind?

Keep in mind that folks may intentionally also add extra unexpected kvs to their conn-opts map for their own purposes.

vise890 commented 9 years ago

Hey there,

I thought you might not want schema in, and this could indeed be as easily done without it.

This would be the two rules with the possible error message:

if :host, :port, :uri, :password, :timeout-ms or :db appear in the top level of the conn-opts map, throw ":host should go under :spec", ":port should go under :spec", etc.
- this is the mistake i made and it was particularly tricky because it was working in the dev environment (where redis was running on 127.0.0.1 and the default was being used...)
- if people really want to store other stuff in the top level, I'm sure they can come up with a more descriptive/specific name? These are only 6 "reserved" keywords after all...
under :spec, if :host and/or :port appear alongside :uri: "The :host/:port option conflicts with :uri, you can only specify one of the two"

Let me know what you think Peter.

ptaoussanis commented 9 years ago

I'm sort of on the fence about this to be honest. For one - there's a number of places where we could check for these (e.g. Pub/Sub) - and we'd need to since I think any inconsistency would be worse than just keeping the current behaviour. Doing this properly may actually involve more code and/or thought than one might expect (i.e. a potential maintenance burden).

I'm also a little uneasy about the ""The :host/:port option conflicts with :uri, you can only specify one of the two" example in general. I understand the incentives behind validating & helping where possible - but Clojure tends (in general) to fall on the side of assuming that folks know what they're doing. This is a strength IMO, though it can be frustrating those times when it bites you (and it does bite us all from time to time).

Unless something's clearly an error - I feel uneasy about throwing. Is providing both a :host and :uri an easy mistake to make? Currently the opts operate as a merge. It's conceivable to me that an advanced user - having looked at the code - would depend on the opts merge ordering. Throwing (or even warning) on the conflict case would help catch this particular mistake - but if it's not a common enough mistake, the cost of this change may outweigh the benefits (easy opts merging for advanced users).

Still mulling over this, so arguments welcome.

As a possible alternative - is there something about the wcar doc-string that could maybe be clarified/improved? I'd hoped that the examples would do a reasonable job of illustrating the necessary opts format. Do they not?

vise890 commented 9 years ago

Ok what about we just add an extra example in the README, like in 490a815 ?

ptaoussanis commented 9 years ago

Hi Martino,

Ok what about we just add an extra example in the README, like in 490a815?

I'm not sure if that's actually any clearer, is it? Just to clarify: did you find the wcar docstring unclear, or did you not realise that you should be checking the wcar docstring?

The change in 490a815 seems to be targeting only the former, yes? But if the former is the problem, couldn't we clarify the docstring itself instead?

Thanks again for your input on this.

vise890 commented 9 years ago

Well it was totally my bad, I read the README, then misread the wcar docstring and put :host and :port straight into conn-opts. I think an example right in the README would have helped me, but I guess this is just a lesson for me to learn: read the docstring more carefully.

I think we can just close this...

Thanks again for your patience Peter.

ptaoussanis commented 9 years ago

No problem, am always happy to hear ideas. Was absolutely worth double-checking, so thanks for your input. Cheers! :-)

taoensso / carmine

Validate `conn-opts` passed to wcar #124