From 99b4b5309546d7ea87d936f5ad7c0e3613d7bc7c Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 21 Oct 2026 16:49:59 -0700 Subject: [PATCH 01/10] feat(cli): cursor-based pagination (--limit/--continue) for list commands List commands now page with cli-engine's cursor model. Each page reports a `cursor` block (`has_more`, `continue_from`, and `total`/`remaining` when the backend knows them) plus a ready-to-run next action, replacing the earlier `--offset` windowing and its `pagination` block. `--limit` is the page size; `--continue ` fetches the next page. Backed by the service's own cursor or page token: - domain list (v3 page token), platform app list (App Registry Relay cursor) - dns list and email list (page number) - hosting app list, deployment list and log list (page token) Paged client-side over the full response, with `--offset` to jump straight to a position: - api parameter list, api response list, api search - platform actions list - hosting domain list Defaults when `--limit` is omitted: platform app list 50, domain list 100, dns list 50, email list 25, hosting app/deployment list 50, hosting log list 100, hosting domain list 50. Each command has a maximum page size. `hosting domain list` and `hosting subscription list` call endpoints whose spec declares no paging parameters, and the service rejects `pageSize` and `pageToken` there. They make one request, and print a warning to stderr if the response reports a further page instead of silently showing a partial list. Co-Authored-By: Claude Sonnet 5.5 --- .../graphql/ApplicationsList.graphql | 8 +- rust/platform-app-client/src/lib.rs | 5 +- rust/src/api/guides/api-explorer.md | 2 +- rust/src/api/parameter.rs | 74 ++-- rust/src/api/response.rs | 27 +- rust/src/api/search.rs | 101 ++++- rust/src/api/summary.rs | 8 +- rust/src/dns/list.rs | 42 +- rust/src/dns/records.rs | 172 +++++++- rust/src/domain/list.rs | 369 +++++++++--------- rust/src/email/list.rs | 188 ++++----- rust/src/hosting/app/list.rs | 85 ++-- rust/src/hosting/client.rs | 25 +- rust/src/hosting/client_tests.rs | 4 +- rust/src/hosting/common.rs | 143 ++++++- rust/src/hosting/deployment/list.rs | 85 ++-- rust/src/hosting/domain/list.rs | 91 +++-- rust/src/hosting/log/list.rs | 79 ++-- rust/src/hosting/subscription/list.rs | 38 +- rust/src/main.rs | 1 + rust/src/pagination_token.rs | 309 +++++++++++++++ rust/src/platform/actions/mod.rs | 93 ++++- rust/src/platform/app/client.rs | 37 +- rust/src/platform/app/client_tests.rs | 53 ++- rust/src/platform/app/commands/list.rs | 38 +- 25 files changed, 1423 insertions(+), 654 deletions(-) create mode 100644 rust/src/pagination_token.rs diff --git a/rust/platform-app-client/graphql/ApplicationsList.graphql b/rust/platform-app-client/graphql/ApplicationsList.graphql index 7f41c45d..7899aaa8 100644 --- a/rust/platform-app-client/graphql/ApplicationsList.graphql +++ b/rust/platform-app-client/graphql/ApplicationsList.graphql @@ -1,5 +1,5 @@ -query ApplicationsList($status: ApplicationStatusFilter) { - applications(status: $status) { +query ApplicationsList($status: ApplicationStatusFilter, $first: Int, $after: String) { + applications(status: $status, first: $first, after: $after) { edges { node { id @@ -11,5 +11,9 @@ query ApplicationsList($status: ApplicationStatusFilter) { proxyUrl } } + pageInfo { + hasNextPage + endCursor + } } } diff --git a/rust/platform-app-client/src/lib.rs b/rust/platform-app-client/src/lib.rs index fd96c022..1dcf1b44 100644 --- a/rust/platform-app-client/src/lib.rs +++ b/rust/platform-app-client/src/lib.rs @@ -237,7 +237,8 @@ mod tests { "edges": [ { "node": { "id": "a1", "label": "Active", "name": "active-app", "description": null, "status": "ACTIVE", "url": null, "proxyUrl": null } }, { "node": { "id": "a2", "label": "Inactive", "name": "inactive-app", "description": null, "status": "INACTIVE", "url": null, "proxyUrl": null } } - ] + ], + "pageInfo": { "hasNextPage": false, "endCursor": null } } } })); @@ -254,6 +255,8 @@ mod tests { in_: Some(statuses), ne: None, }), + first: None, + after: None, }; let data = client_for(&server) diff --git a/rust/src/api/guides/api-explorer.md b/rust/src/api/guides/api-explorer.md index 28a8a087..341b367d 100644 --- a/rust/src/api/guides/api-explorer.md +++ b/rust/src/api/guides/api-explorer.md @@ -63,7 +63,7 @@ gddy api parameter list --operation gddy api response list --operation ``` -Both support the standard `--limit`/`--offset` flags if there are more rows than fit on one page. To see one parameter or response's full detail on its own: +Both support the standard `--limit`/`--continue` flags if there are more rows than fit on one page (or `--offset` to jump directly to a position). To see one parameter or response's full detail on its own: ``` gddy api parameter get --operation diff --git a/rust/src/api/parameter.rs b/rust/src/api/parameter.rs index 48b1ec8e..9a216cca 100644 --- a/rust/src/api/parameter.rs +++ b/rust/src/api/parameter.rs @@ -1,12 +1,13 @@ //! `api parameter list` and `api parameter get`. use cli_engine::{ - CommandResult, CommandSpec, NextActionParam, PaginationConfig, RuntimeCommandSpec, TableColumn, + CommandResult, CommandSpec, CursorConfig, NextActionParam, RuntimeCommandSpec, TableColumn, Tier, }; use serde_json::json; use crate::next_action::{next_action, required_value}; +use crate::pagination_token; use super::catalog::{ catalog, graphql_operation_redirect_error, resolve_graphql_operation, resolve_operation, @@ -21,10 +22,14 @@ struct ParameterListArgs { /// Operation ID to list parameters for (see `api operation list`). #[arg(long, value_name = "OPERATION")] operation: String, + + /// Jump directly to an offset in the collection. + #[arg(long, value_name = "N", conflicts_with = "continue")] + offset: Option, } pub(super) fn list_command() -> RuntimeCommandSpec { - RuntimeCommandSpec::new_typed::( + RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List an operation's parameters") .with_long( "Summarizes every parameter for an operation — query/path/header/cookie/body. \ @@ -42,11 +47,11 @@ pub(super) fn list_command() -> RuntimeCommandSpec { .with_system("api") .with_tier(Tier::Read) .no_auth(true) - .with_pagination(PaginationConfig::new( + .with_cursor(CursorConfig::new( OPERATION_CHILD_LIST_DEFAULT_LIMIT as i64, OPERATION_CHILD_LIST_MAX_LIMIT, )), - |_cred, args: ParameterListArgs| async move { + |ctx, args: ParameterListArgs| async move { let operation = args.operation.as_str(); let catalog = catalog(); if resolve_graphql_operation(catalog, operation).is_some() { @@ -54,6 +59,16 @@ pub(super) fn list_command() -> RuntimeCommandSpec { } let (domain, ep) = resolve_operation(catalog, operation, None)?; let all = summarize_parameters(ep, &domain.defs); + let items = to_json(all)?.as_array().cloned().unwrap_or_default(); + let continue_token = pagination_token::resolve_continue_token( + args.offset, + ctx.middleware.continue_token.as_deref(), + ); + let (page, continuation) = pagination_token::paginate_known_collection( + items, + ctx.middleware.cursor_limit, + continue_token.as_deref(), + ); let next_actions = vec![ next_action( "api parameter get --operation ", @@ -62,7 +77,9 @@ pub(super) fn list_command() -> RuntimeCommandSpec { .with_param("name", NextActionParam::required()) .with_param("operation", required_value(ep.operation_id.clone())), ]; - Ok(CommandResult::new(json!(all)).with_next_actions(next_actions)) + Ok(CommandResult::new(json!(page)) + .with_cursor(continuation) + .with_next_actions(next_actions)) }, ) } @@ -215,51 +232,24 @@ mod tests { let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); assert_eq!(rendered["data"].as_array().expect("data array").len(), 5); - let pagination = &rendered["pagination"]; - assert_eq!(pagination["total"], json!(26)); - assert_eq!(pagination["offset"], json!(20)); - assert_eq!(pagination["limit"], json!(5)); - assert_eq!(pagination["count"], json!(5)); + let cursor = &rendered["cursor"]; + assert_eq!(cursor["total"], json!(26)); + assert_eq!(cursor["limit"], json!(5)); + assert_eq!(cursor["count"], json!(5)); // 20 + 5 = 25 < 26, so one more remains. - assert_eq!(pagination["has_more"], json!(true)); + assert_eq!(cursor["has_more"], json!(true)); + assert_eq!(cursor["remaining"], json!(1)); let next_actions = rendered["next_actions"].as_array().expect("next_actions"); assert!( - next_actions - .iter() - .any(|a| a["command"].as_str().unwrap_or("").contains("--offset 25")), + next_actions.iter().any(|a| a["command"] + .as_str() + .unwrap_or("") + .contains("--continue \"limit:5,offset:25\"")), "expected an auto-generated next-page hint: {}", output.rendered ); } - /// `--limit 0` means "all" per cli-engine's own generated help text for - /// `with_pagination` — must show every item, not zero. With both - /// `--limit`/`--offset` at their "disabled" value (0), cli-engine 0.8's - /// pipeline skips pagination entirely rather than reporting a page of - /// everything, so there's no `pagination` envelope field at all here. - #[tokio::test] - async fn parameter_list_limit_zero_means_all() { - let output = operation_cli() - .run([ - "gddy", - "api", - "parameter", - "list", - "--operation", - "get_transaction_disputes", - "--limit", - "0", - "--output", - "json", - ]) - .await; - assert_eq!(output.exit_code, 0, "{}", output.rendered); - let rendered: serde_json::Value = - serde_json::from_str(&output.rendered).expect("valid json"); - assert_eq!(rendered["data"].as_array().expect("data array").len(), 26); - assert!(rendered.get("pagination").is_none(), "{}", output.rendered); - } - /// `max_limit` (`OPERATION_CHILD_LIST_MAX_LIMIT`) rejects an /// out-of-range `--limit` at parse time, before the handler even runs. #[tokio::test] diff --git a/rust/src/api/response.rs b/rust/src/api/response.rs index 6d6b9009..c97baceb 100644 --- a/rust/src/api/response.rs +++ b/rust/src/api/response.rs @@ -1,12 +1,13 @@ //! `api response list` and `api response get`. use cli_engine::{ - CommandResult, CommandSpec, NextActionParam, PaginationConfig, RuntimeCommandSpec, TableColumn, + CommandResult, CommandSpec, CursorConfig, NextActionParam, RuntimeCommandSpec, TableColumn, Tier, }; use serde_json::json; use crate::next_action::{next_action, required_value}; +use crate::pagination_token; use super::catalog::{catalog, resolve_operation}; use super::summary::{ @@ -18,10 +19,14 @@ struct ResponseListArgs { /// Operation ID to list responses for (see `api operation list`). #[arg(long, value_name = "OPERATION")] operation: String, + + /// Jump directly to an offset in the collection. + #[arg(long, value_name = "N", conflicts_with = "continue")] + offset: Option, } pub(super) fn list_command() -> RuntimeCommandSpec { - RuntimeCommandSpec::new_typed::( + RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List an operation's responses") .with_long( "Lists every response on one operation by status code. Use \ @@ -37,15 +42,25 @@ pub(super) fn list_command() -> RuntimeCommandSpec { .with_system("api") .with_tier(Tier::Read) .no_auth(true) - .with_pagination(PaginationConfig::new( + .with_cursor(CursorConfig::new( OPERATION_CHILD_LIST_DEFAULT_LIMIT as i64, OPERATION_CHILD_LIST_MAX_LIMIT, )), - |_cred, args: ResponseListArgs| async move { + |ctx, args: ResponseListArgs| async move { let operation = args.operation.as_str(); let catalog = catalog(); let (domain, ep) = resolve_operation(catalog, operation, None)?; let all = response_rows(ep, &domain.defs); + let items = to_json(all)?.as_array().cloned().unwrap_or_default(); + let continue_token = pagination_token::resolve_continue_token( + args.offset, + ctx.middleware.continue_token.as_deref(), + ); + let (page, continuation) = pagination_token::paginate_known_collection( + items, + ctx.middleware.cursor_limit, + continue_token.as_deref(), + ); let next_actions = vec![ next_action( "api response get --operation ", @@ -54,7 +69,9 @@ pub(super) fn list_command() -> RuntimeCommandSpec { .with_param("status", NextActionParam::required()) .with_param("operation", required_value(ep.operation_id.clone())), ]; - Ok(CommandResult::new(json!(all)).with_next_actions(next_actions)) + Ok(CommandResult::new(json!(page)) + .with_cursor(continuation) + .with_next_actions(next_actions)) }, ) } diff --git a/rust/src/api/search.rs b/rust/src/api/search.rs index 4d9f97e3..8f4b9faf 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -1,12 +1,13 @@ //! `api search`. use cli_engine::{ - CommandResult, CommandSpec, NextActionParam, PaginationConfig, RuntimeCommandSpec, Tier, + CommandResult, CommandSpec, CursorConfig, NextActionParam, RuntimeCommandSpec, Tier, }; use serde_json::{Value, json}; use crate::next_action::next_action; use crate::output_schema::output_schema; +use crate::pagination_token; use super::catalog::{catalog, graphql_sub_endpoint_rows, search_endpoints}; @@ -28,10 +29,15 @@ struct SearchArgs { /// Search term (matches path, operationId, summary, description). #[arg(value_name = "QUERY")] query: String, + + /// Jump directly to this offset instead of walking there one --continue + /// token at a time. + #[arg(long, value_name = "N", conflicts_with = "continue")] + offset: Option, } pub(super) fn command() -> RuntimeCommandSpec { - RuntimeCommandSpec::new_typed::( + RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("search", "Search API endpoints by keyword") .with_long( "Full-text searches across all API domains, matching against \ @@ -44,11 +50,16 @@ pub(super) fn command() -> RuntimeCommandSpec { .no_auth(true) .with_default_fields("operationId,domain,method,path,summary") .with_output_schema::() - .with_pagination(PaginationConfig::new(0, 100)), - |_cred, args: SearchArgs| async move { + .with_cursor(CursorConfig::new(50, 100)), + |ctx, args: SearchArgs| async move { let hits = search_endpoints(catalog(), &args.query); if hits.is_empty() { - return Ok(CommandResult::new(json!([]))); + let (page, continuation) = pagination_token::paginate_known_collection( + Vec::new(), + ctx.middleware.cursor_limit, + None, + ); + return Ok(CommandResult::new(json!(page)).with_cursor(continuation)); } let mut results: Vec = hits .iter() @@ -112,22 +123,88 @@ pub(super) fn command() -> RuntimeCommandSpec { ); } - Ok(CommandResult::new(json!(results)).with_next_actions(next_actions)) + let continue_token = pagination_token::resolve_continue_token( + args.offset, + ctx.middleware.continue_token.as_deref(), + ); + let (page, continuation) = pagination_token::paginate_known_collection( + results, + ctx.middleware.cursor_limit, + continue_token.as_deref(), + ); + Ok(CommandResult::new(json!(page)) + .with_cursor(continuation) + .with_next_actions(next_actions)) }, ) } #[cfg(test)] mod tests { - use cli_engine::PaginationConfig; + use cli_engine::{Cli, CliConfig, CursorConfig, Stage}; use super::command; #[test] - fn command_opts_into_pagination_with_no_default_and_a_max_limit() { - assert_eq!( - command().spec.pagination, - Some(PaginationConfig::new(0, 100)) - ); + fn command_opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(50, 100))); + } + + fn test_cli() -> Cli { + Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy") + .with_min_stage(Stage::Experimental) + .with_modules(crate::all_modules()), + ) + } + + #[tokio::test] + async fn limit_and_continue_page_through_search_results() { + let cli = test_cli(); + // "list" matches many operation IDs/paths/summaries across the + // embedded catalog — plenty of hits to page through. + let first = cli + .run([ + "gddy", "api", "search", "list", "--limit", "1", "--output", "json", + ]) + .await; + assert_eq!(first.exit_code, 0, "{}", first.rendered); + let first_json: serde_json::Value = + serde_json::from_str(&first.rendered).expect("stdout should contain json"); + assert_eq!(first_json["data"].as_array().expect("array").len(), 1); + assert_eq!(first_json["cursor"]["has_more"], serde_json::json!(true)); + let token = first_json["cursor"]["continue_from"] + .as_str() + .expect("has_more implies a token") + .to_owned(); + + let second = cli + .run([ + "gddy", + "api", + "search", + "list", + "--limit", + "1", + "--continue", + &token, + "--output", + "json", + ]) + .await; + assert_eq!(second.exit_code, 0, "{}", second.rendered); + let second_json: serde_json::Value = + serde_json::from_str(&second.rendered).expect("stdout should contain json"); + + let via_offset = cli + .run([ + "gddy", "api", "search", "list", "--limit", "1", "--offset", "1", "--output", + "json", + ]) + .await; + assert_eq!(via_offset.exit_code, 0, "{}", via_offset.rendered); + let via_offset_json: serde_json::Value = + serde_json::from_str(&via_offset.rendered).expect("stdout should contain json"); + assert_eq!(via_offset_json["data"], second_json["data"]); } } diff --git a/rust/src/api/summary.rs b/rust/src/api/summary.rs index 2fc1d2d0..4c04a4f9 100644 --- a/rust/src/api/summary.rs +++ b/rust/src/api/summary.rs @@ -22,10 +22,10 @@ use super::schema_tree::{SchemaLocation, compute_schema_id, describe_schema}; const SCHEMA_PROPERTY_PREVIEW_CAP: usize = 20; /// Default page size for `api parameter list`/`api response list` (via -/// `CommandSpec::with_pagination`) when neither `--limit` nor `--offset` is -/// passed, and for `api operation get`'s own embedded parameter/response -/// previews (which aren't paginated themselves — see `Summary::capped` -/// there — but use the same "how many rows to show inline" size). +/// `CommandSpec::with_cursor`) when `--limit` is omitted, and for `api +/// operation get`'s own embedded parameter/response previews (which aren't +/// paginated themselves — see `Summary::capped` there — but use the same +/// "how many rows to show inline" size). pub(super) const OPERATION_CHILD_LIST_DEFAULT_LIMIT: usize = 20; /// Upper bound a user can request with `--limit` on `api parameter list`/ diff --git a/rust/src/dns/list.rs b/rust/src/dns/list.rs index e8875be6..e1e4c3bf 100644 --- a/rust/src/dns/list.rs +++ b/rust/src/dns/list.rs @@ -1,17 +1,18 @@ //! `dns list` — list DNS records for a domain, with optional type/name filters. use cli_engine::{ - Alignment, CliCoreError, CommandResult, CommandSpec, PaginationConfig, RuntimeCommandSpec, - TableColumn, Tier, + Alignment, CliCoreError, CommandResult, CommandSpec, CursorConfig, CursorContinuation, + RuntimeCommandSpec, TableColumn, Tier, }; use serde_json::{Value, json}; use crate::domain::make_client; +use crate::pagination_token; use crate::scopes::DOMAINS_READ; use domains_client::types; -use super::records::{fetch_records, merged_tlsa_data, parse_list_type_arg}; +use super::records::{MAX_PAGE_SIZE, fetch_records_page, merged_tlsa_data, parse_list_type_arg}; #[derive(Debug, Clone, clap::Args)] struct ListArgs { @@ -27,6 +28,10 @@ struct ListArgs { /// Only records with this name (requires --type). #[arg(long, value_name = "NAME", requires = "record_type")] name: Option, + + /// Jump directly to a page of results (1-based). + #[arg(long, value_name = "N", conflicts_with = "continue")] + page: Option, } /// A record is unreadable without its type/name/data, so those three are @@ -68,7 +73,7 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_json_schema::() .with_view(view_columns()) .with_scopes(&[DOMAINS_READ]) - .with_pagination(PaginationConfig::new(0, 500)), + .with_cursor(CursorConfig::new(50, MAX_PAGE_SIZE.get() as i64)), |ctx, args: ListArgs| async move { let domain = args.domain; let type_opt = args.record_type; @@ -76,11 +81,19 @@ pub(super) fn command() -> RuntimeCommandSpec { let debug = !ctx.middleware.debug.is_empty(); let client = make_client(&ctx).await?; - let records = fetch_records( + let (page, page_size) = pagination_token::resolve_page_request( + args.page, + ctx.middleware.continue_token.as_deref(), + 1u64, + ctx.middleware.cursor_limit, + ); + let (records, has_more) = fetch_records_page( &client, domain.as_str(), type_opt.as_deref(), name_opt.as_deref(), + page, + page_size, debug, ) .await?; @@ -88,7 +101,16 @@ pub(super) fn command() -> RuntimeCommandSpec { let out = build_list_output(&records).map_err(|e| { CliCoreError::message(format!("failed to serialize DNS records: {e}")) })?; - Ok(CommandResult::new(json!(out))) + let continuation = if has_more { + CursorContinuation::more(pagination_token::build(&[ + ("limit", &page_size.to_string()), + ("page", &(page + 1).to_string()), + ])) + } else { + CursorContinuation::done() + }; + let continuation = continuation.with_limit(page_size); + Ok(CommandResult::new(json!(out)).with_cursor(continuation)) }, ) } @@ -117,13 +139,13 @@ fn build_list_output(records: &[types::DnsRecord]) -> Result, serde_j #[cfg(test)] mod tests { use super::*; - use cli_engine::PaginationConfig; + use cli_engine::CursorConfig; #[test] - fn opts_into_pagination_with_no_default_and_a_max_limit() { + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { assert_eq!( - command().spec.pagination, - Some(PaginationConfig::new(0, 500)) + command().spec.cursor, + Some(CursorConfig::new(50, MAX_PAGE_SIZE.get() as i64)) ); } diff --git a/rust/src/dns/records.rs b/rust/src/dns/records.rs index 03990700..aa349473 100644 --- a/rust/src/dns/records.rs +++ b/rust/src/dns/records.rs @@ -23,10 +23,14 @@ pub(super) const LISTABLE_TYPES: &[&str] = &[ ]; /// Default TTL (seconds) for `dns add`/`set` when `--ttl` is omitted (v3 requires a ttl). pub(super) const DEFAULT_TTL: i64 = 3600; -/// Page size for the paginated v3 list; the handler pages through until every -/// matching record is collected. -const LIST_PAGE_SIZE: std::num::NonZeroU64 = - std::num::NonZeroU64::new(100).expect("LIST_PAGE_SIZE is nonzero"); + +/// Upper bound on a single v3 DNS-records page request. [`fetch_records`] +/// always requests this many — the biggest page minimizes round trips when +/// walking the entire collection; [`fetch_records_page`] (`dns list`'s +/// CLI-facing cursor pagination) clamps the caller's requested page size to +/// at most this. Also `dns list`'s cursor `max_limit`. +pub(super) const MAX_PAGE_SIZE: std::num::NonZeroU64 = + std::num::NonZeroU64::new(100).expect("MAX_PAGE_SIZE is nonzero"); /// clap value-parser for a mutating `--type` (`add`/`set`/`delete`): validate /// against [`WRITABLE_TYPES`] and return the canonical upper-case wire string. @@ -386,12 +390,16 @@ pub(super) async fn fetch_records( let mut all = Vec::new(); let mut page: u64 = 1; loop { - let page_nz = std::num::NonZeroU64::new(page).expect("page starts at 1 and only grows"); + // `page` starts at 1 and only grows (below), so it's always nonzero + // in practice; `unwrap_or(MIN)` keeps that a total, panic-free + // conversion rather than an `.expect()` on an invariant the type + // system can't see. + let page_nz = std::num::NonZeroU64::new(page).unwrap_or(std::num::NonZeroU64::MIN); let mut req = client .list_dns_records() .zone(zone) .page(page_nz) - .page_size(LIST_PAGE_SIZE) + .page_size(MAX_PAGE_SIZE) .total_required(true); if let Some(t) = type_ { req = req.type_(types::DnsRecordType(t.to_owned())); @@ -411,7 +419,7 @@ pub(super) async fn fetch_records( let last = body .total_pages .map(|tp| page >= tp.get()) - .unwrap_or(got < LIST_PAGE_SIZE.get() as usize); + .unwrap_or(got < MAX_PAGE_SIZE.get() as usize); if last || got == 0 { break; } @@ -420,6 +428,46 @@ pub(super) async fn fetch_records( Ok(all) } +/// Fetch a page of matching DNS records. +pub(super) async fn fetch_records_page( + client: &domains_client::Client, + zone: &str, + type_: Option<&str>, + name: Option<&str>, + page: u64, + page_size: i64, + debug: bool, +) -> Result<(Vec, bool), CliCoreError> { + let page_size_nz = + std::num::NonZeroU64::new(page_size.clamp(1, MAX_PAGE_SIZE.get() as i64) as u64) + .unwrap_or(MAX_PAGE_SIZE); + let page_nz = std::num::NonZeroU64::new(page.max(1)).unwrap_or(std::num::NonZeroU64::MIN); + let mut req = client + .list_dns_records() + .zone(zone) + .page(page_nz) + .page_size(page_size_nz) + .total_required(true); + if let Some(t) = type_ { + req = req.type_(types::DnsRecordType(t.to_owned())); + } + if let Some(n) = name { + req = req.name(n); + } + let body = match req.send().await { + Ok(r) => r.into_inner(), + Err(e) => return Err(api_error("listing DNS records", debug, e).await), + }; + let items = body.items.unwrap_or_default(); + let got = items.len(); + let page_size_usize = usize::try_from(page_size_nz.get()).unwrap_or(usize::MAX); + let has_more = body + .total_pages + .map(|tp| page_nz.get() < tp.get()) + .unwrap_or(got == page_size_usize && got > 0); + Ok((items, has_more)) +} + /// Next action pointing back at `dns list` to verify a write, pre-filled with /// the domain/type/name the write just touched. pub(super) fn verify_with_list_action(domain: &str, record_type: &str, name: &str) -> NextAction { @@ -672,4 +720,114 @@ mod tests { let desired_caa = v3_record("@", "CAA", "letsencrypt.org", &caa_opts_wild); assert!(!same_content(&existing_caa, &desired_caa)); } + + #[tokio::test] + async fn fetch_records_page_reports_has_more_from_total_pages() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v3/domains/zones/example.com/dns-records") + .query_param("page", "1") + .query_param("pageSize", "2"); + then.status(200).json_body(serde_json::json!({ + "items": [{"type": "A", "name": "www", "data": "1.2.3.4", "ttl": 3600}], + "totalPages": 2, + })); + }) + .await; + let client = domains_client::client_with_builder( + reqwest::Client::builder(), + &server.base_url(), + "Bearer tok", + "req-1", + ) + .expect("build client"); + + let (items, has_more) = fetch_records_page(&client, "example.com", None, None, 1, 2, false) + .await + .expect("fetch succeeds"); + + assert_eq!(items.len(), 1); + assert!(has_more, "page 1 of 2 total pages has more"); + mock.assert_async().await; + } + + #[tokio::test] + async fn fetch_records_page_reports_no_more_on_the_last_page_without_total_pages() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v3/domains/zones/example.com/dns-records") + .query_param("page", "3") + .query_param("pageSize", "100"); + then.status(200).json_body(serde_json::json!({ + "items": [{"type": "A", "name": "www", "data": "1.2.3.4", "ttl": 3600}], + })); + }) + .await; + let client = domains_client::client_with_builder( + reqwest::Client::builder(), + &server.base_url(), + "Bearer tok", + "req-1", + ) + .expect("build client"); + + let (items, has_more) = fetch_records_page( + &client, + "example.com", + None, + None, + 3, + MAX_PAGE_SIZE.get() as i64, + false, + ) + .await + .expect("fetch succeeds"); + + assert_eq!(items.len(), 1); + assert!( + !has_more, + "a short page with no total_pages means exhausted" + ); + mock.assert_async().await; + } + + #[tokio::test] + async fn fetch_records_page_forwards_type_and_name_filters() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET) + .path("/v3/domains/zones/example.com/dns-records") + .query_param("type", "A") + .query_param("name", "www"); + then.status(200) + .json_body(serde_json::json!({ "items": [] })); + }) + .await; + let client = domains_client::client_with_builder( + reqwest::Client::builder(), + &server.base_url(), + "Bearer tok", + "req-1", + ) + .expect("build client"); + + fetch_records_page( + &client, + "example.com", + Some("A"), + Some("www"), + 1, + 100, + false, + ) + .await + .expect("fetch succeeds"); + + mock.assert_async().await; + } } diff --git a/rust/src/domain/list.rs b/rust/src/domain/list.rs index 0a7255bc..cf4f96d4 100644 --- a/rust/src/domain/list.rs +++ b/rust/src/domain/list.rs @@ -1,8 +1,8 @@ //! `gddy domain list` — list the domains in the account (v3). use cli_engine::{ - CliCoreError, CommandResult, CommandSpec, NextActionParam, PaginationConfig, Result, - RuntimeCommandSpec, Tier, + CliCoreError, CommandResult, CommandSpec, CursorConfig, CursorContinuation, NextActionParam, + Result, RuntimeCommandSpec, Tier, }; use serde_json::json; @@ -10,6 +10,7 @@ use domains_client::types; use super::common::{api_error, comma_joined, make_client}; use crate::next_action::next_action; +use crate::pagination_token; use crate::scopes::DOMAINS_READ; /// Lifecycle groups that make up GoDaddy's default "visible" domain view — @@ -18,18 +19,10 @@ use crate::scopes::DOMAINS_READ; /// (the spec declares it a bare string), so these are validated server-side. const DEFAULT_VISIBLE_GROUPS: [&str; 3] = ["PENDING", "REGISTERED", "PENDING_TERMINAL"]; -/// v3's max `pageSize` (the spec's `maximum: 200`). Requesting it on every -/// page minimizes round trips against an API observed to rate-limit as -/// tightly as 5 requests per period. +/// API's max page size const MAX_PAGE_SIZE: std::num::NonZeroU64 = std::num::NonZeroU64::new(200).expect("MAX_PAGE_SIZE is nonzero"); -/// Defensive cap on pages fetched for one invocation. No real account should -/// ever approach `MAX_PAGE_SIZE * MAX_PAGES` (10,000) domains; hitting this -/// is treated as a bug (a malformed or looping `next` link) rather than -/// silently returned as if it were the complete list. -const MAX_PAGES: usize = 50; - /// Known `DomainStatus` values, mirrored from the schema's `examples` (the /// spec deliberately dropped the closed `enum` it used to declare, so /// progenitor no longer generates one — GoDaddy Domains does this so adding @@ -152,85 +145,79 @@ fn next_page_token(links: &[types::LinkDescription]) -> NextPage { } } -/// Fetch every domain matching `statuses`/`visible_only`, following v3's -/// `links[rel=next]` cursor until the API reports no further page — or until -/// `stop_at` items have been accumulated, when an explicit `--limit`/ -/// `--offset` window needs no more than that many (cli-engine's own -/// pagination pipeline slices the exact window from whatever this returns; -/// fetching further would be wasted work against a tightly rate-limited -/// API). `stop_at: None` fetches everything, matching this command's -/// pre-v3 behavior of returning every domain when unflagged. -async fn fetch_domains( +/// Fetch a page of domains matching `statuses`/`visible_only`, resuming from +/// `resume` (a previously-returned `pageToken`+direction) when given. +async fn fetch_domain_page( client: &domains_client::Client, statuses: &[String], visible_only: bool, - stop_at: Option, + page_size: u64, + resume: Option<(String, Option)>, debug: bool, -) -> Result> { - let mut items = Vec::new(); - let mut page_token = None; - for _ in 0..MAX_PAGES { - let mut req = client.list_domains().page_size(MAX_PAGE_SIZE); - if !statuses.is_empty() { - // `statuses` is `style: form, explode: false` — one - // comma-joined value, not repeated `statuses=` pairs - // (progenitor always seq-serializes a `Vec` as repeated pairs - // regardless of the spec's `explode` setting; see - // `comma_joined`'s doc comment / DEVEX-882). - req = req.statuses(comma_joined(statuses.to_vec())); - } else if visible_only { - req = req.lifecycle_groups( - comma_joined( - DEFAULT_VISIBLE_GROUPS - .into_iter() - .map(str::to_string) - .collect(), - ) - .into_iter() - .map(types::DomainLifecycleGroup::from) - .collect::>(), - ); - } - if let Some((token, direction)) = page_token.take() { - req = req.page_token(token); - if let Some(direction) = direction { - req = req.page_token_direction(direction); - } - } - let collection = match req.send().await { - Ok(r) => r.into_inner(), - Err(e) => return Err(api_error("listing domains", debug, e).await), - }; - items.extend(collection.items.unwrap_or_default()); - if stop_at.is_some_and(|n| items.len() >= n) { - return Ok(items); +) -> Result<(Vec, NextPage)> { + let page_size = + std::num::NonZeroU64::new(page_size.clamp(1, MAX_PAGE_SIZE.get())).unwrap_or(MAX_PAGE_SIZE); + let mut req = client.list_domains().page_size(page_size); + if !statuses.is_empty() { + // `statuses` is `style: form, explode: false` — one comma-joined + // value, not repeated `statuses=` pairs (progenitor always + // seq-serializes a `Vec` as repeated pairs regardless of the spec's + // `explode` setting). + req = req.statuses(comma_joined(statuses.to_vec())); + } else if visible_only { + req = req.lifecycle_groups( + comma_joined( + DEFAULT_VISIBLE_GROUPS + .into_iter() + .map(str::to_string) + .collect(), + ) + .into_iter() + .map(types::DomainLifecycleGroup::from) + .collect::>(), + ); + } + if let Some((token, direction)) = resume { + req = req.page_token(token); + if let Some(direction) = direction { + req = req.page_token_direction(direction); } - let next = collection - .links - .map_or(NextPage::Done, |links| next_page_token(&links)); - page_token = match next { - NextPage::Done => return Ok(items), - NextPage::Token(token, direction) => Some((token, direction)), - NextPage::Unparseable => { - return Err(CliCoreError::message(format!( - "domain list: the API reported another page of results but this CLI \ - couldn't parse its pagination link ({} domains fetched before stopping); \ - this looks like an API or CLI bug, not a real account size", - items.len() - ))); - } - }; } - // Every earlier iteration returned as soon as a page had no `Token` - // (either genuinely `Done`, or an error on `Unparseable`), so reaching - // here means MAX_PAGES was exhausted with a next page still pending — - // a looping `next` link, not a real account size. Error instead of - // returning a partial list that would look complete to the caller. - Err(CliCoreError::message(format!( - "domain list: exceeded {MAX_PAGES} pages ({} domains fetched) without reaching the end \ - of the list; this looks like a pagination bug rather than a real account size", - items.len() - ))) + let collection = match req.send().await { + Ok(r) => r.into_inner(), + Err(e) => return Err(api_error("listing domains", debug, e).await), + }; + let items = collection.items.unwrap_or_default(); + let next = collection + .links + .map_or(NextPage::Done, |links| next_page_token(&links)); + Ok((items, next)) +} + +/// Encodes a [`NextPage::Token`]'s `(pageToken, direction)` pair into the +/// opaque string this command hands back via [`CursorContinuation::more`] +/// and reads back from `--continue` (see [`decode_resume`]). +fn encode_resume(token: &str, direction: Option) -> String { + match direction { + Some(direction) => format!("token:{token},direction:{direction}"), + None => format!("token:{token}"), + } +} + +/// Decodes a `--continue` value produced by [`encode_resume`]. A malformed +/// value (no `token` field, or a `direction` outside the API's two known +/// values) degrades to "start from the beginning" — the same convention +/// [`pagination_token`] uses for a hand-edited/truncated token, rather than +/// erroring the command. +fn decode_resume( + continue_token: Option<&str>, +) -> Option<(String, Option)> { + let fields = pagination_token::parse(continue_token); + let token = fields.get("token")?.clone(); + let direction = fields + .get("direction") + .and_then(|d| d.parse::().ok()); + Some((token, direction)) } pub(super) fn command() -> RuntimeCommandSpec { @@ -248,27 +235,19 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_default_fields("domain,status,expiresAt,autoRenew") .with_json_schema::() .with_scopes(&[DOMAINS_READ]) - .with_pagination(PaginationConfig::new(0, 500)), + .with_cursor(CursorConfig::new(100, MAX_PAGE_SIZE.get() as i64)), |ctx, args: ListArgs| async move { let debug = !ctx.middleware.debug.is_empty(); let statuses = parse_statuses(&args.status)?; let show_hidden = args.show_hidden; let visible_only = wants_visible_only(&statuses, show_hidden); let client = make_client(&ctx).await?; - // An explicit `--limit`/`--offset` window needs no more than - // `offset + limit` domains; cli-engine's pagination pipeline - // (`ctx.middleware.limit`/`.offset`, populated from those flags) - // slices the exact window from whatever this returns, so - // fetching further would be wasted requests against a tightly - // rate-limited API. Unflagged (`limit == 0`, the "unlimited" - // sentinel) fetches every domain, matching this command's - // pre-v3 behavior. - let limit = ctx.middleware.limit; - let stop_at = (limit > 0).then(|| { - usize::try_from(ctx.middleware.offset.max(0).saturating_add(limit)) - .unwrap_or(usize::MAX) - }); - let items = fetch_domains(&client, &statuses, visible_only, stop_at, debug).await?; + let page_size = + u64::try_from(ctx.middleware.cursor_limit).unwrap_or(MAX_PAGE_SIZE.get()); + let resume = decode_resume(ctx.middleware.continue_token.as_deref()); + let (items, next) = + fetch_domain_page(&client, &statuses, visible_only, page_size, resume, debug) + .await?; let domains: Vec = items .iter() .map(serde_json::to_value) @@ -276,10 +255,26 @@ pub(super) fn command() -> RuntimeCommandSpec { .map_err(|e| { CliCoreError::message(format!("failed to serialize domain list: {e}")) })?; - Ok(CommandResult::new(json!(domains)).with_next_actions(vec![ - next_action("dns list ", "View a domain's DNS records") - .with_param("domain", NextActionParam::required()), - ])) + let continuation = match next { + NextPage::Done => CursorContinuation::done(), + NextPage::Token(token, direction) => { + CursorContinuation::more(encode_resume(&token, direction)) + } + NextPage::Unparseable => { + return Err(CliCoreError::message(format!( + "domain list: the API reported another page of results but this CLI \ + couldn't parse its pagination link ({} domains fetched before \ + stopping); this looks like an API or CLI bug, not a real account size", + items.len() + ))); + } + }; + Ok(CommandResult::new(json!(domains)) + .with_cursor(continuation) + .with_next_actions(vec![ + next_action("dns list ", "View a domain's DNS records") + .with_param("domain", NextActionParam::required()), + ])) }, ) } @@ -287,23 +282,52 @@ pub(super) fn command() -> RuntimeCommandSpec { #[cfg(test)] mod tests { use super::{ - NextPage, command, fetch_domains, next_page_token, parse_statuses, wants_visible_only, + NextPage, command, decode_resume, encode_resume, fetch_domain_page, next_page_token, + parse_statuses, wants_visible_only, }; - use cli_engine::PaginationConfig; + use cli_engine::CursorConfig; use domains_client::types; - /// Regression pin: `domain list` opts into pagination with no forced - /// `default_limit` (an unflagged invocation must keep returning every - /// domain, exactly like before this feature existed) but a `max_limit` - /// so `--limit` can't be pointed at an absurd value. + /// Regression pin: `domain list` opts into cursor pagination with a + /// `default_limit` and a `max_limit` matching the API's real per-request + /// cap (`MAX_PAGE_SIZE`), so `--limit` maps straight onto `pageSize`. #[test] - fn opts_into_pagination_with_no_default_and_a_max_limit() { + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(100, 200))); + } + + #[test] + fn encode_and_decode_resume_round_trip_with_a_direction() { + let encoded = encode_resume( + "abc123", + Some(types::ListDomainsPageTokenDirection::Forward), + ); + assert_eq!(encoded, "token:abc123,direction:forward"); assert_eq!( - command().spec.pagination, - Some(PaginationConfig::new(0, 500)) + decode_resume(Some(&encoded)), + Some(( + "abc123".to_string(), + Some(types::ListDomainsPageTokenDirection::Forward) + )) ); } + #[test] + fn encode_and_decode_resume_round_trip_without_a_direction() { + let encoded = encode_resume("abc123", None); + assert_eq!(encoded, "token:abc123"); + assert_eq!( + decode_resume(Some(&encoded)), + Some(("abc123".to_string(), None)) + ); + } + + #[test] + fn decode_resume_of_none_or_malformed_starts_from_the_beginning() { + assert_eq!(decode_resume(None), None); + assert_eq!(decode_resume(Some("garbage")), None); + } + #[test] fn parse_statuses_is_case_insensitive_and_validates() { let parsed = parse_statuses(&["active".to_string(), "CANCELLED".to_string()]) @@ -467,12 +491,10 @@ mod tests { } #[tokio::test] - async fn fetch_domains_follows_the_next_link_across_pages() { - // Regression: `listDomains` is cursor-paginated (`DomainCollection` - // with `links[rel=next]`), but a single `send()` only returns the - // first page — an account with more domains than one page would get - // silently truncated results. This proves `fetch_domains` follows - // `next` until it's exhausted rather than stopping after page one. + async fn fetch_domain_page_returns_a_token_when_a_next_link_is_present() { + // One cursor step is exactly one v3 request now — this proves the + // returned `NextPage` carries the `next` link's token forward + // instead of the old code following it automatically. let server = httpmock::MockServer::start_async().await; let page1 = server .mock_async(|when, then| { @@ -488,16 +510,6 @@ mod tests { })); }) .await; - let page2 = server - .mock_async(|when, then| { - when.method(httpmock::Method::GET) - .path("/v3/domains/domain-names") - .query_param("pageToken", "page-2"); - then.status(200).json_body(serde_json::json!({ - "items": [{"domain": "b.com"}], - })); - }) - .await; let client = domains_client::client_with_builder( reqwest::Client::builder(), &server.base_url(), @@ -506,7 +518,7 @@ mod tests { ) .expect("build client"); - let items = fetch_domains(&client, &[], false, None, false) + let (items, next) = fetch_domain_page(&client, &[], false, 100, None, false) .await .expect("fetch succeeds"); @@ -515,28 +527,22 @@ mod tests { .iter() .filter_map(|d| d.domain.clone()) .collect::>(), - vec!["a.com".to_string(), "b.com".to_string()] + vec!["a.com".to_string()] ); + assert_eq!(next, NextPage::Token("page-2".to_string(), None)); page1.assert_async().await; - page2.assert_async().await; } #[tokio::test] - async fn fetch_domains_stops_once_stop_at_is_satisfied_without_fetching_the_next_page() { - // The `--limit`/`--offset` window is satisfied by page one alone, so - // `fetch_domains` must not spend a second request (and a second hit - // against this API's tight rate limit) fetching page two. + async fn fetch_domain_page_resumes_from_a_provided_token() { let server = httpmock::MockServer::start_async().await; - let page1 = server + let page2 = server .mock_async(|when, then| { when.method(httpmock::Method::GET) - .path("/v3/domains/domain-names"); + .path("/v3/domains/domain-names") + .query_param("pageToken", "page-2"); then.status(200).json_body(serde_json::json!({ - "items": [{"domain": "a.com"}, {"domain": "b.com"}], - "links": [{ - "rel": "next", - "href": "/v3/domains/domain-names?pageToken=page-2", - }], + "items": [{"domain": "b.com"}], })); }) .await; @@ -548,58 +554,34 @@ mod tests { ) .expect("build client"); - let items = fetch_domains(&client, &[], false, Some(2), false) - .await - .expect("fetch succeeds"); + let (items, next) = fetch_domain_page( + &client, + &[], + false, + 100, + Some(("page-2".to_string(), None)), + false, + ) + .await + .expect("fetch succeeds"); - assert_eq!(items.len(), 2); assert_eq!( - page1.calls_async().await, - 1, - "must not fetch beyond the satisfied stop_at window" + items + .iter() + .filter_map(|d| d.domain.clone()) + .collect::>(), + vec!["b.com".to_string()] ); + assert_eq!(next, NextPage::Done); + page2.assert_async().await; } #[tokio::test] - async fn fetch_domains_errors_instead_of_silently_truncating_a_looping_next_link() { - // A malformed or looping `next` link must fail loudly rather than - // return a partial list that looks complete to the caller. - let server = httpmock::MockServer::start_async().await; - let mock = server - .mock_async(|when, then| { - when.method(httpmock::Method::GET) - .path("/v3/domains/domain-names"); - then.status(200).json_body(serde_json::json!({ - "items": [{"domain": "a.com"}], - "links": [{ - "rel": "next", - "href": "/v3/domains/domain-names?pageToken=always-more", - }], - })); - }) - .await; - let client = domains_client::client_with_builder( - reqwest::Client::builder(), - &server.base_url(), - "Bearer tok", - "req-1", - ) - .expect("build client"); - - let err = fetch_domains(&client, &[], false, None, false) - .await - .expect_err("must not silently return a partial list"); - assert!(err.to_string().contains("pages"), "{err}"); - assert_eq!(mock.calls_async().await, super::MAX_PAGES); - } - - #[tokio::test] - async fn fetch_domains_errors_immediately_on_an_unparseable_next_link() { + async fn fetch_domain_page_reports_unparseable_for_the_caller_to_error_on() { // Per the spec, `rel=next` only ever appears when more data exists, - // so a `next` link present but missing a `pageToken` must error on - // the spot — not be treated as "done" and silently return a partial - // list that looks complete. This must fire on page one, well before - // any MAX_PAGES cap. + // so a `next` link present but missing a `pageToken` must surface as + // `Unparseable` — not silently treated as `Done`, which would make a + // partial list look complete. let server = httpmock::MockServer::start_async().await; let mock = server .mock_async(|when, then| { @@ -619,14 +601,11 @@ mod tests { ) .expect("build client"); - let err = fetch_domains(&client, &[], false, None, false) + let (_, next) = fetch_domain_page(&client, &[], false, 100, None, false) .await - .expect_err("must not silently return a partial list"); - assert!(err.to_string().contains("couldn't parse"), "{err}"); - assert_eq!( - mock.calls_async().await, - 1, - "must error on page one, not loop to MAX_PAGES" - ); + .expect("fetch succeeds"); + + assert_eq!(next, NextPage::Unparseable); + mock.assert_async().await; } } diff --git a/rust/src/email/list.rs b/rust/src/email/list.rs index df695a13..a0ee2889 100644 --- a/rust/src/email/list.rs +++ b/rust/src/email/list.rs @@ -1,5 +1,5 @@ use cli_engine::{ - CliCoreError, CommandResult, CommandSpec, NextActionParam, PaginationConfig, + CliCoreError, CommandResult, CommandSpec, CursorConfig, CursorContinuation, NextActionParam, RuntimeCommandSpec, Tier, }; use email_client::types; @@ -7,10 +7,10 @@ use email_client::types; use crate::email::client::{ClientError, EmailClient}; use crate::email::{client_err, make_client}; use crate::next_action::next_action; +use crate::pagination_token; use crate::scopes::EMAIL_READ; -/// panel-v3-api's own page-size cap for `GET /v1/email/mailboxes`. See the -/// design doc's pagination decision. +/// panel-v3-api's page-size cap for `GET /v1/email/mailboxes` const SERVER_PAGE_SIZE_CAP: usize = 100; #[derive(Debug, Clone, clap::Args)] @@ -21,86 +21,82 @@ struct ListArgs { /// Comma-separated list of fields to include in the response. #[arg(long, value_name = "FIELDS")] fields: Option, + /// Jump directly to a page of results (1-based). + #[arg(long, value_name = "N", conflicts_with = "continue")] + page: Option, } -/// Translates `--limit`/`--offset` into the panel API's native `page`/`pageSize` -/// query params via [`fetch_mailboxes`], fetching only as many leading pages as -/// needed to cover the requested window (see -/// `docs/proposals/email-management-cli.md`'s pagination decision) instead of the -/// full collection. Returns a bare JSON array so the engine's `--limit`/`--offset` -/// pipeline (`PaginationConfig`) can slice it to the exact window. +/// Translates `--limit`/`--continue` into the panel API's native +/// `page`/`pageSize` query params via [`fetch_mailbox_page`] pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List your Email mailboxes") .with_system("email") .with_tier(Tier::Read) .with_scopes(&[EMAIL_READ]) - .with_pagination(PaginationConfig::new(25, 500)), + .with_cursor(CursorConfig::new(25, SERVER_PAGE_SIZE_CAP as i64)), |ctx, args: ListArgs| async move { let client = make_client(&ctx, &[EMAIL_READ]).await?; - let mailboxes = fetch_mailboxes( + let (page, page_size) = pagination_token::resolve_page_request( + args.page, + ctx.middleware.continue_token.as_deref(), + 1u32, + ctx.middleware.cursor_limit, + ); + let page_size = usize::try_from(page_size).unwrap_or(SERVER_PAGE_SIZE_CAP); + let (mailboxes, has_more) = fetch_mailbox_page( &client, args.status.as_deref(), args.fields.as_deref(), - ctx.middleware.limit, - ctx.middleware.offset, + page, + page_size, ) .await .map_err(client_err)?; + let continuation = if has_more { + CursorContinuation::more(pagination_token::build(&[ + ("limit", &page_size.to_string()), + ("page", &(page + 1).to_string()), + ])) + } else { + CursorContinuation::done() + }; + let continuation = + continuation.with_limit(i64::try_from(page_size).unwrap_or(i64::MAX)); let value = serde_json::to_value(&mailboxes).map_err(|e| { CliCoreError::message(format!("failed to serialize mailboxes: {e}")) })?; - Ok(CommandResult::new(value).with_next_actions(vec![ - next_action("email get ", "Get a mailbox by ID") - .with_param("mailbox-id", NextActionParam::required()), - ])) + Ok(CommandResult::new(value) + .with_cursor(continuation) + .with_next_actions(vec![ + next_action("email get ", "Get a mailbox by ID") + .with_param("mailbox-id", NextActionParam::required()), + ])) }, ) } -/// Fetches only as many leading pages as needed to cover `[offset, offset + -/// limit)`, plus one extra item beyond that window (when the server isn't -/// already exhausted) so the engine's pagination pipeline reports an accurate -/// `has_more`. `limit <= 0` means unlimited (`PaginationConfig` convention) and -/// fetches every page. Trade-off: the returned `total` reflects only what was -/// fetched, not the panel API's true mailbox count, whenever more exist beyond -/// the requested window — computing an exact `total` would require fetching -/// everything, which this early-stop is meant to avoid. -async fn fetch_mailboxes( +/// Fetches a page from `GET /v1/email/mailboxes`. `has_more` is a +/// short-page heuristic (a full page might still be the exact last one) — +/// this API never reports a true total, so "short page = done" is the best +/// signal available. +async fn fetch_mailbox_page( client: &EmailClient, status: Option<&str>, fields: Option<&str>, - limit: i64, - offset: i64, -) -> Result, ClientError> { - let target = if limit > 0 { - let extra = offset.saturating_add(limit).saturating_add(1); - Some(usize::try_from(extra).unwrap_or(usize::MAX)) - } else { - None - }; - - let mut mailboxes = Vec::new(); - let mut page: u32 = 1; - loop { - let list = client - .list_mailboxes(status, page, SERVER_PAGE_SIZE_CAP as u32, fields) - .await?; - let got = list.items.len(); - mailboxes.extend(list.items); - - let exhausted = got < SERVER_PAGE_SIZE_CAP; - let covered = match target { - Some(t) => mailboxes.len() >= t, - None => false, - }; - if exhausted || covered { - break; - } - page += 1; - } - - Ok(mailboxes) + page: u32, + page_size: usize, +) -> Result<(Vec, bool), ClientError> { + let list = client + .list_mailboxes( + status, + page, + u32::try_from(page_size).unwrap_or(u32::MAX), + fields, + ) + .await?; + let has_more = list.items.len() >= page_size; + Ok((list.items, has_more)) } #[cfg(test)] @@ -118,11 +114,8 @@ mod tests { } #[test] - fn opts_into_pagination_with_a_default_and_a_max_limit() { - assert_eq!( - command().spec.pagination, - Some(PaginationConfig::new(25, 500)) - ); + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(25, 100))); } fn page_of(n: usize, start: usize) -> Vec { @@ -136,89 +129,52 @@ mod tests { } #[tokio::test] - async fn stops_after_one_short_page_even_when_target_is_not_yet_covered() { + async fn fetch_mailbox_page_reports_has_more_when_the_page_is_full() { let server = MockServer::start_async().await; let mock = server .mock_async(|when, then| { when.method(GET) .path("/v1/email/mailboxes") - .query_param("page", "1"); + .query_param("page", "1") + .query_param("pageSize", "3"); then.status(200) .json_body(json!({ "items": page_of(3, 0) })); }) .await; let client = client_for(&server); - let mailboxes = fetch_mailboxes(&client, None, None, 2, 0) + let (mailboxes, has_more) = fetch_mailbox_page(&client, None, None, 1, 3) .await .expect("fetch mailboxes"); mock.assert_calls_async(1).await; assert_eq!(mailboxes.len(), 3); + assert!(has_more, "a full page is treated as possibly having more"); } #[tokio::test] - async fn fetches_a_second_page_when_offset_and_limit_cross_a_page_boundary() { + async fn fetch_mailbox_page_reports_no_more_on_a_short_page() { let server = MockServer::start_async().await; - let page1 = server - .mock_async(|when, then| { - when.method(GET) - .path("/v1/email/mailboxes") - .query_param("page", "1"); - then.status(200) - .json_body(json!({ "items": page_of(SERVER_PAGE_SIZE_CAP, 0) })); - }) - .await; - let page2 = server + let mock = server .mock_async(|when, then| { when.method(GET) .path("/v1/email/mailboxes") - .query_param("page", "2"); + .query_param("page", "2") + .query_param("pageSize", "100"); then.status(200) .json_body(json!({ "items": page_of(10, SERVER_PAGE_SIZE_CAP) })); }) .await; let client = client_for(&server); - let mailboxes = fetch_mailboxes(&client, None, None, 5, 99) - .await - .expect("fetch mailboxes"); - - page1.assert_calls_async(1).await; - page2.assert_calls_async(1).await; - assert_eq!(mailboxes.len(), SERVER_PAGE_SIZE_CAP + 10); - } + let (mailboxes, has_more) = + fetch_mailbox_page(&client, None, None, 2, SERVER_PAGE_SIZE_CAP) + .await + .expect("fetch mailboxes"); - #[tokio::test] - async fn unlimited_limit_fetches_until_a_short_page_is_seen() { - let server = MockServer::start_async().await; - let page1 = server - .mock_async(|when, then| { - when.method(GET) - .path("/v1/email/mailboxes") - .query_param("page", "1"); - then.status(200) - .json_body(json!({ "items": page_of(SERVER_PAGE_SIZE_CAP, 0) })); - }) - .await; - let page2 = server - .mock_async(|when, then| { - when.method(GET) - .path("/v1/email/mailboxes") - .query_param("page", "2"); - then.status(200) - .json_body(json!({ "items": page_of(5, SERVER_PAGE_SIZE_CAP) })); - }) - .await; - - let client = client_for(&server); - let mailboxes = fetch_mailboxes(&client, None, None, 0, 0) - .await - .expect("fetch mailboxes"); - - page1.assert_calls_async(1).await; - page2.assert_calls_async(1).await; - assert_eq!(mailboxes.len(), SERVER_PAGE_SIZE_CAP + 5); + mock.assert_calls_async(1).await; + assert_eq!(mailboxes.len(), 10); + assert!(!has_more, "a short page means the list is exhausted"); } #[tokio::test] @@ -236,7 +192,7 @@ mod tests { .await; let client = client_for(&server); - fetch_mailboxes(&client, Some("ACTIVE"), Some("mailboxId,status"), 10, 0) + fetch_mailbox_page(&client, Some("ACTIVE"), Some("mailboxId,status"), 1, 100) .await .expect("fetch mailboxes"); diff --git a/rust/src/hosting/app/list.rs b/rust/src/hosting/app/list.rs index 1fae8c6b..2c2ab612 100644 --- a/rust/src/hosting/app/list.rs +++ b/rust/src/hosting/app/list.rs @@ -1,9 +1,9 @@ -use cli_engine::{CommandResult, NextActionParam, RuntimeCommandSpec, Tier}; -use serde_json::{Value, json}; +use cli_engine::{CommandResult, CursorConfig, NextActionParam, RuntimeCommandSpec, Tier}; +use serde_json::json; use crate::hosting::common::{ - HostingAppSummary, HostingAppType, app_type_command, client_err, make_client, next_page_token, - supported_app_types, + HostingAppSummary, HostingAppType, app_type_command, client_err, make_client, + page_continuation, page_items, page_limit, supported_app_types, }; use crate::next_action::next_action; use crate::scopes::HOSTING_APPLICATION_READ as APP_READ; @@ -13,18 +13,15 @@ struct AppListArgs { /// Hosting product. #[arg(long = "app-type", value_name = "TYPE", ignore_case = true)] app_type: HostingAppType, - - /// Maximum number of applications to return. Omit to return all. - #[arg(long, value_name = "N", value_parser = clap::value_parser!(u32).range(1..))] - limit: Option, } pub(super) fn command(mhwp: bool) -> RuntimeCommandSpec { RuntimeCommandSpec::new_typed_with_context::( app_type_command::("list", "List hosting applications", mhwp) .with_long(format!( - "List all hosting applications of a given type. Results are autopaginated — \ - all pages are fetched and combined. Use --limit to cap the total returned.\n\ + "List hosting applications of a given type, one page at a time. Use --limit to \ + set the page size and --continue with the token from a page's cursor to \ + fetch the next.\n\ \n\ --app-type is required. Currently supported: {}.", supported_app_types(mhwp) @@ -33,50 +30,42 @@ pub(super) fn command(mhwp: bool) -> RuntimeCommandSpec { .with_tier(Tier::Read) .with_scopes(&[APP_READ]) .with_default_fields("id,name,status") - .with_output_schema::(), + .with_output_schema::() + .with_cursor(CursorConfig::new(50, 100)), |ctx, args: AppListArgs| async move { let app_type = args.app_type; - let limit = args.limit; let client = make_client(&ctx, &[APP_READ]).await?; - let mut all_items: Vec = Vec::new(); - let mut page_token: Option = None; - - loop { - let page_limit = limit.map(|cap| { - let remaining = cap.saturating_sub(all_items.len() as u32); - remaining.min(100) - }); - - let response = client - .list_apps(app_type.as_str(), page_token.as_deref(), page_limit) - .await - .map_err(client_err)?; + let response = client + .list_apps( + app_type.as_str(), + ctx.middleware.continue_token.as_deref(), + page_limit(ctx.middleware.cursor_limit), + ) + .await + .map_err(client_err)?; - if let Some(items) = response.get("items").and_then(|v| v.as_array()) { - all_items.extend(items.iter().cloned()); - } + Ok(CommandResult::new(json!(page_items(&response))) + .with_cursor(page_continuation(&response)) + .with_next_actions(vec![ + next_action( + "hosting app get --app-id ", + "Get details for an application", + ) + .with_param("app-id", NextActionParam::required()), + ])) + }, + ) +} - if limit.is_some_and(|cap| all_items.len() >= cap as usize) { - if let Some(cap) = limit { - all_items.truncate(cap as usize); - } - break; - } +#[cfg(test)] +mod tests { + use cli_engine::CursorConfig; - match next_page_token(&response) { - Some(token) => page_token = Some(token), - None => break, - } - } + use super::command; - Ok(CommandResult::new(json!(all_items)).with_next_actions(vec![ - next_action( - "hosting app get --app-id ", - "Get details for an application", - ) - .with_param("app-id", NextActionParam::required()), - ])) - }, - ) + #[test] + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command(false).spec.cursor, Some(CursorConfig::new(50, 100))); + } } diff --git a/rust/src/hosting/client.rs b/rust/src/hosting/client.rs index 667cf94a..b157c1e7 100644 --- a/rust/src/hosting/client.rs +++ b/rust/src/hosting/client.rs @@ -549,16 +549,14 @@ impl HostingClient { response(client.get_runtime().app_id(app_id).send().await).await } - pub async fn list_domains( - &self, - app_id: &str, - page_token: Option<&str>, - limit: Option, - ) -> Result { + /// One request, no paging parameters: the spec types this response as + /// paged (`items`/`links`) but declares no `pageToken`/`pageSize`, and the + /// service rejects them on the sibling `GET /subscriptions` with a 400 + /// ("Unrecognized key"). Callers use [`super::common::more_pages_warning`] + /// to tell the user when the service reports more than this page. + pub async fn list_domains(&self, app_id: &str) -> Result { let client = self.api()?; - let request = client.list_domains().app_id(app_id); - let _ = (page_token, limit); - response(request.send().await).await + response(client.list_domains().app_id(app_id).send().await).await } pub async fn get_domain(&self, app_id: &str, domain_id: &str) -> Result { @@ -603,17 +601,12 @@ impl HostingClient { .await } - pub async fn list_subscriptions( - &self, - page_token: Option<&str>, - limit: Option, - hosting_product: &str, - ) -> Result { + /// One request, no paging parameters — see [`Self::list_domains`]. + pub async fn list_subscriptions(&self, hosting_product: &str) -> Result { let client = self.api()?; let request = client .list_subscriptions() .hosting_product(HostingProduct::from(hosting_product.to_owned())); - let _ = (page_token, limit); response(request.send().await).await } diff --git a/rust/src/hosting/client_tests.rs b/rust/src/hosting/client_tests.rs index 15b7956c..8fe7ce75 100644 --- a/rust/src/hosting/client_tests.rs +++ b/rust/src/hosting/client_tests.rs @@ -468,7 +468,7 @@ async fn list_domains_hits_correct_path() { .await; client(&server.base_url()) - .list_domains("app-1", None, None) + .list_domains("app-1") .await .expect("list domains"); @@ -529,7 +529,7 @@ async fn list_subscriptions_hits_correct_path() { .await; client(&server.base_url()) - .list_subscriptions(None, None, "WEB_HOSTING") + .list_subscriptions("WEB_HOSTING") .await .expect("list subscriptions"); diff --git a/rust/src/hosting/common.rs b/rust/src/hosting/common.rs index 56bd1e84..ee0ebcb3 100644 --- a/rust/src/hosting/common.rs +++ b/rust/src/hosting/common.rs @@ -1,5 +1,7 @@ use clap::builder::{PossibleValuesParser, TypedValueParser}; -use cli_engine::{CliCoreError, CommandContext, CommandSpec, FlagPolicy, Stage}; +use cli_engine::{ + CliCoreError, CommandContext, CommandSpec, CursorContinuation, FlagPolicy, Stage, +}; use serde::Deserialize; use serde_json::Value; @@ -269,6 +271,69 @@ pub fn next_page_token(response: &Value) -> Option { None } +/// The `href` of a paged response's `links[rel=next]`, if it has one. +pub fn next_link_href(response: &Value) -> Option { + response + .get("links")? + .as_array()? + .iter() + .find(|link| link.get("rel").and_then(Value::as_str) == Some("next"))? + .get("href")? + .as_str() + .map(str::to_owned) +} + +/// The warning to show when a listing that this CLI can only read one page of +/// reports another page via `links[rel=next]` — for the endpoints whose spec +/// types the response as paged but declares no paging request parameters +/// (`hosting domain list`, `hosting subscription list`). `None` when the +/// service reports nothing further. +pub fn more_pages_warning(response: &Value, what: &str) -> Option { + next_link_href(response).map(|_| { + format!( + "The API reports more {what} than can be displayed." + ) + }) +} + +/// Writes `message` as a one-line warning to `out`. +fn write_warning(out: &mut impl std::io::Write, message: &str) { + let _ = writeln!(out, "warning: {message}"); +} + +/// Shows a non-fatal warning to the user on stderr, keeping stdout's JSON/TOON +/// payload clean. +/// +/// Interim: the engine has no result-level warning channel yet. Replace calls +/// with `CommandResult::with_warning` once cli-engine#131 is released and +/// adopted. +pub fn warn_user(message: &str) { + write_warning(&mut std::io::stderr(), message); +} + +/// The `items` array of a paged hosting response (empty when absent). +pub fn page_items(response: &Value) -> Vec { + response + .get("items") + .and_then(Value::as_array) + .cloned() + .unwrap_or_default() +} + +/// Maps a paged hosting response's `links[rel=next]` onto the engine's cursor +/// continuation, using the server's own `pageToken` as the `--continue` token. +pub fn page_continuation(response: &Value) -> CursorContinuation { + match next_page_token(response) { + Some(token) => CursorContinuation::more(token), + None => CursorContinuation::done(), + } +} + +/// The engine's resolved `--limit` as the `pageSize` the hosting client takes. +pub fn page_limit(cursor_limit: i64) -> Option { + u32::try_from(cursor_limit).ok().filter(|n| *n > 0) +} + fn extract_query_param(url: &str, param: &str) -> Option { let query = url.split_once('?')?.1; for pair in query.split('&') { @@ -377,6 +442,82 @@ mod tests { assert!(next_page_token(&response).is_none()); } + #[test] + fn more_pages_warning_appears_only_when_the_api_reports_a_next_page() { + let more = json!({ + "items": [], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/a/domains?pageToken=t2" }] + }); + let warning = more_pages_warning(&more, "domains").expect("next link warns"); + assert!(warning.contains("more domains"), "{warning}"); + assert!(warning.contains("incomplete"), "{warning}"); + + let complete = json!({ + "items": [], + "links": [{ "rel": "self", "href": "/v1/hosting/apps/a/domains" }] + }); + assert_eq!(more_pages_warning(&complete, "domains"), None); + assert_eq!(more_pages_warning(&json!({ "items": [] }), "domains"), None); + } + + #[test] + fn write_warning_emits_one_prefixed_line() { + let mut out = Vec::new(); + write_warning(&mut out, "list may be incomplete"); + assert_eq!( + String::from_utf8(out).expect("utf8"), + "warning: list may be incomplete\n" + ); + } + + #[test] + fn page_items_returns_the_items_array_or_empty() { + assert_eq!( + page_items(&json!({ "items": [{ "id": "a" }, { "id": "b" }] })).len(), + 2 + ); + assert!(page_items(&json!({})).is_empty()); + } + + #[test] + fn page_continuation_carries_the_servers_page_token_and_is_done_without_one() { + let more = page_continuation(&json!({ + "links": [{ "rel": "next", "href": "https://api.godaddy.com/v1/hosting/apps?pageToken=tok-2" }] + })); + assert_eq!(more.continue_from.as_deref(), Some("tok-2")); + assert_eq!( + page_continuation(&json!({ "items": [] })).continue_from, + None + ); + } + + #[test] + fn next_link_href_picks_the_next_relation_only() { + let response = json!({ + "links": [ + { "rel": "self", "href": "/v1/hosting/apps/a/domains" }, + { "rel": "next", "href": "/v1/hosting/apps/a/domains?pageToken=tok-2" } + ] + }); + assert_eq!( + next_link_href(&response).as_deref(), + Some("/v1/hosting/apps/a/domains?pageToken=tok-2") + ); + assert_eq!( + next_link_href(&json!({ "links": [{ "rel": "self", "href": "/x" }] })), + None + ); + assert_eq!(next_link_href(&json!({ "items": [] })), None); + } + + #[test] + fn page_limit_rejects_non_positive_and_oversized_values() { + assert_eq!(page_limit(50), Some(50)); + assert_eq!(page_limit(0), None); + assert_eq!(page_limit(-1), None); + assert_eq!(page_limit(i64::MAX), None); + } + #[test] fn format_api_error_body_prefers_issue_over_repeated_description() { let body = r#"{ diff --git a/rust/src/hosting/deployment/list.rs b/rust/src/hosting/deployment/list.rs index e4f9b181..1a2db847 100644 --- a/rust/src/hosting/deployment/list.rs +++ b/rust/src/hosting/deployment/list.rs @@ -1,7 +1,11 @@ -use cli_engine::{CommandResult, CommandSpec, NextActionParam, RuntimeCommandSpec, Tier}; -use serde_json::{Value, json}; +use cli_engine::{ + CommandResult, CommandSpec, CursorConfig, NextActionParam, RuntimeCommandSpec, Tier, +}; +use serde_json::json; -use crate::hosting::common::{HostingDeploymentSummary, client_err, make_client, next_page_token}; +use crate::hosting::common::{ + HostingDeploymentSummary, client_err, make_client, page_continuation, page_items, page_limit, +}; use crate::next_action::next_action; use crate::scopes::HOSTING_APPLICATION_READ as APP_READ; @@ -10,64 +14,57 @@ struct DeploymentListArgs { /// Application ID. #[arg(long = "app-id", value_name = "APP_ID")] app_id: String, - - /// Maximum number of deployments to return. Omit to return all. - #[arg(long, value_name = "N", value_parser = clap::value_parser!(u32).range(1..))] - limit: Option, } pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List deployments for an application") .with_long( - "List all deployments for a hosting application, newest first. \ - Results are autopaginated. Use --limit to cap the total returned.", + "List deployments for a hosting application, newest first, one page at a time. \ + Use --limit to set the page size and --continue with the token from a \ + page's cursor to fetch the next.", ) .with_system("hosting") .with_tier(Tier::Read) .with_scopes(&[APP_READ]) .with_default_fields("deploymentId,status,createdAt") - .with_output_schema::(), + .with_output_schema::() + .with_cursor(CursorConfig::new(50, 100)), |ctx, args: DeploymentListArgs| async move { let app_id = args.app_id; - let limit = args.limit; let client = make_client(&ctx, &[APP_READ]).await?; - let mut all_items: Vec = Vec::new(); - let mut page_token: Option = None; - - loop { - let page_limit = - limit.map(|cap| cap.saturating_sub(all_items.len() as u32).min(100)); - - let response = client - .list_deployments(&app_id, page_token.as_deref(), page_limit) - .await - .map_err(client_err)?; + let response = client + .list_deployments( + &app_id, + ctx.middleware.continue_token.as_deref(), + page_limit(ctx.middleware.cursor_limit), + ) + .await + .map_err(client_err)?; - if let Some(items) = response.get("items").and_then(|v| v.as_array()) { - all_items.extend(items.iter().cloned()); - } + Ok(CommandResult::new(json!(page_items(&response))) + .with_cursor(page_continuation(&response)) + .with_next_actions(vec![ + next_action( + "hosting deployment get --app-id --deployment-id ", + "Get full details of a deployment", + ) + .with_param("app-id", NextActionParam::required()) + .with_param("deployment-id", NextActionParam::required()), + ])) + }, + ) +} - if limit.is_some_and(|cap| all_items.len() >= cap as usize) { - all_items.truncate(limit.expect("checked") as usize); - break; - } +#[cfg(test)] +mod tests { + use cli_engine::CursorConfig; - match next_page_token(&response) { - Some(token) => page_token = Some(token), - None => break, - } - } + use super::command; - Ok(CommandResult::new(json!(all_items)).with_next_actions(vec![ - next_action( - "hosting deployment get --app-id --deployment-id ", - "Get full details of a deployment", - ) - .with_param("app-id", NextActionParam::required()) - .with_param("deployment-id", NextActionParam::required()), - ])) - }, - ) + #[test] + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(50, 100))); + } } diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 80124f6e..794023aa 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -1,8 +1,13 @@ -use cli_engine::{CommandResult, CommandSpec, NextActionParam, RuntimeCommandSpec, Tier}; -use serde_json::{Value, json}; +use cli_engine::{ + CommandResult, CommandSpec, CursorConfig, NextActionParam, RuntimeCommandSpec, Tier, +}; +use serde_json::json; -use crate::hosting::common::{HostingDomainSummary, client_err, make_client, next_page_token}; +use crate::hosting::common::{ + HostingDomainSummary, client_err, make_client, more_pages_warning, page_items, warn_user, +}; use crate::next_action::next_action; +use crate::pagination_token; use crate::scopes::HOSTING_DOMAIN_READ as DOMAIN_READ; #[derive(Debug, Clone, clap::Args)] @@ -11,62 +16,66 @@ struct DomainListArgs { #[arg(long = "app-id", value_name = "APP_ID")] app_id: String, - /// Maximum number of domains to return. Omit to return all. - #[arg(long, value_name = "N", value_parser = clap::value_parser!(u32).range(1..))] - limit: Option, + /// Jump directly to an offset in the results. + #[arg(long, value_name = "N", conflicts_with = "continue")] + offset: Option, } +/// The API accepts no paging parameters (see `HostingClient::list_domains`), +/// so the one response is paged client-side by `--limit` / `--continue`. pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List domains attached to an application") .with_long( - "List all domains attached to a hosting application. \ - Results are autopaginated.", + "List domains attached to a hosting application. Shows up to 50 by \ + default; use --limit/--continue to page through the rest.", ) .with_system("hosting") .with_tier(Tier::Read) .with_scopes(&[DOMAIN_READ]) .with_default_fields("id,hostname,verificationStatus") - .with_output_schema::(), + .with_output_schema::() + .with_cursor(CursorConfig::new(50, 200)), |ctx, args: DomainListArgs| async move { let app_id = args.app_id.clone(); - let limit = args.limit; let client = make_client(&ctx, &[DOMAIN_READ]).await?; - let mut all_items: Vec = Vec::new(); - let mut page_token: Option = None; - - loop { - let page_limit = - limit.map(|cap| cap.saturating_sub(all_items.len() as u32).min(100)); + let response = client.list_domains(&app_id).await.map_err(client_err)?; + if let Some(warning) = more_pages_warning(&response, "domains") { + warn_user(&warning); + } - let response = client - .list_domains(&app_id, page_token.as_deref(), page_limit) - .await - .map_err(client_err)?; + let continue_token = pagination_token::resolve_continue_token( + args.offset, + ctx.middleware.continue_token.as_deref(), + ); + let (page, continuation) = pagination_token::paginate_known_collection( + page_items(&response), + ctx.middleware.cursor_limit, + continue_token.as_deref(), + ); - if let Some(items) = response.get("items").and_then(|v| v.as_array()) { - all_items.extend(items.iter().cloned()); - } + Ok(CommandResult::new(json!(page)) + .with_cursor(continuation) + .with_next_actions(vec![ + next_action( + "hosting domain attach --app-id --hostname ", + "Attach a new domain", + ) + .with_param("app-id", NextActionParam::value(app_id)), + ])) + }, + ) +} - if limit.is_some_and(|cap| all_items.len() >= cap as usize) { - all_items.truncate(limit.expect("checked") as usize); - break; - } +#[cfg(test)] +mod tests { + use cli_engine::CursorConfig; - match next_page_token(&response) { - Some(token) => page_token = Some(token), - None => break, - } - } + use super::command; - Ok(CommandResult::new(json!(all_items)).with_next_actions(vec![ - next_action( - "hosting domain attach --app-id --hostname ", - "Attach a new domain", - ) - .with_param("app-id", NextActionParam::value(app_id)), - ])) - }, - ) + #[test] + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(50, 200))); + } } diff --git a/rust/src/hosting/log/list.rs b/rust/src/hosting/log/list.rs index 7296b93a..f39399e9 100644 --- a/rust/src/hosting/log/list.rs +++ b/rust/src/hosting/log/list.rs @@ -1,7 +1,9 @@ -use cli_engine::{CommandResult, CommandSpec, RuntimeCommandSpec, Tier}; -use serde_json::{Value, json}; +use cli_engine::{CommandResult, CommandSpec, CursorConfig, RuntimeCommandSpec, Tier}; +use serde_json::json; -use crate::hosting::common::{HostingLogEntry, client_err, make_client, next_page_token}; +use crate::hosting::common::{ + HostingLogEntry, client_err, make_client, page_continuation, page_items, page_limit, +}; use crate::scopes::HOSTING_LOG_READ as LOG_READ; #[derive(Debug, Clone, clap::Args)] @@ -25,65 +27,52 @@ struct LogListArgs { /// Filter by severity level (INFO, WARN, or ERROR). #[arg(long, value_name = "LEVEL", value_parser = ["INFO", "WARN", "ERROR"])] level: Option, - - /// Maximum number of log entries to return (max 500). Omit to return all. - #[arg(long, value_name = "N", value_parser = clap::value_parser!(u32).range(1..=500))] - limit: Option, } pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List application logs") .with_long( - "Retrieve log entries for a hosting application. Results are autopaginated. \ - Use --variant to select the environment, --since for a time window, \ + "Retrieve log entries for a hosting application, one page at a time. Use \ + --limit to set the page size (max 500) and --continue with the token from \ + a page's cursor to fetch the next. Use --variant to select the environment, --since for a time window, \ --source and --level to filter by stream and severity.", ) .with_system("hosting") .with_tier(Tier::Read) .with_scopes(&[LOG_READ]) .with_default_fields("timestamp,level,message") - .with_output_schema::(), + .with_output_schema::() + .with_cursor(CursorConfig::new(100, 500)), |ctx, args: LogListArgs| async move { - let limit = args.limit; let client = make_client(&ctx, &[LOG_READ]).await?; - let mut all_items: Vec = Vec::new(); - let mut page_token: Option = None; - - loop { - let page_limit = - limit.map(|cap| cap.saturating_sub(all_items.len() as u32).min(500)); + let response = client + .list_logs( + &args.app_id, + args.variant.as_deref(), + args.since.as_deref(), + args.source.as_deref(), + args.level.as_deref(), + ctx.middleware.continue_token.as_deref(), + page_limit(ctx.middleware.cursor_limit), + ) + .await + .map_err(client_err)?; - let response = client - .list_logs( - &args.app_id, - args.variant.as_deref(), - args.since.as_deref(), - args.source.as_deref(), - args.level.as_deref(), - page_token.as_deref(), - page_limit, - ) - .await - .map_err(client_err)?; - - if let Some(items) = response.get("items").and_then(|v| v.as_array()) { - all_items.extend(items.iter().cloned()); - } + Ok(CommandResult::new(json!(page_items(&response))).with_cursor(page_continuation(&response))) + }, + ) +} - if limit.is_some_and(|cap| all_items.len() >= cap as usize) { - all_items.truncate(limit.expect("checked") as usize); - break; - } +#[cfg(test)] +mod tests { + use cli_engine::CursorConfig; - match next_page_token(&response) { - Some(token) => page_token = Some(token), - None => break, - } - } + use super::command; - Ok(CommandResult::new(json!(all_items))) - }, - ) + #[test] + fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(100, 500))); + } } diff --git a/rust/src/hosting/subscription/list.rs b/rust/src/hosting/subscription/list.rs index 646f2538..094a680a 100644 --- a/rust/src/hosting/subscription/list.rs +++ b/rust/src/hosting/subscription/list.rs @@ -5,7 +5,8 @@ use cli_engine::{ use serde_json::{Value, json}; use crate::hosting::common::{ - HostingProduct, HostingSubscriptionList, client_err, make_client, next_page_token, + HostingProduct, HostingSubscriptionList, client_err, make_client, more_pages_warning, + page_items, warn_user, }; use crate::next_action::next_action; use crate::scopes::HOSTING_SUBSCRIPTION_READ as SUB_READ; @@ -44,7 +45,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ) .with_long( "List hosting plan subscriptions available to attach to applications. \ - --hosting-product is required. Results are autopaginated.", + --hosting-product is required.", ) .with_system("hosting") .with_tier(Tier::Read) @@ -53,37 +54,18 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_output_schema::() .with_view(view_columns()), |ctx, args: SubscriptionListArgs| async move { - let limit = args.limit; let hosting_product = args.hosting_product.as_str(); let client = make_client(&ctx, &[SUB_READ]).await?; - let mut all_items: Vec = Vec::new(); - let mut page_token: Option = None; - - loop { - let page_limit = - limit.map(|cap| cap.saturating_sub(all_items.len() as u32).min(100)); - - let response = client - .list_subscriptions(page_token.as_deref(), page_limit, hosting_product) - .await - .map_err(client_err)?; - - if let Some(items) = response.get("items").and_then(|v| v.as_array()) { - all_items.extend(items.iter().cloned()); - } - - if limit.is_some_and(|cap| all_items.len() >= cap as usize) { - break; - } - - match next_page_token(&response) { - Some(token) => page_token = Some(token), - None => break, - } + let response = client + .list_subscriptions(hosting_product) + .await + .map_err(client_err)?; + if let Some(warning) = more_pages_warning(&response, "subscriptions") { + warn_user(&warning); } - Ok(list_result(all_items, limit)) + Ok(list_result(page_items(&response), args.limit)) }, ) } diff --git a/rust/src/main.rs b/rust/src/main.rs index 9ca6b6c2..b5a9a44a 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -13,6 +13,7 @@ mod http; mod malware; mod next_action; mod output_schema; +mod pagination_token; mod pat; mod payment_methods; mod platform; diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs new file mode 100644 index 00000000..0d3841a8 --- /dev/null +++ b/rust/src/pagination_token.rs @@ -0,0 +1,309 @@ +//! Shared opaque `--continue` token codec. +//! +//! Most of this CLI's paginated commands are not backed by a real cursor +//! API — a fixed page/pageSize backend, or a fully in-memory collection — +//! so they synthesize their own continuation token instead of getting one +//! from a server. This module gives every one of them the same small +//! `key:value,key2:value2` shape (the proposal's "Instructions" token type) +//! instead of each hand-rolling its own parsing. +//! +//! A command whose backend already hands back a real opaque cursor (e.g. +//! `domain list`'s `pageToken`) does not need this — it just passes that +//! token straight through as [`cli_engine::CursorContinuation::more`]'s +//! argument. + +use std::collections::HashMap; +use std::str::FromStr; + +use cli_engine::CursorContinuation; +use serde_json::Value; + +/// Parses a `key:value,key2:value2` token into a lookup map. +/// +/// `None`/empty input parses to an empty map — an absent or blank +/// `--continue` always means "start from the beginning," never a parse +/// error. A malformed pair (no `:`) is silently skipped rather than +/// rejected, matching [`field`]'s "degrade to the default" philosophy: a +/// hand-edited or truncated `--continue` value should restart pagination, +/// not fail the command outright. +pub(crate) fn parse(token: Option<&str>) -> HashMap { + let Some(token) = token.filter(|t| !t.is_empty()) else { + return HashMap::new(); + }; + token + .split(',') + .filter_map(|pair| pair.split_once(':')) + .map(|(key, value)| (key.to_owned(), value.to_owned())) + .collect() +} + +/// Reads a typed field out of a [`parse`]d map, falling back to `default` +/// when the key is absent or its value doesn't parse as `T` — the same +/// "degrade to start over" behavior `parse` itself uses for a malformed +/// pair. +pub(crate) fn field(fields: &HashMap, key: &str, default: T) -> T { + fields + .get(key) + .and_then(|value| value.parse().ok()) + .unwrap_or(default) +} + +/// Builds a `key:value,key2:value2` token from ordered pairs. +pub(crate) fn build(pairs: &[(&str, &str)]) -> String { + pairs + .iter() + .map(|(key, value)| format!("{key}:{value}")) + .collect::>() + .join(",") +} + +fn saturating_usize(value: i64) -> usize { + usize::try_from(value).unwrap_or(usize::MAX) +} + +fn saturating_i64(value: usize) -> i64 { + i64::try_from(value).unwrap_or(i64::MAX) +} + +/// Resolves the effective `--continue` value for a "known collection" +/// command: an explicit `--offset` wins, synthesized into a bare +/// `offset:` token (deliberately without a `limit` field — a fresh jump +/// has no prior page to inherit a size from, so [`paginate_known_collection`] +/// falls back to the request's own `--limit` when it sees one); otherwise +/// the parsed `--continue` token (which includes a `limit`) is used as-is. Lets +/// a user jump straight to a position without walking there one token at a +/// time, without giving `--offset` and `--continue` two different encodings to +/// keep in sync. +pub(crate) fn resolve_continue_token( + explicit_offset: Option, + continue_token: Option<&str>, +) -> Option { + explicit_offset + .map(|offset| build(&[("offset", &offset.to_string())])) + .or_else(|| continue_token.map(str::to_owned)) +} + +/// Resolves the starting page and page size for a page/pageSize-backed +/// command's request. Unlike an item-anchored token (e.g. +/// [`paginate_known_collection`]'s `offset:`), "page N" is only +/// meaningful relative to the page size that produced it — resuming with +/// `--continue` but a *different* page size would silently request a +/// different slice than the one the token implies (page 2 of size-10 pages +/// covers different items than page 2 of size-25 pages). So a `--continue` +/// token's own embedded `limit` wins whenever present, letting a caller +/// resume with `--continue` alone and no need to repeat `--limit` — the same +/// way a real forward-only cursor already carries its own page size +/// server-side, opaque to the caller. +/// +/// An explicit `--page` (a command's own supplemental direct-jump arg, +/// `conflicts_with = "continue"`) always wins for the page instead, and in +/// that case `limit` (the request's own parsed `--limit`) always wins for +/// the size — there's no prior token to inherit a size from. +pub(crate) fn resolve_page_request( + explicit_page: Option

, + continue_token: Option<&str>, + default_page: P, + limit: i64, +) -> (P, i64) { + let fields = parse(continue_token); + let page = explicit_page.unwrap_or_else(|| field(&fields, "page", default_page)); + let page_size = field(&fields, "limit", limit); + (page, page_size) +} + +/// Slices an already-fully-known collection using a `limit:,offset:` +/// continuation token. +pub(crate) fn paginate_known_collection( + items: Vec, + limit: i64, + continue_token: Option<&str>, +) -> (Vec, CursorContinuation) { + let fields = parse(continue_token); + let offset = field(&fields, "offset", 0usize); + let limit = field(&fields, "limit", limit); + let limit = saturating_usize(limit).max(1); + let total = items.len(); + let start = offset.min(total); + let end = start.saturating_add(limit).min(total); + let page = items.get(start..end).unwrap_or_default().to_vec(); + let remaining = total.saturating_sub(end); + + let mut continuation = if end < total { + CursorContinuation::more(build(&[ + ("limit", &limit.to_string()), + ("offset", &end.to_string()), + ])) + } else { + CursorContinuation::done() + }; + continuation = continuation + .with_total(saturating_i64(total)) + .with_remaining(saturating_i64(remaining)) + .with_limit(saturating_i64(limit)); + (page, continuation) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn parse_reads_key_value_pairs() { + let fields = parse(Some("page:2,limit:50")); + assert_eq!(fields.get("page").map(String::as_str), Some("2")); + assert_eq!(fields.get("limit").map(String::as_str), Some("50")); + } + + #[test] + fn parse_of_none_or_empty_is_an_empty_map() { + assert!(parse(None).is_empty()); + assert!(parse(Some("")).is_empty()); + } + + #[test] + fn parse_skips_a_pair_with_no_colon() { + let fields = parse(Some("page:2,garbage,limit:50")); + assert_eq!(fields.len(), 2); + } + + #[test] + fn field_falls_back_on_missing_or_unparseable_value() { + let fields = parse(Some("page:not-a-number")); + assert_eq!(field(&fields, "page", 1u32), 1); + assert_eq!(field(&fields, "missing", 7u32), 7); + } + + #[test] + fn field_reads_a_present_valid_value() { + let fields = parse(Some("page:3")); + assert_eq!(field(&fields, "page", 1u32), 3); + } + + #[test] + fn build_and_parse_round_trip() { + let token = build(&[("page", "3"), ("limit", "20")]); + let fields = parse(Some(&token)); + assert_eq!(field(&fields, "page", 0u32), 3); + assert_eq!(field(&fields, "limit", 0u32), 20); + } + + #[test] + fn resolve_continue_token_prefers_an_explicit_offset() { + let resolved = resolve_continue_token(Some(20), Some("offset:5")); + assert_eq!(resolved.as_deref(), Some("offset:20")); + } + + #[test] + fn resolve_continue_token_falls_back_to_the_continue_value() { + let resolved = resolve_continue_token(None, Some("offset:5")); + assert_eq!(resolved.as_deref(), Some("offset:5")); + } + + #[test] + fn resolve_continue_token_of_neither_is_none() { + assert_eq!(resolve_continue_token(None, None), None); + } + + #[test] + fn resolve_page_request_with_no_input_defaults_the_page_and_uses_the_given_limit() { + let (page, page_size) = resolve_page_request(None::, None, 1u32, 25); + assert_eq!(page, 1); + assert_eq!(page_size, 25); + } + + #[test] + fn resolve_page_request_inherits_both_page_and_size_from_the_continue_token() { + // The size that produced page 2 (10) must carry forward even though + // this call's own --limit defaulted to something else (25) — page 2 + // of size-10 pages and page 2 of size-25 pages cover different items. + let token = build(&[("limit", "10"), ("page", "2")]); + let (page, page_size) = resolve_page_request(None::, Some(&token), 1u32, 25); + assert_eq!(page, 2); + assert_eq!(page_size, 10); + } + + #[test] + fn resolve_page_request_an_explicit_page_wins_and_uses_the_requests_own_limit() { + // No continue token at all (--page conflicts_with --continue), so + // there's no prior size to inherit — the request's own limit applies. + let (page, page_size) = resolve_page_request(Some(5u32), None, 1u32, 25); + assert_eq!(page, 5); + assert_eq!(page_size, 25); + } + + fn items(n: usize) -> Vec { + (0..n).map(|i| json!({ "id": i })).collect() + } + + #[test] + fn paginate_known_collection_returns_the_first_page_with_no_token() { + let (page, cont) = paginate_known_collection(items(5), 2, None); + assert_eq!(page, vec![json!({"id": 0}), json!({"id": 1})]); + assert!(cont.continue_from.is_some()); + assert_eq!(cont.total, Some(5)); + assert_eq!(cont.remaining, Some(3)); + } + + #[test] + fn paginate_known_collection_resumes_from_the_continue_token() { + let (_, cont) = paginate_known_collection(items(5), 2, None); + let token = cont.continue_from.expect("has_more implies a token"); + let (page, cont2) = paginate_known_collection(items(5), 2, Some(&token)); + assert_eq!(page, vec![json!({"id": 2}), json!({"id": 3})]); + assert!(cont2.continue_from.is_some()); + assert_eq!(cont2.remaining, Some(1)); + } + + #[test] + fn paginate_known_collection_continue_token_embeds_and_inherits_the_limit() { + let (_, cont) = paginate_known_collection(items(5), 2, None); + let token = cont.continue_from.expect("has_more implies a token"); + assert_eq!(token, "limit:2,offset:2"); + + // A caller that resumes with --continue alone, and whose --limit + // defaulted to something else entirely (25) this call, must still + // get the size (2) that produced the token, not the new default. + let (page, cont2) = paginate_known_collection(items(5), 25, Some(&token)); + assert_eq!(page, vec![json!({"id": 2}), json!({"id": 3})]); + assert_eq!(cont2.remaining, Some(1)); + } + + #[test] + fn paginate_known_collection_an_explicit_offset_token_has_no_limit_to_inherit() { + // resolve_continue_token synthesizes a bare `offset:` (no + // `limit`) for an explicit --offset — the request's own limit + // applies, same as a fresh, tokenless call. + let token = resolve_continue_token(Some(2), None).expect("explicit offset"); + let (page, _) = paginate_known_collection(items(5), 3, Some(&token)); + assert_eq!( + page, + vec![json!({"id": 2}), json!({"id": 3}), json!({"id": 4})] + ); + } + + #[test] + fn paginate_known_collection_reaches_the_end_with_no_further_token() { + let (page, cont) = paginate_known_collection(items(5), 2, Some("offset:4")); + assert_eq!(page, vec![json!({"id": 4})]); + assert!(cont.continue_from.is_none()); + assert_eq!(cont.remaining, Some(0)); + } + + #[test] + fn paginate_known_collection_past_the_end_returns_empty() { + let (page, cont) = paginate_known_collection(items(3), 2, Some("offset:10")); + assert!(page.is_empty()); + assert!(cont.continue_from.is_none()); + assert_eq!(cont.remaining, Some(0)); + } + + #[test] + fn paginate_known_collection_of_an_empty_collection() { + let (page, cont) = paginate_known_collection(Vec::new(), 10, None); + assert!(page.is_empty()); + assert!(cont.continue_from.is_none()); + assert_eq!(cont.total, Some(0)); + } +} diff --git a/rust/src/platform/actions/mod.rs b/rust/src/platform/actions/mod.rs index 9d3dfba2..b172f5f0 100644 --- a/rust/src/platform/actions/mod.rs +++ b/rust/src/platform/actions/mod.rs @@ -1,11 +1,12 @@ use cli_engine::{ - CommandResult, CommandSpec, GroupSpec, NextActionParam, PaginationConfig, RuntimeCommandSpec, + CommandResult, CommandSpec, CursorConfig, GroupSpec, NextActionParam, RuntimeCommandSpec, RuntimeGroupSpec, Tier, }; use serde_json::{Value, json}; use crate::next_action::next_action; use crate::output_schema::output_schema; +use crate::pagination_token; use crate::truncation::protect_payload; output_schema!(ActionSummary { @@ -87,12 +88,12 @@ pub fn group() -> RuntimeGroupSpec { `gddy platform action describe` to inspect a specific action's schema.", ), ) - .with_command(RuntimeCommandSpec::new( + .with_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List all available action contracts") .with_long( "Prints the name and short description of every action your \ application can declare. Shows up to 50 actions by default; \ - use --limit/--offset to page through the rest once the \ + use --limit/--continue to page through the rest once the \ catalog grows past that.\n\ Run `gddy platform action describe ` to see the full \ input/output schema for a specific action.", @@ -102,13 +103,35 @@ pub fn group() -> RuntimeGroupSpec { .no_auth(true) .with_default_fields("name,description") .with_output_schema::() - .with_pagination(PaginationConfig::new(50, 200)), - |_cred, _args| async move { + .with_cursor(CursorConfig::new(50, 200)) + .with_arg( + clap::Arg::new("offset") + .long("offset") + .value_name("N") + .value_parser(clap::value_parser!(i64).range(0..)) + .conflicts_with("continue") + .help( + "Jump directly to an offset in the results", + ), + ), + |ctx| async move { let actions: Vec<_> = ACTIONS .iter() .map(|(name, description)| json!({ "name": name, "description": description })) .collect(); - Ok(CommandResult::new(json!(actions)).with_next_actions(vec![ + let explicit_offset = ctx.args.get("offset").and_then(Value::as_i64); + let continue_token = pagination_token::resolve_continue_token( + explicit_offset, + ctx.middleware.continue_token.as_deref(), + ); + let (page, continuation) = pagination_token::paginate_known_collection( + actions, + ctx.middleware.cursor_limit, + continue_token.as_deref(), + ); + Ok(CommandResult::new(json!(page)) + .with_cursor(continuation) + .with_next_actions(vec![ next_action( "platform action describe ", "See an action's full schema", @@ -175,7 +198,7 @@ mod tests { } #[tokio::test] - async fn list_reports_pagination_metadata_via_cli_engine() { + async fn list_reports_cursor_metadata_via_cli_engine() { let cli = test_cli(); let output = cli .run(["gddy", "platform", "actions", "list", "--output", "json"]) @@ -183,9 +206,59 @@ mod tests { assert_eq!(output.exit_code, 0, "{}", output.rendered); let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("stdout should contain json"); - let pagination = &rendered["pagination"]; - assert_eq!(pagination["total"], pagination["count"]); - assert_eq!(pagination["has_more"], serde_json::json!(false)); + let cursor = &rendered["cursor"]; + assert_eq!(cursor["total"], cursor["count"]); + assert_eq!(cursor["has_more"], serde_json::json!(false)); + } + + #[tokio::test] + async fn list_limit_and_continue_page_through_the_catalog() { + let cli = test_cli(); + let first = cli + .run([ + "gddy", "platform", "actions", "list", "--limit", "3", "--output", "json", + ]) + .await; + assert_eq!(first.exit_code, 0, "{}", first.rendered); + let first_json: serde_json::Value = + serde_json::from_str(&first.rendered).expect("stdout should contain json"); + assert_eq!(first_json["data"].as_array().expect("array").len(), 3); + assert_eq!(first_json["cursor"]["has_more"], serde_json::json!(true)); + let token = first_json["cursor"]["continue_from"] + .as_str() + .expect("has_more implies a token"); + + let second = cli + .run([ + "gddy", + "platform", + "actions", + "list", + "--limit", + "3", + "--continue", + token, + "--output", + "json", + ]) + .await; + assert_eq!(second.exit_code, 0, "{}", second.rendered); + let second_json: serde_json::Value = + serde_json::from_str(&second.rendered).expect("stdout should contain json"); + assert_eq!(second_json["data"].as_array().expect("array").len(), 3); + + // An explicit --offset should land on the same page --continue's + // token did, without needing the token at all. + let via_offset = cli + .run([ + "gddy", "platform", "actions", "list", "--limit", "3", "--offset", "3", "--output", + "json", + ]) + .await; + assert_eq!(via_offset.exit_code, 0, "{}", via_offset.rendered); + let via_offset_json: serde_json::Value = + serde_json::from_str(&via_offset.rendered).expect("stdout should contain json"); + assert_eq!(via_offset_json["data"], second_json["data"]); } #[tokio::test] diff --git a/rust/src/platform/app/client.rs b/rust/src/platform/app/client.rs index 25566399..8e92e00a 100644 --- a/rust/src/platform/app/client.rs +++ b/rust/src/platform/app/client.rs @@ -88,6 +88,14 @@ pub struct UploadResult { pub size_bytes: u64, } +/// One Relay cursor page of [`ApplicationClient::list_applications`]. +#[derive(Debug, Clone)] +pub struct ApplicationsPage { + pub items: Vec, + pub has_next_page: bool, + pub end_cursor: Option, +} + /// Retry/backoff tuning for [`ApplicationClient::upload_artifact`]. #[derive(Debug, Clone)] pub struct UploadOptions { @@ -151,14 +159,18 @@ impl ApplicationClient { )?) } - /// Lists applications across every App Registry status. + /// Lists applications across every App Registry status, one Relay cursor + /// page at a time (`first`/`after` per the `applications(...)` field in the + /// vendored subgraph schema). /// /// Always sends an explicit `status.in` of [`APPLICATION_STATUSES`]. Omitting /// the GraphQL `status` argument is *not* equivalent: the API defaults to /// ACTIVE-only. pub async fn list_applications( &self, - ) -> Result, ClientError> { + first: i64, + after: Option<&str>, + ) -> Result { use applications_list::ApplicationStatus; let statuses: Vec = APPLICATION_STATUSES .iter() @@ -179,16 +191,29 @@ impl ApplicationClient { in_: Some(statuses), ne: None, }), + first: Some(first), + after: after.map(str::to_owned), }) .await?; - Ok(response - .applications - .and_then(|connection| connection.edges) + let Some(connection) = response.applications else { + return Ok(ApplicationsPage { + items: Vec::new(), + has_next_page: false, + end_cursor: None, + }); + }; + let items = connection + .edges .into_iter() .flatten() .flatten() .filter_map(|edge| edge.node) - .collect()) + .collect(); + Ok(ApplicationsPage { + items, + has_next_page: connection.page_info.has_next_page, + end_cursor: connection.page_info.end_cursor, + }) } pub async fn get_application( diff --git a/rust/src/platform/app/client_tests.rs b/rust/src/platform/app/client_tests.rs index ba21f78b..61a835d7 100644 --- a/rust/src/platform/app/client_tests.rs +++ b/rust/src/platform/app/client_tests.rs @@ -4,7 +4,7 @@ use serde_json::json; use super::*; #[tokio::test] -async fn list_applications_sends_all_app_registry_statuses() { +async fn list_applications_sends_all_app_registry_statuses_and_the_cursor_window() { let server = MockServer::start_async().await; let mock = server .mock_async(|when, then| { @@ -15,6 +15,8 @@ async fn list_applications_sends_all_app_registry_statuses() { let body = req.body_string(); body.contains("ApplicationsList") && body.contains(r#""in":["ACTIVE","ARCHIVED","BLOCKED","INACTIVE","VERIFYING"]"#) + && body.contains(r#""first":10"#) + && body.contains(r#""after":"cursor-1""#) }); then.status(200).json_body(json!({ "data": { @@ -22,24 +24,63 @@ async fn list_applications_sends_all_app_registry_statuses() { "edges": [ { "node": { "id": "a1", "label": "Active", "name": "active-app", "description": null, "status": "ACTIVE", "url": null, "proxyUrl": null } }, { "node": { "id": "a2", "label": "Inactive", "name": "inactive-app", "description": null, "status": "INACTIVE", "url": null, "proxyUrl": null } } - ] + ], + "pageInfo": { "hasNextPage": true, "endCursor": "cursor-2" } } } })); }) .await; - let nodes = ApplicationClient::new(server.base_url(), "test-token") - .list_applications() + let page = ApplicationClient::new(server.base_url(), "test-token") + .list_applications(10, Some("cursor-1")) .await .expect("list applications"); mock.assert_async().await; - assert_eq!(nodes.len(), 2); + assert_eq!(page.items.len(), 2); assert_eq!( - nodes[1].status, + page.items[1].status, applications_list::ApplicationStatus::INACTIVE ); + assert!(page.has_next_page); + assert_eq!(page.end_cursor, Some("cursor-2".to_string())); +} + +#[tokio::test] +async fn list_applications_with_no_after_omits_it_and_reports_the_last_page() { + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/v1/apps/app-registry-subgraph") + .header("authorization", "Bearer test-token") + .is_true(|req| { + let body = req.body_string(); + body.contains("ApplicationsList") && !body.contains(r#""after""#) + }); + then.status(200).json_body(json!({ + "data": { + "applications": { + "edges": [ + { "node": { "id": "a1", "label": "Active", "name": "active-app", "description": null, "status": "ACTIVE", "url": null, "proxyUrl": null } } + ], + "pageInfo": { "hasNextPage": false, "endCursor": null } + } + } + })); + }) + .await; + + let page = ApplicationClient::new(server.base_url(), "test-token") + .list_applications(10, None) + .await + .expect("list applications"); + + mock.assert_async().await; + assert_eq!(page.items.len(), 1); + assert!(!page.has_next_page); + assert_eq!(page.end_cursor, None); } #[tokio::test] diff --git a/rust/src/platform/app/commands/list.rs b/rust/src/platform/app/commands/list.rs index 4ad00667..8cc6d4bb 100644 --- a/rust/src/platform/app/commands/list.rs +++ b/rust/src/platform/app/commands/list.rs @@ -1,12 +1,17 @@ //! `gddy platform app list` — list all applications visible to the account. use cli_engine::{ - CommandResult, CommandSpec, NextActionParam, PaginationConfig, RuntimeCommandSpec, Tier, + CommandResult, CommandSpec, CursorConfig, CursorContinuation, NextActionParam, + RuntimeCommandSpec, Tier, }; use super::schemas::ApplicationSummary; use crate::next_action::next_action; +/// Pages through `applications(first:, after:)` with the Relay `endCursor` as +/// the `--continue` token. No supplemental `--offset` (unlike the client-side +/// "known collection" commands): a forward-only cursor has no arbitrary-jump +/// concept, since the server never hands back more than one page at a time. pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List all applications") @@ -20,18 +25,30 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_tier(Tier::Read) .with_default_fields("name,label,status") .with_output_schema::() - .with_pagination(PaginationConfig::new(0, 200)), + .with_cursor(CursorConfig::new(50, 200)), |ctx| async move { let client = super::make_client(&ctx).await?; - let apps = client - .list_applications() + let page = client + .list_applications( + ctx.middleware.cursor_limit, + ctx.middleware.continue_token.as_deref(), + ) .await .map_err(super::client_err)?; - let data = serde_json::to_value(apps).map_err(|e| { + let continuation = match (page.has_next_page, page.end_cursor) { + (true, Some(cursor)) => CursorContinuation::more(cursor), + // `hasNextPage: true` with no `endCursor` would be a + // malformed response — treat it the same as "done" rather + // than fabricating a token that can't actually resume. + _ => CursorContinuation::done(), + }; + let data = serde_json::to_value(page.items).map_err(|e| { crate::error::GddyError::unexpected(format!("failed to encode applications: {e}")) .into_cli_error() })?; - Ok(CommandResult::new(data).with_next_actions(vec![ + Ok(CommandResult::new(data) + .with_cursor(continuation) + .with_next_actions(vec![ next_action( "platform app info --name ", "Get details for a specific application", @@ -48,17 +65,14 @@ pub(super) fn command() -> RuntimeCommandSpec { #[cfg(test)] mod tests { - use cli_engine::{Cli, CliConfig, PaginationConfig, Stage}; + use cli_engine::{Cli, CliConfig, CursorConfig, Stage}; use super::command; use crate::platform::app::client::APPLICATION_STATUSES; #[test] - fn command_opts_into_pagination_with_no_default_and_a_max_limit() { - assert_eq!( - command().spec.pagination, - Some(PaginationConfig::new(0, 200)) - ); + fn command_opts_into_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(50, 200))); } #[test] From 3ca6f931b4a5bd053e423a9b39b1bebbfc916376 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 21 Oct 2026 17:04:49 -0700 Subject: [PATCH 02/10] fix(cli): make cursor next-page actions replayable and tighten list paging - A `--continue` token now takes precedence over `--offset`/`--page`. The next-page action replays the command's own arguments, so after a direct jump it reads `... --offset 20 --continue `; the two flags no longer conflict and the token decides the page, so the action advances instead of failing to parse or repeating the same page. - `dns list --page` and `email list --page` reject 0 at parse time, since pages are 1-based. - `dns list` reports `cursor.total` and `cursor.remaining` when the API returns a total. - `email list` decides whether another page exists from the response's `rel=next` link rather than from the item count, so a full final page no longer offers an empty next page. - The incomplete-list warning test asserts the shipped wording, and the warning's formatting matches rustfmt. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/api/parameter.rs | 53 +++++++++++++++++++- rust/src/api/response.rs | 4 +- rust/src/api/search.rs | 4 +- rust/src/dns/list.rs | 49 ++++++++++++++++-- rust/src/dns/records.rs | 34 ++++++++++--- rust/src/email/list.rs | 86 ++++++++++++++++++++++++++++---- rust/src/hosting/common.rs | 8 +-- rust/src/hosting/domain/list.rs | 4 +- rust/src/pagination_token.rs | 63 ++++++++++++++++------- rust/src/platform/actions/mod.rs | 60 +++++++++++++++++++++- 10 files changed, 307 insertions(+), 58 deletions(-) diff --git a/rust/src/api/parameter.rs b/rust/src/api/parameter.rs index 9a216cca..79e83d70 100644 --- a/rust/src/api/parameter.rs +++ b/rust/src/api/parameter.rs @@ -23,8 +23,8 @@ struct ParameterListArgs { #[arg(long, value_name = "OPERATION")] operation: String, - /// Jump directly to an offset in the collection. - #[arg(long, value_name = "N", conflicts_with = "continue")] + /// Jump directly to an offset in the collection. Ignored when `--continue` is given. + #[arg(long, value_name = "N")] offset: Option, } @@ -250,6 +250,55 @@ mod tests { ); } + /// The next-page action the engine advertises after a direct `--offset` + /// jump replays that `--offset` next to the new `--continue`. Executing it + /// exactly as printed must advance to the following page, not fail to + /// parse and not land on the same page again. + #[tokio::test] + async fn parameter_list_next_action_after_an_offset_jump_advances() { + let first = operation_cli() + .run([ + "gddy", + "api", + "parameter", + "list", + "--operation", + "get_transaction_disputes", + "--limit", + "5", + "--offset", + "20", + "--output", + "json", + ]) + .await; + assert_eq!(first.exit_code, 0, "{}", first.rendered); + let first: serde_json::Value = serde_json::from_str(&first.rendered).expect("valid json"); + let command = first["next_actions"] + .as_array() + .and_then(|actions| { + actions + .iter() + .filter_map(|a| a["command"].as_str()) + .find(|c| c.contains("--continue")) + }) + .expect("a next-page action"); + + let argv: Vec<&str> = command + .split_whitespace() + .map(|part| part.trim_matches('"')) + .chain(["--output", "json"]) + .collect(); + let second = operation_cli().run(argv).await; + assert_eq!(second.exit_code, 0, "{command}: {}", second.rendered); + let second: serde_json::Value = serde_json::from_str(&second.rendered).expect("valid json"); + + // 26 rows: offsets 20..25 were page one, so only offset 25 remains. + assert_eq!(second["data"].as_array().expect("data array").len(), 1); + assert_eq!(second["cursor"]["has_more"], json!(false)); + assert_ne!(first["data"], second["data"]); + } + /// `max_limit` (`OPERATION_CHILD_LIST_MAX_LIMIT`) rejects an /// out-of-range `--limit` at parse time, before the handler even runs. #[tokio::test] diff --git a/rust/src/api/response.rs b/rust/src/api/response.rs index c97baceb..22f26cb3 100644 --- a/rust/src/api/response.rs +++ b/rust/src/api/response.rs @@ -20,8 +20,8 @@ struct ResponseListArgs { #[arg(long, value_name = "OPERATION")] operation: String, - /// Jump directly to an offset in the collection. - #[arg(long, value_name = "N", conflicts_with = "continue")] + /// Jump directly to an offset in the collection. Ignored when `--continue` is given. + #[arg(long, value_name = "N")] offset: Option, } diff --git a/rust/src/api/search.rs b/rust/src/api/search.rs index 8f4b9faf..53cd1869 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -31,8 +31,8 @@ struct SearchArgs { query: String, /// Jump directly to this offset instead of walking there one --continue - /// token at a time. - #[arg(long, value_name = "N", conflicts_with = "continue")] + /// token at a time. Ignored when `--continue` is given. + #[arg(long, value_name = "N")] offset: Option, } diff --git a/rust/src/dns/list.rs b/rust/src/dns/list.rs index e1e4c3bf..b473ec69 100644 --- a/rust/src/dns/list.rs +++ b/rust/src/dns/list.rs @@ -29,8 +29,8 @@ struct ListArgs { #[arg(long, value_name = "NAME", requires = "record_type")] name: Option, - /// Jump directly to a page of results (1-based). - #[arg(long, value_name = "N", conflicts_with = "continue")] + /// Jump directly to a page of results (1-based). Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(u64).range(1..))] page: Option, } @@ -87,7 +87,7 @@ pub(super) fn command() -> RuntimeCommandSpec { 1u64, ctx.middleware.cursor_limit, ); - let (records, has_more) = fetch_records_page( + let fetched = fetch_records_page( &client, domain.as_str(), type_opt.as_deref(), @@ -98,10 +98,10 @@ pub(super) fn command() -> RuntimeCommandSpec { ) .await?; - let out = build_list_output(&records).map_err(|e| { + let out = build_list_output(&fetched.records).map_err(|e| { CliCoreError::message(format!("failed to serialize DNS records: {e}")) })?; - let continuation = if has_more { + let mut continuation = if fetched.has_more { CursorContinuation::more(pagination_token::build(&[ ("limit", &page_size.to_string()), ("page", &(page + 1).to_string()), @@ -109,12 +109,35 @@ pub(super) fn command() -> RuntimeCommandSpec { } else { CursorContinuation::done() }; + if let Some(total) = fetched.total { + continuation = continuation + .with_total(total) + .with_remaining(remaining_after( + total, + page, + page_size, + fetched.records.len(), + )); + } let continuation = continuation.with_limit(page_size); Ok(CommandResult::new(json!(out)).with_cursor(continuation)) }, ) } +/// Records left after `page` (1-based) of `page_size`, given the API's `total` +/// and how many records that page returned. Saturating throughout, so a +/// hand-edited `--continue` token or an out-of-range `--page` can't underflow +/// or go negative. +fn remaining_after(total: i64, page: u64, page_size: i64, returned: usize) -> i64 { + let page_size = page_size.clamp(1, MAX_PAGE_SIZE.get() as i64); + let before = i64::try_from(page.saturating_sub(1)) + .unwrap_or(i64::MAX) + .saturating_mul(page_size); + let seen = before.saturating_add(i64::try_from(returned).unwrap_or(i64::MAX)); + total.saturating_sub(seen).max(0) +} + /// Serialize fetched records for `dns list`'s output, re-merging TLSA's /// split fields back into a single `data` string (see /// [`merged_tlsa_data`]) — every other type already has a meaningful `data`. @@ -141,6 +164,22 @@ mod tests { use super::*; use cli_engine::CursorConfig; + #[test] + fn remaining_after_counts_what_is_left_past_the_current_page() { + assert_eq!(remaining_after(250, 1, 100, 100), 150); + assert_eq!(remaining_after(250, 2, 100, 100), 50); + assert_eq!(remaining_after(250, 3, 100, 50), 0); + } + + #[test] + fn remaining_after_never_goes_negative_or_overflows() { + // Page past the end, an oversized page size (clamped), and extreme values. + assert_eq!(remaining_after(10, 9, 100, 0), 0); + assert_eq!(remaining_after(250, 1, 10_000, 100), 150); + assert_eq!(remaining_after(i64::MAX, u64::MAX, i64::MAX, usize::MAX), 0); + assert_eq!(remaining_after(0, 1, 50, 0), 0); + } + #[test] fn opts_into_cursor_pagination_with_a_default_and_a_max_limit() { assert_eq!( diff --git a/rust/src/dns/records.rs b/rust/src/dns/records.rs index aa349473..db0d4d3d 100644 --- a/rust/src/dns/records.rs +++ b/rust/src/dns/records.rs @@ -428,6 +428,14 @@ pub(super) async fn fetch_records( Ok(all) } +/// One page of DNS records plus what the API reports about the rest. +pub(super) struct RecordsPage { + pub records: Vec, + pub has_more: bool, + /// Matching records across all pages; `None` when the API doesn't say. + pub total: Option, +} + /// Fetch a page of matching DNS records. pub(super) async fn fetch_records_page( client: &domains_client::Client, @@ -437,7 +445,7 @@ pub(super) async fn fetch_records_page( page: u64, page_size: i64, debug: bool, -) -> Result<(Vec, bool), CliCoreError> { +) -> Result { let page_size_nz = std::num::NonZeroU64::new(page_size.clamp(1, MAX_PAGE_SIZE.get() as i64) as u64) .unwrap_or(MAX_PAGE_SIZE); @@ -465,7 +473,14 @@ pub(super) async fn fetch_records_page( .total_pages .map(|tp| page_nz.get() < tp.get()) .unwrap_or(got == page_size_usize && got > 0); - Ok((items, has_more)) + let total = body + .total_items + .map(|total| i64::try_from(total.get()).unwrap_or(i64::MAX)); + Ok(RecordsPage { + records: items, + has_more, + total, + }) } /// Next action pointing back at `dns list` to verify a write, pre-filled with @@ -733,6 +748,7 @@ mod tests { then.status(200).json_body(serde_json::json!({ "items": [{"type": "A", "name": "www", "data": "1.2.3.4", "ttl": 3600}], "totalPages": 2, + "totalItems": 3, })); }) .await; @@ -744,12 +760,13 @@ mod tests { ) .expect("build client"); - let (items, has_more) = fetch_records_page(&client, "example.com", None, None, 1, 2, false) + let page = fetch_records_page(&client, "example.com", None, None, 1, 2, false) .await .expect("fetch succeeds"); - assert_eq!(items.len(), 1); - assert!(has_more, "page 1 of 2 total pages has more"); + assert_eq!(page.records.len(), 1); + assert!(page.has_more, "page 1 of 2 total pages has more"); + assert_eq!(page.total, Some(3)); mock.assert_async().await; } @@ -775,7 +792,7 @@ mod tests { ) .expect("build client"); - let (items, has_more) = fetch_records_page( + let page = fetch_records_page( &client, "example.com", None, @@ -787,11 +804,12 @@ mod tests { .await .expect("fetch succeeds"); - assert_eq!(items.len(), 1); + assert_eq!(page.records.len(), 1); assert!( - !has_more, + !page.has_more, "a short page with no total_pages means exhausted" ); + assert_eq!(page.total, None, "no totalItems in the response"); mock.assert_async().await; } diff --git a/rust/src/email/list.rs b/rust/src/email/list.rs index a0ee2889..a6ec0d8e 100644 --- a/rust/src/email/list.rs +++ b/rust/src/email/list.rs @@ -21,8 +21,8 @@ struct ListArgs { /// Comma-separated list of fields to include in the response. #[arg(long, value_name = "FIELDS")] fields: Option, - /// Jump directly to a page of results (1-based). - #[arg(long, value_name = "N", conflicts_with = "continue")] + /// Jump directly to a page of results (1-based). Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(u32).range(1..))] page: Option, } @@ -76,10 +76,10 @@ pub(super) fn command() -> RuntimeCommandSpec { ) } -/// Fetches a page from `GET /v1/email/mailboxes`. `has_more` is a -/// short-page heuristic (a full page might still be the exact last one) — -/// this API never reports a true total, so "short page = done" is the best -/// signal available. +/// Fetches a page from `GET /v1/email/mailboxes`. `has_more` is whether the +/// response carries a `links[rel=next]`, which the spec says is absent on the +/// last page — so a full final page correctly reports no more, and a short +/// page with a next link correctly reports more. async fn fetch_mailbox_page( client: &EmailClient, status: Option<&str>, @@ -95,7 +95,7 @@ async fn fetch_mailbox_page( fields, ) .await?; - let has_more = list.items.len() >= page_size; + let has_more = list.links.iter().any(|link| link.rel == "next"); Ok((list.items, has_more)) } @@ -137,8 +137,13 @@ mod tests { .path("/v1/email/mailboxes") .query_param("page", "1") .query_param("pageSize", "3"); - then.status(200) - .json_body(json!({ "items": page_of(3, 0) })); + then.status(200).json_body(json!({ + "items": page_of(3, 0), + "links": [ + { "rel": "self", "href": "/v1/email/mailboxes?page=1&pageSize=3" }, + { "rel": "next", "href": "/v1/email/mailboxes?page=2&pageSize=3" } + ] + })); }) .await; @@ -149,7 +154,66 @@ mod tests { mock.assert_calls_async(1).await; assert_eq!(mailboxes.len(), 3); - assert!(has_more, "a full page is treated as possibly having more"); + assert!(has_more, "a next link means another page exists"); + } + + #[tokio::test] + async fn fetch_mailbox_page_reports_no_more_on_a_full_final_page() { + // Exactly page-size items and no next link: the last page happens to be + // full. An item-count heuristic would wrongly offer a page that is empty. + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET) + .path("/v1/email/mailboxes") + .query_param("page", "2") + .query_param("pageSize", "3"); + then.status(200).json_body(json!({ + "items": page_of(3, 3), + "links": [ + { "rel": "self", "href": "/v1/email/mailboxes?page=2&pageSize=3" }, + { "rel": "prev", "href": "/v1/email/mailboxes?page=1&pageSize=3" } + ] + })); + }) + .await; + + let client = client_for(&server); + let (mailboxes, has_more) = fetch_mailbox_page(&client, None, None, 2, 3) + .await + .expect("fetch mailboxes"); + + mock.assert_calls_async(1).await; + assert_eq!(mailboxes.len(), 3); + assert!(!has_more, "no next link means this is the last page"); + } + + #[tokio::test] + async fn fetch_mailbox_page_reports_more_on_a_short_page_with_a_next_link() { + // The server may return fewer than pageSize items and still have more + // (e.g. filtering after paging); the link, not the count, decides. + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET).path("/v1/email/mailboxes"); + then.status(200).json_body(json!({ + "items": page_of(2, 0), + "links": [ + { "rel": "next", "href": "/v1/email/mailboxes?page=2&pageSize=100" } + ] + })); + }) + .await; + + let client = client_for(&server); + let (mailboxes, has_more) = + fetch_mailbox_page(&client, None, None, 1, SERVER_PAGE_SIZE_CAP) + .await + .expect("fetch mailboxes"); + + mock.assert_calls_async(1).await; + assert_eq!(mailboxes.len(), 2); + assert!(has_more, "a next link means more even on a short page"); } #[tokio::test] @@ -174,7 +238,7 @@ mod tests { mock.assert_calls_async(1).await; assert_eq!(mailboxes.len(), 10); - assert!(!has_more, "a short page means the list is exhausted"); + assert!(!has_more, "no next link means the list is exhausted"); } #[tokio::test] diff --git a/rust/src/hosting/common.rs b/rust/src/hosting/common.rs index ee0ebcb3..25c12545 100644 --- a/rust/src/hosting/common.rs +++ b/rust/src/hosting/common.rs @@ -289,11 +289,7 @@ pub fn next_link_href(response: &Value) -> Option { /// (`hosting domain list`, `hosting subscription list`). `None` when the /// service reports nothing further. pub fn more_pages_warning(response: &Value, what: &str) -> Option { - next_link_href(response).map(|_| { - format!( - "The API reports more {what} than can be displayed." - ) - }) + next_link_href(response).map(|_| format!("The API reports more {what} than can be displayed.")) } /// Writes `message` as a one-line warning to `out`. @@ -450,7 +446,7 @@ mod tests { }); let warning = more_pages_warning(&more, "domains").expect("next link warns"); assert!(warning.contains("more domains"), "{warning}"); - assert!(warning.contains("incomplete"), "{warning}"); + assert!(warning.contains("than can be displayed"), "{warning}"); let complete = json!({ "items": [], diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 794023aa..61eafcab 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -16,8 +16,8 @@ struct DomainListArgs { #[arg(long = "app-id", value_name = "APP_ID")] app_id: String, - /// Jump directly to an offset in the results. - #[arg(long, value_name = "N", conflicts_with = "continue")] + /// Jump directly to an offset in the results. Ignored when `--continue` is given. + #[arg(long, value_name = "N")] offset: Option, } diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs index 0d3841a8..58536597 100644 --- a/rust/src/pagination_token.rs +++ b/rust/src/pagination_token.rs @@ -66,21 +66,26 @@ fn saturating_i64(value: usize) -> i64 { } /// Resolves the effective `--continue` value for a "known collection" -/// command: an explicit `--offset` wins, synthesized into a bare +/// command. A `--continue` token (which includes a `limit`) is used as-is and +/// wins; otherwise an explicit `--offset` is synthesized into a bare /// `offset:` token (deliberately without a `limit` field — a fresh jump /// has no prior page to inherit a size from, so [`paginate_known_collection`] -/// falls back to the request's own `--limit` when it sees one); otherwise -/// the parsed `--continue` token (which includes a `limit`) is used as-is. Lets -/// a user jump straight to a position without walking there one token at a -/// time, without giving `--offset` and `--continue` two different encodings to +/// falls back to the request's own `--limit` when it sees one). Lets a user +/// jump straight to a position without walking there one token at a time, +/// without giving `--offset` and `--continue` two different encodings to /// keep in sync. +/// +/// The token has to win because the engine's "next page" action replays the +/// command's own arguments, so after `--offset 20` the advertised follow-up is +/// `… --offset 20 --continue `; letting the offset override the token +/// would send the user back to the same page forever. pub(crate) fn resolve_continue_token( explicit_offset: Option, continue_token: Option<&str>, ) -> Option { - explicit_offset - .map(|offset| build(&[("offset", &offset.to_string())])) - .or_else(|| continue_token.map(str::to_owned)) + continue_token + .map(str::to_owned) + .or_else(|| explicit_offset.map(|offset| build(&[("offset", &offset.to_string())]))) } /// Resolves the starting page and page size for a page/pageSize-backed @@ -95,10 +100,12 @@ pub(crate) fn resolve_continue_token( /// way a real forward-only cursor already carries its own page size /// server-side, opaque to the caller. /// -/// An explicit `--page` (a command's own supplemental direct-jump arg, -/// `conflicts_with = "continue"`) always wins for the page instead, and in -/// that case `limit` (the request's own parsed `--limit`) always wins for -/// the size — there's no prior token to inherit a size from. +/// An explicit `--page` (a command's own supplemental direct-jump arg) is +/// used only when there is no `--continue` token, in which case `limit` (the +/// request's own parsed `--limit`) sets the size — there's no prior token to +/// inherit a size from. A token's page wins over `--page` for the same reason +/// as in [`resolve_continue_token`]: the engine's next-page action replays the +/// command's own `--page` alongside the new `--continue`. pub(crate) fn resolve_page_request( explicit_page: Option

, continue_token: Option<&str>, @@ -106,7 +113,8 @@ pub(crate) fn resolve_page_request( limit: i64, ) -> (P, i64) { let fields = parse(continue_token); - let page = explicit_page.unwrap_or_else(|| field(&fields, "page", default_page)); + let token_page = fields.get("page").and_then(|value| value.parse().ok()); + let page = token_page.or(explicit_page).unwrap_or(default_page); let page_size = field(&fields, "limit", limit); (page, page_size) } @@ -190,8 +198,16 @@ mod tests { } #[test] - fn resolve_continue_token_prefers_an_explicit_offset() { - let resolved = resolve_continue_token(Some(20), Some("offset:5")); + fn resolve_continue_token_prefers_a_continue_token_over_an_explicit_offset() { + // The engine's next-page action replays `--offset 20` next to the new + // `--continue`; the token has to win or the user never advances. + let resolved = resolve_continue_token(Some(20), Some("limit:5,offset:25")); + assert_eq!(resolved.as_deref(), Some("limit:5,offset:25")); + } + + #[test] + fn resolve_continue_token_uses_an_explicit_offset_when_there_is_no_token() { + let resolved = resolve_continue_token(Some(20), None); assert_eq!(resolved.as_deref(), Some("offset:20")); } @@ -225,14 +241,25 @@ mod tests { } #[test] - fn resolve_page_request_an_explicit_page_wins_and_uses_the_requests_own_limit() { - // No continue token at all (--page conflicts_with --continue), so - // there's no prior size to inherit — the request's own limit applies. + fn resolve_page_request_an_explicit_page_is_used_without_a_token_and_takes_the_requests_limit() + { + // No continue token, so there's no prior size to inherit — the + // request's own limit applies. let (page, page_size) = resolve_page_request(Some(5u32), None, 1u32, 25); assert_eq!(page, 5); assert_eq!(page_size, 25); } + #[test] + fn resolve_page_request_a_continue_token_wins_over_an_explicit_page() { + // The engine's next-page action replays `--page 5` next to the new + // `--continue`; the token's page has to win or the user never advances. + let token = build(&[("limit", "10"), ("page", "6")]); + let (page, page_size) = resolve_page_request(Some(5u32), Some(&token), 1u32, 25); + assert_eq!(page, 6); + assert_eq!(page_size, 10); + } + fn items(n: usize) -> Vec { (0..n).map(|i| json!({ "id": i })).collect() } diff --git a/rust/src/platform/actions/mod.rs b/rust/src/platform/actions/mod.rs index b172f5f0..2e330d8f 100644 --- a/rust/src/platform/actions/mod.rs +++ b/rust/src/platform/actions/mod.rs @@ -109,9 +109,9 @@ pub fn group() -> RuntimeGroupSpec { .long("offset") .value_name("N") .value_parser(clap::value_parser!(i64).range(0..)) - .conflicts_with("continue") .help( - "Jump directly to an offset in the results", + "Jump directly to an offset in the results. \ + Ignored when `--continue` is given.", ), ), |ctx| async move { @@ -211,6 +211,62 @@ mod tests { assert_eq!(cursor["has_more"], serde_json::json!(false)); } + /// The advertised next-page action replays `--offset` next to the new + /// `--continue`; run exactly as printed it must advance, not fail to parse. + #[tokio::test] + async fn list_next_action_after_an_offset_jump_advances() { + let cli = test_cli(); + let first = cli + .run([ + "gddy", "platform", "actions", "list", "--limit", "3", "--offset", "3", "--output", + "json", + ]) + .await; + assert_eq!(first.exit_code, 0, "{}", first.rendered); + let first: serde_json::Value = + serde_json::from_str(&first.rendered).expect("stdout should contain json"); + let command = first["next_actions"] + .as_array() + .and_then(|actions| { + actions + .iter() + .filter_map(|a| a["command"].as_str()) + .find(|c| c.contains("--continue")) + }) + .expect("a next-page action"); + + let argv: Vec<&str> = command + .split_whitespace() + .map(|part| part.trim_matches('"')) + .chain(["--output", "json"]) + .collect(); + let second = cli.run(argv).await; + assert_eq!(second.exit_code, 0, "{command}: {}", second.rendered); + let second: serde_json::Value = + serde_json::from_str(&second.rendered).expect("stdout should contain json"); + + let names = |page: &serde_json::Value| -> Vec { + page["data"] + .as_array() + .expect("array") + .iter() + .map(|item| item["name"].as_str().unwrap_or_default().to_owned()) + .collect() + }; + let expected: Vec = super::ACTIONS[6..] + .iter() + .map(|(n, _)| (*n).to_owned()) + .collect(); + assert_eq!( + names(&first), + super::ACTIONS[3..6] + .iter() + .map(|(n, _)| (*n).to_owned()) + .collect::>() + ); + assert_eq!(names(&second), expected); + } + #[tokio::test] async fn list_limit_and_continue_page_through_the_catalog() { let cli = test_cli(); From fcd4089b23a52ada624d19ec0031c8a5e09dabe6 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 21 Oct 2026 17:18:44 -0700 Subject: [PATCH 03/10] fix(cli): harden continuation tokens and report incomplete lists - `--continue` tokens escape `%` and `,` in values, so an opaque server page token (domain list) round-trips byte for byte instead of being truncated at an embedded comma. - A page size carried by a `--continue` token is only honored when it is within the command's `1..=max` range; otherwise the request's own validated `--limit` applies. A token can no longer widen a page past the maximum (`limit:-1` no longer returns everything) or send the backend an invalid size. - Hosting app, deployment and log lists report `cursor.total` when the service returns `totalItems`. - `platform app list` warns on stderr when the App Registry reports another page but returns no cursor to fetch it, instead of ending silently. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/api/parameter.rs | 1 + rust/src/api/response.rs | 1 + rust/src/api/search.rs | 7 +- rust/src/dns/list.rs | 1 + rust/src/domain/list.rs | 6 +- rust/src/email/list.rs | 1 + rust/src/hosting/common.rs | 40 +++---- rust/src/hosting/domain/list.rs | 5 +- rust/src/hosting/subscription/list.rs | 3 +- rust/src/pagination_token.rs | 141 ++++++++++++++++++++++--- rust/src/platform/actions/mod.rs | 1 + rust/src/platform/app/commands/list.rs | 54 ++++++++-- 12 files changed, 204 insertions(+), 57 deletions(-) diff --git a/rust/src/api/parameter.rs b/rust/src/api/parameter.rs index 79e83d70..845ed84d 100644 --- a/rust/src/api/parameter.rs +++ b/rust/src/api/parameter.rs @@ -67,6 +67,7 @@ pub(super) fn list_command() -> RuntimeCommandSpec { let (page, continuation) = pagination_token::paginate_known_collection( items, ctx.middleware.cursor_limit, + OPERATION_CHILD_LIST_MAX_LIMIT, continue_token.as_deref(), ); let next_actions = vec![ diff --git a/rust/src/api/response.rs b/rust/src/api/response.rs index 22f26cb3..bfb8efcc 100644 --- a/rust/src/api/response.rs +++ b/rust/src/api/response.rs @@ -59,6 +59,7 @@ pub(super) fn list_command() -> RuntimeCommandSpec { let (page, continuation) = pagination_token::paginate_known_collection( items, ctx.middleware.cursor_limit, + OPERATION_CHILD_LIST_MAX_LIMIT, continue_token.as_deref(), ); let next_actions = vec![ diff --git a/rust/src/api/search.rs b/rust/src/api/search.rs index 53cd1869..3909124d 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -11,6 +11,9 @@ use crate::pagination_token; use super::catalog::{catalog, graphql_sub_endpoint_rows, search_endpoints}; +/// Largest page `api search` returns (its cursor `max_limit`). +const MAX_LIMIT: i64 = 100; + output_schema!(ApiEndpoint { "domain": "string"; "operationId": "string"; @@ -50,13 +53,14 @@ pub(super) fn command() -> RuntimeCommandSpec { .no_auth(true) .with_default_fields("operationId,domain,method,path,summary") .with_output_schema::() - .with_cursor(CursorConfig::new(50, 100)), + .with_cursor(CursorConfig::new(50, MAX_LIMIT)), |ctx, args: SearchArgs| async move { let hits = search_endpoints(catalog(), &args.query); if hits.is_empty() { let (page, continuation) = pagination_token::paginate_known_collection( Vec::new(), ctx.middleware.cursor_limit, + MAX_LIMIT, None, ); return Ok(CommandResult::new(json!(page)).with_cursor(continuation)); @@ -130,6 +134,7 @@ pub(super) fn command() -> RuntimeCommandSpec { let (page, continuation) = pagination_token::paginate_known_collection( results, ctx.middleware.cursor_limit, + MAX_LIMIT, continue_token.as_deref(), ); Ok(CommandResult::new(json!(page)) diff --git a/rust/src/dns/list.rs b/rust/src/dns/list.rs index b473ec69..ab28a4ce 100644 --- a/rust/src/dns/list.rs +++ b/rust/src/dns/list.rs @@ -86,6 +86,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.continue_token.as_deref(), 1u64, ctx.middleware.cursor_limit, + MAX_PAGE_SIZE.get() as i64, ); let fetched = fetch_records_page( &client, diff --git a/rust/src/domain/list.rs b/rust/src/domain/list.rs index cf4f96d4..e9e722a4 100644 --- a/rust/src/domain/list.rs +++ b/rust/src/domain/list.rs @@ -199,8 +199,10 @@ async fn fetch_domain_page( /// and reads back from `--continue` (see [`decode_resume`]). fn encode_resume(token: &str, direction: Option) -> String { match direction { - Some(direction) => format!("token:{token},direction:{direction}"), - None => format!("token:{token}"), + Some(direction) => { + pagination_token::build(&[("token", token), ("direction", &direction.to_string())]) + } + None => pagination_token::build(&[("token", token)]), } } diff --git a/rust/src/email/list.rs b/rust/src/email/list.rs index a6ec0d8e..21c6d127 100644 --- a/rust/src/email/list.rs +++ b/rust/src/email/list.rs @@ -42,6 +42,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.continue_token.as_deref(), 1u32, ctx.middleware.cursor_limit, + SERVER_PAGE_SIZE_CAP as i64, ); let page_size = usize::try_from(page_size).unwrap_or(SERVER_PAGE_SIZE_CAP); let (mailboxes, has_more) = fetch_mailbox_page( diff --git a/rust/src/hosting/common.rs b/rust/src/hosting/common.rs index 25c12545..c1428049 100644 --- a/rust/src/hosting/common.rs +++ b/rust/src/hosting/common.rs @@ -292,21 +292,6 @@ pub fn more_pages_warning(response: &Value, what: &str) -> Option { next_link_href(response).map(|_| format!("The API reports more {what} than can be displayed.")) } -/// Writes `message` as a one-line warning to `out`. -fn write_warning(out: &mut impl std::io::Write, message: &str) { - let _ = writeln!(out, "warning: {message}"); -} - -/// Shows a non-fatal warning to the user on stderr, keeping stdout's JSON/TOON -/// payload clean. -/// -/// Interim: the engine has no result-level warning channel yet. Replace calls -/// with `CommandResult::with_warning` once cli-engine#131 is released and -/// adopted. -pub fn warn_user(message: &str) { - write_warning(&mut std::io::stderr(), message); -} - /// The `items` array of a paged hosting response (empty when absent). pub fn page_items(response: &Value) -> Vec { response @@ -319,9 +304,15 @@ pub fn page_items(response: &Value) -> Vec { /// Maps a paged hosting response's `links[rel=next]` onto the engine's cursor /// continuation, using the server's own `pageToken` as the `--continue` token. pub fn page_continuation(response: &Value) -> CursorContinuation { - match next_page_token(response) { + let continuation = match next_page_token(response) { Some(token) => CursorContinuation::more(token), None => CursorContinuation::done(), + }; + // `totalItems` is only present when the service was asked for it + // (`totalRequired`); leave the total absent otherwise. + match response.get("totalItems").and_then(Value::as_i64) { + Some(total) => continuation.with_total(total), + None => continuation, } } @@ -456,16 +447,6 @@ mod tests { assert_eq!(more_pages_warning(&json!({ "items": [] }), "domains"), None); } - #[test] - fn write_warning_emits_one_prefixed_line() { - let mut out = Vec::new(); - write_warning(&mut out, "list may be incomplete"); - assert_eq!( - String::from_utf8(out).expect("utf8"), - "warning: list may be incomplete\n" - ); - } - #[test] fn page_items_returns_the_items_array_or_empty() { assert_eq!( @@ -506,6 +487,13 @@ mod tests { assert_eq!(next_link_href(&json!({ "items": [] })), None); } + #[test] + fn page_continuation_carries_total_items_only_when_the_service_sends_it() { + let with_total = page_continuation(&json!({ "items": [], "totalItems": 42 })); + assert_eq!(with_total.total, Some(42)); + assert_eq!(page_continuation(&json!({ "items": [] })).total, None); + } + #[test] fn page_limit_rejects_non_positive_and_oversized_values() { assert_eq!(page_limit(50), Some(50)); diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 61eafcab..5a82e725 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -4,10 +4,10 @@ use cli_engine::{ use serde_json::json; use crate::hosting::common::{ - HostingDomainSummary, client_err, make_client, more_pages_warning, page_items, warn_user, + HostingDomainSummary, client_err, make_client, more_pages_warning, page_items, }; use crate::next_action::next_action; -use crate::pagination_token; +use crate::pagination_token::{self, warn_user}; use crate::scopes::HOSTING_DOMAIN_READ as DOMAIN_READ; #[derive(Debug, Clone, clap::Args)] @@ -52,6 +52,7 @@ pub(super) fn command() -> RuntimeCommandSpec { let (page, continuation) = pagination_token::paginate_known_collection( page_items(&response), ctx.middleware.cursor_limit, + 200, continue_token.as_deref(), ); diff --git a/rust/src/hosting/subscription/list.rs b/rust/src/hosting/subscription/list.rs index 094a680a..aa65f413 100644 --- a/rust/src/hosting/subscription/list.rs +++ b/rust/src/hosting/subscription/list.rs @@ -6,9 +6,10 @@ use serde_json::{Value, json}; use crate::hosting::common::{ HostingProduct, HostingSubscriptionList, client_err, make_client, more_pages_warning, - page_items, warn_user, + page_items, }; use crate::next_action::next_action; +use crate::pagination_token::warn_user; use crate::scopes::HOSTING_SUBSCRIPTION_READ as SUB_READ; #[derive(Debug, Clone, clap::Args)] diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs index 58536597..8110c523 100644 --- a/rust/src/pagination_token.rs +++ b/rust/src/pagination_token.rs @@ -33,10 +33,24 @@ pub(crate) fn parse(token: Option<&str>) -> HashMap { token .split(',') .filter_map(|pair| pair.split_once(':')) - .map(|(key, value)| (key.to_owned(), value.to_owned())) + .map(|(key, value)| (key.to_owned(), unescape(value))) .collect() } +/// Escapes the two characters that would corrupt a `key:value,key:value` +/// token — `%` (the escape itself) and `,` (the pair separator) — so an opaque +/// value, such as a server's page token, survives [`build`] then [`parse`] +/// byte for byte. +fn escape(value: &str) -> String { + value.replace('%', "%25").replace(',', "%2C") +} + +/// Inverse of [`escape`]. `%2C` must be decoded before `%25`, so that an +/// original `%2C` (escaped to `%252C`) is not turned into a comma. +fn unescape(value: &str) -> String { + value.replace("%2C", ",").replace("%25", "%") +} + /// Reads a typed field out of a [`parse`]d map, falling back to `default` /// when the key is absent or its value doesn't parse as `T` — the same /// "degrade to start over" behavior `parse` itself uses for a malformed @@ -52,7 +66,7 @@ pub(crate) fn field(fields: &HashMap, key: &str, def pub(crate) fn build(pairs: &[(&str, &str)]) -> String { pairs .iter() - .map(|(key, value)| format!("{key}:{value}")) + .map(|(key, value)| format!("{key}:{}", escape(value))) .collect::>() .join(",") } @@ -111,24 +125,40 @@ pub(crate) fn resolve_page_request( continue_token: Option<&str>, default_page: P, limit: i64, + max_limit: i64, ) -> (P, i64) { let fields = parse(continue_token); let token_page = fields.get("page").and_then(|value| value.parse().ok()); let page = token_page.or(explicit_page).unwrap_or(default_page); - let page_size = field(&fields, "limit", limit); + let page_size = inherited_limit(&fields, limit, max_limit); (page, page_size) } +/// The page size a `--continue` token carries, if it is within the command's +/// `1..=max_limit` range; otherwise the request's own already-validated +/// `--limit`. A hand-edited token must not widen a page past the command's +/// maximum (`limit:-1` would otherwise mean "everything") or send the backend +/// a size it rejects. +fn inherited_limit(fields: &HashMap, request_limit: i64, max_limit: i64) -> i64 { + let inherited = field(fields, "limit", request_limit); + if (1..=max_limit).contains(&inherited) { + inherited + } else { + request_limit + } +} + /// Slices an already-fully-known collection using a `limit:,offset:` /// continuation token. pub(crate) fn paginate_known_collection( items: Vec, limit: i64, + max_limit: i64, continue_token: Option<&str>, ) -> (Vec, CursorContinuation) { let fields = parse(continue_token); let offset = field(&fields, "offset", 0usize); - let limit = field(&fields, "limit", limit); + let limit = inherited_limit(&fields, limit, max_limit); let limit = saturating_usize(limit).max(1); let total = items.len(); let start = offset.min(total); @@ -151,6 +181,21 @@ pub(crate) fn paginate_known_collection( (page, continuation) } +/// Writes `message` as a one-line warning to `out`. +fn write_warning(out: &mut impl std::io::Write, message: &str) { + let _ = writeln!(out, "warning: {message}"); +} + +/// Shows a non-fatal warning about an incomplete listing to the user on +/// stderr, keeping stdout's JSON/TOON payload clean. +/// +/// Interim: the engine has no result-level warning channel yet. Replace calls +/// with `CommandResult::with_warning` once cli-engine#131 is released and +/// adopted. +pub(crate) fn warn_user(message: &str) { + write_warning(&mut std::io::stderr(), message); +} + #[cfg(test)] mod tests { use serde_json::json; @@ -224,7 +269,7 @@ mod tests { #[test] fn resolve_page_request_with_no_input_defaults_the_page_and_uses_the_given_limit() { - let (page, page_size) = resolve_page_request(None::, None, 1u32, 25); + let (page, page_size) = resolve_page_request(None::, None, 1u32, 25, 100); assert_eq!(page, 1); assert_eq!(page_size, 25); } @@ -235,7 +280,7 @@ mod tests { // this call's own --limit defaulted to something else (25) — page 2 // of size-10 pages and page 2 of size-25 pages cover different items. let token = build(&[("limit", "10"), ("page", "2")]); - let (page, page_size) = resolve_page_request(None::, Some(&token), 1u32, 25); + let (page, page_size) = resolve_page_request(None::, Some(&token), 1u32, 25, 100); assert_eq!(page, 2); assert_eq!(page_size, 10); } @@ -245,7 +290,7 @@ mod tests { { // No continue token, so there's no prior size to inherit — the // request's own limit applies. - let (page, page_size) = resolve_page_request(Some(5u32), None, 1u32, 25); + let (page, page_size) = resolve_page_request(Some(5u32), None, 1u32, 25, 100); assert_eq!(page, 5); assert_eq!(page_size, 25); } @@ -255,7 +300,7 @@ mod tests { // The engine's next-page action replays `--page 5` next to the new // `--continue`; the token's page has to win or the user never advances. let token = build(&[("limit", "10"), ("page", "6")]); - let (page, page_size) = resolve_page_request(Some(5u32), Some(&token), 1u32, 25); + let (page, page_size) = resolve_page_request(Some(5u32), Some(&token), 1u32, 25, 100); assert_eq!(page, 6); assert_eq!(page_size, 10); } @@ -266,7 +311,7 @@ mod tests { #[test] fn paginate_known_collection_returns_the_first_page_with_no_token() { - let (page, cont) = paginate_known_collection(items(5), 2, None); + let (page, cont) = paginate_known_collection(items(5), 2, 100, None); assert_eq!(page, vec![json!({"id": 0}), json!({"id": 1})]); assert!(cont.continue_from.is_some()); assert_eq!(cont.total, Some(5)); @@ -275,9 +320,9 @@ mod tests { #[test] fn paginate_known_collection_resumes_from_the_continue_token() { - let (_, cont) = paginate_known_collection(items(5), 2, None); + let (_, cont) = paginate_known_collection(items(5), 2, 100, None); let token = cont.continue_from.expect("has_more implies a token"); - let (page, cont2) = paginate_known_collection(items(5), 2, Some(&token)); + let (page, cont2) = paginate_known_collection(items(5), 2, 100, Some(&token)); assert_eq!(page, vec![json!({"id": 2}), json!({"id": 3})]); assert!(cont2.continue_from.is_some()); assert_eq!(cont2.remaining, Some(1)); @@ -285,14 +330,14 @@ mod tests { #[test] fn paginate_known_collection_continue_token_embeds_and_inherits_the_limit() { - let (_, cont) = paginate_known_collection(items(5), 2, None); + let (_, cont) = paginate_known_collection(items(5), 2, 100, None); let token = cont.continue_from.expect("has_more implies a token"); assert_eq!(token, "limit:2,offset:2"); // A caller that resumes with --continue alone, and whose --limit // defaulted to something else entirely (25) this call, must still // get the size (2) that produced the token, not the new default. - let (page, cont2) = paginate_known_collection(items(5), 25, Some(&token)); + let (page, cont2) = paginate_known_collection(items(5), 25, 100, Some(&token)); assert_eq!(page, vec![json!({"id": 2}), json!({"id": 3})]); assert_eq!(cont2.remaining, Some(1)); } @@ -303,7 +348,7 @@ mod tests { // `limit`) for an explicit --offset — the request's own limit // applies, same as a fresh, tokenless call. let token = resolve_continue_token(Some(2), None).expect("explicit offset"); - let (page, _) = paginate_known_collection(items(5), 3, Some(&token)); + let (page, _) = paginate_known_collection(items(5), 3, 100, Some(&token)); assert_eq!( page, vec![json!({"id": 2}), json!({"id": 3}), json!({"id": 4})] @@ -312,7 +357,7 @@ mod tests { #[test] fn paginate_known_collection_reaches_the_end_with_no_further_token() { - let (page, cont) = paginate_known_collection(items(5), 2, Some("offset:4")); + let (page, cont) = paginate_known_collection(items(5), 2, 100, Some("offset:4")); assert_eq!(page, vec![json!({"id": 4})]); assert!(cont.continue_from.is_none()); assert_eq!(cont.remaining, Some(0)); @@ -320,7 +365,7 @@ mod tests { #[test] fn paginate_known_collection_past_the_end_returns_empty() { - let (page, cont) = paginate_known_collection(items(3), 2, Some("offset:10")); + let (page, cont) = paginate_known_collection(items(3), 2, 100, Some("offset:10")); assert!(page.is_empty()); assert!(cont.continue_from.is_none()); assert_eq!(cont.remaining, Some(0)); @@ -328,9 +373,71 @@ mod tests { #[test] fn paginate_known_collection_of_an_empty_collection() { - let (page, cont) = paginate_known_collection(Vec::new(), 10, None); + let (page, cont) = paginate_known_collection(Vec::new(), 10, 100, None); assert!(page.is_empty()); assert!(cont.continue_from.is_none()); assert_eq!(cont.total, Some(0)); } + + #[test] + fn write_warning_emits_one_prefixed_line() { + let mut out = Vec::new(); + write_warning(&mut out, "list may be incomplete"); + assert_eq!( + String::from_utf8(out).expect("utf8"), + "warning: list may be incomplete\n" + ); + } + + #[test] + fn build_and_parse_round_trip_opaque_values_byte_for_byte() { + for opaque in [ + "abc,def", "100%", "a+b c", "%2C", "%252C", ",,%,%25,", "x:y,z", + ] { + let token = build(&[("token", opaque), ("direction", "forward")]); + let fields = parse(Some(&token)); + assert_eq!( + fields.get("token").map(String::as_str), + Some(opaque), + "{token}" + ); + assert_eq!(fields.get("direction").map(String::as_str), Some("forward")); + } + } + + #[test] + fn a_comma_in_a_value_does_not_split_the_token() { + assert_eq!(build(&[("token", "abc,def")]), "token:abc%2Cdef"); + } + + #[test] + fn paginate_known_collection_ignores_an_out_of_range_token_limit() { + // Negative would otherwise convert to usize::MAX and return everything; + // zero and oversized values fall back to the request's own limit. + for bad in [ + "limit:-1,offset:0", + "limit:0,offset:0", + "limit:101,offset:0", + ] { + let (page, _) = paginate_known_collection(items(300), 7, 100, Some(bad)); + assert_eq!(page.len(), 7, "{bad}"); + } + let (page, _) = paginate_known_collection(items(300), 7, 100, Some("limit:100,offset:0")); + assert_eq!( + page.len(), + 100, + "an in-range token limit is still inherited" + ); + } + + #[test] + fn resolve_page_request_ignores_an_out_of_range_token_limit() { + for bad in ["limit:-1,page:2", "limit:0,page:2", "limit:101,page:2"] { + let (page, page_size) = resolve_page_request(None::, Some(bad), 1u32, 25, 100); + assert_eq!((page, page_size), (2, 25), "{bad}"); + } + let (_, page_size) = + resolve_page_request(None::, Some("limit:100,page:2"), 1u32, 25, 100); + assert_eq!(page_size, 100); + } } diff --git a/rust/src/platform/actions/mod.rs b/rust/src/platform/actions/mod.rs index 2e330d8f..bd1746fa 100644 --- a/rust/src/platform/actions/mod.rs +++ b/rust/src/platform/actions/mod.rs @@ -127,6 +127,7 @@ pub fn group() -> RuntimeGroupSpec { let (page, continuation) = pagination_token::paginate_known_collection( actions, ctx.middleware.cursor_limit, + 200, continue_token.as_deref(), ); Ok(CommandResult::new(json!(page)) diff --git a/rust/src/platform/app/commands/list.rs b/rust/src/platform/app/commands/list.rs index 8cc6d4bb..e82cc84c 100644 --- a/rust/src/platform/app/commands/list.rs +++ b/rust/src/platform/app/commands/list.rs @@ -7,6 +7,7 @@ use cli_engine::{ use super::schemas::ApplicationSummary; use crate::next_action::next_action; +use crate::pagination_token; /// Pages through `applications(first:, after:)` with the Relay `endCursor` as /// the `--continue` token. No supplemental `--offset` (unlike the client-side @@ -35,13 +36,15 @@ pub(super) fn command() -> RuntimeCommandSpec { ) .await .map_err(super::client_err)?; - let continuation = match (page.has_next_page, page.end_cursor) { - (true, Some(cursor)) => CursorContinuation::more(cursor), - // `hasNextPage: true` with no `endCursor` would be a - // malformed response — treat it the same as "done" rather - // than fabricating a token that can't actually resume. - _ => CursorContinuation::done(), - }; + let (continuation, missing_cursor) = + continuation_for(page.has_next_page, page.end_cursor); + if missing_cursor { + pagination_token::warn_user( + "The App Registry reports more applications than this response \ + contains but returned no cursor to fetch them; the list may be \ + incomplete.", + ); + } let data = serde_json::to_value(page.items).map_err(|e| { crate::error::GddyError::unexpected(format!("failed to encode applications: {e}")) .into_cli_error() @@ -63,11 +66,46 @@ pub(super) fn command() -> RuntimeCommandSpec { ) } +/// The continuation for one Relay page, plus whether the page is truncated +/// without a way to resume: `hasNextPage: true` with no `endCursor` is a +/// malformed response, so the listing ends there but the caller should say +/// it may be incomplete rather than letting it look complete. +fn continuation_for(has_next_page: bool, end_cursor: Option) -> (CursorContinuation, bool) { + match (has_next_page, end_cursor) { + (true, Some(cursor)) => (CursorContinuation::more(cursor), false), + (true, None) => (CursorContinuation::done(), true), + (false, _) => (CursorContinuation::done(), false), + } +} + #[cfg(test)] mod tests { use cli_engine::{Cli, CliConfig, CursorConfig, Stage}; - use super::command; + use super::{command, continuation_for}; + + #[test] + fn a_next_page_with_a_cursor_continues() { + let (continuation, missing) = continuation_for(true, Some("cursor-2".to_owned())); + assert_eq!(continuation.continue_from.as_deref(), Some("cursor-2")); + assert!(!missing); + } + + #[test] + fn a_next_page_without_a_cursor_ends_but_is_flagged_as_incomplete() { + let (continuation, missing) = continuation_for(true, None); + assert_eq!(continuation.continue_from, None); + assert!(missing, "must warn instead of looking complete"); + } + + #[test] + fn the_last_page_is_done_and_not_flagged() { + for cursor in [None, Some("stale".to_owned())] { + let (continuation, missing) = continuation_for(false, cursor); + assert_eq!(continuation.continue_from, None); + assert!(!missing); + } + } use crate::platform::app::client::APPLICATION_STATUSES; #[test] From c0ea91e834450c8721addce9095d617addb8eddd Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 21 Oct 2026 17:27:44 -0700 Subject: [PATCH 04/10] fix(cli): apply --filter before paging client-side collections Commands that hold their whole collection (platform actions list, hosting domain list, api parameter list, api response list, api search) now apply the global `--filter` before slicing, using the engine's own filter. An offset jump selects among the matching rows, `cursor.total` and `cursor.remaining` count only those, and an invalid filter expression is reported instead of ignored. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/api/parameter.rs | 2 +- rust/src/api/response.rs | 2 +- rust/src/api/search.rs | 2 +- rust/src/hosting/domain/list.rs | 5 +- rust/src/pagination_token.rs | 60 +++++++++++++++++++++++ rust/src/platform/actions/mod.rs | 83 +++++++++++++++++++++++++++++++- 6 files changed, 149 insertions(+), 5 deletions(-) diff --git a/rust/src/api/parameter.rs b/rust/src/api/parameter.rs index 845ed84d..c90fbdae 100644 --- a/rust/src/api/parameter.rs +++ b/rust/src/api/parameter.rs @@ -65,7 +65,7 @@ pub(super) fn list_command() -> RuntimeCommandSpec { ctx.middleware.continue_token.as_deref(), ); let (page, continuation) = pagination_token::paginate_known_collection( - items, + pagination_token::filter_known_collection(items, &ctx.middleware.filter)?, ctx.middleware.cursor_limit, OPERATION_CHILD_LIST_MAX_LIMIT, continue_token.as_deref(), diff --git a/rust/src/api/response.rs b/rust/src/api/response.rs index bfb8efcc..26664c7b 100644 --- a/rust/src/api/response.rs +++ b/rust/src/api/response.rs @@ -57,7 +57,7 @@ pub(super) fn list_command() -> RuntimeCommandSpec { ctx.middleware.continue_token.as_deref(), ); let (page, continuation) = pagination_token::paginate_known_collection( - items, + pagination_token::filter_known_collection(items, &ctx.middleware.filter)?, ctx.middleware.cursor_limit, OPERATION_CHILD_LIST_MAX_LIMIT, continue_token.as_deref(), diff --git a/rust/src/api/search.rs b/rust/src/api/search.rs index 3909124d..80a373af 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -132,7 +132,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.continue_token.as_deref(), ); let (page, continuation) = pagination_token::paginate_known_collection( - results, + pagination_token::filter_known_collection(results, &ctx.middleware.filter)?, ctx.middleware.cursor_limit, MAX_LIMIT, continue_token.as_deref(), diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 5a82e725..059edede 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -50,7 +50,10 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.continue_token.as_deref(), ); let (page, continuation) = pagination_token::paginate_known_collection( - page_items(&response), + pagination_token::filter_known_collection( + page_items(&response), + &ctx.middleware.filter, + )?, ctx.middleware.cursor_limit, 200, continue_token.as_deref(), diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs index 8110c523..9c9383b3 100644 --- a/rust/src/pagination_token.rs +++ b/rust/src/pagination_token.rs @@ -15,6 +15,8 @@ use std::collections::HashMap; use std::str::FromStr; +use cli_engine::{PipelineOpts, apply_pipeline}; + use cli_engine::CursorContinuation; use serde_json::Value; @@ -148,6 +150,38 @@ fn inherited_limit(fields: &HashMap, request_limit: i64, max_lim } } +/// Applies the global `--filter` to an already-fully-known collection +/// *before* it is sliced. +/// +/// The engine runs `--filter` after the handler returns, so for a cursor +/// command it would filter the page it was handed — pages of a filtered +/// collection would come out short, offsets would select different rows than +/// they did when the engine paginated after filtering, and `cursor.total` +/// would count unfiltered rows. A command that holds the whole collection +/// filters it here first, with the engine's own filter, and propagates its +/// invalid-expression error. The engine's later pass over the returned page is +/// harmless: a per-item predicate keeps what it already kept. +pub(crate) fn filter_known_collection( + items: Vec, + filter: &str, +) -> cli_engine::Result> { + if filter.is_empty() { + return Ok(items); + } + let mut data = Value::Array(items); + apply_pipeline( + &mut data, + &PipelineOpts { + filter: filter.to_owned(), + ..PipelineOpts::default() + }, + )?; + Ok(match data { + Value::Array(items) => items, + _ => Vec::new(), + }) +} + /// Slices an already-fully-known collection using a `limit:,offset:` /// continuation token. pub(crate) fn paginate_known_collection( @@ -379,6 +413,32 @@ mod tests { assert_eq!(cont.total, Some(0)); } + #[test] + fn filter_known_collection_keeps_only_matching_items_in_order() { + let all = vec![ + json!({ "name": "a.one" }), + json!({ "name": "b.two" }), + json!({ "name": "a.three" }), + ]; + let kept = + filter_known_collection(all.clone(), "starts_with(name, 'a.')").expect("filters"); + assert_eq!( + kept, + vec![json!({ "name": "a.one" }), json!({ "name": "a.three" })] + ); + assert_eq!( + filter_known_collection(all.clone(), "").expect("no filter"), + all + ); + } + + #[test] + fn filter_known_collection_propagates_an_invalid_expression() { + let err = filter_known_collection(vec![json!({ "name": "x" })], "starts_with(((") + .expect_err("an invalid filter must surface, not be swallowed"); + assert!(!err.to_string().is_empty()); + } + #[test] fn write_warning_emits_one_prefixed_line() { let mut out = Vec::new(); diff --git a/rust/src/platform/actions/mod.rs b/rust/src/platform/actions/mod.rs index bd1746fa..b61a4858 100644 --- a/rust/src/platform/actions/mod.rs +++ b/rust/src/platform/actions/mod.rs @@ -125,7 +125,7 @@ pub fn group() -> RuntimeGroupSpec { ctx.middleware.continue_token.as_deref(), ); let (page, continuation) = pagination_token::paginate_known_collection( - actions, + pagination_token::filter_known_collection(actions, &ctx.middleware.filter)?, ctx.middleware.cursor_limit, 200, continue_token.as_deref(), @@ -212,6 +212,87 @@ mod tests { assert_eq!(cursor["has_more"], serde_json::json!(false)); } + /// `--filter` has to apply to the whole collection before it is sliced: + /// the offset jump selects among the *matching* rows, `cursor.total` counts + /// only those, and the replayed next-page action keeps the filter. + #[tokio::test] + async fn list_filter_applies_before_an_offset_jump_and_continuation() { + let matching: Vec<&str> = super::ACTIONS + .iter() + .map(|(name, _)| *name) + .filter(|name| name.starts_with("commerce.")) + .collect(); + assert!( + matching.len() >= 3, + "fixture needs several commerce.* actions" + ); + assert!( + matching.len() < super::ACTIONS.len(), + "filter must exclude something" + ); + + let cli = test_cli(); + let first = cli + .run([ + "gddy", + "platform", + "actions", + "list", + "--filter", + "starts_with(name, 'commerce.')", + "--offset", + "1", + "--limit", + "1", + "--output", + "json", + ]) + .await; + assert_eq!(first.exit_code, 0, "{}", first.rendered); + let first: serde_json::Value = + serde_json::from_str(&first.rendered).expect("stdout should contain json"); + assert_eq!(first["data"].as_array().expect("array").len(), 1); + assert_eq!(first["data"][0]["name"], matching[1]); + assert_eq!(first["cursor"]["total"], matching.len()); + assert_eq!(first["cursor"]["remaining"], matching.len() - 2); + + let command = first["next_actions"] + .as_array() + .and_then(|actions| { + actions + .iter() + .filter_map(|a| a["command"].as_str()) + .find(|c| c.contains("--continue")) + }) + .expect("a next-page action"); + assert!( + command.contains("--filter"), + "replay keeps the filter: {command}" + ); + // The replay keeps the filter expression as one quoted token containing spaces, + // so rebuild argv around it rather than splitting on whitespace. + let (before, after) = command.split_once("--filter").expect("has --filter"); + let expr_start = after + .find('"') + .or_else(|| after.find('\'')) + .expect("quoted expr"); + let quote = after.as_bytes()[expr_start] as char; + let expr_end = after[expr_start + 1..].find(quote).expect("closing quote") + expr_start + 1; + let expr = &after[expr_start + 1..expr_end]; + let rest = &after[expr_end + 1..]; + let argv: Vec<&str> = before + .split_whitespace() + .chain(["--filter", expr]) + .chain(rest.split_whitespace().map(|part| part.trim_matches('"'))) + .chain(["--output", "json"]) + .collect(); + let second = cli.run(argv).await; + assert_eq!(second.exit_code, 0, "{command}: {}", second.rendered); + let second: serde_json::Value = + serde_json::from_str(&second.rendered).expect("stdout should contain json"); + assert_eq!(second["data"][0]["name"], matching[2]); + } + /// The advertised next-page action replays `--offset` next to the new /// `--continue`; run exactly as printed it must advance, not fail to parse. #[tokio::test] From 70d700c8826b562cd306739df57d530829380d59 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 22 Oct 2026 15:44:37 -0700 Subject: [PATCH 05/10] fix(hosting): read every page of the domain and subscription lists `hosting domain list` and `hosting subscription list` call endpoints that do not accept paging parameters today (the service rejects them) but are specified to follow the API collections-and-pagination standard, so a `next` link can appear. Both now gather the whole collection, following `next` links by replaying the link's query on the same path, host and credentials (at most 20 pages), and then work from that complete list. - `hosting domain list` pages the list client-side with `--limit`, `--offset` and `--continue`, and stops reading once it holds enough for the requested window. Totals are left out when it stopped before the end. `--filter` applies to the whole list first. - `hosting subscription list` always reads every page, so slot totals and the suggested plan cover all plans, and keeps its existing output shape. - A link that cannot be followed ends the listing and prints a warning to stderr instead of failing. - The App Registry warning no longer names an internal service. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/hosting/client.rs | 169 +++++++++++- rust/src/hosting/client_tests.rs | 363 ++++++++++++++++++++++++- rust/src/hosting/common.rs | 72 +++-- rust/src/hosting/domain/list.rs | 44 ++- rust/src/hosting/subscription/list.rs | 11 +- rust/src/pagination_token.rs | 51 ++++ rust/src/platform/app/commands/list.rs | 5 +- 7 files changed, 661 insertions(+), 54 deletions(-) diff --git a/rust/src/hosting/client.rs b/rust/src/hosting/client.rs index b157c1e7..22e8dc2f 100644 --- a/rust/src/hosting/client.rs +++ b/rust/src/hosting/client.rs @@ -1,6 +1,6 @@ use hosting_client::types::{ AppType, AttachDomainRequest, AttachSubscriptionRequest, CreateAppRequest, Environment, - HostingProduct, ImportGitHubSourceRequest, RestartRequest, + HostingProduct, ImportGitHubSourceRequest, PagedDomains, PagedSubscriptions, RestartRequest, }; use reqwest::{Client, Method}; use serde_json::{Value, json}; @@ -9,6 +9,23 @@ use crate::http::{make_api_upload_http_client, make_http_client}; const BASE_PATH: &str = "/v1/hosting"; +/// Upper bound on pages combined into one aggregate listing. No real account +/// approaches it; it only stops a misbehaving service from turning a list +/// command into an endless crawl. +const MAX_COMBINED_PAGES: usize = 20; + +/// The items gathered from a paged listing, and why any pages were left unread. +pub struct CombinedPages { + pub items: Vec, + /// The service reported another page that could not be read (a link with + /// nothing to replay, a repeated link, or the page limit). Worth a warning. + pub incomplete: bool, + /// Reading stopped on purpose, because the caller's window was already + /// covered, while the service still had more. The collection is therefore + /// not the whole of it, so its length is not a total. + pub stopped_early: bool, +} + #[derive(Debug, thiserror::Error)] pub enum ClientError { #[error("HTTP error {status}: {body}")] @@ -139,6 +156,13 @@ impl HostingClient { let request = req .build() .map_err(|e| ClientError::Network(e.to_string()))?; + self.execute_json(request).await + } + + /// Sends an already-built request and decodes the JSON response, with the + /// transport debug trace and the non-2xx → [`ClientError::Http`] mapping + /// the hand-built requests here share. + async fn execute_json(&self, request: reqwest::Request) -> Result { cli_engine::transport::debug_log_reqwest_request(&request); let resp = self .http @@ -169,6 +193,32 @@ impl HostingClient { serde_json::from_slice(&bytes).map_err(ClientError::Response) } + /// GET `path` with the paging parameters a collection's `links[rel=next]` + /// carried (`query` is that link's query string, e.g. + /// `pageToken=abc&pageSize=50`). + /// + /// For listings whose spec declares no paging request parameters, so the + /// generated builders cannot send them. Only the *query* of the server's + /// link is replayed; the path and host stay ours, so the bearer token only + /// ever goes to the configured API. Replace with the generated builders once + /// the spec declares the parameters (DEVEX-1340). + async fn get_with_query(&self, path: &str, query: &str) -> Result { + // Set the query as given rather than decoding and re-encoding it: a + // cursor is opaque, and a form decode would turn a literal `+` into a + // space. + let mut url = reqwest::Url::parse(&self.url(path)) + .map_err(|e| ClientError::Request(format!("invalid request URL: {e}")))?; + url.set_query(Some(query)); + let request = self + .http + .request(Method::GET, url) + .bearer_auth(&self.token) + .header("x-request-id", Self::new_request_id()) + .build() + .map_err(|e| ClientError::Network(e.to_string()))?; + self.execute_json(request).await + } + // Spec has no request body; Akamai still 411s a POST with no Content-Length. async fn post_empty_json(&self, path: &str) -> Result { self.post_empty_json_inner(path, true).await @@ -549,14 +599,27 @@ impl HostingClient { response(client.get_runtime().app_id(app_id).send().await).await } - /// One request, no paging parameters: the spec types this response as - /// paged (`items`/`links`) but declares no `pageToken`/`pageSize`, and the - /// service rejects them on the sibling `GET /subscriptions` with a 400 - /// ("Unrecognized key"). Callers use [`super::common::more_pages_warning`] - /// to tell the user when the service reports more than this page. - pub async fn list_domains(&self, app_id: &str) -> Result { + /// Every attached domain, combining pages (see [`Self::collect_pages`]). + /// The spec types the response as paged (`items`/`links`) but declares no + /// `pageToken`/`pageSize`, and the service rejects them on the sibling + /// `GET /subscriptions` with a 400 ("Unrecognized key"), so the first request + /// carries no paging parameters. + /// + /// `want` is how many items the caller needs; once that many are held, later + /// pages are not read (see [`CombinedPages::stopped_early`]). + pub async fn list_domains( + &self, + app_id: &str, + want: Option, + ) -> Result { let client = self.api()?; - response(client.list_domains().app_id(app_id).send().await).await + let first = response(client.list_domains().app_id(app_id).send().await).await?; + let path = format!( + "/apps/{}/domains", + crate::api::http::encode_path_segment(app_id) + ); + self.collect_pages::(&path, &[], first, want) + .await } pub async fn get_domain(&self, app_id: &str, domain_id: &str) -> Result { @@ -601,13 +664,97 @@ impl HostingClient { .await } - /// One request, no paging parameters — see [`Self::list_domains`]. - pub async fn list_subscriptions(&self, hosting_product: &str) -> Result { + /// Every subscription, combining pages (see [`Self::collect_pages`]). The + /// command reports slot totals and suggests a plan to attach, both only + /// correct over *all* plans. The service rejects `pageSize`/`pageToken` here + /// with a 400 today, so the first request carries only the required + /// `hostingProduct`. + pub async fn list_subscriptions( + &self, + hosting_product: &str, + ) -> Result { let client = self.api()?; let request = client .list_subscriptions() .hosting_product(HostingProduct::from(hosting_product.to_owned())); - response(request.send().await).await + let first = response(request.send().await).await?; + // Always every page: slot totals and the plan to suggest are only + // correct over all plans, whatever `--limit` shows. + self.collect_pages::( + "/subscriptions", + &[("hostingProduct", hosting_product)], + first, + None, + ) + .await + } + + /// Gathers a collection the service serves in pages, so a command can slice + /// the whole of it client-side. Until the service really paginates these + /// endpoints it returns everything in one page, so this is one request; when + /// it starts sending `links[rel=next]` (per the collections-and-pagination + /// standard) each page is read in turn, up to [`MAX_COMBINED_PAGES`]. + /// + /// Later pages replay the query of the previous page's next link + /// ([`Self::get_with_query`]); `required` names query parameters that must be + /// present on every request, added if a link leaves one out. Each followed + /// page goes through the endpoint's generated type `T`, like the first, so + /// every item has the same shape. + /// + /// With `want`, stops as soon as that many items are held (reporting + /// `stopped_early` if the service still had more), so a command showing a + /// small window never reads pages it won't display. + /// + /// Otherwise stops, keeping what it has and reporting `incomplete`, at a + /// link that repeats, has no query to replay, or is past the page limit — + /// never an error, since the pages already read are good data. A request for + /// a followed page that fails still errors, like any other call. + async fn collect_pages( + &self, + path: &str, + required: &[(&str, &str)], + first: Value, + want: Option, + ) -> Result + where + T: serde::de::DeserializeOwned + serde::Serialize, + { + let mut page = first; + let mut items = super::common::page_items(&page); + let mut seen = std::collections::HashSet::new(); + let mut stopped_early = false; + for _ in 1..MAX_COMBINED_PAGES { + if want.is_some_and(|want| items.len() >= want) { + stopped_early = super::common::next_link_href(&page).is_some(); + break; + } + let Some(mut query) = super::common::next_link_query(&page) else { + break; + }; + if !seen.insert(query.clone()) { + break; + } + for (name, value) in required { + if !query + .split('&') + .any(|pair| pair.split('=').next() == Some(*name)) + { + let value: String = + url::form_urlencoded::byte_serialize(value.as_bytes()).collect(); + query.push_str(&format!("&{name}={value}")); + } + } + let raw = self.get_with_query(path, &query).await?; + let typed: T = serde_json::from_value(raw).map_err(ClientError::Response)?; + page = serde_json::to_value(typed).map_err(ClientError::Response)?; + items.extend(super::common::page_items(&page)); + } + let incomplete = !stopped_early && super::common::next_link_href(&page).is_some(); + Ok(CombinedPages { + items, + incomplete, + stopped_early, + }) } pub async fn get_app_subscription(&self, app_id: &str) -> Result { diff --git a/rust/src/hosting/client_tests.rs b/rust/src/hosting/client_tests.rs index 8fe7ce75..1bd9dfbe 100644 --- a/rust/src/hosting/client_tests.rs +++ b/rust/src/hosting/client_tests.rs @@ -468,13 +468,229 @@ async fn list_domains_hits_correct_path() { .await; client(&server.base_url()) - .list_domains("app-1") + .list_domains("app-1", None) .await .expect("list domains"); mock.assert_async().await; } +#[tokio::test] +async fn list_domains_without_a_cursor_sends_no_paging_parameters() { + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param_missing("pageToken") + .query_param_missing("pageSize") + .query_param_missing("page"); + then.status(200) + .json_body(json!({ "items": [], "links": [] })); + }) + .await; + + client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("list domains"); + + mock.assert_async().await; +} + +fn domains_first_page(server: &MockServer, body: Value) -> httpmock::Mock<'_> { + server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param_missing("pageToken"); + then.status(200).json_body(body); + }) +} + +fn domain_ids(listed: &CombinedPages) -> Vec<&str> { + listed + .items + .iter() + .map(|item| item["domainId"].as_str().unwrap_or("")) + .collect() +} + +#[tokio::test] +async fn list_domains_combines_pages_by_replaying_the_next_links_query() { + let server = MockServer::start_async().await; + // The link's query is already percent-encoded (`%2B` is a plus, `%3D` an + // equals sign in the opaque token); replaying it must not encode it a second + // time. The path and bearer token are ours, not taken from the link. + let first = domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/apps/app-1/domains?pageToken=a%2Bb%3D%3D&pageSize=25" + }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "a+b==") + .query_param("pageSize", "25") + .header("authorization", "Bearer test-token"); + // `noise` is not in the spec; it must not survive on later pages only. + then.status(200).json_body(json!({ + "items": [{ "domainId": "d2", "noise": 1 }, { "domainId": "d3" }], + "links": [{ "rel": "self", "href": "/v1/hosting/apps/app-1/domains" }] + })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("list domains"); + + first.assert_calls(1); + second.assert_calls(1); + assert_eq!(domain_ids(&listed), ["d1", "d2", "d3"]); + assert!(!listed.incomplete); + assert!( + listed.items.iter().all(|i| i.get("noise").is_none()), + "{:?}", + listed.items + ); +} + +#[tokio::test] +async fn list_domains_stops_reading_pages_once_the_wanted_items_are_held() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }, { "domainId": "d2" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "t2"); + then.status(200) + .json_body(json!({ "items": [{ "domainId": "d3" }], "links": [] })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", Some(2)) + .await + .expect("list domains"); + + second.assert_calls(0); + assert_eq!(domain_ids(&listed), ["d1", "d2"]); + assert!(listed.stopped_early, "the service still had a page"); + assert!(!listed.incomplete, "stopping on purpose is not a problem"); +} + +#[tokio::test] +async fn list_domains_reads_on_while_the_wanted_items_are_not_yet_held() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }, { "domainId": "d2" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "t2"); + then.status(200) + .json_body(json!({ "items": [{ "domainId": "d3" }], "links": [] })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", Some(3)) + .await + .expect("list domains"); + + second.assert_calls(1); + assert_eq!(domain_ids(&listed), ["d1", "d2", "d3"]); + assert!( + !listed.stopped_early, + "that was the last page, so it is the whole list" + ); + assert!(!listed.incomplete); +} + +#[tokio::test] +async fn list_domains_does_not_report_an_early_stop_when_nothing_more_exists() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ "items": [{ "domainId": "d1" }, { "domainId": "d2" }], "links": [] }), + ); + + let listed = client(&server.base_url()) + .list_domains("app-1", Some(1)) + .await + .expect("list domains"); + + // More held than wanted, but there was no further page to skip. + assert_eq!(domain_ids(&listed), ["d1", "d2"]); + assert!(!listed.stopped_early); + assert!(!listed.incomplete); +} + +#[tokio::test] +async fn list_domains_stops_at_a_repeated_next_link_and_reports_it() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=loop" }] + }), + ); + let looping = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "loop"); + then.status(200).json_body(json!({ + "items": [{ "domainId": "d2" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=loop" }] + })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("a loop must not fail the listing"); + + looping.assert_calls(1); + assert_eq!(domain_ids(&listed), ["d1", "d2"]); + assert!(listed.incomplete, "pages were left unread"); +} + +#[tokio::test] +async fn list_domains_reports_a_next_link_it_cannot_follow() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }], + // No query to replay, so following it could never advance. + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains" }] + }), + ); + + let listed = client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("list domains"); + + assert_eq!(domain_ids(&listed), ["d1"]); + assert!(listed.incomplete); +} + #[tokio::test] async fn attach_domain_sends_hostname() { let server = MockServer::start_async().await; @@ -536,6 +752,151 @@ async fn list_subscriptions_hits_correct_path() { mock.assert_async().await; } +fn subscriptions_first_page(server: &MockServer, body: Value) -> httpmock::Mock<'_> { + server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("hostingProduct", "WEB_HOSTING") + .query_param_missing("pageToken"); + then.status(200).json_body(body); + }) +} + +fn subscription_ids(listed: &CombinedPages) -> Vec<&str> { + listed + .items + .iter() + .map(|item| item["subscriptionId"].as_str().unwrap_or("")) + .collect() +} + +#[tokio::test] +async fn list_subscriptions_combines_pages_by_following_next_links() { + let server = MockServer::start_async().await; + let first = subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=t2" + }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("hostingProduct", "WEB_HOSTING") + .query_param("pageToken", "t2") + .header("authorization", "Bearer test-token"); + // `noise` is not in the spec; it must not survive on later pages only. + then.status(200).json_body(json!({ + "items": [{ "subscriptionId": "s2", "noise": 1 }, { "subscriptionId": "s3" }], + "links": [{ "rel": "self", "href": "/v1/hosting/subscriptions" }] + })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + first.assert_calls(1); + second.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1", "s2", "s3"]); + assert!(!listed.incomplete); + assert!( + listed.items.iter().all(|i| i.get("noise").is_none()), + "{:?}", + listed.items + ); +} + +#[tokio::test] +async fn list_subscriptions_keeps_the_required_filter_when_a_next_link_drops_it() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("pageToken", "t2") + .query_param("hostingProduct", "WEB_HOSTING"); + then.status(200) + .json_body(json!({ "items": [{ "subscriptionId": "s2" }], "links": [] })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + second.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1", "s2"]); +} + +#[tokio::test] +async fn list_subscriptions_stops_at_a_repeated_next_link_and_reports_it() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=loop" + }] + }), + ); + let looping = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("pageToken", "loop"); + then.status(200).json_body(json!({ + "items": [{ "subscriptionId": "s2" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=loop" + }] + })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("a loop must not fail the listing"); + + looping.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1", "s2"]); + assert!(listed.incomplete, "pages were left unread"); +} + +#[tokio::test] +async fn list_subscriptions_reports_a_next_link_it_cannot_follow() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + // No query to replay, so following it could never advance. + "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions" }] + }), + ); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + assert_eq!(subscription_ids(&listed), ["s1"]); + assert!(listed.incomplete); +} + #[tokio::test] async fn attach_subscription_sends_subscription_id() { let server = MockServer::start_async().await; diff --git a/rust/src/hosting/common.rs b/rust/src/hosting/common.rs index c1428049..d4c0cf10 100644 --- a/rust/src/hosting/common.rs +++ b/rust/src/hosting/common.rs @@ -283,13 +283,24 @@ pub fn next_link_href(response: &Value) -> Option { .map(str::to_owned) } -/// The warning to show when a listing that this CLI can only read one page of -/// reports another page via `links[rel=next]` — for the endpoints whose spec -/// types the response as paged but declares no paging request parameters -/// (`hosting domain list`, `hosting subscription list`). `None` when the -/// service reports nothing further. -pub fn more_pages_warning(response: &Value, what: &str) -> Option { - next_link_href(response).map(|_| format!("The API reports more {what} than can be displayed.")) +/// Longest `next`-link query string accepted as a continuation; anything longer +/// is treated as unusable rather than handed to the user as a token. +const MAX_CURSOR_QUERY_LEN: usize = 2048; + +/// The query string of a paged response's `links[rel=next].href` (for example +/// `pageToken=abc&pageSize=50`), which is everything a follow-up request needs: +/// the collections-and-pagination standard surfaces the cursor only through +/// that link. `None` when there is no next link, or it has no query to replay +/// (it could not advance), or the query is unreasonably long. +pub fn next_link_query(response: &Value) -> Option { + let href = next_link_href(response)?; + let query = href.split_once('?')?.1.split('#').next()?; + (!query.is_empty() && query.len() <= MAX_CURSOR_QUERY_LEN).then(|| query.to_owned()) +} + +/// The warning for a listing that ended with pages left unread. +pub fn incomplete_list_warning(what: &str) -> String { + format!("The API reports more {what} than can be displayed.") } /// The `items` array of a paged hosting response (empty when absent). @@ -430,21 +441,40 @@ mod tests { } #[test] - fn more_pages_warning_appears_only_when_the_api_reports_a_next_page() { - let more = json!({ - "items": [], - "links": [{ "rel": "next", "href": "/v1/hosting/apps/a/domains?pageToken=t2" }] - }); - let warning = more_pages_warning(&more, "domains").expect("next link warns"); - assert!(warning.contains("more domains"), "{warning}"); - assert!(warning.contains("than can be displayed"), "{warning}"); + fn next_link_query_returns_the_query_of_the_next_link() { + let relative = json!({ "links": [ + { "rel": "self", "href": "/v1/hosting/apps/a/domains" }, + { "rel": "next", "href": "/v1/hosting/apps/a/domains?pageToken=ab%2Bc%3D&pageSize=25" } + ]}); + assert_eq!( + next_link_query(&relative).as_deref(), + Some("pageToken=ab%2Bc%3D&pageSize=25"), + "returned verbatim, still encoded" + ); + let absolute = json!({ "links": [ + { "rel": "next", "href": "https://api.example/v1/hosting/x?page=2&pageSize=10#frag" } + ]}); + assert_eq!( + next_link_query(&absolute).as_deref(), + Some("page=2&pageSize=10") + ); + } - let complete = json!({ - "items": [], - "links": [{ "rel": "self", "href": "/v1/hosting/apps/a/domains" }] - }); - assert_eq!(more_pages_warning(&complete, "domains"), None); - assert_eq!(more_pages_warning(&json!({ "items": [] }), "domains"), None); + #[test] + fn next_link_query_is_none_when_the_link_cannot_advance() { + for response in [ + json!({ "items": [] }), + json!({ "links": [{ "rel": "self", "href": "/x?pageToken=t" }] }), + json!({ "links": [{ "rel": "next", "href": "/v1/hosting/x" }] }), + json!({ "links": [{ "rel": "next", "href": "/v1/hosting/x?" }] }), + ] { + assert_eq!(next_link_query(&response), None, "{response}"); + } + let long = format!("/x?pageToken={}", "a".repeat(MAX_CURSOR_QUERY_LEN + 1)); + assert_eq!( + next_link_query(&json!({ "links": [{ "rel": "next", "href": long }] })), + None + ); } #[test] diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 059edede..0d9d01a0 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -4,7 +4,7 @@ use cli_engine::{ use serde_json::json; use crate::hosting::common::{ - HostingDomainSummary, client_err, make_client, more_pages_warning, page_items, + HostingDomainSummary, client_err, incomplete_list_warning, make_client, }; use crate::next_action::next_action; use crate::pagination_token::{self, warn_user}; @@ -21,8 +21,11 @@ struct DomainListArgs { offset: Option, } -/// The API accepts no paging parameters (see `HostingClient::list_domains`), -/// so the one response is paged client-side by `--limit` / `--continue`. +/// The service doesn't paginate this listing yet (see +/// `HostingClient::list_domains`), so every domain is gathered — following the +/// service's `next` links if it ever sends them — and `--limit` / `--continue` +/// page through that whole list client-side. Once the service accepts paging +/// parameters this moves to its own cursor. pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_typed_with_context::( CommandSpec::from_args::("list", "List domains attached to an application") @@ -40,25 +43,42 @@ pub(super) fn command() -> RuntimeCommandSpec { let app_id = args.app_id.clone(); let client = make_client(&ctx, &[DOMAIN_READ]).await?; - let response = client.list_domains(&app_id).await.map_err(client_err)?; - if let Some(warning) = more_pages_warning(&response, "domains") { - warn_user(&warning); - } - let continue_token = pagination_token::resolve_continue_token( args.offset, ctx.middleware.continue_token.as_deref(), ); + // Read only as many pages as the window needs. A `--filter` runs + // after collection, so with one the whole list is needed. + let want = ctx.middleware.filter.is_empty().then(|| { + pagination_token::known_collection_need( + continue_token.as_deref(), + ctx.middleware.cursor_limit, + 200, + ) + }); + let listed = client + .list_domains(&app_id, want) + .await + .map_err(client_err)?; + if listed.incomplete { + warn_user(&incomplete_list_warning("domains")); + } + let (page, continuation) = pagination_token::paginate_known_collection( - pagination_token::filter_known_collection( - page_items(&response), - &ctx.middleware.filter, - )?, + pagination_token::filter_known_collection(listed.items, &ctx.middleware.filter)?, ctx.middleware.cursor_limit, 200, continue_token.as_deref(), ); + // Pages past the window were deliberately not read, so the length of + // what was read says nothing about the total. + let continuation = if listed.stopped_early { + pagination_token::forget_totals(continuation) + } else { + continuation + }; + Ok(CommandResult::new(json!(page)) .with_cursor(continuation) .with_next_actions(vec![ diff --git a/rust/src/hosting/subscription/list.rs b/rust/src/hosting/subscription/list.rs index aa65f413..577dc8a3 100644 --- a/rust/src/hosting/subscription/list.rs +++ b/rust/src/hosting/subscription/list.rs @@ -5,8 +5,7 @@ use cli_engine::{ use serde_json::{Value, json}; use crate::hosting::common::{ - HostingProduct, HostingSubscriptionList, client_err, make_client, more_pages_warning, - page_items, + HostingProduct, HostingSubscriptionList, client_err, incomplete_list_warning, make_client, }; use crate::next_action::next_action; use crate::pagination_token::warn_user; @@ -58,15 +57,15 @@ pub(super) fn command() -> RuntimeCommandSpec { let hosting_product = args.hosting_product.as_str(); let client = make_client(&ctx, &[SUB_READ]).await?; - let response = client + let listed = client .list_subscriptions(hosting_product) .await .map_err(client_err)?; - if let Some(warning) = more_pages_warning(&response, "subscriptions") { - warn_user(&warning); + if listed.incomplete { + warn_user(&incomplete_list_warning("subscriptions")); } - Ok(list_result(page_items(&response), args.limit)) + Ok(list_result(listed.items, args.limit)) }, ) } diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs index 9c9383b3..fca69932 100644 --- a/rust/src/pagination_token.rs +++ b/rust/src/pagination_token.rs @@ -182,6 +182,29 @@ pub(crate) fn filter_known_collection( }) } +/// How many items a [`paginate_known_collection`] call will look at: the +/// window's end plus one, because seeing an item past the window is what says +/// there is a next page. A caller that gathers the collection from a paged +/// service can stop reading once it holds this many. +pub(crate) fn known_collection_need( + continue_token: Option<&str>, + limit: i64, + max_limit: i64, +) -> usize { + let fields = parse(continue_token); + let offset = field(&fields, "offset", 0usize); + let size = saturating_usize(inherited_limit(&fields, limit, max_limit)).max(1); + offset.saturating_add(size).saturating_add(1) +} + +/// Drops `total` and `remaining` from a continuation whose collection was only +/// read as far as the window needed: the length of a prefix is not a count. +pub(crate) fn forget_totals(mut continuation: CursorContinuation) -> CursorContinuation { + continuation.total = None; + continuation.remaining = None; + continuation +} + /// Slices an already-fully-known collection using a `limit:,offset:` /// continuation token. pub(crate) fn paginate_known_collection( @@ -413,6 +436,34 @@ mod tests { assert_eq!(cont.total, Some(0)); } + #[test] + fn known_collection_need_is_the_window_end_plus_one() { + assert_eq!(known_collection_need(None, 10, 100), 11); + assert_eq!( + known_collection_need(Some("limit:5,offset:20"), 10, 100), + 26 + ); + // A token limit out of range falls back to the request's, like slicing. + assert_eq!( + known_collection_need(Some("limit:-1,offset:20"), 10, 100), + 31 + ); + assert_eq!(known_collection_need(Some("offset:3"), 7, 100), 11); + assert_eq!( + known_collection_need(Some("offset:18446744073709551615"), 10, 100), + usize::MAX + ); + } + + #[test] + fn forget_totals_clears_only_the_counts() { + let (_, cont) = paginate_known_collection(items(5), 2, 100, None); + assert_eq!((cont.total, cont.remaining), (Some(5), Some(3))); + let cont = forget_totals(cont); + assert_eq!((cont.total, cont.remaining), (None, None)); + assert_eq!(cont.continue_from.as_deref(), Some("limit:2,offset:2")); + } + #[test] fn filter_known_collection_keeps_only_matching_items_in_order() { let all = vec![ diff --git a/rust/src/platform/app/commands/list.rs b/rust/src/platform/app/commands/list.rs index e82cc84c..00e21cae 100644 --- a/rust/src/platform/app/commands/list.rs +++ b/rust/src/platform/app/commands/list.rs @@ -40,9 +40,8 @@ pub(super) fn command() -> RuntimeCommandSpec { continuation_for(page.has_next_page, page.end_cursor); if missing_cursor { pagination_token::warn_user( - "The App Registry reports more applications than this response \ - contains but returned no cursor to fetch them; the list may be \ - incomplete.", + "More applications exist than were returned, but there is no way to \ + fetch the rest; the list may be incomplete.", ); } let data = serde_json::to_value(page.items).map_err(|e| { From a5a31c46fabf54a91e0907e24fd128fd907c8f42 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 22 Oct 2026 16:09:31 -0700 Subject: [PATCH 06/10] fix(cli): reject malformed --continue tokens and negative offsets A `--continue` value that this command did not issue now fails with a validation error and a fix, instead of quietly restarting from the first page where repeated rows could go unnoticed. That covers a truncated token, a pair with no value, a key the command never issues (for example a `dns list` token passed to `platform actions list`), a non-numeric value, and a page size outside the command's range. A blank `--continue` still starts from the beginning. `--offset` on `api parameter list`, `api response list`, `api search` and `hosting domain list` must now be 0 or greater, and is rejected by argument parsing rather than treated as 0. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/api/parameter.rs | 4 +- rust/src/api/response.rs | 4 +- rust/src/api/search.rs | 6 +- rust/src/dns/list.rs | 2 +- rust/src/domain/list.rs | 57 +++-- rust/src/email/list.rs | 2 +- rust/src/hosting/domain/list.rs | 14 +- rust/src/pagination_token.rs | 360 ++++++++++++++++++++----------- rust/src/platform/actions/mod.rs | 2 +- 9 files changed, 296 insertions(+), 155 deletions(-) diff --git a/rust/src/api/parameter.rs b/rust/src/api/parameter.rs index c90fbdae..4eb6b8d4 100644 --- a/rust/src/api/parameter.rs +++ b/rust/src/api/parameter.rs @@ -24,7 +24,7 @@ struct ParameterListArgs { operation: String, /// Jump directly to an offset in the collection. Ignored when `--continue` is given. - #[arg(long, value_name = "N")] + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] offset: Option, } @@ -69,7 +69,7 @@ pub(super) fn list_command() -> RuntimeCommandSpec { ctx.middleware.cursor_limit, OPERATION_CHILD_LIST_MAX_LIMIT, continue_token.as_deref(), - ); + )?; let next_actions = vec![ next_action( "api parameter get --operation ", diff --git a/rust/src/api/response.rs b/rust/src/api/response.rs index 26664c7b..34027aef 100644 --- a/rust/src/api/response.rs +++ b/rust/src/api/response.rs @@ -21,7 +21,7 @@ struct ResponseListArgs { operation: String, /// Jump directly to an offset in the collection. Ignored when `--continue` is given. - #[arg(long, value_name = "N")] + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] offset: Option, } @@ -61,7 +61,7 @@ pub(super) fn list_command() -> RuntimeCommandSpec { ctx.middleware.cursor_limit, OPERATION_CHILD_LIST_MAX_LIMIT, continue_token.as_deref(), - ); + )?; let next_actions = vec![ next_action( "api response get --operation ", diff --git a/rust/src/api/search.rs b/rust/src/api/search.rs index 80a373af..215673a1 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -35,7 +35,7 @@ struct SearchArgs { /// Jump directly to this offset instead of walking there one --continue /// token at a time. Ignored when `--continue` is given. - #[arg(long, value_name = "N")] + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] offset: Option, } @@ -62,7 +62,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.cursor_limit, MAX_LIMIT, None, - ); + )?; return Ok(CommandResult::new(json!(page)).with_cursor(continuation)); } let mut results: Vec = hits @@ -136,7 +136,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.cursor_limit, MAX_LIMIT, continue_token.as_deref(), - ); + )?; Ok(CommandResult::new(json!(page)) .with_cursor(continuation) .with_next_actions(next_actions)) diff --git a/rust/src/dns/list.rs b/rust/src/dns/list.rs index ab28a4ce..d4d0edca 100644 --- a/rust/src/dns/list.rs +++ b/rust/src/dns/list.rs @@ -87,7 +87,7 @@ pub(super) fn command() -> RuntimeCommandSpec { 1u64, ctx.middleware.cursor_limit, MAX_PAGE_SIZE.get() as i64, - ); + )?; let fetched = fetch_records_page( &client, domain.as_str(), diff --git a/rust/src/domain/list.rs b/rust/src/domain/list.rs index e9e722a4..bb067063 100644 --- a/rust/src/domain/list.rs +++ b/rust/src/domain/list.rs @@ -206,20 +206,26 @@ fn encode_resume(token: &str, direction: Option, -) -> Option<(String, Option)> { - let fields = pagination_token::parse(continue_token); - let token = fields.get("token")?.clone(); - let direction = fields - .get("direction") - .and_then(|d| d.parse::().ok()); - Some((token, direction)) +) -> Result)>> { + let fields = pagination_token::parse(continue_token, &["token", "direction"])?; + if continue_token.is_none_or(|t| t.trim().is_empty()) { + return Ok(None); + } + let Some(token) = fields.text("token") else { + return Err(pagination_token::invalid_token( + continue_token.unwrap_or_default(), + "it has no `token`", + )); + }; + let direction = fields.typed::("direction")?; + Ok(Some((token.to_owned(), direction))) } pub(super) fn command() -> RuntimeCommandSpec { @@ -246,7 +252,7 @@ pub(super) fn command() -> RuntimeCommandSpec { let client = make_client(&ctx).await?; let page_size = u64::try_from(ctx.middleware.cursor_limit).unwrap_or(MAX_PAGE_SIZE.get()); - let resume = decode_resume(ctx.middleware.continue_token.as_deref()); + let resume = decode_resume(ctx.middleware.continue_token.as_deref())?; let (items, next) = fetch_domain_page(&client, &statuses, visible_only, page_size, resume, debug) .await?; @@ -306,7 +312,7 @@ mod tests { ); assert_eq!(encoded, "token:abc123,direction:forward"); assert_eq!( - decode_resume(Some(&encoded)), + decode_resume(Some(&encoded)).expect("valid token"), Some(( "abc123".to_string(), Some(types::ListDomainsPageTokenDirection::Forward) @@ -319,15 +325,30 @@ mod tests { let encoded = encode_resume("abc123", None); assert_eq!(encoded, "token:abc123"); assert_eq!( - decode_resume(Some(&encoded)), + decode_resume(Some(&encoded)).expect("valid token"), Some(("abc123".to_string(), None)) ); } #[test] - fn decode_resume_of_none_or_malformed_starts_from_the_beginning() { - assert_eq!(decode_resume(None), None); - assert_eq!(decode_resume(Some("garbage")), None); + fn decode_resume_of_none_or_blank_starts_from_the_beginning() { + assert_eq!(decode_resume(None).expect("absent"), None); + assert_eq!(decode_resume(Some("")).expect("blank"), None); + assert_eq!(decode_resume(Some(" ")).expect("blank"), None); + } + + #[test] + fn decode_resume_rejects_a_token_it_did_not_issue() { + for bad in [ + "garbage", + "token:abc,direction:sideways", + "direction:forward", + "token:abc,direction", + "limit:5,offset:0", + ] { + let err = decode_resume(Some(bad)).expect_err(bad); + assert!(err.to_string().contains("--continue"), "{bad}: {err}"); + } } #[test] diff --git a/rust/src/email/list.rs b/rust/src/email/list.rs index 21c6d127..2683bf46 100644 --- a/rust/src/email/list.rs +++ b/rust/src/email/list.rs @@ -43,7 +43,7 @@ pub(super) fn command() -> RuntimeCommandSpec { 1u32, ctx.middleware.cursor_limit, SERVER_PAGE_SIZE_CAP as i64, - ); + )?; let page_size = usize::try_from(page_size).unwrap_or(SERVER_PAGE_SIZE_CAP); let (mailboxes, has_more) = fetch_mailbox_page( &client, diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 0d9d01a0..10ee43d0 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -17,7 +17,7 @@ struct DomainListArgs { app_id: String, /// Jump directly to an offset in the results. Ignored when `--continue` is given. - #[arg(long, value_name = "N")] + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] offset: Option, } @@ -49,13 +49,15 @@ pub(super) fn command() -> RuntimeCommandSpec { ); // Read only as many pages as the window needs. A `--filter` runs // after collection, so with one the whole list is needed. - let want = ctx.middleware.filter.is_empty().then(|| { - pagination_token::known_collection_need( + let want = if ctx.middleware.filter.is_empty() { + Some(pagination_token::known_collection_need( continue_token.as_deref(), ctx.middleware.cursor_limit, 200, - ) - }); + )?) + } else { + None + }; let listed = client .list_domains(&app_id, want) .await @@ -69,7 +71,7 @@ pub(super) fn command() -> RuntimeCommandSpec { ctx.middleware.cursor_limit, 200, continue_token.as_deref(), - ); + )?; // Pages past the window were deliberately not read, so the length of // what was read says nothing about the total. diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs index fca69932..d9dfa0d4 100644 --- a/rust/src/pagination_token.rs +++ b/rust/src/pagination_token.rs @@ -15,28 +15,85 @@ use std::collections::HashMap; use std::str::FromStr; -use cli_engine::{PipelineOpts, apply_pipeline}; - -use cli_engine::CursorContinuation; +use cli_engine::{CliCoreError, CursorContinuation, PipelineOpts, apply_pipeline}; use serde_json::Value; -/// Parses a `key:value,key2:value2` token into a lookup map. +/// The validated `key:value` pairs of a `--continue` token. +#[derive(Debug)] +pub(crate) struct Fields { + raw: String, + map: HashMap, +} + +impl Fields { + fn empty() -> Self { + Self { + raw: String::new(), + map: HashMap::new(), + } + } + + /// The raw text of a field, if the token has it. + pub(crate) fn text(&self, key: &str) -> Option<&str> { + self.map.get(key).map(String::as_str) + } + + /// A field parsed as `T`: `Ok(None)` when absent, an error when present but + /// not a valid `T`. + pub(crate) fn typed(&self, key: &str) -> cli_engine::Result> { + match self.map.get(key) { + None => Ok(None), + Some(value) => value + .parse() + .map(Some) + .map_err(|_| invalid_token(&self.raw, format!("`{key}` has an invalid value"))), + } + } +} + +/// The error for a `--continue` value that is not a token this command issued. +/// A mistyped or truncated token must not quietly restart from the first page, +/// where an agent could loop over duplicate pages without noticing. +pub(crate) fn invalid_token(raw: &str, reason: impl std::fmt::Display) -> CliCoreError { + let shown: String = raw.chars().take(80).collect(); + crate::error::GddyError::validation(format!("invalid --continue value \"{shown}\": {reason}")) + .with_fix( + "Use the --continue value from the previous page's next action exactly as printed, \ + or leave --continue out to start from the beginning.", + ) + .into_cli_error() +} + +/// Parses a `key:value,key2:value2` token, accepting only the `allowed` keys. /// -/// `None`/empty input parses to an empty map — an absent or blank -/// `--continue` always means "start from the beginning," never a parse -/// error. A malformed pair (no `:`) is silently skipped rather than -/// rejected, matching [`field`]'s "degrade to the default" philosophy: a -/// hand-edited or truncated `--continue` value should restart pagination, -/// not fail the command outright. -pub(crate) fn parse(token: Option<&str>) -> HashMap { - let Some(token) = token.filter(|t| !t.is_empty()) else { - return HashMap::new(); +/// An absent or blank token is "start from the beginning" and parses to no +/// fields. Anything else must be well-formed: a pair with no `:`, or a key this +/// command never issues (for example a token pasted from another command), is +/// rejected rather than guessed at. +pub(crate) fn parse(token: Option<&str>, allowed: &[&str]) -> cli_engine::Result { + let Some(raw) = token.filter(|t| !t.trim().is_empty()) else { + return Ok(Fields::empty()); }; - token - .split(',') - .filter_map(|pair| pair.split_once(':')) - .map(|(key, value)| (key.to_owned(), unescape(value))) - .collect() + let mut map = HashMap::new(); + for pair in raw.split(',') { + let Some((key, value)) = pair.split_once(':') else { + return Err(invalid_token( + raw, + "expected comma-separated key:value pairs", + )); + }; + if !allowed.contains(&key) { + return Err(invalid_token( + raw, + format!("`{key}` is not part of this command's tokens"), + )); + } + map.insert(key.to_owned(), unescape(value)); + } + Ok(Fields { + raw: raw.to_owned(), + map, + }) } /// Escapes the two characters that would corrupt a `key:value,key:value` @@ -53,17 +110,6 @@ fn unescape(value: &str) -> String { value.replace("%2C", ",").replace("%25", "%") } -/// Reads a typed field out of a [`parse`]d map, falling back to `default` -/// when the key is absent or its value doesn't parse as `T` — the same -/// "degrade to start over" behavior `parse` itself uses for a malformed -/// pair. -pub(crate) fn field(fields: &HashMap, key: &str, default: T) -> T { - fields - .get(key) - .and_then(|value| value.parse().ok()) - .unwrap_or(default) -} - /// Builds a `key:value,key2:value2` token from ordered pairs. pub(crate) fn build(pairs: &[(&str, &str)]) -> String { pairs @@ -128,25 +174,27 @@ pub(crate) fn resolve_page_request( default_page: P, limit: i64, max_limit: i64, -) -> (P, i64) { - let fields = parse(continue_token); - let token_page = fields.get("page").and_then(|value| value.parse().ok()); - let page = token_page.or(explicit_page).unwrap_or(default_page); - let page_size = inherited_limit(&fields, limit, max_limit); - (page, page_size) +) -> cli_engine::Result<(P, i64)> { + let fields = parse(continue_token, &["limit", "page"])?; + let page = match fields.typed::

("page")? { + Some(page) => page, + None => explicit_page.unwrap_or(default_page), + }; + let page_size = limit_from(&fields, limit, max_limit)?; + Ok((page, page_size)) } -/// The page size a `--continue` token carries, if it is within the command's -/// `1..=max_limit` range; otherwise the request's own already-validated -/// `--limit`. A hand-edited token must not widen a page past the command's -/// maximum (`limit:-1` would otherwise mean "everything") or send the backend -/// a size it rejects. -fn inherited_limit(fields: &HashMap, request_limit: i64, max_limit: i64) -> i64 { - let inherited = field(fields, "limit", request_limit); - if (1..=max_limit).contains(&inherited) { - inherited - } else { - request_limit +/// The page size a `--continue` token carries, which must be within the +/// command's `1..=max_limit`; without one, the request's own already-validated +/// `--limit`. A token cannot widen a page past the command's maximum. +fn limit_from(fields: &Fields, request_limit: i64, max_limit: i64) -> cli_engine::Result { + match fields.typed::("limit")? { + None => Ok(request_limit), + Some(limit) if (1..=max_limit).contains(&limit) => Ok(limit), + Some(limit) => Err(invalid_token( + &fields.raw, + format!("`limit` {limit} must be between 1 and {max_limit}"), + )), } } @@ -190,11 +238,11 @@ pub(crate) fn known_collection_need( continue_token: Option<&str>, limit: i64, max_limit: i64, -) -> usize { - let fields = parse(continue_token); - let offset = field(&fields, "offset", 0usize); - let size = saturating_usize(inherited_limit(&fields, limit, max_limit)).max(1); - offset.saturating_add(size).saturating_add(1) +) -> cli_engine::Result { + let fields = parse(continue_token, &["limit", "offset"])?; + let offset = fields.typed::("offset")?.unwrap_or(0); + let size = saturating_usize(limit_from(&fields, limit, max_limit)?).max(1); + Ok(offset.saturating_add(size).saturating_add(1)) } /// Drops `total` and `remaining` from a continuation whose collection was only @@ -212,11 +260,10 @@ pub(crate) fn paginate_known_collection( limit: i64, max_limit: i64, continue_token: Option<&str>, -) -> (Vec, CursorContinuation) { - let fields = parse(continue_token); - let offset = field(&fields, "offset", 0usize); - let limit = inherited_limit(&fields, limit, max_limit); - let limit = saturating_usize(limit).max(1); +) -> cli_engine::Result<(Vec, CursorContinuation)> { + let fields = parse(continue_token, &["limit", "offset"])?; + let offset = fields.typed::("offset")?.unwrap_or(0); + let limit = saturating_usize(limit_from(&fields, limit, max_limit)?).max(1); let total = items.len(); let start = offset.min(total); let end = start.saturating_add(limit).min(total); @@ -235,7 +282,7 @@ pub(crate) fn paginate_known_collection( .with_total(saturating_i64(total)) .with_remaining(saturating_i64(remaining)) .with_limit(saturating_i64(limit)); - (page, continuation) + Ok((page, continuation)) } /// Writes `message` as a one-line warning to `out`. @@ -259,44 +306,71 @@ mod tests { use super::*; + const KEYS: &[&str] = &["page", "limit", "offset"]; + #[test] fn parse_reads_key_value_pairs() { - let fields = parse(Some("page:2,limit:50")); - assert_eq!(fields.get("page").map(String::as_str), Some("2")); - assert_eq!(fields.get("limit").map(String::as_str), Some("50")); + let fields = parse(Some("page:2,limit:50"), KEYS).expect("valid"); + assert_eq!(fields.text("page"), Some("2")); + assert_eq!(fields.text("limit"), Some("50")); } #[test] - fn parse_of_none_or_empty_is_an_empty_map() { - assert!(parse(None).is_empty()); - assert!(parse(Some("")).is_empty()); + fn parse_of_none_or_blank_is_no_fields() { + for token in [None, Some(""), Some(" ")] { + let fields = parse(token, KEYS).expect("start from the beginning"); + assert_eq!(fields.text("page"), None); + } } #[test] - fn parse_skips_a_pair_with_no_colon() { - let fields = parse(Some("page:2,garbage,limit:50")); - assert_eq!(fields.len(), 2); + fn parse_rejects_a_pair_with_no_colon() { + for bad in ["page:2,garbage,limit:50", "limit:5,offs", "garbage"] { + let err = parse(Some(bad), KEYS).expect_err(bad); + assert!( + err.to_string().contains("invalid --continue value"), + "{bad}: {err}" + ); + } } #[test] - fn field_falls_back_on_missing_or_unparseable_value() { - let fields = parse(Some("page:not-a-number")); - assert_eq!(field(&fields, "page", 1u32), 1); - assert_eq!(field(&fields, "missing", 7u32), 7); + fn parse_rejects_a_key_this_command_never_issues() { + // e.g. a `dns list` token (`page`) pasted into a client-side list. + let err = parse(Some("page:2"), &["limit", "offset"]).expect_err("foreign key"); + assert!(err.to_string().contains("`page`"), "{err}"); } #[test] - fn field_reads_a_present_valid_value() { - let fields = parse(Some("page:3")); - assert_eq!(field(&fields, "page", 1u32), 3); + fn invalid_token_errors_carry_an_actionable_fix_and_a_bounded_echo() { + let long = "x".repeat(500); + let err = parse(Some(&long), KEYS).expect_err("garbage"); + let text = format!("{err:?}"); + assert!(text.contains("previous page's next action"), "{text}"); + assert!( + err.to_string().len() < 300, + "echo of the bad value is bounded" + ); + } + + #[test] + fn typed_fields_are_absent_ok_or_an_error() { + let fields = parse(Some("page:3,limit:abc"), KEYS).expect("well-formed"); + assert_eq!(fields.typed::("page").expect("valid"), Some(3)); + assert_eq!(fields.typed::("offset").expect("absent"), None); + let err = fields.typed::("limit").expect_err("not a number"); + assert!(err.to_string().contains("`limit`"), "{err}"); + // A negative or fractional page is not a u32 either. + let fields = parse(Some("page:-1"), KEYS).expect("well-formed"); + assert!(fields.typed::("page").is_err()); } #[test] fn build_and_parse_round_trip() { let token = build(&[("page", "3"), ("limit", "20")]); - let fields = parse(Some(&token)); - assert_eq!(field(&fields, "page", 0u32), 3); - assert_eq!(field(&fields, "limit", 0u32), 20); + let fields = parse(Some(&token), KEYS).expect("valid"); + assert_eq!(fields.typed::("page").expect("valid"), Some(3)); + assert_eq!(fields.typed::("limit").expect("valid"), Some(20)); } #[test] @@ -326,7 +400,8 @@ mod tests { #[test] fn resolve_page_request_with_no_input_defaults_the_page_and_uses_the_given_limit() { - let (page, page_size) = resolve_page_request(None::, None, 1u32, 25, 100); + let (page, page_size) = + resolve_page_request(None::, None, 1u32, 25, 100).expect("valid token"); assert_eq!(page, 1); assert_eq!(page_size, 25); } @@ -337,7 +412,8 @@ mod tests { // this call's own --limit defaulted to something else (25) — page 2 // of size-10 pages and page 2 of size-25 pages cover different items. let token = build(&[("limit", "10"), ("page", "2")]); - let (page, page_size) = resolve_page_request(None::, Some(&token), 1u32, 25, 100); + let (page, page_size) = + resolve_page_request(None::, Some(&token), 1u32, 25, 100).expect("valid token"); assert_eq!(page, 2); assert_eq!(page_size, 10); } @@ -347,7 +423,8 @@ mod tests { { // No continue token, so there's no prior size to inherit — the // request's own limit applies. - let (page, page_size) = resolve_page_request(Some(5u32), None, 1u32, 25, 100); + let (page, page_size) = + resolve_page_request(Some(5u32), None, 1u32, 25, 100).expect("valid token"); assert_eq!(page, 5); assert_eq!(page_size, 25); } @@ -357,7 +434,8 @@ mod tests { // The engine's next-page action replays `--page 5` next to the new // `--continue`; the token's page has to win or the user never advances. let token = build(&[("limit", "10"), ("page", "6")]); - let (page, page_size) = resolve_page_request(Some(5u32), Some(&token), 1u32, 25, 100); + let (page, page_size) = + resolve_page_request(Some(5u32), Some(&token), 1u32, 25, 100).expect("valid token"); assert_eq!(page, 6); assert_eq!(page_size, 10); } @@ -368,7 +446,7 @@ mod tests { #[test] fn paginate_known_collection_returns_the_first_page_with_no_token() { - let (page, cont) = paginate_known_collection(items(5), 2, 100, None); + let (page, cont) = paginate_known_collection(items(5), 2, 100, None).expect("valid token"); assert_eq!(page, vec![json!({"id": 0}), json!({"id": 1})]); assert!(cont.continue_from.is_some()); assert_eq!(cont.total, Some(5)); @@ -377,9 +455,10 @@ mod tests { #[test] fn paginate_known_collection_resumes_from_the_continue_token() { - let (_, cont) = paginate_known_collection(items(5), 2, 100, None); + let (_, cont) = paginate_known_collection(items(5), 2, 100, None).expect("valid token"); let token = cont.continue_from.expect("has_more implies a token"); - let (page, cont2) = paginate_known_collection(items(5), 2, 100, Some(&token)); + let (page, cont2) = + paginate_known_collection(items(5), 2, 100, Some(&token)).expect("valid token"); assert_eq!(page, vec![json!({"id": 2}), json!({"id": 3})]); assert!(cont2.continue_from.is_some()); assert_eq!(cont2.remaining, Some(1)); @@ -387,14 +466,15 @@ mod tests { #[test] fn paginate_known_collection_continue_token_embeds_and_inherits_the_limit() { - let (_, cont) = paginate_known_collection(items(5), 2, 100, None); + let (_, cont) = paginate_known_collection(items(5), 2, 100, None).expect("valid token"); let token = cont.continue_from.expect("has_more implies a token"); assert_eq!(token, "limit:2,offset:2"); // A caller that resumes with --continue alone, and whose --limit // defaulted to something else entirely (25) this call, must still // get the size (2) that produced the token, not the new default. - let (page, cont2) = paginate_known_collection(items(5), 25, 100, Some(&token)); + let (page, cont2) = + paginate_known_collection(items(5), 25, 100, Some(&token)).expect("valid token"); assert_eq!(page, vec![json!({"id": 2}), json!({"id": 3})]); assert_eq!(cont2.remaining, Some(1)); } @@ -405,7 +485,8 @@ mod tests { // `limit`) for an explicit --offset — the request's own limit // applies, same as a fresh, tokenless call. let token = resolve_continue_token(Some(2), None).expect("explicit offset"); - let (page, _) = paginate_known_collection(items(5), 3, 100, Some(&token)); + let (page, _) = + paginate_known_collection(items(5), 3, 100, Some(&token)).expect("valid token"); assert_eq!( page, vec![json!({"id": 2}), json!({"id": 3}), json!({"id": 4})] @@ -414,7 +495,8 @@ mod tests { #[test] fn paginate_known_collection_reaches_the_end_with_no_further_token() { - let (page, cont) = paginate_known_collection(items(5), 2, 100, Some("offset:4")); + let (page, cont) = + paginate_known_collection(items(5), 2, 100, Some("offset:4")).expect("valid token"); assert_eq!(page, vec![json!({"id": 4})]); assert!(cont.continue_from.is_none()); assert_eq!(cont.remaining, Some(0)); @@ -422,7 +504,8 @@ mod tests { #[test] fn paginate_known_collection_past_the_end_returns_empty() { - let (page, cont) = paginate_known_collection(items(3), 2, 100, Some("offset:10")); + let (page, cont) = + paginate_known_collection(items(3), 2, 100, Some("offset:10")).expect("valid token"); assert!(page.is_empty()); assert!(cont.continue_from.is_none()); assert_eq!(cont.remaining, Some(0)); @@ -430,7 +513,8 @@ mod tests { #[test] fn paginate_known_collection_of_an_empty_collection() { - let (page, cont) = paginate_known_collection(Vec::new(), 10, 100, None); + let (page, cont) = + paginate_known_collection(Vec::new(), 10, 100, None).expect("valid token"); assert!(page.is_empty()); assert!(cont.continue_from.is_none()); assert_eq!(cont.total, Some(0)); @@ -438,26 +522,32 @@ mod tests { #[test] fn known_collection_need_is_the_window_end_plus_one() { - assert_eq!(known_collection_need(None, 10, 100), 11); - assert_eq!( - known_collection_need(Some("limit:5,offset:20"), 10, 100), - 26 - ); - // A token limit out of range falls back to the request's, like slicing. - assert_eq!( - known_collection_need(Some("limit:-1,offset:20"), 10, 100), - 31 - ); - assert_eq!(known_collection_need(Some("offset:3"), 7, 100), 11); + let need = |token: Option<&str>, limit| known_collection_need(token, limit, 100); + assert_eq!(need(None, 10).expect("valid"), 11); + assert_eq!(need(Some("limit:5,offset:20"), 10).expect("valid"), 26); + assert_eq!(need(Some("offset:3"), 7).expect("valid"), 11); assert_eq!( - known_collection_need(Some("offset:18446744073709551615"), 10, 100), + need(Some("offset:18446744073709551615"), 10).expect("valid"), usize::MAX ); } + #[test] + fn known_collection_need_rejects_a_token_that_slicing_would_reject() { + for bad in [ + "limit:-1,offset:20", + "limit:0", + "limit:101", + "offset:-3", + "nonsense", + ] { + assert!(known_collection_need(Some(bad), 10, 100).is_err(), "{bad}"); + } + } + #[test] fn forget_totals_clears_only_the_counts() { - let (_, cont) = paginate_known_collection(items(5), 2, 100, None); + let (_, cont) = paginate_known_collection(items(5), 2, 100, None).expect("valid token"); assert_eq!((cont.total, cont.remaining), (Some(5), Some(3))); let cont = forget_totals(cont); assert_eq!((cont.total, cont.remaining), (None, None)); @@ -506,13 +596,9 @@ mod tests { "abc,def", "100%", "a+b c", "%2C", "%252C", ",,%,%25,", "x:y,z", ] { let token = build(&[("token", opaque), ("direction", "forward")]); - let fields = parse(Some(&token)); - assert_eq!( - fields.get("token").map(String::as_str), - Some(opaque), - "{token}" - ); - assert_eq!(fields.get("direction").map(String::as_str), Some("forward")); + let fields = parse(Some(&token), &["token", "direction"]).expect("valid"); + assert_eq!(fields.text("token"), Some(opaque), "{token}"); + assert_eq!(fields.text("direction"), Some("forward")); } } @@ -522,18 +608,19 @@ mod tests { } #[test] - fn paginate_known_collection_ignores_an_out_of_range_token_limit() { + fn paginate_known_collection_rejects_an_out_of_range_token_limit() { // Negative would otherwise convert to usize::MAX and return everything; - // zero and oversized values fall back to the request's own limit. + // zero and oversized values are not sizes this command ever issues. for bad in [ "limit:-1,offset:0", "limit:0,offset:0", "limit:101,offset:0", ] { - let (page, _) = paginate_known_collection(items(300), 7, 100, Some(bad)); - assert_eq!(page.len(), 7, "{bad}"); + let err = paginate_known_collection(items(300), 7, 100, Some(bad)).expect_err(bad); + assert!(err.to_string().contains("`limit`"), "{bad}: {err}"); } - let (page, _) = paginate_known_collection(items(300), 7, 100, Some("limit:100,offset:0")); + let (page, _) = paginate_known_collection(items(300), 7, 100, Some("limit:100,offset:0")) + .expect("in range"); assert_eq!( page.len(), 100, @@ -542,13 +629,44 @@ mod tests { } #[test] - fn resolve_page_request_ignores_an_out_of_range_token_limit() { - for bad in ["limit:-1,page:2", "limit:0,page:2", "limit:101,page:2"] { - let (page, page_size) = resolve_page_request(None::, Some(bad), 1u32, 25, 100); - assert_eq!((page, page_size), (2, 25), "{bad}"); + fn paginate_known_collection_rejects_a_garbled_or_foreign_token() { + for bad in [ + "limit:5,offs", + "garbage", + "offset:abc", + "offset:-3", + "page:2", + ] { + assert!( + paginate_known_collection(items(10), 5, 100, Some(bad)).is_err(), + "{bad}" + ); + } + // Blank is "start from the beginning", not an error. + for blank in [None, Some(""), Some(" ")] { + let (page, _) = paginate_known_collection(items(10), 5, 100, blank).expect("blank"); + assert_eq!(page.len(), 5); + } + } + + #[test] + fn resolve_page_request_rejects_an_out_of_range_or_garbled_token() { + for bad in [ + "limit:-1,page:2", + "limit:0,page:2", + "limit:101,page:2", + "limit:25,page:abc", + "limit:25,pag", + "offset:5", + ] { + assert!( + resolve_page_request(None::, Some(bad), 1u32, 25, 100).is_err(), + "{bad}" + ); } - let (_, page_size) = - resolve_page_request(None::, Some("limit:100,page:2"), 1u32, 25, 100); - assert_eq!(page_size, 100); + let (page, size) = + resolve_page_request(None::, Some("limit:100,page:2"), 1u32, 25, 100) + .expect("in range"); + assert_eq!((page, size), (2, 100)); } } diff --git a/rust/src/platform/actions/mod.rs b/rust/src/platform/actions/mod.rs index b61a4858..80e5cf17 100644 --- a/rust/src/platform/actions/mod.rs +++ b/rust/src/platform/actions/mod.rs @@ -129,7 +129,7 @@ pub fn group() -> RuntimeGroupSpec { ctx.middleware.cursor_limit, 200, continue_token.as_deref(), - ); + )?; Ok(CommandResult::new(json!(page)) .with_cursor(continuation) .with_next_actions(vec![ From 5669527047a64e2a1960e6a05f3faef2701cfad4 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 22 Oct 2026 16:26:56 -0700 Subject: [PATCH 07/10] fix(cli): keep followed pages out of traces and reject zero page tokens - Hosting listing pages read after the first (domains and subscriptions) no longer put their response body in `--debug transport` output, matching the first page, which already traces status and headers only. - `hosting domain list` leaves `cursor.total` and `cursor.remaining` out whenever it did not read the whole collection, including when a next link could not be followed, not only when it stopped early on purpose. - A `--continue` token with `page:0` is rejected for `dns list` and `email list`, so it can no longer repeat the first page. - The paging tests for these listings move to their own file to keep each test file under the size limit. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/hosting/client.rs | 47 ++- rust/src/hosting/client_paging_tests.rs | 386 ++++++++++++++++++++++++ rust/src/hosting/client_tests.rs | 361 ---------------------- rust/src/hosting/domain/list.rs | 8 +- rust/src/pagination_token.rs | 23 +- 5 files changed, 455 insertions(+), 370 deletions(-) create mode 100644 rust/src/hosting/client_paging_tests.rs diff --git a/rust/src/hosting/client.rs b/rust/src/hosting/client.rs index 22e8dc2f..661d3af3 100644 --- a/rust/src/hosting/client.rs +++ b/rust/src/hosting/client.rs @@ -26,6 +26,14 @@ pub struct CombinedPages { pub stopped_early: bool, } +impl CombinedPages { + /// Whether the items are only part of the collection, for either reason — + /// so their count must not be reported as a total. + pub fn is_partial(&self) -> bool { + self.incomplete || self.stopped_early + } +} + #[derive(Debug, thiserror::Error)] pub enum ClientError { #[error("HTTP error {status}: {body}")] @@ -121,6 +129,12 @@ impl HostingClient { const REDACTED_RESPONSE_BODY: &'static [u8] = b""; + /// Marker written to the `--debug transport` trace in place of a listing + /// page's body, which carries customer data. The generated client already + /// traces only status and headers for these endpoints; pages followed by + /// hand must not be more revealing than the first. + const WITHHELD_RESPONSE_BODY: &'static [u8] = b""; + fn api(&self) -> Result { hosting_client::client_with_builder( crate::http::api_client_builder(), @@ -156,13 +170,18 @@ impl HostingClient { let request = req .build() .map_err(|e| ClientError::Network(e.to_string()))?; - self.execute_json(request).await + self.execute_json(request, true).await } /// Sends an already-built request and decodes the JSON response, with the /// transport debug trace and the non-2xx → [`ClientError::Http`] mapping - /// the hand-built requests here share. - async fn execute_json(&self, request: reqwest::Request) -> Result { + /// the hand-built requests here share. `log_response_body: false` keeps the + /// body out of the trace (status and headers still log). + async fn execute_json( + &self, + request: reqwest::Request, + log_response_body: bool, + ) -> Result { cli_engine::transport::debug_log_reqwest_request(&request); let resp = self .http @@ -176,7 +195,11 @@ impl HostingClient { .bytes() .await .map_err(|e| ClientError::Network(e.to_string()))?; - cli_engine::transport::debug_log_reqwest_response(status, &headers, &bytes); + cli_engine::transport::debug_log_reqwest_response( + status, + &headers, + Self::body_for_trace(log_response_body, &bytes), + ); let status = status.as_u16(); if !(200..300).contains(&status) { @@ -193,6 +216,15 @@ impl HostingClient { serde_json::from_slice(&bytes).map_err(ClientError::Response) } + /// What to hand the transport logger for a response body. + fn body_for_trace(log_response_body: bool, bytes: &[u8]) -> &[u8] { + if log_response_body { + bytes + } else { + Self::WITHHELD_RESPONSE_BODY + } + } + /// GET `path` with the paging parameters a collection's `links[rel=next]` /// carried (`query` is that link's query string, e.g. /// `pageToken=abc&pageSize=50`). @@ -216,7 +248,8 @@ impl HostingClient { .header("x-request-id", Self::new_request_id()) .build() .map_err(|e| ClientError::Network(e.to_string()))?; - self.execute_json(request).await + // A followed listing page holds customer data; trace it like the first. + self.execute_json(request, false).await } // Spec has no request body; Akamai still 411s a POST with no Content-Length. @@ -837,3 +870,7 @@ async fn response( #[cfg(test)] #[path = "client_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "client_paging_tests.rs"] +mod paging_tests; diff --git a/rust/src/hosting/client_paging_tests.rs b/rust/src/hosting/client_paging_tests.rs new file mode 100644 index 00000000..3d0872c0 --- /dev/null +++ b/rust/src/hosting/client_paging_tests.rs @@ -0,0 +1,386 @@ +//! Tests for the hosting list commands' multi-page handling: following `next` +//! links on the domain and subscription listings, stopping early, and keeping +//! followed pages out of the transport trace. + +use httpmock::prelude::*; +use serde_json::json; + +use super::*; + +fn client(base_url: &str) -> HostingClient { + HostingClient::new(base_url, "test-token") +} + +#[tokio::test] +async fn list_domains_without_a_cursor_sends_no_paging_parameters() { + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param_missing("pageToken") + .query_param_missing("pageSize") + .query_param_missing("page"); + then.status(200) + .json_body(json!({ "items": [], "links": [] })); + }) + .await; + + client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("list domains"); + + mock.assert_async().await; +} + +fn domains_first_page(server: &MockServer, body: Value) -> httpmock::Mock<'_> { + server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param_missing("pageToken"); + then.status(200).json_body(body); + }) +} + +fn domain_ids(listed: &CombinedPages) -> Vec<&str> { + listed + .items + .iter() + .map(|item| item["domainId"].as_str().unwrap_or("")) + .collect() +} + +#[tokio::test] +async fn list_domains_combines_pages_by_replaying_the_next_links_query() { + let server = MockServer::start_async().await; + // The link's query is already percent-encoded (`%2B` is a plus, `%3D` an + // equals sign in the opaque token); replaying it must not encode it a second + // time. The path and bearer token are ours, not taken from the link. + let first = domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/apps/app-1/domains?pageToken=a%2Bb%3D%3D&pageSize=25" + }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "a+b==") + .query_param("pageSize", "25") + .header("authorization", "Bearer test-token"); + // `noise` is not in the spec; it must not survive on later pages only. + then.status(200).json_body(json!({ + "items": [{ "domainId": "d2", "noise": 1 }, { "domainId": "d3" }], + "links": [{ "rel": "self", "href": "/v1/hosting/apps/app-1/domains" }] + })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("list domains"); + + first.assert_calls(1); + second.assert_calls(1); + assert_eq!(domain_ids(&listed), ["d1", "d2", "d3"]); + assert!(!listed.incomplete); + assert!( + listed.items.iter().all(|i| i.get("noise").is_none()), + "{:?}", + listed.items + ); +} + +#[tokio::test] +async fn list_domains_stops_reading_pages_once_the_wanted_items_are_held() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }, { "domainId": "d2" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "t2"); + then.status(200) + .json_body(json!({ "items": [{ "domainId": "d3" }], "links": [] })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", Some(2)) + .await + .expect("list domains"); + + second.assert_calls(0); + assert_eq!(domain_ids(&listed), ["d1", "d2"]); + assert!(listed.is_partial(), "a skipped page means no total"); + assert!(listed.stopped_early, "the service still had a page"); + assert!(!listed.incomplete, "stopping on purpose is not a problem"); +} + +#[tokio::test] +async fn list_domains_reads_on_while_the_wanted_items_are_not_yet_held() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }, { "domainId": "d2" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "t2"); + then.status(200) + .json_body(json!({ "items": [{ "domainId": "d3" }], "links": [] })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", Some(3)) + .await + .expect("list domains"); + + second.assert_calls(1); + assert_eq!(domain_ids(&listed), ["d1", "d2", "d3"]); + assert!( + !listed.is_partial(), + "that was the last page, so it is the whole list" + ); + assert!(!listed.stopped_early); + assert!(!listed.incomplete); +} + +#[tokio::test] +async fn list_domains_does_not_report_an_early_stop_when_nothing_more_exists() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ "items": [{ "domainId": "d1" }, { "domainId": "d2" }], "links": [] }), + ); + + let listed = client(&server.base_url()) + .list_domains("app-1", Some(1)) + .await + .expect("list domains"); + + // More held than wanted, but there was no further page to skip. + assert_eq!(domain_ids(&listed), ["d1", "d2"]); + assert!(!listed.stopped_early); + assert!(!listed.incomplete); +} + +#[tokio::test] +async fn list_domains_stops_at_a_repeated_next_link_and_reports_it() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=loop" }] + }), + ); + let looping = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/apps/app-1/domains") + .query_param("pageToken", "loop"); + then.status(200).json_body(json!({ + "items": [{ "domainId": "d2" }], + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=loop" }] + })); + }); + + let listed = client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("a loop must not fail the listing"); + + looping.assert_calls(1); + assert_eq!(domain_ids(&listed), ["d1", "d2"]); + assert!(listed.incomplete, "pages were left unread"); + assert!(listed.is_partial()); +} + +#[tokio::test] +async fn list_domains_reports_a_next_link_it_cannot_follow() { + let server = MockServer::start_async().await; + domains_first_page( + &server, + json!({ + "items": [{ "domainId": "d1" }], + // No query to replay, so following it could never advance. + "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains" }] + }), + ); + + let listed = client(&server.base_url()) + .list_domains("app-1", None) + .await + .expect("list domains"); + + assert_eq!(domain_ids(&listed), ["d1"]); + assert!(listed.incomplete); + assert!(listed.is_partial(), "an unreadable page means no total"); +} + +#[test] +fn followed_pages_keep_their_body_out_of_the_transport_trace() { + let body = br#"{"items":[{"hostname":"customer.example"}]}"#; + assert_eq!(HostingClient::body_for_trace(true, body), body.as_slice()); + let traced = HostingClient::body_for_trace(false, body); + assert!(!String::from_utf8_lossy(traced).contains("customer.example")); + assert!(String::from_utf8_lossy(traced).contains("withheld")); +} + +fn subscriptions_first_page(server: &MockServer, body: Value) -> httpmock::Mock<'_> { + server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("hostingProduct", "WEB_HOSTING") + .query_param_missing("pageToken"); + then.status(200).json_body(body); + }) +} + +fn subscription_ids(listed: &CombinedPages) -> Vec<&str> { + listed + .items + .iter() + .map(|item| item["subscriptionId"].as_str().unwrap_or("")) + .collect() +} + +#[tokio::test] +async fn list_subscriptions_combines_pages_by_following_next_links() { + let server = MockServer::start_async().await; + let first = subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=t2" + }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("hostingProduct", "WEB_HOSTING") + .query_param("pageToken", "t2") + .header("authorization", "Bearer test-token"); + // `noise` is not in the spec; it must not survive on later pages only. + then.status(200).json_body(json!({ + "items": [{ "subscriptionId": "s2", "noise": 1 }, { "subscriptionId": "s3" }], + "links": [{ "rel": "self", "href": "/v1/hosting/subscriptions" }] + })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + first.assert_calls(1); + second.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1", "s2", "s3"]); + assert!(!listed.incomplete); + assert!( + listed.items.iter().all(|i| i.get("noise").is_none()), + "{:?}", + listed.items + ); +} + +#[tokio::test] +async fn list_subscriptions_keeps_the_required_filter_when_a_next_link_drops_it() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("pageToken", "t2") + .query_param("hostingProduct", "WEB_HOSTING"); + then.status(200) + .json_body(json!({ "items": [{ "subscriptionId": "s2" }], "links": [] })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + second.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1", "s2"]); +} + +#[tokio::test] +async fn list_subscriptions_stops_at_a_repeated_next_link_and_reports_it() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=loop" + }] + }), + ); + let looping = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("pageToken", "loop"); + then.status(200).json_body(json!({ + "items": [{ "subscriptionId": "s2" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=loop" + }] + })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("a loop must not fail the listing"); + + looping.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1", "s2"]); + assert!(listed.incomplete, "pages were left unread"); +} + +#[tokio::test] +async fn list_subscriptions_reports_a_next_link_it_cannot_follow() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + // No query to replay, so following it could never advance. + "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions" }] + }), + ); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + assert_eq!(subscription_ids(&listed), ["s1"]); + assert!(listed.incomplete); +} diff --git a/rust/src/hosting/client_tests.rs b/rust/src/hosting/client_tests.rs index 1bd9dfbe..639c66ba 100644 --- a/rust/src/hosting/client_tests.rs +++ b/rust/src/hosting/client_tests.rs @@ -475,222 +475,6 @@ async fn list_domains_hits_correct_path() { mock.assert_async().await; } -#[tokio::test] -async fn list_domains_without_a_cursor_sends_no_paging_parameters() { - let server = MockServer::start_async().await; - let mock = server - .mock_async(|when, then| { - when.method(GET) - .path("/v1/hosting/apps/app-1/domains") - .query_param_missing("pageToken") - .query_param_missing("pageSize") - .query_param_missing("page"); - then.status(200) - .json_body(json!({ "items": [], "links": [] })); - }) - .await; - - client(&server.base_url()) - .list_domains("app-1", None) - .await - .expect("list domains"); - - mock.assert_async().await; -} - -fn domains_first_page(server: &MockServer, body: Value) -> httpmock::Mock<'_> { - server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/apps/app-1/domains") - .query_param_missing("pageToken"); - then.status(200).json_body(body); - }) -} - -fn domain_ids(listed: &CombinedPages) -> Vec<&str> { - listed - .items - .iter() - .map(|item| item["domainId"].as_str().unwrap_or("")) - .collect() -} - -#[tokio::test] -async fn list_domains_combines_pages_by_replaying_the_next_links_query() { - let server = MockServer::start_async().await; - // The link's query is already percent-encoded (`%2B` is a plus, `%3D` an - // equals sign in the opaque token); replaying it must not encode it a second - // time. The path and bearer token are ours, not taken from the link. - let first = domains_first_page( - &server, - json!({ - "items": [{ "domainId": "d1" }], - "links": [{ - "rel": "next", - "href": "/v1/hosting/apps/app-1/domains?pageToken=a%2Bb%3D%3D&pageSize=25" - }] - }), - ); - let second = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/apps/app-1/domains") - .query_param("pageToken", "a+b==") - .query_param("pageSize", "25") - .header("authorization", "Bearer test-token"); - // `noise` is not in the spec; it must not survive on later pages only. - then.status(200).json_body(json!({ - "items": [{ "domainId": "d2", "noise": 1 }, { "domainId": "d3" }], - "links": [{ "rel": "self", "href": "/v1/hosting/apps/app-1/domains" }] - })); - }); - - let listed = client(&server.base_url()) - .list_domains("app-1", None) - .await - .expect("list domains"); - - first.assert_calls(1); - second.assert_calls(1); - assert_eq!(domain_ids(&listed), ["d1", "d2", "d3"]); - assert!(!listed.incomplete); - assert!( - listed.items.iter().all(|i| i.get("noise").is_none()), - "{:?}", - listed.items - ); -} - -#[tokio::test] -async fn list_domains_stops_reading_pages_once_the_wanted_items_are_held() { - let server = MockServer::start_async().await; - domains_first_page( - &server, - json!({ - "items": [{ "domainId": "d1" }, { "domainId": "d2" }], - "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=t2" }] - }), - ); - let second = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/apps/app-1/domains") - .query_param("pageToken", "t2"); - then.status(200) - .json_body(json!({ "items": [{ "domainId": "d3" }], "links": [] })); - }); - - let listed = client(&server.base_url()) - .list_domains("app-1", Some(2)) - .await - .expect("list domains"); - - second.assert_calls(0); - assert_eq!(domain_ids(&listed), ["d1", "d2"]); - assert!(listed.stopped_early, "the service still had a page"); - assert!(!listed.incomplete, "stopping on purpose is not a problem"); -} - -#[tokio::test] -async fn list_domains_reads_on_while_the_wanted_items_are_not_yet_held() { - let server = MockServer::start_async().await; - domains_first_page( - &server, - json!({ - "items": [{ "domainId": "d1" }, { "domainId": "d2" }], - "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=t2" }] - }), - ); - let second = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/apps/app-1/domains") - .query_param("pageToken", "t2"); - then.status(200) - .json_body(json!({ "items": [{ "domainId": "d3" }], "links": [] })); - }); - - let listed = client(&server.base_url()) - .list_domains("app-1", Some(3)) - .await - .expect("list domains"); - - second.assert_calls(1); - assert_eq!(domain_ids(&listed), ["d1", "d2", "d3"]); - assert!( - !listed.stopped_early, - "that was the last page, so it is the whole list" - ); - assert!(!listed.incomplete); -} - -#[tokio::test] -async fn list_domains_does_not_report_an_early_stop_when_nothing_more_exists() { - let server = MockServer::start_async().await; - domains_first_page( - &server, - json!({ "items": [{ "domainId": "d1" }, { "domainId": "d2" }], "links": [] }), - ); - - let listed = client(&server.base_url()) - .list_domains("app-1", Some(1)) - .await - .expect("list domains"); - - // More held than wanted, but there was no further page to skip. - assert_eq!(domain_ids(&listed), ["d1", "d2"]); - assert!(!listed.stopped_early); - assert!(!listed.incomplete); -} - -#[tokio::test] -async fn list_domains_stops_at_a_repeated_next_link_and_reports_it() { - let server = MockServer::start_async().await; - domains_first_page( - &server, - json!({ - "items": [{ "domainId": "d1" }], - "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=loop" }] - }), - ); - let looping = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/apps/app-1/domains") - .query_param("pageToken", "loop"); - then.status(200).json_body(json!({ - "items": [{ "domainId": "d2" }], - "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains?pageToken=loop" }] - })); - }); - - let listed = client(&server.base_url()) - .list_domains("app-1", None) - .await - .expect("a loop must not fail the listing"); - - looping.assert_calls(1); - assert_eq!(domain_ids(&listed), ["d1", "d2"]); - assert!(listed.incomplete, "pages were left unread"); -} - -#[tokio::test] -async fn list_domains_reports_a_next_link_it_cannot_follow() { - let server = MockServer::start_async().await; - domains_first_page( - &server, - json!({ - "items": [{ "domainId": "d1" }], - // No query to replay, so following it could never advance. - "links": [{ "rel": "next", "href": "/v1/hosting/apps/app-1/domains" }] - }), - ); - - let listed = client(&server.base_url()) - .list_domains("app-1", None) - .await - .expect("list domains"); - - assert_eq!(domain_ids(&listed), ["d1"]); - assert!(listed.incomplete); -} - #[tokio::test] async fn attach_domain_sends_hostname() { let server = MockServer::start_async().await; @@ -752,151 +536,6 @@ async fn list_subscriptions_hits_correct_path() { mock.assert_async().await; } -fn subscriptions_first_page(server: &MockServer, body: Value) -> httpmock::Mock<'_> { - server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/subscriptions") - .query_param("hostingProduct", "WEB_HOSTING") - .query_param_missing("pageToken"); - then.status(200).json_body(body); - }) -} - -fn subscription_ids(listed: &CombinedPages) -> Vec<&str> { - listed - .items - .iter() - .map(|item| item["subscriptionId"].as_str().unwrap_or("")) - .collect() -} - -#[tokio::test] -async fn list_subscriptions_combines_pages_by_following_next_links() { - let server = MockServer::start_async().await; - let first = subscriptions_first_page( - &server, - json!({ - "items": [{ "subscriptionId": "s1" }], - "links": [{ - "rel": "next", - "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=t2" - }] - }), - ); - let second = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/subscriptions") - .query_param("hostingProduct", "WEB_HOSTING") - .query_param("pageToken", "t2") - .header("authorization", "Bearer test-token"); - // `noise` is not in the spec; it must not survive on later pages only. - then.status(200).json_body(json!({ - "items": [{ "subscriptionId": "s2", "noise": 1 }, { "subscriptionId": "s3" }], - "links": [{ "rel": "self", "href": "/v1/hosting/subscriptions" }] - })); - }); - - let listed = client(&server.base_url()) - .list_subscriptions("WEB_HOSTING") - .await - .expect("list subscriptions"); - - first.assert_calls(1); - second.assert_calls(1); - assert_eq!(subscription_ids(&listed), ["s1", "s2", "s3"]); - assert!(!listed.incomplete); - assert!( - listed.items.iter().all(|i| i.get("noise").is_none()), - "{:?}", - listed.items - ); -} - -#[tokio::test] -async fn list_subscriptions_keeps_the_required_filter_when_a_next_link_drops_it() { - let server = MockServer::start_async().await; - subscriptions_first_page( - &server, - json!({ - "items": [{ "subscriptionId": "s1" }], - "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions?pageToken=t2" }] - }), - ); - let second = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/subscriptions") - .query_param("pageToken", "t2") - .query_param("hostingProduct", "WEB_HOSTING"); - then.status(200) - .json_body(json!({ "items": [{ "subscriptionId": "s2" }], "links": [] })); - }); - - let listed = client(&server.base_url()) - .list_subscriptions("WEB_HOSTING") - .await - .expect("list subscriptions"); - - second.assert_calls(1); - assert_eq!(subscription_ids(&listed), ["s1", "s2"]); -} - -#[tokio::test] -async fn list_subscriptions_stops_at_a_repeated_next_link_and_reports_it() { - let server = MockServer::start_async().await; - subscriptions_first_page( - &server, - json!({ - "items": [{ "subscriptionId": "s1" }], - "links": [{ - "rel": "next", - "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=loop" - }] - }), - ); - let looping = server.mock(|when, then| { - when.method(GET) - .path("/v1/hosting/subscriptions") - .query_param("pageToken", "loop"); - then.status(200).json_body(json!({ - "items": [{ "subscriptionId": "s2" }], - "links": [{ - "rel": "next", - "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=loop" - }] - })); - }); - - let listed = client(&server.base_url()) - .list_subscriptions("WEB_HOSTING") - .await - .expect("a loop must not fail the listing"); - - looping.assert_calls(1); - assert_eq!(subscription_ids(&listed), ["s1", "s2"]); - assert!(listed.incomplete, "pages were left unread"); -} - -#[tokio::test] -async fn list_subscriptions_reports_a_next_link_it_cannot_follow() { - let server = MockServer::start_async().await; - subscriptions_first_page( - &server, - json!({ - "items": [{ "subscriptionId": "s1" }], - // No query to replay, so following it could never advance. - "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions" }] - }), - ); - - let listed = client(&server.base_url()) - .list_subscriptions("WEB_HOSTING") - .await - .expect("list subscriptions"); - - assert_eq!(subscription_ids(&listed), ["s1"]); - assert!(listed.incomplete); -} - #[tokio::test] async fn attach_subscription_sends_subscription_id() { let server = MockServer::start_async().await; diff --git a/rust/src/hosting/domain/list.rs b/rust/src/hosting/domain/list.rs index 10ee43d0..1369c219 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -65,6 +65,7 @@ pub(super) fn command() -> RuntimeCommandSpec { if listed.incomplete { warn_user(&incomplete_list_warning("domains")); } + let partial = listed.is_partial(); let (page, continuation) = pagination_token::paginate_known_collection( pagination_token::filter_known_collection(listed.items, &ctx.middleware.filter)?, @@ -73,9 +74,10 @@ pub(super) fn command() -> RuntimeCommandSpec { continue_token.as_deref(), )?; - // Pages past the window were deliberately not read, so the length of - // what was read says nothing about the total. - let continuation = if listed.stopped_early { + // Pages past the window were left unread, on purpose or because a + // link could not be followed, so the length of what was read says + // nothing about the total. + let continuation = if partial { pagination_token::forget_totals(continuation) } else { continuation diff --git a/rust/src/pagination_token.rs b/rust/src/pagination_token.rs index d9dfa0d4..82925758 100644 --- a/rust/src/pagination_token.rs +++ b/rust/src/pagination_token.rs @@ -177,7 +177,15 @@ pub(crate) fn resolve_page_request( ) -> cli_engine::Result<(P, i64)> { let fields = parse(continue_token, &["limit", "page"])?; let page = match fields.typed::

("page")? { - Some(page) => page, + Some(page) => { + // Pages are 1-based, and a token is only ever minted as `page + 1`. + // Zero would be clamped to page one by a caller that cannot send + // it, then advertise page one again as the next page. + if fields.typed::("page")? == Some(0) { + return Err(invalid_token(&fields.raw, "`page` must be at least 1")); + } + page + } None => explicit_page.unwrap_or(default_page), }; let page_size = limit_from(&fields, limit, max_limit)?; @@ -649,6 +657,18 @@ mod tests { } } + #[test] + fn resolve_page_request_rejects_a_zero_page_token_for_every_page_type() { + // dns list reads the page as u64 and clamps it, email list as u32; either + // would otherwise act on page one while the token claims page zero. + let err = resolve_page_request(None::, Some("limit:25,page:0"), 1u64, 25, 100) + .expect_err("zero page"); + assert!(err.to_string().contains("at least 1"), "{err}"); + assert!(resolve_page_request(None::, Some("limit:25,page:0"), 1u32, 25, 100).is_err()); + // A valid token page passes (an explicit --page is validated by clap). + assert!(resolve_page_request(None::, Some("limit:25,page:1"), 1u64, 25, 100).is_ok()); + } + #[test] fn resolve_page_request_rejects_an_out_of_range_or_garbled_token() { for bad in [ @@ -656,6 +676,7 @@ mod tests { "limit:0,page:2", "limit:101,page:2", "limit:25,page:abc", + "limit:25,page:0", "limit:25,pag", "offset:5", ] { From 0d946d37d75a71b3c4f35f9fe3997109ca01308c Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 22 Oct 2026 16:36:44 -0700 Subject: [PATCH 08/10] fix(cli): compare repeated hosting page requests as sent, and validate empty-search tokens - A hosting listing now recognises a repeated page after the required filter has been added to the link, so a link that omits it and a later one that includes it are not read twice. Previously the same plans could be counted twice and inflate the subscription slot totals. - `api search` handles `--continue` the same way when nothing matches: a malformed token is rejected and a valid one's page size is reported. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/api/search.rs | 57 ++++++++++++++++++++++--- rust/src/hosting/client.rs | 9 ++-- rust/src/hosting/client_paging_tests.rs | 41 ++++++++++++++++++ 3 files changed, 99 insertions(+), 8 deletions(-) diff --git a/rust/src/api/search.rs b/rust/src/api/search.rs index 215673a1..f2a661e7 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -55,13 +55,19 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_output_schema::() .with_cursor(CursorConfig::new(50, MAX_LIMIT)), |ctx, args: SearchArgs| async move { + let continue_token = pagination_token::resolve_continue_token( + args.offset, + ctx.middleware.continue_token.as_deref(), + ); let hits = search_endpoints(catalog(), &args.query); if hits.is_empty() { + // Same token handling as a search with results: a malformed + // token is rejected, and a valid one's page size is reported. let (page, continuation) = pagination_token::paginate_known_collection( Vec::new(), ctx.middleware.cursor_limit, MAX_LIMIT, - None, + continue_token.as_deref(), )?; return Ok(CommandResult::new(json!(page)).with_cursor(continuation)); } @@ -127,10 +133,6 @@ pub(super) fn command() -> RuntimeCommandSpec { ); } - let continue_token = pagination_token::resolve_continue_token( - args.offset, - ctx.middleware.continue_token.as_deref(), - ); let (page, continuation) = pagination_token::paginate_known_collection( pagination_token::filter_known_collection(results, &ctx.middleware.filter)?, ctx.middleware.cursor_limit, @@ -163,6 +165,51 @@ mod tests { ) } + /// A search with no matches takes the same `--continue` handling as one + /// with results: a malformed token is rejected rather than ignored, and a + /// valid one's page size is what the cursor reports. + #[tokio::test] + async fn an_empty_search_validates_and_honours_the_continue_token() { + let cli = test_cli(); + let nothing = "zzqqxxnomatchzzqqxx"; + + let bad = cli + .run([ + "gddy", + "api", + "search", + nothing, + "--continue", + "limit:1,offs", + "--output", + "json", + ]) + .await; + assert_ne!(bad.exit_code, 0, "{}", bad.rendered); + assert!( + bad.rendered.contains("invalid --continue value"), + "{}", + bad.rendered + ); + + let ok = cli + .run([ + "gddy", + "api", + "search", + nothing, + "--continue", + "limit:1,offset:1", + "--output", + "json", + ]) + .await; + assert_eq!(ok.exit_code, 0, "{}", ok.rendered); + let ok: serde_json::Value = serde_json::from_str(&ok.rendered).expect("valid json"); + assert_eq!(ok["data"].as_array().expect("array").len(), 0); + assert_eq!(ok["cursor"]["limit"], serde_json::json!(1)); + } + #[tokio::test] async fn limit_and_continue_page_through_search_results() { let cli = test_cli(); diff --git a/rust/src/hosting/client.rs b/rust/src/hosting/client.rs index 661d3af3..458d8ba3 100644 --- a/rust/src/hosting/client.rs +++ b/rust/src/hosting/client.rs @@ -764,9 +764,6 @@ impl HostingClient { let Some(mut query) = super::common::next_link_query(&page) else { break; }; - if !seen.insert(query.clone()) { - break; - } for (name, value) in required { if !query .split('&') @@ -777,6 +774,12 @@ impl HostingClient { query.push_str(&format!("&{name}={value}")); } } + // Compare the request actually sent: a link that omits a required + // parameter and a later one that includes it are the same request, + // and reading it twice would count its items twice. + if !seen.insert(query.clone()) { + break; + } let raw = self.get_with_query(path, &query).await?; let typed: T = serde_json::from_value(raw).map_err(ClientError::Response)?; page = serde_json::to_value(typed).map_err(ClientError::Response)?; diff --git a/rust/src/hosting/client_paging_tests.rs b/rust/src/hosting/client_paging_tests.rs index 3d0872c0..f048d394 100644 --- a/rust/src/hosting/client_paging_tests.rs +++ b/rust/src/hosting/client_paging_tests.rs @@ -328,6 +328,47 @@ async fn list_subscriptions_keeps_the_required_filter_when_a_next_link_drops_it( assert_eq!(subscription_ids(&listed), ["s1", "s2"]); } +#[tokio::test] +async fn list_subscriptions_does_not_read_the_same_request_twice_when_a_link_adds_the_filter() { + // The first link omits `hostingProduct` (we add it); the next one carries + // it explicitly but is otherwise the identical request. Reading it again + // would count the same plans twice and inflate the slot totals. + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("pageToken", "t2") + .query_param("hostingProduct", "WEB_HOSTING"); + then.status(200).json_body(json!({ + "items": [{ "subscriptionId": "s2" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?pageToken=t2&hostingProduct=WEB_HOSTING" + }] + })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + second.assert_calls(1); + assert_eq!( + subscription_ids(&listed), + ["s1", "s2"], + "no duplicated plans" + ); + assert!(listed.incomplete, "the loop was cut short, so say so"); +} + #[tokio::test] async fn list_subscriptions_stops_at_a_repeated_next_link_and_reports_it() { let server = MockServer::start_async().await; From ec3c0338934e992303379178c4b6eafd2191326c Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 22 Oct 2026 16:44:13 -0700 Subject: [PATCH 09/10] fix(hosting): never re-read the first or a reordered page of a listing When a hosting listing is read across several pages, the request that produced the first page now counts as already read, and two requests are compared by their set of parameters rather than their order. A next link that points back at the first page, or repeats a page with its parameters in another order, ends the listing instead of fetching those plans again and inflating the subscription slot totals. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/hosting/client.rs | 30 +++++++++-- rust/src/hosting/client_paging_tests.rs | 71 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/rust/src/hosting/client.rs b/rust/src/hosting/client.rs index 458d8ba3..8e3acb86 100644 --- a/rust/src/hosting/client.rs +++ b/rust/src/hosting/client.rs @@ -754,7 +754,17 @@ impl HostingClient { { let mut page = first; let mut items = super::common::page_items(&page); - let mut seen = std::collections::HashSet::new(); + // The request that produced `first` counts as already read, so a link + // pointing back at it cannot be fetched (and its items counted) again. + let first_query: Vec = required + .iter() + .map(|(name, value)| { + let value: String = + url::form_urlencoded::byte_serialize(value.as_bytes()).collect(); + format!("{name}={value}") + }) + .collect(); + let mut seen = std::collections::HashSet::from([request_key(&first_query.join("&"))]); let mut stopped_early = false; for _ in 1..MAX_COMBINED_PAGES { if want.is_some_and(|want| items.len() >= want) { @@ -774,10 +784,11 @@ impl HostingClient { query.push_str(&format!("&{name}={value}")); } } - // Compare the request actually sent: a link that omits a required - // parameter and a later one that includes it are the same request, - // and reading it twice would count its items twice. - if !seen.insert(query.clone()) { + // Compare the request actually sent, regardless of parameter order: + // a link that omits a required parameter and a later one that + // includes it, or the same parameters in another order, are the same + // request, and reading it twice would count its items twice. + if !seen.insert(request_key(&query)) { break; } let raw = self.get_with_query(path, &query).await?; @@ -870,6 +881,15 @@ async fn response( } } +/// A query string's identity for repeat detection: its `name=value` pairs +/// sorted, so the same request in a different parameter order compares equal. +/// Used only as a key; the query actually sent is left as the service wrote it. +fn request_key(query: &str) -> String { + let mut pairs: Vec<&str> = query.split('&').filter(|pair| !pair.is_empty()).collect(); + pairs.sort_unstable(); + pairs.join("&") +} + #[cfg(test)] #[path = "client_tests.rs"] mod tests; diff --git a/rust/src/hosting/client_paging_tests.rs b/rust/src/hosting/client_paging_tests.rs index f048d394..7b952b9d 100644 --- a/rust/src/hosting/client_paging_tests.rs +++ b/rust/src/hosting/client_paging_tests.rs @@ -369,6 +369,77 @@ async fn list_subscriptions_does_not_read_the_same_request_twice_when_a_link_add assert!(listed.incomplete, "the loop was cut short, so say so"); } +#[tokio::test] +async fn list_subscriptions_does_not_read_a_page_again_when_parameters_are_reordered() { + let server = MockServer::start_async().await; + subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1" }], + "links": [{ "rel": "next", "href": "/v1/hosting/subscriptions?pageToken=t2" }] + }), + ); + let second = server.mock(|when, then| { + when.method(GET) + .path("/v1/hosting/subscriptions") + .query_param("pageToken", "t2"); + then.status(200).json_body(json!({ + "items": [{ "subscriptionId": "s2" }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING&pageToken=t2" + }] + })); + }); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + second.assert_calls(1); + assert_eq!( + subscription_ids(&listed), + ["s1", "s2"], + "no duplicated plans" + ); + assert!(listed.incomplete); +} + +#[tokio::test] +async fn list_subscriptions_does_not_read_the_first_request_again() { + // A link back to the request that produced the first page must not be + // fetched: its plans would be appended a second time. + let server = MockServer::start_async().await; + let first = subscriptions_first_page( + &server, + json!({ + "items": [{ "subscriptionId": "s1", "availableSlots": 2 }], + "links": [{ + "rel": "next", + "href": "/v1/hosting/subscriptions?hostingProduct=WEB_HOSTING" + }] + }), + ); + + let listed = client(&server.base_url()) + .list_subscriptions("WEB_HOSTING") + .await + .expect("list subscriptions"); + + first.assert_calls(1); + assert_eq!(subscription_ids(&listed), ["s1"], "no duplicated plans"); + assert!(listed.incomplete); +} + +#[test] +fn request_key_ignores_parameter_order_but_not_values() { + assert_eq!(request_key("a=1&b=2"), request_key("b=2&a=1")); + assert_ne!(request_key("a=1&b=2"), request_key("a=1&b=3")); + assert_ne!(request_key("a=1"), request_key("a=1&b=2")); + assert_eq!(request_key(""), request_key("&")); +} + #[tokio::test] async fn list_subscriptions_stops_at_a_repeated_next_link_and_reports_it() { let server = MockServer::start_async().await; From 1f992d33cfa983f198cdfc4d6fdc3b5dbd4c4113 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 22 Oct 2026 17:00:06 -0700 Subject: [PATCH 10/10] fix(hosting): decode page tokens from next links before reuse The page token read from a hosting `next` link is now percent-decoded once. It is passed on to the generated client, which encodes it again, so a token that was left encoded reached the service as a different value (`a%252Bb` instead of `a%2Bb`) and the next page could not be fetched. This affects the application, deployment and log lists' `--continue` tokens. Co-Authored-By: Claude Sonnet 5.5 --- rust/src/hosting/client_tests.rs | 23 +++++++++++++++++++++++ rust/src/hosting/common.rs | 32 +++++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/rust/src/hosting/client_tests.rs b/rust/src/hosting/client_tests.rs index 639c66ba..488042ae 100644 --- a/rust/src/hosting/client_tests.rs +++ b/rust/src/hosting/client_tests.rs @@ -30,6 +30,29 @@ async fn list_apps_sends_app_type_query_param() { assert_eq!(body["items"], json!([])); } +#[tokio::test] +async fn list_apps_sends_a_decoded_page_token_exactly_once_encoded() { + // A cursor taken from a next link and decoded (see `next_page_token`) must + // reach the service as the same cursor: encoded once, not twice. + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET) + .path("/v1/hosting/apps") + .query_param("pageToken", "a+b=="); + then.status(200) + .json_body(json!({ "items": [], "links": [] })); + }) + .await; + + client(&server.base_url()) + .list_apps("NODEJS", Some("a+b=="), None) + .await + .expect("list apps"); + + mock.assert_async().await; +} + #[tokio::test] async fn list_apps_sends_page_token_and_limit() { let server = MockServer::start_async().await; diff --git a/rust/src/hosting/common.rs b/rust/src/hosting/common.rs index d4c0cf10..47f1a89c 100644 --- a/rust/src/hosting/common.rs +++ b/rust/src/hosting/common.rs @@ -332,16 +332,16 @@ pub fn page_limit(cursor_limit: i64) -> Option { u32::try_from(cursor_limit).ok().filter(|n| *n > 0) } +/// The decoded value of query parameter `param` in `url`. +/// +/// Decoded, because the value is handed to the generated client, which encodes +/// it again when it builds the next request; passing it on still encoded would +/// send `a%252Bb` for a cursor that is really `a%2Bb`. fn extract_query_param(url: &str, param: &str) -> Option { - let query = url.split_once('?')?.1; - for pair in query.split('&') { - if let Some((k, v)) = pair.split_once('=') - && k == param - { - return Some(v.to_owned()); - } - } - None + let query = url.split_once('?')?.1.split('#').next()?; + url::form_urlencoded::parse(query.as_bytes()) + .find(|(name, _)| name == param) + .map(|(_, value)| value.into_owned()) } #[cfg(test)] @@ -477,6 +477,20 @@ mod tests { ); } + #[test] + fn next_page_token_is_decoded_so_it_is_not_encoded_twice() { + let response = json!({ "links": [{ + "rel": "next", + "href": "/v1/hosting/apps?appType=NODEJS&pageToken=a%2Bb%3D%3D&pageSize=10#frag" + }]}); + assert_eq!(next_page_token(&response).as_deref(), Some("a+b==")); + // Percent signs and spaces survive the same way. + let response = json!({ "links": [{ + "rel": "next", "href": "/x?pageToken=100%25%20done" + }]}); + assert_eq!(next_page_token(&response).as_deref(), Some("100% done")); + } + #[test] fn page_items_returns_the_items_array_or_empty() { assert_eq!(