diff --git a/crates/omnigraph-cli/src/client.rs b/crates/omnigraph-cli/src/client.rs index d9e77264..4faaa11a 100644 --- a/crates/omnigraph-cli/src/client.rs +++ b/crates/omnigraph-cli/src/client.rs @@ -40,7 +40,7 @@ use serde_json::Value; use crate::cli::CliLoadMode; use crate::helpers::{ ResolvedCliGraph, apply_bearer_token, apply_server_flag, build_http_client, is_remote_uri, - legacy_change_request_body, open_local_db_with_policy, query_params_from_json, remote_branch_url, + legacy_change_request_body, open_local_db_with_policy, query_params_from_json, remote_json, remote_url, resolve_cli_actor, resolve_cli_graph, resolve_remote_bearer_token, select_named_query, }; @@ -173,7 +173,7 @@ impl GraphClient { remote_json( http, Method::GET, - remote_url(base_url, "/branches"), + remote_url(base_url, &["branches"], &[])?, None, token.as_deref(), ) @@ -198,7 +198,7 @@ impl GraphClient { remote_json( http, Method::GET, - format!("{}?branch={}", remote_url(base_url, "/snapshot"), branch), + remote_url(base_url, &["snapshot"], &[("branch", branch)])?, None, token.as_deref(), ) @@ -222,7 +222,7 @@ impl GraphClient { remote_json( http, Method::GET, - remote_url(base_url, "/schema"), + remote_url(base_url, &["schema"], &[])?, None, token.as_deref(), ) @@ -245,8 +245,8 @@ impl GraphClient { token, } => { let url = match branch { - Some(branch) => format!("{}?branch={}", remote_url(base_url, "/commits"), branch), - None => remote_url(base_url, "/commits"), + Some(branch) => remote_url(base_url, &["commits"], &[("branch", branch)])?, + None => remote_url(base_url, &["commits"], &[])?, }; remote_json(http, Method::GET, url, None, token.as_deref()).await } @@ -273,7 +273,7 @@ impl GraphClient { remote_json( http, Method::GET, - remote_url(base_url, &format!("/commits/{commit_id}")), + remote_url(base_url, &["commits", commit_id], &[])?, None, token.as_deref(), ) @@ -310,7 +310,7 @@ impl GraphClient { let output = remote_json::( http, Method::POST, - remote_url(base_url, "/load"), + remote_url(base_url, &["load"], &[])?, Some(serde_json::to_value(IngestRequest { branch: Some(branch.to_string()), from: from.map(ToOwned::to_owned), @@ -354,7 +354,7 @@ impl GraphClient { remote_json( http, Method::POST, - remote_url(base_url, "/ingest"), + remote_url(base_url, &["ingest"], &[])?, Some(serde_json::to_value(IngestRequest { branch: Some(branch.to_string()), from: Some(from.to_string()), @@ -393,7 +393,7 @@ impl GraphClient { remote_json( http, Method::POST, - remote_url(base_url, "/change"), + remote_url(base_url, &["change"], &[])?, Some(legacy_change_request_body( query_source, query_name, @@ -446,7 +446,7 @@ impl GraphClient { remote_json( http, Method::POST, - remote_url(base_url, "/read"), + remote_url(base_url, &["read"], &[])?, Some(serde_json::to_value(ReadRequest { query_source: query_source.to_string(), query_name: query_name.map(ToOwned::to_owned), @@ -484,7 +484,7 @@ impl GraphClient { remote_json( http, Method::POST, - remote_url(base_url, "/branches"), + remote_url(base_url, &["branches"], &[])?, Some(serde_json::to_value(BranchCreateRequest { from: Some(from.to_string()), name: name.to_string(), @@ -518,7 +518,7 @@ impl GraphClient { remote_json( http, Method::DELETE, - remote_branch_url(base_url, name)?, + remote_url(base_url, &["branches", name], &[])?, None, token.as_deref(), ) @@ -547,7 +547,7 @@ impl GraphClient { remote_json( http, Method::POST, - remote_url(base_url, "/branches/merge"), + remote_url(base_url, &["branches", "merge"], &[])?, Some(serde_json::to_value(BranchMergeRequest { source: source.to_string(), target: Some(into.to_string()), @@ -598,7 +598,7 @@ impl GraphClient { remote_json::( http, Method::POST, - remote_url(base_url, "/schema/apply"), + remote_url(base_url, &["schema", "apply"], &[])?, Some(serde_json::to_value(SchemaApplyRequest { schema_source: schema_source.to_string(), allow_data_loss, @@ -642,7 +642,7 @@ impl GraphClient { token, } => { let request = apply_bearer_token( - http.request(Method::POST, remote_url(base_url, "/export")), + http.request(Method::POST, remote_url(base_url, &["export"], &[])?), token.as_deref(), ) .json(&ExportRequest { @@ -690,7 +690,7 @@ impl GraphClient { remote_json( http, Method::GET, - remote_url(base_url, "/graphs"), + remote_url(base_url, &["graphs"], &[])?, None, token.as_deref(), ) diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index 7e1ca15e..6a381b13 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/crates/omnigraph-cli/src/helpers.rs @@ -16,15 +16,41 @@ pub(crate) fn is_remote_uri(uri: &str) -> bool { uri.starts_with("http://") || uri.starts_with("https://") } -pub(crate) fn remote_url(base: &str, path: &str) -> String { - format!("{}{}", base.trim_end_matches('/'), path) -} - -pub(crate) fn remote_branch_url(base: &str, branch: &str) -> Result { - let mut url = reqwest::Url::parse(&format!("{}/", base.trim_end_matches('/')))?; +/// THE one way the CLI composes a remote request URL. Every remote call +/// routes through here so URL assembly has a single mechanism instead of +/// per-callsite string interpolation. +/// +/// - `base` is the resolved server root (single-graph) or `…/graphs/{id}` +/// (multi-graph). +/// - `segments` are appended as individual percent-encoded path segments, so +/// a dynamic component (branch name, commit id, query name) is always one +/// safe segment — e.g. a branch `etl/zendesk/run-1` becomes `%2F`-escaped. +/// - `query` pairs are percent-encoded values. +/// +/// Trailing-slash normalization happens exactly once via `pop_if_empty`: +/// `Url::parse` normalizes a path-less base (`http://host`) to a single empty +/// trailing segment, and a `…/graphs/{id}/` base keeps its own. `extend` +/// appends *after* the last segment, so without dropping a trailing empty one +/// the join emits `…/graphs/{id}//branches/{name}` — the empty `//` segment +/// misses the route and 404s. Because callers pass structured segments rather +/// than a pre-joined string, neither a stray `//` nor an un-encoded dynamic +/// component is representable here. +pub(crate) fn remote_url( + base: &str, + segments: &[&str], + query: &[(&str, &str)], +) -> Result { + let mut url = reqwest::Url::parse(base.trim_end_matches('/'))?; url.path_segments_mut() .map_err(|_| color_eyre::eyre::eyre!("invalid remote base url"))? - .extend(["branches", branch]); + .pop_if_empty() + .extend(segments); + if !query.is_empty() { + let mut pairs = url.query_pairs_mut(); + for (key, value) in query { + pairs.append_pair(key, value); + } + } Ok(url.to_string()) } @@ -340,7 +366,7 @@ pub(crate) async fn execute_operator_alias( remote_json( client, Method::POST, - remote_url(&uri, &format!("/queries/{}", alias.query)), + remote_url(&uri, &["queries", &alias.query], &[])?, body, bearer_token.as_deref(), ) @@ -1059,3 +1085,71 @@ pub(crate) fn rewrite_deprecated_argv(args: Vec) -> Vec { } args } + +#[cfg(test)] +mod tests { + use super::*; + + // `branch delete` interpolates the branch into the URL path. The composed + // path must be exactly `/branches/` with no empty `//` + // segment — an empty segment misses the + // `/graphs/{graph_id}/branches/{branch}` route and 404s. + #[test] + fn remote_url_multi_graph_base_has_no_double_slash() { + let url = remote_url("http://host/graphs/p9-os", &["branches", "tmpbranch"], &[]).unwrap(); + assert_eq!(url, "http://host/graphs/p9-os/branches/tmpbranch"); + assert!( + !url.contains("//branches"), + "double slash before branches: {url}" + ); + } + + #[test] + fn remote_url_single_graph_base_has_no_double_slash() { + let url = remote_url("http://host", &["branches", "tmpbranch"], &[]).unwrap(); + assert_eq!(url, "http://host/branches/tmpbranch"); + } + + #[test] + fn remote_url_tolerates_trailing_slash_on_base() { + let url = remote_url("http://host/graphs/p9-os/", &["branches", "tmpbranch"], &[]).unwrap(); + assert_eq!(url, "http://host/graphs/p9-os/branches/tmpbranch"); + } + + #[test] + fn remote_url_encodes_slashes_in_path_segment() { + let url = remote_url( + "http://host/graphs/p9-os", + &["branches", "etl/zendesk/run-1"], + &[], + ) + .unwrap(); + assert_eq!( + url, + "http://host/graphs/p9-os/branches/etl%2Fzendesk%2Frun-1" + ); + } + + // Sibling cases the unified builder closes by construction: a dynamic + // commit id in the path, and a branch name carried as a query value, are + // both percent-encoded instead of interpolated raw. + #[test] + fn remote_url_encodes_dynamic_path_segment_for_commits() { + let url = remote_url("http://host/graphs/p9-os", &["commits", "a/b c"], &[]).unwrap(); + assert_eq!(url, "http://host/graphs/p9-os/commits/a%2Fb%20c"); + } + + #[test] + fn remote_url_encodes_query_values() { + let url = remote_url( + "http://host/graphs/p9-os", + &["snapshot"], + &[("branch", "feature&x=1")], + ) + .unwrap(); + assert_eq!( + url, + "http://host/graphs/p9-os/snapshot?branch=feature%26x%3D1" + ); + } +}