api: simplify query parameter passing

This keeps the `Pairs` type out of the API (which was forwarded from a
dependent crate) and allows for chaining usage for endpoint
implementations.

CHANGELOG.md

@@ -5,6 +5,11 @@
 It was observed (#41) that the new API pattern was not handling `POST` and
 `PUT` parameters properly. This has now been fixed.
 
+## New request parameter handling
+
+In the process of updating the body handling, a simpler pattern for query
+parameters was also implemented.
+
 ## Additional merge status cases
 
 Some additional merge status names for merge requests were missing and have

src/api.rs

@@ -77,7 +77,6 @@ pub mod users;
 pub use self::client::Client;
 
 pub use self::endpoint::Endpoint;
-pub use self::endpoint::Pairs;
 
 pub use self::error::ApiError;
 pub use self::error::BodyError;

src/api/common.rs

@@ -88,6 +88,12 @@ impl SortOrder {
     }
 }
 
+impl ParamValue<'static> for SortOrder {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str().into()
+    }
+}
+
 /// States for features or flags.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum EnableState {
@@ -198,20 +204,9 @@ impl ParamValue<'static> for VisibilityLevel {
     }
 }
 
-/// The string representation of booleans for GitLab.
-pub fn bool_str(b: bool) -> &'static str {
-    if b {
-        "true"
-    } else {
-        "false"
-    }
-}
-
 #[cfg(test)]
 mod tests {
-    use crate::api::common::{
-        self, AccessLevel, EnableState, NameOrId, SortOrder, VisibilityLevel,
-    };
+    use crate::api::common::{AccessLevel, EnableState, NameOrId, SortOrder, VisibilityLevel};
 
     #[test]
     fn access_level_as_str() {
@@ -310,13 +305,4 @@ mod tests {
             assert_eq!(i.as_str(), *s);
         }
     }
-
-    #[test]
-    fn bool_str() {
-        let items = &[(true, "true"), (false, "false")];
-
-        for (i, s) in items {
-            assert_eq!(common::bool_str(*i), *s);
-        }
-    }
 }

src/api/endpoint.rs

@@ -8,13 +8,8 @@ use std::borrow::Cow;
 
 use reqwest::{header, Method};
 use serde::de::DeserializeOwned;
-use url::form_urlencoded::Serializer;
-use url::UrlQuery;
 
-use crate::api::{ApiError, BodyError, Client, Query};
-
-/// A type for managing query parameters.
-pub type Pairs<'a> = Serializer<'a, UrlQuery<'a>>;
+use crate::api::{ApiError, BodyError, Client, Query, QueryParams};
 
 /// A trait for providing the necessary information for a single REST API endpoint.
 pub trait Endpoint {
@@ -23,9 +18,10 @@ pub trait Endpoint {
     /// The path to the endpoint.
     fn endpoint(&self) -> Cow<'static, str>;
 
-    /// Add query parameters for the endpoint.
-    #[allow(unused_variables)]
-    fn add_parameters(&self, pairs: Pairs) {}
+    /// Query parameters for the endpoint.
+    fn parameters(&self) -> QueryParams {
+        QueryParams::default()
+    }
 
     /// The body for the endpoint.
     ///
@@ -43,7 +39,7 @@ where
 {
     fn query(&self, client: &C) -> Result<T, ApiError<C::Error>> {
         let mut url = client.rest_endpoint(&self.endpoint())?;
-        self.add_parameters(url.query_pairs_mut());
+        self.parameters().add_to_url(&mut url);
 
         let req = client.build_rest(self.method(), url);
         let req = if let Some((mime, data)) = self.body()? {

src/api/endpoint_prelude.rs

@@ -18,5 +18,4 @@ pub use crate::api::Client;
 pub use crate::api::Endpoint;
 pub use crate::api::FormParams;
 pub use crate::api::Pageable;
-pub use crate::api::Pairs;
 pub use crate::api::QueryParams;

src/api/groups/group.rs

@@ -6,7 +6,7 @@
 
 use derive_builder::Builder;
 
-use crate::api::common::{self, NameOrId};
+use crate::api::common::NameOrId;
 use crate::api::endpoint_prelude::*;
 
 /// Query for a specific group on an instance.
@@ -42,15 +42,17 @@ impl<'a> Endpoint for Group<'a> {
         format!("groups/{}", self.group).into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
-        self.with_custom_attributes
-            .map(|value| pairs.append_pair("with_custom_attributes", common::bool_str(value)));
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
+        params.push_opt("with_custom_attributes", self.with_custom_attributes);
 
         #[allow(deprecated)]
         {
-            self.with_projects
-                .map(|value| pairs.append_pair("with_projects", common::bool_str(value)));
+            params.push_opt("with_projects", self.with_projects);
         }
+
+        params
     }
 }
 

src/api/groups/groups.rs

@@ -8,8 +8,9 @@ use std::collections::HashSet;
 
 use derive_builder::Builder;
 
-use crate::api::common::{self, AccessLevel, SortOrder};
+use crate::api::common::{AccessLevel, SortOrder};
 use crate::api::endpoint_prelude::*;
+use crate::api::ParamValue;
 
 /// Keys group results may be ordered by.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -123,28 +124,28 @@ impl<'a> Endpoint for Groups<'a> {
         "groups".into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
-        self.search
-            .as_ref()
-            .map(|value| pairs.append_pair("search", value));
-        self.skip_groups.iter().for_each(|value| {
-            pairs.append_pair("skip_groups[]", &format!("{}", value));
-        });
-        self.all_available
-            .map(|value| pairs.append_pair("all_available", common::bool_str(value)));
-        self.owned
-            .map(|value| pairs.append_pair("owned", common::bool_str(value)));
-        self.min_access_level
-            .map(|value| pairs.append_pair("min_access_level", value.as_str()));
-        self.statistics
-            .map(|value| pairs.append_pair("statistics", common::bool_str(value)));
-        self.with_custom_attributes
-            .map(|value| pairs.append_pair("with_custom_attributes", common::bool_str(value)));
-
-        self.order_by
-            .map(|value| pairs.append_pair("order_by", value.as_str()));
-        self.sort
-            .map(|value| pairs.append_pair("sort", value.as_str()));
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
+        params
+            .push_opt("search", self.search.as_ref())
+            .extend(
+                self.skip_groups
+                    .iter()
+                    .map(|&value| ("skip_groups[]", value)),
+            )
+            .push_opt("all_available", self.all_available)
+            .push_opt("owned", self.owned)
+            .push_opt(
+                "min_access_level",
+                self.min_access_level.map(|level| level.as_str()),
+            )
+            .push_opt("statistics", self.statistics)
+            .push_opt("with_custom_attributes", self.with_custom_attributes)
+            .push_opt("order_by", self.order_by)
+            .push_opt("sort", self.sort);
+
+        params
     }
 }
 

src/api/groups/members/members.rs

@@ -61,13 +61,14 @@ impl<'a> Endpoint for GroupMembers<'a> {
         format!("groups/{}/members", self.group).into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
-        self.query
-            .as_ref()
-            .map(|value| pairs.append_pair("query", value));
-        self.user_ids.iter().for_each(|value| {
-            pairs.append_pair("user_ids[]", &format!("{}", value));
-        });
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
+        params
+            .push_opt("query", self.query.as_ref())
+            .extend(self.user_ids.iter().map(|&value| ("user_ids[]", value)));
+
+        params
     }
 }
 

src/api/ignore.rs

@@ -28,7 +28,7 @@ where
 {
     fn query(&self, client: &C) -> Result<(), ApiError<C::Error>> {
         let mut url = client.rest_endpoint(&self.endpoint.endpoint())?;
-        self.endpoint.add_parameters(url.query_pairs_mut());
+        self.endpoint.parameters().add_to_url(&mut url);
 
         let req = client.build_rest(self.endpoint.method(), url);
         let req = if let Some((mime, data)) = self.endpoint.body()? {

src/api/paged.rs

@@ -186,7 +186,7 @@ where
     fn query(&self, client: &C) -> Result<Vec<T>, ApiError<C::Error>> {
         let url = {
             let mut url = client.rest_endpoint(&self.endpoint.endpoint())?;
-            self.endpoint.add_parameters(url.query_pairs_mut());
+            self.endpoint.parameters().add_to_url(&mut url);
             url
         };
 

src/api/params.rs

@@ -14,7 +14,7 @@ use std::borrow::Cow;
 use chrono::{DateTime, NaiveDate, Utc};
 use url::Url;
 
-use crate::api::{common, BodyError};
+use crate::api::BodyError;
 
 /// A trait representing a parameter value.
 pub trait ParamValue<'a> {
@@ -24,7 +24,11 @@ pub trait ParamValue<'a> {
 
 impl ParamValue<'static> for bool {
     fn as_value(self) -> Cow<'static, str> {
-        common::bool_str(self).into()
+        if self {
+            "true".into()
+        } else {
+            "false".into()
+        }
     }
 }
 
@@ -188,3 +192,17 @@ impl<'a> QueryParams<'a> {
         pairs.extend_pairs(self.params.iter());
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::api::ParamValue;
+
+    #[test]
+    fn bool_str() {
+        let items = &[(true, "true"), (false, "false")];
+
+        for (i, s) in items {
+            assert_eq!((*i).as_value(), *s);
+        }
+    }
+}

src/api/projects/environments/environments.rs

@@ -77,17 +77,21 @@ impl<'a> Endpoint for Environments<'a> {
         format!("projects/{}/environments", self.project).into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
         if let Some(name_or_search) = self.name_or_search.as_ref() {
             match name_or_search {
                 NameOrSearch::Name(name) => {
-                    pairs.append_pair("name", name);
+                    params.push("name", name);
                 },
                 NameOrSearch::Search(search) => {
-                    pairs.append_pair("search", search);
+                    params.push("search", search);
                 },
             }
         }
+
+        params
     }
 }
 

src/api/projects/issues/issues.rs

@@ -10,8 +10,9 @@ use chrono::{DateTime, Utc};
 use derive_builder::Builder;
 use itertools::Itertools;
 
-use crate::api::common::{self, NameOrId, SortOrder};
+use crate::api::common::{NameOrId, SortOrder};
 use crate::api::endpoint_prelude::*;
+use crate::api::ParamValue;
 
 /// Filters for issue states.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -31,6 +32,12 @@ impl IssueState {
     }
 }
 
+impl ParamValue<'static> for IssueState {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str().into()
+    }
+}
+
 #[derive(Debug, Clone)]
 enum Labels<'a> {
     Any,
@@ -48,6 +55,12 @@ impl<'a> Labels<'a> {
     }
 }
 
+impl<'a, 'b: 'a> ParamValue<'static> for &'b Labels<'a> {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str()
+    }
+}
+
 /// Filter issues by a scope.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum IssueScope {
@@ -69,6 +82,12 @@ impl IssueScope {
     }
 }
 
+impl ParamValue<'static> for IssueScope {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str().into()
+    }
+}
+
 #[derive(Debug, Clone)]
 enum Assignee<'a> {
     Assigned,
@@ -78,19 +97,19 @@ enum Assignee<'a> {
 }
 
 impl<'a> Assignee<'a> {
-    fn add_query(&self, pairs: &mut Pairs) {
+    fn add_params<'b>(&'b self, params: &mut QueryParams<'b>) {
         match self {
             Assignee::Assigned => {
-                pairs.append_pair("assignee_id", "Any");
+                params.push("assignee_id", "Any");
             },
             Assignee::Unassigned => {
-                pairs.append_pair("assignee_id", "None");
+                params.push("assignee_id", "None");
             },
             Assignee::Id(id) => {
-                pairs.append_pair("assignee_id", &format!("{}", id));
+                params.push("assignee_id", *id);
             },
             Assignee::Usernames(usernames) => {
-                pairs.extend_pairs(usernames.iter().map(|value| ("assignee_username[]", value)));
+                params.extend(usernames.iter().map(|value| ("assignee_username[]", value)));
             },
         }
     }
@@ -113,6 +132,12 @@ impl<'a> ReactionEmoji<'a> {
     }
 }
 
+impl<'a, 'b: 'a> ParamValue<'a> for &'b ReactionEmoji<'a> {
+    fn as_value(self) -> Cow<'a, str> {
+        self.as_str().into()
+    }
+}
+
 /// Filter issues by weight.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum IssueWeight {
@@ -134,6 +159,12 @@ impl IssueWeight {
     }
 }
 
+impl ParamValue<'static> for IssueWeight {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str()
+    }
+}
+
 /// Keys issue results may be ordered by.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum IssueOrderBy {
@@ -181,6 +212,12 @@ impl IssueOrderBy {
     }
 }
 
+impl ParamValue<'static> for IssueOrderBy {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str().into()
+    }
+}
+
 /// Query for issues within a project.
 ///
 /// TODO: Negation (not) filters are not yet supported.
@@ -405,75 +442,42 @@ impl<'a> Endpoint for Issues<'a> {
         format!("projects/{}/issues", self.project).into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
-        pairs.extend_pairs(
-            self.iids
-                .iter()
-                .map(|value| ("iids[]", format!("{}", value))),
-        );
-        self.state
-            .map(|value| pairs.append_pair("state", value.as_str()));
-        self.labels
-            .as_ref()
-            .map(|value| pairs.append_pair("labels", &value.as_str()));
-        self.with_labels_details
-            .map(|value| pairs.append_pair("with_labels_details", common::bool_str(value)));
-        self.milestone
-            .as_ref()
-            .map(|value| pairs.append_pair("milestone", value));
-        self.scope
-            .map(|value| pairs.append_pair("scope", value.as_str()));
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
+        params
+            .extend(self.iids.iter().map(|&value| ("iids[]", value)))
+            .push_opt("state", self.state)
+            .push_opt("labels", self.labels.as_ref())
+            .push_opt("with_labels_details", self.with_labels_details)
+            .push_opt("milestone", self.milestone.as_ref())
+            .push_opt("scope", self.scope)
+            .push_opt("my_reaction_emoji", self.my_reaction_emoji.as_ref())
+            .push_opt("weight", self.weight)
+            .push_opt("search", self.search.as_ref())
+            .push_opt("created_after", self.created_after)
+            .push_opt("created_before", self.created_before)
+            .push_opt("updated_after", self.updated_after)
+            .push_opt("updated_before", self.updated_before)
+            .push_opt("confidential", self.confidential)
+            .push_opt("order_by", self.order_by)
+            .push_opt("sort", self.sort);
+
         if let Some(author) = self.author.as_ref() {
             match author {
                 NameOrId::Name(name) => {
-                    pairs.append_pair("author_username", name);
+                    params.push("author_username", name);
                 },
                 NameOrId::Id(id) => {
-                    pairs.append_pair("author_id", &format!("{}", id));
+                    params.push("author_id", *id);
                 },
             }
         }
         if let Some(assignee) = self.assignee.as_ref() {
-            assignee.add_query(&mut pairs);
+            assignee.add_params(&mut params);
         }
-        self.my_reaction_emoji
-            .as_ref()
-            .map(|value| pairs.append_pair("my_reaction_emoji", value.as_str()));
-        self.weight
-            .map(|value| pairs.append_pair("weight", &value.as_str()));
-        self.search
-            .as_ref()
-            .map(|value| pairs.append_pair("search", value));
-        self.created_after.map(|value| {
-            pairs.append_pair(
-                "created_after",
-                &value.to_rfc3339_opts(chrono::SecondsFormat::Secs, true),
-            )
-        });
-        self.created_before.map(|value| {
-            pairs.append_pair(
-                "created_before",
-                &value.to_rfc3339_opts(chrono::SecondsFormat::Secs, true),
-            )
-        });
-        self.updated_after.map(|value| {
-            pairs.append_pair(
-                "updated_after",
-                &value.to_rfc3339_opts(chrono::SecondsFormat::Secs, true),
-            )
-        });
-        self.updated_before.map(|value| {
-            pairs.append_pair(
-                "updated_before",
-                &value.to_rfc3339_opts(chrono::SecondsFormat::Secs, true),
-            )
-        });
-        self.confidential
-            .map(|value| pairs.append_pair("confidential", common::bool_str(value)));
-        self.order_by
-            .map(|value| pairs.append_pair("order_by", value.as_str()));
-        self.sort
-            .map(|value| pairs.append_pair("sort", value.as_str()));
+
+        params
     }
 }
 

src/api/projects/jobs/jobs.rs

@@ -10,6 +10,7 @@ use derive_builder::Builder;
 
 use crate::api::common::NameOrId;
 use crate::api::endpoint_prelude::*;
+use crate::api::ParamValue;
 
 /// Scopes for jobs.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -48,6 +49,12 @@ impl JobScope {
     }
 }
 
+impl ParamValue<'static> for JobScope {
+    fn as_value(self) -> Cow<'static, str> {
+        self.as_str().into()
+    }
+}
+
 /// Query for jobs within a project.
 #[derive(Debug, Builder)]
 pub struct Jobs<'a> {
@@ -93,10 +100,12 @@ impl<'a> Endpoint for Jobs<'a> {
         format!("projects/{}/jobs", self.project).into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
-        self.scopes.iter().for_each(|value| {
-            pairs.append_pair("scope[]", value.as_str());
-        });
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
+        params.extend(self.scopes.iter().map(|&value| ("scope[]", value)));
+
+        params
     }
 }
 

src/api/projects/labels/label.rs

@@ -6,7 +6,7 @@
 
 use derive_builder::Builder;
 
-use crate::api::common::{self, NameOrId};
+use crate::api::common::NameOrId;
 use crate::api::endpoint_prelude::*;
 
 /// Query for a label within a project.
@@ -42,9 +42,12 @@ impl<'a> Endpoint for Label<'a> {
         format!("projects/{}/labels/{}", self.project, self.label).into()
     }
 
-    fn add_parameters(&self, mut pairs: Pairs) {
-        self.include_ancestor_groups
-            .map(|value| pairs.append_pair("include_ancestor_groups", common::bool_str(value)));
+    fn parameters(&self) -> QueryParams {
+        let mut params = QueryParams::default();
+
+        params.push_opt("include_ancestor_groups", self.include_ancestor_groups);
+
+        params
     }
 }
 

src/api/projects/labels/labels.rs

@@ -6,7 +6,7 @@
 
 use derive_builder::Builder;
 
-use crate::api::common::{self, NameOrId};
+use crate::api::common::NameOrId;
 use crate::api::endpoint_prelude::*;
 
 /// Query for labels within a project.