Skip to content

Using LINQ expression to handle DateTime comparisons produces inconsistent results. #76

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

Closed
acupofjose opened this issue Nov 11, 2023 · 4 comments

Comments

@acupofjose
Copy link
Contributor

Referenced Lines:

/// <summary>
/// An instantiated class parser (i.e. x => x.CreatedAt &lt;= new DateTime(2022, 08, 20) &lt;- where `new DateTime(...)` is an instantiated expression.
/// </summary>
/// <param name="column"></param>
/// <param name="op"></param>
/// <param name="newExpression"></param>
private void HandleNewExpression(string column, Operator op, NewExpression newExpression)
{
var argumentValues = new List<object>();
foreach (var argument in newExpression.Arguments)
{
var lambda = Expression.Lambda(argument);
var func = lambda.Compile();
argumentValues.Add(func.DynamicInvoke());
}
var constructor = newExpression.Constructor;
var instance = constructor.Invoke(argumentValues.ToArray());
if (instance is DateTime dateTime)
{
Filter = new QueryFilter(column, op, dateTime.ToUniversalTime());
}
else if (instance is Guid guid)
{
Filter = new QueryFilter(column, op, guid.ToString());
}
else if (instance.GetType().IsEnum)
{
Filter = new QueryFilter(column, op, instance);
}
}

Issue Report:
Re: supabase-community/supabase-csharp#121

Reproducing this issue [from @aquatyk here]

DateTime day1 = DateTime.Now.AddDays(-1);
string day = day1.ToString("yyyy-MM-dd", CultureInfo.InvariantCulture);

// Produces incorrect result.
var result = await _client.From<Item>().Where(x => x.Created_at >= day1).Get();

// Produces correct result.
var result = await _client.From<Item>().Filter("created_at", Operator.GreaterThanOrEqual, day).Get();
@acupofjose
Copy link
Contributor Author

@wiverson do you see any obvious reason why it would be problematic to remove .ToUniversalTime() on L205 there?

acupofjose added a commit that referenced this issue Nov 13, 2023
@acupofjose
Copy link
Contributor Author

This is a deviation from how the original query worked, so it's been removed. It is incorrect to force a ToUniversalTime conversion in the clause here.

Now available in 3.2.10

@wiverson
Copy link
Collaborator

Sorry I'm late, FWIW I think most ORMs would prefer deferring to the underlying datetime w/tz instead of the conversion. I wish all dbs and timestamps were UTC but alas.

FWIW I'd add some test cases to cover more time/date stuff for the different db date/time configurations or at least add as a issue for later?

@acupofjose
Copy link
Contributor Author

acupofjose commented Nov 13, 2023

Agreed! I'll add it as a chore. Thanks @wiverson!

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

No branches or pull requests

2 participants