Improve diagnostics for configuration properties binding failures

[wilkinsona]

When configuration properties binding fails (for example due to an unknown field or a value that cannot be coerced), the error that you're greeted with isn't very friendly:

• it doesn't point to the source of the property (application.properties, System properties, environment variable, etc) that caused the problem.
• due to the use of prefixes, it's hard to figure out the full name of the property that's causing the problem
• There are many nested exceptions that are logged multiple times

In order of importance, I'd like to see the following:

• Display the full name of the property that could not be bound
• Display the source of the value that could not be bound
• Clean up the exception logging to improve the signal to noise ratio

[sbrannen]

👍

[sbrannen]

• Display the full name of the property that could not be bound
• Display the source of the value that could not be bound

I'd argue that those two deliverables are "must haves".

[wilkinsona]

A proposal for this is available here /cc @philwebb

[wilkinsona]

The key part to the proposal is a custom BeanWrapper that overrides setPropertyValue to provide the improved diagnostics. Plugging that in isn't as elegant as it could be, and there are some issues with how I've done it at the moment (you'll get a different property accessor calling getPropertyAccessor() vs getInternalBindingResult().getPropertyAccessor()).

It would be quite a bit nicer if DataBinder provided an extension point so that subclasses could provide their own AbstractPropertyBindingResult from which a custom BeanWrapper can then be provided.

Something like this in DataBinder:

    public void initBeanPropertyAccess() {
        Assert.state(
                this.bindingResult == null,
                "DataBinder is already initialized - call initBeanPropertyAccess before other configuration methods");
        this.bindingResult = createBeanPropertyBindingResult();
    }

    protected AbstractPropertyBindingResult createBeanPropertyBindingResult() {
        BeanPropertyBindingResult result = new BeanPropertyBindingResult(getTarget(),
                getObjectName(), isAutoGrowNestedPaths(), getAutoGrowCollectionLimit());
        if (this.conversionService != null) {
            this.bindingResult.initConversion(this.conversionService);
        }
        return result;
    }

I could then override this in RelaxedDataBinder:

    @Override
    protected AbstractPropertyBindingResult createBeanPropertyBindingResult() {
        return new BeanPropertyBindingResult(getTarget(), getObjectName(),
                isAutoGrowNestedPaths(), getAutoGrowCollectionLimit()) {
            @Override
            protected BeanWrapper createBeanWrapper() {
                BeanWrapper beanWrapper = new BeanWrapperImpl(getTarget()) {
                    @Override
                    public void setPropertyValue(PropertyValue pv) throws BeansException {
                        try {
                            super.setPropertyValue(pv);
                        }
                        catch (NotWritablePropertyException ex) {
                            PropertyOrigin origin = findPropertyOrigin(pv);
                            if (origin != null) {
                                throw new RelaxedBindingNotWritablePropertyException(ex,
                                        origin.getName(), origin.getSource());
                            }
                            throw ex;
                        }
                    }
                };
                beanWrapper.setConversionService(new RelaxedConversionService(
                        getConversionService()));
                beanWrapper.registerCustomEditor(InetAddress.class,
                        new InetAddressEditor());
                return beanWrapper;
            }
        };
    }

@snicoll is on the case

[sbrannen]

@wilkinsona & @snicoll, thanks for taking the initiative to get this sorted out so quickly! 😄

[snicoll]

The extension point has been added.