aspnet/Mvc

Can we prune conversion code in ModelBindingHelper

rynowak opened this issue · 10 comments

rynowak commented

These ConvertTo methods in ModelBindingHelper aren't used by model binding anymore and they are likely way too general for the use cases we have in HTML helpers. Can we get rid of it and make HTML helpers using something that only supports the needed scenarios?

dougbu commented

FYI the "needed scenarios" in HTML helpers include pretty much any conversion imaginable. The issue is that that conversion source is not just ModelStateDictionary but also ViewDataDictionary, where users can stick anything they like.

Eilon commented

Looks like we should take a hatchet to GetModelStateValue because its conversion methods are far too powerful (in a bad way) given where they are used. The few call sites that remain should just use TypeConverters directly.

dougbu commented

The few call sites that remain should just use TypeConverters directly.

What will improve as I duplicate the type conversion code across the existing call sites?

Eilon commented

I think the idea is that we would be deleting ~200 lines of bad code and instead we could have a simple helper method that has like 10 lines of code and was called from a few places. The code in ModelBindingHelper is way too complicated for the few things we actually need it to do.

dougbu commented

/cc @Eilon @danroth27 not sure this is urgent enough to do in 1.0.0. ModelBindingHelper is now in Microsoft.AspNetCore.Mvc.ModelBinding.Internal.

Eilon commented

Moved.

dougbu commented

not sure this is urgent enough to do in 1.0.0

Unfortunately I was incorrect in that assessment and we're now locked into at least supporting string and string[] conversions to arbitrary types. The ConvertTo() overloads w/ a CultureInfo parameter are exposed through ValueProviderResultExtensions and that's public.

More generally, the ConvertTo() overloads are used or indirectly exposed for only a few conversions. Note there's no any-to-any conversion requirement.

  1. Conversions to string in DefaultHtmlGenerator (through GetModelStateValue()).
  2. Conversions from string in DictionaryModelBinder and ValueProviderResultExtensions.
  3. One conversion to string[] in DefaultHtmlGenerator.
  4. Conversions from string[] in ValueProviderResultExtensions.
  5. One conversion to bool in DefaultHtmlGenerator.

My leaning is toward removing unused ConvertTo() overloads but otherwise leaving them alone. Could go further in two ways:

  1. Split ConvertTo() up to cover the cases above individually. This could involve covering the cases mostly in private (well, internal for testing) methods because only the conversion from string case is truly shared. Would definitely involve splitting up DefaultHtmlGenerator.GetModelStateValue() into three methods.
  2. Mark the general conversion methods in ValueProviderResultExtensions as [Obsolete]. This would narrow our public API around arbitrary conversions to binary compatibility cases. Some time later we could remove it entirely.
dougbu commented

Removed unused ConvertTo() methods in 52e4ca7