npgsql / efcore.pg

Entity Framework Core provider for PostgreSQL
PostgreSQL License
1.54k stars 226 forks source link

Translate DateTimeOffset members #473

Closed SamHuo213 closed 3 years ago

SamHuo213 commented 6 years ago

Hi,

I am using EF Core 2.1 and PostgreSQL. I have some entities that have a column in DateTimeOffset. Whenever I use these columns in a query the execution is happening on Client side. I would like to be able to do diffs on two DateTimeOffset on client side; I have also looked into DbFunctions but there seems to be no support for that either.

The following code is what I am trying to accomplish:

context.Entity .Where(x => EF.Functions.DateDiffSeconds(x.ExpirationTime, DateTimeOffset.UtcNow)) .ToList();

Thanks Sam

roji commented 6 years ago

DateDiffSeconds is defined in SqlServerDbFunctionsExtensions, which means it's an SqlServer-specific method - there's no reason to expect it to work server-side on PostgreSQL.

The Npgsql provider does translate operations on DateTime, so you should be able to write .Where(x => x.ExpirationTime - DateTime.UtcNow > y). However, DateTimeOffset members aren't translated yet - will keep this issue open to track that.

In that meantime, consider using DateTime instead of DateTimeOffset (especially since PostgreSQL doesn't actually save the offset in the database).

roji commented 6 years ago

Actually, thinking about this again, you should be able to subtract one DateTimeOffset from another, yielding a TimeSpan (just like you can subtract one DateTime from another). Give that a try.

SamHuo213 commented 6 years ago

Thank you for the quick response. I followed your recommendation and tried doing the following which did not work the query is still being executed on client side:

context.Entity .Where(x => (x.ExpirationTime - DateTimeOffset.UtcNow) <= TimeSpan.Zero) .ToList();

However, I did do this and it seems to be working on server side now:

var now = DateTimeOffset.UtcNow; context.Entity .Where(x => x => x.ExpirationTime <= now) .ToList();

roji commented 6 years ago

@SamHuo213 great, your working solution is actually the best one.

Regarding the one that isn't working, I'll try to dig deeper but it will probably take me some time to get around to it. Will keep this issue to track that.

sguryev commented 6 years ago

@SamHuo213 thank you for your post. So the trick is to save the static property value to the local variable?

roji commented 6 years ago

Looking at this again, any query which has a DateTimeOffset member (such as UtcNow) will be evaluated client-side because the Npgsql provider currently doesn't translate any DateTimeOffset members. This issue tracks that, and once it's done you should be able to have expressions such as x.ExpirationTime - DateTimeOffset.UtcNow in your query and have that evaluated by PostgreSQL.

The workaround of having a local variable assigned to DateTimeOffset.UtcNow is ugly and not ideal, but it's probably the best temporary solution until translation is implemented.

Please note that DateTime's members are translated, so if you switch from DateTimeOffset to DateTime everything will work as expected. As a general rule DateTime is recommended over DateTimeOffset when using PostgreSQL, since the database doesn't actually save the offset component. Long story short, unless you're very clear on why you need DateTimeOffset (and these cases should be quite rare), it's really recommended you consider using DateTime instead. If you need any more info on this feel free to ask.

SamHuo213 commented 6 years ago

@sguryev , that is correct. Avoiding DateTimeOffset members is my work around. DateTime members such as Now & UtcNow are working. Looks like it is being explicitly translated here: https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/blob/dev/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMemberTranslator.cs.

@roji , thanks for your reply. Yes I know that DateTime members are working. However, the decision to switch types is not up to me. I will try to get management to reconsider switching back to DateTime.

sguryev commented 6 years ago

Hi guys. It's again me. What about the case when I have to compare the Date part of the DateTime (DateTimeOffset). Something like: .Where(x => x.Date.Date <= nowDateTimeOffset) It looks like the issue is in the Date.Date request. I have tried the DbFunctions: .Where(x => EF.Functions.DateDiffDay(x.Date, nowDateTimeOffset) <= 0) But it doesn't work as well.

Does it mean that all DateTime value mutations before filtering are not supported? Thank you! @roji

roji commented 6 years ago

@sguryev, DateTime.Date is properly translated to SQL, producing the following:

SELECT x."Id", x."Date", x."Name"
        FROM blogs AS x
        WHERE DATE_TRUNC('day', x."Date") <= NOW()

However, as written above, no members of DateTimeOffset are currently translated - this is what this issue is about.

Finally, DateDiffDay() is an SQL Server-specific extension and doesn't work when using Npgsql.

sguryev commented 6 years ago

@roji Oh, thank you! So the problem not with the .Date but with the DateTimeOffset.Date. So DateTime.Date should work. Thank you! I didn't check the DateTime usecase yet.

DateDiffDay() returns the same error about the client side evaluation (as I remember). So nothing noted me that this will not work with NPG. And yes - I had to add the SqlServer Nuget package in order to get this function available. I thought that NPG support all functions from SQL + some own from PostgreExtensions

roji commented 6 years ago

It really is discouraged to use timestamp with time zone unless you have a very good reason to do so - consider regular timestamp with DateTime on the. NET side. timestamp with time zone is a quirky data type that is better avoided in most scenarios (this is also why translation of members on DateTimeOffset haven't been implemented yet).

SQL Server is a different database product with it's own provider, anything coming from that package will only work with that provider, not with Npgsql. Many generic components and translations do exist, but they're in the core of EF Core and in not in the SQL Server package.

Apparently DateDiffDay() contains a C# implementation, which means you can do client evaluation. This is why it worked (but triggered client evaluation).

sguryev commented 6 years ago

I'm always use the DateTimeOffset (after a lot of pain with DateTime) because it's UTC time without any doubts. It's absolutely nightmare to work with DateTime when all parts (client browser, web server, db server) are in different time zones. Did you faced the case when ~DB~ APP server (which stores datetime in the local TZ) migrate to another data center with another time zone? https://stackoverflow.com/a/14268167/1521008

Do you understand that timestamp with no timezone offset can be interpreted by the reader as any TZ. When I say - call me at 3PM what will you do? Right! Ask me - what TZ I mean. That the best answer.

The best practice says - use the UTC everywhere. And DateTimeOffset is a UTC stamp with no Kind or other stuff.

Postgre DateTimeOffset .net type mapping is the other question.

Thank you. I've got your point regarding the DbFunctions.

roji commented 6 years ago

sigh, the amount of confusion around date/time types and the number of times I've had this conversation with users.... Note that it isn't your fault, just frustration at the bad date/time types out there, both on the .NET side (DateTime, DateTimeOffset) and on the PostgreSQL side (timestamp with time zone).

I've written about this at length in https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/issues/303, which I suggest you read before responding.

I'm always use the DateTimeOffset (after a lot of pain with DateTime) because it's UTC time without any doubts. It's absolutely nightmare to work with DateTime when all parts (client browser, web server, db server) are in different time zones.

I've faced lots of issues over the years with having multiple parts of the application in different timezones, and I have lots of experience helping users with how to represent timestamps in their database.

DateTimeOffset is not a UTC time. It's a local time, plus an offset; what purpose does the offset serve if all you want is an unambiguous, UTC timestamp? Also, an offset is not a timezone, because it does not take daylight savings variations into account, making DateTimeOffset even less useful than it appears to be; a local time with a real time zone (like NodaTime's ZoneDateTime) at least represents an actual time in an actual timezone.

A DateTime can represent a UTC timestamp just as well, if not much better than a DateTimeOffset (i.e. without the additional ambiguity and complexity of the offset).

Did you faced the case when DB server (which stores datetime in the local TZ) migrate to another data center with another time zone? https://stackoverflow.com/a/14268167/1521008

Your database server does not store date/time in the local timezone, unless you tell it to. If you're doing that, there's a good chance you're shooting yourself in the foot.

As a general principle, it's a really bad idea for any part of your (backend) application to depend on the system timezone configured on the machine where it runs. Your application and database shouldn't care at all about where in the world they are, or about the operating system's configured timezone.

Do you understand that timestamp with no timezone offset can be interpreted by the reader as any TZ. When I say - call me at 3PM what will you do? Right! Ask me - what TZ I mean. That the best answer.

Do you understand that a timezone and an offset are two very different things? And that different daylight savings rules make it very dangerous to represent a timezone as an offset?

Besides, the questions you ask your users are very different from how your timestamps should be represented in your program, and especially in your databases. In most (but not all) cases, the safest option is to convert to and from the user's local time (as determined by their browser/preferences/geolocalization) when user interaction occurs, and otherwise to have UTC everywhere within your program (as you write below). There should be no time zones or offsets being passed around inside your program, or when it talks to a database - only UTC timestamps. Convert to local user-facing representations only at the very edge of your programs.

(this is just general advice - there are definitely some applications where knowledge of timezones inside the application, and even in the database, is indeed important).

The best practice says - use the UTC everywhere.

As I wrote above, I completely agree with that (as a general guideline, for most applications but not all).

And DateTimeOffset is a UTC stamp with no Kind or other stuff.

That is just plain wrong, DateTimeOffset is a local time with an offset, which is another type of "other stuff".

Finally, regardless of the above discussion, timestamp with time zone is another poor choice (in the general case) for storing a timestamp in the database. It does not store a time zone or offset in the database, but simply makes PostgreSQL perform timezone conversions as it's reading and writing values, based on the PostgreSQL session. This makes reading and writing values depend on an environmental PostgreSQL session variable, which you must make sure is correct (with servers across the globe etc.). It provides very little benefits, and lots of pitfalls, compared to just using a timestamp without time zone and treating it as UTC.

sguryev commented 6 years ago

I know that you have such conversations too many times before. I have tried to skip it. I made myself like didn't see it before. But not this time, sorry. And yes, I read #303 before. And yes I was wrong, DateTimeStamp is local time + offset used. I meant that UTC can be easily retrieved from the value (apart from the Unspecified kind for the DateTime which is pretty confused). And yes I know the difference between offset and TZ. Sorry for using the wrong term. Actually I don't care about the TZ at all. I care about the offset only.

A DateTime can represent a UTC timestamp just as well, if not much better than a DateTimeOffset (i.e. without the additional ambiguity and complexity of the offset).

It has .Kind which can be set independently from the original Date and Time value. The main problem that DateTime doesn't store the offset and during (de)serialization\mapping (json=>.net=>json, db=>dotnet=>db, etc) you will have some default behavior with the .Kind value, which differently can be overridden. I'm sure that you can deal with all these conversions and configurations and finally will be sure that any DateTime value you have is UTC time.

.Kind is a "kind of" offset, related to the local timezone on the machine. I understand it. I just prefer as less uncertainties as possible. For me "+3" is much better than "Local" or "Unspecified"

Postgre timestamp with time zone is not a my chose. We are migrating from MSSQL which has no problem with the offset storing. Actually offset always should be +00 as we store UTC in the DB. But it will be specified by the value and not by the EF config (which read them as Unspecified by default) image

I don't think that we have to continue this conversation. You prefer to depend on the config values and look at the machine TZ in order to understand the Local value. You prefer to be confused by the Unspecified kind. I prefer to have the offset as part of the value and be sure in the actual date and time regarding UTC.

And many thanx for your work and cooperation!

roji commented 6 years ago

Just to be clear, I'm just trying to help people understand some difficult date/time nuances, so that they can avoid some risky practices. Of course you'll do whatever you think is right in your application, and ultimately it's not my concern - I'm only directly concerned when you use Npgsql to read and write values to PostgreSQL. I also don't want to really defend DateTime because it's a very bad type; I really recommend using NodaTime, which (for once) implements date/time concepts cleanly and does not have the mess that the BCL date/time types have.

As you say we don't have to continue this conversation, but some of the things you wrote make it clear that you did not understand what I said. This may be because I'm not expressing myself clearly, but in any case I'll try to make it more explicitly. In any case I'll be happy to help with any Npgsql issues regardless of this conversation.

Postgre timestamp with time zone is not a my chose. We are migrating from MSSQL which has no problem with the offset storing. Actually offset always should be +00 as we store UTC in the DB. But it will be specified by the value and not by the EF config (which read them as Unspecified by default)

First of all, if you're migrating from MSSQL, then part of that migration is choosing which database types you're going to use - whether timestamp or timestamp with time zone. It is your decision, and not something that's imposed on you, especially since timestamp with time zone does not correspond to any MSSQL data type, in any way. So you'd be better off reviewing that decision again.

Second, if in MSSQL you're saving a DateTimeOffset, there are two possibilities here (I'm not sure which one is your case):

  1. The offset varies, sometimes it's 0, sometimes it's something. If this is what you're doing, then you're definitely not "UTC everywhere". I don't think this is your case because you've already said you're UTC everywhere.
  2. The offset is always 0. This is a bit... strange. You're wasting database storage for a value which should never change, and opening yourself up to a possible data error in case one day, some developer actually does insert a DateTimeOffset with a non-zero offset (e.g. by mistake). At that point your assumptions (and probably your program) will break. As a general rule, if a value is really always going to be X, it's safer to not store it.

It has .Kind which can be set independently from the original Date and Time value. The main problem that DateTime doesn't store the offset and during (de)serialization\mapping (json=>.net=>json, db=>dotnet=>db, etc) you will have some default behavior with the .Kind value, which differently can be overridden. I'm sure that you can deal with all these conversions and configurations and finally will be sure that any DateTime value you have is UTC time.

I'll say this again, as simply as I can: if you want UTC everywhere, you can simply use DateTime with Kind=UTC everywhere (or even better, NodaTime's Instant). You can write all your software components with the assumption that all timestamps are UTC, everywhere - exactly like how you assume your DateTimeOffsets are always 0. If you're very stressed about accidentally having a non-UTC timestamp somewhere (very understandable!), add assertions in various strategic places to make sure it's always UTC (again, the same idea is valid for DateTimeOffset). There are no conversions to deal with, no ambiguity, and no possibility of error.

I feel like I'm repeating myself, but if all your timestamps are going to be in UTC, then they really don't need to have an offset field telling you that they are.

You prefer to depend on the config values and look at the machine TZ in order to understand the Local value. You prefer to be confused by the Unspecified kind.

If you think that you really misunderstood me - I wrote (in bold!) that your code should ideally never depend on your machine's configured timezone. I never use DateTime with Kind.Local. Kind.Unspecified can be OK if, as I said above, your program always interprets it to mean UTC.

Actually I don't care about the TZ at all. I care about the offset only.

Unless your offset is always zero (at which point why have an offset at all...!), this is a very, very bad idea.

sguryev commented 6 years ago

First of all, if you're migrating from MSSQL, then part of that migration is choosing which database types you're going to use - whether timestamp or timestamp with time zone. It is your decision, and not something that's imposed on you, especially since timestamp with time zone does not correspond to any MSSQL data type, in any way. So you'd be better off reviewing that decision again.

We use the Code-First approach which is full of DateTimeOffset properties. And we support two providers now. It's a SaaS application with ability to be installed directly to the customer environment as a standalone on promise solution. So we are migrating to the Linux+Npg now. Anyway - we have a model and want to share it between both environments. We have chosen DateTimeOffset and actual DB type is not our decision but something provided by the db provider.

You are partially right about 2. We have a complicated flow of date and time values starting from the client browser and ending in the DB (and vice versa). Json date time serialization is also a bit confusing and I prefer to have a single type everywhere (DateTimeStamp in this case). We can do the .ToUniversalTime() or not do it. And in case if someone will mess something and forget about toUTC conversion and non-0 offset will be saved to the DB we still will be able to know the UTC time from the value (apart from the DateTime value where some mistake with Kind or messing with the always-UTC assumption or forgetting about the ToUniversalTime() call can lead to the problems which will be pretty complicated to find). Simple sample - if you will parse the DateTimeOffset.UtcNow().ToString() from DateTime it will be converted to Local which requires .ToUniversalTime() call later. Do you agree that DateTime converts the values during receiving them from client? Do you agree that in this case you as developer HAVE to call .ToUniversalTime() to convert it to the UTC before sending to the DB? Do you agree that missing this call will lead to the error data in the DB? Do you agree that same mistake with DateTimeOffset just set the non-0 value and you still will be able to retrieve the correct data (db reader module can use the .ToUniversalTime() call again which will do nothing for +0 offset values and will fix the +N offset values)?

We don't assume that offset is always 0 with DteTimeOffset. We do .ToUniversalTime() or UtcNow. Same way we did with the DateTime before. This is a point I'm talking about. We don't assume anything. One more time - my main point - storing the offset right after the value. Let it be 0 and even waste SQL storage (actually 2 bytes diff for each value) but be out of the assumptions and guesses.

If you think that you really misunderstood me - I wrote (in bold!) that your code should ideally never depend on your machine's configured timezone. I never use DateTime with Kind.Local. Kind.Unspecified can be OK if, as I said above, your program always interprets it to mean UTC.

I'm talking about the debugging. I had an experience with tracking the time value from the client browser through the app server to the db which all have separate TZ. And when you see the time value under the debugger you have to know the environment TZ to understand what the UTC value is it. And it was a pain because of DateTime conversion to the Local Kind during deserialization. Yeah, most of problems was between browser and app server AFAIR.

I think that it's similar like making the assumption that all people speak in UTC. If John Doe says - let's meet at 9AM we can assume that he has converted the value to the UTC. In terms of DateTimeOffset it will be 9AM UTC. And we can ask him- hey John why do you add UTC every time if it's UTC by default? But once John will forget to convert the time (e.g. by mistake) and say 9AM UTC+5 we still will be able to know the right moment regardless of the TZ. So I prefer explicit offset value rather than assumption. Errors and mistakes in such case are not so dangerous.

roji commented 6 years ago

We use the Code-First approach which is full of DateTimeOffset properties. And we support two providers now. It's a SaaS application with ability to be installed directly to the customer environment as a standalone on promise solution. So we are migrating to the Linux+Npg now. Anyway - we have a model and want to share it between both environments. We have chosen DateTimeOffset and actual DB type is not our decision but something provided by the db provider.

Fair enough. But the choice of a .NET type (DateTimeOffset) doesn't exactly determine your database type - these are two different things. EF Core has value converters, so you could easily translate DateTimeOffset to (UTC) timestamp without time zone in PostgreSQL. So it's better to decide what PostgreSQL data type you really want to use, and then make your program work with that datatype. Once again, I really see no benefit to timestamp with time zone for your scenario.

You are partially right about 2. We have a complicated flow of date and time values starting from the client browser and ending in the DB (and vice versa). Json date time serialization is also a bit confusing and I prefer to have a single type everywhere (DateTimeStamp in this case). We can do the .ToUniversalTime() or not do it. And in case if someone will mess something and forget about toUTC conversion and non-0 offset will be saved to the DB we still will be able to know the UTC time from the value (apart from the DateTime value where some mistake with Kind or messing with the always-UTC assumption or forgetting about the ToUniversalTime() call can lead to the problems which will be pretty complicated to find).

I don't understand... If you're worried about developers messing up, can't they mess up in the same way by calling DateTimeOffset.Now instead of DateTimeOffset.Now instead of DateTimeOffset.UtcNow? What happens if a developer accidentally accesses the Hour (or even Day) property of some DateTimeOffset, without first converting it to UTC?

Using DateTimeOffset does not ensure your developers won't make errors - you're just depending on them using DateTimeOffset in the way that you want, everywhere. To be fair, DateTime.Kind is also a mess - but you seem to be thinking that by using DateTimeOffset you're somehow in a safer place. If you want actual safety, consider using NodaTime instead, where these kinds of issues simply don't happen.

Simple sample - if you will parse the DateTimeOffset.UtcNow().ToString() from DateTime it will be converted to Local which requires .ToUniversalTime() call later. Do you agree that DateTime converts the values during receiving them from client? Do you agree that in this case you as developer HAVE to call .ToUniversalTime() to convert it to the UTC before sending to the DB? Do you agree that missing this call will lead to the error data in the DB? Do you agree that same mistake with DateTimeOffset just set the non-0 value and you still will be able to retrieve the correct data (db reader module can use the .ToUniversalTime() call again which will do nothing for +0 offset values and will fix the +N offset values)?

You're right that when you parse a DateTime from an external string representation (e.g. JSON), you need to make sure you're getting UTC values. But as I wrote above, if you're worried about developers making mistakes, why wouldn't they make them in other places, e.g. forget to use .ToUniversalTime() when accessing your DateTimeOffsets?

More generally, it's a well-known best practice to sanitize external inputs and enforce all your data invariants as close to the edge of your program as possible. What this means is that your parsing layer should in general be responsible for parsing everything correctly, once, and hand off a correct data model to your application, which can now assume everything is in order. As an example, your JSON may contain a string that in reality represents an enum value; your parsing layer should do that translation, and provide a .NET enum which will be used in your application. Similarly, in a "UTC everywhere" application, your parsing layer parses your JSON, and provides a coherent, single-format UTC timestamp, without needing to include a useless offset from the user which you don't want. Make sure that your data constraints are enforced at the very edge, and then assume they're invariants inside your application.

In a way, you've made the situation worse for your developers. Rather than worrying about timezones/offsets and conversions in a single place (i.e. when parsing incoming JSON), you now have DateTimeOffset everywhere in your application (including your database), with various offsets. Developers have to remember to convert to UTC everywhere in the application, and not just when parsing JSON. You've made the offset/timezone problem leak to your entire application.

At the end of the day, if I'm understanding you correctly, your MSSQL database contains (or at least may contain!) DateTimeOffsets with all kinds of offsets, depending on who inserted them and from where. You don't actually care or want these offsets - all you want are "UTC everywhere" values - so you're depending on your developers to always translate to UTC, everywhere where a DateTimeOffset is accessed. This seems much more risky to me than simply sanitizing your external values during the parse phase, at the edges, and making sure that everything is UTC inside your application and database. This is what "UTC everywhere" usually means.

I'm talking about the debugging. I had an experience with tracking the time value from the client browser through the app server to the db which all have separate TZ. And when you see the time value under the debugger you have to know the environment TZ to understand what the UTC value is it. And it was a pain because of DateTime conversion to the Local Kind during deserialization. Yeah, most of problems was between browser and app server AFAIR.

Once again, if your deserialization does its job properly, you can expect to only have correct data inside your application, i.e. UTC timestamps. The fact that some developer messed up your parsing by passing local timestamps doesn't necessarily mean that you need DateTimeOffset - it could mean that you need to fix your parsing layer.

sguryev commented 6 years ago

If you're worried about developers messing up,

You have mentioned the developers messing up. I just added one more argument. DateTimeOffset stores the offset in the value. And allows to find the error. So DateTimeOffset forgives the errors. DateTime is not.

I didn't say that developers have to

to convert to UTC everywhere in the application

And there DateTimeOffset and DateTime are absolutely equal. Both require to UTC conversion here or there. I don't understand why did you spend so much letters there.

MSSQL may contains different offsets. And I usually don't have a parsing layer. We create a REST API and parsing is usually done by standard ASP.NET mechanism (with some adjustments here or there). So you pass the JSON to the API endpoint and get the C# object model on the server side.

roji commented 6 years ago

You have mentioned the developers messing up. I just added one more argument. DateTimeOffset stores the offset in the value. And allows to find the error. So DateTimeOffset forgives the errors. DateTime is not. [...] And there DateTimeOffset and DateTime are absolutely equal. Both require to UTC conversion here or there. I don't understand why did you spend so much letters there.

You're missing the point. With DateTimeOffset there's the possibility of having different offsets, as you've already said. This means that if any developer accesses the Hours property (or other properties), they get a non-UTC hour and you have a bug. This can happen anywhere in your code, to anyone.

With DateTime, you only have to ensure UTC once, at the edge (i.e. when parsing JSON). After that single verification you can access DateTime's accessors without ever worrying about developer errors. That is a lot less room for errors.

You can think of it another way. If you made sure, when parsing your JSON, to always produce DateTimeOffsets with offset 0, then DateTimeOffset and DateTime would be very similar - you ensure correct data only once, during parsing. The question is if you need to worry about it once, or everywhere.

MSSQL may contains different offsets.

Either it's possible to have different offsets in your database, or it isn't - there is no such thing as "it's technically possible but not really". If it's possible, then you're not really UTC everywhere, and your entire program has to be prepared for non-zero offsets, everywhere, on each and every line of code that handles timestamps.

And I usually don't have a parsing layer. We create a REST API and parsing is usually done by standard ASP.NET mechanism (with some adjustments here or there). So you pass the JSON to the API endpoint and get the C# object model on the server side.

Take a closer look at JSON.NET date serialization: it seems easy to tell your JSON parser to do UTC DateTime (which means you're UTC everywhere in your JSONs as well.

sguryev commented 6 years ago

You're missing the point. With DateTimeOffset there's the possibility of having different offsets, as you've already said. This means that if any developer accesses the Hours property (or other properties), they get a non-UTC hour and you have a bug. This can happen anywhere in your code, to anyone.

It's not a problem to call the .ToUniverslTime() before comparison. https://blogs.msdn.microsoft.com/davidrickard/2012/04/06/datetime-and-datetimeoffset-in-net-good-practices-and-common-pitfalls/

Anyway, I spent pretty much time before on dealing with DateTime issues. I have spent pretty much time on this conversation. I have no problems since use the DateTimeOffset (I think 3+ years). But next time I will try with the DateTime again and come back with the list of problems I faced.

roji commented 6 years ago

OK, thanks for keeping an open mind.

As mentioned by the blog post you linked to, DateTime is also a really problematic type - unfortunately .NET simply doesn't have sane date/time types. If you do look at changing things, I really do recommend that you look at NodaTime. It's true that you'll have to do with conversion at the edges (when read/writing to JSON but also to your database), but these should at least be very local problems. Once you have NodaTime types, it becomes absolutely clear exactly what each type is representing, and there is no possible confusion between local/UTC etc.

voronov-maxim commented 5 years ago

This is expression visitor fakes DateTimeOffset to DateTime This is query transalted to sql

SELECT o."Id", o."AltCustomerCountry", o."AltCustomerId", o."CustomerCountry", o."CustomerId", o."Date", o."Name", o."Status"
FROM dbo."Orders" AS o
WHERE ((DATE_PART('year', o."Date")::INT = 2016) AND (DATE_PART('month', o."Date")::INT > 3)) AND (DATE_PART('day', o."Date")::INT < 20)
using OdataToEntity.Infrastructure;
using System;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;

namespace OdataToEntity.Test
{
    internal sealed class DateTimeOffseMembersVisitor : ExpressionVisitor
    {
        internal sealed class Visitor : ExpressionVisitor
        {
            protected override Expression VisitMember(MemberExpression node)
            {
                Expression expression = base.Visit(node.Expression);
                if (expression.Type == node.Expression.Type)
                {
                    if (node.Member is PropertyInfo property)
                    {
                        if (property.PropertyType == typeof(DateTimeOffset))
                        {
                            var propertyInfo = new OeShadowPropertyInfo(property.DeclaringType, typeof(DateTime), property.Name);
                            return Expression.Property(expression, propertyInfo);
                        }
                        if (property.PropertyType == typeof(DateTimeOffset?))
                        {
                            var propertyInfo = new OeShadowPropertyInfo(property.DeclaringType, typeof(DateTime?), property.Name);
                            return Expression.Property(expression, propertyInfo);
                        }
                    }
                }
                else
                    return Expression.MakeMemberAccess(expression, expression.Type.GetMember(node.Member.Name).Single());

                return node;
            }
            protected override Expression VisitUnary(UnaryExpression node)
            {
                if (node.NodeType == ExpressionType.Quote)
                    return base.Visit(node.Operand);

                if (node.NodeType == ExpressionType.Convert)
                {
                    if (node.Type == typeof(DateTimeOffset))
                        return Expression.Convert(base.Visit(node.Operand), typeof(DateTime));
                    if (node.Type == typeof(DateTimeOffset?))
                        return Expression.Convert(base.Visit(node.Operand), typeof(DateTime?));
                }

                return node;
            }
        }
        protected override Expression VisitMember(MemberExpression node)
        {
            if (node.Expression.Type == typeof(DateTimeOffset) &&
                (node.Member.Name == nameof(DateTimeOffset.Day) ||
                node.Member.Name == nameof(DateTimeOffset.Month) ||
                node.Member.Name == nameof(DateTimeOffset.Year)))
                return new Visitor().Visit(node);

            return base.VisitMember(node);
        }
    }
}

OeShadowPropertyInfo

roji commented 5 years ago

@voronov-maxim if you want to contribute, please submit a pull request. Also please take a look at the existing translator for DateTime and see about integrating it there rather than a new provider.

forenheith commented 3 years ago

Why such a request couldn't be translated?

var emailsDictionary = await context.Set<UserEntity>()
.AsNoTracking()
.GroupBy(entity => entity.Email)
.Select(group => new { Email = group.Key, Count = group.Count() })
.ToDictionaryAsync(entity => entity.Email, entity => entity.Count, CancellationToken.None);

Here is the error I have:

The LINQ expression 'DbSet    .GroupBy(        source: c => c.Email,         keySelector: c => c)' could not be translated.

roji commented 3 years ago

@forenheith your query doesn't seem to have anything to do with DateTimeOffset or this issue. Please open a new issue with a runnable code sample.

roji commented 3 years ago

Pulling in as part of #1970