Tighten nullability contract of PersistentPropertyPath.

We should change the definition of `PersistentPropertyPath` to — in its public API — not allow empty instances anymore. Those violate the concept and bleed into the concept's API by having to make all methods nullable (returning null in exactly that "empty" case). An empty property path doesn't make any actual sense as you cannot reasonably answer the methods declared on the interface except by returning null, which then causes client code having to verify the returned values all the time.

This is now changed into only making `PersistentPropertyPath.getParentPath()` nullable and letting it return null for single segment paths. Adapted client code accordingly. `….getRequiredLeadProperty()` is now deprecated in favour of `….getLeafProperty()` not returning null anymore.

Fixes #2813.

Author: odrotbohm

src/main/java/org/springframework/data/auditing/MappingAuditableBeanWrapperFactory.java

@@ -238,7 +238,7 @@ private TemporalAccessor setDateProperty(
 
 			property.forEach(it -> {
 
-				Class<?> type = it.getRequiredLeafProperty().getType();
+				Class<?> type = it.getLeafProperty().getType();
 
 				this.accessor.setProperty(it, getDateValueToSet(value, type, accessor.getBean()), OPTIONS);
 			});

src/main/java/org/springframework/data/mapping/PersistentPropertyAccessor.java

@@ -62,24 +62,28 @@ default void setProperty(PersistentPropertyPath<? extends PersistentProperty<?>>
 		Assert.notNull(path, "PersistentPropertyPath must not be null");
 		Assert.isTrue(!path.isEmpty(), "PersistentPropertyPath must not be empty");
 
+		PersistentProperty<? extends PersistentProperty<?>> leafProperty = path.getLeafProperty();
 		PersistentPropertyPath<? extends PersistentProperty<?>> parentPath = path.getParentPath();
-		PersistentProperty<? extends PersistentProperty<?>> leafProperty = path.getRequiredLeafProperty();
-		PersistentProperty<? extends PersistentProperty<?>> parentProperty = parentPath.isEmpty() ? null
-				: parentPath.getLeafProperty();
 
-		if (parentProperty != null && (parentProperty.isCollectionLike() || parentProperty.isMap())) {
-			throw new MappingException(
-					String.format("Cannot traverse collection or map intermediate %s", parentPath.toDotPath()));
+		if (parentPath != null) {
+
+			PersistentProperty<? extends PersistentProperty<?>> parentProperty = parentPath.getLeafProperty();
+
+			if (parentProperty.isCollectionLike() || parentProperty.isMap()) {
+				throw new MappingException(
+						"Cannot traverse collection or map intermediate %s".formatted(parentPath.toDotPath()));
+			}
 		}
 
-		Object parent = parentPath.isEmpty() ? getBean() : getProperty(parentPath);
+		Object parent = parentPath == null ? getBean() : getProperty(parentPath);
 
 		if (parent == null) {
 
 			String nullIntermediateMessage = "Cannot lookup property %s on null intermediate; Original path was: %s on %s";
 
 			throw new MappingException(
-					String.format(nullIntermediateMessage, parentProperty, path.toDotPath(), getBean().getClass().getName()));
+					String.format(nullIntermediateMessage, path.getParentPath(), path.toDotPath(),
+							getBean().getClass().getName()));
 		}
 
 		PersistentPropertyAccessor<?> accessor = parent == getBean() //
@@ -88,7 +92,7 @@ default void setProperty(PersistentPropertyPath<? extends PersistentProperty<?>>
 
 		accessor.setProperty(leafProperty, value);
 
-		if (parentPath.isEmpty()) {
+		if (parentPath == null) {
 			return;
 		}
 

src/main/java/org/springframework/data/mapping/PersistentPropertyPath.java

@@ -30,28 +30,25 @@ public interface PersistentPropertyPath<P extends PersistentProperty<P>> extends
 	/**
 	 * Returns the dot based path notation using {@link PersistentProperty#getName()}.
 	 *
-	 * @return
+	 * @return will never be {@literal null}.
 	 */
-	@Nullable
 	String toDotPath();
 
 	/**
 	 * Returns the dot based path notation using the given {@link Converter} to translate individual
 	 * {@link PersistentProperty}s to path segments.
 	 *
 	 * @param converter must not be {@literal null}.
-	 * @return
+	 * @return will never be {@literal null}.
 	 */
-	@Nullable
 	String toDotPath(Converter<? super P, String> converter);
 
 	/**
 	 * Returns a {@link String} path with the given delimiter based on the {@link PersistentProperty#getName()}.
 	 *
-	 * @param delimiter must not be {@literal null}.
-	 * @return
+	 * @param delimiter must not be {@literal null} or empty.
+	 * @return will never be {@literal null}.
 	 */
-	@Nullable
 	String toPath(String delimiter);
 
 	/**
@@ -60,48 +57,57 @@ public interface PersistentPropertyPath<P extends PersistentProperty<P>> extends
 	 *
 	 * @param delimiter must not be {@literal null}.
 	 * @param converter must not be {@literal null}.
-	 * @return
+	 * @return will never be {@literal null}.
 	 */
-	@Nullable
 	String toPath(String delimiter, Converter<? super P, String> converter);
 
 	/**
 	 * Returns the last property in the {@link PersistentPropertyPath}. So for {@code foo.bar} it will return the
 	 * {@link PersistentProperty} for {@code bar}. For a simple {@code foo} it returns {@link PersistentProperty} for
 	 * {@code foo}.
 	 *
-	 * @return
+	 * @return will never be {@literal null}.
 	 */
-	@Nullable
 	P getLeafProperty();
 
+	/**
+	 * Returns the last property in the {@link PersistentPropertyPath}. So for {@code foo.bar} it will return the
+	 * {@link PersistentProperty} for {@code bar}. For a simple {@code foo} it returns {@link PersistentProperty} for
+	 * {@code foo}.
+	 *
+	 * @return will never be {@literal null}.
+	 * @deprecated use {@link #getLeafProperty()} instead.
+	 */
+	@Deprecated(since = "3.1", forRemoval = true)
 	default P getRequiredLeafProperty() {
-
-		P property = getLeafProperty();
-
-		if (property == null) {
-			throw new IllegalStateException("No leaf property found");
-		}
-
-		return property;
+		return getLeafProperty();
 	}
 
 	/**
 	 * Returns the first property in the {@link PersistentPropertyPath}. So for {@code foo.bar} it will return the
 	 * {@link PersistentProperty} for {@code foo}. For a simple {@code foo} it returns {@link PersistentProperty} for
 	 * {@code foo}.
 	 *
-	 * @return
+	 * @return will never be {@literal null}.
 	 */
-	@Nullable
 	P getBaseProperty();
 
+	/**
+	 * Returns whether the current path is located at the root of the traversal. In other words, if the path only contains
+	 * a single property.
+	 *
+	 * @return whether the current path is located at the root of the traversal
+	 */
+	default boolean isRootPath() {
+		return getLength() == 1;
+	}
+
 	/**
 	 * Returns whether the given {@link PersistentPropertyPath} is a base path of the current one. This means that the
 	 * current {@link PersistentPropertyPath} is basically an extension of the given one.
 	 *
 	 * @param path must not be {@literal null}.
-	 * @return
+	 * @return whether the given {@link PersistentPropertyPath} is a base path of the current one.
 	 */
 	boolean isBasePathOf(PersistentPropertyPath<P> path);
 
@@ -111,7 +117,7 @@ default P getRequiredLeafProperty() {
 	 * the current one the current {@link PersistentPropertyPath} will be returned as is.
 	 *
 	 * @param base must not be {@literal null}.
-	 * @return
+	 * @return will never be {@literal null}.
 	 */
 	PersistentPropertyPath<P> getExtensionForBaseOf(PersistentPropertyPath<P> base);
 
@@ -121,13 +127,15 @@ default P getRequiredLeafProperty() {
 	 * returning the property.
 	 *
 	 * @return
+	 * @throws IllegalStateException if the current path only consists of one segment.
 	 */
-	PersistentPropertyPath<P> getParentPath();
+	@Nullable
+	PersistentPropertyPath<P> getParentPath() throws IllegalStateException;
 
 	/**
 	 * Returns the length of the {@link PersistentPropertyPath}.
 	 *
-	 * @return
+	 * @return a value greater than 0.
 	 */
 	int getLength();
 }

src/main/java/org/springframework/data/mapping/context/DefaultPersistentPropertyPath.java

@@ -38,7 +38,7 @@
  */
 class DefaultPersistentPropertyPath<P extends PersistentProperty<P>> implements PersistentPropertyPath<P> {
 
-	private static final Converter<PersistentProperty<?>, String> DEFAULT_CONVERTER = (source) -> source.getName();
+	private static final Converter<PersistentProperty<?>, String> DEFAULT_CONVERTER = PersistentProperty::getName;
 	private static final String DEFAULT_DELIMITER = ".";
 
 	private final List<P> properties;
@@ -60,7 +60,7 @@ public DefaultPersistentPropertyPath(List<P> properties) {
 	 *
 	 * @return
 	 */
-	public static <T extends PersistentProperty<T>> DefaultPersistentPropertyPath<T> empty() {
+	static <T extends PersistentProperty<T>> DefaultPersistentPropertyPath<T> empty() {
 		return new DefaultPersistentPropertyPath<T>(Collections.emptyList());
 	}
 
@@ -71,15 +71,14 @@ public static <T extends PersistentProperty<T>> DefaultPersistentPropertyPath<T>
 	 * @return a new {@link DefaultPersistentPropertyPath} with the given property appended to the current one.
 	 * @throws IllegalArgumentException in case the property is not a property of the type of the current leaf property.
 	 */
-	public DefaultPersistentPropertyPath<P> append(P property) {
+	DefaultPersistentPropertyPath<P> append(P property) {
 
 		Assert.notNull(property, "Property must not be null");
 
 		if (isEmpty()) {
 			return new DefaultPersistentPropertyPath<>(Collections.singletonList(property));
 		}
 
-		@SuppressWarnings("null")
 		Class<?> leafPropertyType = getLeafProperty().getActualType();
 
 		Assert.isTrue(property.getOwner().getType().equals(leafPropertyType),
@@ -91,45 +90,50 @@ public DefaultPersistentPropertyPath<P> append(P property) {
 		return new DefaultPersistentPropertyPath<>(properties);
 	}
 
-	@Nullable
+	@Override
 	public String toDotPath() {
 		return toPath(DEFAULT_DELIMITER, DEFAULT_CONVERTER);
 	}
 
-	@Nullable
+	@Override
 	public String toDotPath(Converter<? super P, String> converter) {
 		return toPath(DEFAULT_DELIMITER, converter);
 	}
 
-	@Nullable
+	@Override
 	public String toPath(String delimiter) {
 		return toPath(delimiter, DEFAULT_CONVERTER);
 	}
 
-	@Nullable
+	@Override
 	public String toPath(String delimiter, Converter<? super P, String> converter) {
 
 		Assert.hasText(delimiter, "Delimiter must not be null or empty");
 		Assert.notNull(converter, "Converter must not be null");
 
-		String result = properties.stream() //
+		return properties.stream() //
 				.map(converter::convert) //
 				.filter(StringUtils::hasText) //
 				.collect(Collectors.joining(delimiter));
-
-		return result.isEmpty() ? null : result;
 	}
 
-	@Nullable
+	@Override
 	public P getLeafProperty() {
-		return properties.isEmpty() ? null : properties.get(properties.size() - 1);
+
+		Assert.state(properties.size() > 0, "Empty PersistentPropertyPath should not exist");
+
+		return properties.get(properties.size() - 1);
 	}
 
-	@Nullable
+	@Override
 	public P getBaseProperty() {
-		return properties.isEmpty() ? null : properties.get(0);
+
+		Assert.state(properties.size() > 0, "Empty PersistentPropertyPath should not exist");
+
+		return properties.get(0);
 	}
 
+	@Override
 	public boolean isBasePathOf(PersistentPropertyPath<P> path) {
 
 		Assert.notNull(path, "PersistentPropertyPath must not be null");
@@ -152,37 +156,31 @@ public boolean isBasePathOf(PersistentPropertyPath<P> path) {
 		return true;
 	}
 
+	@Override
 	public PersistentPropertyPath<P> getExtensionForBaseOf(PersistentPropertyPath<P> base) {
 
 		if (!base.isBasePathOf(this)) {
 			return this;
 		}
 
-		List<P> result = new ArrayList<>();
-		Iterator<P> iterator = iterator();
-
-		for (int i = 0; i < base.getLength(); i++) {
-			iterator.next();
-		}
-
-		while (iterator.hasNext()) {
-			result.add(iterator.next());
-		}
-
-		return new DefaultPersistentPropertyPath<>(result);
+		return new DefaultPersistentPropertyPath<>(properties.subList(base.getLength(), getLength()));
 	}
 
+	@Nullable
+	@Override
 	public PersistentPropertyPath<P> getParentPath() {
 
 		int size = properties.size();
 
-		return size == 0 ? this : new DefaultPersistentPropertyPath<>(properties.subList(0, size - 1));
+		return size == 1 ? null : new DefaultPersistentPropertyPath<>(properties.subList(0, size - 1));
 	}
 
+	@Override
 	public int getLength() {
 		return properties.size();
 	}
 
+	@Override
 	public Iterator<P> iterator() {
 		return properties.iterator();
 	}
@@ -202,7 +200,7 @@ public boolean containsPropertyOfType(@Nullable TypeInformation<?> type) {
 	}
 
 	@Override
-	public boolean equals(Object o) {
+	public boolean equals(@Nullable Object o) {
 
 		if (this == o) {
 			return true;

src/main/java/org/springframework/data/mapping/context/InvalidPersistentPropertyPath.java

@@ -54,7 +54,7 @@ public class InvalidPersistentPropertyPath extends MappingException {
 	public InvalidPersistentPropertyPath(String source, TypeInformation<?> type, String unresolvableSegment,
 			PersistentPropertyPath<? extends PersistentProperty<?>> resolvedPath) {
 
-		super(createMessage(resolvedPath.isEmpty() ? type : resolvedPath.getRequiredLeafProperty().getTypeInformation(),
+		super(createMessage(resolvedPath.isEmpty() ? type : resolvedPath.getLeafProperty().getTypeInformation(),
 				unresolvableSegment));
 
 		Assert.notNull(source, "Source property path must not be null");