Spring Fix property binding issue for spring beans when default value is null

This PR introduced a bug an does not allow us to upgrade passed 5.3.0: https://github.com/spring-projects/spring-framework/issues/25986 . Before, AbstractNestablePropertyAccessor#processLocalProperty threw createNotWritablePropertyException when our class being deserialized did not have a setter on a property. This was then swallowed by AbstractPropertyAccessor#setPropertyValues(org.springframework.beans.PropertyValues, boolean, boolean, boolean) and binding of the next property was attempted. This is the wanted outcome. Now if this exception is not thrown (caught by the guard clause), we don't have default values on these fields so AbstractNestablePropertyAccessor#setDefaultValue throws an assertion which is not swallowed. Setting ignoreInvalid to false does cause createNotWritablePropertyException to be thrown, but it does not swallow the exception as before. This PR keeps the exact same functionality as now, but allows not having a default value for a property to be swallowed, in the same way it is in the other cases if desired. It also is a better way to handle the error and then throwing an assertion.

Comment From: pivotal-cla

@dillonm79 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Comment From: pivotal-cla

@dillonm79 Thank you for signing the Contributor License Agreement!

Comment From: mikejhill

This is a breaking issue for us. The patch seems to fix the problem. It looks like the optimization in #25986 didn't take into account cases where it would prevent a default value from being set. Although, frankly, I'm not sure how the optimization ever would have worked when non-writable property errors are suppressed:

private Object setDefaultValue(PropertyTokenHolder tokens) {
    PropertyValue pv = createDefaultPropertyValue(tokens);
    /*
     * #setPropertyValue is effectively a no-op if the property is non-writable and non-writable exceptions are
     * suppressed (suppressNotWritablePropertyException).
     *
     * Before the optimization (#25986), exceptions would bubble up here and *later* be ignored if suppressed, causing
     * execution to escape without running any remaining instructions in this method. After the optimization, remaining
     * instructions do get executed, causing a different exception to be thrown (see PR description and comments below).
     */
    setPropertyValue(tokens, pv);
    /*
     * #getPropertyValue returns the value of the property read method for the backing instance. This is null if
     * #setPropertyValue was not completed (e.g., in the case that the property is not writable) and if the field does
     * not have a pre-existing value (e.g., private Object myField = null).
     */
    Object defaultValue = getPropertyValue(tokens);
    /*
     * This is presumably attempting to assert that the default value was set properly, but the default value never gets
     * set in the above cases, so an IllegalStateException is always thrown.
     */
    Assert.state(defaultValue != null, "Default value must not be null");
    return defaultValue;
}

The only preconditions I can find to trigger this are as follows: - A property is included for binding that is non-writable (no setter) - Non-writable exceptions are suppressed on the binder (suppressNotWritablePropertyException) - The property has no default value at instantiation time (e.g., private Object myField = null with no default value provided by its getter)

I'm not familiar enough to know whether this is the right fix. In particular, I'm not certain that any of the other setDefaultValue callers are set up to properly handle null return values. But the patch definitely seems to fix the main issue for us.

Comment From: jhoeller

The setDefaultValue method is only really called within auto-growing of non-existent properties. I'm having trouble reproducing this, could you please provide a unit test that fails before and passes after the PR?

Generally speaking, I don't see a need for yet another ignoreXxx property here. If there is a specific regression to be found here, we should make it work within the existing arrangement and the with existing ignore flags.

Comment From: bclozel

Closing as we didn't get the request feedback. We can reopen this PR if we get a test that reproduces a failure with the existing codebase.