From 7ead2676a7d398c5d139f70343e3ac4467b98ca9 Mon Sep 17 00:00:00 2001 From: chasegawa Date: Thu, 2 Sep 2021 11:25:17 -0700 Subject: [PATCH] SHIBUI-2050 Fixing test issues --- .../domain/resolvers/SvnMetadataResource.java | 20 +++-- .../admin/ui/security/model/Group.java | 1 + .../admin/ui/security/model/User.java | 2 +- .../listener/GroupUpdatedEntityListener.java | 9 +- .../listener/UserUpdatedEntityListener.java | 29 ++++--- .../admin/ui/AbstractBaseDataJpaTest.groovy | 9 ++ .../repository/FilterRepositoryTests.groovy | 21 ++--- .../repository/GroupsRepositoryTests.groovy | 11 ++- .../security/service/UserServiceTests.groovy | 87 +++++++++---------- 9 files changed, 98 insertions(+), 91 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/SvnMetadataResource.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/SvnMetadataResource.java index 06f039bc6..7e34fe8c8 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/SvnMetadataResource.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/SvnMetadataResource.java @@ -16,19 +16,21 @@ @EqualsAndHashCode public class SvnMetadataResource { - private String repositoryURL; + private String password; - private String workingCopyDirectory; + private String proxyHost; - private String resourceFile; + private String proxyPassword; - private String username; + private String proxyPort; - private String password; + private String proxyUserName; - private String proxyHost; + private String repositoryURL; - private String proxyUserName; + private String resourceFile; - private String proxyPassword; -} + private String username; + + private String workingCopyDirectory; +} \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java index 6a986040c..c56c403ab 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java @@ -78,6 +78,7 @@ public void registerLoader(ILazyLoaderHelper lazyLoaderHelper) { public Set getOwnedItems() { if (lazyLoaderHelper != null) { lazyLoaderHelper.loadOwnedItems(this); + lazyLoaderHelper = null; } return ownedItems; } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java index 70dc1ccc7..81d9505c0 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java @@ -71,7 +71,6 @@ public class User extends AbstractAuditable implements Owner, Ownable { private Set roles = new HashSet<>(); @EqualsAndHashCode.Exclude - // @JsonIgnore @Transient private Set userGroups = new HashSet<>(); @@ -130,6 +129,7 @@ public String getRole() { public Set getUserGroups() { if (lazyLoaderHelper != null) { lazyLoaderHelper.loadGroups(this); + lazyLoaderHelper = null; } return userGroups; } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/GroupUpdatedEntityListener.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/GroupUpdatedEntityListener.java index 319624fe9..86d3875ef 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/GroupUpdatedEntityListener.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/GroupUpdatedEntityListener.java @@ -4,6 +4,7 @@ import javax.persistence.PostLoad; import javax.persistence.PostPersist; +import javax.persistence.PostRemove; import javax.persistence.PostUpdate; import org.springframework.beans.factory.annotation.Autowired; @@ -11,6 +12,8 @@ import edu.internet2.tier.shibboleth.admin.ui.security.model.Group; import edu.internet2.tier.shibboleth.admin.ui.security.model.Ownership; import edu.internet2.tier.shibboleth.admin.ui.security.repository.OwnershipRepository; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; public class GroupUpdatedEntityListener implements ILazyLoaderHelper { private static OwnershipRepository ownershipRepository; @@ -26,6 +29,7 @@ public void init(OwnershipRepository repo) { @PostPersist @PostUpdate @PostLoad + @PostRemove public synchronized void groupSavedOrFetched(Group group) { // Because of the JPA spec, the listener can't do queries in the callback, so we force lazy loading through // another callback to this at the time that the owned items are needed @@ -33,10 +37,11 @@ public synchronized void groupSavedOrFetched(Group group) { } @Override + @Transactional(propagation = Propagation.REQUIRES_NEW) public void loadOwnedItems(Group group) { - group.registerLoader(null); // once loaded, remove the helper from the group Set ownedItems = ownershipRepository.findAllByOwner(group); - group.setOwnedItems(ownedItems); + group.setOwnedItems(ownedItems); + group.registerLoader(null); // once loaded, remove the helper from the group } } \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/UserUpdatedEntityListener.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/UserUpdatedEntityListener.java index fcbcfa68f..1d4e54d0c 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/UserUpdatedEntityListener.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/listener/UserUpdatedEntityListener.java @@ -1,21 +1,20 @@ package edu.internet2.tier.shibboleth.admin.ui.security.model.listener; -import java.util.HashSet; -import java.util.Set; - -import javax.persistence.PostLoad; -import javax.persistence.PostPersist; -import javax.persistence.PostUpdate; - -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.transaction.annotation.Propagation; -import org.springframework.transaction.annotation.Transactional; - import edu.internet2.tier.shibboleth.admin.ui.security.model.Group; import edu.internet2.tier.shibboleth.admin.ui.security.model.Ownership; import edu.internet2.tier.shibboleth.admin.ui.security.model.User; import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository; import edu.internet2.tier.shibboleth.admin.ui.security.repository.OwnershipRepository; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import javax.persistence.PostLoad; +import javax.persistence.PostPersist; +import javax.persistence.PostRemove; +import javax.persistence.PostUpdate; +import java.util.HashSet; +import java.util.Set; public class UserUpdatedEntityListener implements ILazyLoaderHelper { private static GroupsRepository groupRepository; @@ -33,15 +32,16 @@ public void init(OwnershipRepository repo, GroupsRepository groupRepo) { @PostPersist @PostUpdate @PostLoad - @Transactional(propagation = Propagation.REQUIRES_NEW) + @PostRemove public synchronized void userSavedOrFetched(User user) { // Because of the JPA spec, the listener can't do queries in the callback, so we force lazy loading through // another callback to this at the time that the groups are needed user.registerLoader(this); } - + + @Override + @Transactional(propagation = Propagation.REQUIRES_NEW) public void loadGroups(User user) { - user.setLazyLoaderHelper(null); Set ownerships = ownershipRepository.findAllGroupsForUser(user.getUsername()); HashSet groups = new HashSet<>(); final boolean isAdmin = user.getRole().equals("ROLE_ADMIN"); @@ -52,5 +52,6 @@ public void loadGroups(User user) { groups.add(userGroup); }); user.setGroups(groups); + user.setLazyLoaderHelper(null); } } \ No newline at end of file diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/AbstractBaseDataJpaTest.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/AbstractBaseDataJpaTest.groovy index eed734ea1..1668f757e 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/AbstractBaseDataJpaTest.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/AbstractBaseDataJpaTest.groovy @@ -15,11 +15,16 @@ import org.springframework.test.context.ContextConfiguration import org.springframework.transaction.annotation.Transactional import spock.lang.Specification +import javax.persistence.EntityManager + @DataJpaTest @ContextConfiguration(classes = [BaseDataJpaTestConfiguration]) @EnableJpaRepositories(basePackages = ["edu.internet2.tier.shibboleth.admin.ui"]) @EntityScan("edu.internet2.tier.shibboleth.admin.ui") abstract class AbstractBaseDataJpaTest extends Specification { + @Autowired + EntityManager entityManager + @Autowired GroupServiceForTesting groupService @@ -65,6 +70,10 @@ abstract class AbstractBaseDataJpaTest extends Specification { createAdminUser() } + def cleanup() { + entityManager.clear() + } + protected createAdminUser() { if (userRepository.findByUsername("admin").isEmpty()) { Optional adminRole = roleRepository.findByName("ROLE_ADMIN") diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/FilterRepositoryTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/FilterRepositoryTests.groovy index ccc04ad61..4041b9be1 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/FilterRepositoryTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/FilterRepositoryTests.groovy @@ -8,22 +8,19 @@ import edu.internet2.tier.shibboleth.admin.ui.util.WithMockAdmin import org.springframework.beans.factory.annotation.Autowired import org.springframework.transaction.annotation.Transactional -import javax.persistence.EntityManager - class FilterRepositoryTests extends AbstractBaseDataJpaTest { @Autowired FilterRepository filterRepository - - @Autowired - EntityManager entityManager @Transactional def setup() { filterRepository.deleteAll() } - @WithMockAdmin - @Transactional + def cleanup() { + filterRepository.deleteAll() + } + def "EntityAttributesFilter hashcode works as desired"() { given: def entityAttributesFilterJson = '''{ @@ -58,16 +55,10 @@ class FilterRepositoryTests extends AbstractBaseDataJpaTest { when: def filter = new ObjectMapper().readValue(entityAttributesFilterJson.bytes, EntityAttributesFilter) def persistedFilter = filterRepository.save(filter) - entityManager.flush() - entityManager.clear() - - then: def item1 = filterRepository.findByResourceId("29a5d409-562a-41cd-acee-e9b3d7098d05") - entityManager.flush() - entityManager.clear() - def item2 = filterRepository.findByResourceId("29a5d409-562a-41cd-acee-e9b3d7098d05") - item1.hashCode() == item2.hashCode() + then: + item1.hashCode() == persistedFilter.hashCode() } @WithMockAdmin diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy index fc4b41ad6..2ce75002a 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy @@ -6,8 +6,9 @@ import edu.internet2.tier.shibboleth.admin.ui.security.model.Ownership import org.springframework.beans.factory.annotation.Autowired import org.springframework.dao.DataIntegrityViolationException import org.springframework.test.annotation.Rollback +import org.springframework.transaction.annotation.Transactional -import javax.transaction.Transactional +import javax.persistence.EntityManager /** * Tests to validate the repo and model for groups @@ -84,7 +85,7 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest { then: all.size() == 3 - savedGroup.ownedItems.size() == 3 + savedGroup.getOwnedItems().size() == 3 all.each { savedGroup.ownedItems.contains(it) } @@ -133,7 +134,7 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest { def gList = groupsRepo.findAll() then: - gList.size() == 0 + gList.isEmpty() // save check when: @@ -142,6 +143,8 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest { then: // Missing non-nullable field (name) should thrown error thrown(DataIntegrityViolationException) + entityManager.clear() + groupsRepo.findAll().isEmpty() } def "basic CRUD operations validated"() { @@ -191,7 +194,7 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest { groupsRepo.delete(groupFromDb2) then: - groupsRepo.findAll().size() == 0 + groupsRepo.findAll().isEmpty() when: def nothingThere = groupsRepo.findByResourceId(null) diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/UserServiceTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/UserServiceTests.groovy index f86c79764..44e389229 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/UserServiceTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/UserServiceTests.groovy @@ -5,43 +5,41 @@ import edu.internet2.tier.shibboleth.admin.ui.security.model.Group import edu.internet2.tier.shibboleth.admin.ui.security.model.Ownership import edu.internet2.tier.shibboleth.admin.ui.security.model.Role import edu.internet2.tier.shibboleth.admin.ui.security.model.User -import org.springframework.transaction.annotation.Transactional class UserServiceTests extends AbstractBaseDataJpaTest { - @Transactional + Role userRole + def setup() { - userRepository.findAll().forEach { - userService.delete(it.getUsername()) - } - userRepository.flush() + userRole = roleRepository.findByName("ROLE_USER").get() + } - groupService.clearAllForTesting() //leaves us just the admingroup + protected createAdminUser() { + // don't create the admin user } - + def "When creating user, user is set to the correct group"() { given: Group gb = new Group() gb.setResourceId("testingGroupBBB") gb.setName("Group BBB") gb = groupService.createGroup(gb) - - Optional userRole = roleRepository.findByName("ROLE_USER") - User user = new User(username: "someUser", roles:[userRole.get()], password: "foo") + + User user = new User(username: "someUser", roles:[userRole], password: "foo") user.setGroup(gb) - - when: + + when: User result = userService.save(user) - + then: result.groupId == "testingGroupBBB" result.username == "someUser" result.userGroups.size() == 1 - + // Raw check that the DB is correct for ownership Set users = ownershipRepository.findUsersByOwner(gb) users.size() == 1 users[0].ownedId == "someUser" - + // Validate that loading the group has the correct list as well Group g = groupService.find("testingGroupBBB") g.ownedItems.size() == 1 @@ -53,31 +51,30 @@ class UserServiceTests extends AbstractBaseDataJpaTest { ga.setResourceId("testingGroup") ga.setName("Group A") ga = groupService.createGroup(ga) - + Group gb = new Group() gb.setResourceId("testingGroupBBB") gb.setName("Group BBB") gb = groupService.createGroup(gb) - - Optional userRole = roleRepository.findByName("ROLE_USER") - User user = new User(username: "someUser", roles:[userRole.get()], password: "foo") + + User user = new User(username: "someUser", roles:[userRole], password: "foo") user.setGroup(gb) User userInB = userService.save(user) - + when: userInB.setGroupId("testingGroup") // changing groups will happen by updating the user's groupid (from the ui) User result = userService.save(userInB) - + then: result.groupId == "testingGroup" result.username == "someUser" result.userGroups.size() == 1 - + // Raw check that the DB is correct for ownership Set users = ownershipRepository.findUsersByOwner(ga) users.size() == 1 users[0].ownedId == "someUser" - + // check db is correct for the previous group as well Set users2 = ownershipRepository.findUsersByOwner(gb) users2.size() == 0 @@ -85,86 +82,84 @@ class UserServiceTests extends AbstractBaseDataJpaTest { // Validate that loading the group has the correct list as well Group g = groupService.find("testingGroup") g.ownedItems.size() == 1 - + Group g2 = groupService.find("testingGroupBBB") g2.ownedItems.size() == 0 } - + def "logically try to match user controller test causing headaches"() { given: Group ga = new Group() ga.setResourceId("testingGroup") ga.setName("Group A") ga = groupService.createGroup(ga) - - Optional userRole = roleRepository.findByName("ROLE_USER") - User user = new User(username: "someUser", firstName: "Fred", lastName: "Flintstone", roles:[userRole.get()], password: "foo") + + User user = new User(username: "someUser", firstName: "Fred", lastName: "Flintstone", roles:[userRole], password: "foo") user.setGroup(ga) userService.save(user) - + when: User flintstoneUser = userRepository.findByUsername("someUser").get() flintstoneUser.setFirstName("Wilma") flintstoneUser.setGroupId("testingGroup") - + User result = userService.save(flintstoneUser) - + then: result.groupId == "testingGroup" result.username == "someUser" result.userGroups.size() == 1 result.firstName == "Wilma" } - + def "When creating user, user with multiple groups is saved correctly"() { given: Group ga = new Group() ga.setResourceId("testingGroup") ga.setName("Group A") ga = groupService.createGroup(ga) - + Group gb = new Group() gb.setResourceId("testingGroupBBB") gb.setName("Group BBB") gb = groupService.createGroup(gb) - - Optional userRole = roleRepository.findByName("ROLE_USER") + User user = new User().with( { it.username = "someUser" - it.roles = [userRole.get()] + it.roles = [userRole] it.password = "foo" it }) - + HashSet groups = new HashSet<>() groups.add(ga) groups.add(gb) user.setGroups(groups) - + when: def result = userService.save(user) - + then: result.userGroups.size() == 2 - + // Raw check that the DB is correct for ownership Set users = ownershipRepository.findUsersByOwner(ga) users.size() == 1 users[0].ownedId == "someUser" - + Set users2 = ownershipRepository.findUsersByOwner(gb) users2.size() == 1 users2[0].ownedId == "someUser" - + when: def userFromDb = userRepository.findById(result.id).get() - + then: userFromDb.getUserGroups().size() == 2 - + when: Group gbUpdated = groupService.find("testingGroupBBB") - + then: gbUpdated.ownedItems.size() == 1 }