Merge pull request #917 from SpineEventEngine/column-reader-npe-fix

`ColumnReader` `NullPointerException` when finding a `setSomething()` method that is not a property mutator
SpineEventEngine · Dec 14, 2018 · 479fd46 · 479fd46
2 parents 4d39747 + bd2d068
commit 479fd46
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 16 deletions.
diff --git a/server/src/main/java/io/spine/server/entity/storage/ColumnReader.java b/server/src/main/java/io/spine/server/entity/storage/ColumnReader.java
@@ -20,16 +20,20 @@
 
 package io.spine.server.entity.storage;
 
+import com.google.common.collect.ImmutableSet;
 import io.spine.server.entity.Entity;
 
 import java.beans.BeanInfo;
 import java.beans.IntrospectionException;
 import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.Method;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Objects;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Lists.newLinkedList;
 import static io.spine.server.entity.storage.Methods.getAnnotatedVersion;
 import static io.spine.util.Exceptions.newIllegalStateException;
@@ -63,7 +67,8 @@ private ColumnReader(Class<? extends Entity> entityClass) {
      * <p>The reader can be further used to {@linkplain ColumnReader#readColumns() obtain}
      * {@linkplain EntityColumn entity columns} for the given class.
      *
-     * @param entityClass {@link Entity} class for which to create the instance
+     * @param entityClass
+     *         {@link Entity} class for which to create the instance
      * @return new instance of {@code ColumnReader} for the specified class
      */
     static ColumnReader forClass(Class<? extends Entity> entityClass) {
@@ -80,8 +85,10 @@ static ColumnReader forClass(Class<? extends Entity> entityClass) {
      * <p>If the check for correctness fails, throws {@link IllegalStateException}.
      *
      * @return a {@code Collection} of {@link EntityColumn} corresponded to entity class
-     * @throws IllegalStateException if entity column definitions are incorrect
+     * @throws IllegalStateException
+     *         if entity column definitions are incorrect
      */
+    @SuppressWarnings("ConstantConditions")
     Collection<EntityColumn> readColumns() {
         BeanInfo entityDescriptor;
         try {
@@ -90,28 +97,31 @@ Collection<EntityColumn> readColumns() {
             throw new IllegalStateException(e);
         }
 
-        Collection<EntityColumn> entityColumns = newLinkedList();
-        for (PropertyDescriptor property : entityDescriptor.getPropertyDescriptors()) {
-            Method getter = property.getReadMethod();
-            boolean isEntityColumn = getAnnotatedVersion(getter).isPresent();
-            if (isEntityColumn) {
-                EntityColumn column = EntityColumn.from(getter);
-                entityColumns.add(column);
-            }
-        }
-
-        checkRepeatedColumnNames(entityColumns);
-        return entityColumns;
+        PropertyDescriptor[] propertyDescriptors = entityDescriptor.getPropertyDescriptors();
+        ImmutableSet<EntityColumn> columns = Arrays
+                .stream(propertyDescriptors)
+                .map(PropertyDescriptor::getReadMethod)
+                .filter(Objects::nonNull)
+                .filter(ColumnReader::isAnnotated)
+                .map(EntityColumn::from)
+                .collect(toImmutableSet());
+        checkRepeatedColumnNames(columns);
+        return columns;
     }
 
+    private static boolean isAnnotated(Method method) {
+        return getAnnotatedVersion(method).isPresent();
+    }
 
     /**
      * Ensures that the specified columns have no repeated names.
      *
      * <p>If the check fails, throws {@link IllegalStateException}.
      *
-     * @param columns     the columns to check
-     * @throws IllegalStateException if columns contain repeated names
+     * @param columns
+     *         the columns to check
+     * @throws IllegalStateException
+     *         if columns contain repeated names
      */
     private void checkRepeatedColumnNames(Iterable<EntityColumn> columns) {
         Collection<String> checkedNames = newLinkedList();

diff --git a/server/src/test/java/io/spine/server/entity/storage/ColumnReaderTest.java b/server/src/test/java/io/spine/server/entity/storage/ColumnReaderTest.java
@@ -22,6 +22,7 @@
 
 import com.google.common.testing.NullPointerTester;
 import com.google.common.testing.NullPointerTester.Visibility;
+import io.spine.server.entity.storage.given.ColumnsTestEnv.EntityWithASetterButNoGetter;
 import io.spine.server.entity.storage.given.ColumnsTestEnv.EntityWithColumnFromInterface;
 import io.spine.server.entity.storage.given.ColumnsTestEnv.EntityWithManyGetters;
 import io.spine.server.entity.storage.given.ColumnsTestEnv.EntityWithManyGettersDescendant;
@@ -114,6 +115,14 @@ void fromImplementedInterface() {
         }
     }
 
+    @Test
+    @DisplayName("not confuse a setter method with a property mutator")
+    void testSetterDeclaringEntity() {
+        ColumnReader columnReader = forClass(EntityWithASetterButNoGetter.class);
+        Collection<EntityColumn> entityColumns = columnReader.readColumns();
+        assertThat(entityColumns).isEmpty();
+    }
+
     @Nested
     @DisplayName("when extracting columns, ignore")
     class Ignore {

diff --git a/server/src/test/java/io/spine/server/entity/storage/given/ColumnsTestEnv.java b/server/src/test/java/io/spine/server/entity/storage/given/ColumnsTestEnv.java
@@ -53,6 +53,28 @@ public int getValue() {
         }
     }
 
+    /**
+     * An entity type which declares a {@linkplain #setSecretNumber(Integer) mutator method},
+     * however doesn't declare a respective accessor method.
+     *
+     * <p>{@code ColumnReader} should not get confused and assume that the mutator method is
+     * a property, and, therefore, a potential column.
+     */
+    @SuppressWarnings("unused") // Reflective access
+    public static class EntityWithASetterButNoGetter extends AbstractEntity<String, Any> {
+
+        private Integer secretNumber;
+
+        protected EntityWithASetterButNoGetter(String id) {
+            super(id);
+        }
+
+        @SuppressWarnings("WeakerAccess") // Required for a test
+        public void setSecretNumber(Integer secretNumber) {
+            this.secretNumber = secretNumber;
+        }
+    }
+
     @SuppressWarnings("unused")  // Reflective access
     public static class EntityWithManyGetters extends AbstractEntity<String, Any> {