Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$status var in responses #6225

Merged
merged 2 commits into from
Nov 5, 2024
Merged

$status var in responses #6225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3dd322c
$status var in responses
lennyburdette Nov 4, 2024
9bd73b3
squash fix build
lennyburdette Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apollo-federation/src/sources/connect/expand/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ mod helpers {
// (externally-provided) variables like $this, $args, and
// $config. The $ and @ variables, by contrast, are always bound
// to something within the input data.
KnownVariable::Dollar | KnownVariable::AtSign => {
KnownVariable::Dollar | KnownVariable::AtSign | KnownVariable::Status => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that KnownVariable::Status maps to None here points to something missing here. We're reusing the Variable and VariableType definitions from the URL template code, which doesn't seem right, and doesn't capture a key distinction, which is request vs response variables - status only makes sense in a response context. We've already had bugs here, where $context was passed in where it wasn't supported. I'd like to see some better type definitions around variables which clarify where they can be used and prevent mistakes where a variable is passed in somewhere it isn't supported at the code level, and also enable validations at the user level that they are only referencing variables where they are supported. This doesn't necessarily need to be solved in this PR, but it does come up here as it's really unclear without additional context why this one variable maps to None here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's a good point. i'll make a ticket

return None;
}
};
Expand Down
3 changes: 3 additions & 0 deletions apollo-federation/src/sources/connect/json_selection/known_var.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub(crate) enum KnownVariable {
This,
Args,
Config,
Status,
Dollar,
AtSign,
}
Expand All @@ -16,6 +17,7 @@ impl KnownVariable {
"$this" => Some(Self::This),
"$args" => Some(Self::Args),
"$config" => Some(Self::Config),
"$status" => Some(Self::Status),
"$" => Some(Self::Dollar),
"@" => Some(Self::AtSign),
_ => None,
Expand All @@ -27,6 +29,7 @@ impl KnownVariable {
Self::This => "$this",
Self::Args => "$args",
Self::Config => "$config",
Self::Status => "$status",
Self::Dollar => "$",
Self::AtSign => "@",
}
Expand Down
79 changes: 78 additions & 1 deletion apollo-router/src/plugins/connectors/handle_responses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ pub(crate) async fn handle_responses<T: HttpBody>(
let mut res_data = {
let (res, apply_to_errors) = response_key.selection().apply_with_vars(
&json_data,
&response_key.inputs().merge(connector.config.as_ref(), None),
&response_key.inputs().merge(
connector.config.as_ref(),
None,
Some(parts.status.as_u16()),
),
);

if let Some(ref debug) = debug {
Expand Down Expand Up @@ -748,4 +752,77 @@ mod tests {
}
"###);
}

#[tokio::test]
async fn test_handle_responses_status() {
let connector = Connector {
id: ConnectId::new(
"subgraph_name".into(),
None,
name!(Query),
name!(hello),
0,
"test label",
),
transport: HttpJsonTransport {
source_url: Some(Url::parse("http://localhost/api").unwrap()),
connect_template: "/path".parse().unwrap(),
method: HTTPMethod::Get,
headers: Default::default(),
body: Default::default(),
},
selection: JSONSelection::parse("$status").unwrap(),
entity_resolver: None,
config: Default::default(),
max_requests: None,
};

let response1: http::Response<RouterBody> = http::Response::builder()
.status(201)
.body(hyper::Body::from(r#"{}"#).into())
.expect("response builder");
let response_key1 = ResponseKey::RootField {
name: "hello".to_string(),
inputs: Default::default(),
typename: ResponseTypeName::Concrete("Int".to_string()),
selection: Arc::new(JSONSelection::parse("$status").unwrap()),
};

let res = super::handle_responses(
vec![ConnectorResponse {
result: response1.into(),
key: response_key1,
debug_request: None,
}],
&connector,
&None,
)
.await
.unwrap();

assert_debug_snapshot!(res, @r###"
Response {
response: Response {
status: 200,
version: HTTP/1.1,
headers: {},
body: Response {
label: None,
data: Some(
Object({
"hello": Number(201),
}),
),
path: None,
errors: [],
extensions: {},
has_next: None,
subscribed: None,
created_at: None,
incremental: [],
},
},
}
"###);
}
}
6 changes: 5 additions & 1 deletion apollo-router/src/plugins/connectors/make_requests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ impl RequestInputs {
&self,
config: Option<&CustomConfiguration>,
context: Option<Map<ByteString, Value>>,
status: Option<u16>,
) -> IndexMap<String, Value> {
let mut map = IndexMap::with_capacity_and_hasher(3, Default::default());
map.insert("$args".to_string(), Value::Object(self.args.clone()));
Expand All @@ -43,6 +44,9 @@ impl RequestInputs {
if let Some(config) = config {
map.insert("$config".to_string(), json!(config));
}
if let Some(status) = status {
map.insert("$status".to_string(), Value::Number(status.into()));
}
Comment on lines +47 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to default $status to Value::Number(200.into()) in this map? That way we'd be guaranteed it's always defined, and JSONSelection strings that mention $status can still function even if there's no HTTP response involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's interesting, but i think it's better to have an apply_to error if you try to use $status in a body selection, headers, or URLs. that's certainly a mistake, and it'd be weird to let customers emit 200s in strange places.

we should probably also validate which variables are available in the different selections. i'll make a ticket for that.

map
}
}
Expand Down Expand Up @@ -127,7 +131,7 @@ fn request_params_to_requests(
&connector.transport,
response_key
.inputs()
.merge(connector.config.as_ref(), Some(context.clone())),
.merge(connector.config.as_ref(), Some(context.clone()), None),
original_request,
debug,
)?;
Expand Down