Skip to content
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.

Sign up for GitHub

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

Pass in JsonSerializerOptions to generic methods #187

Merged
merged 8 commits into from
Oct 17, 2023
Merged

Pass in JsonSerializerOptions to generic methods #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8f77bcb
Pass in JsonSerializerOptions to generic methods
MichaelDeutschCoding Sep 14, 2023
4895597
move the '?' to the right place
shacharPash Sep 14, 2023
1b29670
Add unit tests for JsonSerializerOptions
MichaelDeutschCoding Sep 14, 2023
6db97f9
add missing json path
MichaelDeutschCoding Sep 14, 2023
4a80e25
Merge branch 'master' into json-serializer-opts
shacharPash Sep 27, 2023
a9f10d7
Merge branch 'master' into json-serializer-opts
shacharPash Sep 27, 2023
252c8c5
Merge branch 'master' into json-serializer-opts
shacharPash Sep 28, 2023
26e3bb6
Merge branch 'master' into json-serializer-opts
shacharPash Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/NRedisStack/Json/IJsonCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using NRedisStack.Json.DataTypes;
using StackExchange.Redis;
using System.Text.Json;

namespace NRedisStack;

Expand Down Expand Up @@ -103,7 +104,7 @@ public interface IJsonCommands
/// <param name="newLine">sets the string that's printed at the end of each line</param>
/// <param name="space">sets the string that's put between a key and a value</param>
/// <param name="path">the path to get.</param>
/// <returns>The requested Items</returns>
/// <returns>The requested items</returns>
/// <remarks><seealso href="https://redis.io/commands/json.get"/></remarks>
RedisResult Get(RedisKey key, RedisValue? indent = null, RedisValue? newLine = null, RedisValue? space = null, RedisValue? path = null);

Expand All @@ -119,17 +120,18 @@ public interface IJsonCommands
RedisResult Get(RedisKey key, string[] paths, RedisValue? indent = null, RedisValue? newLine = null, RedisValue? space = null);

/// <summary>
/// Generically gets an Item stored in Redis.
/// Generically gets an item stored in Redis.
/// </summary>
/// <param name="key">The key to retrieve</param>
/// <param name="path">The path to retrieve</param>
/// <param name="serializerOptions">Json serializer options to use for deserialization.</param>
/// <typeparam name="T">The type retrieved</typeparam>
/// <returns>The object requested</returns>
/// <remarks><seealso href="https://redis.io/commands/json.get"/></remarks>
T? Get<T>(RedisKey key, string path = "$");
T? Get<T>(RedisKey key, string path = "$", JsonSerializerOptions? serializerOptions = default);

/// <summary>
/// retrieves a group of items stored in redis, appropriate if the path will resolve to multiple records.
/// Retrieves a group of items stored in Redis, appropriate if the path will resolve to multiple records.
/// </summary>
/// <param name="key">The key to pull from.</param>
/// <param name="path">The path to pull.</param>
Expand Down
4 changes: 3 additions & 1 deletion src/NRedisStack/Json/IJsonCommandsAsync.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using NRedisStack.Json.DataTypes;
using StackExchange.Redis;
using System.Text.Json;

namespace NRedisStack;

Expand Down Expand Up @@ -123,10 +124,11 @@ public interface IJsonCommandsAsync
/// </summary>
/// <param name="key">The key to retrieve</param>
/// <param name="path">The path to retrieve</param>
/// <param name="serializerOptions">Json serializer options to use for deserialization.</param>
/// <typeparam name="T">The type retrieved</typeparam>
/// <returns>The object requested</returns>
/// <remarks><seealso href="https://redis.io/commands/json.get"/></remarks>
Task<T?> GetAsync<T>(RedisKey key, string path = "$");
Task<T?> GetAsync<T>(RedisKey key, string path = "$", JsonSerializerOptions? serializerOptions = default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically these API changes are binary breaks, if you overlay the new DLLs you'll get missing member exceptions - as the calling applications will need to recompile. NRedisStack is still pre-1.0 so avoiding breaks is still "best effort" I'd say so there's a few options:

  1. Add the new signature as an extra overload rather than as an optional parameter
  2. Rev the minor version to 0.10.0
  3. YOLO - just merge - probably won't break anyone

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelDeutschCoding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have the context to know which of those options makes the most sense; this is my first contribution to the code base. I'm happy to implement any of those approaches, if you can just give me the direction you'd like to take.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slorello89
To create another signature where serializerOptions is not optional, we will have to do it like this:

T? Get<T>(RedisKey key, JsonSerializerOptions serializerOptions, string path = "$");

What changes the order of the original function (path after key), I think it's better to leave serializerOptions optional.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would just reset this line, and add a new method with:

Task<T?> GetAsync<T>(RedisKey key, string path = "$", JsonSerializerOptions serializerOptions);

as the signature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slorello89 I didn't understand how this is possible, an optional variable should come at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how bad it would be breaking binary/reflection users by changing the method signature, but personally it would feel weird to me to add an additional separate method with a mandatory JsonSerializerOptions, when that parameter is itself optional in the JsonSerializer method we're calling.

If the version number is bumped, does that provide enough "warning" if it does indeed break anyone's use flow?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I didn't understand why it breaks.
If the variable we added is optional, anyone who used the function without JsonSerializerOptions can continue to use it in exactly the same way because the variable is optional.
@slorello89 what do you say?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shacharPash - @MichaelDeutschCoding is correct - adding optional parameters are compiler-compatible but not binary-compatible. You would need to recompile your application if you use the method in question. See my comments above, as this is still not 1.0+ I think all three options are valid (reset the signature, bump minor, or YOLO), however, if we were post 1.0 I'd insist on option 1 as this is a very minor change to make a major revision for and you really shouldn't propagate binary breaks in minors/patches.


/// <summary>
/// retrieves a group of items stored in redis, appropriate if the path will resolve to multiple records.
Expand Down
4 changes: 2 additions & 2 deletions src/NRedisStack/Json/JsonCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,15 @@ public RedisResult Get(RedisKey key, string[] paths, RedisValue? indent = null,
}

/// <inheritdoc/>
public T? Get<T>(RedisKey key, string path = "$")
public T? Get<T>(RedisKey key, string path = "$", JsonSerializerOptions? serializerOptions = default)
{
var res = _db.Execute(JsonCommandBuilder.Get<T>(key, path));
if (res.Type == ResultType.BulkString)
{
var arr = JsonSerializer.Deserialize<JsonArray>(res.ToString()!);
if (arr?.Count > 0)
{
return JsonSerializer.Deserialize<T>(JsonSerializer.Serialize(arr[0]));
return JsonSerializer.Deserialize<T>(JsonSerializer.Serialize(arr[0]), serializerOptions);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/NRedisStack/Json/JsonCommandsAsync.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ public async Task<RedisResult> GetAsync(RedisKey key, string[] paths, RedisValue
return await _db.ExecuteAsync(JsonCommandBuilder.Get(key, paths, indent, newLine, space));
}

public async Task<T?> GetAsync<T>(RedisKey key, string path = "$")
public async Task<T?> GetAsync<T>(RedisKey key, string path = "$", JsonSerializerOptions? serializerOptions = default)
{
var res = await _db.ExecuteAsync(JsonCommandBuilder.Get<T>(key, path));
if (res.Type == ResultType.BulkString)
{
var arr = JsonSerializer.Deserialize<JsonArray>(res.ToString()!);
if (arr?.Count > 0)
{
return JsonSerializer.Deserialize<T>(JsonSerializer.Serialize(arr[0]));
return JsonSerializer.Deserialize<T>(JsonSerializer.Serialize(arr[0]), serializerOptions);
}
}

Expand Down
26 changes: 20 additions & 6 deletions tests/NRedisStack.Tests/Json/JsonTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,14 +681,21 @@ public async Task ForgetAsync()
public void Get()
{
var commands = new JsonCommands(redisFixture.Redis.GetDatabase());
var keys = CreateKeyNames(2);
var keys = CreateKeyNames(3);
var key = keys[0];
var complexKey = keys[1];
var caseInsensitiveKey = keys[2];
commands.Set(key, "$", new Person() { Age = 35, Name = "Alice" });
commands.Set(complexKey, "$", new { a = new Person() { Age = 35, Name = "Alice" }, b = new { a = new Person() { Age = 35, Name = "Alice" } } });
commands.Set(caseInsensitiveKey, "$", new { name = "Alice", AGE = 35 });
var result = commands.Get<Person>(key);
Assert.Equal("Alice", result!.Name);
Assert.Equal(35, result.Age);
var jsonOptions = new JsonSerializerOptions { PropertyNameCaseInsensitive = true };
result = commands.Get<Person>(caseInsensitiveKey, "$", jsonOptions);
Assert.NotNull(result);
Assert.Equal("Alice", result!.Name);
Assert.Equal(35, result.Age);
var people = commands.GetEnumerable<Person>(complexKey, "$..a").ToArray();
Assert.Equal(2, people.Length);
Assert.Equal("Alice", people[0]!.Name);
Expand All @@ -701,14 +708,21 @@ public void Get()
public async Task GetAsync()
{
var commands = new JsonCommandsAsync(redisFixture.Redis.GetDatabase());
var keys = CreateKeyNames(2);
var keys = CreateKeyNames(3);
var key = keys[0];
var complexKey = keys[1];
var caseInsensitiveKey = keys[2];
await commands.SetAsync(key, "$", new Person() { Age = 35, Name = "Alice" });
await commands.SetAsync(complexKey, "$", new { a = new Person() { Age = 35, Name = "Alice" }, b = new { a = new Person() { Age = 35, Name = "Alice" } } });
await commands.SetAsync(caseInsensitiveKey, "$", new { name = "Alice", AGE = 35 });
var result = await commands.GetAsync<Person>(key);
Assert.Equal("Alice", result!.Name);
Assert.Equal(35, result.Age);
var jsonOptions = new JsonSerializerOptions { PropertyNameCaseInsensitive = true };
result = await commands.GetAsync<Person>(caseInsensitiveKey, "$", jsonOptions);
Assert.NotNull(result);
Assert.Equal("Alice", result!.Name);
Assert.Equal(35, result.Age);
var people = (await commands.GetEnumerableAsync<Person>(complexKey, "$..a")).ToArray();
Assert.Equal(2, people.Length);
Assert.Equal("Alice", people[0]!.Name);
Expand All @@ -730,8 +744,8 @@ public void MSet()
new KeyPathValue(key1, "$", new { a = "hello" }),
new KeyPathValue(key2, "$", new { a = "world" })
};
commands.MSet(values)
;
commands.MSet(values);

var result = commands.MGet(keys.Select(x => new RedisKey(x)).ToArray(), "$.a");

Assert.Equal("[\"hello\"]", result[0].ToString());
Expand All @@ -753,8 +767,8 @@ public async Task MSetAsync()
new KeyPathValue(key1, "$", new { a = "hello" }),
new KeyPathValue(key2, "$", new { a = "world" })
};
await commands.MSetAsync(values)
;
await commands.MSetAsync(values);

var result = await commands.MGetAsync(keys.Select(x => new RedisKey(x)).ToArray(), "$.a");

Assert.Equal("[\"hello\"]", result[0].ToString());
Expand Down