Repository: alicebob/miniredis (go)

Issue:
Wrong behavior with SET NX GET
According to Redis specification, if we specify NX with GET in a SET command, even if the condition for NX is not met and the value is not set, we still return the old value instead of returning a `nil`:

https://redis.io/commands/set/
```
[Null reply](https://redis.io/docs/reference/protocol-spec#resp-bulk-strings): (nil) if the SET operation was not performed because the user specified the NX or XX option but the condition was not met.

If the command is issued with the GET option, the above does not apply. It will instead reply as follows, regardless if the SET was actually performed:

[Bulk string reply](https://redis.io/docs/reference/protocol-spec#resp-bulk-strings): the old string value stored at key.

[Null reply](https://redis.io/docs/reference/protocol-spec#resp-bulk-strings): (nil) if the key did not exist.
```

However, it doesn't seem like the behavior is followed [here](https://github.com/alicebob/miniredis/blob/master/cmd_string.go#L134): the function immediately returns with a `nil` value without checking whether `GET` is set. 


Pull request description:
support SET NX GET special case
That should fix #315.

> Starting with Redis version 7.0.0: Allowed the NX and GET options to be used together.

(I see how the case applies to 'SET NX GET', but I don't understand see how it changes the reply for 'SET XX GET', since that will also be nil in the "fail" case. Doesn't matter much.)

Patch:
diff --git a/cmd_string.go b/cmd_string.go
index cec9d48..b8013f2 100644
--- a/cmd_string.go
+++ b/cmd_string.go
@@ -131,16 +131,27 @@ func (m *Miniredis) cmdSet(c *server.Peer, cmd string, args []string) {
 	withTx(m, c, func(c *server.Peer, ctx *connCtx) {
 		db := m.db(ctx.selectedDB)
 
+		readonly := false
 		if opts.nx {
 			if db.exists(opts.key) {
-				c.WriteNull()
-				return
+				if opts.get {
+					// special case for SET NX GET
+					readonly = true
+				} else {
+					c.WriteNull()
+					return
+				}
 			}
 		}
 		if opts.xx {
 			if !db.exists(opts.key) {
-				c.WriteNull()
-				return
+				if opts.get {
+					// special case for SET XX GET
+					readonly = true
+				} else {
+					c.WriteNull()
+					return
+				}
 			}
 		}
 		if opts.keepttl {
@@ -154,14 +165,17 @@ func (m *Miniredis) cmdSet(c *server.Peer, cmd string, args []string) {
 				return
 			}
 		}
+
 		old, existed := db.stringKeys[opts.key]
-		db.del(opts.key, true) // be sure to remove existing values of other type keys.
-		// a vanilla SET clears the expire
-		if opts.ttl >= 0 { // EXAT/PXAT can expire right away
-			db.stringSet(opts.key, opts.value)
-		}
-		if opts.ttl != 0 {
-			db.ttl[opts.key] = opts.ttl
+		if !readonly {
+			db.del(opts.key, true) // be sure to remove existing values of other type keys.
+			// a vanilla SET clears the expire
+			if opts.ttl >= 0 { // EXAT/PXAT can expire right away
+				db.stringSet(opts.key, opts.value)
+			}
+			if opts.ttl != 0 {
+				db.ttl[opts.key] = opts.ttl
+			}
 		}
 		if opts.get {
 			if !existed {


Test patch:
diff --git a/integration/string_test.go b/integration/string_test.go
index 84b3cba..cfe27b4 100644
--- a/integration/string_test.go
+++ b/integration/string_test.go
@@ -32,6 +32,13 @@ func TestString(t *testing.T) {
 		c.Do("SET", "gone", "bar", "EXAT", "123")
 		c.Do("EXISTS", "gone")
 
+		// SET NX GET
+		c.Do("SET", "unique", "value1", "NX", "GET")
+		c.Do("SET", "unique", "value2", "NX", "GET")
+		c.Do("SET", "unique", "value3", "XX", "GET")
+		c.Do("SET", "unique", "value4", "XX", "GET")
+		c.Do("SET", "uniquer", "value5", "XX", "GET")
+
 		// Failure cases
 		c.Error("wrong number", "SET")
 		c.Error("wrong number", "SET", "foo")