docs, error improvements, clippy fixes

src/client.rs

@@ -41,36 +41,57 @@ impl Default for ApiClient {
 impl Debug for ApiClient {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         f.debug_struct("ApiClient")
-            .field("base_url", &self.base_url.to_string())
+            .field("base_url", &format_args!("{}", self.base_url))
             .finish_non_exhaustive()
     }
 }
 
 impl ApiClient {
-    pub const DEMOS_TF_BASE_URL: &'static str = "https://api.demos.tf";
+    pub const DEMOS_TF_BASE_URL: &'static str = "https://api.demos.tf/";
 
     /// Create an api client for the default demos.tf endpoint
+    #[must_use]
     pub fn new() -> Self {
-        ApiClient::with_base_url(ApiClient::DEMOS_TF_BASE_URL).unwrap()
+        ApiClient::with_base_url(ApiClient::DEMOS_TF_BASE_URL).unwrap_or_else(|_| unreachable!())
     }
 
     /// Create an api client using a different api endpoint
+    ///
+    /// # Errors
+    ///
+    /// Returns an error when the provided `base_url` is not a valid url
     pub fn with_base_url(base_url: impl IntoUrl) -> Result<Self, Error> {
         ApiClient::with_base_url_and_timeout(base_url, Duration::from_secs(15))
     }
 
-    /// Create an api client using a different api endpoint
+    /// Create an api client using a different api endpoint and timeout
+    ///
+    /// # Errors
+    ///
+    /// Returns an error when the provided `base_url` is not a valid url
     pub fn with_base_url_and_timeout(
         base_url: impl IntoUrl,
         timeout: Duration,
     ) -> Result<Self, Error> {
+        // ensure there is always a leading / to prevent unexpected behavior with url creation later
+        let mut base_url = base_url.into_url().map_err(|_| Error::InvalidBaseUrl)?;
+        if !base_url.path().ends_with("/") {
+            base_url.set_path(&format!("{}/", base_url.path()));
+        }
+
         Ok(ApiClient {
             base_timeout: timeout,
             client: Client::builder().timeout(timeout).build()?,
-            base_url: base_url.into_url().map_err(Error::InvalidBaseUrl)?,
+            base_url,
         })
     }
 
+    fn url<P: AsRef<str>>(&self, path: P) -> Result<Url, Error> {
+        self.base_url
+            .join(path.as_ref())
+            .map_err(|_| Error::InvalidBaseUrl)
+    }
+
     /// List demos with the provided options
     ///
     /// note that the pages start counting at 1
@@ -95,22 +116,7 @@ impl ApiClient {
     /// ```
     #[instrument]
     pub async fn list(&self, params: ListParams, page: u32) -> Result<Vec<Demo>, Error> {
-        if page == 0 {
+        self.list_url(self.url("demos")?, params, page).await
-            return Err(Error::InvalidPage);
-        }
-
-        let mut url = self.base_url.clone();
-        url.set_path("/demos");
-        Ok(self
-            .client
-            .get(url)
-            .query(&[("page", page)])
-            .query(&params)
-            .send()
-            .await?
-            .error_for_status()?
-            .json()
-            .await?)
     }
 
     /// List demos uploaded by a user with the provided options
@@ -143,12 +149,19 @@ impl ApiClient {
         params: ListParams,
         page: u32,
     ) -> Result<Vec<Demo>, Error> {
+        self.list_url(
+            self.url(format!("uploads/{}", u64::from(uploader)))?,
+            params,
+            page,
+        )
+        .await
+    }
+
+    async fn list_url(&self, url: Url, params: ListParams, page: u32) -> Result<Vec<Demo>, Error> {
         if page == 0 {
             return Err(Error::InvalidPage);
         }
 
-        let mut url = self.base_url.clone();
-        url.set_path(&format!("/uploads/{}", u64::from(uploader)));
         Ok(self
             .client
             .get(url)
@@ -185,9 +198,11 @@ impl ApiClient {
     /// ```
     #[instrument]
     pub async fn get(&self, demo_id: u32) -> Result<Demo, Error> {
-        let mut url = self.base_url.clone();
+        let response = self
-        url.set_path(&format!("/demos/{}", demo_id));
+            .client
-        let response = self.client.get(url).send().await?;
+            .get(self.url(format!("/demos/{}", demo_id))?)
+            .send()
+            .await?;
 
         if response.status() == StatusCode::NOT_FOUND {
             return Err(Error::DemoNotFound(demo_id));
@@ -215,16 +230,17 @@ impl ApiClient {
     /// ```
     #[instrument]
     pub async fn get_user(&self, user_id: u32) -> Result<User, Error> {
-        let mut url = self.base_url.clone();
+        let response = self
-        url.set_path(&format!("/users/{}", user_id));
-        Ok(self
             .client
-            .get(url)
+            .get(self.url(format!("/users/{}", user_id))?)
             .send()
-            .await?
+            .await?;
-            .error_for_status()?
+
-            .json()
+        if response.status() == StatusCode::NOT_FOUND {
-            .await?)
+            return Err(Error::UserNotFound(user_id));
+        }
+
+        Ok(response.error_for_status()?.json().await?)
     }
 
     /// List demos with the provided options
@@ -248,16 +264,17 @@ impl ApiClient {
     /// ```
     #[instrument]
     pub async fn get_chat(&self, demo_id: u32) -> Result<Vec<ChatMessage>, Error> {
-        let mut url = self.base_url.clone();
+        let response = self
-        url.set_path(&format!("/demos/{}/chat", demo_id));
-        Ok(self
             .client
-            .get(url)
+            .get(self.url(format!("/demos/{}/chat", demo_id))?)
             .send()
-            .await?
+            .await?;
-            .error_for_status()?
+
-            .json()
+        if response.status() == StatusCode::NOT_FOUND {
-            .await?)
+            return Err(Error::DemoNotFound(demo_id));
+        }
+
+        Ok(response.error_for_status()?.json().await?)
     }
 
     #[instrument]
@@ -270,11 +287,8 @@ impl ApiClient {
         hash: [u8; 16],
         key: &str,
     ) -> Result<(), Error> {
-        let mut api_url = self.base_url.clone();
-        api_url.set_path(&format!("/demos/{}/url", demo_id));
-
         self.client
-            .post(api_url)
+            .post(self.url(format!("/demos/{}/url", demo_id))?)
             .form(&[
                 ("hash", hex::encode(hash).as_str()),
                 ("backend", backend),
@@ -312,7 +326,7 @@ impl ApiClient {
 
         let resp = self
             .client
-            .post(self.base_url.join("/upload").unwrap())
+            .post(self.url("/upload")?)
             .multipart(form)
             .send()
             .await?
@@ -330,7 +344,7 @@ impl ApiClient {
 
     pub(crate) async fn download_demo(&self, url: &str, duration: u16) -> Result<Response, Error> {
         // set timeout to 1s per 60s (~1mb) with a minimum of 15s, scaled by an configured timeout (default 15s)
-        let timeout_scale = (duration as f32 / 60.0).max(15.0) / 15.0;
+        let timeout_scale = (f32::from(duration) / 60.0).max(15.0) / 15.0;
         let timeout = Duration::from_secs_f32(self.base_timeout.as_secs_f32() * timeout_scale);
         trace!(url = url, timeout = debug(timeout), "requesting demo file");
         Ok(self
@@ -342,3 +356,39 @@ impl ApiClient {
             .error_for_status()?)
     }
 }
+
+#[test]
+fn test_url() {
+    assert_eq!(
+        "https://example.com/demos",
+        ApiClient::with_base_url("https://example.com")
+            .unwrap()
+            .url("demos")
+            .unwrap()
+            .to_string()
+    );
+    assert_eq!(
+        "https://example.com/demos",
+        ApiClient::with_base_url("https://example.com/")
+            .unwrap()
+            .url("demos")
+            .unwrap()
+            .to_string()
+    );
+    assert_eq!(
+        "https://example.com/sub/demos",
+        ApiClient::with_base_url("https://example.com/sub/")
+            .unwrap()
+            .url("demos")
+            .unwrap()
+            .to_string()
+    );
+    assert_eq!(
+        "https://example.com/sub/demos",
+        ApiClient::with_base_url("https://example.com/sub")
+            .unwrap()
+            .url("demos")
+            .unwrap()
+            .to_string()
+    );
+}

src/lib.rs

@@ -18,8 +18,8 @@ mod client;
 #[derive(Debug, Error)]
 #[non_exhaustive]
 pub enum Error {
-    #[error("Invalid base url: {0}")]
+    #[error("Invalid base url")]
-    InvalidBaseUrl(reqwest::Error),
+    InvalidBaseUrl,
     #[error("Request failed: {0}")]
     Request(reqwest::Error),
     #[error("Invalid page requested")]
@@ -34,6 +34,8 @@ pub enum Error {
     InvalidResponse(String),
     #[error("Demo {0} not found")]
     DemoNotFound(u32),
+    #[error("User {0} not found")]
+    UserNotFound(u32),
     #[error("Error while writing demo data")]
     Write(#[source] std::io::Error),
     #[error("Operation timed out")]
@@ -98,6 +100,7 @@ impl Demo {
     }
 
     /// Download a demo, returning a stream of chunks
+    #[instrument]
     pub async fn download(
         &self,
         client: &ApiClient,
@@ -147,6 +150,7 @@ pub enum UserRef {
 
 impl UserRef {
     /// Id of the user
+    #[must_use]
     pub fn id(&self) -> u32 {
         match self {
             UserRef::Id(id) | UserRef::User(User { id, .. }) => *id,
@@ -154,6 +158,7 @@ impl UserRef {
     }
 
     /// Return the stored user info if available
+    #[must_use]
     pub fn user(&self) -> Option<&User> {
         match self {
             UserRef::Id(_) => None,
@@ -272,7 +277,7 @@ pub enum ListOrder {
     Descending,
 }
 
-/// Gametype as recognized by demos.tf, HL, Prolander, 6s or 4v4
+/// Game type as recognized by demos.tf, HL, Prolander, 6s or 4v4
 #[derive(Debug, Clone, Copy, Serialize)]
 pub enum GameType {
     #[serde(rename = "hl")]
@@ -382,6 +387,7 @@ fn test_serialize_player_list() {
 }
 
 impl ListParams {
+    /// Specify the backend name to filter demos with
     #[must_use]
     pub fn with_backend(self, backend: impl Into<String>) -> Self {
         ListParams {
@@ -390,6 +396,7 @@ impl ListParams {
         }
     }
 
+    /// Specify the map name to filter demos with
     #[must_use]
     pub fn with_map(self, map: impl Into<String>) -> Self {
         ListParams {
@@ -398,6 +405,7 @@ impl ListParams {
         }
     }
 
+    /// Specify the player steam ids to filter demos with
     #[must_use]
     pub fn with_players<T: Into<SteamID>, I: IntoIterator<Item = T>>(self, players: I) -> Self {
         ListParams {
@@ -406,6 +414,7 @@ impl ListParams {
         }
     }
 
+    /// Specify the game type to filter demos with
     #[must_use]
     pub fn with_type(self, ty: GameType) -> Self {
         ListParams {
@@ -414,6 +423,7 @@ impl ListParams {
         }
     }
 
+    /// Specify the before date to filter demos with
     #[must_use]
     pub fn with_before(self, before: OffsetDateTime) -> Self {
         ListParams {
@@ -422,6 +432,7 @@ impl ListParams {
         }
     }
 
+    /// Specify the after date to filter demos with
     #[must_use]
     pub fn with_after(self, after: OffsetDateTime) -> Self {
         ListParams {
@@ -430,6 +441,7 @@ impl ListParams {
         }
     }
 
+    /// Specify the sort
     #[must_use]
     pub fn with_order(self, order: ListOrder) -> Self {
         ListParams { order, ..self }