diff --git a/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java b/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java index 29996a6c912..803be899e00 100644 --- a/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java @@ -23,7 +23,6 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Modifier; import java.lang.reflect.UndeclaredThrowableException; import java.security.PrivilegedActionException; -import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.Iterator; @@ -42,7 +41,6 @@ import org.springframework.core.convert.ConverterNotFoundException; import org.springframework.core.convert.TypeDescriptor; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; -import org.springframework.util.StringUtils; /** * A basic {@link ConfigurablePropertyAccessor} that provides the necessary @@ -60,6 +58,7 @@ import org.springframework.util.StringUtils; * @author Rod Johnson * @author Rob Harrop * @author Sam Brannen + * @author Brian Clozel * @since 4.2 * @see #registerCustomEditor * @see #setPropertyValues @@ -82,11 +81,8 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA @Nullable Object rootObject; - /** Map with cached nested Accessors: nested path -> Accessor instance. */ - private @Nullable Map nestedPropertyAccessors; - - /** The number of nested properties traversed to reach the wrapped object. */ - private int nestedPathDepth; + /** Map with cached nested Accessors: path segment -> Accessor instance. */ + private @Nullable Map nestedPropertyAccessors; /** @@ -180,7 +176,6 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA this.nestedPath = (nestedPath != null ? nestedPath : ""); this.rootObject = (!this.nestedPath.isEmpty() ? rootObject : this.wrappedObject); this.nestedPropertyAccessors = null; - this.nestedPathDepth = 0; this.typeConverterDelegate = new TypeConverterDelegate(this, this.wrappedObject); } @@ -219,61 +214,70 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA @Override public void setPropertyValue(String propertyName, @Nullable Object value) throws BeansException { - AbstractNestablePropertyAccessor nestedPa; + ResolvedProperty resolved; try { - nestedPa = getPropertyAccessorForPropertyPath(propertyName); + resolved = resolvePropertyPath(propertyName); + } + catch (InvalidPropertyPathException ex) { + // A malformed path is a syntax error, distinct from a syntactically + // valid path whose intermediate segment genuinely does not exist + // (caught below and reported as "not writable" instead). + throw new InvalidPropertyPathException(getRootInstance(), this.nestedPath + propertyName, value, ex); } catch (NotReadablePropertyException ex) { throw new NotWritablePropertyException(getRootClass(), this.nestedPath + propertyName, "Nested property in path '" + propertyName + "' does not exist", ex); } - PropertyTokenHolder tokens = getPropertyNameTokens(getFinalPath(nestedPa, propertyName)); - nestedPa.setPropertyValue(tokens, new PropertyValue(propertyName, value)); + resolved.accessor().setPropertyValue(resolved.segment(), new PropertyValue(propertyName, value)); } @Override public void setPropertyValue(PropertyValue pv) throws BeansException { - PropertyTokenHolder tokens = (PropertyTokenHolder) pv.resolvedTokens; - if (tokens == null) { + PropertyPath.Segment segment = (PropertyPath.Segment) pv.resolvedTokens; + if (segment == null) { String propertyName = pv.getName(); - AbstractNestablePropertyAccessor nestedPa; + ResolvedProperty resolved; try { - nestedPa = getPropertyAccessorForPropertyPath(propertyName); + resolved = resolvePropertyPath(propertyName); + } + catch (InvalidPropertyPathException ex) { + throw new InvalidPropertyPathException(getRootInstance(), this.nestedPath + propertyName, pv.getValue(), ex); } catch (NotReadablePropertyException ex) { throw new NotWritablePropertyException(getRootClass(), this.nestedPath + propertyName, "Nested property in path '" + propertyName + "' does not exist", ex); } - tokens = getPropertyNameTokens(getFinalPath(nestedPa, propertyName)); - if (nestedPa == this) { - pv.getOriginalPropertyValue().resolvedTokens = tokens; + segment = resolved.segment(); + if (resolved.accessor() == this) { + pv.getOriginalPropertyValue().resolvedTokens = segment; } - nestedPa.setPropertyValue(tokens, pv); + resolved.accessor().setPropertyValue(segment, pv); } else { - setPropertyValue(tokens, pv); + setPropertyValue(segment, pv); } } - protected void setPropertyValue(PropertyTokenHolder tokens, PropertyValue pv) throws BeansException { - if (tokens.keys != null) { - processKeyedProperty(tokens, pv); + protected void setPropertyValue(PropertyPath.Segment segment, PropertyValue pv) throws BeansException { + if (!segment.keys().isEmpty()) { + processKeyedProperty(segment, pv); } else { - processLocalProperty(tokens, pv); + processLocalProperty(segment, pv); } } @SuppressWarnings({"rawtypes", "unchecked"}) - private void processKeyedProperty(PropertyTokenHolder tokens, PropertyValue pv) { - Object propValue = getPropertyHoldingValue(tokens); - PropertyHandler ph = getLocalPropertyHandler(tokens.actualName); + private void processKeyedProperty(PropertyPath.Segment segment, PropertyValue pv) { + Object propValue = getPropertyHoldingValue(segment); + PropertyHandler ph = getLocalPropertyHandler(segment.name()); if (ph == null) { throw new InvalidPropertyException( - getRootClass(), this.nestedPath + tokens.actualName, "No property handler found"); + getRootClass(), this.nestedPath + segment.name(), "No property handler found"); } - Assert.state(tokens.keys != null, "No token keys"); - String lastKey = tokens.keys[tokens.keys.length - 1]; + List keys = segment.keys(); + String lastKey = keys.get(keys.size() - 1); + String canonicalName = segment.toCanonicalName(); if (propValue.getClass().isArray()) { Class componentType = propValue.getClass().componentType(); @@ -283,33 +287,32 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA if (isExtractOldValueForEditor() && arrayIndex < Array.getLength(propValue)) { oldValue = Array.get(propValue, arrayIndex); } - Object convertedValue = convertIfNecessary(tokens.canonicalName, oldValue, pv.getValue(), - componentType, ph.nested(tokens.keys.length)); + Object convertedValue = convertIfNecessary(canonicalName, oldValue, pv.getValue(), + componentType, ph.nested(keys.size())); int length = Array.getLength(propValue); if (arrayIndex >= length && arrayIndex < getAutoGrowCollectionLimit()) { Object newArray = Array.newInstance(componentType, arrayIndex + 1); System.arraycopy(propValue, 0, newArray, 0, length); - int lastKeyIndex = tokens.canonicalName.lastIndexOf('['); - String propName = tokens.canonicalName.substring(0, lastKeyIndex); + String propName = segment.withoutLastKey().toCanonicalName(); setPropertyValue(propName, newArray); propValue = getPropertyValue(propName); } Array.set(propValue, arrayIndex, convertedValue); } catch (IndexOutOfBoundsException ex) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + tokens.canonicalName, - "Invalid array index in property path '" + tokens.canonicalName + "'", ex); + throw new InvalidPropertyException(getRootClass(), this.nestedPath + canonicalName, + "Invalid array index in property path '" + canonicalName + "'", ex); } } else if (propValue instanceof List list) { - TypeDescriptor requiredType = ph.getCollectionType(tokens.keys.length); + TypeDescriptor requiredType = ph.getCollectionType(keys.size()); int index = Integer.parseInt(lastKey); Object oldValue = null; if (isExtractOldValueForEditor() && index < list.size()) { oldValue = list.get(index); } - Object convertedValue = convertIfNecessary(tokens.canonicalName, oldValue, pv.getValue(), + Object convertedValue = convertIfNecessary(canonicalName, oldValue, pv.getValue(), requiredType.getResolvableType().resolve(), requiredType); int size = list.size(); if (index >= size && index < getAutoGrowCollectionLimit()) { @@ -318,9 +321,9 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA list.add(null); } catch (NullPointerException ex) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + tokens.canonicalName, + throw new InvalidPropertyException(getRootClass(), this.nestedPath + canonicalName, "Cannot set element with index " + index + " in List of size " + - size + ", accessed using property path '" + tokens.canonicalName + + size + ", accessed using property path '" + canonicalName + "': List does not support filling up gaps with null elements"); } } @@ -331,15 +334,15 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA list.set(index, convertedValue); } catch (IndexOutOfBoundsException ex) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + tokens.canonicalName, - "Invalid list index in property path '" + tokens.canonicalName + "'", ex); + throw new InvalidPropertyException(getRootClass(), this.nestedPath + canonicalName, + "Invalid list index in property path '" + canonicalName + "'", ex); } } } else if (propValue instanceof Map map) { - TypeDescriptor mapKeyType = ph.getMapKeyType(tokens.keys.length); - TypeDescriptor mapValueType = ph.getMapValueType(tokens.keys.length); + TypeDescriptor mapKeyType = ph.getMapKeyType(keys.size()); + TypeDescriptor mapValueType = ph.getMapValueType(keys.size()); // IMPORTANT: Do not pass full property name in here - property editors // must not kick in for map keys but rather only for map values. Object convertedMapKey = convertIfNecessary(null, null, lastKey, @@ -350,58 +353,54 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } // Pass full property name and old value in here, since we want full // conversion ability for map values. - Object convertedMapValue = convertIfNecessary(tokens.canonicalName, oldValue, pv.getValue(), + Object convertedMapValue = convertIfNecessary(canonicalName, oldValue, pv.getValue(), mapValueType.getResolvableType().resolve(), mapValueType); map.put(convertedMapKey, convertedMapValue); } else { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + tokens.canonicalName, - "Property referenced in indexed property path '" + tokens.canonicalName + + throw new InvalidPropertyException(getRootClass(), this.nestedPath + canonicalName, + "Property referenced in indexed property path '" + canonicalName + "' is neither an array nor a List nor a Map; returned value was [" + propValue + "]"); } } - private Object getPropertyHoldingValue(PropertyTokenHolder tokens) { + private Object getPropertyHoldingValue(PropertyPath.Segment segment) { // Apply indexes and map keys: fetch value for all keys but the last one. - Assert.state(tokens.keys != null, "No token keys"); - PropertyTokenHolder getterTokens = new PropertyTokenHolder(tokens.actualName); - getterTokens.canonicalName = tokens.canonicalName; - getterTokens.keys = new String[tokens.keys.length - 1]; - System.arraycopy(tokens.keys, 0, getterTokens.keys, 0, tokens.keys.length - 1); - + PropertyPath.Segment getterSegment = segment.withoutLastKey(); Object propValue; try { - propValue = getPropertyValue(getterTokens); + propValue = getPropertyValue(getterSegment); } catch (NotReadablePropertyException ex) { - throw new NotWritablePropertyException(getRootClass(), this.nestedPath + tokens.canonicalName, + throw new NotWritablePropertyException(getRootClass(), this.nestedPath + segment.toCanonicalName(), "Cannot access indexed value in property referenced " + - "in indexed property path '" + tokens.canonicalName + "'", ex); + "in indexed property path '" + segment.toCanonicalName() + "'", ex); } if (propValue == null) { // null map value case if (isAutoGrowNestedPaths()) { - int lastKeyIndex = tokens.canonicalName.lastIndexOf('['); - getterTokens.canonicalName = tokens.canonicalName.substring(0, lastKeyIndex); - propValue = setDefaultValue(getterTokens); + propValue = setDefaultValue(getterSegment); } else { - throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + tokens.canonicalName, + throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + segment.toCanonicalName(), "Cannot access indexed value in property referenced " + - "in indexed property path '" + tokens.canonicalName + "': returned null"); + "in indexed property path '" + segment.toCanonicalName() + "': returned null"); } } return propValue; } - private void processLocalProperty(PropertyTokenHolder tokens, PropertyValue pv) { - PropertyHandler ph = getLocalPropertyHandler(tokens.actualName); + private void processLocalProperty(PropertyPath.Segment segment, PropertyValue pv) { + // segment.keys() is always empty here (see setPropertyValue(Segment, PropertyValue) + // above), so the segment's canonical name is always just its raw name. + String name = segment.name(); + PropertyHandler ph = getLocalPropertyHandler(name); if (ph == null || !ph.isWritable()) { if (pv.isOptional()) { if (logger.isDebugEnabled()) { - logger.debug("Ignoring optional value for property '" + tokens.actualName + + logger.debug("Ignoring optional value for property '" + name + "' - property not found on bean class [" + getRootClass().getName() + "]"); } return; @@ -411,7 +410,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA // exception would be caught and swallowed higher up anyway... return; } - throw createNotWritablePropertyException(tokens.canonicalName); + throw createNotWritablePropertyException(name); } Object oldValue = null; @@ -433,12 +432,11 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } if (logger.isDebugEnabled()) { logger.debug("Could not read previous value of property '" + - this.nestedPath + tokens.canonicalName + "'", ex); + this.nestedPath + name + "'", ex); } } } - valueToApply = convertForProperty( - tokens.canonicalName, oldValue, originalValue, ph.toTypeDescriptor()); + valueToApply = convertForProperty(name, oldValue, originalValue, ph.toTypeDescriptor()); } pv.getOriginalPropertyValue().conversionNecessary = (valueToApply != originalValue); } @@ -451,7 +449,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } catch (InvocationTargetException ex) { PropertyChangeEvent propertyChangeEvent = new PropertyChangeEvent( - getRootInstance(), this.nestedPath + tokens.canonicalName, oldValue, pv.getValue()); + getRootInstance(), this.nestedPath + name, oldValue, pv.getValue()); if (ex.getTargetException() instanceof ClassCastException) { throw new TypeMismatchException(propertyChangeEvent, ph.getPropertyType(), ex.getTargetException()); } @@ -466,7 +464,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } catch (Exception ex) { PropertyChangeEvent pce = new PropertyChangeEvent( - getRootInstance(), this.nestedPath + tokens.canonicalName, oldValue, pv.getValue()); + getRootInstance(), this.nestedPath + name, oldValue, pv.getValue()); throw new MethodInvocationException(pce, ex); } } @@ -495,7 +493,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } } } - catch (InvalidPropertyException ex) { + catch (InvalidPropertyException | InvalidPropertyPathException ex) { // Consider as not determinable. } return null; @@ -504,14 +502,14 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA @Override public @Nullable TypeDescriptor getPropertyTypeDescriptor(String propertyName) throws BeansException { try { - AbstractNestablePropertyAccessor nestedPa = getPropertyAccessorForPropertyPath(propertyName); - String finalPath = getFinalPath(nestedPa, propertyName); - PropertyTokenHolder tokens = getPropertyNameTokens(finalPath); - PropertyHandler ph = nestedPa.getLocalPropertyHandler(tokens.actualName); + ResolvedProperty resolved = resolvePropertyPath(propertyName); + PropertyPath.Segment segment = resolved.segment(); + PropertyHandler ph = resolved.accessor().getLocalPropertyHandler(segment.name()); if (ph != null) { - if (tokens.keys != null) { + List keys = segment.keys(); + if (!keys.isEmpty()) { if (ph.isReadable() || ph.isWritable()) { - return ph.nested(tokens.keys.length); + return ph.nested(keys.size()); } } else { @@ -521,7 +519,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } } } - catch (InvalidPropertyException ex) { + catch (InvalidPropertyException | InvalidPropertyPathException ex) { // Consider as not determinable. } return null; @@ -540,7 +538,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA return true; } } - catch (InvalidPropertyException ex) { + catch (InvalidPropertyException | InvalidPropertyPathException ex) { // Cannot be evaluated, so can't be readable. } return false; @@ -559,7 +557,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA return true; } } - catch (InvalidPropertyException ex) { + catch (InvalidPropertyException | InvalidPropertyPathException ex) { // Cannot be evaluated, so can't be writable. } return false; @@ -594,25 +592,25 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA @Override public @Nullable Object getPropertyValue(String propertyName) throws BeansException { - AbstractNestablePropertyAccessor nestedPa = getPropertyAccessorForPropertyPath(propertyName); - PropertyTokenHolder tokens = getPropertyNameTokens(getFinalPath(nestedPa, propertyName)); - return nestedPa.getPropertyValue(tokens); + ResolvedProperty resolved = resolvePropertyPath(propertyName); + return resolved.accessor().getPropertyValue(resolved.segment()); } @SuppressWarnings({"rawtypes", "unchecked"}) - protected @Nullable Object getPropertyValue(PropertyTokenHolder tokens) throws BeansException { - String propertyName = tokens.canonicalName; - String actualName = tokens.actualName; + protected @Nullable Object getPropertyValue(PropertyPath.Segment segment) throws BeansException { + String propertyName = segment.toCanonicalName(); + String actualName = segment.name(); PropertyHandler ph = getLocalPropertyHandler(actualName); if (ph == null || !ph.isReadable()) { throw new NotReadablePropertyException(getRootClass(), this.nestedPath + propertyName); } try { Object value = ph.getValue(); - if (tokens.keys != null) { + List keys = segment.keys(); + if (!keys.isEmpty()) { if (value == null) { if (isAutoGrowNestedPaths()) { - value = setDefaultValue(new PropertyTokenHolder(tokens.actualName)); + value = setDefaultValue(new PropertyPath.Segment(actualName, List.of())); } else { throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + propertyName, @@ -620,10 +618,10 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA "property path '" + propertyName + "': returned null"); } } - StringBuilder indexedPropertyName = new StringBuilder(tokens.actualName); + StringBuilder indexedPropertyName = new StringBuilder(actualName); // apply indexes and map keys - for (int i = 0; i < tokens.keys.length; i++) { - String key = tokens.keys[i]; + for (int i = 0; i < keys.size(); i++) { + String key = keys.get(i); if (value == null) { throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + propertyName, "Cannot access indexed value of property referenced in indexed " + @@ -722,8 +720,8 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA */ protected @Nullable PropertyHandler getPropertyHandler(String propertyName) throws BeansException { Assert.notNull(propertyName, "Property name must not be null"); - AbstractNestablePropertyAccessor nestedPa = getPropertyAccessorForPropertyPath(propertyName); - return nestedPa.getLocalPropertyHandler(getFinalPath(nestedPa, propertyName)); + ResolvedProperty resolved = resolvePropertyPath(propertyName); + return resolved.accessor().getLocalPropertyHandler(resolved.segment().toCanonicalName()); } /** @@ -789,48 +787,55 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } /** - * Get the last component of the path. Also works if not nested. - * @param pa property accessor to work on - * @param nestedPath property path we know is nested - * @return last component of the path (the property on the target bean) + * Resolve a property path to the accessor owning its final segment and + * that segment itself, parsing the path exactly once. + * @param propertyPath the property path, which may be nested + * @return the accessor for the target bean, paired with the final segment + * @throws InvalidPropertyPathException if the given path is not a + * well-formed property path, or if its nesting depth exceeds + * {@link #getMaxNestedPathDepth()} + * @since 7.1 */ - protected String getFinalPath(AbstractNestablePropertyAccessor pa, String nestedPath) { - if (pa == this) { - return nestedPath; - } - return nestedPath.substring(PropertyAccessorUtils.getLastNestedPropertySeparatorIndex(nestedPath) + 1); + protected ResolvedProperty resolvePropertyPath(String propertyPath) { + PropertyPath.Options options = PropertyPath.Options.withMaxNestedPathDepth(getMaxNestedPathDepth()); + List segments = PropertyPath.parse(propertyPath, options).segments(); + AbstractNestablePropertyAccessor accessor = getPropertyAccessorForSegments(segments, 0); + PropertyPath.Segment segment = finalSegment(propertyPath, segments); + return new ResolvedProperty(accessor, segment); } /** - * Recursively navigate to return a property accessor for the nested property path. - *

The default implementation rejects a property path which contains unbalanced - * brackets as well as one which exceeds the {@linkplain #getMaxNestedPathDepth() - * maximum nesting depth}. An override which does not delegate to {@code super} is - * therefore responsible for performing equivalent validation itself. - * @param propertyPath property path, which may be nested - * @return a property accessor for the target bean + * The final segment of an already-parsed property path: the property to + * actually get or set. Handles the one case {@link PropertyPath} itself + * does not produce a segment for: the empty path ({@code ""}), a valid + * property path with zero segments, treated the same as a single segment + * with an empty name and no keys — a shape {@link PropertyPath.Segment}'s + * own constructor allows even though {@link PropertyPath#parse}'s grammar + * validation never produces it. */ - protected AbstractNestablePropertyAccessor getPropertyAccessorForPropertyPath(String propertyPath) { - if (PropertyAccessorUtils.hasUnbalancedBrackets(propertyPath)) { - throw new NotReadablePropertyException(getRootClass(), this.nestedPath + propertyPath, - "Property path '" + propertyPath + "' contains unbalanced brackets"); - } - int pos = PropertyAccessorUtils.getFirstNestedPropertySeparatorIndex(propertyPath); - // Handle nested properties recursively. - if (pos > -1) { - int maxNestedPathDepth = getMaxNestedPathDepth(); - if (this.nestedPathDepth >= maxNestedPathDepth) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyPath, - "Nesting depth of property path exceeds the maximum of " + maxNestedPathDepth); - } - String nestedProperty = propertyPath.substring(0, pos); - String nestedPath = propertyPath.substring(pos + 1); - AbstractNestablePropertyAccessor nestedPa = getNestedPropertyAccessor(nestedProperty); - return nestedPa.getPropertyAccessorForPropertyPath(nestedPath); - } - else { + private static PropertyPath.Segment finalSegment(String propertyPath, List segments) { + return (segments.isEmpty() ? new PropertyPath.Segment(propertyPath, List.of()) : + segments.get(segments.size() - 1)); + } + + /** + * Recursively navigate to return a property accessor for the given, + * already-parsed property path segments, peeling one segment off at a + * time until only the final segment (the property to actually get or + * set on the returned accessor) is left. + * @param segments the segments of the full property path, parsed exactly + * once by the caller + * @param fromIndex the index of the first segment not yet consumed + * @return a property accessor for the bean holding the final segment + */ + private AbstractNestablePropertyAccessor getPropertyAccessorForSegments( + List segments, int fromIndex) { + + if (segments.isEmpty() || fromIndex >= segments.size() - 1) { return this; } + AbstractNestablePropertyAccessor nestedPa = getNestedPropertyAccessor(segments.get(fromIndex)); + return nestedPa.getPropertyAccessorForSegments(segments, fromIndex + 1); } /** @@ -838,68 +843,64 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA * Create a new one if not found in the cache. *

Note: Caching nested PropertyAccessors is necessary now, * to keep registered custom editors for nested properties. - * @param nestedProperty property to create the PropertyAccessor for + * @param segment the already-parsed segment to create the PropertyAccessor for * @return the PropertyAccessor instance, either cached or newly created */ - private AbstractNestablePropertyAccessor getNestedPropertyAccessor(String nestedProperty) { - Map nestedAccessors = this.nestedPropertyAccessors; + private AbstractNestablePropertyAccessor getNestedPropertyAccessor(PropertyPath.Segment segment) { + Map nestedAccessors = this.nestedPropertyAccessors; if (nestedAccessors == null) { nestedAccessors = new HashMap<>(); this.nestedPropertyAccessors = nestedAccessors; } // Get value of bean property. - PropertyTokenHolder tokens = getPropertyNameTokens(nestedProperty); - String canonicalName = tokens.canonicalName; - Object value = getPropertyValue(tokens); + Object value = getPropertyValue(segment); if (value == null || (value instanceof Optional optional && optional.isEmpty())) { if (isAutoGrowNestedPaths()) { - value = setDefaultValue(tokens); + value = setDefaultValue(segment); } else { - throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + canonicalName); + throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + segment.toCanonicalName()); } } // Lookup cached sub-PropertyAccessor, create new one if not found. - AbstractNestablePropertyAccessor nestedPa = nestedAccessors.get(canonicalName); + AbstractNestablePropertyAccessor nestedPa = nestedAccessors.get(segment); if (nestedPa == null || nestedPa.getWrappedInstance() != ObjectUtils.unwrapOptional(value)) { + String canonicalName = segment.toCanonicalName(); if (logger.isTraceEnabled()) { logger.trace("Creating new nested " + getClass().getSimpleName() + " for property '" + canonicalName + "'"); } nestedPa = newNestedPropertyAccessor(value, this.nestedPath + canonicalName + NESTED_PROPERTY_SEPARATOR); - // Track the nesting depth here rather than in a constructor, so that the - // depth is assigned even if a subclass creates the nested property accessor - // without copying the configuration of this accessor. - nestedPa.nestedPathDepth = this.nestedPathDepth + 1; // Inherit all type-specific PropertyEditors. copyDefaultEditorsTo(nestedPa); copyCustomEditorsTo(nestedPa, canonicalName); - nestedAccessors.put(canonicalName, nestedPa); + nestedAccessors.put(segment, nestedPa); } else { if (logger.isTraceEnabled()) { - logger.trace("Using cached nested property accessor for property '" + canonicalName + "'"); + logger.trace("Using cached nested property accessor for property '" + segment.toCanonicalName() + "'"); } } return nestedPa; } - private Object setDefaultValue(PropertyTokenHolder tokens) { - PropertyValue pv = createDefaultPropertyValue(tokens); - setPropertyValue(tokens, pv); - Object defaultValue = getPropertyValue(tokens); + private Object setDefaultValue(PropertyPath.Segment segment) { + PropertyValue pv = createDefaultPropertyValue(segment); + setPropertyValue(segment, pv); + Object defaultValue = getPropertyValue(segment); Assert.state(defaultValue != null, "Default value must not be null"); return defaultValue; } - private PropertyValue createDefaultPropertyValue(PropertyTokenHolder tokens) { - TypeDescriptor desc = getPropertyTypeDescriptor(tokens.canonicalName); + private PropertyValue createDefaultPropertyValue(PropertyPath.Segment segment) { + String canonicalName = segment.toCanonicalName(); + TypeDescriptor desc = getPropertyTypeDescriptor(canonicalName); if (desc == null) { - throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + tokens.canonicalName, + throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + canonicalName, "Could not determine property type for auto-growing a default value"); } - Object defaultValue = newValue(desc.getType(), desc, tokens.canonicalName); - return new PropertyValue(tokens.canonicalName, defaultValue); + Object defaultValue = newValue(desc.getType(), desc, canonicalName); + return new PropertyValue(canonicalName, defaultValue); } private Object newValue(Class type, @Nullable TypeDescriptor desc, String name) { @@ -947,44 +948,6 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } } - /** - * Parse the given property name into the corresponding property name tokens. - * @param propertyName the property name to parse - * @return representation of the parsed property tokens - */ - private PropertyTokenHolder getPropertyNameTokens(String propertyName) { - String actualName = null; - List keys = new ArrayList<>(2); - int searchIndex = 0; - while (searchIndex != -1) { - int keyStart = propertyName.indexOf(PROPERTY_KEY_PREFIX, searchIndex); - searchIndex = -1; - if (keyStart != -1) { - int keyEnd = PropertyAccessorUtils.getPropertyNameKeyEnd(propertyName, keyStart + PROPERTY_KEY_PREFIX.length()); - if (keyEnd != -1) { - if (actualName == null) { - actualName = propertyName.substring(0, keyStart); - } - String key = propertyName.substring(keyStart + PROPERTY_KEY_PREFIX.length(), keyEnd); - if (key.length() > 1 && ((key.startsWith("'") && key.endsWith("'")) || - (key.startsWith("\"") && key.endsWith("\"")))) { - key = key.substring(1, key.length() - 1); - } - keys.add(key); - searchIndex = keyEnd + PROPERTY_KEY_SUFFIX.length(); - } - } - } - PropertyTokenHolder tokens = new PropertyTokenHolder(actualName != null ? actualName : propertyName); - if (!keys.isEmpty()) { - tokens.canonicalName += PROPERTY_KEY_PREFIX + - StringUtils.collectionToDelimitedString(keys, PROPERTY_KEY_SUFFIX + PROPERTY_KEY_PREFIX) + - PROPERTY_KEY_SUFFIX; - tokens.keys = StringUtils.toStringArray(keys); - } - return tokens; - } - @Override public String toString() { String className = getClass().getName(); @@ -995,6 +958,17 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } + /** + * The result of resolving a property path: the accessor owning its final + * segment, and that segment itself. + * @since 7.1 + * @param accessor the accessor for the target bean + * @param segment the final segment of the resolved path (the property to + * actually get or set on {@code accessor}) + */ + protected record ResolvedProperty(AbstractNestablePropertyAccessor accessor, PropertyPath.Segment segment) {} + + /** * A handler for a specific property. */ @@ -1051,22 +1025,4 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA } } - - /** - * Holder class used to store property tokens. - */ - protected static class PropertyTokenHolder { - - public PropertyTokenHolder(String name) { - this.actualName = name; - this.canonicalName = name; - } - - public String actualName; - - public String canonicalName; - - public String @Nullable [] keys; - } - } diff --git a/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java b/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java index 09cb89f49b2..9efcd5fcafc 100644 --- a/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java +++ b/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java @@ -18,6 +18,7 @@ package org.springframework.beans; import java.beans.PropertyDescriptor; import java.lang.reflect.Method; +import java.util.Objects; import org.apache.commons.logging.LogFactory; import org.jspecify.annotations.Nullable; @@ -213,9 +214,17 @@ public class BeanWrapperImpl extends AbstractNestablePropertyAccessor implements @Override public PropertyDescriptor getPropertyDescriptor(String propertyName) throws InvalidPropertyException { - BeanWrapperImpl nestedBw = (BeanWrapperImpl) getPropertyAccessorForPropertyPath(propertyName); - String finalPath = getFinalPath(nestedBw, propertyName); - PropertyDescriptor pd = nestedBw.getCachedIntrospectionResults().getPropertyDescriptor(finalPath); + ResolvedProperty resolved; + try { + resolved = resolvePropertyPath(propertyName); + } + catch (InvalidPropertyPathException ex) { + throw new InvalidPropertyException( + getRootClass(), getNestedPath() + propertyName, Objects.requireNonNull(ex.getMessage()), ex); + } + BeanWrapperImpl nestedBw = (BeanWrapperImpl) resolved.accessor(); + PropertyDescriptor pd = nestedBw.getCachedIntrospectionResults() + .getPropertyDescriptor(resolved.segment().toCanonicalName()); if (pd == null) { throw new InvalidPropertyException(getRootClass(), getNestedPath() + propertyName, "No property '" + propertyName + "' found"); diff --git a/spring-beans/src/main/java/org/springframework/beans/InvalidPropertyPathException.java b/spring-beans/src/main/java/org/springframework/beans/InvalidPropertyPathException.java index 181c2af4feb..3213b2c816a 100644 --- a/spring-beans/src/main/java/org/springframework/beans/InvalidPropertyPathException.java +++ b/spring-beans/src/main/java/org/springframework/beans/InvalidPropertyPathException.java @@ -16,6 +16,11 @@ package org.springframework.beans; +import java.beans.PropertyChangeEvent; +import java.util.Objects; + +import org.jspecify.annotations.Nullable; + /** * Exception thrown when a property path is not a well-formed property path * according to the grammar implemented by {@link PropertyPath}. @@ -56,6 +61,27 @@ public class InvalidPropertyPathException extends PropertyAccessException { this.propertyPath = propertyPath; } + /** + * Create a new {@code InvalidPropertyPathException}. + * @param propertyChangeEvent the event for the property + * @param cause the original parsing exception + */ + public InvalidPropertyPathException(PropertyChangeEvent propertyChangeEvent, InvalidPropertyPathException cause) { + super(propertyChangeEvent, Objects.requireNonNull(cause.getMessage()), cause); + this.propertyPath = cause.propertyPath; + } + + /** + * Create a new {@code InvalidPropertyPathException}. + * @param source the bean that fired the event + * @param propertyName the programmatic name of the property that was changed + * @param newValue the new value of the property + * @param cause the original parsing exception + */ + public InvalidPropertyPathException(Object source, String propertyName, @Nullable Object newValue, InvalidPropertyPathException cause) { + this(new PropertyChangeEvent(source, propertyName, null, newValue), cause); + } + /** * Return the offending property path. diff --git a/spring-beans/src/main/java/org/springframework/beans/PropertyEditorRegistrySupport.java b/spring-beans/src/main/java/org/springframework/beans/PropertyEditorRegistrySupport.java index fcde6786e0b..74ce18cb511 100644 --- a/spring-beans/src/main/java/org/springframework/beans/PropertyEditorRegistrySupport.java +++ b/spring-beans/src/main/java/org/springframework/beans/PropertyEditorRegistrySupport.java @@ -373,7 +373,7 @@ public class PropertyEditorRegistrySupport implements PropertyEditorRegistry { public boolean hasCustomEditorForElement(@Nullable Class elementType, @Nullable String propertyPath) { if (propertyPath != null && this.customEditorsForPath != null) { for (Map.Entry entry : this.customEditorsForPath.entrySet()) { - if (PropertyAccessorUtils.matchesProperty(entry.getKey(), propertyPath) && + if (matchesProperty(entry.getKey(), propertyPath) && entry.getValue().getPropertyEditor(elementType) != null) { return true; } @@ -383,6 +383,26 @@ public class PropertyEditorRegistrySupport implements PropertyEditorRegistry { return (elementType != null && this.customEditors != null && this.customEditors.containsKey(elementType)); } + /** + * Whether {@code registeredPath} is {@code propertyPath} itself or one + * indexed element of it. + */ + private static boolean matchesProperty(String registeredPath, String propertyPath) { + String canonicalRegisteredPath = PropertyPath.canonicalNameOrOriginal(registeredPath); + String canonicalPropertyPath = PropertyPath.canonicalNameOrOriginal(propertyPath); + if (!canonicalRegisteredPath.startsWith(canonicalPropertyPath)) { + return false; + } + if (canonicalRegisteredPath.length() == canonicalPropertyPath.length()) { + return true; + } + if (canonicalRegisteredPath.charAt(canonicalPropertyPath.length()) != PropertyAccessor.PROPERTY_KEY_PREFIX_CHAR) { + return false; + } + return (canonicalRegisteredPath.indexOf(PropertyAccessor.PROPERTY_KEY_SUFFIX_CHAR, canonicalPropertyPath.length() + 1) == + canonicalRegisteredPath.length() - 1); + } + /** * Determine the property type for the given property path. *

Called by {@link #findCustomEditor} if no required type has been specified, @@ -483,18 +503,25 @@ public class PropertyEditorRegistrySupport implements PropertyEditorRegistry { * will be copied. If this is null, all editors will be copied. */ protected void copyCustomEditorsTo(PropertyEditorRegistry target, @Nullable String nestedProperty) { - String actualPropertyName = - (nestedProperty != null ? PropertyAccessorUtils.getPropertyName(nestedProperty) : null); + String actualPropertyName = (nestedProperty != null ? actualPropertyNameOf(nestedProperty) : null); if (this.customEditors != null) { this.customEditors.forEach(target::registerCustomEditor); } if (this.customEditorsForPath != null) { this.customEditorsForPath.forEach((editorPath, editorHolder) -> { if (nestedProperty != null) { - int pos = PropertyAccessorUtils.getFirstNestedPropertySeparatorIndex(editorPath); - if (pos != -1) { - String editorNestedProperty = editorPath.substring(0, pos); - String editorNestedPath = editorPath.substring(pos + 1); + PropertyPath editorPropertyPath; + try { + editorPropertyPath = PropertyPath.parse(editorPath); + } + catch (InvalidPropertyPathException ex) { + // Not a well-formed path; nothing to nest into. + return; + } + List editorSegments = editorPropertyPath.segments(); + if (editorSegments.size() > 1) { + String editorNestedProperty = editorSegments.get(0).toCanonicalName(); + String editorNestedPath = editorPropertyPath.subPath(1).canonicalName(); if (editorNestedProperty.equals(nestedProperty) || editorNestedProperty.equals(actualPropertyName)) { target.registerCustomEditor( editorHolder.getRegisteredType(), editorNestedPath, editorHolder.getPropertyEditor()); @@ -509,6 +536,20 @@ public class PropertyEditorRegistrySupport implements PropertyEditorRegistry { } } + /** + * The raw name of {@code nestedProperty}'s single segment with its keys stripped, + * or {@code null} if malformed. + */ + private static @Nullable String actualPropertyNameOf(String nestedProperty) { + try { + List segments = PropertyPath.parse(nestedProperty).segments(); + return (segments.size() == 1 ? segments.get(0).name() : null); + } + catch (InvalidPropertyPathException ex) { + return null; + } + } + /** * Add property paths with all variations of stripped keys and/or indexes. diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java index 95bd2e43fa8..2dc4241ea13 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java @@ -42,7 +42,7 @@ import org.springframework.beans.BeanWrapperImpl; import org.springframework.beans.BeansException; import org.springframework.beans.InvalidPropertyException; import org.springframework.beans.MutablePropertyValues; -import org.springframework.beans.PropertyAccessorUtils; +import org.springframework.beans.PropertyAccessor; import org.springframework.beans.PropertyValue; import org.springframework.beans.PropertyValues; import org.springframework.beans.TypeConverter; @@ -1751,7 +1751,7 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac */ private boolean isConvertibleProperty(String propertyName, BeanWrapper bw) { try { - return !PropertyAccessorUtils.isNestedOrIndexedProperty(propertyName) && + return !isNestedOrIndexedProperty(propertyName) && BeanUtils.hasUniqueWriteMethod(bw.getPropertyDescriptor(propertyName)); } catch (InvalidPropertyException ex) { @@ -1759,6 +1759,19 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac } } + /** + * Check whether the given property path indicates an indexed or nested property. + */ + private static boolean isNestedOrIndexedProperty(String propertyName) { + for (int i = 0; i < propertyName.length(); i++) { + char ch = propertyName.charAt(i); + if (ch == PropertyAccessor.NESTED_PROPERTY_SEPARATOR_CHAR || ch == PropertyAccessor.PROPERTY_KEY_PREFIX_CHAR) { + return true; + } + } + return false; + } + /** * Convert the given value for the specified target property. */ diff --git a/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java b/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java index 330fca4e4c5..5227c227e0f 100644 --- a/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java @@ -301,9 +301,9 @@ abstract class AbstractPropertyAccessorTests { Person target = createPerson("John", "London", "UK"); AbstractPropertyAccessor accessor = createAccessor(target); - assertThatExceptionOfType(NotReadablePropertyException.class) + assertThatExceptionOfType(InvalidPropertyPathException.class) .isThrownBy(() -> accessor.getPropertyValue(propertyPath)) - .withMessageEndingWith("contains unbalanced brackets"); + .withMessageContaining("Invalid property path '" + propertyPath + "'"); } @Test @@ -1384,9 +1384,9 @@ abstract class AbstractPropertyAccessorTests { Person target = createPerson("John", "Paris", "FR"); AbstractPropertyAccessor accessor = createAccessor(target); - assertThatExceptionOfType(NotWritablePropertyException.class) + assertThatExceptionOfType(InvalidPropertyPathException.class) .isThrownBy(() -> accessor.setPropertyValue(propertyPath, "Zürich")) - .withMessageEndingWith("does not exist"); + .withMessageContaining("Invalid property path '" + propertyPath + "'"); assertThat(target.getAddress().getCity()).isEqualTo("Paris"); } @@ -1441,8 +1441,6 @@ abstract class AbstractPropertyAccessorTests { assertThat(accessor.getPropertyValue("map[key5[foo]].name")).isEqualTo("name8"); assertThat(accessor.getPropertyValue("map['key5[foo]'].name")).isEqualTo("name8"); assertThat(accessor.getPropertyValue("map[\"key5[foo]\"].name")).isEqualTo("name8"); - assertThat(accessor.getPropertyValue("map['].name")).isEqualTo("name9"); - assertThat(accessor.getPropertyValue("map[\"].name")).isEqualTo("name9"); assertThat(accessor.getPropertyValue("iterableMap[key1].name")).isEqualTo("nameC"); assertThat(accessor.getPropertyValue("iterableMap[key2][0].name")).isEqualTo("nameA"); assertThat(accessor.getPropertyValue("iterableMap[key2][1].name")).isEqualTo("nameB"); @@ -1708,9 +1706,9 @@ abstract class AbstractPropertyAccessorTests { } private static void assertNestedPathDepthExceeded(ThrowingCallable throwingCallable, int maxDepth) { - assertThatExceptionOfType(InvalidPropertyException.class) + assertThatExceptionOfType(InvalidPropertyPathException.class) .isThrownBy(throwingCallable) - .withMessageEndingWith("Nesting depth of property path exceeds the maximum of " + maxDepth); + .withMessageEndingWith("nesting depth exceeds the maximum of " + maxDepth); } } diff --git a/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java b/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java index f0d6892eb08..e1464ec4899 100644 --- a/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java @@ -20,7 +20,6 @@ import java.time.Duration; import java.util.Collections; import java.util.Map; import java.util.Optional; -import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; @@ -295,23 +294,18 @@ class BeanWrapperTests extends AbstractPropertyAccessorTests { void incompletelyQuotedKeyLeadsToPropertyException() { TestBean target = new TestBean(); BeanWrapper accessor = createAccessor(target); - assertThatExceptionOfType(NotWritablePropertyException.class) + assertThatExceptionOfType(InvalidPropertyPathException.class) .isThrownBy(() -> accessor.setPropertyValue("[']", "foobar")) - .satisfies(ex -> assertThat(ex.getPossibleMatches()).isNull()); + .withMessageContaining("unterminated quote"); } - @Test // gh-37252 - void overriddenGetPropertyAccessorForPropertyPathIsInvokedForEachNestedLevel() { - TestBean rod = new TestBean("rod", 31); - TestBean kerry = new TestBean("kerry", 35); - rod.setSpouse(kerry); - kerry.setSpouse(rod); - - CountingBeanWrapper accessor = new CountingBeanWrapper(rod); - - assertThat(accessor.getPropertyValue("spouse.spouse.name")).isEqualTo("rod"); - // Once for "spouse.spouse.name", once for "spouse.name", and once for "name". - assertThat(accessor.invocations).hasValue(3); + @Test // gh-37275 + void getPropertyDescriptorForMalformedPathThrowsInvalidPropertyException() { + TestBean target = new TestBean(); + BeanWrapper accessor = createAccessor(target); + assertThatExceptionOfType(InvalidPropertyException.class) + .isThrownBy(() -> accessor.getPropertyDescriptor("map[unterminated")) + .withCauseInstanceOf(InvalidPropertyPathException.class); } @@ -461,35 +455,4 @@ class BeanWrapperTests extends AbstractPropertyAccessorTests { } } - /** - * A {@link BeanWrapperImpl} which tracks how often - * {@link #getPropertyAccessorForPropertyPath(String)} is invoked, in order to - * verify that an override is applied to each level of a nested property path. - */ - private static class CountingBeanWrapper extends BeanWrapperImpl { - - private final AtomicInteger invocations; - - CountingBeanWrapper(Object target) { - super(target); - this.invocations = new AtomicInteger(); - } - - private CountingBeanWrapper(Object object, String nestedPath, CountingBeanWrapper parent) { - super(object, nestedPath, parent.getRootInstance()); - this.invocations = parent.invocations; - } - - @Override - protected BeanWrapperImpl newNestedPropertyAccessor(Object object, String nestedPath) { - return new CountingBeanWrapper(object, nestedPath, this); - } - - @Override - protected AbstractNestablePropertyAccessor getPropertyAccessorForPropertyPath(String propertyPath) { - this.invocations.incrementAndGet(); - return super.getPropertyAccessorForPropertyPath(propertyPath); - } - } - } diff --git a/spring-beans/src/test/java/org/springframework/beans/PropertyPathBehaviorChangeTests.java b/spring-beans/src/test/java/org/springframework/beans/PropertyPathBehaviorChangeTests.java deleted file mode 100644 index c6329caf5db..00000000000 --- a/spring-beans/src/test/java/org/springframework/beans/PropertyPathBehaviorChangeTests.java +++ /dev/null @@ -1,410 +0,0 @@ -/* - * Copyright 2002-present the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.beans; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; - -import org.junit.jupiter.api.Nested; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.assertj.core.api.Assertions.assertThatNoException; - -/** - * Tests that check the behavior changes for a code migration - * from {@link PropertyAccessorUtils} and {@link AbstractNestablePropertyAccessor} - * to {@link PropertyPath}. - *

This test class should be removed after migration. - * - * @author Brian Clozel - */ -class PropertyPathBehaviorChangeTests { - - /** - * The canonical name is the policy-facing form of a path: it is what - * {@code DataBinder.isAllowed} matches against {@code allowedFields} and - * {@code disallowedFields}, and what {@code AbstractPropertyBindingResult} - * reports as the field name. - */ - @Nested - class CanonicalPropertyName { - - @Test - void wellFormedPathsAreCanonicalized() { - assertCanonical("", ""); - assertCanonical("name", "name"); - assertCanonical("person.name", "person.name"); - assertCanonical("map[key1]", "map[key1]"); - assertCanonical("map['key1']", "map[key1]"); - assertCanonical("map[\"key1\"]", "map[key1]"); - assertCanonical("map[key1][key2]", "map[key1][key2]"); - assertCanonical("map['key1'].name", "map[key1].name"); - assertCanonical("map[key[0]]", "map[key[0]]"); - assertCanonical("map['key[0]']", "map[key[0]]"); - assertCanonical("map[]", "map[]"); - assertCanonical("map['']", "map[]"); - assertCanonical("[user]", "[user]"); - } - - @Test // Malformed paths are passed through unchanged, not rejected. - void malformedPathsArePassedThrough() { - assertCanonical("map[key1]other", "map[key1]other"); - assertCanonical("map[key1]other.name", "map[key1]other.name"); - assertCanonical("map[key1]IGNORED[key2]", "map[key1]IGNORED[key2]"); - assertCanonical(".name", ".name"); - assertCanonical("person.", "person."); - assertCanonical("person..name", "person..name"); - assertCanonical("map[key1", "map[key1"); - assertCanonical("map]", "map]"); - assertCanonical("address.].city", "address.].city"); - } - - @Test // An unterminated quote falls back to treating the quote as literal content. - void unterminatedQuotesAreTreatedAsLiteralContent() { - assertCanonical("map['key1]", "map['key1]"); - assertCanonical("map[\"key1]", "map[\"key1]"); - assertCanonical("map[']", "map[']"); - assertCanonical("map[\"]", "map[\"]"); - } - - @Test // Quotes are stripped whenever the key merely starts and ends with one. - void outerQuotesAreStrippedWithoutRegardToNesting() { - assertCanonical("map['a'b']", "map[a'b]"); - assertCanonical("map[a'b]", "map[a'b]"); - assertCanonical("map['a]b']", "map['a]b']"); - } - - private void assertCanonical(String path, String expected) { - assertThat(PropertyAccessorUtils.canonicalPropertyName(path)) - .as("canonicalPropertyName(\"%s\")", path) - .isEqualTo(expected); - } - } - - - /** - * {@code getPropertyName} only strips keys when the path happens to end - * with {@code ]}, which makes it inconsistent between otherwise similar - * malformed paths. - */ - @Nested - class GetPropertyName { - - @Test - void keysAreStrippedOnlyWhenThePathEndsWithAKey() { - assertThat(PropertyAccessorUtils.getPropertyName("map[key1]")).isEqualTo("map"); - assertThat(PropertyAccessorUtils.getPropertyName("map[key1][key2]")).isEqualTo("map"); - assertThat(PropertyAccessorUtils.getPropertyName("[user]")).isEmpty(); - - // Not stripped: the path does not end with ']'. - assertThat(PropertyAccessorUtils.getPropertyName("map[key1].name")).isEqualTo("map[key1].name"); - assertThat(PropertyAccessorUtils.getPropertyName("map[key1]other")).isEqualTo("map[key1]other"); - - // Stripped, even though the path is malformed, because it does end with ']'. - assertThat(PropertyAccessorUtils.getPropertyName("map[key1]IGNORED[key2]")).isEqualTo("map"); - } - } - - - /** - * The access-facing resolution: which property the accessor actually reads - * or writes for a given path. - */ - @Nested - class AccessorResolution { - - @Test - void wellFormedPathsResolveAsExpected() { - assertThat(bind("name")).containsEntry("name", "V"); - assertThat(bind("nested.name")).containsEntry("nested.name", "V"); - assertThat(bind("map[key1]")).containsEntry("map", "{key1=V}"); - assertThat(bind("map['key1']")).containsEntry("map", "{key1=V}"); - assertThat(bind("map[\"key1\"]")).containsEntry("map", "{key1=V}"); - assertThat(bind("map[key[0]]")).containsEntry("map", "{key[0]=V}"); - assertThat(bind("map['key[0]']")).containsEntry("map", "{key[0]=V}"); - assertThat(bind("map[]")).containsEntry("map", "{=V}"); - assertThat(bind("map['']")).containsEntry("map", "{=V}"); - assertThat(bind("list[0]")).containsEntry("list", "[V]"); - assertThat(bind("nestedMap[a][b]")).containsEntry("nestedMap", "{a={b=V}}"); - } - - @ParameterizedTest // gh-36999 - @ValueSource(strings = {"map[key1", "map]", "nested.].name", "nested.[.name", - "nested.[[.name", "nested.]].name", "nested.][.name"}) - void unbalancedBracketsAreRejectedOnWrite(String path) { - assertThatExceptionOfType(NotWritablePropertyException.class) - .isThrownBy(() -> bind(path)) - .withMessageContaining("Nested property in path '" + path + "' does not exist"); - } - - @ParameterizedTest // gh-36999 - @ValueSource(strings = {"map[key1", "map]", "nested.].name", "nested.[.name"}) - void unbalancedBracketsAreRejectedOnRead(String path) { - BeanWrapperImpl accessor = new BeanWrapperImpl(new Target()); - - assertThatExceptionOfType(NotReadablePropertyException.class) - .isThrownBy(() -> accessor.getPropertyValue(path)) - .withMessageEndingWith("contains unbalanced brackets"); - } - - @ParameterizedTest - @ValueSource(strings = {".name", "person.", "nested..name"}) - void emptySegmentsAreRejectedOnWrite(String path) { - assertThatExceptionOfType(NotWritablePropertyException.class) - .isThrownBy(() -> bind(path)); - } - - /** - * The behavior that matters most for {@code DataBinder}: because - * {@code ignoreUnknownFields} defaults to {@code true} and - * {@code AbstractPropertyAccessor.setPropertyValues} swallows - * {@code NotWritablePropertyException} in that mode, a malformed path - * is currently dropped without any error being recorded. - */ - @ParameterizedTest - @ValueSource(strings = {"map[key1", "map]", "nested.].name", ".name", "person."}) - void malformedPathsAreSilentlyDroppedWhenIgnoringUnknownFields(String path) { - Target target = new Target(); - BeanWrapperImpl accessor = new BeanWrapperImpl(target); - MutablePropertyValues pvs = new MutablePropertyValues(Map.of(path, "V")); - - assertThatNoException().isThrownBy(() -> accessor.setPropertyValues(pvs, true, true)); - - assertThat(target.getMap()).isEmpty(); - assertThat(target.getName()).isEmpty(); - } - } - - - /** - * The rows where {@link PropertyPath} knowingly differs from the current - * implementation. Each test asserts the old behavior and the new behavior - * together, so that the diff is explicit rather than discovered later. - */ - @Nested - class IntentionalDivergences { - - /** - * The primary target of the refactor: trailing text after an index is - * dropped by the accessor but kept by {@code canonicalPropertyName}. - */ - @Test - void trailingTextAfterAnIndexIsDroppedByTheAccessorButKeptInTheCanonicalName() { - assertThat(PropertyAccessorUtils.canonicalPropertyName("map[key1]other")) - .isEqualTo("map[key1]other"); - assertThat(bind("map[key1]other")).containsEntry("map", "{key1=V}"); - - assertThatExceptionOfType(InvalidPropertyPathException.class) - .isThrownBy(() -> PropertyPath.parse("map[key1]other")); - } - - @Test - void interleavedTextBetweenIndexesIsDroppedByTheAccessor() { - assertThat(PropertyAccessorUtils.canonicalPropertyName("nestedMap[a]X[b]")) - .isEqualTo("nestedMap[a]X[b]"); - assertThat(bind("nestedMap[a]X[b]")).containsEntry("nestedMap", "{a={b=V}}"); - - assertThatExceptionOfType(InvalidPropertyPathException.class) - .isThrownBy(() -> PropertyPath.parse("nestedMap[a]X[b]")); - } - - @Test - void trailingTextBeforeANestedSeparatorIsDroppedByTheAccessor() { - assertThatExceptionOfType(NotWritablePropertyException.class) - .isThrownBy(() -> bind("map[key1]other.name")) - // The accessor resolved 'map[key1].name', dropping 'other'. - .withMessageContaining("map[key1].name"); - - assertThatExceptionOfType(InvalidPropertyPathException.class) - .isThrownBy(() -> PropertyPath.parse("map[key1]other.name")); - } - - /** - * A key consisting of nothing but a quote character is - * deliberately supported today: commit {@code d3152c11c7} - * ("Consistently expose map key quotes", gh-36765) added - * {@code map.put("'", …)} and {@code map.put("\"", …)} to the shared - * {@code IndexedTestBean} fixture and asserted - * {@code getPropertyValue("map['].name")} in - * {@link AbstractPropertyAccessorTests}. The lenient fallback it - * relies on goes back further, to SPR-14293 ({@code cf0a0cd5d8}), - * where treating an unterminated quote as literal content was the - * chosen remedy for a {@code StringIndexOutOfBoundsException}. - *

The strict quote grammar knowingly reverts that: an opened quote - * must be closed. This is the one intentional divergence that removes - * a documented capability rather than an accident, so it needs - * explicit sign-off from the Beans/Core owners before the migration - * of {@link PropertyAccessorUtils} lands. - *

Auditing every property path literal in the accessor test suites - * found exactly three affected by the strict grammar, all from this - * lineage: {@code map['].name}, {@code map["].name} and {@code [']}. - */ - @ParameterizedTest // gh-36765 - @ValueSource(strings = {"map[']", "map[\"]"}) - void deliberatelySupportedQuoteOnlyKeyNoLongerBinds(String path) { - assertThatNoException().isThrownBy(() -> bind(path)); - - assertThatExceptionOfType(InvalidPropertyPathException.class) - .isThrownBy(() -> PropertyPath.parse(path)) - .withMessageContaining("unterminated quote"); - } - - /** - * These shapes also bind today, but unlike - * {@link #deliberatelySupportedQuoteOnlyKeyNoLongerBinds} they are - * asserted nowhere in the test suite: they are incidental consequences - * of the same SPR-14293 leniency rather than intended behavior. Under - * the strict grammar a raw key may not contain quote characters at all, - * and a closing quote must be followed immediately by {@code ]}. - */ - @ParameterizedTest - @ValueSource(strings = {"map[don't]", "map[a'b]", "map['key1]", "map[\"key1]", "map['a'b']"}) - void incidentallyAcceptedQuoteCharactersInKeysNoLongerBind(String path) { - assertThatNoException().isThrownBy(() -> bind(path)); - - assertThatExceptionOfType(InvalidPropertyPathException.class) - .isThrownBy(() -> PropertyPath.parse(path)); - } - - /** - * Today's canonical name is not a fixed point for nested quotes, so - * canonicalizing twice yields a third, different key. Nothing exploits - * this because {@code DataBinder} canonicalizes patterns once and - * fields once, but it is the fragility that the strict grammar removes: - * these inputs no longer parse, and for everything that does parse - * {@code PropertyPath.canonicalName()} is idempotent. - * @see PropertyPathTests#canonicalNameIdempotent(String) () - */ - @Test - void canonicalNameIsNotIdempotentForNestedQuotes() { - assertThat(PropertyAccessorUtils.canonicalPropertyName("map[''a'']")).isEqualTo("map['a']"); - assertThat(PropertyAccessorUtils.canonicalPropertyName("map['a']")).isEqualTo("map[a]"); - - assertThatExceptionOfType(InvalidPropertyPathException.class) - .isThrownBy(() -> PropertyPath.parse("map[''a'']")); - } - - /** - * Conversely, a quoted key is now opaque, so a key containing an - * unbalanced {@code ]} becomes legal where it is rejected today. - */ - @Test - void quotedKeyWithUnbalancedBracketBecomesLegal() { - assertThatExceptionOfType(NotWritablePropertyException.class) - .isThrownBy(() -> bind("map['a]b']")); - - assertThat(PropertyPath.parse("map['a]b']").segments()) - .containsExactly(new PropertyPath.Segment("map", List.of("a]b"))); - } - } - - - /** - * Bind {@code "V"} to the given path and return a description of the - * resulting target state, keyed by property name. - */ - private static Map bind(String path) { - Target target = new Target(); - BeanWrapperImpl accessor = new BeanWrapperImpl(target); - accessor.setAutoGrowNestedPaths(true); - accessor.setPropertyValue(path, "V"); - return target.describe(); - } - - - @SuppressWarnings("unused") - public static class Target { - - private String name = ""; - - private Target nested; - - private Map map = new LinkedHashMap<>(); - - private Map> nestedMap = new LinkedHashMap<>(); - - private List list = new ArrayList<>(); - - public String getName() { - return this.name; - } - - public void setName(String name) { - this.name = name; - } - - public Target getNested() { - return this.nested; - } - - public void setNested(Target nested) { - this.nested = nested; - } - - public Map getMap() { - return this.map; - } - - public void setMap(Map map) { - this.map = map; - } - - public Map> getNestedMap() { - return this.nestedMap; - } - - public void setNestedMap(Map> nestedMap) { - this.nestedMap = nestedMap; - } - - public List getList() { - return this.list; - } - - public void setList(List list) { - this.list = list; - } - - Map describe() { - Map description = new HashMap<>(); - if (!this.name.isEmpty()) { - description.put("name", this.name); - } - if (!this.map.isEmpty()) { - description.put("map", this.map.toString()); - } - if (!this.nestedMap.isEmpty()) { - description.put("nestedMap", this.nestedMap.toString()); - } - if (!this.list.isEmpty()) { - description.put("list", this.list.toString()); - } - if (this.nested != null) { - this.nested.describe().forEach((key, value) -> description.put("nested." + key, value)); - } - return description; - } - } - -} diff --git a/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/IndexedTestBean.java b/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/IndexedTestBean.java index ffcfa287e5b..5de9303787d 100644 --- a/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/IndexedTestBean.java +++ b/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/IndexedTestBean.java @@ -76,7 +76,6 @@ public class IndexedTestBean { TestBean tb6 = new TestBean("name6", 0); TestBean tb7 = new TestBean("name7", 0); TestBean tb8 = new TestBean("name8", 0); - TestBean tb9 = new TestBean("name9", 0); TestBean tbA = new TestBean("nameA", 0); TestBean tbB = new TestBean("nameB", 0); TestBean tbC = new TestBean("nameC", 0); @@ -105,8 +104,6 @@ public class IndexedTestBean { list.add(tbY); this.map.put("key4", list); this.map.put("key5[foo]", tb8); - this.map.put("'", tb9); - this.map.put("\"", tb9); this.myTestBeans = new MyTestBeans(tbZ); } diff --git a/spring-context/src/main/java/org/springframework/validation/AbstractPropertyBindingResult.java b/spring-context/src/main/java/org/springframework/validation/AbstractPropertyBindingResult.java index 0a8d559c625..945458040b8 100644 --- a/spring-context/src/main/java/org/springframework/validation/AbstractPropertyBindingResult.java +++ b/spring-context/src/main/java/org/springframework/validation/AbstractPropertyBindingResult.java @@ -22,8 +22,8 @@ import org.jspecify.annotations.Nullable; import org.springframework.beans.BeanUtils; import org.springframework.beans.ConfigurablePropertyAccessor; -import org.springframework.beans.PropertyAccessorUtils; import org.springframework.beans.PropertyEditorRegistry; +import org.springframework.beans.PropertyPath; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.TypeDescriptor; import org.springframework.core.convert.support.ConvertingPropertyEditorAdapter; @@ -76,11 +76,11 @@ public abstract class AbstractPropertyBindingResult extends AbstractBindingResul /** * Returns the canonical property name. - * @see org.springframework.beans.PropertyAccessorUtils#canonicalPropertyName + * @see PropertyPath#canonicalNameOrOriginal(String) */ @Override protected String canonicalFieldName(String field) { - return PropertyAccessorUtils.canonicalPropertyName(field); + return PropertyPath.canonicalNameOrOriginal(field); } /** diff --git a/spring-context/src/main/java/org/springframework/validation/DataBinder.java b/spring-context/src/main/java/org/springframework/validation/DataBinder.java index 009fbfe3fa7..7a959d5a64d 100644 --- a/spring-context/src/main/java/org/springframework/validation/DataBinder.java +++ b/spring-context/src/main/java/org/springframework/validation/DataBinder.java @@ -42,12 +42,13 @@ import org.springframework.beans.BeanInstantiationException; import org.springframework.beans.BeanUtils; import org.springframework.beans.ConfigurablePropertyAccessor; import org.springframework.beans.InvalidPropertyException; +import org.springframework.beans.InvalidPropertyPathException; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyAccessException; -import org.springframework.beans.PropertyAccessorUtils; import org.springframework.beans.PropertyBatchUpdateException; import org.springframework.beans.PropertyEditorRegistrar; import org.springframework.beans.PropertyEditorRegistry; +import org.springframework.beans.PropertyPath; import org.springframework.beans.PropertyValue; import org.springframework.beans.PropertyValues; import org.springframework.beans.SimpleTypeConverter; @@ -540,9 +541,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * {@code "xxx*yyy"} matches (with an arbitrary number of pattern parts), as * well as direct equality. *

The default implementation of this method stores allowed field patterns - * in {@linkplain PropertyAccessorUtils#canonicalPropertyName(String) canonical} - * form. Subclasses which override this method must therefore take this into - * account. + * in {@linkplain PropertyPath#canonicalName() canonical} form. Subclasses + * which override this method must therefore take this into account. *

More sophisticated matching can be implemented by overriding the * {@link #isAllowed} method. *

Used for binding to fields with {@link #bind(PropertyValues)}, and not @@ -552,7 +552,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * @see #isAllowed(String) */ public void setAllowedFields(String @Nullable ... allowedFields) { - this.allowedFields = PropertyAccessorUtils.canonicalPropertyNames(allowedFields); + this.allowedFields = canonicalPropertyNames(allowedFields); } /** @@ -573,9 +573,9 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * {@code "xxx*yyy"} matches (with an arbitrary number of pattern parts), * as well as direct equality. *

The default implementation of this method stores disallowed field - * patterns in {@linkplain PropertyAccessorUtils#canonicalPropertyName(String) - * canonical} form, and subsequently pattern matching in {@link #isAllowed} - * is case-insensitive. Subclasses that override this method must therefore + * patterns in {@linkplain PropertyPath#canonicalName() canonical} form, + * and subsequently pattern matching in {@link #isAllowed} is + * case-insensitive. Subclasses that override this method must therefore * take this transformation into account. *

More sophisticated matching can be implemented by overriding the * {@link #isAllowed} method. @@ -592,16 +592,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { */ @Deprecated(since = "7.1", forRemoval = true) public void setDisallowedFields(String @Nullable ... disallowedFields) { - if (disallowedFields == null) { - this.disallowedFields = null; - } - else { - String[] fieldPatterns = new String[disallowedFields.length]; - for (int i = 0; i < fieldPatterns.length; i++) { - fieldPatterns[i] = PropertyAccessorUtils.canonicalPropertyName(disallowedFields[i]); - } - this.disallowedFields = fieldPatterns; - } + this.disallowedFields = canonicalPropertyNames(disallowedFields); } /** @@ -628,7 +619,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * @see DefaultBindingErrorProcessor#MISSING_FIELD_ERROR_CODE */ public void setRequiredFields(String @Nullable ... requiredFields) { - this.requiredFields = PropertyAccessorUtils.canonicalPropertyNames(requiredFields); + this.requiredFields = canonicalPropertyNames(requiredFields); if (logger.isDebugEnabled()) { logger.debug("DataBinder requires binding of required fields [" + StringUtils.arrayToCommaDelimitedString(requiredFields) + "]"); @@ -1294,6 +1285,17 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { applyPropertyValues(mpvs); } + private static String @Nullable [] canonicalPropertyNames(String @Nullable [] fields) { + if (fields == null) { + return null; + } + String[] result = new String[fields.length]; + for (int i = 0; i < fields.length; i++) { + result[i] = PropertyPath.canonicalNameOrOriginal(fields[i]); + } + return result; + } + /** * Check the given property values against the allowed fields, * removing values for fields that are not allowed. @@ -1303,11 +1305,23 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { */ protected void checkAllowedFields(MutablePropertyValues mpvs) { PropertyValue[] pvs = mpvs.getPropertyValues(); + PropertyPath.Options options = PropertyPath.Options.withMaxNestedPathDepth(getMaxNestedPathDepth()); for (PropertyValue pv : pvs) { - String field = PropertyAccessorUtils.canonicalPropertyName(pv.getName()); - if (!isAllowed(field)) { + PropertyPath field; + try { + field = PropertyPath.parse(pv.getName(), options); + } + catch (InvalidPropertyPathException ex) { mpvs.removePropertyValue(pv); - getBindingResult().recordSuppressedField(field); + Object target = getTarget(); + getBindingErrorProcessor().processPropertyAccessException( + new InvalidPropertyPathException(target != null ? target : this, pv.getName(), pv.getValue(), ex), + getInternalBindingResult()); + continue; + } + if (!isAllowed(field.canonicalName())) { + mpvs.removePropertyValue(pv); + getBindingResult().recordSuppressedField(field.canonicalName()); if (logger.isDebugEnabled()) { logger.debug("Field [" + field + "] has been removed from PropertyValues " + "and will not be bound, because it has not been found in the list of allowed fields"); @@ -1327,12 +1341,14 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * matching against disallowed field patterns is case-insensitive. *

A field matching a disallowed pattern will not be accepted even if it * also happens to match a pattern in the allowed list. + *

{@code field} is matched as-is, but it must already have been validated + * and canonicalized via {@link PropertyPath} by the caller first. *

Can be overridden in subclasses, but care must be taken to honor the * aforementioned contract. - * @param field the field to check + * @param field the field to check, expected to already be in canonical form * @return {@code true} if the field is allowed * @see #setAllowedFields - * @see org.springframework.util.PatternMatchUtils#simpleMatch(String, String) + * @see org.springframework.util.PatternMatchUtils#simpleMatch(String[], String) */ protected boolean isAllowed(String field) { String[] allowed = getAllowedFields(); @@ -1361,7 +1377,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { Map propertyValues = new HashMap<>(); PropertyValue[] pvs = mpvs.getPropertyValues(); for (PropertyValue pv : pvs) { - String canonicalName = PropertyAccessorUtils.canonicalPropertyName(pv.getName()); + String canonicalName = PropertyPath.canonicalNameOrOriginal(pv.getName()); propertyValues.put(canonicalName, pv); } for (String field : requiredFields) { diff --git a/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java b/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java index 65bacc9602b..df1c794f482 100644 --- a/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java +++ b/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java @@ -23,6 +23,7 @@ import java.util.Map; import org.junit.jupiter.api.Test; import org.springframework.beans.InvalidPropertyException; +import org.springframework.beans.InvalidPropertyPathException; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.NotWritablePropertyException; import org.springframework.beans.NullValueInNestedPathException; @@ -162,7 +163,7 @@ class DataBinderFieldAccessTests { rod.setSpouse(kerry); kerry.setSpouse(rod); - DataBinder binder = new DataBinder(rod); + DataBinder binder = new DataBinder(rod, "rod"); binder.setMaxNestedPathDepth(2); binder.initDirectFieldAccess(); @@ -173,9 +174,11 @@ class DataBinderFieldAccessTests { MutablePropertyValues tooDeep = new MutablePropertyValues(); tooDeep.add("spouse.spouse.spouse.name", "Joe"); - assertThatExceptionOfType(InvalidPropertyException.class) - .isThrownBy(() -> binder.bind(tooDeep)) - .withMessageEndingWith("Nesting depth of property path exceeds the maximum of 2"); + binder.bind(tooDeep); + assertThat(binder.getBindingResult().getFieldErrors("spouse.spouse.spouse.name")).singleElement().satisfies(error -> { + assertThat(error.getCode()).isEqualTo(InvalidPropertyPathException.ERROR_CODE); + assertThat(error.getRejectedValue()).isEqualTo("Joe"); + }); } @Test diff --git a/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java b/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java index 4b2203ab7e2..8aa6c45c14b 100644 --- a/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java +++ b/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java @@ -42,6 +42,7 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.BeanWrapper; import org.springframework.beans.ConfigurablePropertyAccessor; import org.springframework.beans.InvalidPropertyException; +import org.springframework.beans.InvalidPropertyPathException; import org.springframework.beans.MethodInvocationException; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.NotWritablePropertyException; @@ -735,6 +736,56 @@ class DataBinderTests { assertThat(binder.getBindingResult().getSuppressedFields()).containsExactlyInAnyOrder("age", "favoriteColor"); } + @Test // gh-37275 + void bindingWithMalformedFieldNameSurfacesAsFieldErrorRatherThanBeingSilentlyDropped() throws BindException { + TestBean rod = new TestBean(); + DataBinder binder = new DataBinder(rod, "rod"); + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("name", "Rod"); + pvs.add("map[key1]other", "value"); // trailing garbage after a key: not a well-formed property path + + binder.bind(pvs); + + assertThat(rod.getName()).as("well-formed fields still bind").isEqualTo("Rod"); + assertThat(binder.getBindingResult().getFieldErrors("map[key1]other")).singleElement().satisfies(error -> { + assertThat(error.getCode()).isEqualTo(InvalidPropertyPathException.ERROR_CODE); + assertThat(error.getRejectedValue()).isEqualTo("value"); + }); + assertThatExceptionOfType(BindException.class).isThrownBy(binder::close); + } + + @Test // gh-37275 + void bindingWithMalformedFieldNameIsNotSuppressedEvenWhenAllowedFieldsAreConfigured() throws BindException { + TestBean rod = new TestBean(); + DataBinder binder = new DataBinder(rod, "rod"); + binder.setAllowedFields("name"); + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("name", "Rod"); + pvs.add("map[key1]other", "value"); + + binder.bind(pvs); + + assertThat(binder.getBindingResult().getSuppressedFields()).isEmpty(); + assertThat(binder.getBindingResult().getFieldErrors("map[key1]other")).hasSize(1); + } + + @Test // gh-37275 + void isAllowedNeverThrowsForMalformedField() { + class ExposingBinder extends DataBinder { + ExposingBinder(Object target) { + super(target, "target"); + } + boolean callIsAllowed(String field) { + return isAllowed(field); + } + } + + ExposingBinder binder = new ExposingBinder(new TestBean()); + binder.setAllowedFields("name"); + + assertThat(binder.callIsAllowed("map[key1]other")).isFalse(); + } + @Test @SuppressWarnings("removal") void bindingWithAllowedAndDisallowedFields() throws BindException { @@ -2080,7 +2131,7 @@ class DataBinderTests { rod.setSpouse(kerry); kerry.setSpouse(rod); - DataBinder binder = new DataBinder(rod); + DataBinder binder = new DataBinder(rod, "rod"); binder.setMaxNestedPathDepth(2); MutablePropertyValues pvs = new MutablePropertyValues(); @@ -2090,9 +2141,11 @@ class DataBinderTests { MutablePropertyValues tooDeep = new MutablePropertyValues(); tooDeep.add("spouse.spouse.spouse.name", "Joe"); - assertThatExceptionOfType(InvalidPropertyException.class) - .isThrownBy(() -> binder.bind(tooDeep)) - .withMessageEndingWith("Nesting depth of property path exceeds the maximum of 2"); + binder.bind(tooDeep); + assertThat(binder.getBindingResult().getFieldErrors("spouse.spouse.spouse.name")).singleElement().satisfies(error -> { + assertThat(error.getCode()).isEqualTo(InvalidPropertyPathException.ERROR_CODE); + assertThat(error.getRejectedValue()).isEqualTo("Joe"); + }); } @Test // gh-37252