Skip to content

Add support for DateOnly/TimeOnly #1169

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

Merged
merged 2 commits into from
Feb 7, 2023
Merged

Add support for DateOnly/TimeOnly #1169

merged 2 commits into from
Feb 7, 2023

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Jul 10, 2022

This PR adds support for using DateOnly and TimeOnly in resource models, which were introduced in .NET 6.

PostgreSQL fully supports these types. But unfortunately, System.Text.Json support and ModelState binding support for these types are missing in .NET 6. They were added in .NET 7. To use these types in .NET 6:

  1. Add a NuGet reference to System.Text.Json v7 or higher.
  2. Add a NuGet reference to DateOnlyTimeOnly.AspNet and call builder.Services.AddDateOnlyTimeOnlyStringConverters(); at startup.

While implementing this, I found that query string parsing uses the OS-level culture settings. For example, if you're running your API on an English language version of Windows, but have customized regional settings to display dates in Dutch format, then you'll need to use Dutch notation for date values in query strings (for example 31-12-2022 instead of 12/31/2022). This doesn't apply to JSON request/response bodies, so the behavior is quite inconsistent. This PR fixes that by always using the Invariant culture. A backward-compatibility switch has been added, so we don't need to wait for the next major release. To revert to the old behavior, add the following at startup:

AppContext.SetSwitch("JsonApiDotNetCore.ParseQueryStringsUsingCurrentCulture", true);

Fixes #1168.

NOTE: This PR depends on #1249 and #1253, which should be merged first.

QUALITY CHECKLIST

@bkoelman bkoelman marked this pull request as draft July 11, 2022 22:38
@codecov
Copy link

codecov bot commented Jul 17, 2022

Codecov Report

Merging #1169 (9d17ce1) into master (8e8427e) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
+ Coverage   92.63%   92.69%   +0.05%     
==========================================
  Files         243      243              
  Lines        7806     7815       +9     
==========================================
+ Hits         7231     7244      +13     
+ Misses        575      571       -4     
Impacted Files Coverage Δ
...tations/Resources/Internal/RuntimeTypeConverter.cs 100.00% <100.00%> (+10.81%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bkoelman bkoelman marked this pull request as ready for review July 17, 2022 21:24
@bkoelman bkoelman requested a review from maurei July 17, 2022 21:24
@bkoelman bkoelman marked this pull request as draft September 3, 2022 16:54
@bkoelman
Copy link
Member Author

bkoelman commented Sep 3, 2022

Converted back to Draft because this hasn't been tested with ModelState validation, so may not work.

@bkoelman bkoelman removed the request for review from maurei September 3, 2022 16:56
@bkoelman
Copy link
Member Author

bkoelman commented Oct 2, 2022

Turns out this isn't going to work well with ModelState validation on .NET 6, because RangeAttribute internally depends on System.ComponentModel.TypeConverter, which doesn't support DateOnly and TimeOnly. The missing support was added in .NET 7. But there does not seem to be a separate NuGet package we can reference for .NET 6 to make this work out of the box.

Therefore I think we should postpone this PR until JsonApiDotNetCore targets .NET 7.

Update: There's a polyfill NuGet package now, to fill the gap for .NET 6.

@bkoelman bkoelman force-pushed the data-types branch 5 times, most recently from b7184fd to 78d753c Compare February 4, 2023 17:09
@bkoelman bkoelman marked this pull request as ready for review February 4, 2023 17:46
@bkoelman bkoelman requested a review from maurei February 4, 2023 17:47
@maurei maurei self-requested a review February 7, 2023 08:36
@bkoelman bkoelman merged commit e4cf9a8 into master Feb 7, 2023
@bkoelman bkoelman deleted the data-types branch February 7, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add support for DateOnly and TimeOnly model properties
2 participants