Skip to content

Commit c541edf

Browse files
authored
improve handling of deferred response errors in rhai scripts (#2945)
Currently, if a rhai script errors in rhai, the rhai plugin ignores the error and returns None in the stream of results. This has two unfortunate aspects: - the error is not propagated to the client - the stream is terminated (silently) The fix captures the error and propagates the response to the client. Fixes #2935 #2936 <!-- start metadata --> **Checklist** Complete the checklist (and note appropriate exceptions) before a final PR is raised. - [x] Changes are compatible[^1] - [x] Documentation[^2] completed - [x] Performance impact assessed and acceptable - Tests added and passing[^3] - [x] Unit Tests ~- [ ] Integration Tests~ - [x] Manual Tests **Exceptions** *Note any exceptions here* **Notes** [^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests: - please document the manual testing (extensively) in the Exceptions. - please raise a separate issue to automate the test and label it (or ask for it to be labeled) as `manual test`
1 parent e34906b commit c541edf

File tree

5 files changed

+160
-5
lines changed

5 files changed

+160
-5
lines changed
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
### Improve handling of deferred response errors from rhai scripts ([Issue #2935](https://github.com/apollographql/router/issues/2935)) ([Issue #2936](https://github.com/apollographql/router/issues/2936))
2+
3+
4+
Whilst processing a deferred response; if a rhai script errors in rhai the router ignores the error and returns None in the stream of results. This has two unfortunate aspects:
5+
6+
- the error is not propagated to the client
7+
- the stream is terminated (silently)
8+
9+
The fix captures the error and propagates the response to the client.
10+
11+
This fix also adds support for the `is_primary()` method which may be invoked on both supergraph_service() and execution_service() responses. It may be used to avoid implementing exception handling for header interactions and to determine if a response `is_primary()` (i.e.: first) or not.
12+
13+
e.g.:
14+
15+
```
16+
if response.is_primary() {
17+
print(`all response headers: ${response.headers}`);
18+
} else {
19+
print(`don't try to access headers`);
20+
}
21+
```
22+
23+
vs
24+
25+
```
26+
try {
27+
print(`all response headers: ${response.headers}`);
28+
}
29+
catch(err) {
30+
if err == "cannot access headers on a deferred response" {
31+
print(`don't try to access headers`);
32+
}
33+
}
34+
```
35+
Note: This is a minimal example for purposes of illustration. A real exception handler would handle all error conditions.
36+
37+
38+
By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/2945

apollo-router/src/plugins/rhai/mod.rs

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ type SharedMut<T> = rhai::Shared<Mutex<Option<T>>>;
8585

8686
pub(crate) const RHAI_SPAN_NAME: &str = "rhai_plugin";
8787

88+
const CANNOT_ACCESS_HEADERS_ON_A_DEFERRED_RESPONSE: &str =
89+
"cannot access headers on a deferred response";
90+
8891
impl<T> OptionDance<T> for SharedMut<T> {
8992
fn with_mut<R>(&self, f: impl FnOnce(&mut T) -> R) -> R {
9093
let mut guard = self.lock().expect("poisoned mutex");
@@ -214,11 +217,25 @@ mod router_plugin {
214217
Ok(obj.with_mut(|response| response.response.headers().clone()))
215218
}
216219

220+
#[rhai_fn(name = "is_primary", pure)]
221+
pub(crate) fn supergraph_response_is_primary(
222+
_obj: &mut SharedMut<supergraph::Response>,
223+
) -> bool {
224+
true
225+
}
226+
217227
#[rhai_fn(get = "headers", pure, return_raw)]
218228
pub(crate) fn get_originating_headers_router_deferred_response(
219229
_obj: &mut SharedMut<supergraph::DeferredResponse>,
220230
) -> Result<HeaderMap, Box<EvalAltResult>> {
221-
Err("cannot access headers on a deferred response".into())
231+
Err(CANNOT_ACCESS_HEADERS_ON_A_DEFERRED_RESPONSE.into())
232+
}
233+
234+
#[rhai_fn(name = "is_primary", pure)]
235+
pub(crate) fn supergraph_deferred_response_is_primary(
236+
_obj: &mut SharedMut<supergraph::DeferredResponse>,
237+
) -> bool {
238+
false
222239
}
223240

224241
#[rhai_fn(get = "headers", pure, return_raw)]
@@ -228,11 +245,23 @@ mod router_plugin {
228245
Ok(obj.with_mut(|response| response.response.headers().clone()))
229246
}
230247

248+
#[rhai_fn(name = "is_primary", pure)]
249+
pub(crate) fn execution_response_is_primary(_obj: &mut SharedMut<execution::Response>) -> bool {
250+
true
251+
}
252+
231253
#[rhai_fn(get = "headers", pure, return_raw)]
232254
pub(crate) fn get_originating_headers_execution_deferred_response(
233255
_obj: &mut SharedMut<execution::DeferredResponse>,
234256
) -> Result<HeaderMap, Box<EvalAltResult>> {
235-
Err("cannot access headers on a deferred response".into())
257+
Err(CANNOT_ACCESS_HEADERS_ON_A_DEFERRED_RESPONSE.into())
258+
}
259+
260+
#[rhai_fn(name = "is_primary", pure)]
261+
pub(crate) fn execution_deferred_response_is_primary(
262+
_obj: &mut SharedMut<execution::DeferredResponse>,
263+
) -> bool {
264+
false
236265
}
237266

238267
#[rhai_fn(get = "headers", pure, return_raw)]
@@ -291,7 +320,7 @@ mod router_plugin {
291320
_obj: &mut SharedMut<supergraph::DeferredResponse>,
292321
_headers: HeaderMap,
293322
) -> Result<(), Box<EvalAltResult>> {
294-
Err("cannot access headers on a deferred response".into())
323+
Err(CANNOT_ACCESS_HEADERS_ON_A_DEFERRED_RESPONSE.into())
295324
}
296325

297326
#[rhai_fn(set = "headers", return_raw)]
@@ -308,7 +337,7 @@ mod router_plugin {
308337
_obj: &mut SharedMut<execution::DeferredResponse>,
309338
_headers: HeaderMap,
310339
) -> Result<(), Box<EvalAltResult>> {
311-
Err("cannot access headers on a deferred response".into())
340+
Err(CANNOT_ACCESS_HEADERS_ON_A_DEFERRED_RESPONSE.into())
312341
}
313342

314343
#[rhai_fn(set = "headers", return_raw)]
@@ -907,7 +936,16 @@ macro_rules! gen_map_deferred_response {
907936
);
908937
if let Err(error) = result {
909938
tracing::error!("map_response callback failed: {error}");
910-
return None;
939+
let error_details = process_error(error);
940+
let mut guard = shared_response.lock().unwrap();
941+
let response_opt = guard.take();
942+
let $rhai_deferred_response { mut response, .. } = response_opt.unwrap();
943+
let error = Error {
944+
message: error_details.message.unwrap_or_default(),
945+
..Default::default()
946+
};
947+
response.errors = vec![error];
948+
return Some(response);
911949
}
912950

913951
let mut guard = shared_response.lock().unwrap();

apollo-router/src/plugins/rhai/tests.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,14 @@ async fn it_can_process_supergraph_response() {
407407
gen_response_test!(RhaiSupergraphResponse, "process_supergraph_response");
408408
}
409409

410+
#[tokio::test]
411+
async fn it_can_process_supergraph_response_is_primary() {
412+
gen_response_test!(
413+
RhaiSupergraphResponse,
414+
"process_supergraph_response_is_primary"
415+
);
416+
}
417+
410418
#[tokio::test]
411419
async fn it_can_process_supergraph_deferred_response() {
412420
gen_response_test!(
@@ -415,16 +423,40 @@ async fn it_can_process_supergraph_deferred_response() {
415423
);
416424
}
417425

426+
#[tokio::test]
427+
async fn it_can_process_supergraph_deferred_response_is_not_primary() {
428+
gen_response_test!(
429+
RhaiSupergraphDeferredResponse,
430+
"process_supergraph_deferred_response_is_not_primary"
431+
);
432+
}
433+
418434
#[tokio::test]
419435
async fn it_can_process_execution_response() {
420436
gen_response_test!(RhaiExecutionResponse, "process_execution_response");
421437
}
422438

439+
#[tokio::test]
440+
async fn it_can_process_execution_response_is_primary() {
441+
gen_response_test!(
442+
RhaiExecutionResponse,
443+
"process_execution_response_is_primary"
444+
);
445+
}
446+
423447
#[tokio::test]
424448
async fn it_can_process_execution_deferred_response() {
425449
gen_response_test!(RhaiExecutionDeferredResponse, "process_execution_response");
426450
}
427451

452+
#[tokio::test]
453+
async fn it_can_process_execution_deferred_response_is_not_primary() {
454+
gen_response_test!(
455+
RhaiExecutionDeferredResponse,
456+
"process_execution_deferred_response_is_not_primary"
457+
);
458+
}
459+
428460
#[tokio::test]
429461
async fn it_can_process_subgraph_response() {
430462
let dyn_plugin: Box<dyn DynPlugin> = crate::plugin::plugins()

apollo-router/tests/fixtures/request_response_test.rhai

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,3 +161,40 @@ fn process_subgraph_response_om_missing_message(response) {
161161
status: 400,
162162
};
163163
}
164+
165+
fn process_supergraph_response_is_primary(response) {
166+
if !response.is_primary() {
167+
throw #{
168+
status: 500,
169+
message: "should be primary"
170+
};
171+
}
172+
}
173+
174+
fn process_execution_response_is_primary(response) {
175+
if !response.is_primary() {
176+
throw #{
177+
status: 500,
178+
message: "should be primary"
179+
};
180+
}
181+
}
182+
183+
fn process_supergraph_deferred_response_is_not_primary(response) {
184+
if response.is_primary() {
185+
throw #{
186+
status: 500,
187+
message: "should not be primary"
188+
};
189+
}
190+
}
191+
192+
fn process_execution_deferred_response_is_not_primary(response) {
193+
if response.is_primary() {
194+
throw #{
195+
status: 500,
196+
message: "should not be primary"
197+
};
198+
}
199+
}
200+

docs/source/customizations/rhai-api.mdx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,16 @@ The following fields are identical in behavior to their `request` counterparts:
435435
* [`headers`](#requestheaders)
436436
* [`body.extensions`](#requestbodyextensions)
437437

438+
Note: Be particularly careful when interacting with headers in a response context. For supergraph_service() and execution_service(), response headers only exist for the first response in a deferred response stream. You can handle this by making use of the `is_primary()` function which will return true if a response is the first (or primary) response. If you do try to access the headers in a non-primary response, then you'll raise an exception which can be handled like any other rhai exception, but is not so convenient as using the `is_primary()` method.
439+
440+
```rhai
441+
if response.is_primary() {
442+
print(`all response headers: ${response.headers}`);
443+
} else {
444+
print(`don't try to access headers`);
445+
}
446+
```
447+
438448
Other fields are described below.
439449

440450
### `response.body.label`

0 commit comments

Comments
 (0)