diff --git a/backend/tests/app_version_order.rs b/backend/tests/app_version_order.rs index 2940db058d..66fb01fbfd 100644 --- a/backend/tests/app_version_order.rs +++ b/backend/tests/app_version_order.rs @@ -155,6 +155,14 @@ async fn test_app_head_follows_the_append_order_not_the_timestamps( appended[10..20], "the next page carries on where the first left off, skipping nothing" ); + // A page past the end runs off it rather than overflowing into one. (The clamp on an + // asked-for size is pinned where it lives, in `paginate_optional`'s own test.) + assert!( + versions_at("?per_page=10&page=99999999") + .await? + .is_empty(), + "a page past the end is empty" + ); Ok(()) } diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index fda10cd128..360f75a48c 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -52,7 +52,7 @@ use windmill_common::{ schedule::Schedule, triggers::MovedNativeTrigger, utils::{ - http_get_from_hub, not_found_if_none, paginate, paginate_without_limits, Pagination, + http_get_from_hub, not_found_if_none, paginate, paginate_optional, Pagination, RunnableKind, StripPath, }, }; @@ -937,7 +937,7 @@ async fn get_flow_history( check_scopes(&authed, || format!("flows:read:{}", path))?; // Unasked-for, this listing stays whole: the history panels, the restart picker and // the CLI all read it without paging. The diff picker asks for a page. - let (per_page, offset) = paginate_without_limits(pagination); + let (per_page, offset) = paginate_optional(pagination); let mut tx = user_db.begin(&authed).await?; let flows = sqlx::query_as!( diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 665d0315db..10e272a832 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -55,7 +55,7 @@ use windmill_common::{ min_version_supports_runnable_settings_v0, RunnableSettings, RunnableSettingsTrait, }, scripts::{hash_script, ScriptRunnableSettingsHandle, ScriptRunnableSettingsInline}, - utils::{paginate_without_limits, WarnAfterExt}, + utils::{paginate_optional, paginate_without_limits, WarnAfterExt}, worker::CLOUD_HOSTED, }; use windmill_object_store::upload_artifact_to_store; @@ -3094,7 +3094,7 @@ async fn get_script_history( check_scopes(&authed, || format!("scripts:read:{}", path))?; // Unasked-for, this listing stays whole: the deployment-history panels, the restart // picker and the CLI all read it without paging. The diff picker asks for a page. - let (per_page, offset) = paginate_without_limits(pagination); + let (per_page, offset) = paginate_optional(pagination); let mut tx = user_db.begin(&authed).await?; let query_result = sqlx::query!( "SELECT s.hash as hash, dm.deployment_msg as deployment_msg, s.created_at as created_at, s.created_by as created_by diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 9b9ee17b1a..34a29789ed 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -69,7 +69,7 @@ use windmill_common::{ user_drafts::{overlay_or_draft_only, DraftUserRef, UserDraftItemKind, WithDraftOverlay}, users::username_to_permissioned_as, utils::{ - http_get_from_hub, not_found_if_none, paginate, paginate_without_limits, + http_get_from_hub, not_found_if_none, paginate, paginate_optional, query_elems_from_hub, require_admin, strip_json_nul, Pagination, RunnableKind, StripPath, }, variables::{build_crypt, build_crypt_with_key_suffix, encrypt}, @@ -1243,7 +1243,7 @@ async fn get_app_history( check_scopes(&authed, || format!("apps:read:{}", &path))?; // Unasked-for, this listing stays whole: the deployment-history panel reads it // without paging. The diff picker asks for a page. - let (per_page, offset) = paginate_without_limits(pagination); + let (per_page, offset) = paginate_optional(pagination); let mut tx = user_db.begin(&authed).await?; // Newest first in the order the versions were deployed, which is their position in // `app.versions` and not `created_at`: the latter is the deploying transaction's diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 4b93bb89ff..3fc2d64e39 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -479,6 +479,25 @@ pub fn paginate(pagination: Pagination) -> (usize, usize) { (per_page, offset) } +/// [`paginate`] for a listing that answers whole unless a size is asked for: the deploy +/// histories, which the history panels and the CLI read unpaged while the diff picker takes +/// a page at a time. An asked-for size is still clamped, and the offset saturates rather +/// than wrapping, so no caller can turn this into an unbounded scan or a negative bind. +pub fn paginate_optional(pagination: Pagination) -> (usize, usize) { + let per_page = pagination + .per_page + .unwrap_or(MAX_PER_PAGE) + .max(1) + .min(MAX_PER_PAGE); + let offset = pagination + .page + .unwrap_or(1) + .max(1) + .saturating_sub(1) + .saturating_mul(per_page); + (per_page, offset) +} + pub fn paginate_without_limits(pagination: Pagination) -> (usize, usize) { let per_page = pagination.per_page.unwrap_or(MAX_PER_PAGE); let offset = (pagination.page.unwrap_or(1).max(1) - 1) * per_page; @@ -1657,6 +1676,31 @@ pub fn truncate_with_ellipsis(s: &str, max_chars: usize) -> String { mod tests { use super::*; + #[test] + fn test_paginate_optional_answers_whole_but_bounds_what_is_asked_for() { + // Nothing asked for: the whole listing, which is what the history panels and the + // CLI read. + assert_eq!( + paginate_optional(Pagination { page: None, per_page: None }), + (MAX_PER_PAGE, 0) + ); + assert_eq!( + paginate_optional(Pagination { page: Some(3), per_page: Some(20) }), + (20, 40) + ); + // An asked-for size is still capped, so no caller turns this into an unbounded scan. + assert_eq!( + paginate_optional(Pagination { page: None, per_page: Some(usize::MAX) }), + (MAX_PER_PAGE, 0) + ); + // And the offset saturates instead of wrapping into a negative bind. + assert_eq!( + paginate_optional(Pagination { page: Some(usize::MAX), per_page: Some(20) }).0, + 20 + ); + assert!(paginate_optional(Pagination { page: Some(usize::MAX), per_page: Some(20) }).1 > 0); + } + /// A 5-field crontab line is the most common way to get a schedule rejected, and both /// parsers report it in terms a crontab user cannot act on, so the seconds field and the /// equivalent expression must reach the caller for v1 and v2 alike. diff --git a/frontend/src/lib/components/DiffDrawer.svelte b/frontend/src/lib/components/DiffDrawer.svelte index 0958425f61..2e5810e036 100644 --- a/frontend/src/lib/components/DiffDrawer.svelte +++ b/frontend/src/lib/components/DiffDrawer.svelte @@ -181,7 +181,9 @@ sendUserToast(`Could not load older versions: ${e?.body ?? e?.message ?? e}`, true) } } finally { - loadingMore = false + // Guarded like the writes above: an outlived request clearing this would hand + // the drawer that replaced it a second concurrent page. + if (generation === versionListGeneration) loadingMore = false } } diff --git a/frontend/src/lib/components/FlowBuilder.svelte b/frontend/src/lib/components/FlowBuilder.svelte index 9fc92252a1..cbd6a18207 100644 --- a/frontend/src/lib/components/FlowBuilder.svelte +++ b/frontend/src/lib/components/FlowBuilder.svelte @@ -1138,10 +1138,11 @@ /** Deployed versions for the diff picker, newest first. Best-effort: losing the * list costs the picker, not the diff. */ - async function deployedVersionOptions(page = 1) { + /** Throws: the drawer says so and lets the reader ask for the same page again. */ + async function fetchVersionPage(page: number) { const path = userDraftPath || initialPath if (!opWorkspace || !path) return undefined - try { + { const history = await FlowService.getFlowHistory({ workspace: opWorkspace, path, @@ -1168,16 +1169,28 @@ isHead } }) + } + } + + /** The first page, best-effort: losing it costs the picker, not the diff. */ + async function deployedVersionOptions() { + try { + return await fetchVersionPage(1) } catch { return undefined } } - /** Hands the drawer the next page each time the reader asks for one. Held here rather - * than in the drawer because the page number belongs to this item's history. */ + /** Hands the drawer the next page each time the reader asks for one. The page number + * belongs to this item's history, so it lives here — and only moves once a page has + * actually arrived, or a failed request would skip it. */ function moreVersionsLoader() { - let page = 1 - return async () => deployedVersionOptions(++page) + let loaded = 1 + return async () => { + const page = await fetchVersionPage(loaded + 1) + loaded += 1 + return page + } } /** The opening this editor claimed last. A path change remounts this editor while the diff --git a/frontend/src/lib/components/ScriptBuilder.svelte b/frontend/src/lib/components/ScriptBuilder.svelte index e792ce7435..948425f281 100644 --- a/frontend/src/lib/components/ScriptBuilder.svelte +++ b/frontend/src/lib/components/ScriptBuilder.svelte @@ -844,9 +844,10 @@ /** Deployed versions to offer in the diff picker, newest first. Best-effort: a * failure here costs the picker, not the diff, so the drawer still opens on the * head. `deployment_msg` is all the history endpoint carries besides the hash. */ - async function deployedVersionOptions(headHash: string | undefined, page = 1) { + /** Throws: the drawer says so and lets the reader ask for the same page again. */ + async function fetchVersionPage(headHash: string | undefined, page: number) { if (!opWorkspace || !userDraftPath) return undefined - try { + { const history = await ScriptService.getScriptHistoryByPath({ workspace: opWorkspace, path: userDraftPath, @@ -870,16 +871,28 @@ isHead } }) + } + } + + /** The first page, best-effort: losing it costs the picker, not the diff. */ + async function deployedVersionOptions(headHash: string | undefined) { + try { + return await fetchVersionPage(headHash, 1) } catch { return undefined } } - /** Hands the drawer the next page each time the reader asks for one. Held here rather - * than in the drawer because the page number belongs to this item's history. */ + /** Hands the drawer the next page each time the reader asks for one. The page number + * belongs to this item's history, so it lives here — and only moves once a page has + * actually arrived, or a failed request would skip it. */ function moreVersionsLoader(headHash: string | undefined) { - let page = 1 - return async () => deployedVersionOptions(headHash, ++page) + let loaded = 1 + return async () => { + const page = await fetchVersionPage(headHash, loaded + 1) + loaded += 1 + return page + } } /** The opening this editor claimed last. A path change remounts this editor while the diff --git a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte index 4bef3b5599..84f0f06d5a 100644 --- a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte @@ -433,9 +433,10 @@ /** Deployed versions for the diff picker, newest first. Best-effort: losing the * list costs the picker, not the diff. */ - async function deployedVersionOptions(page = 1) { + /** Throws: the drawer says so and lets the reader ask for the same page again. */ + async function fetchVersionPage(page: number) { if (!opWorkspace || !appPath) return undefined - try { + { const history = await AppService.getAppHistoryByPath({ workspace: opWorkspace, path: appPath, @@ -460,16 +461,28 @@ isHead } }) + } + } + + /** The first page, best-effort: losing it costs the picker, not the diff. */ + async function deployedVersionOptions() { + try { + return await fetchVersionPage(1) } catch { return undefined } } - /** Hands the drawer the next page each time the reader asks for one. Held here rather - * than in the drawer because the page number belongs to this item's history. */ + /** Hands the drawer the next page each time the reader asks for one. The page number + * belongs to this item's history, so it lives here — and only moves once a page has + * actually arrived, or a failed request would skip it. */ function moreVersionsLoader() { - let page = 1 - return async () => deployedVersionOptions(++page) + let loaded = 1 + return async () => { + const page = await fetchVersionPage(loaded + 1) + loaded += 1 + return page + } } /** The opening this editor claimed last. A path change remounts this editor while the