From 0d2bf97d94a1b7c9fddeb1dc0169d7b8bee6c903 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Sun, 27 Oct 2013 16:28:54 +0100 Subject: [PATCH] DATACMNS-387 - Improvements in null handling in PartTree area. We're now rejecting invalid constructor arguments handed to ClassTypeInformation, Part, PartTree and PropertyPath. Beyond that we skip the creation of a Part for an empty path segment, so that you don't end up with an invalid Part instance for a findAllByOrderByFooAsc. --- .../data/mapping/PropertyPath.java | 10 ++-- .../data/repository/query/parser/Part.java | 17 ++++--- .../repository/query/parser/PartTree.java | 4 +- .../data/util/ClassTypeInformation.java | 11 +++-- .../data/mapping/PropertyPathUnitTests.java | 48 ++++++++++++++++++- .../query/parser/PartTreeUnitTests.java | 12 +++++ .../util/ClassTypeInformationUnitTests.java | 8 ++++ 7 files changed, 92 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/PropertyPath.java b/src/main/java/org/springframework/data/mapping/PropertyPath.java index a05a8667e..459d21b02 100644 --- a/src/main/java/org/springframework/data/mapping/PropertyPath.java +++ b/src/main/java/org/springframework/data/mapping/PropertyPath.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2012 the original author or authors. + * Copyright 2011-2013 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. @@ -52,7 +52,6 @@ public class PropertyPath implements Iterable { * @param owningType must not be {@literal null}. */ PropertyPath(String name, Class owningType) { - this(name, ClassTypeInformation.from(owningType), null); } @@ -132,7 +131,6 @@ public class PropertyPath implements Iterable { * @see #hasNext() */ public PropertyPath next() { - return next; } @@ -143,7 +141,6 @@ public class PropertyPath implements Iterable { * @return */ public boolean hasNext() { - return next != null; } @@ -167,7 +164,6 @@ public class PropertyPath implements Iterable { * @return */ public boolean isCollection() { - return isCollection; } @@ -241,7 +237,6 @@ public class PropertyPath implements Iterable { * @return */ public static PropertyPath from(String source, Class type) { - return from(source, ClassTypeInformation.from(type)); } @@ -254,6 +249,9 @@ public class PropertyPath implements Iterable { */ public static PropertyPath from(String source, TypeInformation type) { + Assert.hasText(source, "Source must not be null or empty!"); + Assert.notNull(type, "TypeInformation must not be null or empty!"); + List iteratorSource = new ArrayList(); Matcher matcher = SPLITTER.matcher("_" + source); diff --git a/src/main/java/org/springframework/data/repository/query/parser/Part.java b/src/main/java/org/springframework/data/repository/query/parser/Part.java index 7e86b8d4b..1d3a28b45 100644 --- a/src/main/java/org/springframework/data/repository/query/parser/Part.java +++ b/src/main/java/org/springframework/data/repository/query/parser/Part.java @@ -24,6 +24,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import org.springframework.data.mapping.PropertyPath; +import org.springframework.util.Assert; import org.springframework.util.StringUtils; /** @@ -46,25 +47,27 @@ public class Part { * Creates a new {@link Part} from the given method name part, the {@link Class} the part originates from and the * start parameter index. * - * @param part must not be {@literal null}. + * @param source must not be {@literal null}. * @param clazz must not be {@literal null}. */ - public Part(String part, Class clazz) { - - this(part, clazz, false); + public Part(String source, Class clazz) { + this(source, clazz, false); } /** * Creates a new {@link Part} from the given method name part, the {@link Class} the part originates from and the * start parameter index. * - * @param part must not be {@literal null}. + * @param source must not be {@literal null}. * @param clazz must not be {@literal null}. * @param alwaysIgnoreCase */ - public Part(String part, Class clazz, boolean alwaysIgnoreCase) { + public Part(String source, Class clazz, boolean alwaysIgnoreCase) { - String partToUse = detectAndSetIgnoreCase(part); + Assert.hasText(source, "Part source must not be null or emtpy!"); + Assert.notNull(clazz, "Type must not be null!"); + + String partToUse = detectAndSetIgnoreCase(source); if (alwaysIgnoreCase && ignoreCase != IgnoreCaseType.ALWAYS) { this.ignoreCase = IgnoreCaseType.WHEN_POSSIBLE; } diff --git a/src/main/java/org/springframework/data/repository/query/parser/PartTree.java b/src/main/java/org/springframework/data/repository/query/parser/PartTree.java index 2126a832a..a125be7dd 100644 --- a/src/main/java/org/springframework/data/repository/query/parser/PartTree.java +++ b/src/main/java/org/springframework/data/repository/query/parser/PartTree.java @@ -185,7 +185,9 @@ public class PartTree implements Iterable { String[] split = split(source, "And"); for (String part : split) { - children.add(new Part(part, domainClass, alwaysIgnoreCase)); + if (StringUtils.hasText(part)) { + children.add(new Part(part, domainClass, alwaysIgnoreCase)); + } } } diff --git a/src/main/java/org/springframework/data/util/ClassTypeInformation.java b/src/main/java/org/springframework/data/util/ClassTypeInformation.java index cbcd48c67..944a52dbc 100644 --- a/src/main/java/org/springframework/data/util/ClassTypeInformation.java +++ b/src/main/java/org/springframework/data/util/ClassTypeInformation.java @@ -1,5 +1,5 @@ /* - * Copyright 2011 the original author or authors. + * Copyright 2011-2013 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. @@ -29,6 +29,7 @@ import java.util.Set; import java.util.WeakHashMap; import org.springframework.core.GenericTypeResolver; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; /** @@ -60,11 +61,13 @@ public class ClassTypeInformation extends TypeDiscoverer { * Simple factory method to easily create new instances of {@link ClassTypeInformation}. * * @param - * @param type + * @param type must not be {@literal null}. * @return */ public static TypeInformation from(Class type) { + Assert.notNull(type, "Type must not be null!"); + Reference> cachedReference = CACHE.get(type); TypeInformation cachedTypeInfo = cachedReference == null ? null : cachedReference.get(); @@ -80,10 +83,12 @@ public class ClassTypeInformation extends TypeDiscoverer { /** * Creates a {@link TypeInformation} from the given method's return type. * - * @param method + * @param method must not be {@literal null}. * @return */ public static TypeInformation fromReturnTypeOf(Method method) { + + Assert.notNull(method, "Method must not be null!"); return new ClassTypeInformation(method.getDeclaringClass()).createInfo(method.getGenericReturnType()); } diff --git a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java index 26dff01ef..b0334c733 100644 --- a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2012 the original author or authors. + * Copyright 2011-2013 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. @@ -17,6 +17,7 @@ package org.springframework.data.mapping; import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; +import static org.springframework.data.mapping.PropertyPath.*; import java.util.Iterator; import java.util.Map; @@ -307,6 +308,51 @@ public class PropertyPathUnitTests { assertThat(path.next().getSegment(), is("UUID")); } + /** + * @see DATACMNS-381 + */ + public void exposesPreviouslyReferencedPathInExceptionMessage() { + + exception.expect(PropertyReferenceException.class); + exception.expectMessage("bar"); // missing variable + exception.expectMessage("String"); // type + exception.expectMessage("Bar.user.name"); // previously referenced path + + PropertyPath.from("userNameBar", Bar.class); + } + + /** + * @see DATACMNS-387 + */ + @Test(expected = IllegalArgumentException.class) + public void rejectsNullSource() { + from(null, Foo.class); + } + + /** + * @see DATACMNS-387 + */ + @Test(expected = IllegalArgumentException.class) + public void rejectsEmptySource() { + from("", Foo.class); + } + + /** + * @see DATACMNS-387 + */ + @Test(expected = IllegalArgumentException.class) + public void rejectsNullClass() { + from("foo", (Class) null); + } + + /** + * @see DATACMNS-387 + */ + @Test(expected = IllegalArgumentException.class) + public void rejectsNullTypeInformation() { + from("foo", (TypeInformation) null); + } + private class Foo { String userName; diff --git a/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java b/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java index 9d96e469a..c1304c907 100644 --- a/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java +++ b/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java @@ -374,6 +374,18 @@ public class PartTreeUnitTests { assertThat(new PartTree("findByAnders", Product.class), is(notNullValue())); } + /** + * @see DATACMNS-387 + */ + @Test + public void buildsPartTreeFromEmptyPredicateCorrectly() { + + PartTree tree = new PartTree("findAllByOrderByLastnameAsc", User.class); + + assertThat(tree.getParts(), is(emptyIterable())); + assertThat(tree.getSort(), is(new Sort(Direction.ASC, "lastname"))); + } + private static void assertType(Iterable sources, Type type, String property) { assertType(sources, type, property, 1, true); } diff --git a/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java b/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java index 139941a26..cb57bbcf9 100644 --- a/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java +++ b/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java @@ -277,6 +277,14 @@ public class ClassTypeInformationUnitTests { assertThat(categoryIdInfo, is((TypeInformation) from(Long.class))); } + /** + * @see DATACMNS-387 + */ + @Test(expected = IllegalArgumentException.class) + public void rejectsNullClass() { + from(null); + } + static class StringMapContainer extends MapContainer { }