-
Notifications
You must be signed in to change notification settings - Fork 228
Simple support for DUMP
and RESTORE
#405
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
base: master
Are you sure you want to change the base?
Conversation
7c687cb
to
24e0b1d
Compare
On Sat, May 17, 2025 at 03:53:02PM -0700, Alyssa wrote:
Closes #401
Simple implementation for `DUMP` and `RESTORE` which just supports string keys:
Thanks! I'll have a look soon.
|
(not forgotten, just busy) |
@@ -8,13 +8,7 @@ import ( | |||
|
|||
// Command 'INFO' from https://redis.io/commands/info/ | |||
func (m *Miniredis) cmdInfo(c *server.Peer, cmd string, args []string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one and cmdSelect don't use handleAuth, but with isValidCMD they now do. That looks like it shouldn't change. Otherwise it looks like a nice change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these were using isValidCMD
before my change, I just moved the args validation into it (no auth behaviour has changed here).
In any case, I've tested locally with a dockerised redis and these two commands do produce auth errors, so I think the current code is correct:
$ docker run -d --name redis -p 6379:6379 redis:6.2.7 --requirepass "SUPER_SECRET_PASSWORD"
$ docker exec -it redis redis-cli
127.0.0.1:6379> info
NOAUTH Authentication required.
127.0.0.1:6379> select 0
(error) NOAUTH Authentication required.
127.0.0.1:6379>
Looks good! I merged the first commit to master, since that was an easy one.
I don't think we can do that, it changes existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can do that, it changes existing code.
Do you mean because it would be a breaking change? The thing that prompted me to change it was that otherwise the existing unit tests for this debug feature clashed with the TestDump
that I wanted to write (for the actual redis feature). I could just rename the test suite but thought renaming it to something else was a good change anyway - to avoid anyone confusing it with the Redis command. Happy to revert that though and just rename the tests if you feel strongly!
@@ -8,13 +8,7 @@ import ( | |||
|
|||
// Command 'INFO' from https://redis.io/commands/info/ | |||
func (m *Miniredis) cmdInfo(c *server.Peer, cmd string, args []string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these were using isValidCMD
before my change, I just moved the args validation into it (no auth behaviour has changed here).
In any case, I've tested locally with a dockerised redis and these two commands do produce auth errors, so I think the current code is correct:
$ docker run -d --name redis -p 6379:6379 redis:6.2.7 --requirepass "SUPER_SECRET_PASSWORD"
$ docker exec -it redis redis-cli
127.0.0.1:6379> info
NOAUTH Authentication required.
127.0.0.1:6379> select 0
(error) NOAUTH Authentication required.
127.0.0.1:6379>
thanks for your replies! I'll get back to you. |
Closes #401
Simple implementation for
DUMP
andRESTORE
which just supports string keys:WRONGTYPE Operation against a key holding the wrong kind of value
REPLACE
andABSTTL
optional flags, but notIDLETIME
orFREQ
(wasn't immediately obvious to me what these were 🤷♀️ )I have also:
Dump
toDebugDump
to avoid confusionvalidCMD
and plugged it in for all the existing generic commands. Happy to do this for the remainder as well if the approach looks good to you 😄