openstack
/
monasca-api
Code Issues Proposed changes

Rollback the transaction if an exception is thrown 

The transaction was being left open if a Notification with the
same name and tenant id already existed. This would cause different
views of the MySQL database in a cluster of Monasca APIs

Add tests using Mockito that the transaction is closed and rollback
or commit wsa called when required

Also fix the formatting of previously added code to match the
standards

Change-Id: Iaa4ca1e78dec3c76d139d77a7844b810267fd978
Closes-Bug: #1617097
This commit is contained in:
Craig Bryant 2016-08-25 18:27:21 -06:00
parent e98d29bd0e
commit be0b5874b1
2 changed files with 69 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
java/src/main/java/monasca/api/infrastructure/persistence/mysql/NotificationMethodMySqlRepoImpl.java
View File

@ -54,15 +54,15 @@ public class NotificationMethodMySqlRepoImpl implements NotificationMethodRepo {
@Override
public NotificationMethod create(String tenantId, String name,
String notificationMethodType, String address, int period) {
try (Handle h = db.open()) {
Handle h = db.open();
try {
h.begin();
if (getNotificationIdForTenantIdAndName(h,tenantId, name) != null)
throw new EntityExistsException(
"Notification method %s \"%s\" already exists.", tenantId, name);
if(!isValidNotificationMethodType(h, notificationMethodType)){
throw new EntityNotFoundException(
"Not a valid notification method type %s ", notificationMethodType);
if (!isValidNotificationMethodType(h, notificationMethodType)) {
throw new EntityNotFoundException("Not a valid notification method type %s ", notificationMethodType);
}
String id = UUID.randomUUID().toString();
@ -72,6 +72,11 @@ public class NotificationMethodMySqlRepoImpl implements NotificationMethodRepo {
LOG.debug("Creating notification method {} for {}", name, tenantId);
h.commit();
return new NotificationMethod(id, name, notificationMethodType, address, period);
} catch (RuntimeException e) {
h.rollback();
throw e;
} finally {
h.close();
}
}
@ -196,16 +201,16 @@ public class NotificationMethodMySqlRepoImpl implements NotificationMethodRepo {
@Override
public NotificationMethod update(String tenantId, String notificationMethodId, String name,
String notificationMethodType, String address, int period) {
try (Handle h = db.open()) {
Handle h = db.open();
try {
h.begin();
String notificationID = getNotificationIdForTenantIdAndName(h,tenantId, name);
if (notificationID != null && !notificationID.equalsIgnoreCase(notificationMethodId)) {
throw new EntityExistsException("Notification method %s \"%s\" already exists.",
tenantId, name);
}
if(!isValidNotificationMethodType(h, notificationMethodType)){
throw new EntityNotFoundException(
"Not a valid notification method type %s ", notificationMethodType);
if (!isValidNotificationMethodType(h, notificationMethodType)) {
throw new EntityNotFoundException("Not a valid notification method type %s ", notificationMethodType);
}
if (h
@ -217,6 +222,11 @@ public class NotificationMethodMySqlRepoImpl implements NotificationMethodRepo {
notificationMethodId);
h.commit();
return new NotificationMethod(notificationMethodId, name, notificationMethodType, address, period);
} catch (RuntimeException e) {
h.rollback();
throw e;
} finally {
h.close();
}
}
}

54
java/src/test/java/monasca/api/infrastructure/persistence/mysql/NotificationMethodMySqlRepositoryImplTest.java
View File

@ -14,6 +14,12 @@
package monasca.api.infrastructure.persistence.mysql;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;
@ -27,9 +33,12 @@ import monasca.api.domain.exception.EntityExistsException;
import monasca.api.domain.exception.EntityNotFoundException;
import monasca.api.domain.model.notificationmethod.NotificationMethod;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import org.skife.jdbi.v2.DBI;
import org.skife.jdbi.v2.Handle;
import org.testng.annotations.AfterClass;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
@ -42,6 +51,9 @@ public class NotificationMethodMySqlRepositoryImplTest {
private DBI db;
private Handle handle;
private NotificationMethodMySqlRepoImpl repo;
Handle realHandle;
Handle spyHandle;
private boolean shouldRollback = false;
private static final String NOTIFICATION_METHOD_WEBHOOK = "WEBHOOK";
private static final String NOTIFICATION_METHOD_EMAIL = "EMAIL";
@ -61,7 +73,25 @@ public class NotificationMethodMySqlRepositoryImplTest {
.execute("insert into notification_method_type ( name) values ('PAGERDUTY')");
handle
.execute("insert into notification_method_type ( name) values ('WEBHOOK')");
repo = new NotificationMethodMySqlRepoImpl(db, new PersistUtils());
final DBI mockDb = mock(DBI.class);
when(mockDb.open()).thenAnswer(new Answer<Handle>() {
public Handle answer(InvocationOnMock invocation) {
realHandle = db.open();
spyHandle = spy(realHandle);
// Ensure there is no active transaction when the handle is closed.
// Have to do this in the close() method because calling isInTransaction()
// after the close throws an exception
doAnswer(new Answer<Void>() {
public Void answer(InvocationOnMock invocation) {
assertFalse(spyHandle.isInTransaction());
realHandle.close();
return null;
}
}).when(spyHandle).close();
return spyHandle;
}
});
repo = new NotificationMethodMySqlRepoImpl(mockDb, new PersistUtils());
}
@AfterClass
@ -76,17 +106,33 @@ public class NotificationMethodMySqlRepositoryImplTest {
.execute("insert into notification_method (id, tenant_id, name, type, address, created_at, updated_at) values ('123', '444', 'MyEmail', 'EMAIL', 'a@b', NOW(), NOW())");
handle
.execute("insert into notification_method (id, tenant_id, name, type, address, created_at, updated_at) values ('124', '444', 'OtherEmail', 'EMAIL', 'a@b', NOW(), NOW())");
shouldRollback = false;
}
@AfterMethod
protected void afterMethod() {
if (shouldRollback) {
verify(spyHandle, times(1)).rollback();
}
}
public void shouldCreateEmail() {
NotificationMethod nmA = repo.create("555", "MyEmail", NOTIFICATION_METHOD_EMAIL, "a@b", 0);
verify(spyHandle, times(1)).commit();
NotificationMethod nmB = repo.findById("555", nmA.getId());
assertEquals(nmA, nmB);
}
@Test(expectedExceptions = EntityExistsException.class)
public void shouldNotCreateDuplicateEmail() {
shouldRollback = true;
repo.create("444", "MyEmail", NOTIFICATION_METHOD_EMAIL, "c@d", 0);
}
public void shouldCreateWebhookNonZeroPeriod() {
NotificationMethod nmA = repo.create("555", "MyWebhook", NOTIFICATION_METHOD_WEBHOOK, "http://localhost:33", 60);
verify(spyHandle, times(1)).commit();
NotificationMethod nmB = repo.findById("555", nmA.getId());
assertEquals(nmA, nmB);
@ -116,6 +162,7 @@ public class NotificationMethodMySqlRepositoryImplTest {
public void shouldUpdate() {
repo.update("444", "123", "Foo", NOTIFICATION_METHOD_EMAIL, "abc", 0);
verify(spyHandle, times(1)).commit();
NotificationMethod nm = repo.findById("444", "123");
assertEquals(nm, new NotificationMethod("123", "Foo", NOTIFICATION_METHOD_EMAIL, "abc", 0));
@ -124,6 +171,7 @@ public class NotificationMethodMySqlRepositoryImplTest {
public void shouldUpdateWebhookWithNonZeroPeriod() {
NotificationMethod nmOriginal = repo.create("555", "MyWebhook", NOTIFICATION_METHOD_WEBHOOK, "http://localhost:33", 0);
repo.update("555", nmOriginal.getId(), "MyWebhook", NOTIFICATION_METHOD_WEBHOOK, "http://localhost:33", 60);
verify(spyHandle, times(1)).commit();
NotificationMethod nmUpdated = repo.findById("555", nmOriginal.getId());
assertEquals(nmUpdated.getPeriod(), 60);
@ -141,6 +189,7 @@ public class NotificationMethodMySqlRepositoryImplTest {
public void shouldUpdateDuplicateWithSameValues() {
repo.update("444", "123", "Foo", NOTIFICATION_METHOD_EMAIL, "abc", 0);
verify(spyHandle, times(1)).commit();
NotificationMethod nm = repo.findById("444", "123");
assertEquals(nm, new NotificationMethod("123", "Foo", NOTIFICATION_METHOD_EMAIL, "abc", 0));
@ -148,8 +197,7 @@ public class NotificationMethodMySqlRepositoryImplTest {
@Test(expectedExceptions = EntityExistsException.class)
public void shouldNotUpdateDuplicateWithSameName() {
shouldRollback = true;
repo.update("444", "124", "MyEmail", NOTIFICATION_METHOD_EMAIL, "abc", 0);
}
}