Skip to content

Commit

Permalink
Fixing case-sensitivity in enum parsing (#660)
Browse files Browse the repository at this point in the history 
* Fixing case-sensitivity in enum parsing

* added comment

* format

* Fixed issue

* Fixed GetEnum & added more comments

* small improvement

* remove unnecessary using
  • Loading branch information
@TalZaccai
TalZaccai committed Sep 14, 2024
1 parent 253515b commit 0fb221f
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 14 deletions.
14 changes: 11 additions & 3 deletions libs/server/Resp/Parser/SessionParseState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,8 @@ public string GetString(int i)

/// <summary>
/// Get enum argument at the given index
/// Note: this method exists for compatibility with existing code.
/// For best performance use: ReadOnlySpan.EqualsUpperCaseSpanIgnoringCase(""VALUE""u8) to figure out the current enum value
/// </summary>
/// <returns>True if enum parsed successfully</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand All @@ -216,20 +218,26 @@ public T GetEnum<T>(int i, bool ignoreCase) where T : struct, Enum
Debug.Assert(i < Count);
var strRep = GetString(i);
var value = Enum.Parse<T>(strRep, ignoreCase);
return !Enum.IsDefined(typeof(T), strRep) ? default : value;
// Extra check is to avoid numerical values being successfully parsed as enum value
return string.Equals(Enum.GetName(value), strRep,
ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal) ? default : value;
}

/// <summary>
/// Try to get enum argument at the given index
/// Note: this method exists for compatibility with existing code.
/// For best performance use: ReadOnlySpan.EqualsUpperCaseSpanIgnoringCase(""VALUE""u8) to figure out the current enum value
/// </summary>
/// <returns>True if integer parsed successfully</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool TryGetEnum<T>(int i, bool ignoreCase, out T value) where T : struct, Enum
{
Debug.Assert(i < Count);
var strRep = GetString(i);
var successful = Enum.TryParse(strRep, ignoreCase, out value);
successful = successful && Enum.IsDefined(typeof(T), strRep);
var successful = Enum.TryParse(strRep, ignoreCase, out value) &&
// Extra check is to avoid numerical values being successfully parsed as enum value
string.Equals(Enum.GetName(value), strRep,
ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal);
if (!successful) value = default;
return successful;
}
Expand Down
20 changes: 9 additions & 11 deletions test/Garnet.test/RespTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1577,9 +1577,7 @@ public void KeyExpireObjectTest(string command)
}

[Test]
[TestCase("EXPIRE")]
[TestCase("PEXPIRE")]
public void KeyExpireOptionsTest(string command)
public void KeyExpireOptionsTest([Values("EXPIRE", "PEXPIRE")] string command, [Values(false, true)] bool testCaseSensitivity)
{
using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
var db = redis.GetDatabase(0);
Expand All @@ -1588,32 +1586,32 @@ public void KeyExpireOptionsTest(string command)
object[] args = [key, 1000, ""];
db.StringSet(key, key);

args[2] = "XX";// XX -- Set expiry only when the key has an existing expiry
args[2] = testCaseSensitivity ? "Xx" : "XX";// XX -- Set expiry only when the key has an existing expiry
bool resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsFalse(resp);//XX return false no existing expiry

args[2] = "NX";// NX -- Set expiry only when the key has no expiry
args[2] = testCaseSensitivity ? "nX" : "NX";// NX -- Set expiry only when the key has no expiry
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsTrue(resp);// NX return true no existing expiry

args[2] = "NX";// NX -- Set expiry only when the key has no expiry
args[2] = testCaseSensitivity ? "nx" : "NX";// NX -- Set expiry only when the key has no expiry
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsFalse(resp);// NX return false existing expiry

args[1] = 50;
args[2] = "XX";// XX -- Set expiry only when the key has an existing expiry
args[2] = testCaseSensitivity ? "xx" : "XX";// XX -- Set expiry only when the key has an existing expiry
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsTrue(resp);// XX return true existing expiry
var time = db.KeyTimeToLive(key);
ClassicAssert.IsTrue(time.Value.TotalSeconds <= (double)((int)args[1]) && time.Value.TotalSeconds > 0);

args[1] = 1;
args[2] = "GT";// GT -- Set expiry only when the new expiry is greater than current one
args[2] = testCaseSensitivity ? "Gt" : "GT";// GT -- Set expiry only when the new expiry is greater than current one
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsFalse(resp); // GT return false new expiry < current expiry

args[1] = 1000;
args[2] = "GT";// GT -- Set expiry only when the new expiry is greater than current one
args[2] = testCaseSensitivity ? "gT" : "GT";// GT -- Set expiry only when the new expiry is greater than current one
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsTrue(resp); // GT return true new expiry > current expiry
time = db.KeyTimeToLive(key);
Expand All @@ -1624,12 +1622,12 @@ public void KeyExpireOptionsTest(string command)
ClassicAssert.IsTrue(time.Value.TotalMilliseconds > 500);

args[1] = 2000;
args[2] = "LT";// LT -- Set expiry only when the new expiry is less than current one
args[2] = testCaseSensitivity ? "lt" : "LT";// LT -- Set expiry only when the new expiry is less than current one
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsFalse(resp); // LT return false new expiry > current expiry

args[1] = 15;
args[2] = "LT";// LT -- Set expiry only when the new expiry is less than current one
args[2] = testCaseSensitivity ? "lT" : "LT";// LT -- Set expiry only when the new expiry is less than current one
resp = (bool)db.Execute($"{command}", args);
ClassicAssert.IsTrue(resp); // LT return true new expiry < current expiry
time = db.KeyTimeToLive(key);
Expand Down

0 comments on commit 0fb221f

Please sign in to comment.