[API Proposal]: Supply set of TypeConverters during configuration binding

[christopherbahr]

Background and motivation

When using IConfiguration.Bind() the default TypeConverter provided from the TypeDescriptor https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L874

This proposal is to add an IDictionary<Type, TypeConverter> that would override that behavior and allow callers to specify the TypeConverter to be used during configuration binding.

Likely the best place to add that would be on BinderOptions but I don't have a strong opinion about that.

This would allow customization of the binding without the possible unintended consequences of adding a TypeConverter to the class for all cases like TypeDescriptor.AddAttributes(typeof(TimeSpan), new TypeConverterAttribute(typeof(StrictTimeSpanConverter)));

The particular case I'm trying to address comes out of an incident we had around converting to TimeSpan from string. This has this surprising (although documented) behavior:

TimeSpan.Parse("23:00:00"); // 23 hours, 0 minutes, 0 seconds
TimeSpan.Parse("25:00:00"); // 25 days, 0 hours, 0 minutes

I'd like to add a custom StrictTimeSpanConverter : TypeConverter that disallows TimeSpans of the form "25:00:00" to prevent this error from recurring but I'm wary of overriding the TypeConverter for all TimeSpan conversions.

API Proposal

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.Extensions.Configuration
{
    /// <summary>
    /// Options class used by the <see cref="ConfigurationBinder"/>.
    /// </summary>
    public class BinderOptions
    {
        /// <summary>
        /// When false (the default), the binder will only attempt to set public properties.
        /// If true, the binder will attempt to set all non read-only properties.
        /// </summary>
        public bool BindNonPublicProperties { get; set; }

        /// <summary>
        /// When false (the default), no exceptions are thrown when trying to convert a value or when a configuration
        /// key is found for which the provided model object does not have an appropriate property which matches the key's name.
        /// When true, an <see cref="System.InvalidOperationException"/> is thrown with a description
        /// of the error.
        /// </summary>
        public bool ErrorOnUnknownConfiguration { get; set; }

        ///<summary>
        /// A map of <see cref="System.Type"> to <see cref="System.ComponentModel.TypeConverter">. The specified
        /// converter will be used instead of the default converter when trying to bind properties with of a given Type.
        ///</summary>
        public IDictionary<Type, TypeConverter> TypeConverterOverrides { get; set; }
    }
}

API Usage

/// <summary>
/// Provides a type converter to convert <see cref='System.TimeSpan'/> objects to and from
/// various other representations. Disallowes timespans of the format "d:hh:mm".
/// </summary>
public class StrictTimeSpanConverter : TimeSpanConverter
{
    /// <summary>
    /// Converts the given object to a <see cref='System.TimeSpan'/>
    /// object.
    /// </summary>
    public override object? ConvertFrom(ITypeDescriptorContext? context, CultureInfo? culture, object value)
    {
        var ts = base.ConvertFrom(context, culture, value) as TimeSpan?;
        if(ts != null && ts.Value.Days > 0 && value is string text)
        {
            // Will throw if the timespan doesn't have the days field
            var strictTs = TimeSpan.ParseExact(text, @"d\.hh\:mm\:ss", culture);
            if(strictTs != ts)
            {
                throw new FormatException("Exact parse of TimeSpan including days didn't match generic parse.");
            }
        }
        return ts;
    }
}

public class TimeSpanConfig
{
     TimeSpan Time {get; set;}
}

var builder = new ConfigurationBuilder();
builder.AddJsonFile("config.json");
var config = builder.Build();
TimeSpanConfig timeSpanConfig = new();
config.Bind(timeSpanConfig, (options) => 
                            { 
                                    options.TypeConverterOverrides  = new Dictionary<Type, TypeConverter>
                                    {
                                            [typeof(TimeSpan)] = new StrictTimeSpanConverter()
                                    };
                            });

config.json
{
     "Time" : "25:00:00"
}

Alternative Designs

No response

Risks

No response

[christopherbahr]

This belongs in area-Extensions-Configuration, it doesn't look like I can set my own labels though (or I was supposed to do it during creation and I missed it).

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

When using IConfiguration.Bind() the default TypeConverter provided from the TypeDescriptor https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L874

This proposal is to add an IDictionary<Type, TypeConverter> that would override that behavior and allow callers to specify the TypeConverter to be used during configuration binding.

Likely the best place to add that would be on BinderOptions but I don't have a strong opinion about that.

This would allow customization of the binding without the possible unintended consequences of adding a TypeConverter to the class for all cases like TypeDescriptor.AddAttributes(typeof(TimeSpan), new TypeConverterAttribute(typeof(StrictTimeSpanConverter)));

The particular case I'm trying to address comes out of an incident we had around converting to TimeSpan from string. This has this surprising (although documented) behavior:

TimeSpan.Parse("23:00:00"); // 23 hours, 0 minutes, 0 seconds
TimeSpan.Parse("25:00:00"); // 25 days, 0 hours, 0 minutes

I'd like to add a custom StrictTimeSpanConverter : TypeConverter that disallows TimeSpans of the form "25:00:00" to prevent this error from recurring but I'm wary of overriding the TypeConverter for all TimeSpan conversions.

API Proposal

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.Extensions.Configuration
{
    /// <summary>
    /// Options class used by the <see cref="ConfigurationBinder"/>.
    /// </summary>
    public class BinderOptions
    {
        /// <summary>
        /// When false (the default), the binder will only attempt to set public properties.
        /// If true, the binder will attempt to set all non read-only properties.
        /// </summary>
        public bool BindNonPublicProperties { get; set; }

        /// <summary>
        /// When false (the default), no exceptions are thrown when trying to convert a value or when a configuration
        /// key is found for which the provided model object does not have an appropriate property which matches the key's name.
        /// When true, an <see cref="System.InvalidOperationException"/> is thrown with a description
        /// of the error.
        /// </summary>
        public bool ErrorOnUnknownConfiguration { get; set; }

        ///<summary>
        /// A map of <see cref="System.Type"> to <see cref="System.ComponentModel.TypeConverter">. The specified
        /// converter will be used instead of the default converter when trying to bind properties with of a given Type.
        ///</summary>
        public IDictionary<Type, TypeConverter> TypeConverterOverrides { get; set; }
    }
}

API Usage

/// <summary>
/// Provides a type converter to convert <see cref='System.TimeSpan'/> objects to and from
/// various other representations. Disallowes timespans of the format "d:hh:mm".
/// </summary>
public class StrictTimeSpanConverter : TimeSpanConverter
{
    /// <summary>
    /// Converts the given object to a <see cref='System.TimeSpan'/>
    /// object.
    /// </summary>
    public override object? ConvertFrom(ITypeDescriptorContext? context, CultureInfo? culture, object value)
    {
        var ts = base.ConvertFrom(context, culture, value) as TimeSpan?;
        if(ts != null && ts.Value.Days > 0 && value is string text)
        {
            // Will throw if the timespan doesn't have the days field
            var strictTs = TimeSpan.ParseExact(text, @"d\.hh\:mm\:ss", culture);
            if(strictTs != ts)
            {
                throw new FormatException("Exact parse of TimeSpan including days didn't match generic parse.");
            }
        }
        return ts;
    }
}

public class TimeSpanConfig
{
     TimeSpan Time {get; set;}
}

var builder = new ConfigurationBuilder();
builder.AddJsonFile("config.json");
var config = builder.Build();
TimeSpanConfig timeSpanConfig = new();
config.Bind(timeSpanConfig, (options) => 
                            { 
                                    options.TypeConverterOverrides  = new Dictionary<Type, TypeConverter>
                                    {
                                            [typeof(TimeSpan)] = new StrictTimeSpanConverter()
                                    };
                            });

config.json
{
     "Time" : "25:00:00"
}

Alternative Designs

No response

Risks

No response

Author:	christopherbahr
Assignees:	-
Labels:	api-suggestion, untriaged, area-Extensions-Configuration
Milestone:	-

[layomia]

There's a proposal for binding customization in #83599, can you take a look? It doesn't use TypeConverter for reasons mentioned in that issue, but it should generally support your scenario.

[tarekgh]

Closing this issue and let's discuss the details on #83599. Thanks @christopherbahr for your report.

[christopherbahr]

Thanks for pointing me in the right direction. I've left a comment on the other issue.