redis / redis-om-dotnet

Object mapping, and more, for Redis and .NET
MIT License
457 stars 76 forks source link

Inconsistent results with Redis OM .NET vs CLI query / Node OM #288

Closed paulers closed 1 year ago

paulers commented 1 year ago

Finding inconsistencies between the .NET OM package, Node OM package and regular Redis CLI / Redis Insight. The following query: FT.SEARCH products "@VendorId:{6b11fdc8\\-d5f1\\-477a\\-997f\\-ebc2c1460fdb}" LIMIT 0 10 SORTBY Name DESC fails to execute in RedisOM .NET, but succeeds in RedisOM Node and in the CLI.

Here's the specific code which throws an exception:

var query = new RedisQuery("products")
{
    SortBy = new RedisSortBy
    {
        Direction = sortDirection == "ASC" ? SortDirection.Ascending : SortDirection.Descending,
        Field = sortField
    },
    Limit = new SearchLimit
    {
        Offset = skip,
        Number = take
    }
};

string queryText = "\"@VendorId:{" + vendorId.ToString().Replace("-", "\\\\-") + "}\"";
if (vendorId.HasValue) query.QueryText = queryText;
if (fields?. Length > 0) query.Return = new ReturnFields(fields.ToList());

var result = await _redis.Connection.SearchAsync<Terminal>(query);

The error I'm getting is this: Syntax error at offset 1 near VendorId Failed on FT.SEARCH terminals "@VendorId:{6b11fdc8\\-d5f1\\-477a\\-997f\\-ebc2c1460fdb}" LIMIT 0 10 SORTBY Name DESC

The index is built with VendorId as a tag:

[Indexed][Required] public Guid VendorId { get; set; }

Required tag is from System.ComponentModel.DataAnnotations, but that's not relevant.

When I copy the above query and toss it into RedisInsight's Workbench, it works just fine. Also Node's OM package runs the query successfully.

Using Redis.OM 0.4.1. RediSearch 20603 in the cloud.

paulers commented 1 year ago

Turns out reducing the amount of backslashes in C# worked to fix above issue ("@VendorId:{" + vendorId.ToString().Replace("-", "\\-") + "}";), but now I ran into a new problem with the query.Return property. It's throwing the following error: The given key '$' was not present in the dictionary. This happens only when I pass in some fields which I know are indexed. If I pass in non-indexed fields, it just returns an empty array. Here's the same query in RedisInisight which works correctly: FT.SEARCH terminals "@VendorId:{6b11fdc8\\-d5f1\\-477a\\-997f\\-ebc2c1460fdb}" RETURN 2 Name ProductCode LIMIT 0 10 SORTBY Name ASC

Name is TEXT and ProductCode is TAG in the index.

paulers commented 1 year ago

I cloned this repository and added it as a project to my solution just so I could debug what is going on. Here is some more information:

The exception is thrown exactly at this line: https://github.com/redis/redis-om-dotnet/blob/main/src/Redis.OM/RedisObjectHandler.cs#L60

The hash dictionary that comes in has only the names of the fields as keys, it does not have a '$', so it throws an exception. Passing in a request where ReturnFields takes in a string[] like ["$", "Name", "VendorId"] succeeds but negates the concept of returning just a few fields (all fields are returned).

I believe this method needs to be augmented to take into account the fact only a select few fields may be returned from the Redis query.

slorello89 commented 1 year ago

@paulers - the main inconsistency from the CLI vs Redis OM .NET WRT to basckslashes is that the CLI will try to escape the backslash passed to it unless the backslash itself is escaped (hence why you would need to add to \ to FT.SEARCH products "@VendorId:{6b11fdc8\\-d5f1\\-477a\\-997f\\-ebc2c1460fdb}" LIMIT 0 10 SORTBY Name DESC. Any client which wouldn't explicitly escape the first backslash do not need to send it, in fact if they do pass in the second backslash, it will negate the first backslash causing a syntax error.

To your second question, the way you are using Search<T> with a return is slightly off. When you pass in Return it changes the way the output is formatted. If you pass in the full type as T, and the type is JSON, the output from that query is:

127.0.0.1:6379> FT.SEARCH product-idx *
1) (integer) 2
2) "Program+Product:01GPZVSRG9G8X3ZB0YZJQ8PM03"
3) 1) "$"
   2) "{\"Tags\":[\"phone\"],\"Id\":\"01GPZVSRG9G8X3ZB0YZJQ8PM03\"}"
4) "Program+Product:01GPZVTDQ9FS3G427ZVF9FXSZ3"
5) 1) "$"
   2) "{\"Tags\":[\"phone\"],\"Id\":\"01GPZVTDQ9FS3G427ZVF9FXSZ3\"}"

Notice how the $ becomes the only key in the resultant array for each record.

However, if you pass in a RETURN argument, it changes the output so that it is more like a Hash's search result, with the return fields as keys.

127.0.0.1:6379> FT.SEARCH product-idx * RETURN 1 Id
1) (integer) 2
2) "Program+Product:01GPZVSRG9G8X3ZB0YZJQ8PM03"
3) 1) "Id"
   2) "01GPZVSRG9G8X3ZB0YZJQ8PM03"
4) "Program+Product:01GPZVTDQ9FS3G427ZVF9FXSZ3"
5) 1) "Id"
   2) "01GPZVTDQ9FS3G427ZVF9FXSZ3"

From the users perspective, this means that you'd need a separate type to store this result.

Now if this seems a bit wonky - well, it is - but that's primarily because this raw query interface is considered a somewhat advanced usage - it's not tested as heavily because the intention is that you'd use the RedisCollection to perform advanced queries like this. E.g.

products.Select(x=>x.Id);

Would create a RETURN with Id, but it would also importantly pass down the anonymous type that you actually want to store it in down to the deserialization method.

paulers commented 1 year ago

Thanks for the explanation. I did see the difference in response while debugging. I'd like to use the Select statement to build the query, but I'm letting users define which fields they want to see. That Select needs to be built dynamically, hence the Search<T> being brought in instead. Could you recommend a way to accomplish the aforementioned scenario (user passes in parameters for sort, sort direction, fields they want to see, and skip/take) using RedisOM? Search<T> and SearchAsync<T> require a type to be passed in.

slorello89 commented 1 year ago

@paulers if you change: https://github.com/redis/redis-om-dotnet/blob/main/src/Redis.OM/RedisObjectHandler.cs#L58

from

if (attr != null && attr.StorageType == StorageType.Json)

to

if (attr != null && attr.StorageType == StorageType.Json && hash.ContainsKey("$"))

does that fix the issue for you?

paulers commented 1 year ago

It does indeed. I found that Guid types are not deserialized correctly though. I updated line 407 in the SendToJson method and it's working correctly now:

if (type == typeof(string) || type == typeof(Guid) || type == typeof(GeoLoc) || type == typeof(DateTime) || type == typeof(DateTime?) || type == typeof(DateTimeOffset))

This does fix all the issues I had above for now. Thanks for the assist! Not sure if you want me to close this, or whether you wanna use this issue to track these updates?

slorello89 commented 1 year ago

Keep it open for a bit - this is valid - I see selects working weirdly with GUIDs and I think checking to make sure the $ is in the result set is reasonable enough - so probably worth fixing.