From a8a1ed190df48c2be1ea253f86230231db165edc Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 6 Feb 2024 09:54:39 +0100 Subject: [PATCH 1/3] [kbss-cvut/record-manager-ui#71] Harmonize action history paging with record paging. --- .../persistence/dao/ActionHistoryDao.java | 17 +- .../study/rest/ActionHistoryController.java | 29 +++- .../study/service/ActionHistoryService.java | 6 +- .../RepositoryActionHistoryService.java | 8 +- .../persistence/dao/ActionHistoryDaoTest.java | 74 +++++--- .../rest/ActionHistoryControllerTest.java | 163 +++++++++++------- 6 files changed, 184 insertions(+), 113 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDao.java b/src/main/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDao.java index d8d74f16..928126bb 100644 --- a/src/main/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDao.java +++ b/src/main/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDao.java @@ -7,10 +7,12 @@ import cz.cvut.kbss.study.model.User; import cz.cvut.kbss.study.model.Vocabulary; import cz.cvut.kbss.study.util.Constants; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Repository; import java.net.URI; -import java.util.List; import java.util.Objects; @Repository @@ -35,7 +37,7 @@ public ActionHistory findByKey(String key) { } } - public List findAllWithParams(String typeFilter, User author, int pageNumber) { + public Page findAllWithParams(String typeFilter, User author, Pageable pageSpec) { String params; if (typeFilter == null && author == null) { params = " } "; @@ -52,10 +54,11 @@ public List findAllWithParams(String typeFilter, User author, int params + "ORDER BY DESC(?timestamp)", ActionHistory.class) .setParameter("type", typeUri) - .setParameter("isCreated", URI.create(Vocabulary.s_p_created)) - .setFirstResult((pageNumber - 1) * Constants.DEFAULT_PAGE_SIZE) - .setMaxResults(Constants.DEFAULT_PAGE_SIZE + 1); - + .setParameter("isCreated", URI.create(Vocabulary.s_p_created)); + if (pageSpec.isPaged()) { + q.setFirstResult((int) pageSpec.getOffset()); + q.setMaxResults(pageSpec.getPageSize()); + } if (author != null) { q.setParameter("hasOwner", URI.create(Vocabulary.s_p_has_owner)) .setParameter("author", author.getUri()); @@ -64,6 +67,6 @@ public List findAllWithParams(String typeFilter, User author, int q.setParameter("typeFilter", typeFilter, Constants.PU_LANGUAGE) .setParameter("isType", URI.create(Vocabulary.s_p_action_type)); } - return q.getResultList(); + return new PageImpl<>(q.getResultList(), pageSpec, 0L); } } diff --git a/src/main/java/cz/cvut/kbss/study/rest/ActionHistoryController.java b/src/main/java/cz/cvut/kbss/study/rest/ActionHistoryController.java index af56bfb8..026d7f92 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/ActionHistoryController.java +++ b/src/main/java/cz/cvut/kbss/study/rest/ActionHistoryController.java @@ -3,14 +3,27 @@ import cz.cvut.kbss.study.exception.NotFoundException; import cz.cvut.kbss.study.model.ActionHistory; import cz.cvut.kbss.study.model.User; +import cz.cvut.kbss.study.rest.event.PaginatedResultRetrievedEvent; +import cz.cvut.kbss.study.rest.util.RestUtils; import cz.cvut.kbss.study.security.SecurityConstants; import cz.cvut.kbss.study.service.ActionHistoryService; import cz.cvut.kbss.study.service.UserService; -import org.springframework.beans.factory.annotation.Autowired; +import jakarta.servlet.http.HttpServletResponse; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.data.domain.Page; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.security.access.prepost.PreAuthorize; -import org.springframework.web.bind.annotation.*; +import org.springframework.util.MultiValueMap; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseStatus; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.util.UriComponentsBuilder; import java.util.Collections; import java.util.List; @@ -23,10 +36,13 @@ public class ActionHistoryController extends BaseController { private final UserService userService; + private final ApplicationEventPublisher eventPublisher; + public ActionHistoryController(ActionHistoryService actionHistoryService, - UserService userService) { + UserService userService, ApplicationEventPublisher eventPublisher) { this.actionHistoryService = actionHistoryService; this.userService = userService; + this.eventPublisher = eventPublisher; } @PostMapping(consumes = MediaType.APPLICATION_JSON_VALUE) @@ -42,7 +58,8 @@ public void create(@RequestBody ActionHistory actionHistory) { @GetMapping(produces = MediaType.APPLICATION_JSON_VALUE) public List getActions(@RequestParam(value = "author", required = false) String authorUsername, @RequestParam(value = "type", required = false) String type, - @RequestParam(value = "page") int pageNumber) { + @RequestParam MultiValueMap params, + UriComponentsBuilder uriBuilder, HttpServletResponse response) { User author = null; if (authorUsername != null) { try { @@ -51,7 +68,9 @@ public List getActions(@RequestParam(value = "author", required = return Collections.emptyList(); } } - return actionHistoryService.findAllWithParams(type, author, pageNumber); + final Page result = actionHistoryService.findAllWithParams(type, author, RestUtils.resolvePaging(params)); + eventPublisher.publishEvent(new PaginatedResultRetrievedEvent(this, uriBuilder, response, result)); + return result.getContent(); } @PreAuthorize("hasRole('" + SecurityConstants.ROLE_ADMIN + "')") diff --git a/src/main/java/cz/cvut/kbss/study/service/ActionHistoryService.java b/src/main/java/cz/cvut/kbss/study/service/ActionHistoryService.java index 90f360a8..5bd83524 100644 --- a/src/main/java/cz/cvut/kbss/study/service/ActionHistoryService.java +++ b/src/main/java/cz/cvut/kbss/study/service/ActionHistoryService.java @@ -2,12 +2,12 @@ import cz.cvut.kbss.study.model.ActionHistory; import cz.cvut.kbss.study.model.User; - -import java.util.List; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; public interface ActionHistoryService extends BaseService { ActionHistory findByKey(String key); - List findAllWithParams(String type, User author, int pageNumber); + Page findAllWithParams(String type, User author, Pageable pageSpec); } diff --git a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryActionHistoryService.java b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryActionHistoryService.java index 82522b0b..bbde2b61 100644 --- a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryActionHistoryService.java +++ b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryActionHistoryService.java @@ -5,11 +5,11 @@ import cz.cvut.kbss.study.persistence.dao.ActionHistoryDao; import cz.cvut.kbss.study.persistence.dao.GenericDao; import cz.cvut.kbss.study.service.ActionHistoryService; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; -import java.util.List; - @Service public class RepositoryActionHistoryService extends BaseRepositoryService implements ActionHistoryService { @@ -32,7 +32,7 @@ public ActionHistory findByKey(String key) { @Transactional(readOnly = true) @Override - public List findAllWithParams(String type, User author, int pageNumber) { - return actionHistoryDao.findAllWithParams(type, author, pageNumber); + public Page findAllWithParams(String type, User author, Pageable pageSpec) { + return actionHistoryDao.findAllWithParams(type, author, pageSpec); } } diff --git a/src/test/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDaoTest.java b/src/test/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDaoTest.java index db39b0e7..a046a3cb 100644 --- a/src/test/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDaoTest.java +++ b/src/test/java/cz/cvut/kbss/study/persistence/dao/ActionHistoryDaoTest.java @@ -5,10 +5,14 @@ import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.User; import cz.cvut.kbss.study.persistence.BaseDaoTestRunner; +import cz.cvut.kbss.study.util.Constants; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageRequest; import java.util.List; +import java.util.stream.IntStream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -24,9 +28,9 @@ public class ActionHistoryDaoTest extends BaseDaoTestRunner { @Autowired ActionHistoryDao actionHistoryDao; - private final String LOAD_SUCCESS = "LOAD_SUCCESS"; - private final String LOAD_ERROR = "LOAD_ERROR"; - private final String LOAD_PENDING = "LOAD_PENDING"; + private static final String LOAD_SUCCESS = "LOAD_SUCCESS"; + private static final String LOAD_ERROR = "LOAD_ERROR"; + private static final String LOAD_PENDING = "LOAD_PENDING"; @Test public void findByKeyReturnsActionWithPayload() { @@ -61,9 +65,9 @@ public void findAllWithParamsWithoutParamsReturnsAllActions() { actionHistoryDao.persist(List.of(action1, action2, action3)); }); - List actionsList = actionHistoryDao.findAllWithParams(null, null, 1); + Page actionsList = actionHistoryDao.findAllWithParams(null, null, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); - assertEquals(3, actionsList.size()); + assertEquals(3, actionsList.getNumberOfElements()); } @Test @@ -82,13 +86,13 @@ public void findAllWithParamsWithAuthorReturnsAuthorsActions() { actionHistoryDao.persist(List.of(action1, action2, action3)); }); - List actionsList1 = actionHistoryDao.findAllWithParams(null, user1, 1); - List actionsList2 = actionHistoryDao.findAllWithParams(null, user2, 1); - List actionsList3 = actionHistoryDao.findAllWithParams(null, user3, 1); + Page actionsList1 = actionHistoryDao.findAllWithParams(null, user1, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList2 = actionHistoryDao.findAllWithParams(null, user2, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList3 = actionHistoryDao.findAllWithParams(null, user3, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); - assertEquals(2, actionsList1.size()); - assertEquals(1, actionsList2.size()); - assertEquals(0, actionsList3.size()); + assertEquals(2, actionsList1.getNumberOfElements()); + assertEquals(1, actionsList2.getNumberOfElements()); + assertEquals(0, actionsList3.getNumberOfElements()); } @Test @@ -108,13 +112,13 @@ public void findAllWithParamsWithTypeReturnsActionsWithExactType() { actionHistoryDao.persist(List.of(action1, action2, action3)); }); - List actionsList1 = actionHistoryDao.findAllWithParams(LOAD_SUCCESS, null, 1); - List actionsList2 = actionHistoryDao.findAllWithParams(LOAD_ERROR, null, 1); - List actionsList3 = actionHistoryDao.findAllWithParams(LOAD_PENDING, null, 1); + Page actionsList1 = actionHistoryDao.findAllWithParams(LOAD_SUCCESS, null, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList2 = actionHistoryDao.findAllWithParams(LOAD_ERROR, null, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList3 = actionHistoryDao.findAllWithParams(LOAD_PENDING, null, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); - assertEquals(2, actionsList1.size()); - assertEquals(1, actionsList2.size()); - assertEquals(0, actionsList3.size()); + assertEquals(2, actionsList1.getNumberOfElements()); + assertEquals(1, actionsList2.getNumberOfElements()); + assertEquals(0, actionsList3.getNumberOfElements()); } @Test @@ -133,9 +137,9 @@ public void findAllWithParamsWithTypeReturnsActionsWithTypeContained() { actionHistoryDao.persist(List.of(action1, action2, action3)); }); - List actionsList = actionHistoryDao.findAllWithParams("LOAD", null, 1); + Page actionsList = actionHistoryDao.findAllWithParams("LOAD", null, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); - assertEquals(2, actionsList.size()); + assertEquals(2, actionsList.getNumberOfElements()); } @Test @@ -156,14 +160,30 @@ public void findAllWithParamsReturnsMatchingActions() { actionHistoryDao.persist(List.of(action1, action2, action3)); }); - List actionsList1 = actionHistoryDao.findAllWithParams(LOAD_SUCCESS, user1, 1); - List actionsList2 = actionHistoryDao.findAllWithParams(LOAD_SUCCESS, user2, 1); - List actionsList3 = actionHistoryDao.findAllWithParams(LOAD_ERROR, user2, 1); - List actionsList4 = actionHistoryDao.findAllWithParams("LOAD", user2, 1); + Page actionsList1 = actionHistoryDao.findAllWithParams(LOAD_SUCCESS, user1, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList2 = actionHistoryDao.findAllWithParams(LOAD_SUCCESS, user2, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList3 = actionHistoryDao.findAllWithParams(LOAD_ERROR, user2, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); + Page actionsList4 = actionHistoryDao.findAllWithParams("LOAD", user2, PageRequest.of(0, Constants.DEFAULT_PAGE_SIZE)); - assertEquals(2, actionsList1.size()); - assertEquals(0, actionsList2.size()); - assertEquals(1, actionsList3.size()); - assertEquals(1, actionsList4.size()); + assertEquals(2, actionsList1.getNumberOfElements()); + assertEquals(0, actionsList2.getNumberOfElements()); + assertEquals(1, actionsList3.getNumberOfElements()); + assertEquals(1, actionsList4.getNumberOfElements()); + } + + @Test + void findAllReturnsActionsOnMatchingPage() { + Institution institution = Generator.generateInstitution(); + User user = Generator.generateUser(institution); + final List allActions = IntStream.range(0, 10).mapToObj(i -> Generator.generateActionHistory(user)).toList(); + transactional(() -> { + institutionDao.persist(institution); + userDao.persist(user); + actionHistoryDao.persist(allActions); + }); + + final PageRequest pageSpec = PageRequest.of(2, allActions.size() / 2); + final Page result = actionHistoryDao.findAllWithParams(null, null, pageSpec); + assertEquals(allActions.subList((int) pageSpec.getOffset(), allActions.size()), result.getContent()); } } diff --git a/src/test/java/cz/cvut/kbss/study/rest/ActionHistoryControllerTest.java b/src/test/java/cz/cvut/kbss/study/rest/ActionHistoryControllerTest.java index 393a0ded..52e3d31a 100644 --- a/src/test/java/cz/cvut/kbss/study/rest/ActionHistoryControllerTest.java +++ b/src/test/java/cz/cvut/kbss/study/rest/ActionHistoryControllerTest.java @@ -7,24 +7,34 @@ import cz.cvut.kbss.study.model.ActionHistory; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.User; +import cz.cvut.kbss.study.rest.event.PaginatedResultRetrievedEvent; import cz.cvut.kbss.study.service.ActionHistoryService; import cz.cvut.kbss.study.service.UserService; +import cz.cvut.kbss.study.util.Constants; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.test.web.servlet.MvcResult; -import java.util.ArrayList; -import java.util.Collections; import java.util.List; +import java.util.stream.IntStream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; @@ -39,23 +49,29 @@ public class ActionHistoryControllerTest extends BaseControllerTestRunner { @Mock private UserService userServiceMock; + @Mock + private ApplicationEventPublisher eventPublisherMock; + @InjectMocks private ActionHistoryController controller; + private User user; + @BeforeEach public void setUp() { super.setUp(controller); Institution institution = Generator.generateInstitution(); - User user = Generator.generateUser(institution); + this.user = Generator.generateUser(institution); Environment.setCurrentUser(user); } @Test public void createActionReturnsResponseStatusCreated() throws Exception { - ActionHistory action = Generator.generateActionHistory(Environment.getCurrentUser()); + ActionHistory action = Generator.generateActionHistory(user); final MvcResult result = mockMvc.perform(post("/history").content(toJson(action)) - .contentType(MediaType.APPLICATION_JSON_VALUE)).andReturn(); + .contentType(MediaType.APPLICATION_JSON_VALUE)) + .andReturn(); assertEquals(HttpStatus.CREATED, HttpStatus.valueOf(result.getResponse().getStatus())); } @@ -72,49 +88,48 @@ public void getByKeyThrowsNotFoundWhenActionIsNotFound() throws Exception { @Test public void getByKeyReturnsFoundAction() throws Exception { final String key = "12345"; - ActionHistory action = Generator.generateActionHistory(Environment.getCurrentUser()); + ActionHistory action = Generator.generateActionHistory(user); action.setKey(key); when(actionHistoryServiceMock.findByKey(key)).thenReturn(action); final MvcResult result = mockMvc.perform(get("/history/" + key)).andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); - final ActionHistory res = objectMapper.readValue(result.getResponse().getContentAsString(), ActionHistory.class); - assertEquals(res.getUri(),action.getUri()); + final ActionHistory res = + objectMapper.readValue(result.getResponse().getContentAsString(), ActionHistory.class); + assertEquals(res.getUri(), action.getUri()); verify(actionHistoryServiceMock).findByKey(key); } @Test public void getActionsReturnsEmptyListWhenNoActionsAreFound() throws Exception { - when(actionHistoryServiceMock.findAllWithParams(null, null, 1)).thenReturn(Collections.emptyList()); + when(actionHistoryServiceMock.findAllWithParams(any(), any(), any())).thenReturn(Page.empty()); - final MvcResult result = mockMvc.perform(get("/history/").param("page", "1")).andReturn(); + final MvcResult result = mockMvc.perform(get("/history/")).andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); final List body = objectMapper.readValue(result.getResponse().getContentAsString(), - new TypeReference>() { - }); + new TypeReference<>() { + }); assertTrue(body.isEmpty()); } @Test public void getActionsReturnsAllActions() throws Exception { - ActionHistory action1 = Generator.generateActionHistory(Environment.getCurrentUser()); - ActionHistory action2 = Generator.generateActionHistory(Environment.getCurrentUser()); - List actions = new ArrayList<>(); - - actions.add(action1); - actions.add(action2); + ActionHistory action1 = Generator.generateActionHistory(user); + ActionHistory action2 = Generator.generateActionHistory(user); + List actions = List.of(action1, action2); - when(actionHistoryServiceMock.findAllWithParams(null, null, 1)).thenReturn(actions); + when(actionHistoryServiceMock.findAllWithParams(any(), any(), any( + Pageable.class))).thenReturn(new PageImpl<>(actions)); final MvcResult result = mockMvc.perform(get("/history/").param("page", "1")).andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); final List body = objectMapper.readValue(result.getResponse().getContentAsString(), - new TypeReference>() { - }); + new TypeReference<>() { + }); assertEquals(2, body.size()); - verify(actionHistoryServiceMock).findAllWithParams(null , null, 1); + verify(actionHistoryServiceMock).findAllWithParams(null, null, PageRequest.of(1, Constants.DEFAULT_PAGE_SIZE)); } @Test @@ -123,90 +138,104 @@ public void getActionsByUnknownAuthorReturnsEmptyList() throws Exception { when(userServiceMock.findByUsername(username)).thenThrow(NotFoundException.create("User", username)); final MvcResult result = mockMvc.perform(get("/history/") - .param("author", username) - .param("page", "1")) - .andReturn(); + .param("author", username) + .param("page", "1")) + .andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); final List body = objectMapper.readValue(result.getResponse().getContentAsString(), - new TypeReference>() { - }); + new TypeReference<>() { + }); assertTrue(body.isEmpty()); + verify(actionHistoryServiceMock, never()).findAllWithParams(any(), any(), any()); } @Test public void getActionsByAuthorReturnsActions() throws Exception { - ActionHistory action1 = Generator.generateActionHistory(Environment.getCurrentUser()); - ActionHistory action2 = Generator.generateActionHistory(Environment.getCurrentUser()); - List actions = new ArrayList<>(); - - actions.add(action1); - actions.add(action2); + ActionHistory action1 = Generator.generateActionHistory(user); + ActionHistory action2 = Generator.generateActionHistory(user); + List actions = List.of(action1, action2); - when(actionHistoryServiceMock.findAllWithParams(null, Environment.getCurrentUser(), 1)).thenReturn(actions); - when(userServiceMock.findByUsername(Environment.getCurrentUser().getUsername())).thenReturn(Environment.getCurrentUser()); + when(actionHistoryServiceMock.findAllWithParams(any(), eq(user), any( + Pageable.class))).thenReturn(new PageImpl<>(actions)); + when(userServiceMock.findByUsername(user.getUsername())).thenReturn( + user); final MvcResult result = mockMvc.perform(get("/history/") - .param("author", Environment.getCurrentUser().getUsername()) - .param("page", "1")) - .andReturn(); + .param("author", user.getUsername()) + .param("page", "1")) + .andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); final List body = objectMapper.readValue(result.getResponse().getContentAsString(), - new TypeReference>() { - }); + new TypeReference<>() { + }); assertEquals(2, body.size()); - verify(actionHistoryServiceMock).findAllWithParams(null, Environment.getCurrentUser(), 1); + verify(actionHistoryServiceMock).findAllWithParams(null, user, + PageRequest.of(1, Constants.DEFAULT_PAGE_SIZE)); } @Test public void getActionsByTypeReturnsActions() throws Exception { - ActionHistory action1 = Generator.generateActionHistory(Environment.getCurrentUser()); - ActionHistory action2 = Generator.generateActionHistory(Environment.getCurrentUser()); - List actions = new ArrayList<>(); - - actions.add(action1); - actions.add(action2); + ActionHistory action1 = Generator.generateActionHistory(user); + ActionHistory action2 = Generator.generateActionHistory(user); + List actions = List.of(action1, action2); - when(actionHistoryServiceMock.findAllWithParams("TYPE", null, 1)).thenReturn(actions); + when(actionHistoryServiceMock.findAllWithParams(eq("TYPE"), any(), any(Pageable.class))).thenReturn( + new PageImpl<>(actions)); final MvcResult result = mockMvc.perform(get("/history/") - .param("type", "TYPE") - .param("page", "1")) - .andReturn(); + .param("type", "TYPE") + .param("page", "1")) + .andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); final List body = objectMapper.readValue(result.getResponse().getContentAsString(), - new TypeReference>() { - }); + new TypeReference<>() { + }); assertEquals(2, body.size()); - verify(actionHistoryServiceMock).findAllWithParams("TYPE", null, 1); + verify(actionHistoryServiceMock).findAllWithParams("TYPE", null, + PageRequest.of(1, Constants.DEFAULT_PAGE_SIZE)); } @Test public void getActionsByTypeAndAuthorReturnsActions() throws Exception { - User user = Environment.getCurrentUser(); ActionHistory action1 = Generator.generateActionHistory(user); ActionHistory action2 = Generator.generateActionHistory(user); - List actions = new ArrayList<>(); - - actions.add(action1); - actions.add(action2); + List actions = List.of(action1, action2); when(userServiceMock.findByUsername(user.getUsername())).thenReturn(user); - when(actionHistoryServiceMock.findAllWithParams("TYPE", user, 1)).thenReturn(actions); + when(actionHistoryServiceMock.findAllWithParams(eq("TYPE"), eq(user), any(Pageable.class))).thenReturn( + new PageImpl<>(actions)); final MvcResult result = mockMvc.perform(get("/history/") - .param("author", user.getUsername()) - .param("type", "TYPE") - .param("page", "1")) - .andReturn(); + .param("author", user.getUsername()) + .param("type", "TYPE") + .param("page", "1")) + .andReturn(); assertEquals(HttpStatus.OK, HttpStatus.valueOf(result.getResponse().getStatus())); final List body = objectMapper.readValue(result.getResponse().getContentAsString(), - new TypeReference>() { - }); + new TypeReference<>() { + }); assertEquals(2, body.size()); - verify(actionHistoryServiceMock).findAllWithParams("TYPE", user, 1); + verify(actionHistoryServiceMock).findAllWithParams("TYPE", user, + PageRequest.of(1, Constants.DEFAULT_PAGE_SIZE)); + } + + @Test + void getActionsPublishesPagingEvent() throws Exception { + List actions = + IntStream.range(0, 5).mapToObj(i -> Generator.generateActionHistory(user)).toList(); + final Page page = new PageImpl<>(actions, PageRequest.of(2, 5), 0L); + when(actionHistoryServiceMock.findAllWithParams(any(), any(), any(Pageable.class))).thenReturn(page); + + mockMvc.perform(get("/history/").param("page", "2").param("size", "5")); + verify(actionHistoryServiceMock).findAllWithParams(null, null, PageRequest.of(2, 5)); + final ArgumentCaptor captor = ArgumentCaptor.forClass( + PaginatedResultRetrievedEvent.class); + verify(eventPublisherMock).publishEvent(captor.capture()); + final PaginatedResultRetrievedEvent event = captor.getValue(); + assertEquals(page, event.getPage()); } } From 8e285288254eac0e12c3bade3c760343eed73c39 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 6 Feb 2024 14:28:44 +0100 Subject: [PATCH 2/3] [kbss-cvut/record-manager-ui#71] Ensure first and last rel links are always present in paged record response. --- .../cz/cvut/kbss/study/config/SecurityConfig.java | 1 + .../study/rest/handler/HateoasPagingListener.java | 8 ++++++-- .../rest/handler/HateoasPagingListenerTest.java | 13 ++++++++++--- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/study/config/SecurityConfig.java b/src/main/java/cz/cvut/kbss/study/config/SecurityConfig.java index 148aade2..ded8b3bf 100644 --- a/src/main/java/cz/cvut/kbss/study/config/SecurityConfig.java +++ b/src/main/java/cz/cvut/kbss/study/config/SecurityConfig.java @@ -115,6 +115,7 @@ static CorsConfigurationSource createCorsConfiguration(ConfigReader configReader corsConfiguration.addExposedHeader(HttpHeaders.AUTHORIZATION); corsConfiguration.addExposedHeader(HttpHeaders.LOCATION); corsConfiguration.addExposedHeader(HttpHeaders.CONTENT_DISPOSITION); + corsConfiguration.addExposedHeader(HttpHeaders.LINK); final UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); source.registerCorsConfiguration("/**", corsConfiguration); diff --git a/src/main/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListener.java b/src/main/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListener.java index 025a7ecc..7dd87f15 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListener.java +++ b/src/main/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListener.java @@ -19,13 +19,17 @@ public class HateoasPagingListener implements ApplicationListener page = event.getPage(); final LinkHeader header = new LinkHeader(); + if (!page.isEmpty() || page.getTotalPages() > 0) { + // Always add first and last links, even when there is just one page. This allows clients to know where the limits + // are + header.addLink(generateFirstPageLink(page, event.getUriBuilder()), HttpPaginationLink.FIRST); + header.addLink(generateLastPageLink(page, event.getUriBuilder()), HttpPaginationLink.LAST); + } if (page.hasNext()) { header.addLink(generateNextPageLink(page, event.getUriBuilder()), HttpPaginationLink.NEXT); - header.addLink(generateLastPageLink(page, event.getUriBuilder()), HttpPaginationLink.LAST); } if (page.hasPrevious()) { header.addLink(generatePreviousPageLink(page, event.getUriBuilder()), HttpPaginationLink.PREVIOUS); - header.addLink(generateFirstPageLink(page, event.getUriBuilder()), HttpPaginationLink.FIRST); } if (header.hasLinks()) { event.getResponse().addHeader(HttpHeaders.LINK, header.toString()); diff --git a/src/test/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListenerTest.java b/src/test/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListenerTest.java index ac2a06c2..0d582ce9 100644 --- a/src/test/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListenerTest.java +++ b/src/test/java/cz/cvut/kbss/study/rest/handler/HateoasPagingListenerTest.java @@ -168,11 +168,18 @@ public void generatesPreviousAndFirstLinkForEmptyPageAfterEnd() { } @Test - public void generatesNoLinksForOnlyPage() { - final Page page = new PageImpl<>(records, PageRequest.of(0, records.size()), records.size()); + public void generatesFirstAndLastLinksForOnlyPage() { + final int size = records.size(); + final Page page = new PageImpl<>(records, PageRequest.of(0, size), records.size()); listener.onApplicationEvent(event(page)); final String linkHeader = responseMock.getHeader(HttpHeaders.LINK); - assertNull(linkHeader); + assertNotNull(linkHeader); + final String firstLink = HttpLinkHeaderUtil.extractURIByRel(linkHeader, HttpPaginationLink.FIRST.getName()); + assertThat(firstLink, containsString(page(0))); + assertThat(firstLink, containsString(pageSize(size))); + final String lastLink = HttpLinkHeaderUtil.extractURIByRel(linkHeader, HttpPaginationLink.LAST.getName()); + assertThat(lastLink, containsString(page(0))); + assertThat(lastLink, containsString(pageSize(size))); } } \ No newline at end of file From 1a11a75ebc84db90df93f6e14db3c8f63ef5b05a Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Thu, 8 Feb 2024 13:55:28 +0100 Subject: [PATCH 3/3] [kbss-cvut/record-manager-ui#71] Support providing record phase for filtering (and import) as either IRI or name. --- .../cz/cvut/kbss/study/model/RecordPhase.java | 36 ++++++++++++++- .../study/rest/PatientRecordController.java | 6 +-- .../study/rest/util/RecordFilterMapper.java | 6 ++- .../kbss/study/model/RecordPhaseTest.java | 44 ++++++++++++++++--- .../rest/util/RecordFilterMapperTest.java | 9 ++-- 5 files changed, 85 insertions(+), 16 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java b/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java index 20a09a20..666b602b 100644 --- a/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java +++ b/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java @@ -31,7 +31,7 @@ public String getIri() { * @return matching {@code RecordPhase} * @throws IllegalArgumentException When no matching phase is found */ - public static RecordPhase fromString(String iri) { + public static RecordPhase fromIri(String iri) { for (RecordPhase p : values()) { if (p.getIri().equals(iri)) { return p; @@ -39,4 +39,38 @@ public static RecordPhase fromString(String iri) { } throw new IllegalArgumentException("Unknown record phase identifier '" + iri + "'."); } + + /** + * Returns {@link RecordPhase} with the specified constant name. + * + * @param name record phase name + * @return matching {@code RecordPhase} + * @throws IllegalArgumentException When no matching phase is found + */ + public static RecordPhase fromName(String name) { + for (RecordPhase p : values()) { + if (p.name().equalsIgnoreCase(name)) { + return p; + } + } + throw new IllegalArgumentException("Unknown record phase '" + name + "'."); + } + + /** + * Returns a {@link RecordPhase} with the specified IRI or constant name. + *

+ * This function first tries to find the enum constant by IRI. If it is not found, constant name matching is + * attempted. + * + * @param identification Constant IRI or name to find match by + * @return matching {@code RecordPhase} + * @throws IllegalArgumentException When no matching phase is found + */ + public static RecordPhase fromIriOrName(String identification) { + try { + return fromIri(identification); + } catch (IllegalArgumentException e) { + return fromName(identification); + } + } } diff --git a/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java b/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java index 2774e90c..5bc4d9ff 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java +++ b/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java @@ -101,10 +101,10 @@ public ResponseEntity createRecord(@RequestBody PatientRecord record) { @PostMapping(value = "/import", consumes = MediaType.APPLICATION_JSON_VALUE) public RecordImportResult importRecords(@RequestBody List records, - @RequestParam(name = "phase", required = false) String phaseIri) { + @RequestParam(name = "phase", required = false) String phase) { final RecordImportResult importResult; - if (phaseIri != null) { - final RecordPhase targetPhase = RecordPhase.fromString(phaseIri); + if (phase != null) { + final RecordPhase targetPhase = RecordPhase.fromIriOrName(phase); importResult = recordService.importRecords(records, targetPhase); } else { importResult = recordService.importRecords(records); diff --git a/src/main/java/cz/cvut/kbss/study/rest/util/RecordFilterMapper.java b/src/main/java/cz/cvut/kbss/study/rest/util/RecordFilterMapper.java index f89e5a75..d565f059 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/util/RecordFilterMapper.java +++ b/src/main/java/cz/cvut/kbss/study/rest/util/RecordFilterMapper.java @@ -1,5 +1,6 @@ package cz.cvut.kbss.study.rest.util; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams; import cz.cvut.kbss.study.rest.exception.BadRequestException; import org.slf4j.Logger; @@ -10,11 +11,11 @@ import java.time.LocalDate; import java.time.format.DateTimeParseException; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.stream.Collectors; /** * Maps query parameters to {@link RecordFilterParams} instances. @@ -66,7 +67,8 @@ public static RecordFilterParams constructRecordFilter(MultiValueMap(params.getOrDefault(PHASE_ID_PARAM, Collections.emptyList()))); + result.setPhaseIds(params.getOrDefault(PHASE_ID_PARAM, Collections.emptyList()).stream() + .map(s -> RecordPhase.fromIriOrName(s).getIri()).collect(Collectors.toSet())); return result; } diff --git a/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java b/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java index 59ad29af..ce3d204a 100644 --- a/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java +++ b/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java @@ -8,19 +8,51 @@ class RecordPhaseTest { @Test - void fromStringReturnsMatchingRecordPhase() { + void fromIriReturnsMatchingRecordPhase() { for (RecordPhase p : RecordPhase.values()) { - assertEquals(p, RecordPhase.fromString(p.getIri())); + assertEquals(p, RecordPhase.fromIri(p.getIri())); } } @Test - void fromStringThrowsIllegalArgumentForUnknownPhaseIri() { - assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromString(Generator.generateUri().toString())); + void fromIriThrowsIllegalArgumentForUnknownPhaseIri() { + assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromIri(Generator.generateUri().toString())); } @Test - void fromStringThrowsIllegalArgumentForNullArgument() { - assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromString(null)); + void fromIriThrowsIllegalArgumentForNullArgument() { + assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromIri(null)); + } + + @Test + void fromNameReturnsMatchingRecordPhase() { + for (RecordPhase p : RecordPhase.values()) { + assertEquals(p, RecordPhase.fromName(p.name())); + } + } + + @Test + void fromNameMatchesIgnoringCase() { + for (RecordPhase p : RecordPhase.values()) { + assertEquals(p, RecordPhase.fromName(p.name().toUpperCase())); + } + } + + @Test + void fromNameThrowsIllegalArgumentForUnknownPhaseIri() { + assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromName("unknown")); + } + + @Test + void fromNameThrowsIllegalArgumentForNullArgument() { + assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromName(null)); + } + + @Test + void fromNameOrIriMatchesPhaseByIriAndName() { + for (RecordPhase p : RecordPhase.values()) { + assertEquals(p, RecordPhase.fromIriOrName(p.getIri())); + assertEquals(p, RecordPhase.fromIriOrName(p.name())); + } } } \ No newline at end of file diff --git a/src/test/java/cz/cvut/kbss/study/rest/util/RecordFilterMapperTest.java b/src/test/java/cz/cvut/kbss/study/rest/util/RecordFilterMapperTest.java index 959b08c0..ec36d28d 100644 --- a/src/test/java/cz/cvut/kbss/study/rest/util/RecordFilterMapperTest.java +++ b/src/test/java/cz/cvut/kbss/study/rest/util/RecordFilterMapperTest.java @@ -1,5 +1,6 @@ package cz.cvut.kbss.study.rest.util; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -40,16 +41,16 @@ static Stream testValues() { )), new RecordFilterParams("1111111", LocalDate.EPOCH, LocalDate.now(), Collections.emptySet())), Arguments.of(new LinkedMultiValueMap<>(Map.of( "institution", List.of("1111111"), - "phase", List.of("http://example.org/phaseOne", "http://example.org/phaseTwo") + "phase", List.of(RecordPhase.open.getIri(), RecordPhase.completed.name()) )), new RecordFilterParams("1111111", LocalDate.EPOCH, LocalDate.now(), - Set.of("http://example.org/phaseOne", "http://example.org/phaseTwo"))), + Set.of(RecordPhase.open.getIri(), RecordPhase.completed.getIri()))), Arguments.of(new LinkedMultiValueMap<>(Map.of( "minDate", List.of(LocalDate.now().minusYears(1).toString()), "maxDate", List.of(LocalDate.now().minusDays(1).toString()), "institution", List.of("1111111"), - "phase", List.of("http://example.org/phaseOne") + "phase", List.of(RecordPhase.published.name()) )), new RecordFilterParams("1111111", LocalDate.now().minusYears(1), LocalDate.now().minusDays(1), - Set.of("http://example.org/phaseOne"))) + Set.of(RecordPhase.published.getIri()))) ); } } \ No newline at end of file