From 813aadcee98bf05d6b1d60835f99b62a47bb15d5 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 1 Nov 2022 10:58:44 +0100 Subject: [PATCH 1/8] [Perf] Improve performance of OntologicalAuthenticationProvider tests. --- .../kbss/termit/config/SecurityConfig.java | 2 +- .../security/JwtAuthorizationFilter.java | 8 +- .../OntologicalAuthenticationProvider.java | 8 +- .../service/security/SecurityUtils.java | 2 +- .../kbss/termit/environment/Environment.java | 12 -- .../security/JwtAuthorizationFilterTest.java | 24 ++-- ...OntologicalAuthenticationProviderTest.java | 114 ++++++++---------- 7 files changed, 71 insertions(+), 99 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/termit/config/SecurityConfig.java b/src/main/java/cz/cvut/kbss/termit/config/SecurityConfig.java index 8be7e9a04..9ecaa260d 100644 --- a/src/main/java/cz/cvut/kbss/termit/config/SecurityConfig.java +++ b/src/main/java/cz/cvut/kbss/termit/config/SecurityConfig.java @@ -96,7 +96,7 @@ protected void configure(HttpSecurity http) throws Exception { .and().exceptionHandling().authenticationEntryPoint(new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED)) .and().cors().configurationSource(corsConfigurationSource()).and().csrf().disable() .addFilter(authenticationFilter()) - .addFilter(new JwtAuthorizationFilter(authenticationManager(), jwtUtils, securityUtils, userDetailsService, + .addFilter(new JwtAuthorizationFilter(authenticationManager(), jwtUtils, userDetailsService, objectMapper)); } diff --git a/src/main/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilter.java b/src/main/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilter.java index b790c66bc..33d0107f1 100644 --- a/src/main/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilter.java +++ b/src/main/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilter.java @@ -54,18 +54,14 @@ public class JwtAuthorizationFilter extends BasicAuthenticationFilter { private final JwtUtils jwtUtils; - private final SecurityUtils securityUtils; - private final TermItUserDetailsService userDetailsService; private final ObjectMapper objectMapper; public JwtAuthorizationFilter(AuthenticationManager authenticationManager, JwtUtils jwtUtils, - SecurityUtils securityUtils, TermItUserDetailsService userDetailsService, - ObjectMapper objectMapper) { + TermItUserDetailsService userDetailsService, ObjectMapper objectMapper) { super(authenticationManager); this.jwtUtils = jwtUtils; - this.securityUtils = securityUtils; this.userDetailsService = userDetailsService; this.objectMapper = objectMapper; } @@ -83,7 +79,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse final TermItUserDetails userDetails = jwtUtils.extractUserInfo(authToken); final TermItUserDetails existingDetails = userDetailsService.loadUserByUsername(userDetails.getUsername()); SecurityUtils.verifyAccountStatus(existingDetails.getUser()); - securityUtils.setCurrentUser(existingDetails); + SecurityUtils.setCurrentUser(existingDetails); refreshToken(authToken, response); chain.doFilter(request, response); } catch (JwtException e) { diff --git a/src/main/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProvider.java b/src/main/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProvider.java index 266b3a184..894824a71 100644 --- a/src/main/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProvider.java +++ b/src/main/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProvider.java @@ -39,8 +39,6 @@ public class OntologicalAuthenticationProvider implements AuthenticationProvider private static final Logger LOG = LoggerFactory.getLogger(OntologicalAuthenticationProvider.class); - private final SecurityUtils securityUtils; - private final TermItUserDetailsService userDetailsService; private final PasswordEncoder passwordEncoder; @@ -48,9 +46,7 @@ public class OntologicalAuthenticationProvider implements AuthenticationProvider private ApplicationEventPublisher eventPublisher; @Autowired - public OntologicalAuthenticationProvider(SecurityUtils securityUtils, TermItUserDetailsService userDetailsService, - PasswordEncoder passwordEncoder) { - this.securityUtils = securityUtils; + public OntologicalAuthenticationProvider(TermItUserDetailsService userDetailsService, PasswordEncoder passwordEncoder) { this.userDetailsService = userDetailsService; this.passwordEncoder = passwordEncoder; } @@ -69,7 +65,7 @@ public Authentication authenticate(Authentication authentication) { throw new BadCredentialsException("Provided credentials don't match."); } onLoginSuccess(userDetails.getUser()); - return securityUtils.setCurrentUser(userDetails); + return SecurityUtils.setCurrentUser(userDetails); } private static void verifyUsernameNotEmpty(String username) { diff --git a/src/main/java/cz/cvut/kbss/termit/service/security/SecurityUtils.java b/src/main/java/cz/cvut/kbss/termit/service/security/SecurityUtils.java index 183db968e..b5a039250 100644 --- a/src/main/java/cz/cvut/kbss/termit/service/security/SecurityUtils.java +++ b/src/main/java/cz/cvut/kbss/termit/service/security/SecurityUtils.java @@ -129,7 +129,7 @@ public boolean isAuthenticated() { * @param userDetails Details of the user to set as current * @return The generated authentication token */ - public AuthenticationToken setCurrentUser(TermItUserDetails userDetails) { + public static AuthenticationToken setCurrentUser(TermItUserDetails userDetails) { final AuthenticationToken token = new AuthenticationToken(userDetails.getAuthorities(), userDetails); token.setAuthenticated(true); diff --git a/src/test/java/cz/cvut/kbss/termit/environment/Environment.java b/src/test/java/cz/cvut/kbss/termit/environment/Environment.java index 052179fc0..24720b242 100644 --- a/src/test/java/cz/cvut/kbss/termit/environment/Environment.java +++ b/src/test/java/cz/cvut/kbss/termit/environment/Environment.java @@ -42,11 +42,9 @@ import java.io.InputStream; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.security.Principal; import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Optional; import java.util.stream.Collectors; public class Environment { @@ -87,16 +85,6 @@ public static void setCurrentUser(User user) { setCurrentUser(ua); } - /** - * Gets current user as security principal. - * - * @return Current user authentication as principal or {@code null} if there is no current user - */ - public static Optional getCurrentUserPrincipal() { - return SecurityContextHolder.getContext() != null ? - Optional.ofNullable(SecurityContextHolder.getContext().getAuthentication()) : Optional.empty(); - } - public static UserAccount getCurrentUser() { return currentUser; } diff --git a/src/test/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilterTest.java b/src/test/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilterTest.java index 83407919d..caec7d3d6 100644 --- a/src/test/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilterTest.java +++ b/src/test/java/cz/cvut/kbss/termit/security/JwtAuthorizationFilterTest.java @@ -27,11 +27,11 @@ import cz.cvut.kbss.termit.util.Configuration; import io.jsonwebtoken.Jwts; import io.jsonwebtoken.security.Keys; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.beans.factory.annotation.Autowired; @@ -41,6 +41,7 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.AuthenticationManager; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.userdetails.UsernameNotFoundException; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit.jupiter.SpringExtension; @@ -81,9 +82,6 @@ class JwtAuthorizationFilterTest { @Mock private TermItUserDetailsService detailsServiceMock; - @Mock - private SecurityUtils securityUtilsMock; - private JwtUtils jwtUtilsSpy; private ObjectMapper objectMapper; @@ -100,20 +98,22 @@ void setUp() { this.objectMapper = Environment.getObjectMapper(); this.signingKey = Keys.hmacShaKeyFor(config.getJwt().getSecretKey().getBytes(StandardCharsets.UTF_8)); this.jwtUtilsSpy = spy(new JwtUtils(objectMapper, config)); - this.sut = new JwtAuthorizationFilter(authManagerMock, jwtUtilsSpy, securityUtilsMock, detailsServiceMock, + this.sut = new JwtAuthorizationFilter(authManagerMock, jwtUtilsSpy, detailsServiceMock, objectMapper); } + @AfterEach + void tearDown() { + Environment.resetCurrentUser(); + } + @Test void doFilterInternalExtractsUserInfoFromJwtAndSetsUpSecurityContext() throws Exception { when(detailsServiceMock.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); generateJwtIntoRequest(); sut.doFilterInternal(mockRequest, mockResponse, chainMock); - final ArgumentCaptor captor = ArgumentCaptor.forClass(TermItUserDetails.class); - verify(securityUtilsMock).setCurrentUser(captor.capture()); - final TermItUserDetails userDetails = captor.getValue(); - assertEquals(user, userDetails.getUser()); + assertEquals(user, SecurityUtils.currentUser()); } private void generateJwtIntoRequest() { @@ -141,18 +141,20 @@ void doFilterInternalInvokesFilterChainAfterSuccessfulExtractionOfUserInfo() thr @Test void doFilterInternalLeavesEmptySecurityContextAndPassesRequestDownChainWhenAuthenticationIsMissing() throws Exception { + Environment.resetCurrentUser(); sut.doFilterInternal(mockRequest, mockResponse, chainMock); verify(chainMock).doFilter(mockRequest, mockResponse); - verify(securityUtilsMock, never()).setCurrentUser(any()); + assertNull(SecurityContextHolder.getContext().getAuthentication()); } @Test void doFilterInternalLeavesEmptySecurityContextAndPassesRequestDownChainWhenAuthenticationHasIncorrectFormat() throws Exception { + Environment.resetCurrentUser(); mockRequest.addHeader(HttpHeaders.AUTHORIZATION, generateJwt()); sut.doFilterInternal(mockRequest, mockResponse, chainMock); verify(chainMock).doFilter(mockRequest, mockResponse); - verify(securityUtilsMock, never()).setCurrentUser(any()); + assertNull(SecurityContextHolder.getContext().getAuthentication()); } @Test diff --git a/src/test/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProviderTest.java b/src/test/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProviderTest.java index bd8a4a9e0..261217e60 100644 --- a/src/test/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProviderTest.java +++ b/src/test/java/cz/cvut/kbss/termit/security/OntologicalAuthenticationProviderTest.java @@ -1,64 +1,69 @@ /** * TermIt Copyright (C) 2019 Czech Technical University in Prague *

- * This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at your option) any later version. + * This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public + * License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later + * version. *

- * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. + * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. *

- * You should have received a copy of the GNU General Public License along with this program. If not, see . + * You should have received a copy of the GNU General Public License along with this program. If not, see + * . */ package cz.cvut.kbss.termit.security; import cz.cvut.kbss.termit.environment.Generator; -import cz.cvut.kbss.termit.environment.config.TestSecurityConfig; import cz.cvut.kbss.termit.event.LoginFailureEvent; import cz.cvut.kbss.termit.event.LoginSuccessEvent; import cz.cvut.kbss.termit.model.UserAccount; -import cz.cvut.kbss.termit.persistence.dao.UserAccountDao; import cz.cvut.kbss.termit.security.model.TermItUserDetails; -import cz.cvut.kbss.termit.service.BaseServiceTestRunner; +import cz.cvut.kbss.termit.service.security.TermItUserDetailsService; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; -import org.mockito.Mockito; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.ComponentScan; -import org.springframework.context.event.EventListener; -import org.springframework.security.authentication.*; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.DisabledException; +import org.springframework.security.authentication.LockedException; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextImpl; import org.springframework.security.core.userdetails.UsernameNotFoundException; +import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; -import org.springframework.test.context.ContextConfiguration; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; import static org.junit.jupiter.api.Assertions.*; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.*; @Tag("security") -@ContextConfiguration(classes = {TestSecurityConfig.class, OntologicalAuthenticationProviderTest.TestConfiguration.class}) -class OntologicalAuthenticationProviderTest extends BaseServiceTestRunner { +@ExtendWith(MockitoExtension.class) +class OntologicalAuthenticationProviderTest { - @Autowired - private AuthenticationProvider sut; + @Mock + private TermItUserDetailsService userDetailsService; - @Autowired - private UserAccountDao userAccountDao; + @Spy + private PasswordEncoder passwordEncoder = new BCryptPasswordEncoder(); - @Autowired - private PasswordEncoder passwordEncoder; + @Mock + private ApplicationEventPublisher eventPublisher; - @Autowired - private Listener listener; + @InjectMocks + private OntologicalAuthenticationProvider sut; private UserAccount user; private String plainPassword; @@ -68,14 +73,13 @@ void setUp() { this.user = Generator.generateUserAccountWithPassword(); this.plainPassword = user.getPassword(); user.setPassword(passwordEncoder.encode(plainPassword)); - transactional(() -> userAccountDao.persist(user)); SecurityContextHolder.setContext(new SecurityContextImpl()); + sut.setApplicationEventPublisher(eventPublisher); } @AfterEach void tearDown() { SecurityContextHolder.setContext(new SecurityContextImpl()); - Mockito.reset(listener); } @Test @@ -83,6 +87,8 @@ void successfulAuthenticationSetsSecurityContext() { final Authentication auth = authentication(user.getUsername(), plainPassword); final SecurityContext context = SecurityContextHolder.getContext(); assertNull(context.getAuthentication()); + when(userDetailsService.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); + final Authentication result = sut.authenticate(auth); assertNotNull(SecurityContextHolder.getContext()); final TermItUserDetails details = @@ -98,6 +104,7 @@ private static Authentication authentication(String username, String password) { @Test void authenticateThrowsUserNotFoundExceptionForUnknownUsername() { final Authentication auth = authentication("unknownUsername", user.getPassword()); + when(userDetailsService.loadUserByUsername(anyString())).thenThrow(new UsernameNotFoundException("Unknown")); assertThrows(UsernameNotFoundException.class, () -> sut.authenticate(auth)); final SecurityContext context = SecurityContextHolder.getContext(); assertNull(context.getAuthentication()); @@ -105,6 +112,7 @@ void authenticateThrowsUserNotFoundExceptionForUnknownUsername() { @Test void authenticateThrowsBadCredentialsForInvalidPassword() { + when(userDetailsService.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); final Authentication auth = authentication(user.getUsername(), "unknownPassword"); assertThrows(BadCredentialsException.class, () -> sut.authenticate(auth)); final SecurityContext context = SecurityContextHolder.getContext(); @@ -120,30 +128,37 @@ void supportsUsernameAndPasswordAuthentication() { void authenticateThrowsAuthenticationExceptionForEmptyUsername() { final Authentication auth = authentication("", ""); final UsernameNotFoundException ex = assertThrows(UsernameNotFoundException.class, - () -> sut.authenticate(auth)); + () -> sut.authenticate(auth)); assertThat(ex.getMessage(), containsString("Username cannot be empty.")); + verify(userDetailsService, never()).loadUserByUsername(""); } @Test void successfulLoginEmitsLoginSuccessEvent() { + when(userDetailsService.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); final Authentication auth = authentication(user.getUsername(), plainPassword); sut.authenticate(auth); - verify(listener).onSuccess(any()); - assertEquals(user, listener.user); + final ArgumentCaptor captor = ArgumentCaptor.forClass(Object.class); + verify(eventPublisher).publishEvent(captor.capture()); + assertThat(captor.getValue(), instanceOf(LoginSuccessEvent.class)); + assertEquals(user, ((LoginSuccessEvent) captor.getValue()).getUser()); } @Test void failedLoginEmitsLoginFailureEvent() { + when(userDetailsService.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); final Authentication auth = authentication(user.getUsername(), "unknownPassword"); assertThrows(BadCredentialsException.class, () -> sut.authenticate(auth)); - verify(listener).onFailure(any()); - assertEquals(user, listener.user); + final ArgumentCaptor captor = ArgumentCaptor.forClass(Object.class); + verify(eventPublisher).publishEvent(captor.capture()); + assertThat(captor.getValue(), instanceOf(LoginFailureEvent.class)); + assertEquals(user, ((LoginFailureEvent) captor.getValue()).getUser()); } @Test void authenticateThrowsLockedExceptionForLockedUser() { user.lock(); - transactional(() -> userAccountDao.update(user)); + when(userDetailsService.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); final Authentication auth = authentication(user.getUsername(), plainPassword); final LockedException ex = assertThrows(LockedException.class, () -> sut.authenticate(auth)); assertEquals("Account of user " + user + " is locked.", ex.getMessage()); @@ -152,34 +167,9 @@ void authenticateThrowsLockedExceptionForLockedUser() { @Test void authenticationThrowsDisabledExceptionForDisabledUser() { user.disable(); - transactional(() -> userAccountDao.update(user)); + when(userDetailsService.loadUserByUsername(user.getUsername())).thenReturn(new TermItUserDetails(user)); final Authentication auth = authentication(user.getUsername(), plainPassword); final DisabledException ex = assertThrows(DisabledException.class, () -> sut.authenticate(auth)); assertEquals("Account of user " + user + " is disabled.", ex.getMessage()); } - - @org.springframework.boot.test.context.TestConfiguration - @ComponentScan(basePackages = "cz.cvut.kbss.termit.security") - public static class TestConfiguration { - @Bean - public Listener listener() { - return spy(new Listener()); - } - - } - - public static class Listener { - - private UserAccount user; - - @EventListener - public void onSuccess(LoginSuccessEvent event) { - this.user = event.getUser(); - } - - @EventListener - public void onFailure(LoginFailureEvent event) { - this.user = event.getUser(); - } - } } From 6039c773dfde089d570f08c353c60fc8526283a6 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 1 Nov 2022 12:44:35 +0100 Subject: [PATCH 2/8] [Perf] Improve performance of DataDao tests. --- .../java/cz/cvut/kbss/termit/persistence/dao/DataDaoTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/java/cz/cvut/kbss/termit/persistence/dao/DataDaoTest.java b/src/test/java/cz/cvut/kbss/termit/persistence/dao/DataDaoTest.java index 03ae826bf..a200b43ce 100644 --- a/src/test/java/cz/cvut/kbss/termit/persistence/dao/DataDaoTest.java +++ b/src/test/java/cz/cvut/kbss/termit/persistence/dao/DataDaoTest.java @@ -41,7 +41,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.test.annotation.DirtiesContext; import java.io.IOException; import java.net.URI; @@ -50,7 +49,6 @@ import static org.junit.jupiter.api.Assertions.*; -@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) class DataDaoTest extends BaseDaoTestRunner { private static final String FIRST_NAME_LABEL = "First name"; From aaacb143c2c836c70e676721d3198ecd7fa29c9b Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 1 Nov 2022 14:02:42 +0100 Subject: [PATCH 3/8] [Perf] Rewrite some service tests to Mock-based unit tests. --- .../termit/service/SystemInitializerTest.java | 113 ++++++++++-------- .../repository/UserRepositoryServiceTest.java | 105 ++++++++++------ 2 files changed, 130 insertions(+), 88 deletions(-) diff --git a/src/test/java/cz/cvut/kbss/termit/service/SystemInitializerTest.java b/src/test/java/cz/cvut/kbss/termit/service/SystemInitializerTest.java index 6825aabe8..aa127dfe8 100644 --- a/src/test/java/cz/cvut/kbss/termit/service/SystemInitializerTest.java +++ b/src/test/java/cz/cvut/kbss/termit/service/SystemInitializerTest.java @@ -1,17 +1,19 @@ /** * TermIt Copyright (C) 2019 Czech Technical University in Prague *

- * This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at your option) any later version. + * This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public + * License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later + * version. *

- * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. + * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. *

- * You should have received a copy of the GNU General Public License along with this program. If not, see . + * You should have received a copy of the GNU General Public License along with this program. If not, see + * . */ package cz.cvut.kbss.termit.service; -import cz.cvut.kbss.jopa.model.EntityManager; import cz.cvut.kbss.termit.environment.Generator; import cz.cvut.kbss.termit.model.UserAccount; import cz.cvut.kbss.termit.service.repository.UserRepositoryService; @@ -20,36 +22,42 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.security.crypto.password.PasswordEncoder; -import org.springframework.test.annotation.DirtiesContext; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; +import org.springframework.transaction.PlatformTransactionManager; import java.io.File; import java.io.IOException; import java.net.URI; import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; import java.util.List; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.*; -@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) -class SystemInitializerTest extends BaseServiceTestRunner { +@ExtendWith(MockitoExtension.class) +class SystemInitializerTest { private static final URI ADMIN_URI = URI.create(Vocabulary.ONTOLOGY_IRI_termit + "/system-admin-user"); - @Autowired - private Configuration config; + @Spy + private Configuration config = new Configuration(); - @Autowired + @Mock private UserRepositoryService userService; - @Autowired - private EntityManager em; - - @Autowired - private PasswordEncoder passwordEncoder; + @Mock + private PlatformTransactionManager txManager; private String adminCredentialsDir; @@ -61,82 +69,85 @@ void setUp() { this.adminCredentialsDir = System.getProperty("java.io.tmpdir") + File.separator + Generator.randomInt(0, 10000); config.getAdmin().setCredentialsLocation(adminCredentialsDir); + config.getAdmin().setCredentialsFile(".termit-admin"); this.sut = new SystemInitializer(config, userService, txManager); } @AfterEach - void tearDown() throws Exception { - final File dir = new File(adminCredentialsDir); - if (dir.listFiles() != null) { - for (File child : dir.listFiles()) { - Files.deleteIfExists(child.toPath()); - } + void tearDown() throws IOException { + final File toDelete = new File(adminCredentialsDir); + if (toDelete.exists()) { + Files.walk(toDelete.toPath()) + .sorted(Comparator.reverseOrder()) + .map(Path::toFile) + .forEach(File::delete); } - Files.deleteIfExists(dir.toPath()); } @Test void persistsSystemAdminWhenHeDoesNotExist() { sut.initSystemAdmin(); - assertNotNull(em.find(UserAccount.class, ADMIN_URI)); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userService).persist(captor.capture()); + assertEquals(ADMIN_URI, captor.getValue().getUri()); } @Test void doesNotCreateNewAdminWhenOneAlreadyExists() { + when(userService.doesAdminExist()).thenReturn(true); sut.initSystemAdmin(); - final UserAccount admin = em.find(UserAccount.class, ADMIN_URI); - sut.initSystemAdmin(); - final UserAccount result = em.find(UserAccount.class, ADMIN_URI); - // We know that password is generated, so the same password means no new instance was created - assertEquals(admin.getPassword(), result.getPassword()); - } - - @Test - void doesNotCreateNewAdminWhenDifferentAdminAlreadyExists() { - final UserAccount differentAdmin = Generator.generateUserAccount(); - differentAdmin.addType(Vocabulary.s_c_administrator_termitu); - transactional(() -> em.persist(differentAdmin)); - sut.initSystemAdmin(); - assertNull(em.find(UserAccount.class, ADMIN_URI)); + verify(userService, never()).persist(any(UserAccount.class)); } @Test void savesAdminLoginCredentialsIntoHiddenFileInUserHome() throws Exception { + doAnswer(arg -> { + final UserAccount account = arg.getArgument(0, UserAccount.class); + account.setPassword(new BCryptPasswordEncoder().encode(account.getPassword())); + return null; + }).when(userService).persist(any(UserAccount.class)); sut.initSystemAdmin(); - final UserAccount admin = em.find(UserAccount.class, ADMIN_URI); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userService).persist(captor.capture()); final String home = config.getAdmin().getCredentialsLocation(); final File credentialsFile = new File(home + File.separator + config.getAdmin().getCredentialsFile()); assertTrue(credentialsFile.exists()); assertTrue(credentialsFile.isHidden()); - verifyAdminCredentialsFileContent(admin, credentialsFile); + verifyAdminCredentialsFileContent(captor.getValue(), credentialsFile); } private void verifyAdminCredentialsFileContent(UserAccount admin, File credentialsFile) throws IOException { final List lines = Files.readAllLines(credentialsFile.toPath()); assertThat(lines.get(0), containsString(admin.getUsername() + "/")); final String password = lines.get(0).substring(lines.get(0).indexOf('/') + 1); - assertTrue(passwordEncoder.matches(password, admin.getPassword())); + assertTrue(new BCryptPasswordEncoder().matches(password, admin.getPassword())); } @Test void savesAdminLoginCredentialsIntoConfiguredFile() throws Exception { + doAnswer(arg -> { + final UserAccount account = arg.getArgument(0, UserAccount.class); + account.setPassword(new BCryptPasswordEncoder().encode(account.getPassword())); + return null; + }).when(userService).persist(any(UserAccount.class)); final String adminFileName = ".admin-file-with-different-name"; config.getAdmin().setCredentialsFile(adminFileName); this.sut = new SystemInitializer(config, userService, txManager); sut.initSystemAdmin(); - final UserAccount admin = em.find(UserAccount.class, ADMIN_URI); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userService).persist(captor.capture()); final File credentialsFile = new File(adminCredentialsDir + File.separator + adminFileName); assertTrue(credentialsFile.exists()); assertTrue(credentialsFile.isHidden()); - verifyAdminCredentialsFileContent(admin, credentialsFile); + verifyAdminCredentialsFileContent(captor.getValue(), credentialsFile); } @Test void ensuresGeneratedAccountIsAdmin() { sut.initSystemAdmin(); - final UserAccount result = em.find(UserAccount.class, ADMIN_URI); - assertNotNull(result); - assertThat(result.getTypes(), hasItem(Vocabulary.s_c_administrator_termitu)); - assertThat(result.getTypes(), not(hasItem(Vocabulary.s_c_omezeny_uzivatel_termitu))); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userService).persist(captor.capture()); + assertThat(captor.getValue().getTypes(), hasItem(Vocabulary.s_c_administrator_termitu)); + assertThat(captor.getValue().getTypes(), not(hasItem(Vocabulary.s_c_omezeny_uzivatel_termitu))); } } diff --git a/src/test/java/cz/cvut/kbss/termit/service/repository/UserRepositoryServiceTest.java b/src/test/java/cz/cvut/kbss/termit/service/repository/UserRepositoryServiceTest.java index 4ed04d245..8d62d135c 100644 --- a/src/test/java/cz/cvut/kbss/termit/service/repository/UserRepositoryServiceTest.java +++ b/src/test/java/cz/cvut/kbss/termit/service/repository/UserRepositoryServiceTest.java @@ -14,40 +14,62 @@ */ package cz.cvut.kbss.termit.service.repository; -import cz.cvut.kbss.jopa.model.EntityManager; import cz.cvut.kbss.termit.environment.Environment; import cz.cvut.kbss.termit.environment.Generator; import cz.cvut.kbss.termit.exception.ValidationException; import cz.cvut.kbss.termit.model.UserAccount; -import cz.cvut.kbss.termit.service.BaseServiceTestRunner; +import cz.cvut.kbss.termit.persistence.dao.UserAccountDao; +import cz.cvut.kbss.termit.service.IdentifierResolver; +import cz.cvut.kbss.termit.util.Configuration; +import cz.cvut.kbss.termit.util.Vocabulary; import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; +import javax.validation.Validation; +import javax.validation.Validator; import java.net.URI; import java.util.Optional; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; -class UserRepositoryServiceTest extends BaseServiceTestRunner { +@ExtendWith(MockitoExtension.class) +class UserRepositoryServiceTest { - @Autowired - private EntityManager em; + @Spy + private PasswordEncoder passwordEncoder = new BCryptPasswordEncoder(); - @Autowired - private PasswordEncoder passwordEncoder; + @Mock + private UserAccountDao userAccountDao; - @Autowired + @Spy + private IdentifierResolver identifierResolver; + + @Spy + private Configuration configuration = new Configuration(); + + @Spy + private Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); + + @InjectMocks private UserRepositoryService sut; @Test void existsByUsernameReturnsTrueForExistingUsername() { final UserAccount user = Generator.generateUserAccountWithPassword(); - transactional(() -> em.persist(user)); + when(userAccountDao.exists(user.getUsername())).thenReturn(true); assertTrue(sut.exists(user.getUsername())); + verify(userAccountDao).exists(user.getUsername()); } @Test @@ -55,12 +77,13 @@ void persistGeneratesIdentifierForUser() { final UserAccount user = Generator.generateUserAccount(); user.setPassword("12345"); user.setUri(null); + configuration.getNamespace().setUser(Vocabulary.s_c_uzivatel_termitu + "/"); sut.persist(user); assertNotNull(user.getUri()); - final UserAccount result = em.find(UserAccount.class, user.getUri()); - assertNotNull(result); - assertEquals(user, result); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userAccountDao).persist(captor.capture()); + assertEquals(user, captor.getValue()); } @Test @@ -70,20 +93,25 @@ void persistEncodesUserPassword() { user.setPassword(plainPassword); sut.persist(user); - final UserAccount result = em.find(UserAccount.class, user.getUri()); - assertTrue(passwordEncoder.matches(plainPassword, result.getPassword())); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userAccountDao).persist(captor.capture()); + assertTrue(passwordEncoder.matches(plainPassword, captor.getValue().getPassword())); } @Test void updateEncodesPasswordWhenItWasChanged() { - final UserAccount user = persistUser(); + final UserAccount user = Generator.generateUserAccountWithPassword(); + user.setPassword(passwordEncoder.encode(user.getPassword())); + when(userAccountDao.find(user.getUri())).thenReturn(Optional.of(user)); + doAnswer(arg -> arg.getArgument(0)).when(userAccountDao).update(any()); Environment.setCurrentUser(user); final String plainPassword = "updatedPassword01"; user.setPassword(plainPassword); sut.update(user); - final UserAccount result = em.find(UserAccount.class, user.getUri()); - assertTrue(passwordEncoder.matches(plainPassword, result.getPassword())); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userAccountDao).update(captor.capture()); + assertTrue(passwordEncoder.matches(plainPassword, captor.getValue().getPassword())); } @Test @@ -91,37 +119,40 @@ void updateRetainsOriginalPasswordWhenItDoesNotChange() { final UserAccount user = Generator.generateUserAccountWithPassword(); final String plainPassword = user.getPassword(); user.setPassword(passwordEncoder.encode(user.getPassword())); - transactional(() -> em.persist(user)); + when(userAccountDao.find(user.getUri())).thenReturn(Optional.of(user)); + doAnswer(arg -> arg.getArgument(0)).when(userAccountDao).update(any()); Environment.setCurrentUser(user); - user.setPassword(null); // Simulate instance being loaded from repo + final UserAccount update = new UserAccount(); + update.setUri(user.getUri()); + update.setFirstName(user.getFirstName()); + update.setUsername(user.getUsername()); + update.setPassword(null); // Simulate instance being loaded from repo final String newLastName = "newLastName"; - user.setLastName(newLastName); + update.setLastName(newLastName); - sut.update(user); - final UserAccount result = em.find(UserAccount.class, user.getUri()); - assertTrue(passwordEncoder.matches(plainPassword, result.getPassword())); - assertEquals(newLastName, result.getLastName()); + sut.update(update); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userAccountDao).update(captor.capture()); + assertTrue(passwordEncoder.matches(plainPassword, captor.getValue().getPassword())); + assertEquals(newLastName, captor.getValue().getLastName()); } @Test void postLoadErasesPasswordFromInstance() { - final UserAccount user = persistUser(); + final UserAccount user = Generator.generateUserAccountWithPassword(); + user.setPassword(passwordEncoder.encode(user.getPassword())); + when(userAccountDao.find(user.getUri())).thenReturn(Optional.of(user)); final Optional result = sut.find(user.getUri()); assertTrue(result.isPresent()); assertNull(result.get().getPassword()); } - private UserAccount persistUser() { - final UserAccount user = Generator.generateUserAccountWithPassword(); - user.setPassword(passwordEncoder.encode(user.getPassword())); - transactional(() -> em.persist(user)); - return user; - } - @Test void updateThrowsValidationExceptionWhenUpdatedInstanceIsMissingValues() { - final UserAccount user = persistUser(); + final UserAccount user = Generator.generateUserAccountWithPassword(); + user.setPassword(passwordEncoder.encode(user.getPassword())); + when(userAccountDao.find(user.getUri())).thenReturn(Optional.of(user)); Environment.setCurrentUser(user); user.setUsername(null); @@ -136,8 +167,8 @@ void persistDoesNotGenerateUriIfItIsAlreadyPresent() { final URI originalUri = user.getUri(); sut.persist(user); - final UserAccount result = em.find(UserAccount.class, originalUri); - assertNotNull(result); - assertEquals(originalUri, result.getUri()); + final ArgumentCaptor captor = ArgumentCaptor.forClass(UserAccount.class); + verify(userAccountDao).persist(captor.capture()); + assertEquals(originalUri, captor.getValue().getUri()); } } From 437285986109d0ad843e58c43363b7529890ade4 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Thu, 3 Nov 2022 08:58:30 +0100 Subject: [PATCH 4/8] [kbss-cvut/termit-ui#340] Export pretty RDF/XML. --- .../termit/persistence/dao/skos/SKOSExporter.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/termit/persistence/dao/skos/SKOSExporter.java b/src/main/java/cz/cvut/kbss/termit/persistence/dao/skos/SKOSExporter.java index 800b7dd83..fccb8eaf5 100644 --- a/src/main/java/cz/cvut/kbss/termit/persistence/dao/skos/SKOSExporter.java +++ b/src/main/java/cz/cvut/kbss/termit/persistence/dao/skos/SKOSExporter.java @@ -11,8 +11,10 @@ import org.eclipse.rdf4j.model.vocabulary.*; import org.eclipse.rdf4j.query.*; import org.eclipse.rdf4j.repository.RepositoryConnection; -import org.eclipse.rdf4j.rio.RDFFormat; +import org.eclipse.rdf4j.rio.RDFWriter; import org.eclipse.rdf4j.rio.Rio; +import org.eclipse.rdf4j.rio.rdfxml.util.RDFXMLPrettyWriterFactory; +import org.eclipse.rdf4j.rio.turtle.TurtleWriterFactory; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -199,18 +201,18 @@ private void exportReferencedGlossaries() { */ public byte[] exportAs(ExportFormat format) { final ByteArrayOutputStream bos = new ByteArrayOutputStream(); - final RDFFormat rdf4jFormat; + final RDFWriter writer; switch (format) { case TURTLE: - rdf4jFormat = RDFFormat.TURTLE; + writer = new TurtleWriterFactory().getWriter(bos); break; case RDF_XML: - rdf4jFormat = RDFFormat.RDFXML; + writer = new RDFXMLPrettyWriterFactory().getWriter(bos); break; default: throw new IllegalArgumentException("Unsupported SKOS export format " + format); } - Rio.write(model, bos, rdf4jFormat); + Rio.write(model, writer); return bos.toByteArray(); } } From 4a1d85b51122f247c888a319508b143ebe5e827a Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Sun, 6 Nov 2022 18:59:53 +0100 Subject: [PATCH 5/8] [2.14.0] Bump version. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 8a10fe622..07547a9c1 100644 --- a/pom.xml +++ b/pom.xml @@ -11,7 +11,7 @@ termit - 2.13.0 + 2.14.0 TermIt Terminology manager based on Semantic Web technologies. ${packaging} From e176e3cfd102206d01ea181d8df8be2243e9eb49 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Mon, 7 Nov 2022 10:32:55 +0100 Subject: [PATCH 6/8] [Perf] Simplify ResourceRepositoryServiceTest. Rewrite to mocks, remove tests duplicating DAO functionality. --- .../persistence/dao/ResourceDaoTest.java | 74 ++++++ .../ResourceRepositoryServiceTest.java | 243 ++++-------------- 2 files changed, 118 insertions(+), 199 deletions(-) diff --git a/src/test/java/cz/cvut/kbss/termit/persistence/dao/ResourceDaoTest.java b/src/test/java/cz/cvut/kbss/termit/persistence/dao/ResourceDaoTest.java index 100f25e06..9f40f7b4d 100644 --- a/src/test/java/cz/cvut/kbss/termit/persistence/dao/ResourceDaoTest.java +++ b/src/test/java/cz/cvut/kbss/termit/persistence/dao/ResourceDaoTest.java @@ -26,6 +26,10 @@ import cz.cvut.kbss.termit.model.resource.File; import cz.cvut.kbss.termit.model.resource.Resource; import cz.cvut.kbss.termit.persistence.context.DescriptorFactory; +import org.eclipse.rdf4j.model.ValueFactory; +import org.eclipse.rdf4j.model.vocabulary.RDFS; +import org.eclipse.rdf4j.repository.Repository; +import org.eclipse.rdf4j.repository.RepositoryConnection; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -321,4 +325,74 @@ void updateRefreshesLastModifiedValue() throws Exception { final long after = sut.getLastModified(); assertThat(after, greaterThan(before)); } + + @Test + void removeFileUpdatesParentDocumentInVocabularyContext() { + final Document document = Generator.generateDocumentWithId(); + final cz.cvut.kbss.termit.model.Vocabulary vocabulary = new cz.cvut.kbss.termit.model.Vocabulary(); + vocabulary.setUri(Generator.generateUri()); + vocabulary.setLabel("Vocabulary"); + vocabulary.setGlossary(new Glossary()); + vocabulary.setModel(new Model()); + vocabulary.setDocument(document); + document.setVocabulary(vocabulary.getUri()); + final File file = new File(); + file.setLabel("test.html"); + file.setUri(Generator.generateUri()); + file.setDocument(document); + final File fileTwo = new File(); + fileTwo.setLabel("test-two.html"); + fileTwo.setUri(Generator.generateUri()); + fileTwo.setDocument(document); + document.addFile(fileTwo); + transactional(() -> { + em.persist(vocabulary, descriptorFactory.vocabularyDescriptor(vocabulary)); + em.persist(document, descriptorFactory.documentDescriptor(vocabulary)); + em.persist(file, descriptorFactory.fileDescriptor(vocabulary)); + em.persist(fileTwo, descriptorFactory.fileDescriptor(vocabulary)); + }); + + transactional(() -> { + final Resource toRemove = sut.getReference(file.getUri()).get(); + sut.remove(toRemove); + }); + + final cz.cvut.kbss.termit.model.Vocabulary + result = em.find(cz.cvut.kbss.termit.model.Vocabulary.class, vocabulary.getUri(), + descriptorFactory.vocabularyDescriptor(vocabulary)); + assertEquals(1, result.getDocument().getFiles().size()); + assertTrue(result.getDocument().getFiles().contains(fileTwo)); + } + + @Test + void updateSupportsSubclassesOfResource() { + final Document doc = Generator.generateDocumentWithId(); + final File fileOne = Generator.generateFileWithId("test.html"); + doc.addFile(fileOne); + final File fileTwo = Generator.generateFileWithId("testTwo.html"); + transactional(() -> { + // Ensure correct RDFS class hierarchy interpretation + final Repository repository = em.unwrap(Repository.class); + try (final RepositoryConnection conn = repository.getConnection()) { + final ValueFactory vf = conn.getValueFactory(); + conn.add(vf.createIRI(cz.cvut.kbss.termit.util.Vocabulary.s_c_dokument), RDFS.SUBCLASSOF, vf.createIRI( + cz.cvut.kbss.termit.util.Vocabulary.s_c_zdroj)); + } + em.persist(doc); + em.persist(fileOne); + em.persist(fileTwo); + }); + + final String newName = "Updated name"; + doc.setLabel(newName); + final String newDescription = "Document description."; + doc.setDescription(newDescription); + doc.addFile(fileTwo); + transactional(() -> sut.update(doc)); + final Document result = em.find(Document.class, doc.getUri()); + assertEquals(newName, result.getLabel()); + assertEquals(newDescription, result.getDescription()); + assertEquals(2, result.getFiles().size()); + assertTrue(result.getFiles().contains(fileTwo)); + } } diff --git a/src/test/java/cz/cvut/kbss/termit/service/repository/ResourceRepositoryServiceTest.java b/src/test/java/cz/cvut/kbss/termit/service/repository/ResourceRepositoryServiceTest.java index a4f8af956..338d04147 100644 --- a/src/test/java/cz/cvut/kbss/termit/service/repository/ResourceRepositoryServiceTest.java +++ b/src/test/java/cz/cvut/kbss/termit/service/repository/ResourceRepositoryServiceTest.java @@ -14,71 +14,62 @@ */ package cz.cvut.kbss.termit.service.repository; -import cz.cvut.kbss.jopa.model.EntityManager; -import cz.cvut.kbss.jopa.model.descriptors.Descriptor; -import cz.cvut.kbss.jopa.model.descriptors.EntityDescriptor; import cz.cvut.kbss.termit.environment.Environment; import cz.cvut.kbss.termit.environment.Generator; import cz.cvut.kbss.termit.exception.ResourceExistsException; import cz.cvut.kbss.termit.exception.ValidationException; -import cz.cvut.kbss.termit.model.Glossary; -import cz.cvut.kbss.termit.model.Model; -import cz.cvut.kbss.termit.model.Term; -import cz.cvut.kbss.termit.model.User; -import cz.cvut.kbss.termit.model.assignment.FileOccurrenceTarget; -import cz.cvut.kbss.termit.model.assignment.TermFileOccurrence; -import cz.cvut.kbss.termit.model.assignment.TermOccurrence; import cz.cvut.kbss.termit.model.resource.Document; import cz.cvut.kbss.termit.model.resource.File; import cz.cvut.kbss.termit.model.resource.Resource; -import cz.cvut.kbss.termit.model.selector.Selector; -import cz.cvut.kbss.termit.model.selector.TextQuoteSelector; -import cz.cvut.kbss.termit.persistence.context.DescriptorFactory; -import cz.cvut.kbss.termit.service.BaseServiceTestRunner; +import cz.cvut.kbss.termit.persistence.dao.ResourceDao; +import cz.cvut.kbss.termit.persistence.dao.TermOccurrenceDao; +import cz.cvut.kbss.termit.service.IdentifierResolver; +import cz.cvut.kbss.termit.util.Configuration; import cz.cvut.kbss.termit.util.Vocabulary; -import org.eclipse.rdf4j.model.ValueFactory; -import org.eclipse.rdf4j.model.vocabulary.RDFS; -import org.eclipse.rdf4j.repository.Repository; -import org.eclipse.rdf4j.repository.RepositoryConnection; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.test.annotation.DirtiesContext; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; -import java.util.Collections; +import javax.validation.Validation; +import javax.validation.Validator; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.collection.IsEmptyCollection.empty; import static org.hamcrest.core.AnyOf.anyOf; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; -@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) -class ResourceRepositoryServiceTest extends BaseServiceTestRunner { +@ExtendWith(MockitoExtension.class) +class ResourceRepositoryServiceTest { - @Autowired - private DescriptorFactory descriptorFactory; + @Mock + private ResourceDao resourceDao; - @Autowired - private EntityManager em; + @Mock + private TermOccurrenceDao occurrenceDao; - @Autowired - private ResourceRepositoryService sut; + @Spy + private IdentifierResolver idResolver = new IdentifierResolver(); + + @Spy + private Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); - private User user; + @Spy + private Configuration config = new Configuration(); + + @InjectMocks + private ResourceRepositoryService sut; @BeforeEach void setUp() { - this.user = Generator.generateUserWithId(); - transactional(() -> em.persist(user)); - Environment.setCurrentUser(user); - } - - private Term generateTermWithUriAndPersist() { - final Term t = Generator.generateTerm(); - t.setUri(Generator.generateUri()); - transactional(() -> em.persist(t)); - return t; + Environment.setCurrentUser(Generator.generateUserWithId()); } @Test @@ -86,98 +77,33 @@ void persistThrowsValidationExceptionWhenResourceLabelIsMissing() { final Resource resource = Generator.generateResourceWithId(); resource.setLabel(null); assertThrows(ValidationException.class, () -> sut.persist(resource)); + verify(resourceDao, never()).persist(any(Resource.class)); } @Test void removeDeletesOccurrenceTargetsAndTermOccurrencesAssociatedWithResource() { - enableRdfsInference(em); - final File file = Generator.generateFileWithId("test.txt"); - transactional(() -> em.persist(file)); - final Term tOne = generateTermWithUriAndPersist(); - final FileOccurrenceTarget target = new FileOccurrenceTarget(file); - final Selector selector = new TextQuoteSelector("test"); - target.setSelectors(Collections.singleton(selector)); - final TermOccurrence occurrence = new TermFileOccurrence(tOne.getUri(), target); - transactional(() -> { - final Descriptor descriptor = new EntityDescriptor(occurrence.resolveContext()); - em.persist(target, descriptor); - em.persist(occurrence, descriptor); - }); - - sut.remove(file); - assertNull(em.find(File.class, file.getUri())); - verifyInstancesDoNotExist(Vocabulary.s_c_vyskyt_termu, em); - verifyInstancesDoNotExist(Vocabulary.s_c_cil_vyskytu, em); - verifyInstancesDoNotExist(Vocabulary.s_c_selektor_text_quote, em); - } - - @Test - void removeDeletesTermOccurrencesAndAllTargetsAssociatedWithResource() { - enableRdfsInference(em); final File file = Generator.generateFileWithId("test.txt"); - transactional(() -> em.persist(file)); - final Term tOne = generateTermWithUriAndPersist(); - final FileOccurrenceTarget occurrenceTarget = new FileOccurrenceTarget(file); - final Selector selector = new TextQuoteSelector("test"); - occurrenceTarget.setSelectors(Collections.singleton(selector)); - final TermOccurrence occurrence = new TermFileOccurrence(tOne.getUri(), occurrenceTarget); - transactional(() -> { - final Descriptor descriptor = new EntityDescriptor(occurrence.resolveContext()); - em.persist(occurrenceTarget, descriptor); - em.persist(occurrence, descriptor); - }); sut.remove(file); - verifyInstancesDoNotExist(Vocabulary.s_c_vyskyt_termu, em); - verifyInstancesDoNotExist(Vocabulary.s_c_cil_vyskytu, em); - verifyInstancesDoNotExist(Vocabulary.s_c_selektor_text_quote, em); - } - - @Test - void updateSupportsSubclassesOfResource() { - final Document doc = Generator.generateDocumentWithId(); - final File fileOne = Generator.generateFileWithId("test.html"); - doc.addFile(fileOne); - final File fileTwo = Generator.generateFileWithId("testTwo.html"); - transactional(() -> { - // Ensure correct RDFS class hierarchy interpretation - final Repository repository = em.unwrap(Repository.class); - try (final RepositoryConnection conn = repository.getConnection()) { - final ValueFactory vf = conn.getValueFactory(); - conn.add(vf.createIRI(Vocabulary.s_c_dokument), RDFS.SUBCLASSOF, vf.createIRI(Vocabulary.s_c_zdroj)); - } - em.persist(doc); - em.persist(fileOne); - em.persist(fileTwo); - }); - - final String newName = "Updated name"; - doc.setLabel(newName); - final String newDescription = "Document description."; - doc.setDescription(newDescription); - doc.addFile(fileTwo); - sut.update(doc); - final Document result = em.find(Document.class, doc.getUri()); - assertEquals(newName, result.getLabel()); - assertEquals(newDescription, result.getDescription()); - assertEquals(2, result.getFiles().size()); - assertTrue(result.getFiles().contains(fileTwo)); + verify(resourceDao).remove(file); + verify(occurrenceDao).removeAll(file); } @Test void persistGeneratesResourceIdentifierWhenItIsNotSet() { + config.getNamespace().setResource(Vocabulary.s_c_zdroj + "/"); final Resource resource = Generator.generateResource(); assertNull(resource.getUri()); - transactional(() -> sut.persist(resource)); + sut.persist(resource); assertNotNull(resource.getUri()); - final Resource result = em.find(Resource.class, resource.getUri()); - assertEquals(resource, result); + verify(idResolver).generateIdentifier(config.getNamespace().getResource(), resource.getLabel()); + verify(resourceDao).persist(resource); } @Test void persistThrowsResourceExistsExceptionWhenResourceIdentifierAlreadyExists() { final Resource existing = Generator.generateResourceWithId(); - transactional(() -> em.persist(existing)); + when(resourceDao.exists(existing.getUri())).thenReturn(true); final Resource toPersist = Generator.generateResource(); toPersist.setUri(existing.getUri()); @@ -190,92 +116,11 @@ void removeDeletesReferenceFromParentDocumentToRemovedFile() { final Document parent = Generator.generateDocumentWithId(); parent.addFile(file); file.setDocument(parent); // Manually set the inferred attribute - transactional(() -> { - em.persist(file); - em.persist(parent); - }); - - transactional(() -> sut.remove(file)); - - assertFalse(sut.exists(file.getUri())); - final Document result = em.find(Document.class, parent.getUri()); - assertThat(result.getFiles(), anyOf(nullValue(), empty())); - } - - @Test - void persistWithVocabularyPersistsInstanceIntoVocabularyContext() { - final Document document = Generator.generateDocumentWithId(); - final cz.cvut.kbss.termit.model.Vocabulary vocabulary = Generator.generateVocabularyWithId(); - - sut.persist(document, vocabulary); - - final Document result = em - .find(Document.class, document.getUri(), descriptorFactory.documentDescriptor(vocabulary)); - assertNotNull(result); - assertEquals(document, result); - } - - @Test - void updateDocumentWithVocabularyUpdatesInstanceInVocabularyContext() { - enableRdfsInference(em); - final Document document = Generator.generateDocumentWithId(); - final cz.cvut.kbss.termit.model.Vocabulary vocabulary = Generator.generateVocabularyWithId(); - document.setVocabulary(vocabulary.getUri()); - transactional(() -> em.persist(document, descriptorFactory.documentDescriptor(vocabulary))); - - final String newLabel = "Updated document"; - document.setLabel(newLabel); - sut.update(document); - - final Document result = em - .find(Document.class, document.getUri(), descriptorFactory.documentDescriptor(vocabulary)); - assertNotNull(result); - assertEquals(newLabel, result.getLabel()); - } - - @Test - void removeFileUpdatesParentDocumentInVocabularyContext() { - final Document document = Generator.generateDocumentWithId(); - final cz.cvut.kbss.termit.model.Vocabulary vocabulary = new cz.cvut.kbss.termit.model.Vocabulary(); - vocabulary.setUri(Generator.generateUri()); - vocabulary.setLabel("Vocabulary"); - vocabulary.setGlossary(new Glossary()); - vocabulary.setModel(new Model()); - vocabulary.setDocument(document); - document.setVocabulary(vocabulary.getUri()); - final File file = new File(); - file.setLabel("test.html"); - file.setUri(Generator.generateUri()); - file.setDocument(document); - final File fileTwo = new File(); - fileTwo.setLabel("test-two.html"); - fileTwo.setUri(Generator.generateUri()); - fileTwo.setDocument(document); - document.addFile(fileTwo); - transactional(() -> { - em.persist(vocabulary, descriptorFactory.vocabularyDescriptor(vocabulary)); - em.persist(document, descriptorFactory.documentDescriptor(vocabulary)); - em.persist(file, descriptorFactory.fileDescriptor(vocabulary)); - em.persist(fileTwo, descriptorFactory.fileDescriptor(vocabulary)); - }); - - transactional(() -> { - final Resource toRemove = sut.getRequiredReference(file.getUri()); - sut.remove(toRemove); - }); - - final cz.cvut.kbss.termit.model.Vocabulary - result = em.find(cz.cvut.kbss.termit.model.Vocabulary.class, vocabulary.getUri(), - descriptorFactory.vocabularyDescriptor(vocabulary)); - assertEquals(1, result.getDocument().getFiles().size()); - assertTrue(result.getDocument().getFiles().contains(fileTwo)); - } + sut.remove(file); - @Test - void getLastModifiedReturnsInitializedValue() { - final long result = sut.getLastModified(); - assertThat(result, greaterThan(0L)); - assertThat(result, lessThanOrEqualTo(System.currentTimeMillis())); + final ArgumentCaptor captor = ArgumentCaptor.forClass(Document.class); + verify(resourceDao).update(captor.capture()); + assertThat(captor.getValue().getFiles(), anyOf(nullValue(), empty())); } } From 6aef1b3a3ae57e7324c15b9d3976cc444c40d433 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 22 Nov 2022 14:18:04 +0100 Subject: [PATCH 7/8] Do not send notification mail when there are no comment changes. --- .../notification/CommentChangeNotifier.java | 10 +++++--- .../notification/NotificationService.java | 5 ++-- .../CommentChangeNotifierTest.java | 23 +++++++++++++++---- .../notification/NotificationServiceTest.java | 17 +++++++++++--- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifier.java b/src/main/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifier.java index d284b973b..5fe0fde38 100644 --- a/src/main/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifier.java +++ b/src/main/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifier.java @@ -59,8 +59,12 @@ public CommentChangeNotifier(CommentService commentService, TermService termServ * @param to Interval end * @return Notification message ready for sending via email */ - public Message createCommentChangesMessage(Instant from, Instant to) { + public Optional createCommentChangesMessage(Instant from, Instant to) { final Map, List> comments = findChangedComments(from, to); + if (comments.isEmpty()) { + LOG.debug("No new or updated comments found for the specified interval."); + return Optional.empty(); + } final Map variables = new HashMap<>(); final List assetsWithComments = comments.entrySet().stream() .map(e -> new AssetWithComments( @@ -74,10 +78,10 @@ public Message createCommentChangesMessage(Instant from, Instant to) { variables.put("from", LocalDate.ofInstant(from, ZoneId.systemDefault())); variables.put("to", LocalDate.ofInstant(to, ZoneId.systemDefault())); variables.put("commentedAssets", assetsWithComments); - return Message.to(resolveNotificationRecipients(comments).stream().map( + return Optional.of(Message.to(resolveNotificationRecipients(comments).stream().map( AbstractUser::getUsername).toArray(String[]::new)) .content(messageComposer.composeMessage(COMMENT_CHANGES_TEMPLATE, variables)) - .subject("TermIt News").build(); + .subject("TermIt News").build()); } /** diff --git a/src/main/java/cz/cvut/kbss/termit/service/notification/NotificationService.java b/src/main/java/cz/cvut/kbss/termit/service/notification/NotificationService.java index 722935a9c..6f5de29b0 100644 --- a/src/main/java/cz/cvut/kbss/termit/service/notification/NotificationService.java +++ b/src/main/java/cz/cvut/kbss/termit/service/notification/NotificationService.java @@ -11,6 +11,7 @@ import org.springframework.stereotype.Service; import java.time.*; +import java.util.Optional; @Service public class NotificationService { @@ -39,8 +40,8 @@ public void notifyOfCommentChanges() { LOG.debug("Running comment change notification."); final Instant now = Utils.timestamp(); final Instant previous = resolvePreviousRun(now, scheduleConfig.getCron().getNotification().getComments()); - final Message changeNotificationMessage = commentChangeNotifier.createCommentChangesMessage(previous, now); - postman.sendMessage(changeNotificationMessage); + final Optional changeNotificationMessage = commentChangeNotifier.createCommentChangesMessage(previous, now); + changeNotificationMessage.ifPresent(postman::sendMessage); } private static Instant resolvePreviousRun(Instant now, String cronExpression) { diff --git a/src/test/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifierTest.java b/src/test/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifierTest.java index aa60a58e0..c950487cf 100644 --- a/src/test/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifierTest.java +++ b/src/test/java/cz/cvut/kbss/termit/service/notification/CommentChangeNotifierTest.java @@ -35,8 +35,7 @@ import static cz.cvut.kbss.termit.service.notification.CommentChangeNotifier.COMMENT_CHANGES_TEMPLATE; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasItems; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -162,8 +161,9 @@ void createCommentChangesMessageComposesMessageUsingResolvedCommentsToResolvedRe new MessageAssetFactory.MessageAsset(term.getPrimaryLabel(), link)); when(messageComposer.composeMessage(any(), anyMap())).thenReturn("Test message content"); - final Message result = sut.createCommentChangesMessage(from, to); - assertEquals(Collections.singletonList(author.getUsername()), result.getRecipients()); + final Optional result = sut.createCommentChangesMessage(from, to); + assertTrue(result.isPresent()); + assertEquals(Collections.singletonList(author.getUsername()), result.get().getRecipients()); verify(changeRecordService).getAuthors(new cz.cvut.kbss.termit.model.Vocabulary(term.getVocabulary())); verify(commentService).findAll(null, from, to); final ArgumentCaptor> captor = ArgumentCaptor.forClass(Map.class); @@ -176,4 +176,19 @@ void createCommentChangesMessageComposesMessageUsingResolvedCommentsToResolvedRe Collections.singletonList(new CommentChangeNotifier.CommentForMessage(comment)))), variables.get("commentedAssets")); } + + @Test + void createCommentChangesMessageReturnsEmptyOptionalWhenThereAreNoCommentChanges() throws Exception { + final Instant from = Utils.timestamp().minus(7, ChronoUnit.DAYS); + final Instant to = Utils.timestamp(); + final Term term = Generator.generateTermWithId(Generator.generateUri()); + // Simulate autowired configuration + final Field configField = Term.class.getDeclaredField("config"); + configField.setAccessible(true); + configField.set(term, new Configuration()); + when(commentService.findAll(any(), any(Instant.class), any(Instant.class))).thenReturn(Collections.emptyList()); + + final Optional result = sut.createCommentChangesMessage(from, to); + assertTrue(result.isEmpty()); + } } diff --git a/src/test/java/cz/cvut/kbss/termit/service/notification/NotificationServiceTest.java b/src/test/java/cz/cvut/kbss/termit/service/notification/NotificationServiceTest.java index 3e4ecd066..4bfeb862f 100644 --- a/src/test/java/cz/cvut/kbss/termit/service/notification/NotificationServiceTest.java +++ b/src/test/java/cz/cvut/kbss/termit/service/notification/NotificationServiceTest.java @@ -17,12 +17,12 @@ import java.time.Instant; import java.time.OffsetDateTime; import java.time.temporal.TemporalAdjusters; +import java.util.Optional; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.lessThan; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; @ExtendWith(MockitoExtension.class) class NotificationServiceTest { @@ -62,9 +62,20 @@ void notifyOfCommentChangesSendsMessageProvidedByChangeNotifier() { configuration.getSchedule().getCron().getNotification().setComments(cronExpr); final Message testMessage = Message.to("test@example.org").content("Test message").subject("Test").build(); when(commentChangeNotifier.createCommentChangesMessage(any(Instant.class), any(Instant.class))).thenReturn( - testMessage); + Optional.of(testMessage)); sut.notifyOfCommentChanges(); verify(postman).sendMessage(testMessage); } + + @Test + void notifyOfCommentChangesDoesNotSendAnythingWhenCommentChangeNotifierReturnsEmptyMessage() { + final String cronExpr = "0 1 5 * * MON"; + configuration.getSchedule().getCron().getNotification().setComments(cronExpr); + when(commentChangeNotifier.createCommentChangesMessage(any(Instant.class), any(Instant.class))).thenReturn( + Optional.empty()); + + sut.notifyOfCommentChanges(); + verify(postman, never()).sendMessage(any()); + } } From 7772be186eee6bd6aabd4dc6fc745be01be59d95 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Sun, 27 Nov 2022 17:53:45 +0100 Subject: [PATCH 8/8] [2.14.1] Bump version, update JOPA to 0.19.2. --- pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 07547a9c1..7cd23c81c 100644 --- a/pom.xml +++ b/pom.xml @@ -11,7 +11,7 @@ termit - 2.14.0 + 2.14.1 TermIt Terminology manager based on Semantic Web technologies. ${packaging} @@ -32,7 +32,7 @@ 2.7.5 6.2.3.Final 2.5.0 - 0.19.0 + 0.19.2 0.9.0 1.9.7