From 739cfb4c0423ba0a531b474c4a32d0028a539838 Mon Sep 17 00:00:00 2001 From: Gary Wu Date: Wed, 2 Dec 2015 17:05:13 -0800 Subject: [PATCH] Refactor and fix TenantManage for repeated queries Refactor and fix TenantManage so that when repeated requests are made to getUsers(), the data is re-queried from MD-SAL. Change-Id: I7fd97d491ba7799a73d432bcdce12589c2bb4a21 Signed-off-by: Gary Wu --- .../nemo/user/tenantmanager/AAA.java | 6 +- .../nemo/user/tenantmanager/RegisterUser.java | 6 +- .../nemo/user/tenantmanager/TenantManage.java | 146 +++++++----------- .../nemo/user/tenantmanager/AAATest.java | 10 +- .../user/tenantmanager/RegisterUserTest.java | 14 +- .../user/tenantmanager/TenantManageTest.java | 40 ++--- 6 files changed, 87 insertions(+), 135 deletions(-) diff --git a/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/AAA.java b/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/AAA.java index 51e693e..3af90fc 100644 --- a/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/AAA.java +++ b/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/AAA.java @@ -36,12 +36,11 @@ public class AAA { private String checkUser(UserId userId, UserName userName, UserPassword userPassword, UserRoleName userRoleName) { - tenantManage.fetchUsers(); final Map users = tenantManage.getUsers(); String errorInfo = null; - final User user = users.get(userId); + final User user = (users != null) ? users.get(userId) : null; - if (users.containsKey(userId) && user != null) { + if (user != null) { if (!user.getUserName().equals(userName)) { errorInfo = "The user name is not right."; } else if (!user.getUserPassword().equals(userPassword)) { @@ -49,6 +48,7 @@ public class AAA { } else if (!user.getUserRole().equals(userRoleName)) { errorInfo = "The role is not right."; } + } else { errorInfo = "The user is not exist."; } diff --git a/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/RegisterUser.java b/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/RegisterUser.java index 5dbf6a0..4559649 100644 --- a/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/RegisterUser.java +++ b/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/RegisterUser.java @@ -30,12 +30,10 @@ public class RegisterUser { { String errorInfo = null; - tenantManage.fetchUserRoles(); Map userRoles = tenantManage.getUserRoles(); - tenantManage.fetchUsers(); Map users = tenantManage.getUsers(); - if (userRoles.isEmpty()) + if (userRoles == null || userRoles.isEmpty()) { errorInfo = "There are no roles be defined."; } @@ -43,7 +41,7 @@ public class RegisterUser { { if (userRoles.containsKey(input.getUserRole())) { - if (users.containsKey(input.getUserId())) + if (users != null && users.containsKey(input.getUserId())) { errorInfo = "The user has been registered."; } diff --git a/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/TenantManage.java b/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/TenantManage.java index cbf22e1..6bacfb9 100644 --- a/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/TenantManage.java +++ b/nemo-impl/src/main/java/org/opendaylight/nemo/user/tenantmanager/TenantManage.java @@ -8,11 +8,8 @@ package org.opendaylight.nemo.user.tenantmanager; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import org.opendaylight.controller.md.sal.binding.api.DataBroker; import org.opendaylight.controller.md.sal.binding.api.WriteTransaction; @@ -31,12 +28,13 @@ import org.opendaylight.yangtools.yang.binding.InstanceIdentifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.base.Function; import com.google.common.base.Optional; +import com.google.common.collect.Maps; import com.google.common.util.concurrent.CheckedFuture; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; -import com.google.common.util.concurrent.SettableFuture; /** * Created by z00293636 on 2015/8/29. @@ -46,8 +44,6 @@ import com.google.common.util.concurrent.SettableFuture; public class TenantManage { private static final Logger LOG = LoggerFactory.getLogger(TenantManage.class); private DataBroker dataBroker; - private final SettableFuture> userRoleListFuture = SettableFuture.create(); - private final SettableFuture> usersListFuture = SettableFuture.create(); private User user; public TenantManage(DataBroker dataBroker) @@ -55,121 +51,87 @@ public class TenantManage { this.dataBroker = dataBroker; } - private void setUserRoleList(List userRoleList) - { - this.userRoleListFuture.set(userRoleList); - } - - private void setUserList(List userList) - { - this.usersListFuture.set(userList); - } - private void setUser(User user) { this.user = user; } - public List getUserRoleList() { - try { - return userRoleListFuture.get(1, TimeUnit.SECONDS); - } catch (InterruptedException | ExecutionException | TimeoutException e) { - LOG.error("Cannot read role information.", e); - return null; - } + public User getUser() + { + return user; } /** * - * @return Map from UserRoleName to UserRole. If no roles exist, an empty (not-null) map is returned. + * @return null if an error was encountered, or an empty map if there was no + * error but no data was retrieved. */ public Map getUserRoles() { - final Map map = new HashMap<>(); - final List userRoleList = getUserRoleList(); - if (userRoleList != null) { - for (UserRole role : userRoleList) { - map.put(role.getRoleName(), role); - } - } - return map; - } - public List getUsersList() { + InstanceIdentifier userRolesInsId = InstanceIdentifier.builder(UserRoles.class).build(); + ListenableFuture> userRolesFuture = this.dataBroker.newReadOnlyTransaction().read( + LogicalDatastoreType.CONFIGURATION, userRolesInsId); + + final Optional userRolesOpt; try { - return usersListFuture.get(1, TimeUnit.SECONDS); - } catch (InterruptedException | ExecutionException | TimeoutException e) { - LOG.error("Cannot read user information.", e); + // TODO: consider time out here? + userRolesOpt = userRolesFuture.get(); + } catch (InterruptedException | ExecutionException e) { + LOG.error("Cannot read role information.", e); return null; } - } - - /** - * - * @return Map from UserId to User. If no users exist, an empty (not-null) map is returned. - */ - public Map getUsers() - { - final Map map = new HashMap<>(); - final List userList = getUsersList(); - if (userList != null) { - for (User user : userList) { - map.put(user.getUserId(), user); - } - } - return map; - } - - public User getUser() - { - return user; - } - - public void fetchUserRoles(){ - - InstanceIdentifier userRolesInsId = InstanceIdentifier.builder(UserRoles.class).build(); - ListenableFuture> userRolesFuture = this.dataBroker.newReadOnlyTransaction().read(LogicalDatastoreType.CONFIGURATION, userRolesInsId); - Futures.addCallback(userRolesFuture, new FutureCallback>() { - @Override - public void onSuccess(Optional result) - { - setUserRoleList(result.get().getUserRole()); - } + // TODO: change to Java 8 lambda expressions + return userRolesOpt.transform(new Function>() { @Override - public void onFailure(Throwable t) - { - LOG.error("Can not read role information.", t); + public Map apply(UserRoles input) { + return Maps.uniqueIndex(input.getUserRole(), new Function() { + @Override + public UserRoleName apply(UserRole role) { + return role.getRoleName(); + } + }); } - }); + }).or(new HashMap()); } - public void fetchUsers(){ + /** + * + * @return null if an error was encountered, or an empty map if there was no + * error but no data was retrieved. + */ + public Map getUsers() { InstanceIdentifier usersInsId = InstanceIdentifier.builder(Users.class).build(); - ListenableFuture> usersFuture = dataBroker.newReadOnlyTransaction().read(LogicalDatastoreType.CONFIGURATION, usersInsId); - Futures.addCallback(usersFuture, new FutureCallback>() { - @Override - public void onSuccess(Optional result) - { - setUserList(result.get().getUser()); - } + ListenableFuture> usersFuture = dataBroker.newReadOnlyTransaction().read( + LogicalDatastoreType.CONFIGURATION, usersInsId); + + final Optional usersOpt; + try { + // TODO: consider time out here? + usersOpt = usersFuture.get(); + } catch (InterruptedException | ExecutionException e) { + LOG.error("Cannot read user information.", e); + return null; + } + // TODO: change to Java 8 lambda expressions + return usersOpt.transform(new Function>() { @Override - public void onFailure(Throwable t) - { - LOG.error("Can not read users information.", t); + public Map apply(Users input) { + return Maps.uniqueIndex(input.getUser(), new Function() { + @Override + public UserId apply(User user) { + return user.getUserId(); + } + }); } - }); + }).or(new HashMap()); } public void fetchVNSpace(UserId userId) { - fetchUsers(); final Map users = getUsers(); - - User user = users.get(userId); - if (users.containsKey(userId) && user != null) { - setUser(user); - } + setUser((users != null) ? users.get(userId) : null); } public void addUser(RegisterUserInput registerUserInput){ diff --git a/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/AAATest.java b/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/AAATest.java index 24f50ac..7fd37f0 100644 --- a/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/AAATest.java +++ b/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/AAATest.java @@ -13,15 +13,19 @@ */ package org.opendaylight.nemo.user.tenantmanager; -import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; + +import java.util.HashMap; + import junit.framework.TestCase; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.common.rev151010.UserId; +import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.intent.rev151010.users.User; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.user.rev151010.UserInstance; /** @@ -43,11 +47,9 @@ public class AAATest extends TestCase { @Test public void testCheckUser() throws Exception { - doNothing().when(tenantManage).fetchUsers(); - when(tenantManage.getUsersList()).thenReturn(null); + when(tenantManage.getUsers()).thenReturn(new HashMap()); String acutal = aaa.checkUser(userInstance); String expected = "The user is not exist."; - verify(tenantManage).fetchUsers(); verify(tenantManage).getUsers(); Assert.assertNotNull(aaa); Assert.assertEquals(expected,acutal); diff --git a/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/RegisterUserTest.java b/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/RegisterUserTest.java index 26ce176..b98400b 100644 --- a/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/RegisterUserTest.java +++ b/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/RegisterUserTest.java @@ -14,19 +14,19 @@ package org.opendaylight.nemo.user.tenantmanager; -import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.util.LinkedList; +import java.util.HashMap; import junit.framework.TestCase; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.common.rev151010.UserId; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.common.rev151010.UserRoleName; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.intent.rev151010.RegisterUserInput; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.intent.rev151010.users.User; @@ -51,15 +51,13 @@ public class RegisterUserTest extends TestCase { public void testRegisterUser() throws Exception { //no data test - doNothing().when(tenantManage).fetchUserRoles(); - when(tenantManage.getUserRoleList()).thenReturn(null);//return nothing - doNothing().when(tenantManage).fetchUsers(); - when(tenantManage.getUsersList()).thenReturn(null); + when(tenantManage.getUserRoles()).thenReturn(null);//return nothing + when(tenantManage.getUsers()).thenReturn(null); registerUser.registerUser(input); //data exists . and test other branch - when(tenantManage.getUserRoleList()).thenReturn(new LinkedList());//return nothing - when(tenantManage.getUsersList()).thenReturn(new LinkedList()); + when(tenantManage.getUserRoles()).thenReturn(new HashMap());//return nothing + when(tenantManage.getUsers()).thenReturn(new HashMap()); when(input.getUserRole()).thenReturn(mock(UserRoleName.class)); registerUser.registerUser(input); diff --git a/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/TenantManageTest.java b/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/TenantManageTest.java index f1ba81b..9d9b2df 100644 --- a/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/TenantManageTest.java +++ b/nemo-impl/src/test/java/org/opendaylight/nemo/user/tenantmanager/TenantManageTest.java @@ -19,8 +19,8 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.util.LinkedList; -import java.util.List; +import java.util.HashMap; +import java.util.Map; import junit.framework.TestCase; @@ -32,11 +32,13 @@ import org.opendaylight.controller.md.sal.binding.api.ReadOnlyTransaction; import org.opendaylight.controller.md.sal.binding.api.WriteTransaction; import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.common.rev151010.UserId; +import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.common.rev151010.UserRoleName; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.intent.rev151010.RegisterUserInput; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.intent.rev151010.users.User; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.nemo.user.rev151010.user.roles.UserRole; import org.opendaylight.yangtools.yang.binding.InstanceIdentifier; +import com.google.common.base.Optional; import com.google.common.util.concurrent.CheckedFuture; /** * Created by zhangmeng on 2015/11/20. @@ -44,33 +46,20 @@ import com.google.common.util.concurrent.CheckedFuture; public class TenantManageTest extends TestCase { private TenantManage tenantManage; private DataBroker dataBroker; - private List userRoleList; - private List usersList ; + private Map userRoles; + private Map users ; private User user; + @Override @Before public void setUp() throws Exception { - userRoleList = new LinkedList(); - usersList = new LinkedList(); + userRoles = new HashMap(); + users = new HashMap(); user = mock(User.class); dataBroker = mock(DataBroker.class); tenantManage = new TenantManage(dataBroker); } - @Test - public void testGetUserRoleList() throws Exception { - Assert.assertNotNull(userRoleList); - userRoleList = tenantManage.getUserRoleList(); - Assert.assertNotEquals(new LinkedList(),userRoleList); - } - - @Test - public void testGetUsersList() throws Exception { - Assert.assertNotNull(usersList); - usersList = tenantManage.getUsersList(); - Assert.assertNotEquals(new LinkedList(),usersList); - } - @Test public void testGetUser() throws Exception { user = tenantManage.getUser(); @@ -78,7 +67,7 @@ public class TenantManageTest extends TestCase { } @Test - public void testFetchUserRoles() throws Exception { + public void testGetUserRoles() throws Exception { //ListenableFuture> userRolesFuture = mock(ListenableFuture.class); CheckedFuture userRolesFuture = mock(CheckedFuture.class); ReadOnlyTransaction readOnlyTransaction = mock(ReadOnlyTransaction.class); @@ -86,20 +75,22 @@ public class TenantManageTest extends TestCase { // any(InstanceIdentifier.class))).thenReturn(userRolesFuture); when(dataBroker.newReadOnlyTransaction()).thenReturn(readOnlyTransaction); when(readOnlyTransaction.read(any(LogicalDatastoreType.class), any(InstanceIdentifier.class))).thenReturn(userRolesFuture); - tenantManage.fetchUserRoles(); + when(userRolesFuture.get()).thenReturn(Optional.absent()); + tenantManage.getUserRoles(); verify(dataBroker).newReadOnlyTransaction(); verify(readOnlyTransaction).read(any(LogicalDatastoreType.class), any(InstanceIdentifier.class)); Assert.assertNotNull(tenantManage); } @Test - public void testFetchUsers() throws Exception { + public void testGetUsers() throws Exception { CheckedFuture usersFuture = mock(CheckedFuture.class); ReadOnlyTransaction readOnlyTransaction = mock(ReadOnlyTransaction.class); when(dataBroker.newReadOnlyTransaction()).thenReturn(readOnlyTransaction); when(readOnlyTransaction.read(any(LogicalDatastoreType.class), any(InstanceIdentifier.class))).thenReturn(usersFuture); - tenantManage.fetchUsers(); + when(usersFuture.get()).thenReturn(Optional.absent()); + tenantManage.getUsers(); verify(dataBroker).newReadOnlyTransaction(); verify(readOnlyTransaction).read(any(LogicalDatastoreType.class), any(InstanceIdentifier.class)); Assert.assertNotNull(tenantManage); @@ -112,6 +103,7 @@ public class TenantManageTest extends TestCase { when(dataBroker.newReadOnlyTransaction()).thenReturn(readOnlyTransaction); when(readOnlyTransaction.read(any(LogicalDatastoreType.class), any(InstanceIdentifier.class))).thenReturn(usersFuture); + when(usersFuture.get()).thenReturn(Optional.absent()); tenantManage.fetchVNSpace(mock(UserId.class)); verify(dataBroker).newReadOnlyTransaction(); verify(readOnlyTransaction).read(any(LogicalDatastoreType.class), any(InstanceIdentifier.class)); -- 2.36.6