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.

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

Incorrect DateTime filtering #395

Closed
AlexanderAnatolich opened this issue Jun 26, 2023 · 5 comments · Fixed by #406
Closed

Incorrect DateTime filtering #395

AlexanderAnatolich opened this issue Jun 26, 2023 · 5 comments · Fixed by #406

Comments

@AlexanderAnatolich
Copy link

AlexanderAnatolich commented Jun 26, 2023

Versions:
.Net 6
Redis OM 0.5.2

Redis modules:
Redis search 2.4.16
Redis JSON 2.2.0

Steps to reproduce:
Create models, like that:

[Document(IndexName = "SomeModel", Prefixes = new[] { "SomeModel" }, StorageType = StorageType.Json)]
    public class SomeModel
    {
        [RedisIdField]
        [Indexed(Aggregatable = true, Sortable = true)]
        public long Id { get; set; }

        [Indexed]
        public DateTime EventDate { get; set; }
}

Insert list of entities to Redis
In Redis you should have the next data:

{
Id:1
EventDate: 1687824000000  (6/26/2023 5:00:00 PM)
},
{
Id:2
EventDate: 1687881600000 (6/27/2023 9:00:00 AM)
}

Pass the next filter:

model.ToDate (6/28/2023 12:00:00 AM) (2023-06-28T00:00:00.000Z) have the next ticks 638235072000000000
model.FromDate (6/27/2023 12:00:00 AM) (2023-06-27T00:00:00.000Z) have the next ticks 638234208000000000

Execute it the next way:

public async Task<IList<SomeModel>> GetData(SomeFilterModel model)
        {
            try
            {
                var resulted = redisCollection.Where(it=>it.EventDate <= model.ToDate && it.EventDate >= model.FromDate);

               // ToList or ToListAsync doesn't matter
                var result = await resulted.ToListAsync();

                return result;
}

Actual result:
Items with id 1 and id 2 are returned

Expected result:
The only item with id 2 is returned

Comments
The Redis receives an execution command like that:
FT.SEARCH SomeModel ((@EventDate:[-inf 1687910400000])
(@EventDate:[1687824000000 inf])) LIMIT 0 1000
I think the problem is related to time convertation.
Because when I tried to execute the same code from Redis.OM.Modeling.DateTimeJsonConverter, it returns another result:

var datetime = new DateTime(638234208000000000)
new DateTimeOffset(datetime).ToUnixTimeMilliseconds() == 1687849200000

but in the query, I have another value 1687824000000

@AlexanderAnatolich
Copy link
Author

When I'm trying to do it in the next way:

var val1 = new DateTimeOffset(new DateTime(model.FromDate.Value.Ticks));
var val2 = new DateTimeOffset(new DateTime(model.ToDate.Value.Ticks));
var resulted = redisCollection.Where(it=>it.EventDate >= val1 && it.EventDate <= val2);

I'm receiving an error:
Type provided must be an Enum. (Parameter 'enumType')

StackTrace:
at System.Enum.ValidateRuntimeType(Type enumType)
at System.Enum.TryParse(Type enumType, ReadOnlySpan1 value, Boolean ignoreCase, Boolean throwOnFailure, Object& result) at System.Enum.TryParse(Type enumType, String value, Boolean ignoreCase, Boolean throwOnFailure, Object& result) at Redis.OM.Common.ExpressionTranslator.TranslateBinaryExpression(BinaryExpression binExpression) at Redis.OM.Common.ExpressionTranslator.TranslateBinaryExpression(BinaryExpression binExpression) at Redis.OM.Common.ExpressionTranslator.TranslateBinaryExpression(BinaryExpression binExpression) at Redis.OM.Common.ExpressionTranslator.TranslateBinaryExpression(BinaryExpression binExpression) at Redis.OM.Common.ExpressionTranslator.TranslateBinaryExpression(BinaryExpression binExpression) at Redis.OM.Common.ExpressionTranslator.BuildQueryFromExpression(Expression expression, Type type, Expression mainBooleanExpression, Type rootType) at Redis.OM.Searching.RedisCollectionEnumerator1..ctor(Expression exp, IRedisConnection connection, Int32 chunkSize, RedisCollectionStateManager stateManager, Expression1 booleanExpression, Boolean saveState, Type rootType, Type type) at Redis.OM.Searching.RedisCollection1.GetAsyncEnumerator(CancellationToken cancellationToken)
at Redis.OM.Searching.RedisCollection1.<ToListAsync>d__40.MoveNext() at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.<Execute>d__0.MoveNext() at System.Threading.Tasks.ValueTask1.get_Result()
at System.Runtime.CompilerServices.ValueTaskAwaiter`1.GetResult()
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<g__Logged|12_1>d.MoveNext()
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<g__Awaited|10_0>d.MoveNext()
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<g__Awaited|20_0>d.MoveNext()
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<g__Logged|17_1>d.MoveNext()
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<g__Logged|17_1>d.MoveNext()
at Microsoft.AspNetCore.Routing.EndpointMiddleware.<g__AwaitRequestTask|6_0>d.MoveNext()
at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.d__6.MoveNext()
at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.d__5.MoveNext()
at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.d__4.MoveNext()
at HttpStatusCodeExceptionMiddleware.d__3.MoveNext() in HttpStatusCodeExceptionMiddleware.cs:line 25

@imansafari1991
Copy link
Contributor

Try to index the Datetime field as long.
I did this one in the same scenario because I wanted to store DateTime.

[Document(IndexName = "SomeModel", Prefixes = new[] { "SomeModel" }, StorageType = StorageType.Json)]
    public class SomeModel
    {
        [RedisIdField]
        [Indexed(Aggregatable = true, Sortable = true)]
        public long Id { get; set; }
        public DateTimeOffset EventDate { get; set; }
        [Indexed(Sortable = true)]
        public DateTimeOffset EventDateTimeLong =>EventDate.ToUnixTimeSeconds();

}

So, you no longer need to index EventDate.

@AlexanderAnatolich
Copy link
Author

Thanks for the option, but few comments on that:

  • Mostly it looks like a workaround.
  • I will not have the ability to search by that, you can see the error in my previous comment.
  • If I'll have a few DateTime fields, it will look not very clear, why we need to duplicate it and why we are using DateTimeOffset instead of DateTime.

@AlexanderAnatolich
Copy link
Author

AlexanderAnatolich commented Jun 28, 2023

Event if I'm trying to parse the date in the next way:

DateTime parsedDateTime = DateTime.Parse("2023-06-28T00:00:00.000Z", CultureInfo.InvariantCulture, DateTimeStyles.AdjustToUniversal);

And use it in the query, it's anyway working incorrectly. Value in the query applies to my timezone 2023-06-28 00:00:00.000 - 8h.

I have no idea why, but it's working correctly if I wrapping DateTime like that:

var fromDate = new DateTimeOffset(model.FromDate);
var toDate = new DateTimeOffset(model.ToDate); 
redisCollection.Where(it=>it.EventDate >= fromDate.DateTime && it.EventDate <= toDate.DateTime)

@slorello89
Copy link
Member

Hi @AlexanderAnatolich - yeah so if you are querying with DateTimes Redis OM converts the DateTime to a UNIX timestamp prior to submitting it in it's Redis query, and all DateTIme/DateTimeOffset fields should be being converted to UNIX timestamps prior to insertion into Redis. Of course DateTime's do not have timezone information, so when the conversion happens it adjusts the date time based on the local time (hence the unexpected numbers in your range). Im adding a fix to allow you to use DateTimeOffsets in your queries. That should mitigate this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants