I am having a problem with custom model binders in the release candidate. I have a class decorated with the
[ModelBinder(typeof(UserProfileBinder))] attribute. The UserProfileBinder
class implements the IModelBinder interface as seen below. When i use the UserProfile class in strongly typed view via the Html.TextBox helper i get a nullreference excpetion. according to the stack trace the error occurs in the System.Web.Mvc.HtmlHelper.GetModelStateValue
method. Any ideas for work arounds
public
class UserProfileBInders :
IModelBinder{ public
object BindModel(ControllerContext controllerContext,
ModelBindingContext bindingContext){
UserProfile p =
new UserProfile();
p.FirstName = controllerContext.HttpContext.Request.Form["FirstName"];
I've seen this exception when AddModelError() is called without calling SetModelValue(), but I've never seen this when neither method is called. :( Is the sample code posted above complete? We'll continue looking into this.
Some clarification on my above post and the explanation behind the exception -
If you have a custom binder that calls AddModelError() without calling SetModelValue(), the HTML helpers will sometimes throw a NullReferenceException when you try to use them. The reason is that a call to AddModelError() without a call to SetModelValue()
doesn't really mean anything. Consider that model errors are due to the user submitting bad form data, and by using the HTML helpers you're implicitly telling us that you want them to redisplay the user's invalid submitted form data, but the binder never
told us what the bad data was! It only gave the helpers half the data necessary to render themselves properly; it told them that there was an error, but it didn't provide the expected old value.
Obviously it's not really desirable behavior for us to throw this exception. We're looking into ways to address this, but choosing an answer isn't as trivial as it seems. We could silently fall back to a blank value, we could throw a different exception,
etc., but in any case we're going to confuse and irritate developers who wrote custom binders and wonder why the users' bad values aren't being propagated between requests.
Now back to the original post -
If you're seeing a NullReferenceException even though your binder doesn't modify the ModelStateDictionary, we're definitely interested in investigating this further, since there are no known issues regarding this particular scenario at this time.
I am not calling SetModelValue. In my controller I am adding a model error in my validation ie
ViewData.ModelState.AddModelError(
"FirstName",
"Required field");
I don't want to call setmodel value from my modelbinder. My model binder handles loading the model class. Once I get to my controller i perform my validations then business logic.
I don't want to call setmodel value from my modelbinder. My model binder handles loading the model class.
Why don't you want to call it from here? This seems like the appropriate place for it. Perhaps we're just miscommunicating what the SetModelValue() method does. It simply says "this is what the user typed in for this field", nothing more. It doesn't
say whether the value is good or bad, only what the user gave us.
I've experienced this behaviour when not using SetModelValue() after I've added an error with AddModelError() and I looked through the source code to find why it throw a NullReferenceException and as you mentioned it's because the key is in the modelstatedictionary
but there is no "attempted value".
The method SetModelValue(string key, ValueProviderResult value) isn't very clear to me. It doesn't express that it's supposed to be the attempted value. The "previous" method SetAttemptedValue(string key, string attemptedValue) was much better in that sense.
My second opinion about the method SetModelValue(string key, ValueProviderResult result) is the last parameter. The ValueProviderResult is also not good at expressing how it should be used. The constructor looks like this: ValueProviderResult(object rawValue,
string attemptedValue, CultureInfo culture). I had to look through the code to find out what object rawValue should be, is it the attemptedValue but casted into an object? I'm probably missing the point with this class here. The other two parameters I can
understand. Except that when I look through the source code for MVC the attemptedValue is barely used.
Anyhow, those were my opinions. My suggestion to answering how MVC should behave when it doesn't have all the information it needs as you clarified in your previous post is to merge those two methods. Perhaps with an overload AddModelError(string key, string
errorMessage, string attemptedValue), this way there won't be a state where the helpers only will have the half of the data and cannot continue. That's just my opinion, in the meanwhile I'll just create an extension method to do it for me.
I believe the overloaded AddModelError method that you suggested existed pre-beta (or at least in some of the previews). I never really understood why they changed it. I totally agree about the ValueProviderResult, it seems like more work and more confusing
and I don't understand what benefits it provides.
I believe the overloaded AddModelError method that you suggested existed pre-beta (or at least in some of the previews). I never really understood why they changed it.
It was changed because the code which calls SetModelValue() and the code which calls AddModelError() aren't necessarily in the same method and aren't necessarily aware of one another. SetModelValue() is meant to be called by value-binding methods, while
AddModelError() is meant to be called by validation methods. The methods are meant to be callable in any order because we don't force value-binding or validation to take place in any order.
bradleylandis
I totally agree about the ValueProviderResult, it seems like more work and more confusing and I don't understand what benefits it provides.
The main benefit is that it encapsulates a CultureInfo, as values coming from the Route have to be culture-invariant and values coming from the Form have to be culture-aware. The RawValue property is what is consumed by the ConvertTo() method, while the
AttemptedValue property is what is consumed by the validation system to provide an error message. To see an example of this, break into one of your action methods and inspect the Controller.ValueProvider entries. Some of the RawValue entries will be strings
(like those from Routing), some of them will be string arrays (like those from QueryString or Form), etc. The reason for the differentiation between RawValue and AttemptedValue is that RawValue supports passing array types to the ConvertTo() method.
We can't convert to a multi-element array from a single AttemptedValue, but we
can convert to a multi-element array from a RawValue that is itself an array. This supports the infrastructure to allow binding to collection types.
I agree about the unfortunate naming, though unfortunately this isn't likely to change. :(
Thanks for the information. I understand there are cases when the extra functionality is needed, but I'd like to see a simpler overload for when I don't need it. Also, why do I have to set the culture for every single input? I can't think of a situation
where I would have 1 form with different cultures for different inputs. I certainly wouldn't think it the most common scenario.
hvossos
Member
20 Points
8 Posts
Custom ModelBinder and Release Candidate
Jan 29, 2009 01:52 AM|LINK
I am having a problem with custom model binders in the release candidate. I have a class decorated with the [ModelBinder(typeof(UserProfileBinder))] attribute. The UserProfileBinder class implements the IModelBinder interface as seen below. When i use the UserProfile class in strongly typed view via the Html.TextBox helper i get a nullreference excpetion. according to the stack trace the error occurs in the System.Web.Mvc.HtmlHelper.GetModelStateValue method. Any ideas for work arounds
public class UserProfileBInders : IModelBinder{ public object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext){
UserProfile p = new UserProfile(); p.FirstName = controllerContext.HttpContext.Request.Form["FirstName"];p.LastName = controllerContext.HttpContext.Request.Form[
"LastName"]; return p;}
}
asp.mvc modelbinder
bradleylandis
Member
233 Points
109 Posts
Re: Custom ModelBinder and Release Candidate
Jan 29, 2009 03:14 AM|LINK
I believe you need to call SetModelValue for each property.
levib
Star
7636 Points
1092 Posts
AspNetTeam
Re: Custom ModelBinder and Release Candidate
Jan 29, 2009 04:53 AM|LINK
I've seen this exception when AddModelError() is called without calling SetModelValue(), but I've never seen this when neither method is called. :( Is the sample code posted above complete? We'll continue looking into this.
Thanks!
levib
Star
7636 Points
1092 Posts
AspNetTeam
Re: Custom ModelBinder and Release Candidate
Jan 29, 2009 06:44 AM|LINK
Some clarification on my above post and the explanation behind the exception -
If you have a custom binder that calls AddModelError() without calling SetModelValue(), the HTML helpers will sometimes throw a NullReferenceException when you try to use them. The reason is that a call to AddModelError() without a call to SetModelValue() doesn't really mean anything. Consider that model errors are due to the user submitting bad form data, and by using the HTML helpers you're implicitly telling us that you want them to redisplay the user's invalid submitted form data, but the binder never told us what the bad data was! It only gave the helpers half the data necessary to render themselves properly; it told them that there was an error, but it didn't provide the expected old value.
Obviously it's not really desirable behavior for us to throw this exception. We're looking into ways to address this, but choosing an answer isn't as trivial as it seems. We could silently fall back to a blank value, we could throw a different exception, etc., but in any case we're going to confuse and irritate developers who wrote custom binders and wonder why the users' bad values aren't being propagated between requests.
Now back to the original post -
If you're seeing a NullReferenceException even though your binder doesn't modify the ModelStateDictionary, we're definitely interested in investigating this further, since there are no known issues regarding this particular scenario at this time.
hvossos
Member
20 Points
8 Posts
Re: Custom ModelBinder and Release Candidate
Jan 29, 2009 02:17 PM|LINK
ViewData.ModelState.AddModelError(
"FirstName", "Required field");I don't want to call setmodel value from my modelbinder. My model binder handles loading the model class. Once I get to my controller i perform my validations then business logic.
levib
Star
7636 Points
1092 Posts
AspNetTeam
Re: Custom ModelBinder and Release Candidate
Jan 29, 2009 06:13 PM|LINK
Why don't you want to call it from here? This seems like the appropriate place for it. Perhaps we're just miscommunicating what the SetModelValue() method does. It simply says "this is what the user typed in for this field", nothing more. It doesn't say whether the value is good or bad, only what the user gave us.
bjn
Member
4 Points
2 Posts
Re: Custom ModelBinder and Release Candidate
Feb 01, 2009 04:36 PM|LINK
I've experienced this behaviour when not using SetModelValue() after I've added an error with AddModelError() and I looked through the source code to find why it throw a NullReferenceException and as you mentioned it's because the key is in the modelstatedictionary but there is no "attempted value".
The method SetModelValue(string key, ValueProviderResult value) isn't very clear to me. It doesn't express that it's supposed to be the attempted value. The "previous" method SetAttemptedValue(string key, string attemptedValue) was much better in that sense. My second opinion about the method SetModelValue(string key, ValueProviderResult result) is the last parameter. The ValueProviderResult is also not good at expressing how it should be used. The constructor looks like this: ValueProviderResult(object rawValue, string attemptedValue, CultureInfo culture). I had to look through the code to find out what object rawValue should be, is it the attemptedValue but casted into an object? I'm probably missing the point with this class here. The other two parameters I can understand. Except that when I look through the source code for MVC the attemptedValue is barely used.
Anyhow, those were my opinions. My suggestion to answering how MVC should behave when it doesn't have all the information it needs as you clarified in your previous post is to merge those two methods. Perhaps with an overload AddModelError(string key, string errorMessage, string attemptedValue), this way there won't be a state where the helpers only will have the half of the data and cannot continue. That's just my opinion, in the meanwhile I'll just create an extension method to do it for me.
bradleylandis
Member
233 Points
109 Posts
Re: Custom ModelBinder and Release Candidate
Feb 01, 2009 11:18 PM|LINK
bjn,
I believe the overloaded AddModelError method that you suggested existed pre-beta (or at least in some of the previews). I never really understood why they changed it. I totally agree about the ValueProviderResult, it seems like more work and more confusing and I don't understand what benefits it provides.
levib
Star
7636 Points
1092 Posts
AspNetTeam
Re: Custom ModelBinder and Release Candidate
Feb 01, 2009 11:45 PM|LINK
It was changed because the code which calls SetModelValue() and the code which calls AddModelError() aren't necessarily in the same method and aren't necessarily aware of one another. SetModelValue() is meant to be called by value-binding methods, while AddModelError() is meant to be called by validation methods. The methods are meant to be callable in any order because we don't force value-binding or validation to take place in any order.
The main benefit is that it encapsulates a CultureInfo, as values coming from the Route have to be culture-invariant and values coming from the Form have to be culture-aware. The RawValue property is what is consumed by the ConvertTo() method, while the AttemptedValue property is what is consumed by the validation system to provide an error message. To see an example of this, break into one of your action methods and inspect the Controller.ValueProvider entries. Some of the RawValue entries will be strings (like those from Routing), some of them will be string arrays (like those from QueryString or Form), etc. The reason for the differentiation between RawValue and AttemptedValue is that RawValue supports passing array types to the ConvertTo() method. We can't convert to a multi-element array from a single AttemptedValue, but we can convert to a multi-element array from a RawValue that is itself an array. This supports the infrastructure to allow binding to collection types.
I agree about the unfortunate naming, though unfortunately this isn't likely to change. :(
bradleylandis
Member
233 Points
109 Posts
Re: Custom ModelBinder and Release Candidate
Feb 02, 2009 12:52 AM|LINK
levib,
Thanks for the information. I understand there are cases when the extra functionality is needed, but I'd like to see a simpler overload for when I don't need it. Also, why do I have to set the culture for every single input? I can't think of a situation where I would have 1 form with different cultures for different inputs. I certainly wouldn't think it the most common scenario.