Different error codes on same formatter registered in different ways [SPR-14345]

[spring-projects-issues]

Ruslan Stelmachenko opened SPR-14345 and commented

Using Spring Web MVC.

When new org.springframework.format.Formatter registered using org.springframework.core.convert.ConversionService.addFormatter, then when conversion fails because of parse error (in Formatter.parse method), then catched exception in the end turns into org.springframework.beans.TypeMismatchException.

When the same formatter registered in controller's @InitBinder method using org.springframework.web.bind.WebDataBinder.addCustomFormatter, then when conversion fails because of parse error (in Formatter.parse method), then catched exception in the end turns into org.springframework.beans.MethodInvocationException.

This difference leads to new problem: when you use Spring's i18n feature (messages.properties) to localize error messages, then message code is different depending on how formatter was registered.

In first case it is: typeMismatch.*.
In second case it is: methodInvocation.*.

I'm not sure this is the intended behavior
It is very strange and inconvinient, that the same formatter throws different error message depending on the way in which it was registered.

---

Affects: 4.2.6

Attachments:

• spr-14345.zip (71.26 kB)

Issue Links:

• TypeConverterSupport does not propagate custom exceptions anymore [SPR-14661] #19225 TypeConverterSupport does not propagate custom exceptions anymore

Referenced from: commits 367e663, f9e8924, c6f63bd, d51c22a

Backported to: 4.2.7

[spring-projects-issues]

Juergen Hoeller commented

I can't reproduce this: With either variant, I consistently get a TypeMismatchException if the actual formatter invocation failed, whereas a MethodInvocationException only happens if the target setter is being invoked with a converted value but fails to execute properly.

Could you please double-check your scenario there and provide a corresponding unit test?

[spring-projects-issues]

Ruslan Stelmachenko commented

I've attached the mini-project to reproduce this.

Run it with gradlew bootRun and enter http://localhost:8080?ym=123 in the browser.

You will see It is MethodInvocation Error message.

Uncomment one of the lines in demo.controller.HomeController.initBinder method to test different ways to register YearMonthFormatter formatter.

When formatter is registered using ConversionService, you will see It is TypeMismatch Error message.

Also note, that if you enter just http://localhost:8080 (without ym parameter), then in case when formatter registered using WebDataBinder, it causes NullPointerException! It is no case, when formatter registered using ConversionService.

I thought Formatter.print method guarantees, that object will not be null.

[spring-projects-issues]

Ruslan Stelmachenko commented

I also found #12429 and see now that registering custom formatter using DataBinder.addCustomFormatter(Formatter formatter) is implemented using bridge (FormatterPropertyEditorAdapter) between formatters and propertyEditors... I think it's the core of the problem!

It's actualy not register formatter. It's register PropertyEditor (using bridge) when I register it using this method!

This is the reson why behaviour is different.

For instanse, when ConversionService invokes the formatter, it doing it using ConversionUtils.invokeConverter, which catches all exceptions (catch (Exception ex)) and throwing ConversionFailedException:

public static Object invokeConverter(GenericConverter converter, Object source, TypeDescriptor sourceType,
          TypeDescriptor targetType) {
     try {
          return converter.convert(source, sourceType, targetType);
     }
     catch (ConversionFailedException ex) {
          throw ex;
     }
     catch (Exception ex) {
          throw new ConversionFailedException(sourceType, targetType, source, ex);
     }
}

The GenericConversionService also handles nulls in it's convert method. So converter is not invoked at all if the source is null.

But FormatterPropertyEditorAdapter in contrast just calling formatter.parse catching only java.text.ParseException, which will never happen if formatter's parse method uses other parsing tecniques, for example java.time family methods. Also it doesn't handle case when source object is null.

@Override
public void setAsText(String text) throws IllegalArgumentException {
     try {
          setValue(this.formatter.parse(text, LocaleContextHolder.getLocale()));
     }
     catch (ParseException ex) {
          throw new IllegalArgumentException("Parse attempt failed for value [" + text + "]", ex);
     }
}

Maybe this bridge (FormatterPropertyEditorAdapter) should be smarter and mimic ConversionService logic when it registers new formatters?

[spring-projects-issues]

Ruslan Stelmachenko commented

So I think it should be something like this (simplified):

@Override
public void setAsText(String text) throws IllegalArgumentException {
     if (text == null) {
          setValue(null);
     }
     else {
          try {
               setValue(this.formatter.parse(text, LocaleContextHolder.getLocale()));
          }
          catch (Exception ex) {
               throw new ConversionFailedException(sourceType, targetType, source, ex);
          }
     }
}

Or maybe using ConversionUtils. You know better. :)

[spring-projects-issues]

Juergen Hoeller commented

Thanks for the analysis! It's indeed custom Formatter implementations throwing RuntimeExceptions where the mismatch comes in. I'll revise that bridging for 4.3 GA and will also backport it to 4.2.7.

[spring-projects-issues]

Juergen Hoeller commented

Since PropertyEditors are not supposed to throw ConversionExceptions in terms of their API contract, the better option seems to be more defensiveness in our PropertyEditor invocation infrastructure: i.e. catching RuntimeException (even Throwable) instead of just IllegalArgumentException and consistently turning those into TypeMismatchException at that level.

Point taken about null handling: FormatterPropertyEditorAdapter has the same checks as FormattingConversionService there now.

This will be available in the upcoming 4.3.0.BUILD-SNAPSHOT. Please give it a try if you have the chance...

[spring-projects-issues]

Ruslan Stelmachenko commented

I'm tried 4.3.0.BUILD-SNAPSHOT and I could say, it works now!
The NPE is gone. The error is typeMismatch.

And I agree with the arguments about the API contract.

Thanks for the fixes!

—

But I should say, this will slightly change the behaviour of real property editors. For example:

binder.registerCustomEditor(YearMonth.class, new PropertyEditorSupport() {
     @Override
     public void setAsText(String text) throws IllegalArgumentException {
          if (text == null) {
                   setValue(null);
          } else {
                   setValue(YearMonth.parse(text));
          }
     }
     @Override
     public String getAsText() {
          if (getValue() == null) {
                   return null;
          }
          return getValue().toString();
     }
});

After fix, it will cause typeMismatch error. While previously it throws methodInvocation.

I think this is "bad" property editor, because it violates setAsText contract, which says:

May raise java.lang.IllegalArgumentException if either the String is badly formatted or if this kind of property can't be expressed as text.
But the contract say nothing about other exceptions...
I can't find any documentation about which exception is "right" here though. So, I'm not sure that it is important.

Maybe it is better if adapter will catch Throwable and throw IllegalArgumentException. This way other propery editors (registered in binder as propery editors, not as formatters) will not change it's behaviour after this fix.

Of course it is only if the change of native ProperyEditors behavior is not intended.

[spring-projects-issues]

Juergen Hoeller commented

From my perspective, that's ok: Exceptions coming out of PropertyEditors should always lead to a type mismatch error. A method invocation error code is meant to indicate an exception coming out of a target setter method, after all.