-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add nx/xx #206
Add nx/xx #206
Changes from all commits
47edac0
af08452
e91238c
cfb8fc0
a03939e
70af77a
04670a4
a1f4f7e
f3a8d8b
d8762b2
0454cf3
f306c4a
85e3ef9
264e909
b3dfc0c
538f4a5
76d720b
3c1ed22
45ffc86
b8e71c5
1144b6e
48e7a33
357852b
988956d
ab0330a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,6 @@ | |
using System.Threading.Tasks; | ||
using Redis.OM.Contracts; | ||
using Redis.OM.Modeling; | ||
using StackExchange.Redis; | ||
|
||
namespace Redis.OM | ||
{ | ||
|
@@ -187,6 +186,48 @@ public static async Task<bool> JsonSetAsync(this IRedisConnection connection, st | |
return result; | ||
} | ||
|
||
/// <summary> | ||
/// Sets a value as JSON in redis. | ||
/// </summary> | ||
/// <param name="connection">the connection.</param> | ||
/// <param name="key">the key for the object.</param> | ||
/// <param name="path">the path within the json to set.</param> | ||
/// <param name="json">the json.</param> | ||
/// <param name="when">XX - set if exist, NX - set if not exist.</param> | ||
/// <param name="timeSpan">the the timespan to set for your (TTL).</param> | ||
/// <returns>whether the operation succeeded.</returns> | ||
public static async Task<bool> JsonSetAsync(this IRedisConnection connection, string key, string path, string json, WhenKey when, TimeSpan? timeSpan = null) | ||
{ | ||
var argList = new List<string> { timeSpan != null ? ((long)timeSpan.Value.TotalMilliseconds).ToString() : "-1", path, json }; | ||
switch (when) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use a switch instead of an if/else? If/else is better for boolean values, as traditionally it's a single block passed to the compiler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a switch here over an if/else because the semantic isn't to see whether or not the key exists, it's whether or not the user wants to add the |
||
{ | ||
case WhenKey.Exists: | ||
argList.Add("XX"); | ||
break; | ||
case WhenKey.NotExists: | ||
argList.Add("NX"); | ||
break; | ||
} | ||
|
||
return await connection.CreateAndEvalAsync(nameof(Scripts.JsonSetWithExpire), new[] { key }, argList.ToArray()) == 1; | ||
} | ||
|
||
/// <summary> | ||
/// Sets a value as JSON in redis. | ||
/// </summary> | ||
/// <param name="connection">the connection.</param> | ||
/// <param name="key">the key for the object.</param> | ||
/// <param name="path">the path within the json to set.</param> | ||
/// <param name="obj">the object to serialize to json.</param> | ||
/// <param name="when">XX - set if exist, NX - set if not exist.</param> | ||
/// <param name="timeSpan">the the timespan to set for your (TTL).</param> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why timeSpan and not just seconds or evevn TTL? When I see timeSpan, as a user I assume I need seconds.. but maybe these are milliseconds? I think the API should be more explict. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a fair bit of precedent for .NET users for using a TimeSpan over straight up using seconds or millisecond in situations like this. TimeSpan's conveniently have a fair number of functions for simply producing these, e.g. TimeSpan.From(Millisecond/Hours/Day/Weeks etc. . .). Also when performing arithmetic on two |
||
/// <returns>whether the operation succeeded.</returns> | ||
public static async Task<bool> JsonSetAsync(this IRedisConnection connection, string key, string path, object obj, WhenKey when, TimeSpan? timeSpan = null) | ||
{ | ||
var json = JsonSerializer.Serialize(obj, Options); | ||
return await connection.JsonSetAsync(key, path, json, when, timeSpan); | ||
} | ||
|
||
/// <summary> | ||
/// Set's values in a hash. | ||
/// </summary> | ||
|
@@ -286,6 +327,48 @@ public static bool JsonSet(this IRedisConnection connection, string key, string | |
return connection.JsonSet(key, path, json, timeSpan); | ||
} | ||
|
||
/// <summary> | ||
/// Sets a value as JSON in redis. | ||
/// </summary> | ||
/// <param name="connection">the connection.</param> | ||
/// <param name="key">the key for the object.</param> | ||
/// <param name="path">the path within the json to set.</param> | ||
/// <param name="json">the json.</param> | ||
/// <param name="when">XX - set if exist, NX - set if not exist.</param> | ||
/// <param name="timeSpan">the the timespan to set for your (TTL).</param> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for both when + timespan There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
/// <returns>whether the operation succeeded.</returns> | ||
public static bool JsonSet(this IRedisConnection connection, string key, string path, string json, WhenKey when, TimeSpan? timeSpan = null) | ||
{ | ||
var argList = new List<string> { timeSpan != null ? ((long)timeSpan.Value.TotalMilliseconds).ToString() : "-1", path, json }; | ||
switch (when) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question re if/else There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
{ | ||
case WhenKey.Exists: | ||
argList.Add("XX"); | ||
break; | ||
case WhenKey.NotExists: | ||
argList.Add("NX"); | ||
break; | ||
} | ||
|
||
return connection.CreateAndEval(nameof(Scripts.JsonSetWithExpire), new[] { key }, argList.ToArray()) == 1; | ||
} | ||
|
||
/// <summary> | ||
/// Sets a value as JSON in redis. | ||
/// </summary> | ||
/// <param name="connection">the connection.</param> | ||
/// <param name="key">the key for the object.</param> | ||
/// <param name="path">the path within the json to set.</param> | ||
/// <param name="obj">the object to serialize to json.</param> | ||
/// <param name="when">XX - set if exist, NX - set if not exist.</param> | ||
/// <param name="timeSpan">the the timespan to set for your (TTL).</param> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And again |
||
/// <returns>whether the operation succeeded.</returns> | ||
public static bool JsonSet(this IRedisConnection connection, string key, string path, object obj, WhenKey when, TimeSpan? timeSpan = null) | ||
{ | ||
var json = JsonSerializer.Serialize(obj, Options); | ||
return connection.JsonSet(key, path, json, when, timeSpan); | ||
} | ||
|
||
/// <summary> | ||
/// Serializes an object to either hash or json (depending on how it's decorated), and saves it in redis. | ||
/// </summary> | ||
|
@@ -315,6 +398,108 @@ public static string Set(this IRedisConnection connection, object obj) | |
return id; | ||
} | ||
|
||
/// <summary> | ||
/// Serializes an object to either hash or json (depending on how it's decorated, and saves it to redis conditionally based on the WhenKey, | ||
/// NOTE: <see cref="WhenKey.Exists"/> will replace the object in redis if it exists. | ||
/// </summary> | ||
/// <param name="connection">The connection to redis.</param> | ||
/// <param name="obj">The object to save.</param> | ||
/// <param name="when">The condition for when to set the object.</param> | ||
/// <param name="timespan">The length of time before the key expires.</param> | ||
/// <returns>the key for the object, null if nothing was set.</returns> | ||
public static string? Set(this IRedisConnection connection, object obj, WhenKey when, TimeSpan? timespan = null) | ||
{ | ||
var id = obj.SetId(); | ||
var type = obj.GetType(); | ||
|
||
if (Attribute.GetCustomAttribute(type, typeof(DocumentAttribute)) is not DocumentAttribute attr || attr.StorageType == StorageType.Hash) | ||
{ | ||
if (when == WhenKey.Always) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can these be combined into a single branch? if when == && timespan.HasValue? Branching multiple portions in the same if statement (rather than sub) is usually more performant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it can, but you'd be adding an extra comparison: if(when == WhenKey.Always && timeSpan.HasValue)
{
// set with timespan
}
else if (when == WhenKey.Always)
{
// set without timespan
} The perf difference again would be negligible here. It's probably faster as is because in the case where |
||
{ | ||
if (timespan.HasValue) | ||
{ | ||
return connection.Set(obj, timespan.Value); | ||
} | ||
|
||
return connection.Set(obj); | ||
} | ||
|
||
var kvps = obj.BuildHashSet(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kvps == key value properties? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
var argsList = new List<string>(); | ||
int? res = null; | ||
argsList.Add(timespan != null ? ((long)timespan.Value.TotalMilliseconds).ToString() : "-1"); | ||
foreach (var kvp in kvps) | ||
{ | ||
argsList.Add(kvp.Key); | ||
argsList.Add(kvp.Value); | ||
} | ||
|
||
if (when == WhenKey.Exists) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this become a straight boolean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
{ | ||
res = connection.CreateAndEval(nameof(Scripts.ReplaceHashIfExists), new[] { id }, argsList.ToArray()); | ||
} | ||
else if (when == WhenKey.NotExists) | ||
{ | ||
res = connection.CreateAndEval(nameof(Scripts.HsetIfNotExists), new[] { id }, argsList.ToArray()); | ||
} | ||
|
||
return res == 1 ? id : null; | ||
} | ||
|
||
return connection.JsonSet(id, "$", obj, when, timespan) ? id : null; | ||
} | ||
|
||
/// <summary> | ||
/// Serializes an object to either hash or json (depending on how it's decorated, and saves it to redis conditionally based on the WhenKey, | ||
/// NOTE: <see cref="WhenKey.Exists"/> will replace the object in redis if it exists. | ||
/// </summary> | ||
/// <param name="connection">The connection to redis.</param> | ||
/// <param name="obj">The object to save.</param> | ||
/// <param name="when">The condition for when to set the object.</param> | ||
/// <param name="timespan">The length of time before the key expires.</param> | ||
/// <returns>the key for the object, null if nothing was set.</returns> | ||
public static async Task<string?> SetAsync(this IRedisConnection connection, object obj, WhenKey when, TimeSpan? timespan = null) | ||
{ | ||
var id = obj.SetId(); | ||
var type = obj.GetType(); | ||
|
||
if (Attribute.GetCustomAttribute(type, typeof(DocumentAttribute)) is not DocumentAttribute attr || attr.StorageType == StorageType.Hash) | ||
{ | ||
if (when == WhenKey.Always) | ||
{ | ||
if (timespan.HasValue) | ||
{ | ||
return await connection.SetAsync(obj, timespan.Value); | ||
} | ||
|
||
return await connection.SetAsync(obj); | ||
} | ||
|
||
var kvps = obj.BuildHashSet(); | ||
var argsList = new List<string>(); | ||
int? res = null; | ||
argsList.Add(timespan != null ? ((long)timespan.Value.TotalMilliseconds).ToString() : "-1"); | ||
foreach (var kvp in kvps) | ||
{ | ||
argsList.Add(kvp.Key); | ||
argsList.Add(kvp.Value); | ||
} | ||
|
||
if (when == WhenKey.Exists) | ||
{ | ||
res = await connection.CreateAndEvalAsync(nameof(Scripts.ReplaceHashIfExists), new[] { id }, argsList.ToArray()); | ||
} | ||
else if (when == WhenKey.NotExists) | ||
{ | ||
res = await connection.CreateAndEvalAsync(nameof(Scripts.HsetIfNotExists), new[] { id }, argsList.ToArray()); | ||
} | ||
|
||
return res == 1 ? id : null; | ||
} | ||
|
||
return await connection.JsonSetAsync(id, "$", obj, when, timespan) ? id : null; | ||
} | ||
|
||
/// <summary> | ||
/// Serializes an object to either hash or json (depending on how it's decorated), and saves it in redis. | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
namespace Redis.OM | ||
{ | ||
/// <summary> | ||
/// Indicates when this operation should be performed (only some variations are legal in a given context). | ||
/// </summary> | ||
public enum WhenKey | ||
{ | ||
/// <summary> | ||
/// The operation should occur whether or not there is an existing value. | ||
/// </summary> | ||
Always, | ||
|
||
/// <summary> | ||
/// The operation should only occur when there is an existing value. | ||
/// </summary> | ||
Exists, | ||
|
||
/// <summary> | ||
/// The operation should only occur when there is not an existing value. | ||
/// </summary> | ||
NotExists, | ||
} | ||
} |
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'm of two sides here.
On the client side, it's very clear that we have to expose based on name XX vs NX. But here.. I think it's a bad variable name. Ideally, to help users we could just name ifExists or similar. WDYT?
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.
The key here is that we are NOT in fact just exposing XX/NX, we are exposing whether we care to take any special action WRT to caring whether or not the object has existed WhenKey uses the same semantics as the When enum from StackExchange.Redis. We explicitly didn't want to expose that part of StackExchange.Redis' API, so we used a similarly named enum with the same semantics.