Simple support for DUMP and RESTORE

[alyssaruth]

Closes #401

Simple implementation for DUMP and RESTORE which just supports string keys:

• Attempting to dump a different key type will produce WRONGTYPE Operation against a key holding the wrong kind of value
• For RESTORE I've implemented the REPLACE and ABSTTL optional flags, but not IDLETIME or FREQ (wasn't immediately obvious to me what these were 🤷‍♀️ )

I have also:

• Renamed the existing Dump to DebugDump to avoid confusion
• Extracted constants for the various key types
• Collected common validation into validCMD 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 😄

[alicebob]

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.

[alicebob]

(not forgotten, just busy)

[alicebob]

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.

[alyssaruth]

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> 

[alicebob]

Looks good! I merged the first commit to master, since that was an easy one.

Renamed the existing Dump to DebugDump to avoid confusion

I don't think we can do that, it changes existing code.

[alyssaruth]

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!

[alyssaruth]

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> 

[alicebob]

thanks for your replies! I'll get back to you.

[alicebob]

sorry for the delay. PR looks great and I'll merge it, but I do prefer to keep .Dump() named as is. I know it's an unlucky name, but I don't want to introduce a breaking change.

[alyssaruth]

sorry for the delay. PR looks great and I'll merge it, but I do prefer to keep .Dump() named as is. I know it's an unlucky name, but I don't want to introduce a breaking change.

Ok! I've reverted that part of the change 👍

[alicebob]

Thanks! And sorry again for the slow reviewing.