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

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

Closed
3 tasks
pranavkm opened this issue Jul 21, 2021 · 14 comments
Closed
3 tasks

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

pranavkm opened this issue Jul 21, 2021 · 14 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@pranavkm
Copy link
Contributor

pranavkm commented Jul 21, 2021

  • Required for Blazor/MVC
    • Model Binding
    • Blazor Routing
    • Regular Routing
@pranavkm pranavkm added area-runtime area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-runtime labels Jul 21, 2021
@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding labels Jul 21, 2021
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Jul 21, 2021
@ghost
Copy link

ghost commented Jul 21, 2021

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
Copy link
Member

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

@pranavkm
Copy link
Contributor Author

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: #11584 (comment)

@ghost
Copy link

ghost commented Aug 12, 2021

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.

@TanayParikh TanayParikh changed the title Add DateOnly and TimeOnly support to model binding (Mvc) Add DateOnly and TimeOnly support to model binding & routing Aug 23, 2021
@TanayParikh TanayParikh added the area-blazor Includes: Blazor, Razor Components label Aug 23, 2021
@rafikiassumani-msft rafikiassumani-msft removed their assignment Aug 30, 2021
@srpeirce
Copy link

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
Copy link
Member

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

@maxkoshevoi
Copy link

@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 (dotnet/runtime#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
Copy link
Contributor

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

@pranavkm pranavkm modified the milestones: Backlog, .NET 7 Planning Oct 19, 2021
@mkArtakMSFT mkArtakMSFT added Priority:1 Work that is critical for the release, but we could probably ship without triaged labels Nov 11, 2021
@mkArtakMSFT mkArtakMSFT removed the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Dec 6, 2021
@mkArtakMSFT
Copy link
Member

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

@simonziegler
Copy link

simonziegler commented Feb 10, 2022

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
Copy link
Member

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

@Nick-Stanton
Copy link
Member

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 #45243 (comment)).

@maxkoshevoi
Copy link

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
Copy link
Member

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

@ghost ghost locked as resolved and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

No branches or pull requests