Skip to content

Commit

Permalink
Issue #108: Opened constructor and fixed NPE
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@derjust
derjust committed Jan 14, 2018
1 parent 4e568b0 commit 3b034ca
Show file tree
Hide file tree
Showing 8 changed files with 350 additions and 53 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
*.class
.project
.metadata
target
*.iml
.idea
Expand Down
24 changes: 20 additions & 4 deletions src/changes/changes.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,18 @@
<author email="zeeman@zeeman.de" >derjust</author>
</properties>
<body>
<release version="5.0.1" date="" description="Maintenance release" >
<release version="5.0.2" date="" description="Maintenance release">
<action dev="vitolimandibhrata" issue="40" type="add" date="2017-01-07">
Added support for Auditing
</action>
<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" >
Respecting DynamoDBOperations.batchSave()'s List&lt;FailedBatch&gt; return value and turn it into a BatchWriteException
</action>
Expand All @@ -25,15 +36,20 @@
Added Spring 5 / Spring-Data Kay support
</action>
</release>
<release version="4.5.1" date="2017-12-19">
<action dev="Michael Wyraz" type="fix" issue="91">
Add constructor to DynamoDBTemplate that takes a preconfigured DynamoDBMapper
<release version="4.5.3" date="2017-01-14" description="Backport of PR #108">
<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="4.5.2" date="2017-12-24" description="Backport of PR #106">
<action dev="Gaurav Rawat" type="fix" issue="91">>
Fixed false assertion introduced implementing #91
</action>
</release>
<release version="4.5.1" date="2017-12-19">
<action dev="Michael Wyraz" type="fix" issue="91">
Add constructor to DynamoDBTemplate that takes a preconfigured DynamoDBMapper
</action>
</release>
</body>
</document>
40 changes: 25 additions & 15 deletions src/main/java/org/socialsignin/spring/data/dynamodb/core/DynamoDBTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.util.Assert;

import com.amazonaws.services.dynamodbv2.AmazonDynamoDB;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapper;
Expand All @@ -36,36 +37,45 @@ public class DynamoDBTemplate implements DynamoDBOperations,ApplicationContextAw
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
Expand Down
67 changes: 52 additions & 15 deletions ...va/org/socialsignin/spring/data/dynamodb/mapping/event/AbstractDynamoDBEventListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,26 @@
* limitations under the License.
*/

import com.amazonaws.services.dynamodbv2.datamodeling.PaginatedQueryList;
import com.amazonaws.services.dynamodbv2.datamodeling.PaginatedScanList;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.ApplicationListener;
import org.springframework.core.GenericTypeResolver;

import java.util.List;

/**
* Base class to implement domain class specific {@link ApplicationListener}s.
*
* @author Michael Lavelle
*/
public abstract class AbstractDynamoDBEventListener<E> implements
ApplicationListener<DynamoDBMappingEvent<?>> {
// inspired by Java8
private interface Consumer<T> {
void accept(T t);
}

private static final Logger LOG = LoggerFactory
.getLogger(AbstractDynamoDBEventListener.class);
Expand Down Expand Up @@ -55,25 +63,54 @@ public void onApplicationEvent(DynamoDBMappingEvent<?> event) {
@SuppressWarnings("unchecked")
E source = (E) event.getSource();

// Check for matching domain type and invoke callbacks
if (source != null && !domainClass.isAssignableFrom(source.getClass())) {
if (source == null) {
return;
}

if (event instanceof BeforeSaveEvent) {
onBeforeSave(source);
} else if (event instanceof AfterSaveEvent) {
onAfterSave(source);
} else if (event instanceof BeforeDeleteEvent) {
onBeforeDelete(source);
} else if (event instanceof AfterDeleteEvent) {
onAfterDelete(source);
} else if (event instanceof AfterLoadEvent) {
onAfterLoad(source);
} else if (event instanceof AfterScanEvent) {
onAfterScan(source);
if (event instanceof AfterScanEvent) {
if (source instanceof PaginatedScanList) {
publishEachElement((PaginatedScanList<E>)source, new Consumer<E>() {
@Override
public void accept(E e) {
onAfterScan(e);
}
});
}
} else if (event instanceof AfterQueryEvent) {
onAfterQuery(source);
if (source instanceof PaginatedQueryList) {
publishEachElement((PaginatedQueryList<E>)source, new Consumer<E>() {
@Override
public void accept(E e) {
onAfterQuery(e);
}
});
}
}
// Check for matching domain type and invoke callbacks
else if (domainClass.isAssignableFrom(source.getClass())) {
if (event instanceof BeforeSaveEvent) {
onBeforeSave(source);
} else if (event instanceof AfterSaveEvent) {
onAfterSave(source);
} else if (event instanceof BeforeDeleteEvent) {
onBeforeDelete(source);
} else if (event instanceof AfterDeleteEvent) {
onAfterDelete(source);
} else if (event instanceof AfterLoadEvent) {
onAfterLoad(source);
} else {
assert false;
}
} else {
assert false;
}
}

private void publishEachElement(List<E> list, Consumer<E> publishMethod) {
for(E o : list) {
if (domainClass.isAssignableFrom(o.getClass())) {
publishMethod.accept(o);
}
}
}

Expand Down
23 changes: 11 additions & 12 deletions .../org/socialsignin/spring/data/dynamodb/mapping/event/ValidatingDynamoDBEventListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,17 @@
* limitations under the License.
*/

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.util.Assert;

import javax.validation.ConstraintViolation;
import javax.validation.ConstraintViolationException;
import javax.validation.Validator;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.util.Assert;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;


/**
Expand All @@ -47,7 +46,7 @@ public class ValidatingDynamoDBEventListener extends AbstractDynamoDBEventListen
* @param validator must not be {@literal null}.
*/
public ValidatingDynamoDBEventListener(Validator validator) {
Assert.notNull(validator);
Assert.notNull(validator, "validator must not be null!");
this.validator = validator;
}

Expand All @@ -60,16 +59,16 @@ public void onBeforeSave(Object source) {

LOG.debug("Validating object: {}", source);

List<String> messages = new ArrayList<String>();
List<String> messages = new ArrayList<>();
Set<ConstraintViolation<Object>> violations = validator.validate(source);
Set<ConstraintViolation<?>> genericViolationSet = new HashSet<ConstraintViolation<?>>();
if (!violations.isEmpty()) {
Set<ConstraintViolation<?>> genericViolationSet = new HashSet<>();
for (ConstraintViolation<?> v : violations) {
genericViolationSet.add(v);
messages.add(v.toString());
}
LOG.info("During object: {} validation violations found: {}", source, violations);
throw new ConstraintViolationException(messages.toString(),genericViolationSet);
throw new ConstraintViolationException(messages.toString(), genericViolationSet);
}
}
}
38 changes: 31 additions & 7 deletions src/test/java/org/socialsignin/spring/data/dynamodb/core/DynamoDBTemplateUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,42 +3,66 @@
import java.util.ArrayList;
import java.util.List;

import com.amazonaws.services.dynamodbv2.AmazonDynamoDB;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapper;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig.TableNameResolver;

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.runners.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 com.amazonaws.services.dynamodbv2.AmazonDynamoDB;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapper;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig;
import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig.TableNameResolver;

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);

Expand Down
Loading

0 comments on commit 3b034ca

Please sign in to comment.