-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Input fields don't emit (and thus round-trip) offset value when binding against DateTimeOffset fields #6648
Comments
@dougbu can you take a look and share thoughts on this? |
Options include:
|
Thanks Doug, but given that 'DateTimeOffset' is the generally recommended way to store dates in .NET now, I think we need to think more about a first-class way to support this better. It isn't always about the time-zone the user is in, but just the general proper handling of the data being bound. In this case, you suffer data loss which is terrible. We need to to mitigate that somehow, e.g. render a hidden field with original offset and have the model binder use it in connection with the value in text box when binding. Ideally we need a solution for user selection of the offset too though. |
@DamianEdwards your "data loss" is my "incomplete" 😺 (I didn't mention the user's timezone except in 3️⃣) To clarify, data loss occurs today in two ways: Users can't see what timezone a value is in and model binding can't infer what timezone a user meant. About all that works semi-cleanly is round-tripping when the user does not make a change. I agree a separate field (option 2️⃣ perhaps with an added hidden field or tag helper attributes to control the generated field) avoids the data loss in both directions. Is there another generally-accepted approach I've missed? Separately, should MVC generate something different when the user explicitly chooses |
Data is lost when the original value has an offset that is different to the server's system locale. That should be fixed as it's clearly a bug in our handling of this end-to-end. First class support for offset selection is a more advanced matter that we should investigate. |
Yes, the chosen RFC3339 format ( I'm unsure about offering a partial solution e.g. just providing the hidden field only helps with round-tripping data the server provided. |
We can talk in person then. It's simply broken for cross-timezone scenarios, and we have no guidance on allowing selection of time-zone on top of that. We need to do something here. |
Looks like this is actually a regression in ASP.NET Core 2.0.0. We currently run in RFC 3339-compliant mode and render This appears to also be a regression from ASP.NET Core 1.1.0 behaviour. We switched to be RFC 3339-compliant by default in in 1.0.0-rc2 (see issue #4102, PR #4250 and 6b369ef). But, the change to use I'll test ASP.NET Core 1.1 to confirm the defaults round-trip the offset later today or tomorrow… |
Have confirmed problem does /not/ exist in ASP.NET Core 1.1.0. |
Possible approaches:
If we're talking about taking this in 2.0.1, I recommend doing 1. initially. That's an E2E fix which involves very little code. I'm probably leaving out another half dozen approaches. Please speak up, especially if you have an idea we could implement in a patch. |
I agree on fixing this via option 1 in a servicing release. We may need to offer a quirk switch to enable opting in to the 2.0 RTM behavior given the breaking nature of the fix. Beyond that I don't care to think too much about right now other than I think we should do a much better job of supporting |
You should audit how well ASP.NET Core handles Converting to and from ISO date strings and DateTime(local,utc,unspecified) + DateTimeOffset is surprisingly complex. |
Allow me to offer some guidance. First, to clear up a few points:
Regarding presenting the user with a drop-down lists of time zones, I'll point out that the example @dougbu linked to on Mozilla's site is flawed. Time zone names can only be mapped to identifiers such as those used with The ideal mapping for My recommendation is to use Offsets are interesting. To be future-proof and spec-compliant with RFC 3339 and ISO 8601, they would have to support any valid I've written some code in this dotnetfiddle that shows how to build a list of offsets, both in "simplified" form that shows just the current offsets used throughout the world (covering both standard and daylight time), and how to build an expanded form showing all representable offsets that a Of course, one might want to generate the full list via JavaScript to save on bandwidth - but that's not currently possible with the simplified form without pulling in JS libraries. |
By the way, a similar drop-down list is already in use as a common control in the Azure Portal. For example, try to create a scheduled web job, and you'll notice the schedule asks for a Date, Time, and UTC Offset. The offsets in the list are created from code similar to what I gave in my fiddle above. |
The points @JamesNK and @mj1856 raised are important especially for our long-term plans and the guidance we eventually provide. With respect to #6648 aka our short-term fixes, our plan was basically to stop auto-selecting Going beyond the above, I'd appreciate input on:
A couple of quibbles:
|
This is what Json.NET does with DateTimeKind (and DataContractSerializer which I largely followed) when parsing ISO dates. 2017-08-30T20:45:13+00:00 = Local And vice-versa when writing. You then need to worry about converting each of those to DateTimeOffset when parsing. Writing DateTimeOffset is simpler: always include the offset with the ISO date. DateTimeOffset = 2017-08-30T20:45:13+00:00 |
Thanks @JamesNK - Yes, that's the expected behavior. Though worth noting that if an offset is present and thus a @dougbu - further response forthcoming. Thanks. |
I did some testing. Presently with a (Also, the docs need updating to show If we wanted to be able to round-trip For All of the above and current implementations leave To answer some questions posed:
It is quite a distinction, in that
No, they don't. They have the same date and time values, but when
I think I think the confusion around the word "local" here is part of the problem. HTML5 is borrowing terminology from ISO8601 (as do Noda Time, Java 8, and others). In that context, "local" just means "local to someone somewhere", where in .NET local means "on the local machine where the code is executing". (BTW, there's a bike-shed for this in tc39/proposal-temporal#33 if you're interested). Really,
We can warn them in documentation, but I cannot think of any technical way to resolve this other than that already described. Round-tripping the offset in a hidden field doesn't seem like a good option, as it doesn't allow the user to adjust it. (ex: Pacific is now -07:00 (PDT), but maybe -08:00 (PST) for a different date).
YES - by promoting the
No, according to this chart, and still limited support for ``type="datetime-local"`. I tested myself on Chrome, Edge, FF, and IE (all latest on Win10) and found the same results.
Sure, but .NET does have a one-way implicit cast from |
Thanks very much @mj1856. However, I'd like to focus this issue on what we can do about losing
...
...
What is the bottom line here? Should MVC's next patch change how
Seems like a good idea. But, what does "promote" mean in this context? |
For
|
Going back to workarounds:
@model DateTimeOffset
<input asp-for="@Model" type="datetime" class="form-control" />
[DisplayFormat(ApplyFormatInEditMode = true, DataFormatString = "{0:o}")]
[UIHint("text")]
services.AddMvc(options => options.ModelMetadataDetailsProviders.Add(new DatetimeMetadataProvider()));
...
private class DatetimeMetadataProvider : IDisplayMetadataProvider
{
public void CreateDisplayMetadata(DisplayMetadataProviderContext context)
{
if (context.Key.ModelType == typeof(DateTimeOffset) ||
context.Key.ModelType == typeof(DateTimeOffset?))
{
// If model type hasn't been annotated as we'd like...
var metadata = context.DisplayMetadata;
if (string.IsNullOrEmpty(metadata.EditFormatString) &&
string.IsNullOrEmpty(metadata.DataTypeName) &&
string.IsNullOrEmpty(metadata.TemplateHint))
{
// Fix things up. TemplateHint = "DateTime" does not help. Changing EditFormatString is not
// required when using DateTimeOffset. Then again, the default DateTimeOffset format in most
// locales loses sub-Second information.
metadata.EditFormatString = "{0:o}";
metadata.TemplateHint = "text";
}
}
}
} The above choices also work for |
@dougbu - I agree with most of that. Note that I do think it's super important that |
I really appreciate your suggestions @mj1856!
This seems like cruft in the documentation and I agree we should improve it. Suggest we also mention:
A PR would certainly help. @scottaddie? @Rick-Anderson? |
Agreed. I'm not going to make any changes undoing a developer's explicit selection of this |
Right, I mean that |
FYI I edited my earlier comment because using
|
- #6648 - a different take on #4871 - `DateTime` can also round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]` - since they're now handled differently by default, add more `DateTime` tests - expand tests involving `Html5DateRenderingMode.CurrentCulture` nits: make VS-suggested changes to files updated in this PR
@mj1856 Please let me know if you plan to submit a docs PR for this; otherwise, I'll add it to my list of things to do. |
@scottaddie - Yep, will do shortly. Thanks. |
- cherry-picked from 7e4a8fe in dev - #6648 - a different take on #4871 - `DateTime` can also round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]` - since they're now handled differently by default, add more `DateTime` tests - expand tests involving `Html5DateRenderingMode.CurrentCulture` nits: make VS-suggested changes to files updated in this PR
…quirks mode - patch recipients can use switch to undo the #6648 fix
- cherry-picked from 7e4a8fe in dev - #6648 - a different take on #4871 - `DateTime` can also round-trip `DateTimeKind.UTC` with `[DataType("datetimeoffset")]` or `[UIHint("datetimeoffset")]` - since they're now handled differently by default, add more `DateTime` tests - expand tests involving `Html5DateRenderingMode.CurrentCulture` nits: make VS-suggested changes to files updated in this PR
…quirks mode - patch recipients can use switch to undo the #6648 fix
Was working on the ASP.NET Core App-building Workshop and discovered an interesting issue when model binding to properties of type
DateTimeOffset
, especially when the application is running on a machine with a different system locale (and thus default time zone) than theDateTimeOffset
values being edited.Consider a Tag Helper use like
<input asp-for="StartTime" />
, whereStartTime
is defined aspublic DateTimeOffset StartTime { get; set; }
on the model. The rendered HTML for theinput
field will not include the offset value:<input class="form-control" type="datetime-local" id="StartTime" name="StartTime" value="2017-08-14T09:00:00.000" />
. When this is posted back, the model bound value will now have an offset of the machine's system locale, even if the field wasn't changed, resulting in a different value.We need to consider what the correct behavior of the helpers should be here, and how it interacts with the HTML5
datetime-local
field type. Or perhaps we need specific guidance (and maybe helpers/binders) for handlingDateTimeOffset
values.@rynowak @Eilon
The text was updated successfully, but these errors were encountered: