Issue #108: Opened constructor and fixed NPE (#115)

derjust · web-flow · commit c1840a0ddae0 · 2018-01-14T01:30:28.000-05:00
Issue #108: Opened constructor and fixed NPE In some use cases it is required to pass in all three parameters into `DynamoDBTemplate` - therefore made the internal constructor public (that was used by all other constructors anyway) Also passing in a combination of `null`/non-`null` parameters could lead to NPEs down the road - ie. by calling `getOverrideTableName`. The constructor now initalizes all fields with non-`null` objects and does not relay on AWS constructors to pick some proper default configs.
diff --git a/.gitignore b/.gitignore
@@ -1,5 +1,6 @@
 *.class
 .project
+.metadata
 target
 *.iml
 .idea
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
@@ -13,6 +13,9 @@
       <action dev="derjust" issue="103" type="add" date="2018-01-07">
         Update Marshaller to use DynamoDBTypeConverter (while keeping old inheritance intact)
       </action>
+      <action dev="derjust" issue="108" type="fix" date="2018-01-14">
+        Opened constructor and fixed NPE in case of missing DynamoDBConfig
+      </action>
     </release>
     <release version="5.0.1" date="2018-01-06" description="Maintenance release">
       <action dev="derjust" issue="68" type="fix" date="2017-12-01" >
diff --git a/src/main/java/org/socialsignin/spring/data/dynamodb/core/DynamoDBTemplate.java b/src/main/java/org/socialsignin/spring/data/dynamodb/core/DynamoDBTemplate.java
@@ -25,6 +25,7 @@
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.context.ApplicationEventPublisher;
+import org.springframework.util.Assert;
 
 import java.util.List;
 import java.util.Map;
@@ -36,36 +37,45 @@ public class DynamoDBTemplate implements DynamoDBOperations, ApplicationContextA
 	private final DynamoDBMapperConfig dynamoDBMapperConfig;
 	private ApplicationEventPublisher eventPublisher;
 	
+	/** Convenient constructor to use the default {@link DynamoDBMapper#DynamoDBMapper(AmazonDynamoDB)} */
 	public DynamoDBTemplate(AmazonDynamoDB amazonDynamoDB, DynamoDBMapperConfig dynamoDBMapperConfig)
 	{
 	    this(amazonDynamoDB, dynamoDBMapperConfig, null);
 	}
     
+	/** Convenient constructor to use the {@link DynamoDBMapperConfig.DEFAULT} */
+
     public DynamoDBTemplate(AmazonDynamoDB amazonDynamoDB, DynamoDBMapper dynamoDBMapper)
     {
         this(amazonDynamoDB, null, dynamoDBMapper);
     }
     
+    /** Convenient construcotr to thse the {@link DynamoDBMapperConfig.DEFAULT} and default {@link DynamoDBMapper#DynamoDBMapper(AmazonDynamoDB)} */
     public DynamoDBTemplate(AmazonDynamoDB amazonDynamoDB)
     {
         this(amazonDynamoDB, null, null);
     }
 	
-	DynamoDBTemplate(AmazonDynamoDB amazonDynamoDB, DynamoDBMapperConfig dynamoDBMapperConfig, DynamoDBMapper dynamoDBMapper) {
-       this.amazonDynamoDB = amazonDynamoDB;
-       if (dynamoDBMapper == null && dynamoDBMapperConfig == null) {
-            this.dynamoDBMapperConfig = DynamoDBMapperConfig.DEFAULT;
-            this.dynamoDBMapper = new DynamoDBMapper(amazonDynamoDB);
-            // Mapper must be null as it could not have been constructed without a Config
-            assert dynamoDBMapper == null;
-        } else {
-            this.dynamoDBMapperConfig = dynamoDBMapperConfig;
-            if (dynamoDBMapper == null) {
-                this.dynamoDBMapper = new DynamoDBMapper(amazonDynamoDB, dynamoDBMapperConfig);
-            } else {
-                this.dynamoDBMapper = dynamoDBMapper;
-            }
-        }
+    /** Initializes a new {@code DynamoDBTemplate}.
+     * The following combinations are valid:
+     * @param amazonDynamoDB must not be {@code null}
+     * @param dynamoDBMapperConfig can be {@code null} - {@link DynamoDBMapperConfig.DEFAULT} is used if {@code null} is passed in
+     * @param dynamoDBMapper can be {@code null} - {@link DynamoDBMapper#DynamoDBMapper(AmazonDynamoDB, DynamoDBMapperConfig)} is used if {@code null} is passed in */
+	public DynamoDBTemplate(AmazonDynamoDB amazonDynamoDB, DynamoDBMapperConfig dynamoDBMapperConfig, DynamoDBMapper dynamoDBMapper) {
+       Assert.notNull(amazonDynamoDB, "amazonDynamoDB must not be null!");
+	   this.amazonDynamoDB = amazonDynamoDB;
+	   
+	  if (dynamoDBMapperConfig == null) {
+		  this.dynamoDBMapperConfig = DynamoDBMapperConfig.DEFAULT;		  
+	  } else {
+		  this.dynamoDBMapperConfig = dynamoDBMapperConfig;
+	  }
+	  
+	  if (dynamoDBMapper == null) {
+          this.dynamoDBMapper = new DynamoDBMapper(amazonDynamoDB, dynamoDBMapperConfig);
+	  } else {
+          this.dynamoDBMapper = dynamoDBMapper;
+	  }
     }
 	
 	@Override
diff --git a/src/test/java/org/socialsignin/spring/data/dynamodb/core/DynamoDBTemplateUnitTest.java b/src/test/java/org/socialsignin/spring/data/dynamodb/core/DynamoDBTemplateUnitTest.java
@@ -4,40 +4,66 @@
 import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapper;
 import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig;
 import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig.TableNameResolver;
+import com.amazonaws.services.dynamodbv2.document.DynamoDB;
+
 import org.junit.Assert;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 import org.socialsignin.spring.data.dynamodb.domain.sample.Playlist;
 import org.socialsignin.spring.data.dynamodb.domain.sample.User;
+import org.springframework.context.ApplicationContext;
 
 import java.util.ArrayList;
 import java.util.List;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
 @RunWith(MockitoJUnitRunner.class)
 public class DynamoDBTemplateUnitTest {
-	
+	@Rule
+	public ExpectedException expectedException = ExpectedException.none();
 	@Mock
 	private DynamoDBMapper dynamoDBMapper;
 	@Mock
 	private AmazonDynamoDB dynamoDB;
 	@Mock
 	private DynamoDBMapperConfig dynamoDBMapperConfig;
+	@Mock
+	private ApplicationContext applicationContext;
 	
 	private DynamoDBTemplate dynamoDBTemplate;
 
 	@Before
 	public void setUp() {
 		this.dynamoDBTemplate = new DynamoDBTemplate(dynamoDB, dynamoDBMapperConfig, dynamoDBMapper);
+		this.dynamoDBTemplate.setApplicationContext(applicationContext);
+	}
+	
+	@Test
+	public void testConstructorMandatory() {
+		expectedException.expect(IllegalArgumentException.class);
+		expectedException.expectMessage("must not be null!");
+		new DynamoDBTemplate(null);
+	}
+
+	@Test
+	public void testConstructorOptionalAllNull() {
+		dynamoDBTemplate = new DynamoDBTemplate(dynamoDB, null, null);
+		
+		// check that the defaults are properly initialized - #108
+		String userTableName = dynamoDBTemplate.getOverriddenTableName(User.class, "UserTable");
+		assertEquals("user", userTableName);
 	}
 
 	@Test
-	public void testPreconfiguredDynamoDBMapper() {
+	public void testConstructorOptionalPreconfiguredDynamoDBMapper() {
 		// Introduced constructor via #91 should not fail its assert
 		DynamoDBTemplate usePreconfiguredDynamoDBMapper = new DynamoDBTemplate(dynamoDB, dynamoDBMapper);
 
diff --git a/src/test/java/org/socialsignin/spring/data/dynamodb/mapping/event/AbstractDynamoDBEventListenerTest.java b/src/test/java/org/socialsignin/spring/data/dynamodb/mapping/event/AbstractDynamoDBEventListenerTest.java
@@ -27,7 +27,7 @@ public class AbstractDynamoDBEventListenerTest {
     @Mock
     private PaginatedScanList<User> sampleScanList;
 
-    private AbstractDynamoDBEventListener underTest;
+    private AbstractDynamoDBEventListener<User> underTest;
 
     @Before
     public void setUp() {
@@ -42,7 +42,7 @@ public void setUp() {
 
     @Test
     public void testAfterDelete() {
-        underTest.onApplicationEvent(new AfterDeleteEvent(sampleEntity));
+        underTest.onApplicationEvent(new AfterDeleteEvent<User>(sampleEntity));
 
         verify(underTest).onAfterDelete(sampleEntity);
         verify(underTest, never()).onAfterLoad(any());
@@ -55,7 +55,7 @@ public void testAfterDelete() {
 
     @Test
     public void testAfterLoad() {
-        underTest.onApplicationEvent(new AfterLoadEvent(sampleEntity));
+        underTest.onApplicationEvent(new AfterLoadEvent<>(sampleEntity));
 
         verify(underTest, never()).onAfterDelete(any());
         verify(underTest).onAfterLoad(sampleEntity);
@@ -68,7 +68,7 @@ public void testAfterLoad() {
 
     @Test
     public void testAfterQuery() {
-        underTest.onApplicationEvent(new AfterQueryEvent(sampleQueryList));
+        underTest.onApplicationEvent(new AfterQueryEvent<User>(sampleQueryList));
 
         verify(underTest, never()).onAfterDelete(any());
         verify(underTest, never()).onAfterLoad(any());
@@ -81,7 +81,7 @@ public void testAfterQuery() {
 
     @Test
     public void testAfterSave() {
-        underTest.onApplicationEvent(new AfterSaveEvent(sampleEntity));
+        underTest.onApplicationEvent(new AfterSaveEvent<>(sampleEntity));
 
         verify(underTest, never()).onAfterDelete(any());
         verify(underTest, never()).onAfterLoad(any());
@@ -94,7 +94,7 @@ public void testAfterSave() {
 
     @Test
     public void testAfterScan() {
-        underTest.onApplicationEvent(new AfterScanEvent(sampleScanList));
+        underTest.onApplicationEvent(new AfterScanEvent<>(sampleScanList));
 
         verify(underTest, never()).onAfterDelete(any());
         verify(underTest, never()).onAfterLoad(any());
@@ -107,7 +107,7 @@ public void testAfterScan() {
 
     @Test
     public void testBeforeDelete() {
-        underTest.onApplicationEvent(new BeforeDeleteEvent(sampleEntity));
+        underTest.onApplicationEvent(new BeforeDeleteEvent<>(sampleEntity));
 
         verify(underTest, never()).onAfterDelete(any());
         verify(underTest, never()).onAfterLoad(any());
@@ -120,7 +120,7 @@ public void testBeforeDelete() {
 
     @Test
     public void testBeforeSave() {
-        underTest.onApplicationEvent(new BeforeSaveEvent(sampleEntity));
+        underTest.onApplicationEvent(new BeforeSaveEvent<>(sampleEntity));
 
         verify(underTest, never()).onAfterDelete(any());
         verify(underTest, never()).onAfterLoad(any());
diff --git a/src/test/java/org/socialsignin/spring/data/dynamodb/mapping/event/ValidatingDynamoDBEventListenerTest.java b/src/test/java/org/socialsignin/spring/data/dynamodb/mapping/event/ValidatingDynamoDBEventListenerTest.java
@@ -1,9 +1,5 @@
 package org.socialsignin.spring.data.dynamodb.mapping.event;
 
-import com.amazonaws.services.dynamodbv2.document.Expected;
-import net.bytebuddy.pool.TypePool;
-import org.hamcrest.core.CombinableMatcher;
-import org.hamcrest.core.StringContains;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -15,13 +11,10 @@
 
 import javax.validation.ConstraintViolation;
 import javax.validation.ConstraintViolationException;
-import javax.validation.Path;
 import javax.validation.Validator;
-import javax.validation.metadata.ConstraintDescriptor;
 
 import java.util.HashSet;
 import java.util.Set;
-import java.util.concurrent.ExecutionException;
 
 import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.containsString;

-Original file line number
+Diff line change
@@ @@ -1,5 +1,6 @@ @@
 *.class
 .project
 +.metadata
 target
 *.iml
 .idea