From 999c3f9516e30dbfb624903052ef7a4413ba378b Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Wed, 18 Sep 2024 09:48:50 -0600 Subject: [PATCH 1/6] More robust quote handling for URL substrings --- Cargo.lock | 7 +++ apollo-federation/Cargo.toml | 1 + .../sources/connect/validation/http/url.rs | 58 +++++++++++-------- 3 files changed, 41 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 913b9ece62..a47274c453 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -189,6 +189,7 @@ dependencies = [ "insta", "itertools 0.13.0", "lazy_static", + "line-col", "multimap 0.10.0", "nom", "nom_locate", @@ -3984,6 +3985,12 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "line-col" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e69cdf6b85b5c8dce514f694089a2cf8b1a702f6cd28607bcb3cf296c9778db" + [[package]] name = "linked-hash-map" version = "0.5.6" diff --git a/apollo-federation/Cargo.toml b/apollo-federation/Cargo.toml index 32e49485fc..f02f1971e4 100644 --- a/apollo-federation/Cargo.toml +++ b/apollo-federation/Cargo.toml @@ -26,6 +26,7 @@ http.workspace = true indexmap = { version = "2.2.6", features = ["serde"] } itertools = "0.13.0" lazy_static = "1.4.0" +line-col = "0.2.1" multimap = "0.10.0" nom = "7.1.3" petgraph = { version = "0.6.4", features = ["serde-1"] } diff --git a/apollo-federation/src/sources/connect/validation/http/url.rs b/apollo-federation/src/sources/connect/validation/http/url.rs index e653e54313..7bcf69261d 100644 --- a/apollo-federation/src/sources/connect/validation/http/url.rs +++ b/apollo-federation/src/sources/connect/validation/http/url.rs @@ -26,11 +26,11 @@ pub(crate) fn validate_template( Message { code: Code::InvalidUrl, message: format!("{coordinate} must be a valid URL template. {message}"), - locations: select_substring_location( - value.line_column_range(sources), - str_value, - location, - ), + locations: location + .and_then(|location| select_substring_location(value, location, sources)) + .or_else(|| value.line_column_range(sources)) + .into_iter() + .collect(), } })?; @@ -61,25 +61,33 @@ pub(crate) fn validate_base_url( } fn select_substring_location( - line_column: Option>, - full_url: &str, - substring_location: Option>, -) -> Vec> { - line_column - .map(|mut template_location| { - // The default location includes the parameter name, we just want the value, - // so we need to calculate that. - template_location.end.column -= 1; // Get rid of the end quote - template_location.start.column = template_location.end.column - full_url.len(); + value: &Node, + substring_location: Range, + sources: &SourceMap, +) -> Option> { + let value_without_quotes = value.as_str()?; - if let Some(location) = substring_location { - // We can point to a substring of the URL template! do it. - template_location.start.column += location.start; - template_location.end.column = - template_location.start.column + location.end - location.start; - } - template_location - }) - .into_iter() - .collect() + let source_span = value.location()?; + let file = sources.get(&source_span.file_id())?; + let source_text = file.source_text(); + let start_of_quotes = source_span.offset(); + let end_of_quotes = source_span.end_offset(); + let value_with_quotes = source_text.get(start_of_quotes..end_of_quotes)?; + + let len_of_starting_quotes = value_with_quotes.find(value_without_quotes)?; + let len_of_ending_quotes = + value_with_quotes.len() - value_without_quotes.len() - len_of_starting_quotes; + + let subslice_start_offset = start_of_quotes + len_of_starting_quotes + substring_location.start; + let subslice_end_offset = end_of_quotes + - len_of_ending_quotes + - (value_without_quotes.len() - substring_location.end); + + let lookup = line_col::LineColLookup::new(source_text); // TODO: store and reuse + let (line, column) = lookup.get(subslice_start_offset); + let start = LineColumn { line, column }; + let (line, column) = lookup.get(subslice_end_offset); + let end = LineColumn { line, column }; + + Some(start..end) } From f40853dce3cad705de4a7f6ac65a8644818093e6 Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Thu, 19 Sep 2024 17:32:03 -0600 Subject: [PATCH 2/6] Use a custom string type for better sub-locations --- .../src/sources/connect/validation/entity.rs | 42 +++-- .../connect/validation/extended_type.rs | 59 +++---- .../src/sources/connect/validation/graphql.rs | 2 +- .../connect/validation/graphql/strings.rs | 77 +++++++--- .../sources/connect/validation/http/method.rs | 4 +- .../sources/connect/validation/http/url.rs | 144 +++++++----------- .../src/sources/connect/validation/mod.rs | 82 ++++++---- .../sources/connect/validation/selection.rs | 3 +- ...ts@invalid-path-parameter.graphql.snap.new | 21 +++ ...ld_arg_input_type_missing.graphql.snap.new | 21 +++ ...in_url_template_variables.graphql.snap.new | 60 ++++++++ ...ts@nullable_path_var_hint.graphql.snap.new | 34 +++++ ...alid_connect_absolute_url.graphql.snap.new | 21 +++ ...eld_arg_key_multiple_keys.graphql.snap.new | 21 +++ ...ld_arg_key_non_resolvable.graphql.snap.new | 21 +++ ...g_field_arg_keys_disjoint.graphql.snap.new | 34 +++++ ...valid_entity_arg_on_field.graphql.snap.new | 21 +++ .../sources/connect/validation/source_name.rs | 13 +- 18 files changed, 468 insertions(+), 212 deletions(-) create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new diff --git a/apollo-federation/src/sources/connect/validation/entity.rs b/apollo-federation/src/sources/connect/validation/entity.rs index 625aae21b8..a3823956db 100644 --- a/apollo-federation/src/sources/connect/validation/entity.rs +++ b/apollo-federation/src/sources/connect/validation/entity.rs @@ -4,7 +4,6 @@ use apollo_compiler::ast::InputValueDefinition; use apollo_compiler::ast::Value; use apollo_compiler::executable::FieldSet; use apollo_compiler::executable::Selection; -use apollo_compiler::parser::SourceMap; use apollo_compiler::schema::Component; use apollo_compiler::schema::Directive; use apollo_compiler::schema::ExtendedType; @@ -12,7 +11,6 @@ use apollo_compiler::schema::InputObjectType; use apollo_compiler::schema::ObjectType; use apollo_compiler::Name; use apollo_compiler::Node; -use apollo_compiler::Schema; use super::coordinates::connect_directive_entity_argument_coordinate; use super::coordinates::field_with_connect_directive_entity_true_coordinate; @@ -23,13 +21,13 @@ use super::Message; use crate::sources::connect::expand::visitors::FieldVisitor; use crate::sources::connect::expand::visitors::GroupVisitor; use crate::sources::connect::spec::schema::CONNECT_ENTITY_ARGUMENT_NAME; +use crate::sources::connect::validation::graphql::SchemaInfo; pub(super) fn validate_entity_arg( field: &Component, connect_directive: &Node, object: &Node, - schema: &Schema, - source_map: &SourceMap, + schema: &SchemaInfo, category: ObjectCategory, ) -> Vec { let mut messages = vec![]; @@ -52,7 +50,7 @@ pub(super) fn validate_entity_arg( "{coordinate} is invalid. Entity resolvers can only be declared on root `Query` fields.", coordinate = connect_directive_entity_argument_coordinate(connect_directive_name, entity_arg_value.as_ref(), object, &field.name) ), - locations: entity_arg.line_column_range(source_map) + locations: entity_arg.line_column_range(&schema.sources) .into_iter() .collect(), }) @@ -73,7 +71,7 @@ pub(super) fn validate_entity_arg( ) ), locations: entity_arg - .line_column_range(source_map) + .line_column_range(&schema.sources) .into_iter() .collect(), }) @@ -95,7 +93,7 @@ pub(super) fn validate_entity_arg( ), ), locations: entity_arg - .line_column_range(source_map) + .line_column_range(&schema.sources) .into_iter() .collect(), }); @@ -104,11 +102,9 @@ pub(super) fn validate_entity_arg( if let Some(message) = (ArgumentVisitor { schema, - connect_directive_name, entity_arg, entity_arg_value, object, - source_map, field: &field.name, key_fields, }) @@ -160,12 +156,10 @@ struct Field<'schema> { /// Since input types may contain fields with subtypes, and the fields of those subtypes can be /// part of composite keys, this potentially requires visiting a tree. struct ArgumentVisitor<'schema> { - schema: &'schema Schema, - connect_directive_name: &'schema Name, + schema: &'schema SchemaInfo<'schema>, entity_arg: &'schema Node, entity_arg_value: &'schema Node, object: &'schema Node, - source_map: &'schema SourceMap, field: &'schema Name, key_fields: Vec
, } @@ -229,7 +223,7 @@ impl<'schema> FieldVisitor> for ArgumentVisitor<'schema> { message: format!( "{coordinate} has invalid arguments. Mismatched type on field `{field_name}` - expected `{entity_type}` but found `{input_type}`.", coordinate = field_with_connect_directive_entity_true_coordinate( - self.connect_directive_name, + self.schema.connect_directive_name, self.entity_arg_value.as_ref(), self.object, self.field, @@ -239,9 +233,9 @@ impl<'schema> FieldVisitor> for ArgumentVisitor<'schema> { entity_type = field.entity_type.name(), ), locations: field.node - .line_column_range(self.source_map) + .line_column_range(&self.schema.sources) .into_iter() - .chain(self.entity_arg.line_column_range(self.source_map)) + .chain(self.entity_arg.line_column_range(&self.schema.sources)) .collect(), }) } @@ -292,7 +286,7 @@ impl<'schema> ArgumentVisitor<'schema> { message: format!( "{coordinate} has invalid arguments. Argument `{arg_name}` does not have a matching field `{arg_name}` on type `{entity_type}`.", coordinate = field_with_connect_directive_entity_true_coordinate( - self.connect_directive_name, + self.schema.connect_directive_name, self.entity_arg_value.as_ref(), self.object, &field.name @@ -301,9 +295,9 @@ impl<'schema> ArgumentVisitor<'schema> { entity_type = entity_type.name, ), locations: arg - .line_column_range(self.source_map) + .line_column_range(&self.schema.sources) .into_iter() - .chain(self.entity_arg.line_column_range(self.source_map)) + .chain(self.entity_arg.line_column_range(&self.schema.sources)) .collect(), })) } @@ -364,7 +358,7 @@ impl<'schema> ArgumentVisitor<'schema> { message: format!( "{coordinate} has invalid arguments. Field `{name}` on `{input_type}` does not have a matching field `{name}` on `{entity_type}`.", coordinate = field_with_connect_directive_entity_true_coordinate( - self.connect_directive_name, + self.schema.connect_directive_name, self.entity_arg_value.as_ref(), self.object, self.field, @@ -373,9 +367,9 @@ impl<'schema> ArgumentVisitor<'schema> { entity_type = entity_object_type.name, ), locations: input_field - .line_column_range(self.source_map) + .line_column_range(&self.schema.sources) .into_iter() - .chain(self.entity_arg.line_column_range(self.source_map)) + .chain(self.entity_arg.line_column_range(&self.schema.sources)) .collect(), })) } @@ -419,16 +413,16 @@ impl<'schema> ArgumentVisitor<'schema> { None => format!("Argument `{name}`"), }, coordinate = field_with_connect_directive_entity_true_coordinate( - self.connect_directive_name, + self.schema.connect_directive_name, self.entity_arg_value.as_ref(), self.object, self.field, ), ), locations: node - .line_column_range(self.source_map) + .line_column_range(&self.schema.sources) .into_iter() - .chain(self.entity_arg.line_column_range(self.source_map)) + .chain(self.entity_arg.line_column_range(&self.schema.sources)) .collect(), }) } else { diff --git a/apollo-federation/src/sources/connect/validation/extended_type.rs b/apollo-federation/src/sources/connect/validation/extended_type.rs index e04c8e32d2..c3d1e385ac 100644 --- a/apollo-federation/src/sources/connect/validation/extended_type.rs +++ b/apollo-federation/src/sources/connect/validation/extended_type.rs @@ -1,11 +1,6 @@ -use std::sync::Arc; - use apollo_compiler::ast::FieldDefinition; -use apollo_compiler::collections::IndexMap; use apollo_compiler::collections::IndexSet; use apollo_compiler::executable::Selection; -use apollo_compiler::parser::FileId; -use apollo_compiler::parser::SourceFile; use apollo_compiler::parser::SourceMap; use apollo_compiler::parser::SourceSpan; use apollo_compiler::schema::Component; @@ -13,7 +8,6 @@ use apollo_compiler::schema::ExtendedType; use apollo_compiler::schema::ObjectType; use apollo_compiler::Name; use apollo_compiler::Node; -use apollo_compiler::Schema; use itertools::Itertools; use super::coordinates::ConnectDirectiveCoordinate; @@ -33,33 +27,26 @@ use super::Message; use crate::sources::connect::spec::schema::CONNECT_BODY_ARGUMENT_NAME; use crate::sources::connect::spec::schema::CONNECT_SOURCE_ARGUMENT_NAME; use crate::sources::connect::spec::schema::HTTP_ARGUMENT_NAME; +use crate::sources::connect::validation::graphql::SchemaInfo; pub(super) fn validate_extended_type( extended_type: &ExtendedType, - schema: &Schema, - connect_directive_name: &Name, - source_directive_name: &Name, + schema: &SchemaInfo, all_source_names: &[SourceName], - source_map: &Arc>>, seen_fields: &mut IndexSet<(Name, Name)>, ) -> Vec { match extended_type { - ExtendedType::Object(object) => validate_object_fields( - object, - schema, - connect_directive_name, - source_directive_name, - all_source_names, - seen_fields, - ), + ExtendedType::Object(object) => { + validate_object_fields(object, schema, all_source_names, seen_fields) + } ExtendedType::Union(union_type) => vec![validate_abstract_type( SourceSpan::recompose(union_type.location(), union_type.name.location()), - source_map, + &schema.sources, "union", )], ExtendedType::Interface(interface) => vec![validate_abstract_type( SourceSpan::recompose(interface.location(), interface.name.location()), - source_map, + &schema.sources, "interface", )], _ => Vec::new(), @@ -77,9 +64,7 @@ pub enum ObjectCategory { /// are resolvable by some combination of `@connect` directives. fn validate_object_fields( object: &Node, - schema: &Schema, - connect_directive_name: &Name, - source_directive_name: &Name, + schema: &SchemaInfo, source_names: &[SourceName], seen_fields: &mut IndexSet<(Name, Name)>, ) -> Vec { @@ -122,7 +107,8 @@ fn validate_object_fields( return vec![Message { code: Code::SubscriptionInConnectors, message: format!( - "A subscription root type is not supported when using `@{connect_directive_name}`." + "A subscription root type is not supported when using `@{connect_directive_name}`.", + connect_directive_name = schema.connect_directive_name, ), locations: object.line_column_range(source_map).into_iter().collect(), }]; @@ -154,8 +140,6 @@ fn validate_object_fields( object_category, source_names, object, - connect_directive_name, - source_directive_name, schema, seen_fields, ) @@ -163,20 +147,17 @@ fn validate_object_fields( .collect() } -#[allow(clippy::too_many_arguments)] fn validate_field( field: &Component, category: ObjectCategory, source_names: &[SourceName], object: &Node, - connect_directive_name: &Name, - source_directive_name: &Name, - schema: &Schema, + schema: &SchemaInfo, seen_fields: &mut IndexSet<(Name, Name)>, ) -> Vec { let field_coordinate = FieldCoordinate { object, field }; let connect_coordinate = ConnectDirectiveCoordinate { - connect_directive_name, + connect_directive_name: schema.connect_directive_name, field_coordinate, }; let source_map = &schema.sources; @@ -184,7 +165,7 @@ fn validate_field( let connect_directives = field .directives .iter() - .filter(|directive| directive.name == *connect_directive_name) + .filter(|directive| directive.name == *schema.connect_directive_name) .collect_vec(); if connect_directives.is_empty() { @@ -194,14 +175,14 @@ fn validate_field( field, object, source_map, - connect_directive_name, + schema.connect_directive_name, )), ObjectCategory::Mutation => errors.push(get_missing_connect_directive_message( Code::MutationFieldMissingConnect, field, object, source_map, - connect_directive_name, + schema.connect_directive_name, )), _ => (), } @@ -240,7 +221,6 @@ fn validate_field( connect_directive, object, schema, - source_map, category, )); @@ -294,10 +274,8 @@ fn validate_field( &field.name, &object.name, source_name, - source_map, source_names, - source_directive_name, - &connect_directive.name, + schema, )); if let Some((template, coordinate)) = url_template { @@ -320,7 +298,8 @@ fn validate_field( code: Code::RelativeConnectUrlWithoutSource, message: format!( "{coordinate} specifies the relative URL {raw_value}, but no `{CONNECT_SOURCE_ARGUMENT_NAME}` is defined. Either use an absolute URL including scheme (e.g. https://), or add a `@{source_directive_name}`.", - raw_value = coordinate.node + raw_value = coordinate.node, + source_directive_name = schema.source_directive_name, ), locations: coordinate.node.line_column_range(source_map).into_iter().collect() }) @@ -332,7 +311,7 @@ fn validate_field( http_arg, source_map, HttpHeadersCoordinate::Connect { - directive_name: connect_directive_name, + directive_name: schema.connect_directive_name, object: &object.name, field: &field.name, }, diff --git a/apollo-federation/src/sources/connect/validation/graphql.rs b/apollo-federation/src/sources/connect/validation/graphql.rs index 7edbfeb6d8..e566be9057 100644 --- a/apollo-federation/src/sources/connect/validation/graphql.rs +++ b/apollo-federation/src/sources/connect/validation/graphql.rs @@ -1,6 +1,6 @@ +//! Helper structs & functions for dealing with GraphQL schemas use std::ops::Deref; -///! Helper structs & functions for dealing with GraphQL schemas use apollo_compiler::Name; use apollo_compiler::Schema; use line_col::LineColLookup; diff --git a/apollo-federation/src/sources/connect/validation/graphql/strings.rs b/apollo-federation/src/sources/connect/validation/graphql/strings.rs index 78f8755635..ef36ffc45d 100644 --- a/apollo-federation/src/sources/connect/validation/graphql/strings.rs +++ b/apollo-federation/src/sources/connect/validation/graphql/strings.rs @@ -14,6 +14,7 @@ use apollo_compiler::parser::SourceMap; use apollo_compiler::Node; use line_col::LineColLookup; +#[derive(Clone, Copy)] pub(crate) struct GraphQLString<'schema> { /// The GraphQL String literal without quotes, but with all whitespace intact raw_string: &'schema str, @@ -59,7 +60,7 @@ impl<'schema> GraphQLString<'schema> { lookup: &LineColLookup, ) -> Option> { let start_offset = self.offset + substring_location.start; - let end_offset = start_offset + substring_location.end; + let end_offset = self.offset + substring_location.end; let (line, column) = lookup.get(start_offset); let start = LineColumn { line, column }; @@ -72,47 +73,89 @@ impl<'schema> GraphQLString<'schema> { #[cfg(test)] mod tests { + use apollo_compiler::ast::Value; use apollo_compiler::parser::LineColumn; use apollo_compiler::schema::ExtendedType; + use apollo_compiler::Node; use apollo_compiler::Schema; use line_col::LineColLookup; use pretty_assertions::assert_eq; use crate::sources::connect::validation::graphql::GraphQLString; - #[test] - fn single_quoted_string() { - let text = r#" + const SCHEMA: &str = r#" type Query { - field: String @connect(http: {GET: "https://example.com"}) + field: String @connect( + http: { + GET: "https://example.com" + }, + selection: """ + something + somethingElse { + nested + } + """ + ) } "#; - let schema = Schema::parse(text, "test.graphql").unwrap(); + fn connect_argument<'schema>(schema: &'schema Schema, name: &str) -> &'schema Node { let ExtendedType::Object(query) = schema.types.get("Query").unwrap() else { panic!("Query type not found"); }; let field = query.fields.get("field").unwrap(); let directive = field.directives.get("connect").unwrap(); - let http = directive - .argument_by_name("http") - .unwrap() - .as_object() - .unwrap(); + directive.argument_by_name(name).unwrap() + } + + #[test] + fn single_quoted_string() { + let schema = Schema::parse(SCHEMA, "test.graphql").unwrap(); + let http = connect_argument(&schema, "http").as_object().unwrap(); let value = &http[0].1; let string = GraphQLString::new(value, &schema.sources).unwrap(); assert_eq!(string.as_str(), "https://example.com"); - let lookup = LineColLookup::new(text); + let lookup = LineColLookup::new(SCHEMA); + assert_eq!( + string.line_col_for_subslice(2..5, &lookup), + Some( + LineColumn { + line: 5, + column: 25 + }..LineColumn { + line: 5, + column: 28 + } + ) + ); + } + + #[test] + fn multi_line_string() { + let schema = Schema::parse(SCHEMA, "test.graphql").unwrap(); + let value = connect_argument(&schema, "selection"); + + let string = GraphQLString::new(value, &schema.sources).unwrap(); + assert_eq!( + string.as_str(), + r#" + something + somethingElse { + nested + } + "# + ); + let lookup = LineColLookup::new(SCHEMA); assert_eq!( - string.line_col_for_subslice(0..4, &lookup), + string.line_col_for_subslice(8..16, &lookup), Some( LineColumn { - line: 3, - column: 38 + line: 4, + column: 12 }..LineColumn { - line: 3, - column: 42 + line: 4, + column: 16 } ) ); diff --git a/apollo-federation/src/sources/connect/validation/http/method.rs b/apollo-federation/src/sources/connect/validation/http/method.rs index 27c8b1a546..808d8df655 100644 --- a/apollo-federation/src/sources/connect/validation/http/method.rs +++ b/apollo-federation/src/sources/connect/validation/http/method.rs @@ -1,7 +1,6 @@ use apollo_compiler::ast::Value; use apollo_compiler::Name; use apollo_compiler::Node; -use apollo_compiler::Schema; use super::url::validate_template; use crate::sources::connect::spec::schema::CONNECT_HTTP_ARGUMENT_DELETE_METHOD_NAME; @@ -11,6 +10,7 @@ use crate::sources::connect::spec::schema::CONNECT_HTTP_ARGUMENT_POST_METHOD_NAM use crate::sources::connect::spec::schema::CONNECT_HTTP_ARGUMENT_PUT_METHOD_NAME; use crate::sources::connect::validation::coordinates::ConnectHTTPCoordinate; use crate::sources::connect::validation::coordinates::HttpMethodCoordinate; +use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::validation::Code; use crate::sources::connect::validation::Message; use crate::sources::connect::URLTemplate; @@ -19,7 +19,7 @@ pub(crate) fn validate<'schema>( http_arg: &'schema [(Name, Node)], coordinate: ConnectHTTPCoordinate<'schema>, http_arg_node: &Node, - schema: &Schema, + schema: &SchemaInfo, ) -> Result<(URLTemplate, HttpMethodCoordinate<'schema>), Vec> { let source_map = &schema.sources; let mut methods = http_arg diff --git a/apollo-federation/src/sources/connect/validation/http/url.rs b/apollo-federation/src/sources/connect/validation/http/url.rs index 3113f9a228..cae37f4145 100644 --- a/apollo-federation/src/sources/connect/validation/http/url.rs +++ b/apollo-federation/src/sources/connect/validation/http/url.rs @@ -1,12 +1,9 @@ use std::fmt::Display; -use std::ops::Range; use std::str::FromStr; use apollo_compiler::ast::FieldDefinition; use apollo_compiler::ast::Type; use apollo_compiler::ast::Value; -use apollo_compiler::parser::LineColumn; -use apollo_compiler::parser::SourceMap; use apollo_compiler::schema::Component; use apollo_compiler::schema::ExtendedType; use apollo_compiler::Node; @@ -16,7 +13,8 @@ use url::Url; use crate::sources::connect::url_template; use crate::sources::connect::url_template::VariableType; use crate::sources::connect::validation::coordinates::HttpMethodCoordinate; -use crate::sources::connect::validation::require_value_is_str; +use crate::sources::connect::validation::graphql::GraphQLString; +use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::validation::Code; use crate::sources::connect::validation::Message; use crate::sources::connect::URLTemplate; @@ -24,24 +22,16 @@ use crate::sources::connect::Variable; pub(crate) fn validate_template( coordinate: HttpMethodCoordinate, - schema: &Schema, + schema: &SchemaInfo, ) -> Result> { - let (template, str_value) = match parse_template(coordinate, &schema.sources) { + let (template, str_value) = match parse_template(coordinate, schema) { Ok(tuple) => tuple, Err(message) => return Err(vec![message]), }; let mut messages = Vec::new(); if let Some(base) = template.base.as_ref() { - messages.extend( - validate_base_url( - base, - coordinate, - coordinate.node, - str_value, - &schema.sources, - ) - .err(), - ); + messages + .extend(validate_base_url(base, coordinate, coordinate.node, str_value, schema).err()); } for variable in template.path_variables() { @@ -54,11 +44,10 @@ pub(crate) fn validate_template( "Variables in path parameters should be non-null, but {coordinate} contains `{{{variable}}}` which is nullable. \ If a null value is provided at runtime, the request will fail.", ), - locations: select_substring_location( - coordinate.node.line_column_range(&schema.sources), - str_value, - Some(variable.location.clone()), - ), + locations: str_value.line_col_for_subslice( + variable.location.clone(), + &schema.lookup, + ).into_iter().collect(), }); } Ok(_) => {} // Type is non-null, or unknowable @@ -80,21 +69,28 @@ pub(crate) fn validate_template( fn parse_template<'schema>( coordinate: HttpMethodCoordinate<'schema>, - sources: &SourceMap, -) -> Result<(URLTemplate, &'schema str), Message> { - let str_value = require_value_is_str(coordinate.node, coordinate, sources)?; - let template = - URLTemplate::from_str(str_value).map_err(|url_template::Error { message, location }| { - Message { - code: Code::InvalidUrl, - message: format!("{coordinate} must be a valid URL template. {message}"), - locations: select_substring_location( - coordinate.node.line_column_range(sources), - str_value, - location, - ), - } + schema: &'schema SchemaInfo, +) -> Result<(URLTemplate, GraphQLString<'schema>), Message> { + let str_value = + GraphQLString::new(coordinate.node, &schema.sources).ok_or_else(|| Message { + code: Code::GraphQLError, + message: format!("The value for {coordinate} must be a string."), + locations: coordinate + .node + .line_column_range(&schema.sources) + .into_iter() + .collect(), })?; + let template = URLTemplate::from_str(str_value.as_str()).map_err( + |url_template::Error { message, location }| Message { + code: Code::InvalidUrl, + message: format!("{coordinate} must be a valid URL template. {message}"), + locations: location + .and_then(|location| str_value.line_col_for_subslice(location, &schema.lookup)) + .into_iter() + .collect(), + }, + )?; Ok((template, str_value)) } @@ -102,65 +98,32 @@ pub(crate) fn validate_base_url( url: &Url, coordinate: impl Display, value: &Node, - str_value: &str, - sources: &SourceMap, + str_value: GraphQLString, + schema: &SchemaInfo, ) -> Result<(), Message> { let scheme = url.scheme(); if scheme != "http" && scheme != "https" { - let scheme_location = Some(0..scheme.len()); + let scheme_location = 0..scheme.len(); Err(Message { code: Code::InvalidUrlScheme, message: format!( "The value {value} for {coordinate} must be http or https, got {scheme}.", ), - locations: select_substring_location( - value.line_column_range(sources), - str_value, - scheme_location, - ), + locations: str_value + .line_col_for_subslice(scheme_location, &schema.lookup) + .into_iter() + .collect(), }) } else { Ok(()) } } -fn select_substring_location( - value: &Node, - substring_location: Range, - sources: &SourceMap, -) -> Option> { - let value_without_quotes = value.as_str()?; - - let source_span = value.location()?; - let file = sources.get(&source_span.file_id())?; - let source_text = file.source_text(); - let start_of_quotes = source_span.offset(); - let end_of_quotes = source_span.end_offset(); - let value_with_quotes = source_text.get(start_of_quotes..end_of_quotes)?; - - let len_of_starting_quotes = value_with_quotes.find(value_without_quotes)?; - let len_of_ending_quotes = - value_with_quotes.len() - value_without_quotes.len() - len_of_starting_quotes; - - let subslice_start_offset = start_of_quotes + len_of_starting_quotes + substring_location.start; - let subslice_end_offset = end_of_quotes - - len_of_ending_quotes - - (value_without_quotes.len() - substring_location.end); - - let lookup = line_col::LineColLookup::new(source_text); // TODO: store and reuse - let (line, column) = lookup.get(subslice_start_offset); - let start = LineColumn { line, column }; - let (line, column) = lookup.get(subslice_end_offset); - let end = LineColumn { line, column }; - - Some(start..end) -} - fn validate_variable<'schema>( variable: &'schema Variable, - url_value: &str, + url_value: GraphQLString, coordinate: HttpMethodCoordinate<'schema>, - schema: &'schema Schema, + schema: &'schema SchemaInfo, ) -> Result, Message> { let field_coordinate = coordinate.connect.field_coordinate; let field = field_coordinate.field; @@ -179,11 +142,10 @@ fn validate_variable<'schema>( message: format!( "{coordinate} contains `{{{variable}}}`, but {field_coordinate} does not have an argument named `{path_root}`.", ), - locations: select_substring_location( - coordinate.node.line_column_range(&schema.sources), - url_value, - Some(path_component_start..path_component_end), - ) + locations: url_value.line_col_for_subslice( + path_component_start..path_component_end, + &schema.lookup, + ).into_iter().collect() } }).map(|arg| arg.ty.as_ref().clone())? } @@ -194,11 +156,10 @@ fn validate_variable<'schema>( "{coordinate} contains `{{{variable}}}`, but {object} does not have a field named `{path_root}`.", object = field_coordinate.object.name, ), - locations: select_substring_location( - coordinate.node.line_column_range(&schema.sources), - url_value, - Some(path_component_start..path_component_end), - ) + locations: url_value.line_col_for_subslice( + path_component_start..path_component_end, + &schema.lookup, + ).into_iter().collect() }).map(|field| field.ty.clone())? } }; @@ -234,11 +195,10 @@ fn validate_variable<'schema>( message: format!( "{coordinate} contains `{{{variable}}}`, but `{variable_type}` does not have a field named `{nested_field_name}`.", ), - locations: select_substring_location( - coordinate.node.line_column_range(&schema.sources), - url_value, - Some(path_component_start..path_component_end), - ) + locations: url_value.line_col_for_subslice( + path_component_start..path_component_end, + &schema.lookup, + ).into_iter().collect(), }) })?.clone(); if parent_is_nullable && variable_type.is_non_null() { diff --git a/apollo-federation/src/sources/connect/validation/mod.rs b/apollo-federation/src/sources/connect/validation/mod.rs index 53abf5e532..3d269886a8 100644 --- a/apollo-federation/src/sources/connect/validation/mod.rs +++ b/apollo-federation/src/sources/connect/validation/mod.rs @@ -17,6 +17,7 @@ mod coordinates; mod entity; mod extended_type; +mod graphql; mod http; mod selection; mod source_name; @@ -44,6 +45,7 @@ use apollo_compiler::Schema; use coordinates::source_http_argument_coordinate; use extended_type::validate_extended_type; use itertools::Itertools; +use line_col::LineColLookup; use source_name::SourceName; use url::Url; @@ -59,6 +61,8 @@ use crate::sources::connect::spec::schema::SOURCE_DIRECTIVE_NAME_IN_SPEC; use crate::sources::connect::spec::schema::SOURCE_NAME_ARGUMENT_NAME; use crate::sources::connect::validation::coordinates::BaseUrlCoordinate; use crate::sources::connect::validation::coordinates::HttpHeadersCoordinate; +use crate::sources::connect::validation::graphql::GraphQLString; +use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::validation::http::headers; use crate::sources::connect::ConnectSpecDefinition; use crate::subgraph::spec::CONTEXT_DIRECTIVE_NAME; @@ -70,7 +74,11 @@ use crate::subgraph::spec::INTF_OBJECT_DIRECTIVE_NAME; /// /// This function attempts to collect as many validation errors as possible, so it does not bail /// out as soon as it encounters one. -pub fn validate(schema: Schema) -> Vec { +pub fn validate(source_text: &str, file_name: &str) -> Vec { + // TODO: Use parse_and_validate (adding in directives as needed) + // TODO: Handle schema errors rather than relying on JavaScript to catch it later + let schema = Schema::parse(source_text, file_name) + .unwrap_or_else(|schema_with_errors| schema_with_errors.partial); let connect_identity = ConnectSpecDefinition::identity(); let Some((link, link_directive)) = Link::for_identity(&schema, &connect_identity) else { return Vec::new(); // There are no connectors-related directives to validate @@ -83,15 +91,22 @@ pub fn validate(schema: Schema) -> Vec { let mut messages = check_conflicting_directives(&schema); - let source_map = &schema.sources; let source_directive_name = ConnectSpecDefinition::source_directive_name(&link); let connect_directive_name = ConnectSpecDefinition::connect_directive_name(&link); + let lookup = LineColLookup::new(source_text); + let schema_info = SchemaInfo { + schema: &schema, + lookup, + connect_directive_name: &connect_directive_name, + source_directive_name: &source_directive_name, + }; + let source_directives: Vec = schema .schema_definition .directives .iter() .filter(|directive| directive.name == source_directive_name) - .map(|directive| validate_source(directive, source_map)) + .map(|directive| validate_source(directive, &schema_info)) .collect(); let mut valid_source_names = HashMap::new(); @@ -101,12 +116,17 @@ pub fn validate(schema: Schema) -> Vec { .collect_vec(); for directive in source_directives { messages.extend(directive.errors); - match directive.name.into_value_or_error(source_map) { + match directive.name.into_value_or_error(&schema_info.sources) { Err(error) => messages.push(error), Ok(name) => valid_source_names .entry(name) .or_insert_with(Vec::new) - .extend(directive.directive.node.line_column_range(source_map)), + .extend( + directive + .directive + .node + .line_column_range(&schema_info.sources), + ), } } for (name, locations) in valid_source_names { @@ -124,11 +144,8 @@ pub fn validate(schema: Schema) -> Vec { let connect_errors = schema.types.values().flat_map(|extended_type| { validate_extended_type( extended_type, - &schema, - &connect_directive_name, - &source_directive_name, + &schema_info, &all_source_names, - source_map, &mut seen_fields, ) }); @@ -136,10 +153,8 @@ pub fn validate(schema: Schema) -> Vec { if should_check_seen_fields(&messages) { messages.extend(check_seen_fields( - &schema, + &schema_info, &seen_fields, - source_map, - &connect_directive_name, &external_directive_name, )); } @@ -152,7 +167,7 @@ pub fn validate(schema: Schema) -> Vec { messages.push(Message { code: Code::NoSourceImport, message: format!("The `@{SOURCE_DIRECTIVE_NAME_IN_SPEC}` directive is not imported. Try adding `@{SOURCE_DIRECTIVE_NAME_IN_SPEC}` to `import` for `@{link_name}(url: \"{connect_identity}\")`", link_name=link_directive.name), - locations: link_directive.line_column_range(source_map) + locations: link_directive.line_column_range(&schema.sources) .into_iter() .collect(), }); @@ -179,10 +194,8 @@ fn should_check_seen_fields(messages: &[Message]) -> bool { /// Check that all fields defined in the schema are resolved by a connector. fn check_seen_fields( - schema: &Schema, + schema: &SchemaInfo, seen_fields: &IndexSet<(Name, Name)>, - source_map: &SourceMap, - connect_directive_name: &Name, external_directive_name: &Name, ) -> Vec { let all_fields: IndexSet<_> = schema @@ -235,8 +248,9 @@ fn check_seen_fields( code: Code::UnresolvedField, message: format!( "No connector resolves field `{parent_type}.{field_name}`. It must have a `@{connect_directive_name}` directive or appear in `@{connect_directive_name}(selection:)`.", + connect_directive_name = schema.connect_directive_name ), - locations: field_def.line_column_range(source_map).into_iter().collect(), + locations: field_def.line_column_range(&schema.sources).into_iter().collect(), } }).collect() } @@ -290,7 +304,7 @@ fn check_conflicting_directives(schema: &Schema) -> Vec { const DEFAULT_SOURCE_DIRECTIVE_NAME: &str = "connect__source"; const DEFAULT_CONNECT_DIRECTIVE_NAME: &str = "connect__connect"; -fn validate_source(directive: &Component, sources: &SourceMap) -> SourceDirective { +fn validate_source(directive: &Component, schema: &SchemaInfo) -> SourceDirective { let name = SourceName::from_directive(directive); let mut errors = Vec::new(); @@ -308,7 +322,7 @@ fn validate_source(directive: &Component, sources: &SourceMap) -> Sou BaseUrlCoordinate { source_directive_name: &directive.name, }, - sources, + schema, ) .err() { @@ -319,7 +333,7 @@ fn validate_source(directive: &Component, sources: &SourceMap) -> Sou errors.extend( headers::validate_arg( http_arg, - sources, + &schema.sources, HttpHeadersCoordinate::Source { directive_name: &directive.name, }, @@ -334,7 +348,10 @@ fn validate_source(directive: &Component, sources: &SourceMap) -> Sou "{coordinate} must have a `{HTTP_ARGUMENT_NAME}` argument.", coordinate = source_http_argument_coordinate(&directive.name), ), - locations: directive.line_column_range(sources).into_iter().collect(), + locations: directive + .line_column_range(&schema.sources) + .into_iter() + .collect(), }) } @@ -355,15 +372,25 @@ struct SourceDirective { fn parse_url( value: &Node, coordinate: Coordinate, - sources: &SourceMap, + schema: &SchemaInfo, ) -> Result<(), Message> { - let str_value = require_value_is_str(value, coordinate, sources)?; - let url = Url::parse(str_value).map_err(|inner| Message { + let str_value = GraphQLString::new(value, &schema.sources).ok_or_else(|| Message { + code: Code::GraphQLError, + message: format!("The value for {coordinate} must be a string."), + locations: value + .line_column_range(&schema.sources) + .into_iter() + .collect(), + })?; + let url = Url::parse(str_value.as_str()).map_err(|inner| Message { code: Code::InvalidUrl, message: format!("The value {value} for {coordinate} is not a valid URL: {inner}."), - locations: value.line_column_range(sources).into_iter().collect(), + locations: value + .line_column_range(&schema.sources) + .into_iter() + .collect(), })?; - http::url::validate_base_url(&url, coordinate, value, str_value, sources) + http::url::validate_base_url(&url, coordinate, value, str_value, schema) } fn require_value_is_str<'a, Coordinate: Display>( @@ -550,8 +577,7 @@ mod test_validate_source { insta::with_settings!({prepend_module_to_snapshot => false}, { glob!("test_data", "*.graphql", |path| { let schema = read_to_string(path).unwrap(); - let schema = Schema::parse(schema, path).unwrap(); - let errors = validate(schema); + let errors = validate(&schema, path.to_str().unwrap()); assert_snapshot!(format!("{:#?}", errors)); }); }); diff --git a/apollo-federation/src/sources/connect/validation/selection.rs b/apollo-federation/src/sources/connect/validation/selection.rs index 0f4f7d6115..17ba23fc92 100644 --- a/apollo-federation/src/sources/connect/validation/selection.rs +++ b/apollo-federation/src/sources/connect/validation/selection.rs @@ -24,6 +24,7 @@ use crate::sources::connect::expand::visitors::GroupVisitor; use crate::sources::connect::json_selection::NamedSelection; 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::SchemaInfo; use crate::sources::connect::JSONSelection; use crate::sources::connect::SubSelection; @@ -31,7 +32,7 @@ pub(super) fn validate_selection( field: &Component, connect_directive: &Node, parent_type: &Node, - schema: &Schema, + schema: &SchemaInfo, seen_fields: &mut IndexSet<(Name, Name)>, ) -> Option { let (selection_value, json_selection) = diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new new file mode 100644 index 0000000000..a2359c8435 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new @@ -0,0 +1,21 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 581 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/invalid-path-parameter.graphql +--- +[ + Message { + code: InvalidUrl, + message: "`GET` in `@connect(http:)` on `Query.resources` must be a valid URL template. Variable type must be one of $args, $this, $config, got $blah", + locations: [ + LineColumn { + line: 12, + column: 23, + }..LineColumn { + line: 12, + column: 30, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new new file mode 100644 index 0000000000..ecd5aba704 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new @@ -0,0 +1,21 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 519 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_entity_arg_field_arg_input_type_missing.graphql +--- +[ + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.product` must be a string.", + locations: [ + LineColumn { + line: 17, + column: 17, + }..LineColumn { + line: 17, + column: 148, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new new file mode 100644 index 0000000000..1255cbb7f8 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new @@ -0,0 +1,60 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 581 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_nested_paths_in_url_template_variables.graphql +--- +[ + Message { + code: UndefinedField, + message: "`GET` in `@connect(http:)` on `Query.scalar` contains `{$args.scalar.blah}`, but `String` does not have a field named `blah`.", + locations: [ + LineColumn { + line: 10, + column: 62, + }..LineColumn { + line: 13, + column: 10, + }, + ], + }, + Message { + code: UndefinedField, + message: "`GET` in `@connect(http:)` on `Query.object` contains `{$args.input.fieldThatDoesntExist}`, but `InputObject` does not have a field named `fieldThatDoesntExist`.", + locations: [ + LineColumn { + line: 15, + column: 61, + }..LineColumn { + line: 18, + column: 8, + }, + ], + }, + Message { + code: UndefinedField, + message: "`GET` in `@connect(http:)` on `Query.enum` contains `{$args.enum.cantHaveFields}`, but `Enum` does not have a field named `cantHaveFields`.", + locations: [ + LineColumn { + line: 20, + column: 58, + }..LineColumn { + line: 25, + column: 9, + }, + ], + }, + Message { + code: UndefinedField, + message: "`GET` in `@connect(http:)` on `Object.newField` contains `{$this.fieldThatDoesntExist}`, but Object does not have a field named `fieldThatDoesntExist`.", + locations: [ + LineColumn { + line: 29, + column: 55, + }..LineColumn { + line: 33, + column: 1, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new new file mode 100644 index 0000000000..14acaec7d3 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new @@ -0,0 +1,34 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 581 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/nullable_path_var_hint.graphql +--- +[ + Message { + code: NullablePathVariable, + message: "Variables in path parameters should be non-null, but `GET` in `@connect(http:)` on `Query.something` contains `{$args.isNull}` which is nullable. If a null value is provided at runtime, the request will fail.", + locations: [ + LineColumn { + line: 5, + column: 82, + }..LineColumn { + line: 5, + column: 114, + }, + ], + }, + Message { + code: NullablePathVariable, + message: "Variables in path parameters should be non-null, but `GET` in `@connect(http:)` on `Query.nestedNull` contains `{$args.nullabled.notNull}` which is nullable. If a null value is provided at runtime, the request will fail.", + locations: [ + LineColumn { + line: 6, + column: 88, + }..LineColumn { + line: 6, + column: 131, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new new file mode 100644 index 0000000000..2dbf024952 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new @@ -0,0 +1,21 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 519 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/valid_connect_absolute_url.graphql +--- +[ + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.resources` must be a string.", + locations: [ + LineColumn { + line: 6, + column: 22, + }..LineColumn { + line: 6, + column: 60, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new new file mode 100644 index 0000000000..325f89e3e3 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new @@ -0,0 +1,21 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 519 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_field_arg_key_multiple_keys.graphql +--- +[ + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.product` must be a string.", + locations: [ + LineColumn { + line: 17, + column: 17, + }..LineColumn { + line: 17, + column: 148, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new new file mode 100644 index 0000000000..70ea84289b --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new @@ -0,0 +1,21 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 519 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_field_arg_key_non_resolvable.graphql +--- +[ + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.product` must be a string.", + locations: [ + LineColumn { + line: 14, + column: 17, + }..LineColumn { + line: 14, + column: 68, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new new file mode 100644 index 0000000000..48b106927f --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new @@ -0,0 +1,34 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 519 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_field_arg_keys_disjoint.graphql +--- +[ + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.productById` must be a string.", + locations: [ + LineColumn { + line: 16, + column: 35, + }..LineColumn { + line: 16, + column: 62, + }, + ], + }, + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.productBySku` must be a string.", + locations: [ + LineColumn { + line: 18, + column: 35, + }..LineColumn { + line: 18, + column: 63, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new new file mode 100644 index 0000000000..a2c356dce1 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new @@ -0,0 +1,21 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +assertion_line: 519 +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_on_field.graphql +--- +[ + Message { + code: GraphQLError, + message: "The value for `GET` in `@connect(http:)` on `Query.user` must be a string.", + locations: [ + LineColumn { + line: 7, + column: 15, + }..LineColumn { + line: 7, + column: 64, + }, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/source_name.rs b/apollo-federation/src/sources/connect/validation/source_name.rs index e735d52040..9da9e86bdd 100644 --- a/apollo-federation/src/sources/connect/validation/source_name.rs +++ b/apollo-federation/src/sources/connect/validation/source_name.rs @@ -15,22 +15,21 @@ use super::Code; use super::DirectiveName; use super::Message; use crate::sources::connect::spec::schema::SOURCE_NAME_ARGUMENT_NAME; +use crate::sources::connect::validation::graphql::SchemaInfo; pub(super) fn validate_source_name_arg( field_name: &Name, object_name: &Name, source_name: &Node, - source_map: &SourceMap, source_names: &[SourceName], - source_directive_name: &Name, - connect_directive_name: &Name, + schema: &SchemaInfo, ) -> Vec { let mut messages = vec![]; if source_names.iter().all(|name| name != &source_name.value) { // TODO: Pick a suggestion that's not just the first defined source let qualified_directive = connect_directive_name_coordinate( - connect_directive_name, + schema.connect_directive_name, &source_name.value, object_name, field_name, @@ -41,7 +40,7 @@ pub(super) fn validate_source_name_arg( message: format!( "{qualified_directive} does not match any defined sources. Did you mean {first_source_name}?", ), - locations: source_name.line_column_range(source_map) + locations: source_name.line_column_range(&schema.sources) .into_iter() .collect(), }); @@ -50,9 +49,9 @@ pub(super) fn validate_source_name_arg( code: Code::NoSourcesDefined, message: format!( "{qualified_directive} specifies a source, but none are defined. Try adding {coordinate} to the schema.", - coordinate = source_name_value_coordinate(source_directive_name, &source_name.value), + coordinate = source_name_value_coordinate(schema.source_directive_name, &source_name.value), ), - locations: source_name.line_column_range(source_map) + locations: source_name.line_column_range(&schema.sources) .into_iter() .collect(), }); From cb5b491dc1050a285189a72e09d435a941d7fa67 Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Thu, 19 Sep 2024 17:34:41 -0600 Subject: [PATCH 3/6] Fix a test --- .../sources/connect/validation/graphql/strings.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/apollo-federation/src/sources/connect/validation/graphql/strings.rs b/apollo-federation/src/sources/connect/validation/graphql/strings.rs index ef36ffc45d..91f24e8339 100644 --- a/apollo-federation/src/sources/connect/validation/graphql/strings.rs +++ b/apollo-federation/src/sources/connect/validation/graphql/strings.rs @@ -139,24 +139,19 @@ mod tests { let string = GraphQLString::new(value, &schema.sources).unwrap(); assert_eq!( string.as_str(), - r#" - something + r#"something somethingElse { nested - } - "# + }"# ); let lookup = LineColLookup::new(SCHEMA); assert_eq!( string.line_col_for_subslice(8..16, &lookup), Some( LineColumn { - line: 4, - column: 12 - }..LineColumn { - line: 4, - column: 16 - } + line: 8, + column: 21 + }..LineColumn { line: 9, column: 7 } ) ); } From d39d99a5f50e07b2b757376fe05c79a3a9b0c120 Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Thu, 19 Sep 2024 17:43:05 -0600 Subject: [PATCH 4/6] Remove temp snapshots --- ...ts@invalid-path-parameter.graphql.snap.new | 21 ------- ...in_url_template_variables.graphql.snap.new | 60 ------------------- ...ts@nullable_path_var_hint.graphql.snap.new | 34 ----------- ...alid_connect_absolute_url.graphql.snap.new | 21 ------- ...eld_arg_key_multiple_keys.graphql.snap.new | 21 ------- ...ld_arg_key_non_resolvable.graphql.snap.new | 21 ------- ...g_field_arg_keys_disjoint.graphql.snap.new | 34 ----------- ...valid_entity_arg_on_field.graphql.snap.new | 21 ------- 8 files changed, 233 deletions(-) delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new deleted file mode 100644 index a2359c8435..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid-path-parameter.graphql.snap.new +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 581 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/invalid-path-parameter.graphql ---- -[ - Message { - code: InvalidUrl, - message: "`GET` in `@connect(http:)` on `Query.resources` must be a valid URL template. Variable type must be one of $args, $this, $config, got $blah", - locations: [ - LineColumn { - line: 12, - column: 23, - }..LineColumn { - line: 12, - column: 30, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new deleted file mode 100644 index 1255cbb7f8..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_url_template_variables.graphql.snap.new +++ /dev/null @@ -1,60 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 581 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_nested_paths_in_url_template_variables.graphql ---- -[ - Message { - code: UndefinedField, - message: "`GET` in `@connect(http:)` on `Query.scalar` contains `{$args.scalar.blah}`, but `String` does not have a field named `blah`.", - locations: [ - LineColumn { - line: 10, - column: 62, - }..LineColumn { - line: 13, - column: 10, - }, - ], - }, - Message { - code: UndefinedField, - message: "`GET` in `@connect(http:)` on `Query.object` contains `{$args.input.fieldThatDoesntExist}`, but `InputObject` does not have a field named `fieldThatDoesntExist`.", - locations: [ - LineColumn { - line: 15, - column: 61, - }..LineColumn { - line: 18, - column: 8, - }, - ], - }, - Message { - code: UndefinedField, - message: "`GET` in `@connect(http:)` on `Query.enum` contains `{$args.enum.cantHaveFields}`, but `Enum` does not have a field named `cantHaveFields`.", - locations: [ - LineColumn { - line: 20, - column: 58, - }..LineColumn { - line: 25, - column: 9, - }, - ], - }, - Message { - code: UndefinedField, - message: "`GET` in `@connect(http:)` on `Object.newField` contains `{$this.fieldThatDoesntExist}`, but Object does not have a field named `fieldThatDoesntExist`.", - locations: [ - LineColumn { - line: 29, - column: 55, - }..LineColumn { - line: 33, - column: 1, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new deleted file mode 100644 index 14acaec7d3..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@nullable_path_var_hint.graphql.snap.new +++ /dev/null @@ -1,34 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 581 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/nullable_path_var_hint.graphql ---- -[ - Message { - code: NullablePathVariable, - message: "Variables in path parameters should be non-null, but `GET` in `@connect(http:)` on `Query.something` contains `{$args.isNull}` which is nullable. If a null value is provided at runtime, the request will fail.", - locations: [ - LineColumn { - line: 5, - column: 82, - }..LineColumn { - line: 5, - column: 114, - }, - ], - }, - Message { - code: NullablePathVariable, - message: "Variables in path parameters should be non-null, but `GET` in `@connect(http:)` on `Query.nestedNull` contains `{$args.nullabled.notNull}` which is nullable. If a null value is provided at runtime, the request will fail.", - locations: [ - LineColumn { - line: 6, - column: 88, - }..LineColumn { - line: 6, - column: 131, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new deleted file mode 100644 index 2dbf024952..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_connect_absolute_url.graphql.snap.new +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 519 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/valid_connect_absolute_url.graphql ---- -[ - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.resources` must be a string.", - locations: [ - LineColumn { - line: 6, - column: 22, - }..LineColumn { - line: 6, - column: 60, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new deleted file mode 100644 index 325f89e3e3..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_multiple_keys.graphql.snap.new +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 519 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_field_arg_key_multiple_keys.graphql ---- -[ - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.product` must be a string.", - locations: [ - LineColumn { - line: 17, - column: 17, - }..LineColumn { - line: 17, - column: 148, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new deleted file mode 100644 index 70ea84289b..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_key_non_resolvable.graphql.snap.new +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 519 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_field_arg_key_non_resolvable.graphql ---- -[ - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.product` must be a string.", - locations: [ - LineColumn { - line: 14, - column: 17, - }..LineColumn { - line: 14, - column: 68, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new deleted file mode 100644 index 48b106927f..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_field_arg_keys_disjoint.graphql.snap.new +++ /dev/null @@ -1,34 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 519 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_field_arg_keys_disjoint.graphql ---- -[ - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.productById` must be a string.", - locations: [ - LineColumn { - line: 16, - column: 35, - }..LineColumn { - line: 16, - column: 62, - }, - ], - }, - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.productBySku` must be a string.", - locations: [ - LineColumn { - line: 18, - column: 35, - }..LineColumn { - line: 18, - column: 63, - }, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new deleted file mode 100644 index a2c356dce1..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_entity_arg_on_field.graphql.snap.new +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 519 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/valid_entity_arg_on_field.graphql ---- -[ - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.user` must be a string.", - locations: [ - LineColumn { - line: 7, - column: 15, - }..LineColumn { - line: 7, - column: 64, - }, - ], - }, -] From 9c839de313a40757c7c5ab4f08108837fad33767 Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Thu, 19 Sep 2024 17:45:05 -0600 Subject: [PATCH 5/6] Remove temp snapshots --- ...ld_arg_input_type_missing.graphql.snap.new | 21 ------------------- 1 file changed, 21 deletions(-) delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new deleted file mode 100644 index ecd5aba704..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_entity_arg_field_arg_input_type_missing.graphql.snap.new +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -assertion_line: 519 -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_entity_arg_field_arg_input_type_missing.graphql ---- -[ - Message { - code: GraphQLError, - message: "The value for `GET` in `@connect(http:)` on `Query.product` must be a string.", - locations: [ - LineColumn { - line: 17, - column: 17, - }..LineColumn { - line: 17, - column: 148, - }, - ], - }, -] From 7ead3e93a1482c81b449b49d028ca7ed7b599ae9 Mon Sep 17 00:00:00 2001 From: Matthew Hawkins Date: Wed, 25 Sep 2024 16:12:47 -0600 Subject: [PATCH 6/6] Suggestions from PR --- .../src/sources/connect/validation/graphql.rs | 54 ++++++++++++++++++- .../connect/validation/graphql/strings.rs | 48 +++++++++-------- .../sources/connect/validation/http/url.rs | 31 ++++++----- .../src/sources/connect/validation/mod.rs | 16 +++--- 4 files changed, 102 insertions(+), 47 deletions(-) diff --git a/apollo-federation/src/sources/connect/validation/graphql.rs b/apollo-federation/src/sources/connect/validation/graphql.rs index e566be9057..b5b202c6dc 100644 --- a/apollo-federation/src/sources/connect/validation/graphql.rs +++ b/apollo-federation/src/sources/connect/validation/graphql.rs @@ -11,11 +11,41 @@ pub(super) use strings::GraphQLString; pub(super) struct SchemaInfo<'schema> { pub(crate) schema: &'schema Schema, - pub(crate) lookup: LineColLookup<'schema>, + len: usize, + lookup: LineColLookup<'schema>, pub(crate) connect_directive_name: &'schema Name, pub(crate) source_directive_name: &'schema Name, } +impl<'schema> SchemaInfo<'schema> { + pub(crate) fn new( + schema: &'schema Schema, + src: &'schema str, + connect_directive_name: &'schema Name, + source_directive_name: &'schema Name, + ) -> Self { + Self { + schema, + len: src.len(), + lookup: LineColLookup::new(src), + connect_directive_name, + source_directive_name, + } + } + + /// Get the 1-based line and column values for an offset into this schema. + /// + /// # Returns + /// The line and column, or `None` if the offset is not within the schema. + pub(crate) fn line_col(&self, offset: usize) -> Option<(usize, usize)> { + if offset > self.len { + None + } else { + Some(self.lookup.get(offset)) + } + } +} + impl Deref for SchemaInfo<'_> { type Target = Schema; @@ -23,3 +53,25 @@ impl Deref for SchemaInfo<'_> { self.schema } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn line_col_lookup() { + let src = r#" + type Query { + foo: String + } + "#; + let schema = Schema::parse(src, "testSchema").unwrap(); + + let name = "unused".try_into().unwrap(); + let schema_info = SchemaInfo::new(&schema, src, &name, &name); + + assert_eq!(schema_info.line_col(0), Some((1, 1))); + assert_eq!(schema_info.line_col(4), Some((2, 4))); + assert_eq!(schema_info.line_col(200), None); + } +} diff --git a/apollo-federation/src/sources/connect/validation/graphql/strings.rs b/apollo-federation/src/sources/connect/validation/graphql/strings.rs index 91f24e8339..863bfe5489 100644 --- a/apollo-federation/src/sources/connect/validation/graphql/strings.rs +++ b/apollo-federation/src/sources/connect/validation/graphql/strings.rs @@ -12,7 +12,8 @@ use apollo_compiler::ast::Value; use apollo_compiler::parser::LineColumn; use apollo_compiler::parser::SourceMap; use apollo_compiler::Node; -use line_col::LineColLookup; + +use crate::sources::connect::validation::graphql::SchemaInfo; #[derive(Clone, Copy)] pub(crate) struct GraphQLString<'schema> { @@ -26,25 +27,28 @@ impl<'schema> GraphQLString<'schema> { /// Get the raw string value of this GraphQL string literal /// /// Returns `None` if the value was not a string literal or if location data is messed up - pub fn new(value: &'schema Node, sources: &'schema SourceMap) -> Option { - let value_without_quotes = value.as_str()?; - let source_span = value.location()?; - let file = sources.get(&source_span.file_id())?; + pub fn new(value: &'schema Node, sources: &'schema SourceMap) -> Result { + let value_without_quotes = value.as_str().ok_or(())?; + let source_span = value.location().ok_or(())?; + let file = sources.get(&source_span.file_id()).ok_or(())?; let source_text = file.source_text(); let start_of_quotes = source_span.offset(); let end_of_quotes = source_span.end_offset(); - let value_with_quotes = source_text.get(start_of_quotes..end_of_quotes)?; + let value_with_quotes = source_text.get(start_of_quotes..end_of_quotes).ok_or(())?; // On each line, the whitespace gets messed up for multi-line strings // So we find the first and last lines of the parsed value (no whitespace) within // the raw value (with whitespace) to get our raw string. - let first_line_of_value = value_without_quotes.lines().next()?; - let start_of_value = value_with_quotes.find(first_line_of_value)?; - let last_line_of_value = value_without_quotes.lines().last()?; - let end_of_value = value_with_quotes.rfind(last_line_of_value)? + last_line_of_value.len(); - let raw_string = value_with_quotes.get(start_of_value..end_of_value)?; - - Some(Self { + let first_line_of_value = value_without_quotes.lines().next().ok_or(())?; + let start_of_value = value_with_quotes.find(first_line_of_value).ok_or(())?; + let last_line_of_value = value_without_quotes.lines().last().ok_or(())?; + let end_of_value = + value_with_quotes.rfind(last_line_of_value).ok_or(())? + last_line_of_value.len(); + let raw_string = value_with_quotes + .get(start_of_value..end_of_value) + .ok_or(())?; + + Ok(Self { raw_string, offset: start_of_quotes + start_of_value, }) @@ -57,14 +61,14 @@ impl<'schema> GraphQLString<'schema> { pub fn line_col_for_subslice( &self, substring_location: Range, - lookup: &LineColLookup, + schema_info: &SchemaInfo, ) -> Option> { let start_offset = self.offset + substring_location.start; let end_offset = self.offset + substring_location.end; - let (line, column) = lookup.get(start_offset); + let (line, column) = schema_info.line_col(start_offset)?; let start = LineColumn { line, column }; - let (line, column) = lookup.get(end_offset); + let (line, column) = schema_info.line_col(end_offset)?; let end = LineColumn { line, column }; Some(start..end) @@ -78,10 +82,10 @@ mod tests { use apollo_compiler::schema::ExtendedType; use apollo_compiler::Node; use apollo_compiler::Schema; - use line_col::LineColLookup; use pretty_assertions::assert_eq; use crate::sources::connect::validation::graphql::GraphQLString; + use crate::sources::connect::validation::graphql::SchemaInfo; const SCHEMA: &str = r#" type Query { @@ -116,9 +120,10 @@ mod tests { let string = GraphQLString::new(value, &schema.sources).unwrap(); assert_eq!(string.as_str(), "https://example.com"); - let lookup = LineColLookup::new(SCHEMA); + let name = "unused".try_into().unwrap(); + let schema_info = SchemaInfo::new(&schema, SCHEMA, &name, &name); assert_eq!( - string.line_col_for_subslice(2..5, &lookup), + string.line_col_for_subslice(2..5, &schema_info), Some( LineColumn { line: 5, @@ -144,9 +149,10 @@ mod tests { nested }"# ); - let lookup = LineColLookup::new(SCHEMA); + let name = "unused".try_into().unwrap(); + let schema_info = SchemaInfo::new(&schema, SCHEMA, &name, &name); assert_eq!( - string.line_col_for_subslice(8..16, &lookup), + string.line_col_for_subslice(8..16, &schema_info), Some( LineColumn { line: 8, diff --git a/apollo-federation/src/sources/connect/validation/http/url.rs b/apollo-federation/src/sources/connect/validation/http/url.rs index cae37f4145..68e9683d29 100644 --- a/apollo-federation/src/sources/connect/validation/http/url.rs +++ b/apollo-federation/src/sources/connect/validation/http/url.rs @@ -46,7 +46,7 @@ pub(crate) fn validate_template( ), locations: str_value.line_col_for_subslice( variable.location.clone(), - &schema.lookup, + schema, ).into_iter().collect(), }); } @@ -71,22 +71,21 @@ fn parse_template<'schema>( coordinate: HttpMethodCoordinate<'schema>, schema: &'schema SchemaInfo, ) -> Result<(URLTemplate, GraphQLString<'schema>), Message> { - let str_value = - GraphQLString::new(coordinate.node, &schema.sources).ok_or_else(|| Message { - code: Code::GraphQLError, - message: format!("The value for {coordinate} must be a string."), - locations: coordinate - .node - .line_column_range(&schema.sources) - .into_iter() - .collect(), - })?; + let str_value = GraphQLString::new(coordinate.node, &schema.sources).map_err(|_| Message { + code: Code::GraphQLError, + message: format!("The value for {coordinate} must be a string."), + locations: coordinate + .node + .line_column_range(&schema.sources) + .into_iter() + .collect(), + })?; let template = URLTemplate::from_str(str_value.as_str()).map_err( |url_template::Error { message, location }| Message { code: Code::InvalidUrl, message: format!("{coordinate} must be a valid URL template. {message}"), locations: location - .and_then(|location| str_value.line_col_for_subslice(location, &schema.lookup)) + .and_then(|location| str_value.line_col_for_subslice(location, schema)) .into_iter() .collect(), }, @@ -110,7 +109,7 @@ pub(crate) fn validate_base_url( "The value {value} for {coordinate} must be http or https, got {scheme}.", ), locations: str_value - .line_col_for_subslice(scheme_location, &schema.lookup) + .line_col_for_subslice(scheme_location, schema) .into_iter() .collect(), }) @@ -144,7 +143,7 @@ fn validate_variable<'schema>( ), locations: url_value.line_col_for_subslice( path_component_start..path_component_end, - &schema.lookup, + schema, ).into_iter().collect() } }).map(|arg| arg.ty.as_ref().clone())? @@ -158,7 +157,7 @@ fn validate_variable<'schema>( ), locations: url_value.line_col_for_subslice( path_component_start..path_component_end, - &schema.lookup, + schema, ).into_iter().collect() }).map(|field| field.ty.clone())? } @@ -197,7 +196,7 @@ fn validate_variable<'schema>( ), locations: url_value.line_col_for_subslice( path_component_start..path_component_end, - &schema.lookup, + schema, ).into_iter().collect(), }) })?.clone(); diff --git a/apollo-federation/src/sources/connect/validation/mod.rs b/apollo-federation/src/sources/connect/validation/mod.rs index 3d269886a8..9864439e53 100644 --- a/apollo-federation/src/sources/connect/validation/mod.rs +++ b/apollo-federation/src/sources/connect/validation/mod.rs @@ -45,7 +45,6 @@ use apollo_compiler::Schema; use coordinates::source_http_argument_coordinate; use extended_type::validate_extended_type; use itertools::Itertools; -use line_col::LineColLookup; use source_name::SourceName; use url::Url; @@ -93,13 +92,12 @@ pub fn validate(source_text: &str, file_name: &str) -> Vec { let source_directive_name = ConnectSpecDefinition::source_directive_name(&link); let connect_directive_name = ConnectSpecDefinition::connect_directive_name(&link); - let lookup = LineColLookup::new(source_text); - let schema_info = SchemaInfo { - schema: &schema, - lookup, - connect_directive_name: &connect_directive_name, - source_directive_name: &source_directive_name, - }; + let schema_info = SchemaInfo::new( + &schema, + source_text, + &connect_directive_name, + &source_directive_name, + ); let source_directives: Vec = schema .schema_definition @@ -374,7 +372,7 @@ fn parse_url( coordinate: Coordinate, schema: &SchemaInfo, ) -> Result<(), Message> { - let str_value = GraphQLString::new(value, &schema.sources).ok_or_else(|| Message { + let str_value = GraphQLString::new(value, &schema.sources).map_err(|_| Message { code: Code::GraphQLError, message: format!("The value for {coordinate} must be a string."), locations: value