Skip to content

Commit 119b965

Browse files
LarryOstermanLarryOsterman
authored
Allow package specific error codes in check_success (Azure#3012)
Also: - Added options to `Pipeline::send()` - Propagate pipeline send options from `azure_core` to `typespec_client_core`
1 parent c2199dc commit 119b965

File tree

25 files changed

+601
-333
lines changed

25 files changed

+601
-333
lines changed

‎sdk/core/azure_core/benches/http_transport_benchmarks.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ impl TestServiceClient {
9696

9797
let response = self
9898
.pipeline
99-
.send(&options.method_options.context, &mut request)
99+
.send(&options.method_options.context, &mut request, None)
100100
.await?;
101101
if !response.status().is_success() {
102102
return Err(azure_core::Error::with_message(

‎sdk/core/azure_core/src/error/error_response.rs‎

Lines changed: 104 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,19 +108,43 @@ struct ErrorDetailsInternal<'a> {
108108
message: Option<&'a str>,
109109
}
110110

111+
/// Options for customizing the behavior of `check_success`.
112+
#[derive(Debug, Default)]
113+
pub struct CheckSuccessOptions {
114+
/// A list of HTTP status codes that should be considered successful.
115+
///
116+
/// If this list is empty, any 2xx status code is considered successful.
117+
pub success_codes: &'static [u16],
118+
}
119+
111120
/// Checks if the response is a success and if not, creates an appropriate error.
112121
///
113122
/// # Arguments
114123
/// * `response` - The HTTP response to check.
124+
/// * `options` - Optional parameters to customize the success criteria.
115125
///
116126
/// # Returns
117127
/// * `Ok(RawResponse)` if the response is a success.
118128
/// * `Err(Error)` if the response is an error, with details extracted from the response
119129
/// body if possible.
120130
///
121-
pub async fn check_success(response: BufResponse) -> crate::Result<BufResponse> {
131+
pub async fn check_success(
132+
response: BufResponse,
133+
options: Option<CheckSuccessOptions>,
134+
) -> crate::Result<BufResponse> {
122135
let status = response.status();
123-
if status.is_success() {
136+
137+
if options
138+
.as_ref()
139+
.map(|o| {
140+
if o.success_codes.is_empty() {
141+
status.is_success()
142+
} else {
143+
o.success_codes.contains(&status)
144+
}
145+
})
146+
.unwrap_or_else(|| status.is_success())
147+
{
124148
return Ok(response);
125149
}
126150

@@ -186,7 +210,32 @@ mod tests {
186210
Bytes::from_static(br#"{"error": {"code":"teapot","message":"I'm a teapot"}}"#),
187211
);
188212

189-
let err = check_success(response).await.unwrap_err();
213+
let err = check_success(response, None).await.unwrap_err();
214+
let kind = err.kind();
215+
assert!(matches!(
216+
kind,
217+
ErrorKind::HttpResponse {
218+
status: StatusCode::ImATeapot,
219+
error_code,
220+
raw_response: None
221+
}
222+
if error_code.as_deref() == Some("teapot")
223+
));
224+
}
225+
226+
#[tokio::test]
227+
async fn matching_against_custom_http_error_empty_set() {
228+
let mut headers = Headers::new();
229+
headers.insert(headers::CONTENT_TYPE, "application/json".to_string());
230+
let response = BufResponse::from_bytes(
231+
StatusCode::ImATeapot,
232+
headers,
233+
Bytes::from_static(br#"{"error": {"code":"teapot","message":"I'm a teapot"}}"#),
234+
);
235+
236+
let err = check_success(response, Some(CheckSuccessOptions { success_codes: &[] }))
237+
.await
238+
.unwrap_err();
190239
let kind = err.kind();
191240
assert!(matches!(
192241
kind,
@@ -199,13 +248,63 @@ mod tests {
199248
));
200249
}
201250

251+
#[tokio::test]
252+
async fn matching_against_custom_http_error_in_set() {
253+
let mut headers = Headers::new();
254+
headers.insert(headers::CONTENT_TYPE, "application/json".to_string());
255+
let response = BufResponse::from_bytes(
256+
StatusCode::ImATeapot,
257+
headers,
258+
Bytes::from_static(br#"{"error": {"code":"teapot","message":"I'm a teapot"}}"#),
259+
);
260+
261+
let _ = check_success(
262+
response,
263+
Some(CheckSuccessOptions {
264+
success_codes: &[418],
265+
}),
266+
)
267+
.await
268+
.expect("Should be a success return");
269+
}
270+
271+
#[tokio::test]
272+
async fn matching_against_custom_http_error_in_set_success_should_fail() {
273+
let mut headers = Headers::new();
274+
headers.insert(headers::CONTENT_TYPE, "application/json".to_string());
275+
let response = BufResponse::from_bytes(
276+
StatusCode::Ok,
277+
headers,
278+
Bytes::from_static(br#"{"error": {"code":"teapot","message":"I'm a teapot"}}"#),
279+
);
280+
281+
let err = check_success(
282+
response,
283+
Some(CheckSuccessOptions {
284+
success_codes: &[418],
285+
}),
286+
)
287+
.await
288+
.expect_err("Should be a failure return");
289+
let kind = err.kind();
290+
assert!(matches!(
291+
kind,
292+
ErrorKind::HttpResponse {
293+
status: StatusCode::Ok,
294+
error_code,
295+
raw_response: None
296+
}
297+
if error_code.as_deref() == Some("teapot")
298+
));
299+
}
300+
202301
#[tokio::test]
203302
async fn matching_against_http_error_no_body() {
204303
let mut headers = Headers::new();
205304
headers.insert(headers::ERROR_CODE, "testError".to_string());
206305
let response = BufResponse::from_bytes(StatusCode::ImATeapot, headers, Bytes::new());
207306

208-
let err = check_success(response).await.unwrap_err();
307+
let err = check_success(response, None).await.unwrap_err();
209308
let kind = err.kind();
210309
assert_eq!(
211310
*kind,
@@ -227,7 +326,7 @@ mod tests {
227326
Bytes::from_static(br#"{"json": "error"}"#),
228327
);
229328

230-
let err = check_success(response).await.unwrap_err();
329+
let err = check_success(response, None).await.unwrap_err();
231330
let kind = err.kind();
232331
assert_eq!(
233332
*kind,

‎sdk/core/azure_core/src/http/pager.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ impl<P: Page> ItemIterator<P> {
223223
/// .append_pair("api-version", &api_version);
224224
/// }
225225
/// let resp = pipeline
226-
/// .send(&Context::new(), &mut req)
226+
/// .send(&Context::new(), &mut req, None)
227227
/// .await?;
228228
/// let (status, headers, body) = resp.deconstruct();
229229
/// let bytes = body.collect().await?;
@@ -269,7 +269,7 @@ impl<P: Page> ItemIterator<P> {
269269
/// req.insert_header("x-ms-continuation", continuation);
270270
/// }
271271
/// let resp: Response<ListItemsResult> = pipeline
272-
/// .send(&Context::new(), &mut req)
272+
/// .send(&Context::new(), &mut req, None)
273273
/// .await?
274274
/// .into();
275275
/// Ok(PagerResult::from_response_header(resp, &HeaderName::from_static("x-next-continuation")))
@@ -416,7 +416,7 @@ impl<P> PageIterator<P> {
416416
/// .append_pair("api-version", &api_version);
417417
/// }
418418
/// let resp = pipeline
419-
/// .send(&Context::new(), &mut req)
419+
/// .send(&Context::new(), &mut req, None)
420420
/// .await?;
421421
/// let (status, headers, body) = resp.deconstruct();
422422
/// let bytes = body.collect().await?;
@@ -453,7 +453,7 @@ impl<P> PageIterator<P> {
453453
/// req.insert_header("x-ms-continuation", continuation);
454454
/// }
455455
/// let resp: Response<ListItemsResult> = pipeline
456-
/// .send(&Context::new(), &mut req)
456+
/// .send(&Context::new(), &mut req, None)
457457
/// .await?
458458
/// .into();
459459
/// Ok(PagerResult::from_response_header(resp, &HeaderName::from_static("x-ms-continuation")))

‎sdk/core/azure_core/src/http/pipeline.rs‎

Lines changed: 77 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,19 @@
22
// Licensed under the MIT License.
33

44
use super::policies::ClientRequestIdPolicy;
5-
use crate::http::{
6-
headers::{RETRY_AFTER_MS, X_MS_RETRY_AFTER_MS},
7-
policies::{
8-
Policy, PublicApiInstrumentationPolicy, RequestInstrumentationPolicy, UserAgentPolicy,
5+
use crate::{
6+
error::CheckSuccessOptions,
7+
http::{
8+
check_success,
9+
headers::{RETRY_AFTER_MS, X_MS_RETRY_AFTER_MS},
10+
policies::{
11+
Policy, PublicApiInstrumentationPolicy, RequestInstrumentationPolicy, UserAgentPolicy,
12+
},
13+
ClientOptions,
914
},
10-
ClientOptions,
1115
};
1216
use std::{
1317
any::{Any, TypeId},
14-
ops::Deref,
1518
sync::Arc,
1619
};
1720
use typespec_client_core::http::{
@@ -40,6 +43,37 @@ use typespec_client_core::http::{
4043
#[derive(Debug, Clone)]
4144
pub struct Pipeline(http::Pipeline);
4245

46+
/// Options for sending a request through the pipeline.
47+
#[derive(Debug, Default)]
48+
pub struct PipelineSendOptions {
49+
/// If true, skip all checks including `[check_success]`.
50+
pub skip_checks: bool,
51+
52+
/// Options for `[check_success]`. If `skip_checks` is true, this field is ignored.
53+
pub check_success: CheckSuccessOptions,
54+
}
55+
56+
/// Internal structure used to pass options to the core pipeline.
57+
#[derive(Debug, Default)]
58+
struct CorePipelineSendOptions {
59+
check_success: CheckSuccessOptions,
60+
skip_checks: bool,
61+
}
62+
63+
impl PipelineSendOptions {
64+
/// Deconstructs the `PipelineSendOptions` into its core components.
65+
#[expect(private_interfaces)]
66+
pub fn deconstruct(self) -> (CorePipelineSendOptions, Option<http::PipelineSendOptions>) {
67+
(
68+
CorePipelineSendOptions {
69+
skip_checks: self.skip_checks,
70+
check_success: self.check_success,
71+
},
72+
None,
73+
)
74+
}
75+
}
76+
4377
impl Pipeline {
4478
/// Creates a new pipeline given the client library crate name and version,
4579
/// alone with user-specified and client library-specified policies.
@@ -116,6 +150,32 @@ impl Pipeline {
116150
Some(pipeline_options),
117151
))
118152
}
153+
154+
/// Sends the request through the pipeline, returning the response or an error.
155+
///
156+
/// # Arguments
157+
/// * `ctx` - The context for the request.
158+
/// * `request` - The request to send.
159+
/// * `options` - Options for sending the request, including check success options. If none, `[check_success]` will not be called.
160+
///
161+
/// # Returns
162+
/// A [`http::BufResponse`] if the request was successful, or an `Error` if it failed.
163+
/// If the response status code indicates an HTTP error, the function will attempt to parse the error response
164+
/// body into an `ErrorResponse` and include it in the `Error`.
165+
pub async fn send(
166+
&self,
167+
ctx: &http::Context<'_>,
168+
request: &mut http::Request,
169+
options: Option<PipelineSendOptions>,
170+
) -> crate::Result<http::BufResponse> {
171+
let (core_send_options, send_options) = options.unwrap_or_default().deconstruct();
172+
let result = self.0.send(ctx, request, send_options).await?;
173+
if !core_send_options.skip_checks {
174+
check_success(result, Some(core_send_options.check_success)).await
175+
} else {
176+
Ok(result)
177+
}
178+
}
119179
}
120180

121181
#[inline]
@@ -125,13 +185,13 @@ fn push_unique<T: Policy + 'static>(policies: &mut Vec<Arc<dyn Policy>>, policy:
125185
}
126186
}
127187

128-
// TODO: Should we instead use the newtype pattern?
129-
impl Deref for Pipeline {
130-
type Target = http::Pipeline;
131-
fn deref(&self) -> &Self::Target {
132-
&self.0
133-
}
134-
}
188+
// // TODO: Should we instead use the newtype pattern?
189+
// impl Deref for Pipeline {
190+
// type Target = http::Pipeline;
191+
// fn deref(&self) -> &Self::Target {
192+
// &self.0
193+
// }
194+
// }
135195

136196
#[cfg(test)]
137197
mod tests {
@@ -205,7 +265,7 @@ mod tests {
205265

206266
// Act
207267
pipeline
208-
.send(&ctx, &mut request)
268+
.send(&ctx, &mut request, None)
209269
.await
210270
.expect("Pipeline execution failed");
211271
}
@@ -259,7 +319,7 @@ mod tests {
259319

260320
// Act
261321
pipeline
262-
.send(&ctx, &mut request)
322+
.send(&ctx, &mut request, None)
263323
.await
264324
.expect("Pipeline execution failed");
265325
}
@@ -313,7 +373,7 @@ mod tests {
313373

314374
// Act
315375
pipeline
316-
.send(&ctx, &mut request)
376+
.send(&ctx, &mut request, None)
317377
.await
318378
.expect("Pipeline execution failed");
319379
}
@@ -374,7 +434,7 @@ mod tests {
374434

375435
// Act
376436
pipeline
377-
.send(&ctx, &mut request)
437+
.send(&ctx, &mut request, None)
378438
.await
379439
.expect("Pipeline execution failed");
380440
}

‎sdk/core/azure_core/src/http/poller.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ where
292292
/// .append_pair("api-version", &api_version);
293293
///
294294
/// let resp = pipeline
295-
/// .send(&Context::new(), &mut req)
295+
/// .send(&Context::new(), &mut req, None)
296296
/// .await?;
297297
/// let (status, headers, body) = resp.deconstruct();
298298
/// let bytes = body.collect().await?;

‎sdk/core/azure_core_opentelemetry/tests/telemetry_service_implementation.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ impl TestServiceClient {
131131

132132
let response = self
133133
.pipeline
134-
.send(&options.method_options.context, &mut request)
134+
.send(&options.method_options.context, &mut request, None)
135135
.await?;
136136
if !response.status().is_success() {
137137
return Err(azure_core::Error::with_message(

‎sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ impl TestServiceClientWithMacros {
106106

107107
let response = self
108108
.pipeline
109-
.send(&options.method_options.context, &mut request)
109+
.send(&options.method_options.context, &mut request, None)
110110
.await?;
111111
if !response.status().is_success() {
112112
return Err(azure_core::Error::with_message(
@@ -157,7 +157,7 @@ impl TestServiceClientWithMacros {
157157

158158
let response = self
159159
.pipeline
160-
.send(&options.method_options.context, &mut request)
160+
.send(&options.method_options.context, &mut request, None)
161161
.await?;
162162
if !response.status().is_success() {
163163
return Err(azure_core::Error::with_message(

0 commit comments

Comments
 (0)