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

Model binding fails with FormatException if dictionary key is numeric type and value is missing from POST #35594

Open
karlra opened this issue Aug 22, 2021 · 9 comments · May be fixed by #59911
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-model-binding
Milestone

Comments

@karlra
Copy link

karlra commented Aug 22, 2021

Describe the bug

In ASP.net, it is possible to POST a dictionary in a form submission in the following format:

<input type="text" name="sortOrder[319]" value="0" />
<input type="text"  name="sortOrder[320]" value="10" />

This will be bound correctly to a controller action like this:

public async Task<IActionResult> TagDetails(Dictionary<int, int> sortOrder)

However, if the sortOrder field is completely missing from the POST, such as is very likely to happen if this data comes from a database and the list to sort is empty, the controller action crashes with a validation error IF the key in the dictionary is a numeric type like int/long/float/decimal OR a field that requires parsing, like DateTime.

If we attempt to map it to a Dictionary<DateTime, string> we see the error as "FormatException: The string 'submit' was not recognized as a valid DateTime. There is an unknown word starting at index '0'.". For numbers, the error is a similar parsing error. Depending on what the form looks like the name of the field it attempts to parse ("submit") in the example above is different, so it happens regardless of whether this list is the only thing on the form or not.

It seems that ASP.net is attempting to over-parse this, because surely it should be attempting to parse only named parameters if it's a form being posted? If the type is int[] or List or similar it works fine (the parameter becomes null, as expected).

To Reproduce

Repro attached for ASP.net Core 5.

dictionary_parse_bug.zip

@karlra
Copy link
Author

karlra commented Aug 22, 2021

In order to work around this, I attempted to change the type to Dictionary<string, int> which doesn't crash, but instead it binds the entire form to the Dictionary?

image

I am using the example from https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0 (scroll down to the Dictionaries section), and it seems like that example also crashes if selectedCourses is missing from the POST.

@javiercn javiercn added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views labels Aug 23, 2021
@pranavkm pranavkm added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views labels Aug 24, 2021
@pranavkm
Copy link
Contributor

Model binding will fallback to an empty prefix if it does not find any values that matches the parameter's prefix - https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0#prefix--parameter-name. In your case, since no keys with the prefix sortOrder were present, model binding falls back to the empty prefix at which point every field is a candidate to be bound.

You can limit what prefixes are allowed to be bound by specifying it on the parameter - https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0#custom-prefix.

Also just to be certain, I would expect binding to add the format exception as a a validation error to the ModelStateDictionary in and not throw an exception in this case. Is that not the case?

@pranavkm pranavkm added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Aug 24, 2021
@karlra
Copy link
Author

karlra commented Aug 24, 2021

It doesn't even enter the controller action but crashes before.

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Aug 24, 2021
@pranavkm pranavkm added bug This issue describes a behavior which is not expected - a bug. and removed Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. labels Aug 24, 2021
@pranavkm
Copy link
Contributor

Ooh that's a good find. It's weird that format exceptions from dictionaries are rethrown:

An unhandled exception has occurred while executing the request.
      System.FormatException: Input string was not in a correct format.
         at System.Number.ThrowOverflowOrFormatException(ParsingStatus status, TypeCode type)
         at System.Number.ParseInt32(ReadOnlySpan`1 value, NumberStyles styles, NumberFormatInfo info)
         at System.Int32.Parse(String s, NumberStyles style, IFormatProvider provider)
         at System.ComponentModel.Int32Converter.FromString(String value, NumberFormatInfo formatInfo)
         at System.ComponentModel.BaseNumberConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value)
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingHelper.ConvertSimpleType(Object value, Type destinationType, CultureInfo culture)
         at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingHelper.UnwrapPossibleArrayType(Object value, Type destinationType, CultureInfo culture)
         at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingHelper.ConvertTo(Object value, Type type, CultureInfo culture)
         at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DictionaryModelBinder`2.BindModelAsync(ModelBindingContext bindingContext)
         at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value, Object container)
         at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext()
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
         at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)

FYI @dougbu in case this stacktrace rings a bell.

@dougbu
Copy link
Member

dougbu commented Aug 24, 2021

Doesn't ring a bell but we have lots of try / catch blocks in other model binders around xyz.ConvertTo(...) calls. We probably aren't testing Dictionary<{numeric type}> enough.

@rafikiassumani-msft rafikiassumani-msft added this to the Backlog milestone Sep 1, 2021
@ghost
Copy link

ghost commented Sep 1, 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.

@anibal-acosta
Copy link

anibal-acosta commented May 27, 2022

Same problem here (.net core 6.0 razor page), I have this dictionary
[BindProperty] public Dictionary<Int32, String> Relations { get; set; }

When user POST and there are no "Relations" sent the page throw exception Invalid Input format string.

So, I decide to add a hidden field and works
<input type="hidden" name="Relations[0]" value="0" />

and in the Post method I check that dictionary key is not 0 in the iteration.

but definitely is a bug, dictionary should be null when no data with that name is present in the POST

@GonziHere
Copy link

GonziHere commented Aug 4, 2022

I have the same issue. I've tried [CanBeNull] and it didn't help. Also, it took significant time to find the issue, since error is somewhat cryptic.

My workaround is this:

@if (Model.MyArray.Count == 0)
{
    <input type="hidden" asp-for="@Model.MyArray" value="null" />
}

With combination of [CanBeNull] (maybe not necessary).

@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

@captainsafia captainsafia added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 20, 2023
@captainsafia captainsafia removed the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jun 20, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
ebalders added a commit to ebalders/aspnetcore that referenced this issue Jan 17, 2025
ebalders added a commit to ebalders/aspnetcore that referenced this issue Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-model-binding
Projects
No open projects
Status: No status
Development

Successfully merging a pull request may close this issue.

9 participants