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

Jump to bottom

Fix __typename inside router response #6009

Merged
merged 18 commits into from
Oct 16, 2024
Merged

Fix __typename inside router response #6009

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7402a1b
Fix __typename inside router response
IvanGoncharov Sep 17, 2024
d7b4f90
Add test for subgraph returning { data: null }
IvanGoncharov Sep 17, 2024
6de302e
add test for subgraph returning different typename on query root
IvanGoncharov Sep 17, 2024
4241a8c
fix lint
IvanGoncharov Sep 17, 2024
cad5265
correctly fix issue with __typename on primary part of defer
IvanGoncharov Sep 17, 2024
c052392
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov Sep 17, 2024
0d61767
add changeset
IvanGoncharov Sep 17, 2024
9115847
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov Sep 17, 2024
ea7ded9
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov Sep 18, 2024
b411e5f
Add insta snapshot
IvanGoncharov Sep 18, 2024
ddeed8d
switch to shorter json snapshot code
IvanGoncharov Sep 18, 2024
e871c93
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov Sep 19, 2024
f637705
fixed unintentional change in fed snapshot
IvanGoncharov Sep 19, 2024
95cfb79
Typo fix
SimonSapin Oct 8, 2024
f15535e
Merge branch 'dev' into i1g/fix-typename
SimonSapin Oct 8, 2024
b9d3f77
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov Oct 15, 2024
37c5b1e
address review feedback
IvanGoncharov Oct 15, 2024
39a9fdc
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov Oct 15, 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
30 changes: 24 additions & 6 deletions apollo-router/src/services/supergraph/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,13 +1154,31 @@ async fn root_typename_with_defer_and_empty_first_response() {
.await
.unwrap();

let query = r#"
query {
...OnlyTypename
... @defer {
currentUser {
activeOrganization {
id
suborga {
id
name
}
}
}
}
}

fragment OnlyTypename on Query {
__typename
}
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
"#;
let request = supergraph::Request::fake_builder()
.context(defer_context())
.query(
"query { __typename ... @defer { currentUser { activeOrganization { id suborga { id name } } } } }",
)
.build()
.unwrap();
.context(defer_context())
.query(query)
.build()
.unwrap();

let mut stream = service.oneshot(request).await.unwrap();
let res = stream.next_response().await.unwrap();
Expand Down
211 changes: 110 additions & 101 deletions apollo-router/src/spec/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ use crate::spec::Selection;
use crate::spec::SpecError;
use crate::Configuration;

use super::Fragment;

pub(crate) mod change;
pub(crate) mod subselections;
pub(crate) mod transform;
Expand Down Expand Up @@ -146,18 +148,6 @@ impl Query {
errors: Vec::new(),
nullified: Vec::new(),
};
// Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections)
// cf https://github.com/apollographql/router/issues/1677
let operation_kind_if_root_typename =
original_operation.and_then(|op| {
op.selection_set
.iter()
.any(|f| f.is_typename_field())
.then(|| *op.kind())
});
if let Some(operation_kind) = operation_kind_if_root_typename {
output.insert(TYPENAME, operation_kind.default_type_name().into());
}
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved

response.data = Some(
match self.apply_root_selection_set(
Expand Down Expand Up @@ -239,31 +229,31 @@ impl Query {
}
}
Some(Value::Null) => {
// Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections)
// cf https://github.com/apollographql/router/issues/1677
let operation_kind_if_root_typename = original_operation.and_then(|op| {
op.selection_set
.iter()
.any(|f| f.is_typename_field())
.then(|| *op.kind())
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
});
response.data = match operation_kind_if_root_typename {
Some(operation_kind) => {
if let Some(operation) = original_operation {
// Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections)
// cf https://github.com/apollographql/router/issues/1677
if self.has_only_typename_field(&operation.selection_set, &variables) {
let operation_type_name = schema
.root_operation(operation.kind.into())
.map(|name| name.as_str())
.unwrap_or(operation.kind.default_type_name());

let mut output = Object::default();
output.insert(TYPENAME, operation_kind.default_type_name().into());
Some(output.into())
output.insert(TYPENAME, operation_type_name.into());
response.data = Some(output.into());
return vec![];
}
None => Some(Value::default()),
};
}

response.data = Some(Value::Null);
return vec![];
}
_ => {
failfast_debug!("invalid type for data in response. data: {:#?}", data);
}
}

response.data = Some(Value::default());
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
response.data = Some(Value::Null);

vec![]
}
Expand Down Expand Up @@ -571,19 +561,13 @@ impl Query {
.and_then(|s| apollo_compiler::ast::NamedType::new(s).ok())
.map(apollo_compiler::ast::Type::Named);

let current_type = if parameters
.schema
.get_interface(field_type.inner_named_type())
.is_some()
|| parameters
.schema
.get_union(field_type.inner_named_type())
.is_some()
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
{
typename.as_ref().unwrap_or(field_type)
} else {
field_type
};
let current_type =
match parameters.schema.types.get(field_type.inner_named_type()) {
Some(ExtendedType::Interface(..) | ExtendedType::Union(..)) => {
typename.as_ref().unwrap_or(field_type)
}
_ => field_type,
};

if self
.apply_selection_set(
Expand Down Expand Up @@ -640,21 +624,11 @@ impl Query {
}

if name.as_str() == TYPENAME {
let input_value = input
.get(field_name.as_str())
.cloned()
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
.filter(|v| v.is_string())
.unwrap_or_else(|| {
Value::String(ByteString::from(
current_type.inner_named_type().as_str().to_owned(),
))
Copy link
Member Author

@IvanGoncharov IvanGoncharov Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

here, we tried to use __typename values returned by the subgraph but didn't validate them. I assume (not tested) it can even be the name of the interface if the subgraph uses @interfaceObject

According to the GraphQL spec, __typename should always use an object name (interfaces and unions are forbidden), so I changed the code to first use current_type (which is always valid) and then fallback to using the subgraph's value only if current_type is not an object type.

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be checked with what the output rewriting code in the QP is doing

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what to do here.
Realistically, I can't inspect QP in the scope of this PR.
I can revert this particular change, because it separated from other fixes.
@Geal Do you think it worth to revert it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The change looks correct to me: when a field is an object type in the supergraph we know what the correct __typename is and could even skip asking the subgraph

});
if let Some(input_str) = input_value.as_str() {
if parameters.schema.get_object(input_str).is_some() {
output.insert((*field_name).clone(), input_value);
} else {
return Err(InvalidValue);
}
let typename = current_type.inner_named_type();
if parameters.schema.get_object(typename).is_some() {
output.insert((*field_name).clone(), typename.as_str().into());
} else {
return Err(InvalidValue);
}
continue;
}
Expand Down Expand Up @@ -751,11 +725,15 @@ impl Query {
continue;
}

if let Some(fragment) = self.fragments.get(name) {
if let Some(Fragment {
type_condition,
selection_set,
}) = self.fragments.get(name)
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
{
let is_apply = current_type.inner_named_type().as_str()
== fragment.type_condition.as_str()
== type_condition.as_str()
|| parameters.schema.is_subtype(
&fragment.type_condition,
&type_condition,
current_type.inner_named_type().as_str(),
);

Expand All @@ -768,7 +746,7 @@ impl Query {
}

self.apply_selection_set(
&fragment.selection_set,
&selection_set,
parameters,
input,
output,
Expand Down Expand Up @@ -811,7 +789,12 @@ impl Query {

let field_name = alias.as_ref().unwrap_or(name);
let field_name_str = field_name.as_str();
if let Some(input_value) = input.get_mut(field_name_str) {

if name.as_str() == TYPENAME {
if !output.contains_key(field_name_str) {
output.insert(field_name.clone(), Value::String(root_type_name.into()));
}
} else if let Some(input_value) = input.get_mut(field_name_str) {
// if there's already a value for that key in the output it means either:
// - the value is a scalar and was moved into output using take(), replacing
// the input value with Null
Expand All @@ -837,10 +820,6 @@ impl Query {
);
path.pop();
res?
} else if name.as_str() == TYPENAME {
if !output.contains_key(field_name_str) {
output.insert(field_name.clone(), Value::String(root_type_name.into()));
}
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
} else if field_type.is_non_null() {
parameters.errors.push(Error {
message: format!(
Expand All @@ -861,25 +840,27 @@ impl Query {
include_skip,
..
} => {
// top level objects will not provide a __typename field
if type_condition.as_str() != root_type_name {
return Err(InvalidValue);
}
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved

if include_skip.should_skip(parameters.variables) {
continue;
}

self.apply_selection_set(
Copy link
Member Author

Choose a reason for hiding this comment

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

We are still inside root selection, even if we go inside an inline fragment.
So it should be apply_root_selection_set

Copy link
Contributor

Choose a reason for hiding this comment

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

this sounds reasonable but looks like a dangerous change. Do you have a test that would show what happens before and after that change?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it tested here:

router/apollo-router/tests/integration/subgraph_response.rs

Lines 34 to 82 in e871c93

async fn test_subgraph_returning_different_typename_on_query_root() -> Result<(), BoxError> {
let mut router = IntegrationTest::builder()
.config(CONFIG)
.responder(ResponseTemplate::new(200).set_body_json(json!({
"data": {
"topProducts": null,
"__typename": "SomeQueryRoot",
"aliased": "SomeQueryRoot",
"inside_fragment": "SomeQueryRoot",
"inside_inline_fragment": "SomeQueryRoot"
}
})))
.build()
.await;
router.start().await;
router.assert_started().await;
let query = r#"
{
topProducts { name }
__typename
aliased: __typename
...TypenameFragment
... {
inside_inline_fragment: __typename
}
}
fragment TypenameFragment on Query {
inside_fragment: __typename
}
"#;
let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await;
assert_eq!(response.status(), 200);
assert_eq!(
response.json::<serde_json::Value>().await?,
json!({
"data": {
"topProducts": null,
"__typename": "Query",
"aliased": "Query",
"inside_fragment": "Query",
"inside_inline_fragment": "Query"
}
})
);
Ok(())
}

selection_set,
parameters,
input,
output,
path,
// FIXME: use `ast::Name` everywhere so fallible conversion isn’t needed
#[allow(clippy::unwrap_used)]
&FieldType::new_named(type_condition.try_into().unwrap()).0,
)?;
// check if the fragment matches the input type directly, and if not, check if the
// input type is a subtype of the fragment's type condition (interface, union)
let is_apply = (root_type_name == type_condition.as_str())
|| parameters
.schema
.is_subtype(&type_condition, root_type_name);

if is_apply {
self.apply_root_selection_set(
root_type_name,
selection_set,
parameters,
input,
output,
path,
)?;
}
}
Selection::FragmentSpread {
name,
Expand All @@ -892,30 +873,28 @@ impl Query {
continue;
}

if let Some(fragment) = self.fragments.get(name) {
let is_apply = {
// check if the fragment matches the input type directly, and if not, check if the
// input type is a subtype of the fragment's type condition (interface, union)
root_type_name == fragment.type_condition.as_str()
|| parameters
.schema
.is_subtype(&fragment.type_condition, root_type_name)
};
if let Some(Fragment {
type_condition,
selection_set,
}) = self.fragments.get(name)
{
// check if the fragment matches the input type directly, and if not, check if the
// input type is a subtype of the fragment's type condition (interface, union)
let is_apply = (root_type_name == type_condition.as_str())
|| parameters
.schema
.is_subtype(&type_condition, root_type_name);

if !is_apply {
return Err(InvalidValue);
if is_apply {
self.apply_root_selection_set(
root_type_name,
&selection_set,
parameters,
input,
output,
path,
)?;
}

self.apply_selection_set(
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
&fragment.selection_set,
parameters,
input,
output,
path,
// FIXME: use `ast::Name` everywhere so fallible conversion isn’t needed
#[allow(clippy::unwrap_used)]
&FieldType::new_named(root_type_name.try_into().unwrap()).0,
)?;
} else {
// the fragment should have been already checked with the schema
failfast_debug!("missing fragment named: {}", name);
Expand All @@ -927,6 +906,36 @@ impl Query {
Ok(())
}

fn has_only_typename_field(&self, selection_set: &Vec<Selection>, variables: &Object) -> bool {
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
selection_set.iter().all(|s| match s {
Selection::Field { name, .. } => name.as_str() == TYPENAME,
Selection::InlineFragment {
selection_set,
include_skip,
defer,
..
} => {
defer.eval(variables).unwrap_or(true)
|| include_skip.should_skip(variables)
|| self.has_only_typename_field(selection_set, variables)
}
Selection::FragmentSpread {
name,
include_skip,
defer,
..
} => {
defer.eval(variables).unwrap_or(true)
|| include_skip.should_skip(variables)
|| if let Some(fragment) = self.fragments.get(name) {
self.has_only_typename_field(&fragment.selection_set, variables)
} else {
false
}
}
})
}

/// Validate a [`Request`]'s variables against this [`Query`] using a provided [`Schema`].
#[tracing::instrument(skip_all, level = "trace")]
pub(crate) fn validate_variables(
Expand Down
5 changes: 0 additions & 5 deletions apollo-router/src/spec/selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use crate::spec::query::DeferStats;
use crate::spec::FieldType;
use crate::spec::Schema;
use crate::spec::SpecError;
use crate::spec::TYPENAME;

#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub(crate) enum Selection {
Expand Down Expand Up @@ -190,10 +189,6 @@ impl Selection {
})
}

pub(crate) fn is_typename_field(&self) -> bool {
matches!(self, Selection::Field {name, ..} if name.as_str() == TYPENAME)
}

pub(crate) fn contains_error_path(&self, path: &[PathElement], fragments: &Fragments) -> bool {
match (path.first(), self) {
(None, _) => true,
Expand Down
Loading
Loading