AttemptedValueIsInvalidAccessor on nullable types results in "Nullable`1." message

[ashleydw]

Given the following where MyEnum is an optional value, where the default is null:

public async Task<IEnumerable<object>> DoSomethingAsync(MyEnum? myE)
{
// do something
// e.g. if(myE == null) {}  else {}
}

public enum MyEnum
{
   MyValOne = 0,
   MyValTwo = 1
}

And providing the query string with an invalid value (e.g. ?myE=asd) the request will fail via the ModelBindingMessageProvider and AttemptedValueIsInvalidAccessor; where the message is The value '{0}' is not valid for {1}..

However, with a nullable type, the {1} results in the message:

`The value 'asd' is not valid for Nullable`1.

I see a work around using localisation on the message, but should the message not attempt to find the underlying Type rather than the nullable?

Is this loosely related to #2985?

[rynowak]

/cc @dougbu @Eilon should we be leaking the implementation type here at all? - should this name the key that we're binding (The value 'asd' is not valid for myE.) instead?

[dougbu]

IIRC we set this message up to mimic a similar message in MVC 5.x. But, I agree the message only helps users fix their submission if the value "asd" is unique on the form. We shouldn't be prioritizing site developers here.

Might want to expand on this and look at other errors we add to ModelState for GetType() calls.

[Eilon]

Yeah I think the property display name (or parameter display name in this case) should be used in the error message. The type names shouldn't leak out. Don't care what previous versions of MVC did.

So the question is, do we have this metadata available at the point the error is created?

@dougbu can you take a look?

[dougbu]

@Eilon / @rynowak we shouldn't change the error to The value 'asd' is not valid for myE. because that leaks deeper internals of the application than the type name -- the action's parameter name. If the metadata included a display name, it would be used here automatically. If the metadata was for a property and no display name was defined, the property name would be used here automatically.

It's only in the specific case of a parameter that the error mentions a type name and there's nothing better to use in that case. (Same thing probably happens for elements of a collection property but, again, there's nothing better to use then.)

However, the error could mention MyEnum rather than Nullable`1. We have the information and could update either GetDisplayName() or the code adding the error to use UnderlyingOrModelType.Name and not ModelType.Name as the final fallback.

If we go this way, I lean toward creating a new ModelMetadata.GetDisplayNameOrUnderlyingTypeName() and using it most of the places MVC currently calls GetDisplayName(): Updating GetDisplayName() could break our users and we add lots of errors (e.g. for failed [Compare] validations) that could also do with a clearer error message in this corner case.

[dougbu]

Of course I meant ModelMetadata.GetDisplayNamePropertyNameOrUnderlyingTypeName() 😈

[rynowak]

Can we use the key (model-name)?

[dougbu]

Since the original report was about a conversion error and the browser must have submitted something with the ModelState key, that might work in this case.

That said, ModelState keys are usually not visible to users; they appear only in the HTML source. And, I was making a suggestion for the general case -- where the key could be the empty string.

That that said, the even-more general (still corner) cases include a top-level array or generic type. My suggestion to use UnderlyingOrModelType still leaves us with a almost-gibberish error message.

@Eilon thoughts?

[Eilon]

Per discussion with @dougbu :

1. I think that end-user-visible error messages should never have the type name in them. Showing display names is great, and showing property names is acceptable.
2. If no display-name/property-name is available, we should have a fallback message format that omits the for {typename} part of the message.

[dougbu]

Experimented a bit and found a fair number of cases where error messages in ModelState may contain type names if (for all but one case) the type is used for a top-level model (parameter) or element of a collection and

• a FormatException or OverflowException occurs
  • involves ModelBindingMessageProvider.AttemptedValueIsInvalidAccessor or ModelBindingMessageProvider.UnknownValueIsInvalidAccessor (if the error is hit before an AttemptedValue is recorded)
  • the original server-side case
• a [CustomValidation] applied to the type fails
  • involves the ValidationContext passed in
  • server-side
• the type implements IValidatableObject and fails its validation
  • involves the ValidationContext passed in
  • broader than all other cases because ValidationContext.MemberName isn't set i.e. problem impacts properties too
  • server-side
• the type is float, double, decimal, or Nullable<T> (where T is one of the previous types) and the user enters a non-numeric value in the <input/>
  • involves ModelBindingMessageProvider.ValueMustBeANumberAccessor
  • client-side
• the type's ModelMetadata.IsRequired is true and the user does not enter a value in the <input/>
  • involves the ValidationContext passed in
  • could override the error message (to ignore the context's DisplayName) when creating our default RequiredAttribute for a type
  • occurs for most simple types though not Nullable<T>
  • client-side

[dougbu]

a90f411 addresses the original reported problem and float, decimal, etc. handling. Also improves the handling of IValidatableObject properties (gets rid of the bold text above). The remaining points (basically the ValidationContext.DisplayName fallback and our error message for ModelMetadata.IsRequired) are unreported corner cases.