Mvc: Inconsistent property names returned from BadRequestObjectResult(ModelStateDictionary)

[dchristensen]

Given the following minimal model/controller:

public class Person {
    [Required]
    public string FirstName { get; set; }    
    [Required]
    public string LastName { get; set; }
}

[Route("api/person")]
public class PersonController : Controller {
    [HttpPost]
    public IActionResult UpdatePerson([FromBody] Person person) {
        if (!this.ModelState.IsValid) {
            return new BadRequestObjectResult(ModelState);
        }
        return Ok(person);
    }
}

Posting an invalid Person object:

POST /api/person HTTP/1.1
Host: localhost:5000
Content-Type: application/json

{ "firstName": "", "lastName": "" }

returns the following ModelState:

{
    "LastName": [ "The LastName field is required." ],
    "FirstName": [ "The FirstName field is required." ]
}

Posting a valid Personobject:

POST /api/person HTTP/1.1
Host: localhost:5000
Content-Type: application/json

{ "firstName": "John", "lastName": "Doe" }

returns the object as expected:

{ "firstName": "John", "lastName": "Doe" }

I would expect the property names in the ModelState result to match the property names returned from the serialized object. This would make it easier to map the returned errors back to the properties on my JS model that is generating the request.

I understand that BadRequestObjectResult is just serializing the model state dictionary and that I could customize the JSON serializer to prevent the property names on the object from being serialized to camel case, but it seems like since camel case is on by default in the serializer that model state property names should match.

[Eilon]

cc @rynowak @dougbu

I believe this is the intended design for the reasons you mention. There isn't a direction connection between the cases/names of the data sent over the wire to the names/paths of the properties that were bound. Value providers and model binders can do just about anything and there's no way to "go back" and know the original structure.

This is a related issue: https://github.com/aspnet/Mvc/issues/5387

[rynowak]

I think the biggest issue here is that the keys in model state will almost never match the keys in the request body. As long as that's true this will continue to be unsatisfying 👎

Validations like a foo is required message are input validations, and the messages/keys should be related to the input.

I wonder if it would be clearer for developers if we just omitted the field name.... They are already in the messages.

[frankabbruzzese]

@rynowak , is right! Input fileds will have their names in upper case, while JavaScript properties will be lower case. Now, what is more appropriate for error keys depends on how error messages are matched with inputs: If they are matched by input names then upper case is better, but if they are matched through some client side binding with an error object, then lower case is better. If developer uses uses an MVVM based framework like Angula or Aurelia or Knockout, it may use binding with an error object...It may...not it MUST. Otherwise, upper case is better, since error will be attached based on input name.

I disagree with removing keys and sending just error messages, since this will prevent developers from locating error messages next to the input in error , or to give a reddish appearence to the input in error.

The better, way would be avoiding to change names when serializing in Json...since a lot of stuffs both on the server and client side rely on name conventions. In the future one might expect further problems from this choice. However, maybe, it is not a good idea to change this setting NOW. Therefore, maybe, either a global setting and/or a further optional boolean parameter to BadRequestObjectResult might solve the problem. This way the user may choose if to camel case ModelState or not.

[dougbu]

@rynowak "messages/keys should be related to the input." may in some cases work for developers but those messages are unlikely to work for end users. The root problem here is having too many names to choose from -- display names, C# property names, HTML field names, submitted keys, keys matching the serialized model (as @dchristensen suggested), and probably a few more options I'm not thinking of. None of those work for everyone. The best we can do is usually (a) avoid showing end users the internal keys (avoid type names too) and (b) be consistent.

@frankabbruzzese it's already possible to camel-case dictionary keys using Json.NET and MVC: Choose a different NamingStrategy in the JsonSerializerSettings used. But, remapping the names won't necessarily line up with preferred name sources any better.

[frankabbruzzese]

@dougbu ,ModelState camel casing differs from dictionary keys camel casing. Dictionary keys camel casing would transform "Addrees.Street" into "address.Street" while camel case needed for ModelState dictionary should transform Addrees.Street" into "addrees.street", since dots encode nested properties.

Moreover, removing completely camel casing from JsonSerializerSettings while possible in a new project maight cause problem in an existing project where the devoler has already writte JavaScript code that relies on some names being camel case,

We need a ModelState specific camel case setting, so user, if needed, may change just that without affecting the way other propetied are serialized.

[dougbu]

may change just that without affecting the way other propertied are serialized.

That's possible too though it's not a setting e.g.
``` c#
private static readonly DefaultContractResolver SharedContractResolver = new DefaultContractResolver
{
NamingStrategy = new CamelCaseNamingStrategy
{
ProcessDictionaryKeys = true,
},
};
private static readonly JsonSerializerSettings SerializerSettings;

static MyClass()
{
SerializerSettings = JsonSerializerSettingsProvider.CreateSerializerSettings();
SerializerSettings.ContractResolver = SharedContractResolver;
}

public ActionResult Action()
{
if (!ModelState.IsValid)
{
var error = new SerializableError(ModelState);
return new JsonResult(error, SerializerSettings);
}

// ...

}
```

@dchristensen is this what you were looking for? The above will use the same keys as those used when serializing the object (were it valid). Of course, this is all JSON-specific and doesn't solve more-general issues around name selection.

If yes, we could make the above simpler though it would (like the above) also override any content negotiation. We can't make this the default, both for back-compat reasons and because a valid request does not always result in a JSON response.

[dchristensen]

@dougbu Yes, I was unaware of that option on the CamelCaseNamingStrategy. I think that should solve this for my use case, thanks for your help!