From 2500e8f8fab3baab649d6ea67594ce63de0316bf Mon Sep 17 00:00:00 2001 From: Jonas Platte Date: Fri, 11 Nov 2022 10:57:57 +0100 Subject: [PATCH] api: Make query_string parameter of make_endpoint_url non-optional When Some(_), the argument would often be an empty string. By always using the empty string to mean "no query", we avoid trailing a `?` on URLs without having two parameter values that mean the same thing. --- crates/ruma-client-api/src/push/set_pushrule.rs | 2 +- .../src/state/get_state_events_for_key.rs | 2 +- .../ruma-client-api/src/state/send_state_event.rs | 9 +++++++-- crates/ruma-common/src/api/metadata.rs | 15 +++++++-------- .../ruma-common/tests/api/manual_endpoint_impl.rs | 8 ++------ crates/ruma-macros/src/api/request/outgoing.rs | 10 +++------- 6 files changed, 21 insertions(+), 25 deletions(-) diff --git a/crates/ruma-client-api/src/push/set_pushrule.rs b/crates/ruma-client-api/src/push/set_pushrule.rs index 9702837c..e40920ec 100644 --- a/crates/ruma-client-api/src/push/set_pushrule.rs +++ b/crates/ruma-client-api/src/push/set_pushrule.rs @@ -91,7 +91,7 @@ pub mod v3 { considering_versions, base_url, &[&self.scope, &self.rule.kind(), &self.rule.rule_id()], - Some(&query_string), + &query_string, )?; let body: RequestBody = self.rule.into(); diff --git a/crates/ruma-client-api/src/state/get_state_events_for_key.rs b/crates/ruma-client-api/src/state/get_state_events_for_key.rs index fa3c23e6..83f4c5af 100644 --- a/crates/ruma-client-api/src/state/get_state_events_for_key.rs +++ b/crates/ruma-client-api/src/state/get_state_events_for_key.rs @@ -88,7 +88,7 @@ pub mod v3 { considering_versions, base_url, &[&self.room_id, &self.event_type, &self.state_key], - None, + "", )?) .header(header::CONTENT_TYPE, "application/json") .header( diff --git a/crates/ruma-client-api/src/state/send_state_event.rs b/crates/ruma-client-api/src/state/send_state_event.rs index 19c2ec07..156eb2f9 100644 --- a/crates/ruma-client-api/src/state/send_state_event.rs +++ b/crates/ruma-client-api/src/state/send_state_event.rs @@ -134,7 +134,7 @@ pub mod v3 { considering_versions, base_url, &[&self.room_id, &self.event_type, &self.state_key], - Some(&query_string), + &query_string, )?) .header(header::CONTENT_TYPE, "application/json") .header( @@ -220,7 +220,7 @@ pub mod v3 { }; // This used to panic in make_endpoint_url because of a mismatch in the path parameter count - Request::new( + let req = Request::new( room_id!("!room:server.tld"), &EmptyStateKey, &RoomNameEventContent::new(Some("Test room".to_owned())), @@ -232,5 +232,10 @@ pub mod v3 { &[MatrixVersion::V1_1], ) .unwrap(); + + assert_eq!( + req.uri(), + "https://server.tld/_matrix/client/v3/rooms/%21room%3Aserver%2Etld/state/m%2Eroom%2Ename/" + ); } } diff --git a/crates/ruma-common/src/api/metadata.rs b/crates/ruma-common/src/api/metadata.rs index d50a8fcc..264b244c 100644 --- a/crates/ruma-common/src/api/metadata.rs +++ b/crates/ruma-common/src/api/metadata.rs @@ -90,7 +90,7 @@ impl Metadata { versions: &[MatrixVersion], base_url: &str, path_args: &[&dyn Display], - query_string: Option<&str>, + query_string: &str, ) -> Result { let path_with_placeholders = self.history.select_path(versions, self.name)?; @@ -117,9 +117,9 @@ impl Metadata { } } - if let Some(query) = query_string { + if !query_string.is_empty() { res.push('?'); - res.push_str(query); + res.push_str(query_string); } Ok(res) @@ -699,22 +699,21 @@ mod tests { #[test] fn make_simple_endpoint_url() { let meta = stable_only_metadata(&[(V1_0, "/s")]); - let url = meta.make_endpoint_url(&[V1_0], "https://example.org", &[], None).unwrap(); + let url = meta.make_endpoint_url(&[V1_0], "https://example.org", &[], "").unwrap(); assert_eq!(url, "https://example.org/s"); } #[test] fn make_endpoint_url_with_path_args() { let meta = stable_only_metadata(&[(V1_0, "/s/:x")]); - let url = meta.make_endpoint_url(&[V1_0], "https://example.org", &[&"123"], None).unwrap(); + let url = meta.make_endpoint_url(&[V1_0], "https://example.org", &[&"123"], "").unwrap(); assert_eq!(url, "https://example.org/s/123"); } #[test] fn make_endpoint_url_with_query() { let meta = stable_only_metadata(&[(V1_0, "/s/")]); - let url = - meta.make_endpoint_url(&[V1_0], "https://example.org", &[], Some("foo=bar")).unwrap(); + let url = meta.make_endpoint_url(&[V1_0], "https://example.org", &[], "foo=bar").unwrap(); assert_eq!(url, "https://example.org/s/?foo=bar"); } @@ -722,7 +721,7 @@ mod tests { #[should_panic] fn make_endpoint_url_wrong_num_path_args() { let meta = stable_only_metadata(&[(V1_0, "/s/:x")]); - _ = meta.make_endpoint_url(&[V1_0], "https://example.org", &[], None); + _ = meta.make_endpoint_url(&[V1_0], "https://example.org", &[], ""); } const EMPTY: VersionHistory = diff --git a/crates/ruma-common/tests/api/manual_endpoint_impl.rs b/crates/ruma-common/tests/api/manual_endpoint_impl.rs index b4ee3997..417549f5 100644 --- a/crates/ruma-common/tests/api/manual_endpoint_impl.rs +++ b/crates/ruma-common/tests/api/manual_endpoint_impl.rs @@ -51,12 +51,8 @@ impl OutgoingRequest for Request { _access_token: SendAccessToken<'_>, considering_versions: &'_ [MatrixVersion], ) -> Result, IntoHttpError> { - let url = METADATA.make_endpoint_url( - considering_versions, - base_url, - &[&self.room_alias], - None, - )?; + let url = + METADATA.make_endpoint_url(considering_versions, base_url, &[&self.room_alias], "")?; let request_body = RequestBody { room_id: self.room_id }; diff --git a/crates/ruma-macros/src/api/request/outgoing.rs b/crates/ruma-macros/src/api/request/outgoing.rs index fe79e558..8d770039 100644 --- a/crates/ruma-macros/src/api/request/outgoing.rs +++ b/crates/ruma-macros/src/api/request/outgoing.rs @@ -38,9 +38,7 @@ impl Request { let request_query = RequestQuery(self.#field_name); assert_trait_impl(&request_query.0); - ::std::option::Option::Some( - &#ruma_common::serde::urlencoded::to_string(request_query)? - ) + &#ruma_common::serde::urlencoded::to_string(request_query)? }} } else if self.has_query_fields() { let request_query_init_fields = struct_init_fields( @@ -53,12 +51,10 @@ impl Request { #request_query_init_fields }; - ::std::option::Option::Some( - &#ruma_common::serde::urlencoded::to_string(request_query)? - ) + &#ruma_common::serde::urlencoded::to_string(request_query)? }} } else { - quote! { ::std::option::Option::None } + quote! { "" } }; // If there are no body fields, the request body will be empty (not `{}`), so the