Skip to content

Commit

Permalink
fix: visit rest: * selections
Browse files Browse the repository at this point in the history 
this fixes an incorrect unused field validation when using `rest: *`:

```
No connector resolves field `User.rest`. It must have a `@connect` directive or appear in `@connect(selection:)`.
```
  • Loading branch information
@lennyburdette
lennyburdette committed Oct 15, 2024
1 parent 408ac64 commit 309ad5f
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 99 deletions.
229 changes: 130 additions & 99 deletions apollo-federation/src/sources/connect/validation/selection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use apollo_compiler::schema::ExtendedType;
use apollo_compiler::schema::ObjectType;
use apollo_compiler::Node;
use apollo_compiler::Schema;
use either::Either;
use itertools::Itertools;

use super::coordinates::ConnectDirectiveCoordinate;
Expand All @@ -24,6 +25,7 @@ use crate::sources::connect::expand::visitors::FieldVisitor;
use crate::sources::connect::expand::visitors::GroupVisitor;
use crate::sources::connect::json_selection::NamedSelection;
use crate::sources::connect::json_selection::Ranged;
use crate::sources::connect::json_selection::StarSelection;
use crate::sources::connect::spec::schema::CONNECT_SELECTION_ARGUMENT_NAME;
use crate::sources::connect::validation::coordinates::connect_directive_http_body_coordinate;
use crate::sources::connect::validation::graphql::GraphQLString;
Expand Down Expand Up @@ -196,7 +198,7 @@ impl SelectionValidator<'_, '_> {
new_object_name = object.name,
),
// TODO: make a helper function for easier range collection
locations: self.get_selection_location(field.selection)
locations: self.get_range_location(field.inner_range())
// Skip over fields which duplicate the location of the selection
.chain(if depth > 1 {ancestor_field.and_then(|def| def.line_column_range(&self.schema.sources))} else {None})
.chain(field.definition.line_column_range(&self.schema.sources))
Expand All @@ -220,14 +222,44 @@ impl SelectionValidator<'_, '_> {
})
.into_iter()
}

fn get_range_location(
&self,
selection: Option<Range<usize>>,
) -> impl Iterator<Item = Range<LineColumn>> {
selection
.as_ref()
.and_then(|range| {
self.selection_arg
.value
.line_col_for_subslice(range.clone(), self.schema)
})
.into_iter()
}
}

#[derive(Clone, Copy, Debug)]
struct Field<'schema> {
selection: &'schema NamedSelection,
selection: Either<&'schema NamedSelection, &'schema StarSelection>,
definition: &'schema Node<FieldDefinition>,
}

impl<'schema> Field<'schema> {
fn next_subselection(&self) -> Option<&'schema SubSelection> {
match self.selection {
Either::Left(selection) => selection.next_subselection(),
Either::Right(_) => None,
}
}

fn inner_range(&self) -> Option<Range<usize>> {
match self.selection {
Either::Left(selection) => selection.range(),
Either::Right(selection) => selection.range(),
}
}
}

#[derive(Clone, Copy, Debug)]
enum PathPart<'a> {
// Query, Mutation, Subscription OR an Entity type
Expand Down Expand Up @@ -265,7 +297,7 @@ impl<'schema> GroupVisitor<Group<'schema>, Field<'schema>> for SelectionValidato
&self,
field: &Field<'schema>,
) -> Result<Option<Group<'schema>>, Self::Error> {
let Some(selection) = field.selection.next_subselection() else {
let Some(selection) = field.next_subselection() else {
return Ok(None);
};
let Some(ty) = self
Expand Down Expand Up @@ -293,7 +325,7 @@ impl<'schema> GroupVisitor<Group<'schema>, Field<'schema>> for SelectionValidato
for field_name in selection.names() {
if let Some(definition) = group.ty.fields.get(field_name) {
results.push(Ok(Field {
selection,
selection: Either::Left(selection),
definition,
}));
} else {
Expand All @@ -310,7 +342,94 @@ impl<'schema> GroupVisitor<Group<'schema>, Field<'schema>> for SelectionValidato
}
results
}).chain(
self.validate_star_selection(group).into_iter().map(Err)
group.selection.star_iter().map(|star| {
let Some(alias) = star.alias() else {
return Err(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `*` without an alias. Use `fieldName: *` to map properties to a field.",
coordinate = self.selection_arg.coordinate,
),
locations: self.get_selection_location(star).collect(),
});
};

let field_name = alias.name();

let Some(definition) = group.ty.fields.get(field_name) else {
return Err(Message {
code: Code::SelectedFieldNotFound,
message: format!(
"{coordinate} contains field `{field_name}`, which does not exist on `{parent_type}`.",
coordinate = self.selection_arg.coordinate,
parent_type = group.ty.name,
),
locations: self.get_selection_location(alias).collect(),
});
};

let locations = self.get_selection_location(star).chain(definition.line_column_range(&self.schema.sources));

if definition.ty.is_list() {
return Err(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns a list. It must be a non-list custom scalar.",
coordinate = self.selection_arg.coordinate,
field_name = field_name,
parent_type = group.ty.name,
ty = definition.ty
),
locations: locations.collect(),
});
}

let Some(ty) = self.schema.types.get(definition.ty.inner_named_type()) else {
return Err(Message {
code: Code::GraphQLError,
message: format!(
"{coordinate} contains field `{field_name}`, which has undefined type `{type_name}.",
coordinate = self.selection_arg.coordinate,
type_name = definition.ty.inner_named_type()
),
locations: locations.collect(),
});
};

if !ty.is_scalar() {
return Err(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns {ty_kind} type. It must be a non-list custom scalar.",
coordinate = self.selection_arg.coordinate,
field_name = field_name,
parent_type = group.ty.name,
ty = definition.ty,
ty_kind = type_sentence_part(ty)
),
locations: locations.collect(),
});
}

if ty.is_built_in() {
return Err(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns a built-in scalar type. It must be a non-list custom scalar.",
coordinate = self.selection_arg.coordinate,
field_name = field_name,
parent_type = group.ty.name,
ty = definition.ty
),
locations: locations.collect(),
});
}

Ok(Field {
selection: Either::Right(star),
definition,
})
})
).collect()
}

Expand All @@ -332,9 +451,9 @@ impl<'schema> FieldVisitor<Field<'schema>> for SelectionValidator<'schema, '_> {
message: format!(
"{coordinate} contains field `{field_name}`, which has undefined type `{type_name}.",
),
locations: self.get_selection_location(field.selection).collect(),
locations: self.get_range_location(field.inner_range()).collect(),
})?;
let is_group = field.selection.next_subselection().is_some();
let is_group = field.next_subselection().is_some();

self.seen_fields.insert((
self.last_field().ty().name.clone(),
Expand All @@ -348,7 +467,7 @@ impl<'schema> FieldVisitor<Field<'schema>> for SelectionValidator<'schema, '_> {
"{coordinate} selects field `{parent_type}.{field_name}`, which has arguments. Only fields with a connector can have arguments.",
parent_type = self.last_field().ty().name,
),
locations: self.get_selection_location(field.selection).chain(field.definition.line_column_range(&self.schema.sources)).collect(),
locations: self.get_range_location(field.inner_range()).chain(field.definition.line_column_range(&self.schema.sources)).collect(),
});
}

Expand All @@ -363,7 +482,7 @@ impl<'schema> FieldVisitor<Field<'schema>> for SelectionValidator<'schema, '_> {
"{coordinate} selects a group `{field_name}{{}}`, but `{parent_type}.{field_name}` is of type `{type_name}` which is not an object.",
parent_type = self.last_field().ty().name,
),
locations: self.get_selection_location(field.selection).chain(field.definition.line_column_range(&self.schema.sources)).collect(),
locations: self.get_range_location(field.inner_range()).chain(field.definition.line_column_range(&self.schema.sources)).collect(),
})
},
(ExtendedType::Object(_), false) => {
Expand All @@ -373,15 +492,15 @@ impl<'schema> FieldVisitor<Field<'schema>> for SelectionValidator<'schema, '_> {
"`{parent_type}.{field_name}` is an object, so {coordinate} must select a group `{field_name}{{}}`.",
parent_type = self.last_field().ty().name,
),
locations: self.get_selection_location(field.selection).chain(field.definition.line_column_range(&self.schema.sources)).collect(),
locations: self.get_range_location(field.inner_range()).chain(field.definition.line_column_range(&self.schema.sources)).collect(),
})
},
(_, false) => Ok(()),
}
}
}

impl SelectionValidator<'_, '_> {
impl<'schema, 'a> SelectionValidator<'schema, 'a> {
fn path_with_root(&self) -> impl Iterator<Item = PathPart> {
once(self.root).chain(self.path.iter().copied())
}
Expand All @@ -399,94 +518,6 @@ impl SelectionValidator<'_, '_> {
fn last_field(&self) -> &PathPart {
self.path.last().unwrap_or(&self.root)
}

/// When using `*`, it must be mapped to a field via an alias, and the field
/// must be a non-list custom scalar.
fn validate_star_selection(&self, group: &Group) -> Vec<Message> {
let coordinate = self.selection_arg.coordinate;
group
.selection
.star_iter()
.filter_map(|star| {
let Some(alias) = star.alias() else {
return Some(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `*` without an alias. Use `fieldName: *` to map properties to a field.",
),
locations: self.get_selection_location(star).collect(),
});
};
let field_name = alias.name();

let Some(definition) = group.ty.fields.get(field_name) else {
return Some(Message {
code: Code::SelectedFieldNotFound,
message: format!(
"{coordinate} contains field `{field_name}`, which does not exist on `{parent_type}`.",
parent_type = group.ty.name,
),
locations: self.get_selection_location(alias).collect(),
});
};

let locations = self.get_selection_location(star).chain(definition.line_column_range(&self.schema.sources));

if definition.ty.is_list() {
return Some(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns a list. It must be a non-list custom scalar.",
field_name = field_name,
parent_type = group.ty.name,
ty = definition.ty
),
locations: locations.collect(),
});
}

let Some(ty) = self.schema.types.get(definition.ty.inner_named_type()) else {
return Some(Message {
code: Code::GraphQLError,
message: format!(
"{coordinate} contains field `{field_name}`, which has undefined type `{type_name}.",
type_name = definition.ty.inner_named_type()
),
locations: locations.collect(),
});
};

if !ty.is_scalar() {
return Some(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns {ty_kind} type. It must be a non-list custom scalar.",
field_name = field_name,
parent_type = group.ty.name,
ty = definition.ty,
ty_kind = type_sentence_part(ty)
),
locations: locations.collect(),
});
}

if ty.is_built_in() {
return Some(Message {
code: Code::InvalidStarSelection,
message: format!(
"{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns a built-in scalar type. It must be a non-list custom scalar.",
field_name = field_name,
parent_type = group.ty.name,
ty = definition.ty
),
locations: locations.collect(),
});
}

None
})
.collect()
}
}

fn type_sentence_part(ty: &ExtendedType) -> String {
Expand Down
6 changes: 6 additions & 0 deletions ...deration/src/sources/connect/validation/snapshots/[email protected]
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
source: apollo-federation/src/sources/connect/validation/mod.rs
expression: "format!(\"{:#?}\", errors)"
input_file: apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql
---
[]
18 changes: 18 additions & 0 deletions apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
extend schema
@link(
url: "https://specs.apollo.dev/connect/v0.1"
import: ["@connect", "@source"]
)
@source(name: "v2", http: { baseURL: "http://127.0.0.1" })

type Query {
good: [Good!]!
@connect(source: "v2", http: { GET: "/" }, selection: "id rest: *")
}

type Good {
id: ID!
rest: JSON
}

scalar JSON

0 comments on commit 309ad5f

Please sign in to comment.