diff --git a/services/api/src/integrationTest/kotlin/net/blueshell/api/user/web/UserControllerIT.kt b/services/api/src/integrationTest/kotlin/net/blueshell/api/user/web/UserControllerIT.kt index 73b5e89b7..a4fcf76f7 100644 --- a/services/api/src/integrationTest/kotlin/net/blueshell/api/user/web/UserControllerIT.kt +++ b/services/api/src/integrationTest/kotlin/net/blueshell/api/user/web/UserControllerIT.kt @@ -245,6 +245,38 @@ class UserControllerIT : UserTestSupport() { .andExpect(jsonPath("$.content").isArray) } + @Test + fun `a size without a page is a size, not a suggestion`() { + val board = createUserWithRole(Role.BOARD) + repeat(3) { createUserWithRole(Role.MEMBER) } + + mvc.perform(get("/users").param("size", "1").with(bearer(board))) + .andExpect(status().isOk) + .andExpect(jsonPath("$.content.length()").value(1)) + .andExpect(jsonPath("$.page.size").value(1)) + } + + @Test + fun `a page without a size is paged at the default rather than not at all`() { + val board = createUserWithRole(Role.BOARD) + repeat(3) { createUserWithRole(Role.MEMBER) } + + mvc.perform(get("/users").param("page", "0").with(bearer(board))) + .andExpect(status().isOk) + .andExpect(jsonPath("$.page.size").value(20)) + } + + @Test + fun `naming no paging at all still answers with everybody`() { + val board = createUserWithRole(Role.BOARD) + repeat(3) { createUserWithRole(Role.MEMBER) } + + mvc.perform(get("/users").with(bearer(board))) + .andExpect(status().isOk) + .andExpect(jsonPath("$.content.length()").value(4)) + .andExpect(jsonPath("$.page.size").value(4)) + } + @Test fun `a search finds the user it names rather than the page they fall on`() { val board = createUserWithRole(Role.BOARD) diff --git a/services/api/src/main/kotlin/net/blueshell/api/platform/config/PagingConfig.kt b/services/api/src/main/kotlin/net/blueshell/api/platform/config/PagingConfig.kt index aaa67c4e3..9750974e9 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/platform/config/PagingConfig.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/platform/config/PagingConfig.kt @@ -1,20 +1,43 @@ package net.blueshell.api.platform.config -import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration +import org.springframework.core.MethodParameter +import org.springframework.core.Ordered +import org.springframework.core.annotation.Order import org.springframework.data.domain.Pageable import org.springframework.data.web.PageableHandlerMethodArgumentResolver -import org.springframework.data.web.config.PageableHandlerMethodArgumentResolverCustomizer +import org.springframework.web.method.support.HandlerMethodArgumentResolver +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer +/** + * A listing answers unpaged when nobody asked for a page, and answers the page anybody did ask + * for, whichever half of it they named. + * + * Spring's resolver reads a fallback when the request does not carry **both** `page` and `size`, + * and this api's fallback is unpaged — so `?size=500` was answered with the whole table. A caller + * naming a size is one that cannot hold an unbounded answer, and it was handed exactly that, + * silently. See #1145. + */ @Configuration -internal class PagingConfig { - @Bean - fun unpagedByDefault(): PageableHandlerMethodArgumentResolverCustomizer { - return PageableHandlerMethodArgumentResolverCustomizer { resolver: PageableHandlerMethodArgumentResolver? -> - resolver!!.setFallbackPageable( +@Order(Ordered.HIGHEST_PRECEDENCE) +internal class PagingConfig : WebMvcConfigurer { + + override fun addArgumentResolvers(resolvers: MutableList) { + resolvers.add(EitherHalfMeansPaged()) + } + + private class EitherHalfMeansPaged : PageableHandlerMethodArgumentResolver() { + override fun getPageable(parameter: MethodParameter, pageString: String?, sizeString: String?): Pageable = + if (pageString == null && sizeString == null) { Pageable.unpaged() - ) - } + } else { + // The half that was named decides; the other takes the value the api documents. + super.getPageable(parameter, pageString ?: "0", sizeString ?: DEFAULT_PAGE_SIZE.toString()) + } } -} + private companion object { + /** What the OpenAPI document says `size` defaults to, so the spec and the api agree. */ + const val DEFAULT_PAGE_SIZE = 20 + } +} diff --git a/services/frontend/src/components/form/fields/UserPicker.vue b/services/frontend/src/components/form/fields/UserPicker.vue index ca283a6fd..70ddd056f 100644 --- a/services/frontend/src/components/form/fields/UserPicker.vue +++ b/services/frontend/src/components/form/fields/UserPicker.vue @@ -20,7 +20,9 @@ async function loadUsers() { if (loaded.value || loading.value) return loading.value = true try { - const resp = await findUsers({ query: { size: 500 } }) + // No size: this picker filters what it holds, so it wants the whole listing. The 500 it used + // to name never bounded anything — the answer was everybody regardless (#1145). + const resp = await findUsers({}) const content = resp.data?.content ?? [] items.value = content.slice().sort((a, b) => { const left = a.fullName ?? a.email ?? "" diff --git a/services/frontend/src/domains/user/adapters/users.ts b/services/frontend/src/domains/user/adapters/users.ts index 9abbc3426..a99f5daf0 100644 --- a/services/frontend/src/domains/user/adapters/users.ts +++ b/services/frontend/src/domains/user/adapters/users.ts @@ -26,7 +26,9 @@ export interface MemberAccount { * emptiness, a refused request tells a board member that nobody here has an account. */ export async function loadMemberAccounts(): Promise { - const res = await findUsers({query: {size: 500}}) + // No size: this wants the whole listing, and a size that named a bound never gave one — it + // was answered with everybody anyway (#1145). Saying so beats a number that did nothing. + const res = await findUsers({}) if (res.error || !res.data?.content) return null return res.data.content .filter(user => user.id != null) diff --git a/services/frontend/tests/unit/domains/user/adapters/users.test.ts b/services/frontend/tests/unit/domains/user/adapters/users.test.ts index 716e5739a..34be2e2d1 100644 --- a/services/frontend/tests/unit/domains/user/adapters/users.test.ts +++ b/services/frontend/tests/unit/domains/user/adapters/users.test.ts @@ -10,12 +10,12 @@ vi.mock("@/services/api", async (importOriginal) => ({ const page = (content: unknown[]) => ({data: {content}}) as never describe("loadMemberAccounts", () => { - it("asks for one page big enough to filter where it is used", async () => { + it("asks for the whole listing rather than a page whose size it would have to guess", async () => { vi.mocked(findUsers).mockResolvedValue(page([])) await loadMemberAccounts() - expect(findUsers).toHaveBeenCalledWith({query: {size: 500}}) + expect(findUsers).toHaveBeenCalledWith({}) }) it("names an account by the full name on it", async () => {