Avoids an issue where bad metadata breaks Redis.hmset

[davetron5000]

Since the metadata isn't vetted in any way, and it's being totally flattened, it's possible that blank values in the original hash result in an argument list with odd numbers of values, which causes hmset to raise an exception.

I can't explain why this is happening, but it definitely happens on our Slack. Brand new install, latest lita, the bot can't connect because someone had the value :fields => [] in the metadata coming out of Slack. Since there's no other sanity-checks, this PR seems reasonable, but I don't know the codebase to know if this is the right way to solve this.

[jimmycuadra]

This particular bug is happening in the Slack adapter: see also kenjij/lita-slack#57.

[jimmycuadra]

I think I'd rather just address the specific bug in the Slack adapter than add this code to Lita itself, but thank you very much for the PR and for bringing the issue to my attention!

[davetron5000]

It's not clear to me that it's a bug in the slack adapter (at least not from that issue, which is the same stack trace I got). I don't see any code in lita itself that prevents sending illegal data to redis. Any other adapter could provide a metadata hash that will cause this problem. This fix ensures that no adapter can produce metadata that causes the bot to crash.

[jimmycuadra]

Good points, but I'd still prefer not to accept this code. I'd rather have it continue to raise an error on bad input than try to guess what the programmer might have intended and modify the input for them.

[davetron5000]

I totally see your point, but this makes lita a very brittle system. I wonder if the issue is in using flatten? This radically transforms the input hash. Imagine { foo: [], bar: [] }. Calling to_a.flatten returned [:foo, :bar], which will be read back out as { foo: :bar }. That seems bad and super hard to detect/debug.

[jimmycuadra]

If anything, it could be worth adding to the documentation that user metadata does not support any arbitrary values: all keys and values must be scalar.

[davetron5000]

Where's the best place to put that? (I actually really love writing docs :)

And, is it worth checking and blowing up with a more directed error message? i.e. instead of letting Redis blow up for us?

[jimmycuadra]

Probably the API docs for the User.create and/or User constructor methods.

I'm honestly on the fence about whether or not Lita should check for potential errors like this in its dependencies. While a user getting an error from a Lita dependency is not a good user experience, it's very, very hard to try to predict and handle all the ways someone might use an API in an unexpected way that causes an internal error. The nature of Ruby is such that data types aren't usually enforced, neither by the language nor by the common practices of the community, and this issue (the difference between a list of scalars and a list of arbitrary types) is a prime example of something that'd be kind of crazy to attempt to paper over for people.

The one place I do think a user should never see an error other than one intentionally raised by Lita is via any of the lita CLI commands. If the error is something that can or should have been caught during the plugin development phase, it's likely out of scope for Lita itself to try to predict and guard against. I think in this specific case the bug making it to the published gem was an issue of insufficient testing. I'll continue to think about it, though.