Commit c888af88 authored by Brad King's avatar Brad King Committed by Kitware Robot

Merge topic 'better-request-body-handling'

db8172fc api: simplify query parameter passing
e4b7124a api/params: add a structure for handling query parameters
3b28489e api: fix POST and PUT parameter passing
c72bc11f api/params: add a structure for handling form parameters
e6fd1997 api/projects: fix expiration policy parameter name
e1a4e284 api/project: fix description parameter name
1d4271a6 api/common: fix VisibilityLevel::as_str lifetime
760a0760 api: handle endpoints with body data better
...
Acked-by: Kitware Robot's avatarKitware Robot <kwrobot@kitware.com>
Merge-request: !235
parents 3683c5bf db8172fc
# v0.1300.0 (unreleased)
## New request body handling
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
been added.
# v0.1210.1
## New API strategy
......
......@@ -31,6 +31,7 @@ chrono = { version = "~0.4", features = ["serde"] }
graphql_client = { version = "~0.8", optional = true }
serde = { version = "~1.0", features = ["derive"] }
serde_json = "^1.0"
serde_urlencoded = "~0.5"
[dev-dependencies]
itertools = { version = "~0.8" }
......@@ -62,6 +62,7 @@ mod endpoint;
mod error;
mod ignore;
mod paged;
mod params;
mod query;
mod raw;
mod sudo;
......@@ -76,9 +77,9 @@ 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;
pub use self::ignore::ignore;
pub use self::ignore::Ignore;
......@@ -90,6 +91,10 @@ pub use self::paged::Paged;
pub use self::paged::Pagination;
pub use self::paged::PaginationError;
pub use self::params::FormParams;
pub use self::params::ParamValue;
pub use self::params::QueryParams;
pub use self::query::Query;
pub use self::raw::raw;
......
......@@ -14,6 +14,8 @@ use std::fmt;
use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS};
use crate::api::ParamValue;
/// Access levels for groups and projects.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub enum AccessLevel {
......@@ -86,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 {
......@@ -105,6 +113,12 @@ impl EnableState {
}
}
impl ParamValue<'static> for EnableState {
fn as_value(self) -> Cow<'static, str> {
self.as_str().into()
}
}
/// A strucutre for storing a name or ID where either is allowed.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum NameOrId<'a> {
......@@ -175,7 +189,7 @@ pub enum VisibilityLevel {
impl VisibilityLevel {
/// The string representation of the visibility level.
pub fn as_str(&self) -> &str {
pub fn as_str(self) -> &'static str {
match self {
VisibilityLevel::Public => "public",
VisibilityLevel::Internal => "internal",
......@@ -184,20 +198,15 @@ impl VisibilityLevel {
}
}
/// The string representation of booleans for GitLab.
pub fn bool_str(b: bool) -> &'static str {
if b {
"true"
} else {
"false"
impl ParamValue<'static> for VisibilityLevel {
fn as_value(self) -> Cow<'static, str> {
self.as_str().into()
}
}
#[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() {
......@@ -296,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);
}
}
}
......@@ -6,15 +6,10 @@
use std::borrow::Cow;
use reqwest::Method;
use reqwest::{header, Method};
use serde::de::DeserializeOwned;
use url::form_urlencoded::Serializer;
use url::UrlQuery;
use crate::api::{ApiError, 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,13 +18,16 @@ 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()
}
/// Form data for the endpoint.
fn form_data(&self) -> Vec<u8> {
Vec::new()
/// The body for the endpoint.
///
/// Returns the `Content-Encoding` header for the data as well as the data itself.
fn body(&self) -> Result<Option<(&'static str, Vec<u8>)>, BodyError> {
Ok(None)
}
}
......@@ -41,11 +39,14 @@ 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());
let req = client
.build_rest(self.method(), url)
.form(&self.form_data());
self.parameters().add_to_url(&mut url);
let req = client.build_rest(self.method(), url);
let req = if let Some((mime, data)) = self.body()? {
req.header(header::CONTENT_TYPE, mime).body(data)
} else {
req
};
let rsp = client.rest(req)?;
let status = rsp.status();
let v = serde_json::from_reader(rsp)?;
......
......@@ -13,7 +13,9 @@ pub use std::borrow::Cow;
pub use reqwest::Method;
pub use crate::api::BodyError;
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;
......@@ -11,6 +11,25 @@ use thiserror::Error;
use crate::api::PaginationError;
/// Errors which may occur when creating form data.
#[derive(Debug, Error)]
// TODO #[non_exhaustive]
pub enum BodyError {
/// Body data could not be serialized from form parameters.
#[error("failed to URL encode form parameters: {}", source)]
UrlEncoded {
/// The source of the error.
#[from]
source: serde_urlencoded::ser::Error,
},
/// This is here to force `_` matching right now.
///
/// **DO NOT USE**
#[doc(hidden)]
#[error("unreachable...")]
_NonExhaustive,
}
/// Errors which may occur when using API endpoints.
#[derive(Debug, Error)]
// TODO #[non_exhaustive]
......@@ -31,6 +50,13 @@ where
#[from]
source: url::ParseError,
},
/// Body data could not be created.
#[error("failed to create form data: {}", source)]
Body {
/// The source of the error.
#[from]
source: BodyError,
},
/// JSON deserialization from GitLab failed.
#[error("could not parse JSON response: {}", source)]
Json {
......
......@@ -6,8 +6,9 @@
use derive_builder::Builder;
use crate::api::common::{self, VisibilityLevel};
use crate::api::common::VisibilityLevel;
use crate::api::endpoint_prelude::*;
use crate::api::ParamValue;
/// Access levels for creating a project within a group.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
......@@ -30,6 +31,12 @@ impl GroupProjectCreationAccessLevel {
}
}
impl ParamValue<'static> for GroupProjectCreationAccessLevel {
fn as_value(self) -> Cow<'static, str> {
self.as_str().into()
}
}
/// Access levels for creating a subgroup within a group.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SubgroupCreationAccessLevel {
......@@ -48,6 +55,12 @@ impl SubgroupCreationAccessLevel {
}
}
impl ParamValue<'static> for SubgroupCreationAccessLevel {
fn as_value(self) -> Cow<'static, str> {
self.as_str().into()
}
}
/// Branch protection rules for groups.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum BranchProtection {
......@@ -69,6 +82,12 @@ impl BranchProtection {
}
}
impl ParamValue<'static> for BranchProtection {
fn as_value(self) -> Cow<'static, str> {
self.as_str().into()
}
}
/// Create a new group on an instance.
#[derive(Debug, Builder)]
#[builder(setter(strip_option))]
......@@ -151,47 +170,40 @@ impl<'a> Endpoint for CreateGroup<'a> {
"groups".into()
}
fn add_parameters(&self, mut pairs: Pairs) {
pairs.append_pair("name", &self.name);
pairs.append_pair("path", &self.path);
fn body(&self) -> Result<Option<(&'static str, Vec<u8>)>, BodyError> {
let mut params = FormParams::default();
params
.push("name", &self.name)
.push("path", &self.path)
.push_opt("description", self.description.as_ref())
.push_opt("membership_lock", self.membership_lock)
.push_opt("visibility", self.visibility)
.push_opt("share_with_group_lock", self.share_with_group_lock)
.push_opt(
"require_two_factor_authentication",
self.require_two_factor_authentication,
)
.push_opt("two_factor_grace_period", self.two_factor_grace_period)
.push_opt("project_creation_level", self.project_creation_level)
.push_opt("auto_devops_enabled", self.auto_devops_enabled)
.push_opt("subgroup_creation_level", self.subgroup_creation_level)
.push_opt("emails_disabled", self.emails_disabled)
.push_opt("mentions_disabled", self.mentions_disabled)
.push_opt("lfs_enabled", self.lfs_enabled)
.push_opt("request_access_enabled", self.request_access_enabled)
.push_opt("parent_id", self.parent_id)
.push_opt("default_branch_protection", self.default_branch_protection)
.push_opt(
"shared_runners_minutes_limit",
self.shared_runners_minutes_limit,
)
.push_opt(
"extra_shared_runners_minutes_limit",
self.extra_shared_runners_minutes_limit,
);
self.description
.as_ref()
.map(|value| pairs.append_pair("description", value));
self.membership_lock
.map(|value| pairs.append_pair("membership_lock", common::bool_str(value)));
self.visibility
.map(|value| pairs.append_pair("visibility", value.as_str()));
self.share_with_group_lock
.map(|value| pairs.append_pair("share_with_group_lock", common::bool_str(value)));
self.require_two_factor_authentication.map(|value| {
pairs.append_pair("require_two_factor_authentication", common::bool_str(value))
});
self.two_factor_grace_period
.map(|value| pairs.append_pair("two_factor_grace_period", &format!("{}", value)));
self.project_creation_level
.map(|value| pairs.append_pair("project_creation_level", value.as_str()));
self.auto_devops_enabled
.map(|value| pairs.append_pair("auto_devops_enabled", common::bool_str(value)));
self.subgroup_creation_level
.map(|value| pairs.append_pair("subgroup_creation_level", value.as_str()));
self.emails_disabled
.map(|value| pairs.append_pair("emails_disabled", common::bool_str(value)));
self.mentions_disabled
.map(|value| pairs.append_pair("mentions_disabled", common::bool_str(value)));
self.lfs_enabled
.map(|value| pairs.append_pair("lfs_enabled", common::bool_str(value)));
self.request_access_enabled
.map(|value| pairs.append_pair("request_access_enabled", common::bool_str(value)));
self.parent_id
.map(|value| pairs.append_pair("parent_id", &format!("{}", value)));
self.default_branch_protection
.map(|value| pairs.append_pair("default_branch_protection", value.as_str()));
self.shared_runners_minutes_limit
.map(|value| pairs.append_pair("shared_runners_minutes_limit", &format!("{}", value)));
self.extra_shared_runners_minutes_limit.map(|value| {
pairs.append_pair("extra_shared_runners_minutes_limit", &format!("{}", value))
});
params.into_body()
}
}
......
......@@ -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
}
}
......
......@@ -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)]
......@@ -39,6 +40,12 @@ impl GroupOrderBy {
}
}
impl ParamValue<'static> for GroupOrderBy {
fn as_value(self) -> Cow<'static, str> {
self.as_str().into()
}
}
/// Query for groups on an instance.
#[derive(Debug, Builder)]
#[builder(setter(strip_option))]
......@@ -117,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
}
}
......
......@@ -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
}
}
......
......@@ -4,6 +4,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
use reqwest::header;
use crate::api::{ApiError, Client, Endpoint, Query};
/// A query modifier that ignores the data returned from an endpoint.
......@@ -26,11 +28,14 @@ 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)
.form(&self.endpoint.form_data());
let req = client.build_rest(self.endpoint.method(), url);
let req = if let Some((mime, data)) = self.endpoint.body()? {
req.header(header::CONTENT_TYPE, mime).body(data)
} else {
req
};
let rsp = client.rest(req)?;
if !rsp.status().is_success() {
let v = serde_json::from_reader(rsp)?;
......
......@@ -4,7 +4,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
use reqwest::header::HeaderMap;
use reqwest::header::{self, HeaderMap};
use serde::de::DeserializeOwned;
use thiserror::Error;
use url::Url;
......@@ -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
};
......@@ -198,6 +198,8 @@ where
let mut next_url = None;
let use_keyset_pagination = self.endpoint.use_keyset_pagination();
let body = self.endpoint.body()?;
loop {
let page_url = if let Some(url) = next_url.take() {
url
......@@ -220,6 +222,11 @@ where
};
let req = client.build_rest(self.endpoint.method(), page_url);
let req = if let Some((mime, data)) = body.as_ref() {
req.header(header::CONTENT_TYPE, *mime).body(data.clone())
} else {
req
};
let rsp = client.rest(req)?;
let status = rsp.status();
......
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
//! Endpoint prelude
//!
//! This module re-exports all of the types needed for endpoints to implement the
//! [`Endpoint`](../trait.Endpoint.html) trait.
use std::borrow::Cow;
use chrono::{DateTime, NaiveDate, Utc};
use url::Url;
use crate::api::BodyError;
/// A trait representing a parameter value.