DateTime handling between model binders and RestApiClient (ARC-153)

[cYCL157]

Cort
5:39 PM
i know it's not your concern any more, but thinking over this....
5:39
https://github.com/cortside/cortside.aspnetcore/pull/16/files
5:40
this is handling of datetimes -- trying to make sure they are handled the same between model binders and deserializers
5:40
i've learned a few things that make me even more surprised we have not had issues to this point
5:41
i think:
we want to parse request data to either utc OR local consistently
we want to serialize to UTC always
5:42
the first would mean that we could have a service that runs and relies on running in a specific timezone -- i.e. product-api -- values in a request should always be UTC but could be any qualified datetime
5:45
if request data has datetime that is not qualified, i.e. 2000-10-02 12:00:00 AM, parse that as if it were already in the target datetime kind -- utc or local
5:45
that means the service can use just use the dates without having to convert anything and the kind should always end up being whatever the expected handling is -- utc or local
5:46
i am trying to avoid anything that would be a datetimekind of unspecified and not supporting round trip
5:48
for serializing things, i.e. for controller response, always serialize to utc as iso8601 -- least ambiguous and should always work for deserialization
5:48
no -- this does not handled birthdate
5:49
that also means that serializing request and stringifying of values in request in restapiclient would always be utc -- and deserialization of response would have to depend on that global set of settings and deserialize to whatever was set.....utc or local (edited)

Erik
5:51 PM
haha yeah I should bill regions for this
☝️
1

Cort
5:52 PM
i added a new enum for cortside.aspnetcore -- DateTimeHandling with values of Utc and Local -- it's similar to DateTimeKind and DateTimeZoneHandling but limited to only the 2 values i think i really want
5:53
in typing this all out -- i can see 2 things that i did not do to match this -- i think i override the json serializer in the controller setup but did not for global -- and i am not always serializing to utc

Erik
5:53 PM
seems sound though - other than I hadn't gotten to the point of which way might be better to assume for unspecified kind. should the caller be responsible for properly formatting? I think it might only matter when local != utc... or was thinking that at one point anyway

Cort
5:54 PM
yeah, it probably only does matter when local != utc -- but it did seem significant
5:55
i've done and changed things a few times -- it's hadn't been easy to see a clear definition....until typing this.....assuming it's "right"

Erik
5:56 PM
yeah agreed - I did run into .ToLocalTime() behaving a little differently than I thought it was going to when handling a UTC time, unspecified/non-iso8601 format from application-api

Cort
5:56 PM
i think i based it on the fact that ideally we want everything utc everywhere -- incoming, outgoing, persisted

Erik
5:57 PM
yeah, and only stop-gap to fix anything legacy in the meantime but ensure utc "contract" expecation

Cort
5:57 PM
along with the accept anything and convert to utc

---

add DateTimeModelBinderProvider to product-api, to convert DateTime query params to local time while product-api continues to run in mountain time zone instead of UTC

Story Narrative

Gracefully handle UTC or any iso8601 compliant datetime in Product-api, which is a query parameter (as opposed to part of json body), so that GET requests can convert timezone information to Local while product-api containers continue to run in mountain time.

Acceptance Criteria
All system rest services and front-end clients should be using iso8601 compliant datetime values when making http requests so that roundtrip timezone handling is consistent and reliable.

In product-api, add a DateTimeModelBinderProvider : IModelBinderProvider (example below) to convert datetime values in query parameters to local time, which is needed because product-api containers run in mountain time zone in the kubernetes cluster.

set up in Startup.cs as follows

order set to 0 to be “first” – where does that fit with deserialization from json/xml?

this seems like it would impact both query string parameters and body the same – not necessarily bad, but does potentially overlap some of the json deserializer settings

services.AddControllers()
    .AddMvcOptions(options =>
    {
        options.ModelBinderProviders.Insert(0, new DateTimeModelBinderProvider());
    });

Remove any existing .ToLocalTime() use in GET endpoints and provide list of endpoints affected for validation

In maintenance-web, check all requests to product-api that send datetimes as query parameters to ensure they are iso8601 compliant and converted to utc timezone

smoke test with local machine timezone set to other than mountain time against product-api in a deployed environment as well as in mountain time

Example code

using System;
using System.Globalization;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc.ModelBinding;

namespace Acme.ShoppingCart.WebApi.Filters {
    /// <summary>
    /// Converts UTC datetimes in query params to local time
    /// Should remove this if product-api is ever fully converted to run in utc timezone in kubernetes cluster
    /// </summary>
    public class DateTimeModelBinderProvider : IModelBinderProvider {
        internal static readonly DateTimeStyles SupportedStyles = DateTimeStyles.AssumeUniversal;

        /// <inheritdoc />
        public IModelBinder GetBinder(ModelBinderProviderContext context) {
            if (context == null) {
                throw new ArgumentNullException(nameof(context));
            }

            var modelType = context.Metadata.UnderlyingOrModelType;
            if (modelType == typeof(DateTime)) {
                return new UtcAwareDateTimeModelBinder(SupportedStyles);
            }

            return null;
        }
    }

    /// <summary>
    /// Model binder to convert UTC to local in query param iso8601 compliant formatted dates
    /// </summary>
    public class UtcAwareDateTimeModelBinder : IModelBinder {
        private readonly DateTimeStyles _supportedStyles;


        /// <summary>
        /// constructor
        /// </summary>
        /// <param name="supportedStyles"></param>
        /// <exception cref="ArgumentNullException"></exception>
        public UtcAwareDateTimeModelBinder(DateTimeStyles supportedStyles) {

            _supportedStyles = supportedStyles;
        }

        /// <summary>
        /// Bind model
        /// </summary>
        /// <param name="bindingContext"></param>
        /// <returns></returns>
        /// <exception cref="ArgumentNullException"></exception>
        /// <exception cref="NotSupportedException"></exception>
        public Task BindModelAsync(ModelBindingContext bindingContext) {
            if (bindingContext == null) {
                throw new ArgumentNullException(nameof(bindingContext));
            }

            var modelName = bindingContext.ModelName;
            var valueProviderResult = bindingContext.ValueProvider.GetValue(modelName);
            if (valueProviderResult == ValueProviderResult.None) {
                // no entry
                return Task.CompletedTask;
            }

            var modelState = bindingContext.ModelState;
            modelState.SetModelValue(modelName, valueProviderResult);

            var metadata = bindingContext.ModelMetadata;
            var type = metadata.UnderlyingOrModelType;

            var value = valueProviderResult.FirstValue;
            var culture = valueProviderResult.Culture;

            object model;
            if (string.IsNullOrWhiteSpace(value)) {
                model = null;
            } else if (type == typeof(DateTime)) {
                // You could put custom logic here to sniff the raw value and call other DateTime.Parse overloads, use TryParse instead etc
                model = DateTime.Parse(value, culture).ToLocalTime();
            } else {
                // unreachable
                throw new NotSupportedException();
            }

            // When converting value, a null model may indicate a failed conversion for an otherwise required
            // model (can't set a ValueType to null). This detects if a null model value is acceptable given the
            // current bindingContext. If not, an error is logged.
            if (model == null && !metadata.IsReferenceOrNullableType) {
                modelState.TryAddModelError(
                    modelName,
                    metadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor(
                        valueProviderResult.ToString()));
            } else {
                bindingContext.Result = ModelBindingResult.Success(model);
            }

            return Task.CompletedTask;
        }
    }
}