OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.33k stars 2.36k forks source link

DateTimeOffset Field #4593

Open ns8482e opened 4 years ago

ns8482e commented 4 years ago

There should be a DateTimeOffset field. Currently date/time is stored in UTC and The original creator date/time is calculated using timezone defined in Website setting. As timezone offset that created the record is not stored in db.

If DateTimeOffset field is added then It will store date/time along with timezone offset which created the record, that can be used independently to retrieve original local date/time and/or calculated to any requested timezone. (Scenarios like ETL, Import, Export, Timezone in user preference)

The possible use case is Airline booking where the Date/Time displayed are always local to From/To.

Also, DateTime field with UTC is not enough when collaboration between users with geographically located in different areas of the world is required within same Tenant

sebastienros commented 4 years ago

At first I wrote that it was not necessary. Then I wrote that you only needed an extra number field for the timezone. Then I wrote that you needed a custom timezone field because an extra number would not be accurate. Then I realized the date would still be converted in utc.

So now I will just write that I agree. We need an extra Datetime + timezone field (two properties I think, easier to store). Unless we already made a date field and a time one, in which case we just need a TZ.

Skrypt commented 4 years ago

In fact what you need is a Offset context. The Datetime in that case would be still saved as UTC in the database. But then you need a context over that Datetime which will apply the Offset on that Datetime. Then the only difference is the context in that case. The context itself carries the Offset required.

Let's take the example of the Airline booking.

The date of the travel is set as UTC in the database but the Airline company itself has a location. So then the Datetime is converted accordingly to the location of the Airline company. If you want to know when you will arrive to destination you still require to make some Datetime zone conversion because you could also be changing timezones while traveling... In all these cases there is not really a need for storing a TZ with the Datetimes but a related context to apply a timezone to.

If you want to always display your stored Datetimes stored as UTC in your database to a specific timezone then you can just make a custom fixed context for it.

Here are some good reading : https://codeblog.jonskeet.uk/2019/03/27/storing-utc-is-not-a-silver-bullet/

SQLite doesn't support saving as a DatetimeOffset. And yes 2 properties like @sebastienros said. So to make this easier, it would be a Field that stores 2 values. One Datetime saved as UTC and one TimeZoneId to define the offset which should be a string value (TimeZoneId: Europe/Amsterdam). That way you can also retrieve the region ; a simple offset is not really great.

Though if we want to follow how fields are designed. They should save data as to a DB type that match with them. For example a Textfield saves a nvarchar in the database. So, one solution could also be to have a special editor for the Textfield that lists all the TimezoneId's. Then you would have a Datetime field template that would use this textfield timezoneid to display it's proper fixed timezone.

Other solution could be to create a DatetimeOffset Content Part that has 2 fields. One datetime and one Textfield with this timezoneid selector.

ns8482e commented 4 years ago

@sebastienros @Skrypt DateTimeOffset field will reduce ambiguity and will be better to represent point-in-time in data store. It will save, Local time and Offset from UTC (when instance was created). This will allow OC developer to have one more option to choose from :)

Here's what Microsoft's doc says about C# DateTimeOffset.
https://docs.microsoft.com/en-us/dotnet/standard/datetime/choosing-between-datetime#the-datetimeoffset-structure

Uniquely and unambiguously identify a single point in time. The DateTimeOffset type can be used to unambiguously define the meaning of "now", to log transaction times, to log the times of system or application events, and to record file creation and modification times.

Perform general date and time arithmetic.

Preserve multiple related times, as long as those times are stored as two separate values or as two members of a structure.

It makes perfect sense to use DateTimeOffset in certain scenario in-place of DateTime (UTC). In such scenarios , UTC doesn't make sense e.g. date/time as an abstract - All stores around the world would open 9:00 AM on 2nd Jan 2020, in such case exported date would represent different meaning.

I don't foresee Timezone to be part of DatetimeOffset field, as It's location part of DateTime that will be in most cases as input for requested local time so storing Offset is enough.

Application like Dynamics uses DateTimeOffset. Also vertical applications for Banking and Finance rely on DateTimeOffset.

As far as SQLite's support is concern, OC is not using DBType - It's stores C# serialized value in JSON hence, we can simply serialize C# DateTimeOffset object.

Skrypt commented 4 years ago

Ok, the main reason why normally you would not save a DateTime with an Offset directly is because of the fact that a TimeZone Offset could potentially change. This is why it is safer to save a TimeZoneId than save a Offset in a database. The Offset could change from -5 to -4 depending on daylight saving time or just because the country decided to move from -3 to -3.5 for business reasons.

If we allow to save a DateTimeOffset then why are we even using NodaTime? Please read carefully what Jon Skeet says about it. DatetimeOffset is good for manipulating dates at runtime but for data persistence in a database it is ambiguous!

And Microsoft is wrong about it 😉

As far as SQLite's support is concern, OC is not using DBType - It's stores C# serialized value in JSON hence, we can simply serialize C# DateTimeOffset object.

For this part there is no issue at all I agree. Though, we have a feature that indexes all the Fields named Field Indexing which will take those values and map them in actual tables. For now it works with Datetime DBType but it would not work for DatetimeOffset as SQLite doesn't support it.

Skrypt commented 4 years ago

@ns8482e @sebastienros I'm not against the idea as we are not here to prevent people making mistakes and these DBTypes are existing for people to use them. If people want to have a DateTimeOffset Field then give them one. Personally though, I don't agree that this would be a good solution unless we save the NodaTime TimeZoneId with the DateTime.

Else, this would be a Field that would not be matching with the principles NodaTime tries to enact. So let's say that this Field would exist for those who "don't care" about NodaTime and require to store DateTimeOffsets that could be prone to errors.

And as Jon Skeet points out in his blog post we might want instead to introduce timestamps.

ns8482e commented 4 years ago

@Skrypt There is no request to change any existing API or field here. The request here is to add one more option for customers who likes to build their solution on OC to allow them to define their data as they see in their other applications outside orchard core so that data (date/time) stays unchanged between either side of the fence.

They can choose either DateTime or DateTimeOffset depending on their specific requirement.

Even with DateTimeOffset field, user will still continue to see DateTime converted using Website setting in UI- only underlying datastore values would look different.

If Website's Timezone is set to LA PDT(-07:00), data in database would be stored as following

DateTime Field

"EndTime": {
        "Value": "2019-10-20T05:00:00Z"
 }

DateTimeOffset Field

"EndTime": {
        "Value": "2019-10-19T22:00:00-07:00"
 }

In UI End time: 10/19/2019 10:00 PM

Skrypt commented 4 years ago

Question is more if we should support this and provide support over it. Because I can already see users ask questions about it and how they should be able to convert it from one timezone to an other while remaining accurate. At this point this is not my call. I'm just giving you my point of view on it.

You can move forward with the implementation ; I won't stop you from doing this 😄

sebastienros commented 4 years ago

I checked and we do have a Date, Time and DateTime (UTC) fields. I think we only need a TimeZone field that stores the exact time zone by its identifier, probably a Nodatime one, and ensure we have the tools (filters, services) to apply this TZ to existing DateTime fields.

@ns8482e with that, following your latest example, you want to render the datetime with a fixed timezone, whatever the end user is using locally, then you store two fields, and when loading the datetime, you can convert it to the TZ (the second field) using a special filter/razor-helper that we would need to provide with this field.

ns8482e commented 4 years ago

@sebastienros In my example, the UI will still show DateTime by calculating using website timezone. Means If the website timezone is changed to CDT - UI will show 10/20/2019 : 12:00 AM. Whereas the database value will remain unchanged "2019-10-19T22:00:00-07:00"

sebastienros commented 4 years ago

using a special filter/razor-helper that we would need to provide with this field.

This way you force which timezone you want to render the datetime in

sebastienros commented 4 years ago

Another option is to create a custom "Display" that lets you select which timezone a field should use to be rendered. But the setting would then be set for the field itself.

ns8482e commented 4 years ago

@sebastienros @Skrypt I understand your concern about handling DateTime in general in OC, However using two fields to define single field means like going back to SQL 2008 era for some people.

To me (and also others who are looking forward to use Orchard core CMS as App framework for their applications)---

  1. People use application to manage their data and Ideally App should store data as-is without alteration/modification if possible. Only owner of the data understands the true meaning and context of such data and their usage inside and outside of the application.

  2. Secondly, for some there are some regulatory requirements that doesn't allow application to alter the user defined data , example could be financial transactions, prescription drug information - that can only be modified by physician under federal law. Some historical events stays unchanged. To meet such requirement, in SQL 2008 and earlier versions people use two datetime fields one for local date time (for regulatory requirement) and another one for UTC (to be globalize the nature of the app)

With introduction of datatype of DateTimeOffset - It eliminates need of maintaining two fields in database, as it holds local value + offset to UTC, that allows application developer to calculate UTC. Also the use of these datatype increased in cloud applications in place of just DateTime. Name a few like Microsoft Dynamics, Azure Data lakes and Common Data Service.

I guess It is equally important for orchard community that how it would like to see orchard core down the road?

and I believe no one likes to convert their date/time of their data to UTC to utilize orchard core in their next application.

sebastienros commented 4 years ago

Now I am confused if you want a single Orchard Field because you actually want a single database field. First, we don't store any "orchard field value" in a database field directly. It's only in json. And we provide an index usually to query them with SQL, but these indexes can be customized if you need something different. But the most important restriction is that only SQL Server supports DateTimeOffset, so we would definitely do any default index with this type. Even the EF team tells not to use it. Expose DateTimeOffset in APIs if you want, but always store a UTC datetime, that's the recommendation.

I agree with providing a new core Orchard Filed for Timezone. But if you need one that creates a DataeTimeOffset column then you should create your own.

ns8482e commented 4 years ago

@sebastienros Sorry If I have confused you. I understand that using ContentField module I can create custom fields of my own.

However, My only concern with out-of-the-box date/time field is, that original user provided value is not stored but calculated in UI

It would be nice to have a way in OC to store the original user provided date/time value out-of-the-box, which can be achieved using DateTimeOffset field ( stored as in example above )

I agree that - DateTimeOffset is not the silver bullet - However many applications out there that uses it - If customers would like to use Orchard Core, then they can't bring data as is from these applications, as it would require them to convert all dates to UTC first, which leads to programming / maintenance overhead just for one date/time field.

Even export from OC to these applications could become overhead for them as they cannot able to use stored value as-is as it requires conversion of date/time from UTC.

Skrypt commented 4 years ago

@ns8482e We understand your concern with this. You need to consider that we want to avoid having people getting into issues with Datetimes. I don't think that converting datetimes from UI is a performance concern.

It's worth nothing what I will say here but I'm migrating data all the time using SQL Server and this is part of the game ...

One thing here that is true is that DatetimeOffset is used in other applications out there. But that is not a reason that we should support it because of that. For me, I prefer having accurate timezone conversion.

You are saying that you will need to migrate some DatetimeOffset to OC eventually but your issue is that you can't map these to a Nodatime TimeZone as for example GMT-5 could point to multiple different TimeZones. Here, the issue is that your DatetimeOffset is not accurate enough to be migrated. So, what I would do here is simply create a Datetime Field and a TextField that list all the Nodatime Timezones with an optional NumericField that is used to store the fixed Offset of your DatetimeOffset migrated data. That's a good compromise and people could start using the Nodatime Timezone selector instead of the NumericField for setting their Offset. And you could filter that Nodatime list by displaying only timezones that matches the offset you had in your DatetimeOffset. And who knows if you have actually some localization data (country code) then you can easily find timezones with that too.

Also here some way to list only timezones that are using a specific offset in Nodatime :

https://stackoverflow.com/questions/33544494/how-to-get-all-iana-timezones-from-an-offset-in-nodatime

https://nodatime.org/TimeZones

Apart than that, if you still don't want to migrate any DatetimeOffset because you want to save yourself the dev overhead of that then you can create your own DatetimeOffsetField for your own usage but I would consider migrating the data.

Skrypt commented 4 years ago

@sebastienros We might consider doing a "DatetimeOffsetPart" that has these 3 Fields and does some logic/magic on using either the TimeZoneId or the Numeric Offset when it's set.

Datetime : [ datetime picker ] Offset : [ List of offsets available with Nodatime ] Country : [ List of available countries ] Timezone : [ List of timezone id's] (required)

Each of these values could be saved in database when selected and they could pre-filter the next lists. So for example selecting an Offset would filter the list of countries available and also timezones ...

That way you can store and migrate your DatetimeOffsets from a different database without loosing your Offsets.

ns8482e commented 4 years ago

@Skrypt I agree that because others are using DateTimeOffset, should not be the reason to add such field in orchard core. But the reason should be the underlying problem. There should be out-of-the-box support for local date (or an absolute date without tied to timezone) i.e. what you enter is what you see in json) in orchard core.

DateTime Field(UTC) filed is surely way to make application globalized and work for 80% scenarios - and most importantly for 100% in orchard core with one time migration

But in co-exists scenario, conversion could be nightmare - like a toll booth :).

I feel there should also be an OOTB option in OC for those users who wants to develop co-exist scenario with orchard core, and would like to have their date/time managed by themselves (you may call it a bad decision not using UTC, but that sometime required by regulatory requirements) and not converted to UTC.

Skrypt commented 4 years ago

Who made those regulatory requirements and if you can, show me a website that refers to it or something?

If you absolutely need to use DatetimeOffset because of some law and because of migration pain then make your own field to bypass this issue ; but I personally don't see these 2 reasons valid enough to force ourselves into supporting this and having it in OC because of previously named reasons. Show me something that would change my mind... I'm still just not convinced enough.

ns8482e commented 4 years ago

@Skrypt looks like you missed to read my previous comment related to regulatory requirement example below,

example could be financial transactions, prescription drug information - that can only be modified by physician under federal law.

Easy to reproduce, currently. Create content type "prescription" add "prescription date" and "prescription expiry date", - Create content item- publish and print on paper. As administrator (who likely to be not a physician) Change website settings and change time zone. Go back to prescription record and print - You compare two printout - You see that Administrator who's not physician has changed the prescription data i.e. date and expire date.

I am not a lawyer nor developer from a lawfirm so I can't provide specifics on regulatory requirements, but I can certainly pinpoint that DateTime field has a problem but it's upto you decide on whether to look at the problem or to ignore the problem - I can't convince anyone.

I will certainly bypass the issue and close.

Skrypt commented 4 years ago

So here the issue you are refering to is about converting a datetime when rendering it ; not about how you will store it. If the intent is to have a fixed timezone for when rendering a datetime ; refer to the ideas I gave you in my previous posts. Maybe, the default of converting a datetime field with the current preferred website timezone for you is not what you are looking into. There are ways to bypass this conversion that I've not looked into yet but I could certainly take time to look at a solution for you for this later on this week.

Sorry to not say what you wanted to hear. Nothing against you.

And it needs to be a Field not a Part as we couldn't have more than one part per content type. And you can leave the issue opened as we are not ignoring it.

ns8482e commented 4 years ago

@Skrypt thank you. The Issue is not just for display but it starts when it stored as UTC- once input date/time is converted to UTC the original offset is lost as its not stored - hence co-exists scenarios mentioned in above comments is not possible ( i.e. reading values directly from db outside orchard core system). That's the reason I can think DateTimeOffset would solve the issue - However I am open to better solution If the same can be achieved using single field with alternate ways.

sebastienros commented 4 years ago

I am ok with adding a field that contains both a local DateTime (not converted to UTC, and not a DateTimeOffset) and a TimeZone. Such that the default display would render the date as entered, and the timezone. But custom displays, and custom templates (these are different, very important here) could render it differently. We also need to ensure the helpers and filters allow to convert this field values to other timezones (like UTC), or to convert this field to DateTimeOffset.

And we might also still need a TimeZone content field.

ns8482e commented 4 years ago

@sebastienros If offset is not stored, then I believe, the Timezone will return correct offset for past dates (and future dates) based on DST changed history.

e.g. If i am not mistaken In 2006 and earlier, DST used be end in last Sunday October but 2007 till now it ends on first Sunday of November.

Skrypt commented 4 years ago

I need to test this but it should yes.