dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.48k stars 10.04k forks source link

Add DateOnly and TimeOnly support to model binding & routing #34591

Closed pranavkm closed 1 year ago

pranavkm commented 3 years ago
ghost commented 3 years ago

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

davidfowl commented 3 years ago

I want to mention that the TryParse heuristic that we added in minimal API means we support these out of the box 😄

pranavkm commented 3 years ago

Hmm, your comment reminded me that we want to special case DateTime. The default DateTime.Parse (and I imagine DateTime.TryParse), formats the date to a server local time. We had to author a model binder in MVC to parse it as UTC. @DamianEdwards did some research on this: https://github.com/dotnet/aspnetcore/issues/11584#issuecomment-506007647

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

srpeirce commented 3 years ago

Is this the reason I get the following error when returning DateOnly when returning via controller? Or should I raise a separate issue.

System.NotSupportedException: Serialization and deserialization of 'System.DateOnly' instances are not supported and should be avoided since they can lead to security issues. Path: $.Date.
 ---> System.NotSupportedException: Serialization and deserialization of 'System.DateOnly' instances are not supported and should be avoided since they can lead to security issues.

🤔 image

DamianEdwards commented 3 years ago

Seems that's from the underlying serializer in System.Text.Json, see this issue dotnet/runtime#53539

maxkoshevoi commented 3 years ago

@srajkovic Here's a workaround if you're interested: https://kevsoft.net/2021/05/22/formatting-dateonly-types-as-iso-8601-in-asp-net-core-responses.html (NuGet)

@DamianEdwards DateOnly/TimeOnly promise to be popular types, and currently (as of RC1) Asp.Net doesn't support them as both body (dotnet/runtime#53539) and query (https://github.com/dotnet/runtime/issues/59253) parameters. Both can be worked around with something like this, but shouldn't Asp.Net support it out of the box until runtime catches up in .Net 7? I feel like this will be a common issue after the release of .Net 6.

srajkovic commented 3 years ago

@maxkoshevoi, I think you meant to tag @srpeirce. DateOnly and TimeOnly look great though!

mkArtakMSFT commented 2 years ago

@rafikiassumani-msft some of this work will be an ask from your team for .NET 7.

simonziegler commented 2 years ago

Are you going to review change detection for component parameters too in https://github.com/dotnet/aspnetcore/blob/main/src/Components/Components/src/ChangeDetection.cs#L37? Suggested change as below.

    // The contents of this list need to trade off false negatives against computation
    // time. So we don't want a huge list of types to check (or would have to move to
    // a hashtable lookup, which is differently expensive). It's better not to include
    // uncommon types here even if they are known to be immutable.
    private static bool IsKnownImmutableType(Type type)
        => type.IsPrimitive
        || type == typeof(string)
        || type == typeof(DateTime)
        || type == typeof(DateOnly)    // to be added
        || type == typeof(TimeOnly)    // to be added
        || type == typeof(Type)
        || type == typeof(decimal)
        || type == typeof(Guid);
danroth27 commented 2 years ago

At this point we don't believe this will make it into .NET 7. Moving to .NET 8.

Nick-Stanton commented 1 year ago

Is there anyone who is still blocked by this? It seems that these types should work following the addition of the TryParseModelBinder in .NET 7 and discussion has pivoted to whether there is merit to providing public DateOnly and TimeOnly model binders for those that want further customization (see https://github.com/dotnet/aspnetcore/pull/45243#discussion_r1036612318).

maxkoshevoi commented 1 year ago

Seems like everything except using TimeOnly as a dictionary key (fix is already merged for .NET 8) is working out of the box in .NET 7

Nick-Stanton commented 1 year ago

Closing this issue since it's been a month and it appears that DateOnly and TimeOnly already work in .NET 7 with the addition of the TryParseModelBinder