ETag Follow Up: Address Open Comments (#912)

* ETag Follow Up: Address Open Comments

* fmt

* PR feedback:

* definitely an issue with pattern matching precedence

---------

Co-authored-by: Hamdaan Khalid <[email protected]>
Co-authored-by: Tal Zaccai <[email protected]>

libs/common/RespWriteUtils.cs

@@ -702,11 +702,11 @@ public static bool WriteArrayWithNullElements(int len, ref byte* curr, byte* end
         /// Writes an array consisting of an ETag followed by a Bulk string value into the buffer.
         /// NOTE: Caller should make sure there is enough space in the buffer for sending the etag, and value array.
         /// </summary>
-        /// <param name="etag"></param>
-        /// <param name="value"></param>
-        /// <param name="curr"></param>
-        /// <param name="end"></param>
-        /// <param name="writeDirect"></param>
+        /// <param name="etag">etag value to write in the array</param>
+        /// <param name="value">value to write in the array</param>
+        /// <param name="curr">start of destination buffer</param>
+        /// <param name="end">end of destincation buffer</param>
+        /// <param name="writeDirect">Whether to write the value directly to buffer or transform it to a resp bulk string</param>
         public static void WriteEtagValArray(long etag, ref ReadOnlySpan<byte> value, ref byte* curr, byte* end, bool writeDirect)
         {
             // Writes a Resp encoded Array of Integer for ETAG as first element, and bulk string for value as second element

libs/server/Resp/BasicCommands.cs

@@ -439,29 +439,6 @@ private bool NetworkSETNX<TGarnetApi>(bool highPrecision, ref TGarnetApi storage
             return NetworkSETEXNX(ref storageApi);
         }
 
-        enum EtagOption : byte
-        {
-            None,
-            WithETag,
-        }
-
-        enum ExpirationOption : byte
-        {
-            None,
-            EX,
-            PX,
-            EXAT,
-            PXAT,
-            KEEPTTL
-        }
-
-        enum ExistOptions : byte
-        {
-            None,
-            NX,
-            XX
-        }
-
         /// <summary>
         /// SET EX NX [WITHETAG]
         /// </summary>
@@ -491,59 +468,34 @@ private bool NetworkSETEXNX<TGarnetApi>(ref TGarnetApi storageApi)
                     nextOpt = parseState.GetArgSliceByRef(tokenIdx++).Span;
                 }
 
-                if (nextOpt.SequenceEqual(CmdStrings.EX))
+                // nextOpt was an expiration option
+                if (parseState.TryGetExpirationOptionWithToken(ref nextOpt, out ExpirationOption parsedOption))
                 {
-                    // Validate expiry
-                    if (!parseState.TryGetInt(tokenIdx++, out expiry))
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER;
-                        break;
-                    }
-
-                    if (expOption != ExpirationOption.None)
+                    // Make sure there aren't multiple expiration options in the options sent by user
+                    // and that whatever parsedOption we have recieved is one of the acceptable ones only
+                    if (expOption != ExpirationOption.None || (parsedOption is not (ExpirationOption.EX or ExpirationOption.PX or ExpirationOption.KEEPTTL)))
                     {
                         errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
                         break;
                     }
 
-                    expOption = ExpirationOption.EX;
-                    if (expiry <= 0)
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_SET;
-                        break;
-                    }
-                }
-                else if (nextOpt.SequenceEqual(CmdStrings.PX))
-                {
-                    // Validate expiry
-                    if (!parseState.TryGetInt(tokenIdx++, out expiry))
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER;
-                        break;
-                    }
-
-                    if (expOption != ExpirationOption.None)
+                    expOption = parsedOption;
+                    // based on above check if it is not KEEPTTL, it has to be either EX or PX
+                    if (expOption != ExpirationOption.KEEPTTL)
                     {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
-                        break;
-                    }
+                        // EX and PX optionare followed by an expiry argument; account for the expiry argument by moving past the tokenIdx
+                        if (!parseState.TryGetInt(tokenIdx++, out expiry))
+                        {
+                            errorMessage = CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER;
+                            break;
+                        }
 
-                    expOption = ExpirationOption.PX;
-                    if (expiry <= 0)
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_SET;
-                        break;
-                    }
-                }
-                else if (nextOpt.SequenceEqual(CmdStrings.KEEPTTL))
-                {
-                    if (expOption != ExpirationOption.None)
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
-                        break;
+                        if (expiry <= 0)
+                        {
+                            errorMessage = CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_SET;
+                            break;
+                        }
                     }
-
-                    expOption = ExpirationOption.KEEPTTL;
                 }
                 else if (nextOpt.SequenceEqual(CmdStrings.NX))
                 {
@@ -705,6 +657,7 @@ private bool NetworkSET_Conditional<TGarnetApi>(RespCommand cmd, int expiry, ref
 
                 bool ok = status != GarnetStatus.NOTFOUND;
 
+                // the status returned for SETEXNX as NOTFOUND is the expected status in the happy path, so flip the ok flag
                 if (cmd == RespCommand.SETEXNX)
                     ok = !ok;
 

libs/server/Resp/BasicEtagCommands.cs

@@ -3,10 +3,7 @@
 
 using System;
 using System.Diagnostics;
-using System.Text;
-using System.Threading.Tasks;
 using Garnet.common;
-using Microsoft.Extensions.Logging;
 using Tsavorite.core;
 
 namespace Garnet.server
@@ -26,7 +23,7 @@ private bool NetworkGETWITHETAG<TGarnetApi>(ref TGarnetApi storageApi)
             Debug.Assert(parseState.Count == 1);
 
             var key = parseState.GetArgSliceByRef(0).SpanByte;
-            var input = new RawStringInput(RespCommand.GETWITHETAG, ref parseState, startIdx: 1);
+            var input = new RawStringInput(RespCommand.GETWITHETAG);
             var output = new SpanByteAndMemory(dcurr, (int)(dend - dcurr));
             var status = storageApi.GET(ref key, ref input, ref output);
 
@@ -81,7 +78,7 @@ private bool NetworkGETIFNOTMATCH<TGarnetApi>(ref TGarnetApi storageApi)
         }
 
         /// <summary>
-        /// SETIFMATCH key val etag EX|PX expiry
+        /// SETIFMATCH key val etag [EX|PX] [expiry]
         /// Sets a key value pair only if the already existing etag matches the etag sent as a part of the request.
         /// </summary>
         /// <typeparam name="TGarnetApi"></typeparam>
@@ -95,79 +92,22 @@ private bool NetworkSETIFMATCH<TGarnetApi>(ref TGarnetApi storageApi)
                 return AbortWithWrongNumberOfArguments(nameof(RespCommand.SETIFMATCH));
             }
 
-            // SETIFMATCH Args: KEY VAL ETAG -> [ ((EX || PX) expiration)]
             int expiry = 0;
             ReadOnlySpan<byte> errorMessage = default;
-            var expOption = ExpirationOption.None;
-
             var tokenIdx = 3;
-            Span<byte> nextOpt = default;
-            var optUpperCased = false;
-            while (tokenIdx < parseState.Count || optUpperCased)
-            {
-                if (!optUpperCased)
-                {
-                    nextOpt = parseState.GetArgSliceByRef(tokenIdx++).Span;
-                }
 
-                if (nextOpt.SequenceEqual(CmdStrings.EX))
-                {
-                    // Validate expiry
-                    if (!parseState.TryGetInt(tokenIdx++, out expiry))
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER;
-                        break;
-                    }
-
-                    if (expOption != ExpirationOption.None)
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
-                        break;
-                    }
-
-                    expOption = ExpirationOption.EX;
-                    if (expiry <= 0)
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_SET;
-                        break;
-                    }
-                }
-                else if (nextOpt.SequenceEqual(CmdStrings.PX))
+            ExpirationOption expOption = ExpirationOption.None;
+            if (tokenIdx < parseState.Count)
+            {
+                if (!parseState.TryGetExpirationOption(tokenIdx++, out expOption) || (expOption is not ExpirationOption.EX and not ExpirationOption.PX))
+                    errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
+                else
                 {
-                    // Validate expiry
                     if (!parseState.TryGetInt(tokenIdx++, out expiry))
-                    {
                         errorMessage = CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER;
-                        break;
-                    }
-
-                    if (expOption != ExpirationOption.None)
-                    {
-                        errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
-                        break;
-                    }
-
-                    expOption = ExpirationOption.PX;
-                    if (expiry <= 0)
-                    {
+                    else if (expiry <= 0)
                         errorMessage = CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_SET;
-                        break;
-                    }
-                }
-                else
-                {
-                    if (!optUpperCased)
-                    {
-                        AsciiUtils.ToUpperInPlace(nextOpt);
-                        optUpperCased = true;
-                        continue;
-                    }
-
-                    errorMessage = CmdStrings.RESP_ERR_GENERIC_UNK_CMD;
-                    break;
                 }
-
-                optUpperCased = false;
             }
 
             if (!errorMessage.IsEmpty)

libs/server/Resp/RespEnums.cs

@@ -0,0 +1,25 @@
+namespace Garnet.server
+{
+    internal enum ExpirationOption : byte
+    {
+        None,
+        EX,
+        PX,
+        EXAT,
+        PXAT,
+        KEEPTTL
+    }
+
+    internal enum EtagOption : byte
+    {
+        None,
+        WithETag,
+    }
+
+    internal enum ExistOptions : byte
+    {
+        None,
+        NX,
+        XX
+    }
+}

libs/server/SessionParseStateExtensions.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
+using System;
 using Garnet.common;
 
 namespace Garnet.server
@@ -236,5 +237,44 @@ internal static bool TryGetSortedSetAggregateType(this SessionParseState parseSt
 
             return true;
         }
+
+        /// <summary>
+        /// Given the parseState and an index, potentially get the expiration option at that index.
+        /// </summary>
+        /// <param name="parseState">The parse state</param>
+        /// <param name="idx">The argument index</param>
+        /// <param name="value">Parsed expiration option value</param>
+        /// <returns>If the argument at that index is a valid expiration option return true, else return false</returns>
+        internal static bool TryGetExpirationOption(this SessionParseState parseState, int idx, out ExpirationOption value)
+        {
+            var sbArg = parseState.GetArgSliceByRef(idx).Span;
+            return parseState.TryGetExpirationOptionWithToken(ref sbArg, out value);
+        }
+
+        /// <summary>
+        /// Given the parse state and a token, potentially get the expiration option represented by the token.
+        /// </summary>
+        /// <param name="parseState">The parse state (used only to provide the dot notation for this method)</param>
+        /// <param name="token">The token to parse</param>
+        /// <param name="value">Parsed expiration option value</param>
+        /// <returns>If the token is a valid expiration option return true, else false</returns>
+        internal static bool TryGetExpirationOptionWithToken(this SessionParseState parseState, ref Span<byte> token, out ExpirationOption value)
+        {
+            value = default;
+            if (token.EqualsUpperCaseSpanIgnoringCase(CmdStrings.EX))
+                value = ExpirationOption.EX;
+            else if (token.EqualsUpperCaseSpanIgnoringCase(CmdStrings.PX))
+                value = ExpirationOption.PX;
+            else if (token.EqualsUpperCaseSpanIgnoringCase(CmdStrings.EXAT))
+                value = ExpirationOption.EXAT;
+            else if (token.EqualsUpperCaseSpanIgnoringCase(CmdStrings.PXAT))
+                value = ExpirationOption.PXAT;
+            else if (token.EqualsUpperCaseSpanIgnoringCase(CmdStrings.KEEPTTL))
+                value = ExpirationOption.KEEPTTL;
+            else
+                return false;
+
+            return true;
+        }
     }
 }

libs/server/Storage/Functions/EtagState.cs

@@ -5,6 +5,13 @@
 
 namespace Garnet.server
 {
+    internal static class EtagConstants
+    {
+        public const byte EtagSize = sizeof(long);
+
+        public const long BaseEtag = 0;
+    }
+
     /// <summary>
     /// Indirection wrapper to provide a way to set offsets related to Etags and use the getters opaquely from outside.
     /// </summary>
@@ -32,15 +39,15 @@ public EtagState()
         /// <summary>
         /// Field provides access to getting an Etag from a record, hiding whether it is actually present or not.
         /// </summary>
-        public long etag { get; private set; } = Constants.BaseEtag;
+        public long etag { get; private set; } = EtagConstants.BaseEtag;
 
         /// <summary>
         /// Sets the values to indicate the presence of an Etag as a part of the payload value
         /// </summary>
         public static void SetValsForRecordWithEtag(ref EtagState curr, ref SpanByte value)
         {
-            curr.etagOffsetForVarlen = Constants.EtagSize;
-            curr.etagSkippedStart = Constants.EtagSize;
+            curr.etagOffsetForVarlen = EtagConstants.EtagSize;
+            curr.etagSkippedStart = EtagConstants.EtagSize;
             curr.etagAccountedLength = value.LengthWithoutMetadata;
             curr.etag = value.GetEtagInPayload();
         }
@@ -49,7 +56,7 @@ public static void ResetState(ref EtagState curr)
         {
             curr.etagOffsetForVarlen = 0;
             curr.etagSkippedStart = 0;
-            curr.etag = Constants.BaseEtag;
+            curr.etag = EtagConstants.BaseEtag;
             curr.etagAccountedLength = -1;
         }
     }