From edd79b1c28968e7662cc25b81a67b2eb4942f449 Mon Sep 17 00:00:00 2001 From: Joris Wouter Jonkers <74975850+ExtraToast@users.noreply.github.com> Date: Sat, 5 Sep 2026 23:51:26 +0200 Subject: [PATCH] fix(api): a request that names a page or a size gets one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /users?size=500 answered with every user. Spring's resolver reads its fallback unless the request carries both `page` and `size`, and this api's fallback is unpaged, so naming one half of a page silently bought nothing. A caller that names a size is one that cannot hold an unbounded answer, and it was handed exactly that. Measured on four users, before: no parameters, `?size=1` and `?page=0` all answered with four rows; only `?page=0&size=1` paged. After: the two halves each page, with 0 and the documented 20 filling in for whichever was not named, and a request naming neither still answers unpaged, which is what the listing is for. The two callers that named a size wanted the whole listing and were getting it by accident: loadMemberAccounts and UserPicker both filter what they hold, so they now ask for the listing and name no size. Left as they were, this change would have truncated them at 500 — the bug #1139 has just been fixed. Closes #1145 --- .../api/user/web/UserControllerIT.kt | 32 ++++++++++++++ .../api/platform/config/PagingConfig.kt | 43 ++++++++++++++----- .../src/components/form/fields/UserPicker.vue | 4 +- .../src/domains/user/adapters/users.ts | 4 +- .../unit/domains/user/adapters/users.test.ts | 4 +- 5 files changed, 73 insertions(+), 14 deletions(-) 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 () => {