Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation on db parameter in adapter config #9

Merged
merged 2 commits into from
Nov 3, 2016

Conversation

bberry6
Copy link
Contributor

@bberry6 bberry6 commented Jul 7, 2015

When running "sails new", the socket.io-redis adapter config/sockets.js file has the value of "sails". If the user were to use this setting by simply uncommenting that line, then the subClient.select and the pubClient.select silently fail (the errors are not being monitored).

The only valid databases that redis has are indices 0 to 15. This pull request adds validation on the db parameter to prevent the silent failure.

@sgress454
Copy link
Member

Thanks @bberry6, sorry I'm just finally getting around to this. First of all, thanks for pointing out the error in our default config file; I've patched that.

I'm hesitant to duplicate the Redis validation in our code, because it's not future-proofed. If a newer version of Redis allows database IDs > 16, or databases with string indexes, this would fail unnecessarily. But I definitely agree we should be doing something to indicate when the select command fails. At the very least we should display the error, but ideally we would wrap it in our own language since

{ [Error: ERR invalid DB index] command: 'SELECT', code: 'ERR' }

is not exactly a wealth of information (it doesn't even mention Redis).

If you'd like to take a crack at this I'd be happy to merge.

@mikermcneil
Copy link
Member

@bberry6 @sgress454 can this error code be negotiated from redis? Best way to check is prbly to sniff it and log its _.keys(). If not, we should just inline this in this hook-- but the place to do that is the configure() function -- specifically by inserting these validations between what is currently lines 64 and 65

Thanks!

@mikermcneil mikermcneil merged commit b1d09b2 into balderdashy:master Nov 3, 2016
@mikermcneil
Copy link
Member

@bberry6 to @sgress454's point, I'll follow up to open up this check so that it allows between 0 and 1000000, provided the number is an integer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants