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..4eb6b8d4 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. Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] + 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,17 @@ 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( + pagination_token::filter_known_collection(items, &ctx.middleware.filter)?, + ctx.middleware.cursor_limit, + OPERATION_CHILD_LIST_MAX_LIMIT, + continue_token.as_deref(), + )?; let next_actions = vec![ next_action( "api parameter get --operation ", @@ -62,7 +78,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,31 +233,31 @@ 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. + /// 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_limit_zero_means_all() { - let output = operation_cli() + async fn parameter_list_next_action_after_an_offset_jump_advances() { + let first = operation_cli() .run([ "gddy", "api", @@ -248,16 +266,38 @@ mod tests { "--operation", "get_transaction_disputes", "--limit", - "0", + "5", + "--offset", + "20", "--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); + 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 diff --git a/rust/src/api/response.rs b/rust/src/api/response.rs index 6d6b9009..34027aef 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. Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] + 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,26 @@ 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( + pagination_token::filter_known_collection(items, &ctx.middleware.filter)?, + ctx.middleware.cursor_limit, + OPERATION_CHILD_LIST_MAX_LIMIT, + continue_token.as_deref(), + )?; let next_actions = vec![ next_action( "api response get --operation ", @@ -54,7 +70,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..f2a661e7 100644 --- a/rust/src/api/search.rs +++ b/rust/src/api/search.rs @@ -1,15 +1,19 @@ //! `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}; +/// Largest page `api search` returns (its cursor `max_limit`). +const MAX_LIMIT: i64 = 100; + output_schema!(ApiEndpoint { "domain": "string"; "operationId": "string"; @@ -28,10 +32,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. Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] + 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 +53,23 @@ 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, 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() { - return Ok(CommandResult::new(json!([]))); + // 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, + continue_token.as_deref(), + )?; + return Ok(CommandResult::new(json!(page)).with_cursor(continuation)); } let mut results: Vec = hits .iter() @@ -112,22 +133,130 @@ pub(super) fn command() -> RuntimeCommandSpec { ); } - Ok(CommandResult::new(json!(results)).with_next_actions(next_actions)) + let (page, continuation) = pagination_token::paginate_known_collection( + pagination_token::filter_known_collection(results, &ctx.middleware.filter)?, + ctx.middleware.cursor_limit, + MAX_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()), + ) + } + + /// 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(); + // "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 d370785f..40788032 100644 --- a/rust/src/dns/list.rs +++ b/rust/src/dns/list.rs @@ -1,18 +1,19 @@ //! `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::idn::DomainFormArgs; +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 { @@ -29,6 +30,10 @@ struct ListArgs { #[arg(long, value_name = "NAME", requires = "record_type", value_parser = crate::idn::parse_name_arg)] name: Option, + /// 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, + #[command(flatten)] domain_format: DomainFormArgs, } @@ -72,7 +77,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; @@ -80,26 +85,67 @@ 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, + MAX_PAGE_SIZE.get() as i64, + )?; + let fetched = fetch_records_page( &client, domain.as_str(), type_opt.as_deref(), name_opt.as_deref(), + page, + page_size, debug, ) .await?; - let mut out = build_list_output(&records).map_err(|e| { + let mut out = build_list_output(&fetched.records).map_err(|e| { CliCoreError::message(format!("failed to serialize DNS records: {e}")) })?; for record in &mut out { args.domain_format.form().convert_fields(record, &["name"]); } - Ok(CommandResult::new(json!(out))) + let mut continuation = if fetched.has_more { + CursorContinuation::more(pagination_token::build(&[ + ("limit", &page_size.to_string()), + ("page", &(page + 1).to_string()), + ])) + } 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`. @@ -124,13 +170,29 @@ 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 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_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 68adc683..c6fab688 100644 --- a/rust/src/dns/records.rs +++ b/rust/src/dns/records.rs @@ -25,13 +25,17 @@ 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; + /// The output fields of a DNS write that hold the zone and the record name. pub(super) const ZONE_AND_NAME: &[&str] = &["domain", "name"]; -/// 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. @@ -394,12 +398,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())); @@ -419,7 +427,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; } @@ -428,6 +436,61 @@ 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, + zone: &str, + type_: Option<&str>, + name: Option<&str>, + page: u64, + page_size: i64, + debug: bool, +) -> 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); + 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); + 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 /// the domain/type/name the write just touched. pub(super) fn verify_with_list_action(domain: &str, record_type: &str, name: &str) -> NextAction { @@ -733,4 +796,117 @@ 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, + "totalItems": 3, + })); + }) + .await; + let client = domains_client::client_with_builder( + reqwest::Client::builder(), + &server.base_url(), + "Bearer tok", + "req-1", + ) + .expect("build client"); + + let page = fetch_records_page(&client, "example.com", None, None, 1, 2, false) + .await + .expect("fetch succeeds"); + + 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; + } + + #[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 page = fetch_records_page( + &client, + "example.com", + None, + None, + 3, + MAX_PAGE_SIZE.get() as i64, + false, + ) + .await + .expect("fetch succeeds"); + + assert_eq!(page.records.len(), 1); + assert!( + !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; + } + + #[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 957fc854..fab7defe 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; @@ -11,6 +11,7 @@ use domains_client::types; use super::common::{api_error, comma_joined, make_client}; use crate::idn::DomainFormArgs; 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 — @@ -19,18 +20,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 @@ -156,85 +149,87 @@ 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); - } +) -> 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 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); + } + 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) => { + pagination_token::build(&[("token", token), ("direction", &direction.to_string())]) } - 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() - ))); - } - }; + None => pagination_token::build(&[("token", token)]), + } +} + +/// Decodes a `--continue` value produced by [`encode_resume`]. An absent or +/// blank value starts from the beginning; anything else must be a token this +/// command issued — a `token` field and, if present, one of the API's known +/// `direction` values — or it is rejected, since quietly restarting from page +/// one would repeat rows without any sign that the value was wrong. +fn decode_resume( + continue_token: Option<&str>, +) -> Result)>> { + let fields = pagination_token::parse(continue_token, &["token", "direction"])?; + if continue_token.is_none_or(|t| t.trim().is_empty()) { + return Ok(None); } - // 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 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 { @@ -252,27 +247,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 mut domains: Vec = items .iter() .map(serde_json::to_value) @@ -280,15 +267,31 @@ pub(super) fn command() -> RuntimeCommandSpec { .map_err(|e| { CliCoreError::message(format!("failed to serialize domain list: {e}")) })?; + 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() + ))); + } + }; for domain in &mut domains { args.domain_format .form() .convert_fields(domain, &["domain", "nameServers"]); } - Ok(CommandResult::new(json!(domains)).with_next_actions(vec![ - next_action("dns list ", "View a domain's DNS records") - .with_param("domain", NextActionParam::required()), - ])) + 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()), + ])) }, ) } @@ -296,23 +299,67 @@ 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_cursor_pagination_with_a_default_and_a_max_limit() { + assert_eq!(command().spec.cursor, Some(CursorConfig::new(100, 200))); + } + #[test] - fn opts_into_pagination_with_no_default_and_a_max_limit() { + 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)).expect("valid token"), + 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)).expect("valid token"), + Some(("abc123".to_string(), None)) + ); + } + + #[test] + 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] fn parse_statuses_is_case_insensitive_and_validates() { let parsed = parse_statuses(&["active".to_string(), "CANCELLED".to_string()]) @@ -476,12 +523,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| { @@ -497,16 +542,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(), @@ -515,7 +550,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"); @@ -524,28 +559,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; @@ -557,58 +586,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| { @@ -628,14 +633,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..2683bf46 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,83 @@ 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). Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(u32).range(1..))] + 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, + 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, 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 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>, 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.links.iter().any(|link| link.rel == "next"); + Ok((list.items, has_more)) } #[cfg(test)] @@ -118,11 +115,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 +130,116 @@ 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"); - then.status(200) - .json_body(json!({ "items": page_of(3, 0) })); + .query_param("page", "1") + .query_param("pageSize", "3"); + 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; 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 next link means another page exists"); } #[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_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 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"); - then.status(200) - .json_body(json!({ "items": page_of(10, SERVER_PAGE_SIZE_CAP) })); + .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 = fetch_mailboxes(&client, None, None, 5, 99) + let (mailboxes, has_more) = fetch_mailbox_page(&client, None, None, 2, 3) .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); + 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 unlimited_limit_fetches_until_a_short_page_is_seen() { + 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 page1 = server + let mock = 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) })); + 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 page2 = server + + 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] + async fn fetch_mailbox_page_reports_no_more_on_a_short_page() { + 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("page", "2") + .query_param("pageSize", "100"); then.status(200) - .json_body(json!({ "items": page_of(5, SERVER_PAGE_SIZE_CAP) })); + .json_body(json!({ "items": page_of(10, SERVER_PAGE_SIZE_CAP) })); }) .await; let client = client_for(&server); - let mailboxes = fetch_mailboxes(&client, None, None, 0, 0) - .await - .expect("fetch mailboxes"); + let (mailboxes, has_more) = + fetch_mailbox_page(&client, None, None, 2, SERVER_PAGE_SIZE_CAP) + .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, "no next link means the list is exhausted"); } #[tokio::test] @@ -236,7 +257,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..8e3acb86 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,31 @@ 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, +} + +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}")] @@ -104,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(), @@ -139,6 +170,18 @@ impl HostingClient { let request = req .build() .map_err(|e| ClientError::Network(e.to_string()))?; + 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. `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 @@ -152,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) { @@ -169,6 +216,42 @@ 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`). + /// + /// 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()))?; + // 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. async fn post_empty_json(&self, path: &str) -> Result { self.post_empty_json_inner(path, true).await @@ -549,16 +632,27 @@ impl HostingClient { response(client.get_runtime().app_id(app_id).send().await).await } + /// 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, - page_token: Option<&str>, - limit: Option, - ) -> Result { + want: Option, + ) -> Result { let client = self.api()?; - let request = client.list_domains().app_id(app_id); - let _ = (page_token, limit); - response(request.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 { @@ -603,18 +697,111 @@ impl HostingClient { .await } + /// 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, - page_token: Option<&str>, - limit: Option, hosting_product: &str, - ) -> Result { + ) -> 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 + 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); + // 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) { + stopped_early = super::common::next_link_href(&page).is_some(); + break; + } + let Some(mut query) = super::common::next_link_query(&page) else { + 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}")); + } + } + // 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?; + 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 { @@ -694,6 +881,19 @@ 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; + +#[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..7b952b9d --- /dev/null +++ b/rust/src/hosting/client_paging_tests.rs @@ -0,0 +1,498 @@ +//! 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_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_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; + 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 15b7956c..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; @@ -468,7 +491,7 @@ async fn list_domains_hits_correct_path() { .await; client(&server.base_url()) - .list_domains("app-1", None, None) + .list_domains("app-1", None) .await .expect("list domains"); @@ -529,7 +552,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..47f1a89c 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,16 +271,77 @@ pub fn next_page_token(response: &Value) -> Option { None } -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()); - } +/// 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) +} + +/// 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). +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 { + 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, } - None +} + +/// 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) +} + +/// 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.split('#').next()?; + url::form_urlencoded::parse(query.as_bytes()) + .find(|(name, _)| name == param) + .map(|(_, value)| value.into_owned()) } #[cfg(test)] @@ -377,6 +440,112 @@ mod tests { assert!(next_page_token(&response).is_none()); } + #[test] + 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") + ); + } + + #[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] + 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!( + 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_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)); + 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 268944e1..7f1d34db 100644 --- a/rust/src/hosting/domain/list.rs +++ b/rust/src/hosting/domain/list.rs @@ -1,9 +1,14 @@ -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, incomplete_list_warning, make_client, +}; use crate::idn::DomainFormArgs; use crate::next_action::next_action; +use crate::pagination_token::{self, warn_user}; use crate::scopes::HOSTING_DOMAIN_READ as DOMAIN_READ; #[derive(Debug, Clone, clap::Args)] @@ -12,70 +17,106 @@ 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. Ignored when `--continue` is given. + #[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..))] + offset: Option, #[command(flatten)] domain_format: DomainFormArgs, } +/// 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") .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, 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()); - } - - if limit.is_some_and(|cap| all_items.len() >= cap as usize) { - all_items.truncate(limit.expect("checked") as usize); - break; - } - - match next_page_token(&response) { - Some(token) => page_token = Some(token), - None => break, - } + 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 = 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 + .map_err(client_err)?; + if listed.incomplete { + warn_user(&incomplete_list_warning("domains")); } + let partial = listed.is_partial(); - for item in &mut all_items { + // Names are shown in the form the user asked for before `--filter` + // and paging, so a filter matches what is displayed. + let mut items = listed.items; + for item in &mut items { args.domain_format .form() .convert_fields(item, &["hostname"]); } - 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)), - ])) + + let (page, continuation) = pagination_token::paginate_known_collection( + pagination_token::filter_known_collection(items, &ctx.middleware.filter)?, + ctx.middleware.cursor_limit, + 200, + continue_token.as_deref(), + )?; + + // 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 + }; + + 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)), + ])) }, ) } + +#[cfg(test)] +mod tests { + use cli_engine::CursorConfig; + + use super::command; + + #[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..577dc8a3 100644 --- a/rust/src/hosting/subscription/list.rs +++ b/rust/src/hosting/subscription/list.rs @@ -5,9 +5,10 @@ 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, incomplete_list_warning, make_client, }; 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)] @@ -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 listed = client + .list_subscriptions(hosting_product) + .await + .map_err(client_err)?; + if listed.incomplete { + warn_user(&incomplete_list_warning("subscriptions")); } - Ok(list_result(all_items, limit)) + Ok(list_result(listed.items, args.limit)) }, ) } diff --git a/rust/src/main.rs b/rust/src/main.rs index edc916d0..8f65eeaa 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -14,6 +14,7 @@ mod idn; 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..82925758 --- /dev/null +++ b/rust/src/pagination_token.rs @@ -0,0 +1,693 @@ +//! 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::{CliCoreError, CursorContinuation, PipelineOpts, apply_pipeline}; +use serde_json::Value; + +/// 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. +/// +/// 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()); + }; + 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` +/// 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", "%") +} + +/// 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}:{}", escape(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. 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). 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 { + 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 +/// 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) 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>, + default_page: P, + limit: i64, + max_limit: i64, +) -> cli_engine::Result<(P, i64)> { + let fields = parse(continue_token, &["limit", "page"])?; + let page = match fields.typed::

("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)?; + Ok((page, page_size)) +} + +/// 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}"), + )), + } +} + +/// 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(), + }) +} + +/// 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, +) -> 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 +/// 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( + items: Vec, + limit: i64, + max_limit: i64, + continue_token: Option<&str>, +) -> 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); + 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)); + Ok((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; + + use super::*; + + const KEYS: &[&str] = &["page", "limit", "offset"]; + + #[test] + fn parse_reads_key_value_pairs() { + 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_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_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 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 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), KEYS).expect("valid"); + assert_eq!(fields.typed::("page").expect("valid"), Some(3)); + assert_eq!(fields.typed::("limit").expect("valid"), Some(20)); + } + + #[test] + 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")); + } + + #[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, 100).expect("valid token"); + 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, 100).expect("valid token"); + assert_eq!(page, 2); + assert_eq!(page_size, 10); + } + + #[test] + 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, 100).expect("valid token"); + 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, 100).expect("valid token"); + assert_eq!(page, 6); + assert_eq!(page_size, 10); + } + + 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, 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)); + assert_eq!(cont.remaining, Some(3)); + } + + #[test] + fn paginate_known_collection_resumes_from_the_continue_token() { + 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)).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)); + } + + #[test] + fn paginate_known_collection_continue_token_embeds_and_inherits_the_limit() { + 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)).expect("valid 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, 100, Some(&token)).expect("valid 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, 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)); + } + + #[test] + fn paginate_known_collection_past_the_end_returns_empty() { + 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)); + } + + #[test] + fn paginate_known_collection_of_an_empty_collection() { + 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)); + } + + #[test] + fn known_collection_need_is_the_window_end_plus_one() { + 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!( + 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).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)); + 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![ + 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(); + 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), &["token", "direction"]).expect("valid"); + assert_eq!(fields.text("token"), Some(opaque), "{token}"); + assert_eq!(fields.text("direction"), 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_rejects_an_out_of_range_token_limit() { + // Negative would otherwise convert to usize::MAX and return everything; + // 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 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")) + .expect("in range"); + assert_eq!( + page.len(), + 100, + "an in-range token limit is still inherited" + ); + } + + #[test] + 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_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 [ + "limit:-1,page:2", + "limit:0,page:2", + "limit:101,page:2", + "limit:25,page:abc", + "limit:25,page:0", + "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) + .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 9d3dfba2..80e5cf17 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,36 @@ 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..)) + .help( + "Jump directly to an offset in the results. \ + Ignored when `--continue` is given.", + ), + ), + |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( + pagination_token::filter_known_collection(actions, &ctx.middleware.filter)?, + ctx.middleware.cursor_limit, + 200, + 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 +199,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 +207,196 @@ 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)); + } + + /// `--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] + 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(); + 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..00e21cae 100644 --- a/rust/src/platform/app/commands/list.rs +++ b/rust/src/platform/app/commands/list.rs @@ -1,12 +1,18 @@ //! `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; +use crate::pagination_token; +/// 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 +26,31 @@ 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, missing_cursor) = + continuation_for(page.has_next_page, page.end_cursor); + if missing_cursor { + pagination_token::warn_user( + "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| { 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", @@ -46,19 +65,51 @@ 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, PaginationConfig, Stage}; + use cli_engine::{Cli, CliConfig, CursorConfig, Stage}; + + 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); + } - use super::command; + #[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] - 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]