Add nx/xx #206

shacharPash · 2022-09-20T13:07:42Z

Related to #204

…ary spaces

slorello89 · 2022-09-20T13:09:11Z

src/Redis.OM/RedisCommands.cs

+        /// <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, string when, TimeSpan? timeSpan = null)


when here should be an enum.

StackExchange already has such an Enum, I was debating whether to create a new one or use theirs, so for now I left it as a string.

yeah, we can add a different enum here, I don't want to expose any data-types from SE that we don't have too.

slorello89 · 2022-09-20T13:10:29Z

src/Redis.OM/RedisCommands.cs

+        /// <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 bool JsonSet(this IRedisConnection connection, string key, string path, object obj, string when, TimeSpan? timeSpan = null)


probably also want to have the equivalent overloads for the generic Set commands

I bypassed Set and went straight from Insert to JsonSet. I think it's better that way because HSET doesn't have a When option

slorello89 · 2022-09-20T13:11:00Z

src/Redis.OM/Searching/IRedisCollection.cs

+        /// <param name="item">an item.</param>
+        /// <param name="timeSpan">The timespan of the document's (TTL).</param>
+        /// <returns>the key. or null if the key did not exist.</returns>
+        string? InsertIfExist(T item, TimeSpan? timeSpan = null);


why the explicit overloads here vs the when condition in the generic commands?

I felt that this way is simpler to implement without breaking too many functions, but you can also use "when".

…o AddNX/XX

…sts for json, modified api for insert in colleciton

slorello89 · 2022-10-21T11:40:56Z

@chayim - at this point, I've added to much to this PR to be an objective reviewer - can you please take a look?

chayim · 2022-10-26T10:03:08Z

src/Redis.OM/RedisCommands.cs

+        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)


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.

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 XX/NX options (or their equivalent), you could use an if/else-if. The perf difference between an if/else-if and switch here is going to be negligible on a tiny set(might even be 0 depending on how the IL is compiled).

chayim · 2022-10-26T10:03:43Z

src/Redis.OM/RedisCommands.cs

+        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)


Same question re if/else

chayim · 2022-10-26T10:04:33Z

src/Redis.OM/RedisCommands.cs

+
+            if (Attribute.GetCustomAttribute(type, typeof(DocumentAttribute)) is not DocumentAttribute attr || attr.StorageType == StorageType.Hash)
+            {
+                if (when == WhenKey.Always)


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.

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 when == WhenKey.Always you would always be performing two comparisons, versus with the if/else-if where you could be performing as many as 3. And then for the cases where when != WhenKey.Always in the current implementation you'd only perform 1 comparison, whereas with the if/else-if you'd be performing 3. Again, this is a super tiny thing, IMO it's better as is.

chayim · 2022-10-26T10:05:34Z

src/Redis.OM/RedisCommands.cs

+                    return connection.Set(obj);
+                }
+
+                var kvps = obj.BuildHashSet();


kvps == key value properties?

kvps in this content is referring to KeyValuePair<TKey,TValue> The response is actually a Dictionary<TKey,TValue> (equivalent of a HashMap/Dict) - but if you see it's usage in the foreach below, it's enumerator is being accessed, meaning we're effectively using it as a collection of KeyValuePair<TKey,TValue> (that's the type of enumerator a Dictionary exposes)

chayim · 2022-10-26T10:12:13Z

src/Redis.OM/RedisCommands.cs

+        /// <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)


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?

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.

chayim · 2022-10-26T10:12:53Z

src/Redis.OM/RedisCommands.cs

+        /// <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>


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'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 DateTime or DateTimeOffset structs, the result is a timespan. So it's just easier (and what people are used to)

chayim · 2022-10-26T10:13:04Z

src/Redis.OM/RedisCommands.cs

+        /// <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>


Same for both when + timespan

chayim · 2022-10-26T10:13:14Z

src/Redis.OM/RedisCommands.cs

+        /// <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>


chayim · 2022-10-26T10:13:54Z

src/Redis.OM/RedisCommands.cs

+                    argsList.Add(kvp.Value);
+                }
+
+                if (when == WhenKey.Exists)


Can this become a straight boolean?

chayim

Logic works, some suggestions on perf and (perceived) API improvements for users.

slorello89

Thanks' @chayim - see my responses in this review.

slorello89 · 2022-10-26T10:41:01Z

src/Redis.OM/RedisCommands.cs

+        /// <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'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 DateTime or DateTimeOffset structs, the result is a timespan. So it's just easier (and what people are used to)

slorello89 · 2022-10-26T10:45:15Z

src/Redis.OM/RedisCommands.cs

+        /// <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)


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.

slorello89 · 2022-10-26T10:45:31Z

src/Redis.OM/RedisCommands.cs

+        /// <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>


slorello89 · 2022-10-26T10:46:37Z

src/Redis.OM/RedisCommands.cs

+                    return connection.Set(obj);
+                }
+
+                var kvps = obj.BuildHashSet();


kvps in this content is referring to KeyValuePair<TKey,TValue> The response is actually a Dictionary<TKey,TValue> (equivalent of a HashMap/Dict) - but if you see it's usage in the foreach below, it's enumerator is being accessed, meaning we're effectively using it as a collection of KeyValuePair<TKey,TValue> (that's the type of enumerator a Dictionary exposes)

slorello89 · 2022-10-26T10:56:17Z

src/Redis.OM/RedisCommands.cs

+        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)


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 XX/NX options (or their equivalent), you could use an if/else-if. The perf difference between an if/else-if and switch here is going to be negligible on a tiny set(might even be 0 depending on how the IL is compiled).

slorello89 · 2022-10-26T10:56:33Z

src/Redis.OM/RedisCommands.cs

+        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)


slorello89 · 2022-10-26T11:07:04Z

src/Redis.OM/RedisCommands.cs

+
+            if (Attribute.GetCustomAttribute(type, typeof(DocumentAttribute)) is not DocumentAttribute attr || attr.StorageType == StorageType.Hash)
+            {
+                if (when == WhenKey.Always)


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 when == WhenKey.Always you would always be performing two comparisons, versus with the if/else-if where you could be performing as many as 3. And then for the cases where when != WhenKey.Always in the current implementation you'd only perform 1 comparison, whereas with the if/else-if you'd be performing 3. Again, this is a super tiny thing, IMO it's better as is.

slorello89 · 2022-10-26T11:09:31Z

src/Redis.OM/RedisCommands.cs

+                    argsList.Add(kvp.Value);
+                }
+
+                if (when == WhenKey.Exists)


slorello89

LGTM 👍

stale

shacharPash and others added 13 commits June 7, 2022 18:28

Update to 2.5.61 version

47edac0

Merge branch 'redis:main' into main

af08452

Fix naming 'TestToLower' insted of 'TestTooLower' & Deleting unnecess…

e91238c

…ary spaces

Merge branch 'redis:main' into main

cfb8fc0

Merge branch 'redis:main' into main

a03939e

Merge branch 'redis:main' into main

70af77a

Merge branch 'redis:main' into main

04670a4

Merge branch 'redis:main' into main

a1f4f7e

Merge branch 'redis:main' into main

f3a8d8b

Merge branch 'redis:main' into main

d8762b2

Merge branch 'main' of github.com:shacharPash/redis-om-dotnet into main

0454cf3

Merge branch 'redis:main' into main

f306c4a

Add NX/XX features to JsonSet & InsertIfExsist/NotExist

85e3ef9

shacharPash requested a review from slorello89 September 20, 2022 13:08

Fix Test and Add Async Test

264e909

shacharPash added the feature label Sep 20, 2022

fixing tests

b3dfc0c

slorello89 requested changes Sep 21, 2022

View reviewed changes

shacharPash and others added 10 commits October 12, 2022 14:31

Change to When Type Instead of string

538f4a5

Merge branch 'AddNX/XX' of github.com:shacharPash/redis-om-dotnet int…

76d720b

…o AddNX/XX

fixing tests

3c1ed22

Create local enum When

45ffc86

Fixes

b8e71c5

Merge branch 'AddNX/XX' of github.com:shacharPash/redis-om-dotnet int…

1144b6e

…o AddNX/XX

More fixes

48e7a33

fixing tests

357852b

adding exists/notexists options for hash, added expiry+exists/not-exi…

988956d

…sts for json, modified api for insert in colleciton

moving WhenKey

ab0330a

slorello89 requested a review from chayim October 21, 2022 11:40

chayim reviewed Oct 26, 2022

View reviewed changes

chayim previously requested changes Oct 26, 2022

View reviewed changes

slorello89 reviewed Oct 26, 2022

View reviewed changes

slorello89 requested a review from chayim October 26, 2022 11:10

slorello89 linked an issue Oct 27, 2022 that may be closed by this pull request

JSON.SET with NX/XX #204

Closed

slorello89 approved these changes Dec 9, 2022

View reviewed changes

slorello89 merged commit 812d633 into redis:main Dec 9, 2022

slorello89 mentioned this pull request Feb 7, 2023

integration test timing issues #306

Merged

Add nx/xx #206

Add nx/xx #206

Conversation

shacharPash commented Sep 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorello89 commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chayim left a comment

Choose a reason for hiding this comment

slorello89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorello89 left a comment

Choose a reason for hiding this comment