Skip to content

Commit 06bed76

Browse files
rholshausenclaude
andcommitted
fix(verifier): trim trailing slash from broker context path and preserve query strings
Addresses remaining review comments on PR #539: - A broker URL with a trailing slash (e.g. http://host/pact/) produced a context path of "/pact/", causing normalize_path_inner and resolve_path_inner to emit double slashes ("/pact//...") when prepending/joining paths. - normalize_path_inner extracted only the path from absolute broker link URLs, silently dropping any query string (e.g. pagination params). - fetch() logged the same info!/trace! lines that fetch_with_url() already logs, duplicating log output on every broker fetch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent e3d90c1 commit 06bed76

1 file changed

Lines changed: 62 additions & 6 deletions

File tree

rust/pact_verifier/src/pact_broker.rs

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,21 +77,37 @@ fn has_context_prefix(path: &str, context_path: &str) -> bool {
7777
path == context_path || path.starts_with(&format!("{}/", context_path))
7878
}
7979

80+
/// Strips a single trailing `/` from `context_path`, leaving the root path `/` as is.
81+
///
82+
/// A broker URL configured with a trailing slash (e.g. `http://host/pact/`) would
83+
/// otherwise yield a context path of `/pact/`, producing double slashes (`/pact//...`)
84+
/// wherever the context path is prepended or joined with another path.
85+
fn trim_context_path(context_path: &str) -> &str {
86+
if context_path == "/" {
87+
context_path
88+
} else {
89+
context_path.trim_end_matches('/')
90+
}
91+
}
92+
8093
/// Extracts the path component from `url_string` (stripping scheme/host if it is an
8194
/// absolute URL) and prepends `broker_url`'s context path when it is missing.
8295
///
8396
/// Accepts an already-parsed `broker_url` so callers that have parsed it for
8497
/// another purpose need not parse it a second time.
8598
fn normalize_path_inner(url_string: &str, broker_url: &Url) -> String {
8699
let mut path = if let Ok(parsed_url) = url_string.parse::<Url>() {
87-
// Absolute URL: extract only the path, so we keep the broker's host.
88-
parsed_url.path().to_string()
100+
// Absolute URL: extract the path (plus query, if any), so we keep the broker's host.
101+
match parsed_url.query() {
102+
Some(query) => format!("{}?{}", parsed_url.path(), query),
103+
None => parsed_url.path().to_string()
104+
}
89105
} else {
90106
// Already a path (relative or absolute).
91107
url_string.to_string()
92108
};
93109

94-
let context_path = broker_url.path();
110+
let context_path = trim_context_path(broker_url.path());
95111
if !context_path.is_empty() && context_path != "/" && path.starts_with('/') {
96112
if !has_context_prefix(&path, context_path) {
97113
debug!("Prepending context path '{}' to path '{}'", context_path, path);
@@ -107,7 +123,7 @@ fn normalize_path_inner(url_string: &str, broker_url: &Url) -> String {
107123
/// Accepts ownership of `broker_url` so callers that have already parsed it
108124
/// can pass it through without a second `parse::<Url>()` call.
109125
fn resolve_path_inner(path: &str, broker_url: Url) -> Result<Url, PactBrokerError> {
110-
let context_path = broker_url.path().to_string();
126+
let context_path = trim_context_path(broker_url.path()).to_string();
111127
let url = if path.is_empty() {
112128
broker_url
113129
} else if !context_path.is_empty() && context_path != "/" {
@@ -410,8 +426,6 @@ impl HALClient {
410426
}
411427

412428
async fn fetch(&self, path: &str) -> Result<Value, PactBrokerError> {
413-
info!("Fetching path '{}' from pact broker", path);
414-
trace!(%path, broker_url = %self.url, ">> fetch");
415429
let url = self.resolve_path(path)?;
416430
self.fetch_with_url(path, url).await
417431
}
@@ -2776,6 +2790,48 @@ mod tests {
27762790
expect!(normalized).to(be_equal_to("/pact/pacts/provider/Example%20API/latest"));
27772791
}
27782792

2793+
#[test]
2794+
fn normalize_path_from_url_drops_query_string() {
2795+
// Copilot review comment: normalize_path_inner extracts only `parsed_url.path()`
2796+
// from an absolute URL, so any query string on a broker-provided href (e.g.
2797+
// pagination or filter params) is silently discarded.
2798+
let broker_url = "http://127.0.0.1:8080/pact".parse::<Url>().unwrap();
2799+
let absolute_link = "http://other-broker.example.com/pact/pacts/provider/Example%20API?page=2&size=10";
2800+
2801+
let normalized = normalize_path_inner(absolute_link, &broker_url);
2802+
2803+
expect!(normalized).to(be_equal_to("/pact/pacts/provider/Example%20API?page=2&size=10"));
2804+
}
2805+
2806+
#[test]
2807+
fn normalize_path_from_url_context_with_trailing_slash_does_not_double_slash() {
2808+
// Copilot review comment: if the broker URL has a trailing slash (e.g.
2809+
// "http://host/pact/"), context_path is "/pact/" and the naive
2810+
// `format!("{}{}", context_path, path)` prepend produces "/pact//...".
2811+
let broker_url = "http://127.0.0.1:8080/pact/".parse::<Url>().unwrap();
2812+
let absolute_link = "http://other-broker.example.com/pacts/provider/Example%20API/for-verification";
2813+
2814+
let normalized = normalize_path_inner(absolute_link, &broker_url);
2815+
2816+
expect!(normalized).to(be_equal_to("/pact/pacts/provider/Example%20API/for-verification"));
2817+
}
2818+
2819+
#[test]
2820+
fn resolve_path_context_with_trailing_slash_does_not_double_slash() {
2821+
// Copilot review comment: resolve_path_inner also takes broker_url.path()
2822+
// verbatim, so a trailing-slash context path ("/pact/") makes
2823+
// `format!("{}/", context_path)` produce "/pact//", which can 404 on
2824+
// brokers/proxies that don't normalize repeated slashes.
2825+
let client = HALClientBuilder::builder()
2826+
.with_url("http://127.0.0.1:8080/pact/", None)
2827+
.build();
2828+
2829+
let path = "pacts/provider/Example%20API/for-verification";
2830+
let resolved = client.resolve_path(path).expect("Should resolve path");
2831+
2832+
expect!(resolved.path()).to(be_equal_to("/pact/pacts/provider/Example%20API/for-verification"));
2833+
}
2834+
27792835
#[test_log::test(tokio::test)]
27802836
async fn subpath_broker_templates_are_substituted() {
27812837
let pact_broker = PactBuilderAsync::new("RustPactVerifier", "TemplateTest")

0 commit comments

Comments
 (0)