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.
This commit is contained in:
Oliver Drotbohm
2023-03-01 22:10:02 +01:00
parent 83162b25c7
commit 35a2f45736
12 changed files with 115 additions and 121 deletions

View File

@@ -81,15 +81,6 @@ public class PersistentPropertyAccessorUnitTests {
assertThat(customer.firstname).isEqualTo("Oliver August");
}
@Test // DATACMNS-1275
public void rejectsEmptyPathToSetValues() {
setUp(new Order(null), "");
assertThatIllegalArgumentException() //
.isThrownBy(() -> accessor.setProperty(path, "Oliver August"));
}
@Test // DATACMNS-1275
public void rejectsIntermediateNullValuesForRead() {

View File

@@ -27,7 +27,6 @@ import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.core.convert.converter.Converter;
import org.springframework.data.mapping.PersistentProperty;
import org.springframework.data.mapping.PersistentPropertyPath;
@@ -110,18 +109,10 @@ class DefaultPersistentPropertyPathUnitTests<P extends PersistentProperty<P>> {
}
@Test
void returnsEmptyPathForRootLevelProperty() {
assertThat(oneLeg.getParentPath()).isEmpty();
}
void returnsNullForRootLevelProperty() {
@Test
void returnItselfForEmptyPath() {
var parent = oneLeg.getParentPath();
var parentsParent = parent.getParentPath();
assertThat(parentsParent).isEmpty();
assertThat(parentsParent).isSameAs(parent);
assertThat(oneLeg.isRootPath()).isTrue();
assertThat(oneLeg.getParentPath()).isNull();
}
@Test
@@ -132,27 +123,27 @@ class DefaultPersistentPropertyPathUnitTests<P extends PersistentProperty<P>> {
@Test // DATACMNS-444
void skipsMappedPropertyNameIfConverterReturnsNull() {
assertThat(twoLegs.toDotPath(source -> null)).isNull();
assertThat(twoLegs.toDotPath(source -> null)).isEmpty();
}
@Test // DATACMNS-444
void skipsMappedPropertyNameIfConverterReturnsEmptyStrings() {
assertThat(twoLegs.toDotPath(source -> "")).isNull();
assertThat(twoLegs.toDotPath(source -> "")).isEmpty();
}
@Test // DATACMNS-1466
void returnsNullForLeafPropertyOnEmptyPath() {
void throwsExceptionForLeafPropertyOnEmptyPath() {
PersistentPropertyPath<P> path = new DefaultPersistentPropertyPath<P>(Collections.emptyList());
assertThat(path.getLeafProperty()).isNull();
assertThatIllegalStateException().isThrownBy(() -> path.getLeafProperty());
}
@Test // DATACMNS-1466
void returnsNullForBasePropertyOnEmptyPath() {
void throwsExceptionForBasePropertyOnEmptyPath() {
PersistentPropertyPath<P> path = new DefaultPersistentPropertyPath<P>(Collections.emptyList());
assertThat(path.getBaseProperty()).isNull();
assertThatIllegalStateException().isThrownBy(() -> path.getBaseProperty());
}
}

View File

@@ -22,7 +22,6 @@ import jakarta.inject.Inject;
import java.util.List;
import org.junit.jupiter.api.Test;
import org.springframework.data.annotation.Reference;
import org.springframework.data.mapping.MappingException;
import org.springframework.data.mapping.PersistentPropertyPath;
@@ -106,8 +105,8 @@ class PersistentPropertyPathFactoryUnitTests {
}
@Test // DATACMNS-1275
void createsEmptyPropertyPathCorrectly() {
assertThat(factory.from(Wrapper.class, "")).isEmpty();
void rejectsEmptyPropertyPathCreation() {
assertThatIllegalArgumentException().isThrownBy(() -> factory.from(Wrapper.class, ""));
}
@Test // DATACMNS-1275