Skip to content

Commit

Permalink
cleanup: error handling for query parameters
Browse files Browse the repository at this point in the history 
Only the conversion from a message type to a `serde_json::Value` can
result in an error. If we convert first, then `gax::query_parameter`
can work without errors, can operate on values (vs. references).
  • Loading branch information
@coryan
coryan committed Jan 21, 2025
1 parent 48706f4 commit a72d04c
Show file tree
Hide file tree
Showing 12 changed files with 158 additions and 151 deletions.
4 changes: 2 additions & 2 deletions generator/internal/language/rust.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,9 +540,9 @@ func rustAddQueryParameter(f *api.Field) string {
// few requests use nested objects as query parameters. Furthermore,
// the conversion is skipped if the object field is `None`.`
if f.Optional || f.Repeated {
return fmt.Sprintf(`let builder = req.%s.iter().try_fold(builder, |builder, p| { use gax::query_parameter::QueryParameter; serde_json::to_value(p).map_err(Error::serde)?.add(builder, "%s").map_err(Error::other) })?;`, rustToSnake(f.Name), f.JSONName)
return fmt.Sprintf(`let builder = req.%s.as_ref().map(|p| serde_json::to_value(p).map_err(Error::serde) ).transpose()?.into_iter().fold(builder, |builder, v| { use gax::query_parameter::QueryParameter; v.add(builder, "%s") });`, rustToSnake(f.Name), f.JSONName)
}
return fmt.Sprintf(`let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.%s).map_err(Error::serde)?.add(builder, "%s").map_err(Error::other)? };`, rustToSnake(f.Name), f.JSONName)
return fmt.Sprintf(`let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.%s).map_err(Error::serde)?.add(builder, "%s") };`, rustToSnake(f.Name), f.JSONName)
default:
if f.Optional || f.Repeated {
return fmt.Sprintf(`let builder = req.%s.iter().fold(builder, |builder, p| builder.query(&[("%s", p)]));`, rustToSnake(f.Name), f.JSONName)
Expand Down
6 changes: 3 additions & 3 deletions generator/internal/language/rust_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1121,15 +1121,15 @@ func TestRust_AsQueryParameter(t *testing.T) {
field *api.Field
want string
}{
{optionsField, `let builder = req.options_field.iter().try_fold(builder, |builder, p| { use gax::query_parameter::QueryParameter; serde_json::to_value(p).map_err(Error::serde)?.add(builder, "optionsField").map_err(Error::other) })?;`},
{optionsField, `let builder = req.options_field.as_ref().map(|p| serde_json::to_value(p).map_err(Error::serde) ).transpose()?.into_iter().fold(builder, |builder, v| { use gax::query_parameter::QueryParameter; v.add(builder, "optionsField") });`},
{requiredField, `let builder = builder.query(&[("requiredField", &req.required_field)]);`},
{optionalField, `let builder = req.optional_field.iter().fold(builder, |builder, p| builder.query(&[("optionalField", p)]));`},
{repeatedField, `let builder = req.repeated_field.iter().fold(builder, |builder, p| builder.query(&[("repeatedField", p)]));`},
{requiredEnumField, `let builder = builder.query(&[("requiredEnumField", &req.required_enum_field.value())]);`},
{optionalEnumField, `let builder = req.optional_enum_field.iter().fold(builder, |builder, p| builder.query(&[("optionalEnumField", p.value())]));`},
{repeatedEnumField, `let builder = req.repeated_enum_field.iter().fold(builder, |builder, p| builder.query(&[("repeatedEnumField", p.value())]));`},
{requiredFieldMaskField, `let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.required_field_mask).map_err(Error::serde)?.add(builder, "requiredFieldMask").map_err(Error::other)? };`},
{optionalFieldMaskField, `let builder = req.optional_field_mask.iter().try_fold(builder, |builder, p| { use gax::query_parameter::QueryParameter; serde_json::to_value(p).map_err(Error::serde)?.add(builder, "optionalFieldMask").map_err(Error::other) })?;`},
{requiredFieldMaskField, `let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.required_field_mask).map_err(Error::serde)?.add(builder, "requiredFieldMask") };`},
{optionalFieldMaskField, `let builder = req.optional_field_mask.as_ref().map(|p| serde_json::to_value(p).map_err(Error::serde) ).transpose()?.into_iter().fold(builder, |builder, v| { use gax::query_parameter::QueryParameter; v.add(builder, "optionalFieldMask") });`},
} {
got := rustAddQueryParameter(test.field)
if test.want != got {
Expand Down
4 changes: 2 additions & 2 deletions generator/testdata/rust/openapi/golden/src/transport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ impl crate::traits::SecretManagerService for SecretManagerService {
))
.query(&[("alt", "json")])
.header("x-goog-api-client", reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER));
let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.update_mask).map_err(Error::serde)?.add(builder, "updateMask").map_err(Error::other)? };
let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.update_mask).map_err(Error::serde)?.add(builder, "updateMask") };
self.inner.execute(
builder,
Some(req.request_body),
Expand Down Expand Up @@ -328,7 +328,7 @@ impl crate::traits::SecretManagerService for SecretManagerService {
))
.query(&[("alt", "json")])
.header("x-goog-api-client", reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER));
let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.update_mask).map_err(Error::serde)?.add(builder, "updateMask").map_err(Error::other)? };
let builder = { use gax::query_parameter::QueryParameter; serde_json::to_value(&req.update_mask).map_err(Error::serde)?.add(builder, "updateMask") };
self.inner.execute(
builder,
Some(req.request_body),
Expand Down
4 changes: 2 additions & 2 deletions generator/testdata/rust/protobuf/golden/secretmanager/src/transport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ impl crate::traits::SecretManagerService for SecretManagerService {
))
.query(&[("alt", "json")])
.header("x-goog-api-client", reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER));
let builder = req.update_mask.iter().try_fold(builder, |builder, p| { use gax::query_parameter::QueryParameter; serde_json::to_value(p).map_err(Error::serde)?.add(builder, "updateMask").map_err(Error::other) })?;
let builder = req.update_mask.as_ref().map(|p| serde_json::to_value(p).map_err(Error::serde) ).transpose()?.into_iter().fold(builder, |builder, v| { use gax::query_parameter::QueryParameter; v.add(builder, "updateMask") });
self.inner.execute(
builder,
Some(req.secret),
Expand Down Expand Up @@ -308,7 +308,7 @@ impl crate::traits::SecretManagerService for SecretManagerService {
))
.query(&[("alt", "json")])
.header("x-goog-api-client", reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER));
let builder = req.options.iter().try_fold(builder, |builder, p| { use gax::query_parameter::QueryParameter; serde_json::to_value(p).map_err(Error::serde)?.add(builder, "options").map_err(Error::other) })?;
let builder = req.options.as_ref().map(|p| serde_json::to_value(p).map_err(Error::serde) ).transpose()?.into_iter().fold(builder, |builder, v| { use gax::query_parameter::QueryParameter; v.add(builder, "options") });
self.inner.execute(
builder,
None::<gax::http_client::NoBody>,
Expand Down
34 changes: 13 additions & 21 deletions src/gax/src/query_parameter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,29 +29,21 @@
//! public because we will generate many crates (roughly one per service), and
//! most of these crates will use these helpers.
type Result<T> = std::result::Result<T, crate::request_parameter::Error>;

/// [QueryParameter] is a trait representing types that can be used as a query
/// parameter.
pub trait QueryParameter {
// TODO(#610) - remove the add() method.
fn add(&self, builder: reqwest::RequestBuilder, name: &str) -> Result<reqwest::RequestBuilder>;
fn append(&self, builder: reqwest::RequestBuilder, name: &str) -> reqwest::RequestBuilder;
fn add(self, builder: reqwest::RequestBuilder, name: &str) -> reqwest::RequestBuilder;
}

impl QueryParameter for serde_json::Value {
fn add(&self, builder: reqwest::RequestBuilder, name: &str) -> Result<reqwest::RequestBuilder> {
Ok(self.append(builder, name))
}

fn append(&self, builder: reqwest::RequestBuilder, name: &str) -> reqwest::RequestBuilder {
match &self {
Self::Object(object) => object.iter().fold(builder, |builder, (k, v)| {
v.append(builder, format!("{name}.{k}").as_str())
fn add(self, builder: reqwest::RequestBuilder, name: &str) -> reqwest::RequestBuilder {
match self {
Self::Object(object) => object.into_iter().fold(builder, |builder, (k, v)| {
v.add(builder, format!("{name}.{k}").as_str())
}),
Self::Array(array) => array
.iter()
.fold(builder, |builder, v| v.append(builder, name)),
.into_iter()
.fold(builder, |builder, v| v.add(builder, name)),
Self::Null => builder,
Self::String(s) => builder.query(&[(name, s)]),
Self::Number(n) => builder.query(&[(name, format!("{n}"))]),
Expand Down Expand Up @@ -93,7 +85,7 @@ mod tests {
let builder = reqwest::Client::builder()
.build()?
.get("https://test.googleapis.com/v1/unused");
let builder = value.add(builder, "name")?;
let builder = value.add(builder, "name");
let request = builder.build()?;
assert_eq!(
split_query(&request),
Expand All @@ -118,7 +110,7 @@ mod tests {
let builder = reqwest::Client::builder()
.build()?
.get("https://test.googleapis.com/v1/unused");
let builder = value.add(builder, "name")?;
let builder = value.add(builder, "name");
let request = builder.build()?;
assert_eq!(
split_query(&request),
Expand All @@ -133,7 +125,7 @@ mod tests {
let builder = reqwest::Client::builder()
.build()?
.get("https://test.googleapis.com/v1/unused");
let builder = value.add(builder, "name")?;
let builder = value.add(builder, "name");
let request = builder.build()?;
assert_eq!(split_query(&request), Vec::<&str>::new());
Ok(())
Expand All @@ -145,7 +137,7 @@ mod tests {
let builder = reqwest::Client::builder()
.build()?
.get("https://test.googleapis.com/v1/unused");
let builder = value.add(builder, "name")?;
let builder = value.add(builder, "name");
let request = builder.build()?;
assert_eq!(split_query(&request), vec!["name=abc123"]);
Ok(())
Expand All @@ -157,7 +149,7 @@ mod tests {
let builder = reqwest::Client::builder()
.build()?
.get("https://test.googleapis.com/v1/unused");
let builder = value.add(builder, "name")?;
let builder = value.add(builder, "name");
let request = builder.build()?;
assert_eq!(split_query(&request), vec!["name=7.5"]);
Ok(())
Expand All @@ -169,7 +161,7 @@ mod tests {
let builder = reqwest::Client::builder()
.build()?
.get("https://test.googleapis.com/v1/unused");
let builder = value.add(builder, "name")?;
let builder = value.add(builder, "name");
let request = builder.build()?;
assert_eq!(split_query(&request), vec!["name=true"]);
Ok(())
Expand Down
6 changes: 0 additions & 6 deletions src/gax/src/request_parameter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@

pub(crate) trait RequestParameter {}

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[error("cannot format as request parameter {0:?}")]
Format(Box<dyn std::error::Error + Send + Sync>),
}

impl RequestParameter for i32 {}
impl RequestParameter for i64 {}
impl RequestParameter for u32 {}
Expand Down
74 changes: 38 additions & 36 deletions src/gax/tests/query_parameters.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ mod test {

// A new version of the generator will generate code as follows for query parameters.
fn add_query_parameters(request: &FakeRequest) -> Result<reqwest::RequestBuilder> {
use gax::error::Error;
let client = reqwest::Client::builder().build()?;
let builder = client.get("https://test.googleapis.com/v1/unused");

Expand Down Expand Up @@ -164,52 +165,53 @@ mod test {
builder.query(&[("repeatedEnumValue", &p.value())])
});

let builder = request.duration.iter().try_fold(builder, |builder, p| {
use gax::error::Error;
use gax::query_parameter::QueryParameter;
serde_json::to_value(&p)
.map_err(Error::serde)?
.add(builder, "optionalDuration")
.map_err(Error::other)
})?;

let builder = request.field_mask.iter().try_fold(builder, |builder, p| {
use gax::error::Error;
use gax::query_parameter::QueryParameter;
serde_json::to_value(&p)
.map_err(Error::serde)?
.add(builder, "fieldMask")
.map_err(Error::other)
})?;
let builder = request
.duration
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
v.add(builder, "optionalDuration")
});
let builder = request
.field_mask
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
v.add(builder, "fieldMask")
});
let builder = {
use gax::error::Error;
use gax::query_parameter::QueryParameter;
serde_json::to_value(&request.required_field_mask)
.map_err(Error::serde)?
.add(builder, "requiredFieldMask")
.map_err(Error::other)?
};

let builder = request.timestamp.iter().try_fold(builder, |builder, p| {
use gax::error::Error;
use gax::query_parameter::QueryParameter;
serde_json::to_value(&p)
.map_err(Error::serde)?
.add(builder, "expiration")
.map_err(Error::other)
})?;

let builder = request
.timestamp
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
v.add(builder, "expiration")
});
let builder = request
.optional_nested
.iter()
.try_fold(builder, |builder, p| {
use gax::error::Error;
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
serde_json::to_value(&p)
.map_err(Error::serde)?
.add(builder, "optionalNested")
.map_err(Error::other)
})?;
v.add(builder, "optionalNested")
});

Ok(builder)
}
Expand Down
34 changes: 20 additions & 14 deletions src/generated/cloud/secretmanager/v1/src/transport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,16 @@ impl crate::traits::SecretManagerService for SecretManagerService {
"x-goog-api-client",
reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER),
);
let builder = req.update_mask.iter().try_fold(builder, |builder, p| {
use gax::query_parameter::QueryParameter;
serde_json::to_value(p)
.map_err(Error::serde)?
.add(builder, "updateMask")
.map_err(Error::other)
})?;
let builder = req
.update_mask
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
v.add(builder, "updateMask")
});
self.inner.execute(builder, Some(req.secret), options).await
}

Expand Down Expand Up @@ -319,13 +322,16 @@ impl crate::traits::SecretManagerService for SecretManagerService {
"x-goog-api-client",
reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER),
);
let builder = req.options.iter().try_fold(builder, |builder, p| {
use gax::query_parameter::QueryParameter;
serde_json::to_value(p)
.map_err(Error::serde)?
.add(builder, "options")
.map_err(Error::other)
})?;
let builder = req
.options
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
v.add(builder, "options")
});
self.inner
.execute(builder, None::<gax::http_client::NoBody>, options)
.await
Expand Down
17 changes: 10 additions & 7 deletions src/generated/cloud/workflows/v1/src/transport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,16 @@ impl crate::traits::Workflows for Workflows {
"x-goog-api-client",
reqwest::header::HeaderValue::from_static(&crate::info::X_GOOG_API_CLIENT_HEADER),
);
let builder = req.update_mask.iter().try_fold(builder, |builder, p| {
use gax::query_parameter::QueryParameter;
serde_json::to_value(p)
.map_err(Error::serde)?
.add(builder, "updateMask")
.map_err(Error::other)
})?;
let builder = req
.update_mask
.as_ref()
.map(|p| serde_json::to_value(p).map_err(Error::serde))
.transpose()?
.into_iter()
.fold(builder, |builder, v| {
use gax::query_parameter::QueryParameter;
v.add(builder, "updateMask")
});
self.inner
.execute(builder, Some(req.workflow), options)
.await
Expand Down
2 changes: 0 additions & 2 deletions src/generated/openapi-validation/src/transport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,6 @@ impl crate::traits::SecretManagerService for SecretManagerService {
serde_json::to_value(&req.update_mask)
.map_err(Error::serde)?
.add(builder, "updateMask")
.map_err(Error::other)?
};
self.inner
.execute(builder, Some(req.request_body), options)
Expand Down Expand Up @@ -418,7 +417,6 @@ impl crate::traits::SecretManagerService for SecretManagerService {
serde_json::to_value(&req.update_mask)
.map_err(Error::serde)?
.add(builder, "updateMask")
.map_err(Error::other)?
};
self.inner
.execute(builder, Some(req.request_body), options)
Expand Down
Loading

0 comments on commit a72d04c

Please sign in to comment.