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

Conversion of DateTime.Date doesn't work as expected #28380

Closed
elfico opened this issue Jul 6, 2022 · 20 comments
Closed

Conversion of DateTime.Date doesn't work as expected #28380

elfico opened this issue Jul 6, 2022 · 20 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@elfico
Copy link

elfico commented Jul 6, 2022

Hello everyone,

We have a project we are working on and we need to query the database for some data for a particular date.

We configured our DB to read and write date as UTC.

When writing the query to get the data, I noticed that the data for a date was not being pulled from the database.

Here is the code:

transactionDate = Convert.ToDateTime("2021-11-10:T10:00:00").ToLocalTime();

var transactions2 = _transactionsRepo.Query()
  .Where(transaction => transaction.AccountId == pharmacy.AccountId.Value)
  .Where(transaction => transaction.TransactionDate.Date == transactionDate.Date)
  .OrderByDescending(transaction => transaction.TransactionDate)
  .Skip(numToSkip)
  .Take(pageSize);

On investigation, I noticed that when pulling the data, the DB returns the date as UTC as it should and the date is compared to the input date. But no data is returned. I checked the query generated and noticed this:

DECLARE @__transactionDate_1 datetime = '2021-11-10T10:00:00.000';
DECLARE @__p_2 int = 0;
DECLARE @__p_3 int = 10;

SELECT *
FROM [WalletTransactions] AS [w]
WHERE ([w].[AccountId] = @__AccountId_Value_0) AND (CONVERT(date, [w].[TransactionDate]) = @__transactionDate_1)
ORDER BY [w].[TransactionDate] DESC
OFFSET @__p_2 ROWS FETCH NEXT @__p_3 ROWS ONLY

From the above, the query generated shows that the TransactionDate is converted to just Date and compared to the input date @__transactionDate_1 which is in DateTime form.

Any help on this will be deeply appreciated.

@roji
Copy link
Member

roji commented Jul 6, 2022

My first guess here would be some timezone-related mismatch, because of mixing local and UTC timestamps; the use of ToLocalTime above seems like it could be problematic given you're saying you're reading and writing as UTC.

To be able to help further, can you share a fully runnable, minimal example showing the problem?

@elfico
Copy link
Author

elfico commented Jul 7, 2022

My first guess here would be some timezone-related mismatch, because of mixing local and UTC timestamps; the use of ToLocalTime above seems like it could be problematic given you're saying you're reading and writing as UTC.

To be able to help further, can you share a fully runnable, minimal example showing the problem?

Hi @roji thanks for the response.

Here is a link to the Repo containing a runnable minimal example: https://github.com/elfico/dateconversion

@ajcvickers
Copy link
Contributor

@elfico Running your code with a few more WriteLines shows this output:

3 data seeded
7/7/2022 4:30:06 PM
7/7/2022 4:30:06 PM
7/7/2022 4:30:06 PM
Local DateTime for 2022-07-07 is 7/7/2022 12:00:00 AM
Universal DateTime for 2022-07-07 is 7/6/2022 11:00:00 PM
Looking for 7/6/2022 12:00:00 AM
0
Looking for 7/7/2022 12:00:00 AM
3

Notice that the result of Convert.ToDateTime("2022-07-07") is local time for today. But for me, once I convert this to UTC, it becomes yesterday. Your results will vary depending on your time zone, but I expect that the same thing is happening on your machine.

@stevendarby
Copy link
Contributor

The issue raised is that the sql parameter contains the time.

@ajcvickers
Copy link
Contributor

@stevendarby That doesn't repro in the full project posted:

      Executed DbCommand (3ms) [Parameters=[@__transactionDate_Date_0='2022-07-06T00:00:00.0000000Z' (DbType = DateTime), @__p_1='0', @__p_2='10'], CommandType='Text', CommandTimeout='30']
      SELECT COUNT(*)
      FROM (
          SELECT [a].[TransactionId], [a].[AccountId], [a].[Amount], [a].[Balance], [a].[DateCreated], [a].[DateUpdated], [a].[TransactionDate], [a].[TransactionStatus], [a].[TransactionType]
          FROM [AccountTransactions] AS [a]
          WHERE CONVERT(date, [a].[TransactionDate]) = @__transactionDate_Date_0
          ORDER BY [a].[TransactionDate] DESC
          OFFSET @__p_1 ROWS FETCH NEXT @__p_2 ROWS ONLY
      ) AS [t]

@stevendarby
Copy link
Contributor

stevendarby commented Jul 7, 2022

@ajcvickers yeah it doesn't seem a good repro of the initial post, but try changing the setting of dateTime1 in RunTasksAsync to:
DateTime dateTime1 = Convert.ToDateTime("2022-07-07T10:00:00").ToLocalTime();

If you're in a non-UTC aligned time zone you might see a time in the parameter. In the UK, I see 2022-07-06T23:00:00.0000000Z. Maybe this is the expected behaviour given the conversion set up but I found it a bit surprising to see .Date translated to something with a time and I think that's what the original issue was about - not 100% though.

@ajcvickers
Copy link
Contributor

@stevendarby This is interesting. I suspect there is some interaction between the DateTime.Kind and the parameters, hopefully only in the way they are displayed in logging. It's worth investigating.

@elfico
Copy link
Author

elfico commented Jul 7, 2022

Apologies @ajcvickers and @stevendarby , the original post was pulled from an API project and I had to create a console app to contain just the part needed.
@stevendarby , you are right. The .Date is getting translated with a time. Using .ToUniversalTime() doesn't make any difference either.

@stevendarby
Copy link
Contributor

stevendarby commented Jul 8, 2022

@ajcvickers It doesn't seem to just be the logging, I used SQL Server Profiler and captured

@__transactionDate_Date_0='2022-07-06 23:00:00'

Commenting out the HasConversion code in the context sends it through as 2022-07-07. I didn't think value converters on the column would affect values it's compared to, but that could just be lack of knowledge on my part - hopefully these findings are useful though.

@roji
Copy link
Member

roji commented Jul 8, 2022

@stevendarby can you post a minimal code sample which shows this odd behavior? I'm no longer sure from the above how to minimally repro it.

@stevendarby
Copy link
Contributor

@roji Sure, see below. Can comment out the OnModelCreating to see change in the value used.

using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using System;
using System.Linq;

{
    using var context = new MyContext();
    context.Database.EnsureDeleted();
    context.Database.EnsureCreated();

    var dateTime = new DateTime(2022, 07, 07, 12, 0, 0);
    var query = context.Entities.Where(x => x.DateTime.Date == dateTime.Date);

    Console.WriteLine(query.ToQueryString());
}


public class MyContext : DbContext
{
    public DbSet<Entity> Entities { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder
            .UseSqlServer("Server=.;Database=DateTest;Trusted_Connection=True;TrustServerCertificate=True;")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder
            .Entity<Entity>()
            .Property(x => x.DateTime)
            .HasConversion(
                v => v.ToUniversalTime(),
                v => DateTime.SpecifyKind(v, DateTimeKind.Utc));
    }
}

public class Entity
{
    public int Id { get; set; }
    public DateTime DateTime { get; set; }
}

@roji
Copy link
Member

roji commented Jul 8, 2022

This seems like the expected behavior to me. Here's what's going on:

  1. dateTime.Date is evaluated in .NET, yielding a value with no time (midnight).
  2. Since dateTime has Kind=Unspecified, the resulting date also has Unspecified.
  3. The value converter receives this value, and calls ToUniversalTime on it. For Unspecified values, this assumes the it's in the machine's current time zone, and performs a timezone conversion.

I understand it may look weird, but things are working as expected... To clarify, imagine that instead of dateTime.Date you happen to pass in a regular Unspecified dateTime instance which happens to be at midnight; you'd expect that to undergo a timezone conversion, and no longer be at midnight, right? The above is pretty much the same: we don't see the fact that .Date is being called client-side, just as if you extracted the Date call out of the LINQ query:

var dateTime = new DateTime(2022, 07, 07, 12, 0, 0);
var date = dateTime.Date;
var query = context.Entities.Where(x => x.DateTime.Date == date);

My general recommendation would be to work with explicitly-UTC DateTimes, rather than converting them in an EF value converter; but I'm guessing that's not something you want to do. When SqlClient adds support for DateOnly (dotnet/SqlClient#1009), that would be a good way to work around this.

@stevendarby
Copy link
Contributor

stevendarby commented Jul 8, 2022

@roji I think it makes sense that a parameter values passes through a column-mapped property's converter if it the parameter is being directly compared to the property - otherwise, comparisons between different types wouldn't work (the fact this is a DateTime-DateTime converter somewhat obscures this I think).

However, the parameter isn't actually being compared directly to the property, but a property on that property (x.DateTime.Date). So is it actually right to pass the parameter value through that converter?

Compare to this, where instead of x.DateTime.Date we use x.DateTime.TimeOfDay, which returns a totally different type:

var time = new TimeSpan(12, 0, 0);
var query = context.Set<Entity>().Where(x => x.DateTime.TimeOfDay == time);

In this case, time obviously doesn't pass through the property's value converter and the query is like this:

DECLARE @__time_0 time = '12:00:00';

SELECT [e].[Id], [e].[DateTime]
FROM [Entity] AS [e]
WHERE CONVERT(time, [e].[DateTime]) = @__time_0

So I'm think that just because x.DateTime.Date returns the same type as x.DateTime, it still doesn't mean the parameter value should pass through the value converter for x.DateTime.

In summary my questions would be

// Passes dateTime through Entity.DateTime converter - OK because direct comparison
var dateTime = new DateTime(2022, 07, 07, 12, 0, 0);
var query = context.Set<Entity>().Where(x => x.DateTime == dateTime);

// Does not pass time through the converter - OK because it's obvious this wouldn't make sense
var time = new TimeSpan(12, 0, 0);
query = context.Set<Entity>().Where(x => x.DateTime.TimeOfDay == time);

// Passes dateTime through Entity.DateTime convert converter - Why? 
dateTime = new DateTime(2022, 07, 07, 12, 0, 0);
query = context.Set<Entity>().Where(x => x.DateTime.Date == dateTime);

@roji
Copy link
Member

roji commented Jul 8, 2022

However, the parameter isn't actually being compared directly to the property, but a property on that property (x.DateTime.Date). So is it actually right to pass the parameter value through that converter?

Well, consider dateTime.AddDays() instead of Date:

var query = context.Entities.Where(x => x.DateTime.AddDays(1) == dateTime.AddDays(1));

It's essentially the same as the Date example above, but I suspect you do want the value converter to apply to the results of AddDays(), no?

In this case, time obviously doesn't pass through the property's value converter and the query is like this:
[...]
So I'm think that just because x.DateTime.Date returns the same type as x.DateTime, it still doesn't mean the parameter value should pass through the value converter for x.DateTime.

We could indeed do this, and stop propagating the operand's type mapping when Date is applied (we could even make it return date instead of datetime2). But I'm not sure that's what most users want; consider the following:

var query = context.Entities.Where(x => x.DateTime.Date == dateTime);

If dateTime is Unspecified/Local, I think user's may legitimately expect it to be converted to UTC before being compared; that's why they set up the value converter, after all.

In other words, I can see pros and cons in both directions, and I think some people would be unhappy with either one. If that's the case, it's probably better to not break anything and leave the current behavior.

@stevendarby
Copy link
Contributor

In other words, I can see pros and cons in both directions, and I think some people would be unhappy with either one. If that's the case, it's probably better to not break anything and leave the current behavior.

I agree. I enjoyed poking at this but it's a complex area and current behaviour is mostly fine. I also agree that SqlClient (and EF) support for DateOnly could help with this specific case in future. For example, one day this might be a better way to do a comparison of the date only, which the value converter probably wouldn't 'interfere' with:

var date = new DateOnly(2022, 7, 7);
var query = context.Set<Entity>().Where(x => DateOnly.FromDateTime(x.DateTime) == date);

@roji
Copy link
Member

roji commented Jul 8, 2022

Yep, exactly. At the end of the day, DateTime really is a badly designed type - I keep harping on how NodaTime is a superior alternative, and is even supported on some EF providers. DateOnly/TimeOnly make the situation a bit better, though I'd still be very wary of DateTime.

@roji
Copy link
Member

roji commented Jul 8, 2022

@elfico is there anything that still needs addressing here from your perspective?

@elfico
Copy link
Author

elfico commented Jul 9, 2022

@elfico is there anything that still needs addressing here from your perspective?

Thanks @roji . I quite understand you explanation. I was thinking that since .Date was called on the datetime object then a conversion will also be applied to the supplied input in the sql query just like the conversion applied to the database column.
At the end, irrespective of what date is entered or passed to the value converter i.e, Utc or Local, I would only want to do a comparison to the date part of the datetime.

What would you suggest as the best way to go about this. Removing the value converter would mean refactoring the entire codebase to convert all dates to UTC first before saving. We figured doing it in one place would eliminate that kind of work and also ensure future code converts automatically rather than relying on the engineer to remember to convert to UTC.

Thanks for you timely response.

@roji
Copy link
Member

roji commented Jul 9, 2022

First, while I understand the desire to avoid refactoring your entire codebase to use UTC, I do recommend considering this. If the intent is to store UTC timestamps, then it's highly recommended that your DateTime instances be of Kind=UTC. This would also prevent possible bugs if someone calls e.g. ToUniversalTime, or various other cases. Bottom line: if it's UTC, mark it as such.

That aside, you should be able to specifically work around this issue by making sure that the specific DateTime instance is UTC:

// Assume we get an Unspecified DateTime from somewhere:
var dateTime = new DateTime(2022, 07, 07, 12, 0, 0);
// Mark the Unspecified DateTime as UTC as follows (this can be done outside the query as well).
// This will make the result of Date be a (truncated) UTC DateTime as well, and your value converter calling ToUniversalTime won't have any effect on it.
var query = context.Entities.Where(x => x.DateTime.Date == DateTime.SpecifyKind(dateTime, DateTimeKind.Utc).Date);

@elfico
Copy link
Author

elfico commented Jul 10, 2022

Thanks @roji for the recommendations. I will discuss with this with my team.

Thanks @stevendarby and @ajcvickers for your contributions.

@elfico elfico closed this as completed Jul 10, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2022
@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants