Gabriele Del Prete opened SPR-8321 and commented

ParserConverter will skip calling the parse() method of a Parser\ if the to-be-converted field is empty, and will return null as the parsing result.

(You can see this at the very beginning of the convert() method in ParserConverter).

This mean that you get two different behaviors when receiving data from fields in a form, depending on whether you use or don't use parsers.

1) Suppose you have a String property on your command bean, and a matching input field on the view. Then:

  • if the user inputs any non empty string, the form bean property will be set to that string;
  • if the user inputs an empty string, the form bean property will be set to the empty string.

So far, so good, this is the expected behavior.

2) Now suppose you annotate the String property on your command bean with a Formatter\ annotation with its parse() method implemented.

Then: - if the user inputs any non empty string, the parse() method will get correctly called, and will return whatever is its parsing result. - if the user inputs an empty string, the parse() method will never get called, and moreover the form bean property will be set to null.

This is wrong in my opinion because:

1) Spring is not allowing a Parser to handle empty strings on input, however one might very well want to consider empty strings as lecit value when parsing.

2) from the application point-of-view, using a Parser may result in having your command bean properties set to null... ironically enough, something you should never get as Parsers and Printers supposedly are not allowed to handle null values.

The proposed fix is, simply allow empty string to reach the Parser's parse() method, so that it can eventually handle it.

I'm attaching a sample webapp which shows the problem; the webapp shows a form with two String fields, one not annotated with a Formatter, and the other annotated with a simple @Trim formatters which trims strings when parsing and is a no-op when printing.

The form is pre-populated with non empty string; if you submit it, you can see both the Parser and the Printer part of the Formatter getting called on the annotated property (watch the log for this). If you otherwise submit the form with empty fields, you'll see the Parser is never called, and the annotated property is set to null (again, see the log as on the resulting webpage nulls are converted to the empty string).

Affects: 3.0.5

Reference URL: http://forum.springsource.org/showthread.php?101583-Why-does-the-Formatter-system-ignore-null-values

Attachments: - formatters-test.tar.gz (7.50 kB)

Issue Links: - #12973 Formatter subsystem: allow Parser to return null as the parsed object; allow Printers to accept null as the object to be formatted - #12201 Consider mapping empty form values to [null] for Object properties - #12038 Formatters bind empty form fields to [null] property values while DataBinder by default binds empty string

Comment From: spring-projects-issues

Gabriele Del Prete commented

Note: in the original forum thread I was requesting also to allow Parser and Printers to handle null values. I'm opening an enhancement request for this other issue now.

Comment From: spring-projects-issues

Keith Donald commented

A few notes/questions on this:

Such a change is not backwards compatible. As an example, the existing Parsers (e.g. Date/Number) do not have special handling for "" values, and would have to be modified to deal with this case (as would all other Formatter and/or Parser implementations).

The goal of the present behavior was to relieve the Parser implementor from having to deal with "empty field values" -- where an "empty field value" is defined as either [null] or "". Given the current spec, an "empty field value" is always parsed to [null] for you, and bound to the corresponding model property as [null]--it was expected this would be a welcome responsibility handled by the framework.

Unfortunately, I acknowledge this is not consistent with Spring MVC's default DataBinder behavior, which always maps "empty field values" to "" for String properties. I suspect this may be where the confusion is stemming from here. Apart from this DataBinder inconsistency, which leads to confusing model binding differences based on whether a String field has a Parser associated with it or not, what other issues do you have with the current behavior? Put another way, lets say that Spring MVC's binder always mapped "empty string values" to null model property values consistently. Would you still have concerns? This seems like a useful behavior to me, and one that simplifies Parser implementation.

Comment From: spring-projects-issues

Gabriele Del Prete commented

Hello,

I have no issues right now with the current behavior, that is, I simply rewrote my app taking this inconsistency in consideration when we started using Formatters.

But I believe the Parser contract should still be modified to match the DataBinder one because:

  • inconsistencies like this make Spring harder to learn and use; it's kind of a violation of the principle of least surprise IMHO;
  • empty fields should really always be mapped to "" and not [null];
  • I don't see it as a useful behavior which simplify Parser implementation; I rather see it as a limitation in what Parsers are allowed to do.

For the backward compatibility issue: of course the Spring Team must decide on it. That said, I could bet that many existing Parsers implementation would not even need a change in their code, as "" would simply be another illegal input which would make them generate a IllegalArgumentException or ParseException, which in turn would result in the property being se to [null].

Also, where in the reference documentation (v. 3.0) is the promise for Parser never being passed the empty string in ? I can only see a promise for Converters not being passed [null] in the source object.

Regards, Gabriele

Comment From: spring-projects-issues

Gabriele Del Prete commented

Addendum: please note that in the 3.0 reference documentation, the example DateFormatter class shown handles "" as an input by itself in parse() method. You may want to update the documentation in case you choose to keep the current behavior.

Comment From: spring-projects-issues

Keith Donald commented

Hey,

I agree the inconsistency is confusing, you'll get no argument from me on that.

Ignoring that for the moment, why do you think empty String fields should really always be mapped to "" and not [null]? I find it easier to manage when empty values for all fields all consistency map to [null]. For example, if I bind an empty value to an Integer field, I must set the model property value to [null]. Why should I not do this for String fields as well? If I don't, and I am using JSR303, I end up having to use @Size(min=1) on String fields and @NotNull on the others to mean the same thing--that the field is required (or I have to rely on a proprietary extension like Hibernate Validator's @NotEmpty). If all empty fields were bound as [null], I could simply use @NotNull to mean "this field is required".

Can you be a specific about what you would like to do that you cannot do due to this "limitation"? I'm assuming one thing you might want to do is "set a default value when the user-entered field value is empty". However, I don't think that's a responsibility of a Formatter. I think that's a responsibility of the model, and the right place to do that is in the property setter of the model itself when the parsed value is null. What other things do you want to be able to do that you cannot? I need to consider the specifics carefully and weigh them before deciding on any change in the present behavior.

A note on DataBinder behavior: if there is a parse error, the invalid value is never bound to the backing model property. The DataBinder holds the invalid value for the field as an intermediary, and uses that value when re-rendering the form so the user can revise what he or she entered. This would surprise developers of existing applications as the error code for empty field values would change from 'required' to 'typeMismatch', which would likely change the error message shown to end-users. Also, if the field was not required, the user would now see an error for an empty field where they didn't before.

Comment From: spring-projects-issues

Keith Donald commented

Discussion on this same issue in the ASP.NET MVC community: http://forums.asp.net/t/1465586.aspx http://forums.asp.net/t/1522119.aspx Interestingly, with ASP.NET MVC 2, the default behavior was changed to bind empty field values as null.

Comment From: spring-projects-issues

Gabriele Del Prete commented

Here are my thoughts:

1) in my mind, a Integer property gets mapped to null when its field is empty because a empty value is not a number (thus [null] really means "no value/not available/can't be decided"), while an empty string is actually a perfectly legal value for a string (though sometimes a value you don't want to allow), so it could be mapped as-is. I admit this is very personal.

2) Please note that in JSR-303, @NotNull means "property should never be set to [null]" (literally) not "property is required". In JSR-303 the official way to mark a String property as required is annotating it with BOTH @NotNull AND @Size(min=1); either @NotNull or @Size(min=1) is not enough. IIRC this is a requirement for having JSR-303 match DBs (when used in ORMs), otherwise you'd get either a non-nullable column with (potentially) empty string values in it, or a nullable column with only length>0 strings in it.

3) about the "set a default value when the user-entered field value is empty" idea: you're definitely right it's not a Formatters' duty though. However, it could really help in at least two cases: the model is shared across different apps, so you don't want to/cannot pollute it; model classes are ORM entities and you want to have trivial setters and getters to avoid complications with your persistence provider.

4) I don't understand the problem with DataBinder: a Parser right now forcibly maps "" to [null], why allowing it to handle the mapping of "" by itself should change anything in DataBinder? DataBinder will simply work on what Parser returned.

Hope this helps.

Bye, Gabriele

Comment From: spring-projects-issues

Keith Donald commented

Re 4. The problem is a compatibility issue. Existing formatters will throw exceptions if we start passing them "" or null. These exceptions will get translated to type mismatch failures, which is different than a required failure or no failure at all. The developer would have no choice but to change all their formatters.

Comment From: spring-projects-issues

Julian Bell commented

Hi, my feedback is related to null values not being picked up by a formatter. My formatter is as follows:

public class KeywordSetFormatter implements Formatter> { @Override public String print(Set\ keywords, Locale locale) { // snip code }

@Override
public Set<Keyword> parse(String keywordString, Locale locale) throws ParseException {
    Set<Keyword> keywords = new HashSet<Keyword>();
    // snip code
    return keywords;
}

}

What I wanted was if the parse method had a null value passed in it would return an empty set - however, the parse method isn't called at all. I coded around this, but it would have been nice. Thanks.

Comment From: spring-projects-issues

Gabriele Del Prete commented

@Keith: I know it's not very clean, but what about introducing a tag interface EmptyStringAwareParser ? If the Formatter implements it, it will get passed in empty strings, if not, you skip the invocation.

@Julian Bell: when does you get null in input to a Parser? I thought you could never get [null]s on input.

Comment From: spring-projects-issues

Julian Bell commented

//@Julian Bell: when does you get null in input to a Parser? I thought you could never get [null]s on input.

Yes - an empty string/null value never called the parse method, but it would have been nice if the formatter was called in my example. For the old property editors you could convert an empty string to another object to make things simpler down the track. Anyway - I'm just letting you know my preference - I definitely appreciate formatters.

Comment From: spring-projects-issues

Keith Donald commented

Current behavior is as-designed and preferred in many cases (though I acknowledge there are different opinions on the subject). Changing current behavior would introduce break compatibility. For specific new feature ideas in this area, such as special Formatter marker interfaces, lets create separate JIRAs for those.