From 88aa072daf705326d2db051912f2cf11f60e0fd8 Mon Sep 17 00:00:00 2001 From: Kevin Brightwell Date: Tue, 30 May 2017 13:40:44 -0400 Subject: [PATCH 1/2] Fixes #105 - Could not find predicate methods that collide * Added `findGetterDescriptionForField(FieldDescription base)` to check against all known prefixes for methods that clash * Extended test case for FieldPropertyClash to include predicate case --- .../generator/BaseAssertionGenerator.java | 17 +- .../description/ClassDescription.java | 51 +++++- .../assertions/generator/util/ClassUtil.java | 2 +- .../generator/data/FieldPropertyClash.java | 20 +++ .../resources/FieldPropertyClash.expected.txt | 148 ++++++++++++++++++ 5 files changed, 222 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java b/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java index 81ef9838..ed7c12b3 100644 --- a/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java +++ b/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java @@ -469,19 +469,14 @@ protected void generateAssertionsForPublicFields(StringBuilder assertionsForPubl } private String assertionContentForField(FieldDescription field, ClassDescription classDescription) { - final TypeToken owningType = field.getOwningType(); - final String fieldName = field.getName(); - final String fieldNameCap = capitalize(field.getName()); - try { - Method m = owningType.getRawType().getMethod("get" + fieldNameCap); - if (classDescription.getGettersDescriptions().contains(new GetterDescription(fieldName, owningType, m))) { - return ""; - } - } catch (NoSuchMethodException nsme) { - // ignore it, let flow keep going + // Check for getter existing + GetterDescription getter = classDescription.findGetterDescriptionForField(field); + if (getter != null) { + return ""; } + final String fieldName = field.getName(); String assertionContent = baseAssertionContentFor(field, classDescription); // we reuse template for properties to have consistent assertions for property and field but change the way we get @@ -510,7 +505,7 @@ private String assertionContentForField(FieldDescription field, ClassDescription assertionContent = replace(assertionContent, PREDICATE, field.getPredicate()); assertionContent = replace(assertionContent, PREDICATE_NEG, field.getNegativePredicate()); } - assertionContent = replace(assertionContent, PROPERTY_WITH_UPPERCASE_FIRST_CHAR, fieldNameCap); + assertionContent = replace(assertionContent, PROPERTY_WITH_UPPERCASE_FIRST_CHAR, capitalize(field.getName())); assertionContent = replace(assertionContent, PROPERTY_SIMPLE_TYPE, field.getTypeName(false, false)); assertionContent = replace(assertionContent, PROPERTY_ASSERT_TYPE, diff --git a/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java b/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java index 15b63659..e6277e18 100644 --- a/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java +++ b/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java @@ -13,12 +13,11 @@ package org.assertj.assertions.generator.description; import com.google.common.reflect.TypeToken; +import org.apache.commons.lang3.StringUtils; import org.assertj.assertions.generator.util.ClassUtil; +import org.assertj.assertions.generator.util.StringUtil; -import java.util.Collection; -import java.util.Objects; -import java.util.Set; -import java.util.TreeSet; +import java.util.*; /** * @@ -101,6 +100,50 @@ public void addDeclaredGetterDescriptions(Collection declared public void addDeclaredFieldDescriptions(Set declaredFieldDescriptions) { this.declaredFieldsDescriptions.addAll(declaredFieldDescriptions); } + + public GetterDescription findGetterDescriptionForField(FieldDescription base) { + + final String capName = StringUtils.capitalize(base.getName()); + if (ClassUtil.isBoolean(base.getValueType())) { + // deal with predicates + + // Build a map for better look-up + Map fieldMap = new HashMap<>(); + for (GetterDescription getter: this.gettersDescriptions) { + fieldMap.put(getter.getOriginalMember().getName(), getter); + } + for (GetterDescription getter: this.declaredGettersDescriptions) { + fieldMap.put(getter.getOriginalMember().getName(), getter); + } + + for (String prefix: ClassUtil.PREDICATE_PREFIXES.keySet()) { + String propName = prefix + capName; + + GetterDescription getterDesc = fieldMap.get(propName); + if (getterDesc != null) { + return getterDesc; + } + } + } else { + + String propName = "get" + capName; + + for (GetterDescription desc: this.gettersDescriptions) { + if (Objects.equals(desc.getOriginalMember().getName(), propName)) { + return desc; + } + } + + for (GetterDescription desc: this.declaredGettersDescriptions) { + if (Objects.equals(desc.getOriginalMember().getName(), propName)) { + return desc; + } + } + } + + // wasn't found + return null; + } @Override public String toString() { diff --git a/src/main/java/org/assertj/assertions/generator/util/ClassUtil.java b/src/main/java/org/assertj/assertions/generator/util/ClassUtil.java index 4ed3befe..039d87ad 100644 --- a/src/main/java/org/assertj/assertions/generator/util/ClassUtil.java +++ b/src/main/java/org/assertj/assertions/generator/util/ClassUtil.java @@ -302,7 +302,7 @@ public static boolean isValidGetterName(String methodName) { static private final Pattern PREFIX_PATTERN; - static private final Map PREDICATE_PREFIXES; + static public final Map PREDICATE_PREFIXES; static private final Comparator LONGEST_TO_SHORTEST = new Comparator() { @Override diff --git a/src/test/java/org/assertj/assertions/generator/data/FieldPropertyClash.java b/src/test/java/org/assertj/assertions/generator/data/FieldPropertyClash.java index 3bb62381..a0b5d242 100644 --- a/src/test/java/org/assertj/assertions/generator/data/FieldPropertyClash.java +++ b/src/test/java/org/assertj/assertions/generator/data/FieldPropertyClash.java @@ -20,4 +20,24 @@ public class FieldPropertyClash { public String getString() { return ""; } + + // joel-costigliola/assertj-assertions-generator#105 + // Predicate properties were not properly discerned vs non-predicate + public boolean isBoolean; + public boolean isBoolean() { return false; } + + public boolean isNotBoolean() { return false; } + + + public boolean shouldNotBeSomewhere; + public boolean shouldNotBeSomewhere() { return shouldNotBeSomewhere; } + + // different "tenses" + public boolean willBeOutside; + public boolean willNotBeOutside() { return willBeOutside; } + + // + public boolean willNotBeUpsideDown; + public boolean willBeUpsideDown() { return !willNotBeUpsideDown; } + } diff --git a/src/test/resources/FieldPropertyClash.expected.txt b/src/test/resources/FieldPropertyClash.expected.txt index 1a2be51d..da79bf4e 100644 --- a/src/test/resources/FieldPropertyClash.expected.txt +++ b/src/test/resources/FieldPropertyClash.expected.txt @@ -28,6 +28,114 @@ public class FieldPropertyClashAssert extends AbstractObjectAssert Date: Thu, 1 Jun 2017 11:05:43 -0400 Subject: [PATCH 2/2] Address comments from #106 * Addresses comments from @joel-costigliola in #106 * Broke method into two: one for presence, one for getting the values * Added many more tests --- .gitignore | 2 + .../generator/BaseAssertionGenerator.java | 36 ++--- .../description/ClassDescription.java | 76 +++++----- .../description/FieldDescriptionTest.java | 136 +++++++++++++++++- 4 files changed, 184 insertions(+), 66 deletions(-) diff --git a/.gitignore b/.gitignore index b270863a..74f1fd65 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,5 @@ *.jar *.war *.ear +/Vagrantfile +/.vagrant/ diff --git a/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java b/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java index ed7c12b3..e85e4b7a 100644 --- a/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java +++ b/src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java @@ -12,29 +12,27 @@ */ package org.assertj.assertions.generator; -import com.google.common.collect.Sets; -import com.google.common.reflect.TypeToken; -import org.assertj.assertions.generator.Template.Type; -import org.assertj.assertions.generator.description.ClassDescription; -import org.assertj.assertions.generator.description.DataDescription; -import org.assertj.assertions.generator.description.FieldDescription; -import org.assertj.assertions.generator.description.GetterDescription; -import org.assertj.assertions.generator.util.ClassUtil; +import static com.google.common.collect.Sets.newHashSet; +import static java.lang.String.format; +import static org.apache.commons.lang3.StringUtils.*; +import static org.assertj.assertions.generator.Template.Type.ASSERT_CLASS; +import static org.assertj.assertions.generator.util.ClassUtil.getTypeDeclarationWithinPackage; import java.io.File; import java.io.FileWriter; import java.io.IOException; -import java.lang.reflect.Method; import java.nio.file.Paths; import java.util.*; import java.util.regex.Matcher; import java.util.regex.Pattern; -import static com.google.common.collect.Sets.newHashSet; -import static java.lang.String.format; -import static org.apache.commons.lang3.StringUtils.*; -import static org.assertj.assertions.generator.Template.Type.ASSERT_CLASS; -import static org.assertj.assertions.generator.util.ClassUtil.getTypeDeclarationWithinPackage; +import com.google.common.reflect.TypeToken; +import org.assertj.assertions.generator.Template.Type; +import org.assertj.assertions.generator.description.ClassDescription; +import org.assertj.assertions.generator.description.DataDescription; +import org.assertj.assertions.generator.description.FieldDescription; +import org.assertj.assertions.generator.description.GetterDescription; +import org.assertj.assertions.generator.util.ClassUtil; @SuppressWarnings("WeakerAccess") public class BaseAssertionGenerator implements AssertionGenerator, AssertionsEntryPointGenerator { @@ -372,12 +370,7 @@ private String generateAssertionEntryPointMethodsFor(final Set } private String determineBestEntryPointsAssertionsClassPackage(final Set classDescriptionSet) { - SortedSet packages = new TreeSet<>(new Comparator() { - @Override - public int compare(final String o1, final String o2) { - return o1.length() - o2.length(); - } - }); + SortedSet packages = new TreeSet<>(Comparator.comparing(String::length)); for (ClassDescription classDescription : classDescriptionSet) { packages.add(classDescription.getPackageName()); } @@ -471,8 +464,7 @@ protected void generateAssertionsForPublicFields(StringBuilder assertionsForPubl private String assertionContentForField(FieldDescription field, ClassDescription classDescription) { // Check for getter existing - GetterDescription getter = classDescription.findGetterDescriptionForField(field); - if (getter != null) { + if (classDescription.hasGetterForField(field)) { return ""; } diff --git a/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java b/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java index e6277e18..77a3842d 100644 --- a/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java +++ b/src/main/java/org/assertj/assertions/generator/description/ClassDescription.java @@ -12,12 +12,16 @@ */ package org.assertj.assertions.generator.description; +import java.util.Collection; +import java.util.Objects; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; +import java.util.stream.Stream; + import com.google.common.reflect.TypeToken; import org.apache.commons.lang3.StringUtils; import org.assertj.assertions.generator.util.ClassUtil; -import org.assertj.assertions.generator.util.StringUtil; - -import java.util.*; /** * @@ -100,49 +104,37 @@ public void addDeclaredGetterDescriptions(Collection declared public void addDeclaredFieldDescriptions(Set declaredFieldDescriptions) { this.declaredFieldsDescriptions.addAll(declaredFieldDescriptions); } - - public GetterDescription findGetterDescriptionForField(FieldDescription base) { - + + Stream getGetterStreamForField(FieldDescription base) { final String capName = StringUtils.capitalize(base.getName()); + + Stream getterStream = + Stream.concat(this.gettersDescriptions.stream(), + this.declaredGettersDescriptions.stream()) + .distinct() + .filter(getter -> Objects.equals(base.getValueType(), getter.getValueType())); + if (ClassUtil.isBoolean(base.getValueType())) { - // deal with predicates - - // Build a map for better look-up - Map fieldMap = new HashMap<>(); - for (GetterDescription getter: this.gettersDescriptions) { - fieldMap.put(getter.getOriginalMember().getName(), getter); - } - for (GetterDescription getter: this.declaredGettersDescriptions) { - fieldMap.put(getter.getOriginalMember().getName(), getter); - } - - for (String prefix: ClassUtil.PREDICATE_PREFIXES.keySet()) { - String propName = prefix + capName; - - GetterDescription getterDesc = fieldMap.get(propName); - if (getterDesc != null) { - return getterDesc; - } - } - } else { - - String propName = "get" + capName; - - for (GetterDescription desc: this.gettersDescriptions) { - if (Objects.equals(desc.getOriginalMember().getName(), propName)) { - return desc; - } - } - - for (GetterDescription desc: this.declaredGettersDescriptions) { - if (Objects.equals(desc.getOriginalMember().getName(), propName)) { - return desc; - } - } + // deal with predicates by building a set of all of the valid predicates available + Set validNames = ClassUtil.PREDICATE_PREFIXES.keySet() + .stream() + .map(prefix -> prefix + capName) + .collect(Collectors.toSet()); + + return getterStream.filter(getter -> validNames.contains(getter.getOriginalMember().getName())); } - // wasn't found - return null; + final String propName = "get" + capName; + + return getterStream.filter(getter -> Objects.equals(getter.getOriginalMember().getName(), propName)); + } + + public boolean hasGetterForField(FieldDescription base) { + return getGetterStreamForField(base).findAny().isPresent(); + } + + public Set findGettersForField(FieldDescription base) { + return getGetterStreamForField(base).collect(Collectors.toCollection(TreeSet::new)); } @Override diff --git a/src/test/java/org/assertj/assertions/generator/description/FieldDescriptionTest.java b/src/test/java/org/assertj/assertions/generator/description/FieldDescriptionTest.java index fe0ccb0f..1dfb77fd 100644 --- a/src/test/java/org/assertj/assertions/generator/description/FieldDescriptionTest.java +++ b/src/test/java/org/assertj/assertions/generator/description/FieldDescriptionTest.java @@ -12,13 +12,16 @@ */ package org.assertj.assertions.generator.description; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Arrays; +import java.util.Collections; + import com.google.common.reflect.TypeToken; import org.assertj.assertions.generator.data.Name; import org.assertj.assertions.generator.data.nba.Player; import org.junit.Test; -import static org.assertj.core.api.Assertions.assertThat; - public class FieldDescriptionTest { private static final TypeToken PLAYER_TYPE = TypeToken.of(Player.class); @@ -82,4 +85,133 @@ public void should_generate_readable_predicate_for_error_message() throws Except assertThat(fieldDescription.getPredicateForErrorMessagePart1()).isEqualTo("is disabled"); assertThat(fieldDescription.getPredicateForErrorMessagePart2()).isEqualTo("is not"); } + + private static final TypeToken FIELD_PROP_TYPE = TypeToken.of(FieldPropertyNames.class); + + private static class FieldPropertyNames { + public boolean isBoolean = true; + public boolean isBoolean() { + return isBoolean; + } + + public boolean isNotBoolean() { + return !isBoolean; + } + + public String stringProp; + + public String getStringProp() { + return stringProp; + } + + public int onlyField; + + public String badGetter; + public int getBadGetter() { + return -1; + } + } + + @Test + public void should_find_getter_method_for_predicate_field() throws Exception { + + fieldDescription = new FieldDescription(FieldPropertyNames.class.getDeclaredField("isBoolean"), FIELD_PROP_TYPE); + + GetterDescription posGetter = new GetterDescription("boolean", FIELD_PROP_TYPE, + FieldPropertyNames.class.getMethod("isBoolean")); + GetterDescription negGetter = new GetterDescription("boolean", FIELD_PROP_TYPE, + FieldPropertyNames.class.getMethod("isNotBoolean")); + + ClassDescription classDesc = new ClassDescription(FIELD_PROP_TYPE); + classDesc.addGetterDescriptions(Arrays.asList(posGetter, negGetter)); + + assertThat(classDesc.findGettersForField(fieldDescription)) + .as("found pos/neg getters") + .containsOnlyOnce(posGetter, negGetter); + + assertThat(classDesc.hasGetterForField(fieldDescription)) + .as("Correctly identifies there is a getter") + .isTrue(); + } + + @Test + public void should_find_getter_method_for_field() throws Exception { + + fieldDescription = new FieldDescription(FieldPropertyNames.class.getDeclaredField("stringProp"), FIELD_PROP_TYPE); + + GetterDescription getter = new GetterDescription("stringProp", FIELD_PROP_TYPE, + FieldPropertyNames.class.getMethod("getStringProp")); + + ClassDescription classDesc = new ClassDescription(FIELD_PROP_TYPE); + classDesc.addGetterDescriptions(Collections.singletonList(getter)); + + assertThat(classDesc.findGettersForField(fieldDescription)) + .as("found single getter") + .containsOnly(getter); + + assertThat(classDesc.hasGetterForField(fieldDescription)) + .as("Correctly identifies there is a getter") + .isTrue(); + } + + @Test + public void should_not_find_getter_method_for_mismatched_return_type() throws Exception { + + fieldDescription = new FieldDescription(FieldPropertyNames.class.getDeclaredField("badGetter"), FIELD_PROP_TYPE); + + GetterDescription getter = new GetterDescription("badGetter", FIELD_PROP_TYPE, + FieldPropertyNames.class.getMethod("getBadGetter")); + + ClassDescription classDesc = new ClassDescription(FIELD_PROP_TYPE); + classDesc.addGetterDescriptions(Collections.singletonList(getter)); + + assertThat(classDesc.findGettersForField(fieldDescription)) + .as("found no getters") + .isEmpty(); + + assertThat(classDesc.hasGetterForField(fieldDescription)) + .as("Correctly identifies there is no getter") + .isFalse(); + } + + + @Test + public void should_not_find_getter_method_for_missing_getter() throws Exception { + + fieldDescription = new FieldDescription(FieldPropertyNames.class.getDeclaredField("onlyField"), FIELD_PROP_TYPE); + + ClassDescription classDesc = new ClassDescription(FIELD_PROP_TYPE); + + assertThat(classDesc.findGettersForField(fieldDescription)) + .as("found no getters") + .isEmpty(); + + assertThat(classDesc.hasGetterForField(fieldDescription)) + .as("Correctly identifies there is no getter") + .isFalse(); + } + + // Given that everything is written via reflection, we can not actually get access to a + // non-existant field anymore. Thus, there is no need to test it! +// @Test +// public void should_not_find_getter_method_for_non_existant_field() throws Exception { +// +// } + + @Test(expected = NullPointerException.class) + public void should_fail_find_with_npe_for_null_field_desc() throws Exception { + + ClassDescription classDesc = new ClassDescription(FIELD_PROP_TYPE); + + classDesc.findGettersForField(null); + } + + @Test(expected = NullPointerException.class) + public void should_fail_has_with_npe_for_null_field_desc() throws Exception { + + ClassDescription classDesc = new ClassDescription(FIELD_PROP_TYPE); + + classDesc.hasGetterForField(null); + } + }