Skip to content

Commit 80eb019

Browse files
BrynCookebryn
BrynCooke
and
bryn
authored
Follow up on the Plugin utils cleanup to add nicer header, variable and extension support. (#908)
* Follow up on the Plugin utils cleanup to add nicer header support. Mostly this is around adding `Into` support for headers, but also around fallability of builders. Users can supply, String, &str, HeaderName and HeaderValue. `builder` is now consistently fallible. `fake_builder` is now consistently fallible * Extensions and variables are now a map. Fix failing tests * Downgrade mac resource class Co-authored-by: bryn <bryn@apollographql.com>
1 parent 34a66db commit 80eb019

File tree

23 files changed

+502
-300
lines changed

23 files changed

+502
-300
lines changed

‎.circleci/config.yml

-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ executors:
1717
rust_macos: &rust_macos_executor
1818
macos:
1919
xcode: 11.4
20-
resource_class: macos.x86.medium.gen2
2120
rust_windows: &rust_windows_executor
2221
machine:
2322
image: "windows-server-2019-vs2019:stable"

‎Cargo.lock

+6-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎NEXT_CHANGELOG.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2424

2525
# [v0.1.0-preview.7] - (unreleased)
2626
## ❗ BREAKING ❗
27-
### Plugin utilities cleanup ([PR #819](https://github.com/apollographql/router/pull/819))
27+
28+
### Plugin utilities cleanup ([PR #819](https://github.com/apollographql/router/pull/819)) ([PR #908](https://github.com/apollographql/router/pull/908))
2829
Utilities around creating Request and Response structures have been migrated to builders.
2930

3031
Migration:

‎apollo-router-benchmarks/src/shared.rs

+2-7
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ use apollo_router_core::{
77
};
88
use once_cell::sync::Lazy;
99
use serde_json::json;
10-
use serde_json_bytes::{ByteString, Value};
1110
use std::sync::Arc;
1211
use tower::{util::BoxCloneService, BoxError, Service, ServiceExt};
1312

@@ -22,12 +21,8 @@ pub async fn basic_composition_benchmark(
2221
) {
2322
let request = RouterRequest::fake_builder()
2423
.query(QUERY.to_string())
25-
.variables(Arc::new(
26-
vec![(ByteString::from("first"), Value::Number(2usize.into()))]
27-
.into_iter()
28-
.collect(),
29-
))
30-
.build();
24+
.variable("first", 2usize)
25+
.build().expect("expecting valid request");
3126

3227
let response = router_service
3328
.ready()

‎apollo-router-core/Cargo.toml

+2-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ apollo-parser = "0.2.5"
1818
async-trait = "0.1.53"
1919
atty = "0.2.14"
2020
axum = { version = "0.5.3", optional = true }
21-
buildstructor = "0.1.5"
21+
buildstructor = "0.1.9"
2222
bytes = "1.1.0"
2323
dashmap = { version = "5.1.0", features = ["serde"] }
2424
derivative = "2.2.0"
@@ -36,6 +36,7 @@ lazy_static = "1.4.0"
3636
lru = "0.7.5"
3737
miette = { version = "4.2.1", features = ["fancy"] }
3838
mockall = "0.11.0"
39+
multimap = "0.8.3"
3940
moka = { version = "0.7.2", features = ["future", "futures-util"] }
4041
once_cell = "1.9.0"
4142
opentelemetry = "0.17.0"

‎apollo-router-core/src/layers/apq.rs

+36-25
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
66
use std::ops::ControlFlow;
77

8-
use crate::{checkpoint::CheckpointService, Object, RouterRequest, RouterResponse};
8+
use crate::{checkpoint::CheckpointService, RouterRequest, RouterResponse};
99
use moka::sync::Cache;
1010
use serde::Deserialize;
11-
use serde_json_bytes::json;
11+
use serde_json_bytes::{json, Value};
1212
use sha2::{Digest, Sha256};
1313
use tower::{BoxError, Layer, Service};
1414

@@ -98,11 +98,11 @@ where
9898
.unwrap(),
9999
}];
100100
let res = RouterResponse::builder()
101-
.data(Default::default())
101+
.data(Value::default())
102102
.errors(errors)
103-
.extensions(Object::new())
104103
.context(req.context)
105-
.build();
104+
.build()
105+
.expect("response is valid");
106106

107107
Ok(ControlFlow::Break(res))
108108
}
@@ -126,7 +126,7 @@ mod apq_tests {
126126
use crate::{plugin::utils::test::MockRouterService, Context, ResponseBody};
127127
use serde_json_bytes::json;
128128
use std::borrow::Cow;
129-
use std::sync::Arc;
129+
use std::collections::HashMap;
130130
use tower::ServiceExt;
131131

132132
#[tokio::test]
@@ -168,7 +168,9 @@ mod apq_tests {
168168

169169
assert!(body.query.is_some());
170170

171-
Ok(RouterResponse::fake_builder().build())
171+
Ok(RouterResponse::fake_builder()
172+
.build()
173+
.expect("expecting valid request"))
172174
});
173175
mock_service
174176
// the last one should have the right APQ header and the full query string
@@ -193,33 +195,38 @@ mod apq_tests {
193195
hash.as_slice()
194196
));
195197

196-
Ok(RouterResponse::fake_builder().build())
198+
Ok(RouterResponse::fake_builder()
199+
.build()
200+
.expect("expecting valid request"))
197201
});
198202

199203
let mock = mock_service.build();
200204

201205
let mut service_stack = APQLayer::default().layer(mock);
202206

203-
let extensions = vec![(
204-
"persistedQuery",
207+
let extensions = HashMap::from([(
208+
"persistedQuery".to_string(),
205209
json!({
206210
"version" : 1,
207211
"sha256Hash" : "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"
208212
}),
209-
)];
213+
)]);
210214

211215
let hash_only = RouterRequest::fake_builder()
212216
.extensions(extensions.clone())
213-
.build();
217+
.build()
218+
.expect("expecting valid request");
214219

215220
let second_hash_only = RouterRequest::fake_builder()
216221
.extensions(extensions.clone())
217-
.build();
222+
.build()
223+
.expect("expecting valid request");
218224

219225
let with_query = RouterRequest::fake_builder()
220226
.extensions(extensions)
221227
.query("{__typename}".to_string())
222-
.build();
228+
.build()
229+
.expect("expecting valid request");
223230

224231
let services = service_stack.ready().await.unwrap();
225232
let apq_error = services.call(hash_only).await.unwrap();
@@ -274,7 +281,9 @@ mod apq_tests {
274281

275282
assert!(body.query.is_some());
276283

277-
Ok(RouterResponse::fake_builder().build())
284+
Ok(RouterResponse::fake_builder()
285+
.build()
286+
.expect("expecting valid request"))
278287
});
279288
mock_service_builder
280289
// the second last one should have the right APQ header and the full query string
@@ -300,42 +309,44 @@ mod apq_tests {
300309
hash.as_slice()
301310
));
302311

303-
Ok(RouterResponse::fake_builder().build())
312+
Ok(RouterResponse::fake_builder()
313+
.build()
314+
.expect("expecting valid request"))
304315
});
305316

306317
let mock_service = mock_service_builder.build();
307318

308319
let mut service_stack = APQLayer::default().layer(mock_service);
309320

310-
let extensions = vec![(
311-
"persistedQuery",
321+
let extensions = HashMap::from([(
322+
"persistedQuery".to_string(),
312323
json!({
313324
"version" : 1,
314325
"sha256Hash" : "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b36"
315326
}),
316-
)];
327+
)]);
317328

318329
let request_builder = RouterRequest::fake_builder().extensions(extensions.clone());
319330

320331
let hash_only = request_builder
321-
.variables(Arc::new(vec![].into_iter().collect()))
322332
.context(Context::new())
323-
.build();
333+
.build()
334+
.expect("expecting valid request");
324335

325336
let request_builder = RouterRequest::fake_builder().extensions(extensions.clone());
326337

327338
let second_hash_only = request_builder
328-
.variables(Arc::new(vec![].into_iter().collect()))
329339
.context(Context::new())
330-
.build();
340+
.build()
341+
.expect("expecting valid request");
331342

332343
let request_builder = RouterRequest::fake_builder().extensions(extensions);
333344

334345
let with_query = request_builder
335346
.query("{__typename}".to_string())
336-
.variables(Arc::new(vec![].into_iter().collect()))
337347
.context(Context::new())
338-
.build();
348+
.build()
349+
.expect("expecting valid request");
339350

340351
let services = service_stack.ready().await.unwrap();
341352
// This apq call will miss the APQ cache

‎apollo-router-core/src/layers/ensure_query_presence.rs

+22-19
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
77
use crate::checkpoint::CheckpointService;
88
use crate::{RouterRequest, RouterResponse};
9-
use http::{HeaderMap, StatusCode};
9+
use http::StatusCode;
10+
use serde_json_bytes::Value;
1011
use std::ops::ControlFlow;
1112
use tower::{BoxError, Layer, Service};
1213

@@ -23,7 +24,7 @@ where
2324

2425
fn layer(&self, service: S) -> Self::Service {
2526
CheckpointService::new(
26-
|mut req: RouterRequest| {
27+
|req: RouterRequest| {
2728
// A query must be available at this point
2829
let query = req.originating_request.body().query.as_ref();
2930
if query.is_none() || query.unwrap().trim().is_empty() {
@@ -33,19 +34,15 @@ where
3334
path: Default::default(),
3435
extensions: Default::default(),
3536
}];
36-
let headers =
37-
std::mem::replace(req.originating_request.headers_mut(), HeaderMap::new())
38-
.into_iter()
39-
.filter_map(|(k, v)| k.map(|k| (k, v)))
40-
.collect();
37+
38+
//We do not copy headers from the request to the response as this may lead to leakable of sensitive data
4139
let res = RouterResponse::builder()
42-
.data(Default::default())
40+
.data(Value::default())
4341
.errors(errors)
44-
.extensions(Default::default())
4542
.status_code(StatusCode::BAD_REQUEST)
4643
.context(req.context)
47-
.headers(headers)
48-
.build();
44+
.build()
45+
.expect("response is valid");
4946
Ok(ControlFlow::Break(res))
5047
} else {
5148
Ok(ControlFlow::Continue(req))
@@ -66,17 +63,19 @@ mod ensure_query_presence_tests {
6663
#[tokio::test]
6764
async fn it_works_with_query() {
6865
let mut mock_service = MockRouterService::new();
69-
mock_service
70-
.expect_call()
71-
.times(1)
72-
.returning(move |_req| Ok(RouterResponse::fake_builder().build()));
66+
mock_service.expect_call().times(1).returning(move |_req| {
67+
Ok(RouterResponse::fake_builder()
68+
.build()
69+
.expect("expecting valid request"))
70+
});
7371

7472
let mock = mock_service.build();
7573
let service_stack = EnsureQueryPresence::default().layer(mock);
7674

7775
let request: crate::RouterRequest = RouterRequest::fake_builder()
7876
.query("{__typename}".to_string())
79-
.build();
77+
.build()
78+
.expect("expecting valid request");
8079

8180
let _ = service_stack.oneshot(request).await.unwrap();
8281
}
@@ -90,8 +89,10 @@ mod ensure_query_presence_tests {
9089

9190
let service_stack = EnsureQueryPresence::default().layer(mock);
9291

93-
let request: crate::RouterRequest =
94-
RouterRequest::fake_builder().query("".to_string()).build();
92+
let request: crate::RouterRequest = RouterRequest::fake_builder()
93+
.query("".to_string())
94+
.build()
95+
.expect("expecting valid request");
9596

9697
let response = service_stack
9798
.oneshot(request)
@@ -116,7 +117,9 @@ mod ensure_query_presence_tests {
116117
let mock = mock_service.build();
117118
let service_stack = EnsureQueryPresence::default().layer(mock);
118119

119-
let request: crate::RouterRequest = RouterRequest::fake_builder().build();
120+
let request: crate::RouterRequest = RouterRequest::fake_builder()
121+
.build()
122+
.expect("expecting valid request");
120123

121124
let response = service_stack
122125
.oneshot(request)

‎apollo-router-core/src/layers/forbid_http_get_mutations.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//!
33
//! See [`Layer`] and [`Service`] for more details.
44
5-
use crate::{checkpoint::CheckpointService, ExecutionRequest, ExecutionResponse};
5+
use crate::{checkpoint::CheckpointService, ExecutionRequest, ExecutionResponse, Object};
66
use http::{header::HeaderName, Method, StatusCode};
77
use std::ops::ControlFlow;
88
use tower::{BoxError, Layer, Service};
@@ -32,7 +32,7 @@ where
3232
}];
3333
let mut res = ExecutionResponse::builder()
3434
.errors(errors)
35-
.extensions(Default::default())
35+
.extensions(Object::default())
3636
.status_code(StatusCode::METHOD_NOT_ALLOWED)
3737
.context(req.context)
3838
.build();
@@ -185,7 +185,7 @@ mod forbid_http_get_mutations_tests {
185185
.method(method)
186186
.body(crate::Request::default())
187187
.build()
188-
.unwrap();
188+
.expect("expecting valid request");
189189

190190
ExecutionRequest::fake_builder()
191191
.originating_request(request)

‎apollo-router-core/src/plugins/forbid_mutations.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ mod forbid_http_get_mutations_tests {
168168
.method(method)
169169
.body(crate::Request::default())
170170
.build()
171-
.unwrap();
171+
.expect("expecting valid request");
172172
ExecutionRequest::fake_builder()
173173
.originating_request(request)
174174
.query_plan(Arc::new(QueryPlan { root }))

0 commit comments

Comments
 (0)