Improve error messages for expressions

apollo-federation/src/sources/connect/validation/coordinates.rs

@@ -135,7 +135,7 @@ impl Display for HttpMethodCoordinate<'_> {
         } = self;
         write!(
             f,
-            "`{http_method}` in `@{connect_directive_name}({HTTP_ARGUMENT_NAME}:)` on {field_coordinate}",
+            "`@{connect_directive_name}({HTTP_ARGUMENT_NAME}.{http_method}:)` on {field_coordinate}",
             connect_directive_name = directive.name,
         )
     }

apollo-federation/src/sources/connect/validation/expression.rs

@@ -85,36 +85,26 @@ pub(crate) fn validate(expression: &Expression, context: &Context) -> Result<(),
         location,
     } = expression;
     let shape = expression.shape();
-    let errors: Vec<Error> = shape
-        .errors()
-        .map(|err| Error {
-            message: err.message.clone(),
-            location: err
-                .range
-                .as_ref()
-                .map(|range| range.start + location.start..range.end + location.start)
-                .unwrap_or_else(|| location.clone()),
-        })
-        .collect();
-    if !errors.is_empty() {
-        return Err(errors);
-    }
 
     validate_shape(&shape, context).map_err(|message| {
         vec![Error {
-            message,
+            message: format!("`{{{expression}}}` {message}"),
             location: location.clone(),
         }]
     })
 }
 
 /// Validate that the shape is an acceptable output shape for an Expression.
 ///
+/// Error messages produced here will eventually look like "In {coordinate}: {expression} {message}"
+///
 /// TODO: Some day, whether objects or arrays are allowed will be dependent on &self (i.e., is the * modifier used)
 fn validate_shape(shape: &Shape, context: &Context) -> Result<(), String> {
     match shape.case() {
-        ShapeCase::Array { .. } => Err("array values aren't valid here".to_string()),
-        ShapeCase::Object { .. } => Err("object values aren't valid here".to_string()),
+        ShapeCase::Array { .. } => Err("evaluates to an array, which isn't valid here".to_string()),
+        ShapeCase::Object { .. } => {
+            Err("evaluates to an object, which isn't valid here".to_string())
+        }
         ShapeCase::One(shapes) | ShapeCase::All(shapes) => {
             for shape in shapes {
                 validate_shape(shape, context)?;
@@ -124,13 +114,13 @@ fn validate_shape(shape: &Shape, context: &Context) -> Result<(), String> {
         ShapeCase::Name(name, key) => {
             let mut shape = if name == "$root" {
                 return Err(format!(
-                    "`{key}` must start with an argument name, like `$this` or `$args`",
-                    key = key.iter().map(|key| key.to_string()).join(".")
+                    "must start with one of {namespaces}",
+                    namespaces = context.var_lookup.keys().map(|ns| ns.as_str()).join(", ")
                 ));
             } else if name.starts_with('$') {
                 let namespace = Namespace::from_str(name).map_err(|_| {
                     format!(
-                        "unknown variable `{name}`, must be one of {namespaces}",
+                        "uses invalid variable `{name}`, must be one of {namespaces}",
                         namespaces = context.var_lookup.keys().map(|ns| ns.as_str()).join(", ")
                     )
                 })?;
@@ -139,7 +129,7 @@ fn validate_shape(shape: &Shape, context: &Context) -> Result<(), String> {
                     .get(&namespace)
                     .ok_or_else(|| {
                         format!(
-                            "{namespace} is not valid here, must be one of {namespaces}",
+                            "uses invalid variable `{namespace}`, must be one of {namespaces}",
                             namespaces = context.var_lookup.keys().map(|ns| ns.as_str()).join(", "),
                         )
                     })?
@@ -150,13 +140,13 @@ fn validate_shape(shape: &Shape, context: &Context) -> Result<(), String> {
                     .shape_lookup
                     .get(name.as_str())
                     .cloned()
-                    .ok_or_else(|| format!("unknown type `{name}`"))?
+                    .ok_or_else(|| format!("uses unknown type `{name}`"))?
             };
             let mut path = name.clone();
             for key in key {
                 let child = shape.child(key);
                 if child.is_none() {
-                    return Err(format!("`{path}` doesn't have a field named `{key}`"));
+                    return Err(format!("references unknown field `{key}` on `{path}`"));
                 }
                 shape = child;
                 path = format!("{path}.{key}");

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@headers__expressions_that_evaluate_to_invalid_types.graphql.snap

@@ -1,33 +1,33 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/headers/expressions_that_evaluate_to_invalid_types.graphql
 ---
 [
     Message {
         code: InvalidHeader,
-        message: "In `@source(http.headers:)`: array values aren't valid here",
+        message: "In `@source(http.headers:)`: `{$->echo([])}` evaluates to an array, which isn't valid here",
         locations: [
             11:42..11:53,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@source(http.headers:)`: object values aren't valid here",
+        message: "In `@source(http.headers:)`: `{$({})}` evaluates to an object, which isn't valid here",
         locations: [
             12:43..12:48,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@source(http.headers:)`: object values aren't valid here",
+        message: "In `@source(http.headers:)`: `{}` evaluates to an object, which isn't valid here",
         locations: [
             13:73..13:73,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.blah`: array values aren't valid here",
+        message: "In `@connect(http.headers:)` on `Query.blah`: `{$args.anArray}` evaluates to an array, which isn't valid here",
         locations: [
             21:67..21:80,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@headers__invalid_namespace_in_header_variables.graphql.snap

@@ -1,6 +1,6 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/headers/invalid_namespace_in_header_variables.graphql
 ---
 [
@@ -13,14 +13,14 @@ input_file: apollo-federation/src/sources/connect/validation/test_data/headers/i
     },
     Message {
         code: InvalidHeader,
-        message: "In `@source(http.headers:)`: $this is not valid here, must be one of $config, $context",
+        message: "In `@source(http.headers:)`: `{$this.bar}` uses invalid variable `$this`, must be one of $config, $context",
         locations: [
             12:62..12:71,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@source(http.headers:)`: `config.bar` must start with an argument name, like `$this` or `$args`",
+        message: "In `@source(http.headers:)`: `{config.bar}` must start with one of $config, $context",
         locations: [
             13:56..13:66,
         ],
@@ -34,21 +34,21 @@ input_file: apollo-federation/src/sources/connect/validation/test_data/headers/i
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.scalar`: $status is not valid here, must be one of $args, $config, $context",
+        message: "In `@connect(http.headers:)` on `Query.scalar`: `{$status}` uses invalid variable `$status`, must be one of $args, $config, $context",
         locations: [
             25:62..25:69,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.scalar`: $this is not valid here, must be one of $args, $config, $context",
+        message: "In `@connect(http.headers:)` on `Query.scalar`: `{$this}` uses invalid variable `$this`, must be one of $args, $config, $context",
         locations: [
             26:47..26:52,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.scalar`: `config.bar` must start with an argument name, like `$this` or `$args`",
+        message: "In `@connect(http.headers:)` on `Query.scalar`: `{config.bar}` must start with one of $args, $config, $context",
         locations: [
             27:56..27:66,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@headers__invalid_nested_paths_in_header_variables.graphql.snap

@@ -1,33 +1,33 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/headers/invalid_nested_paths_in_header_variables.graphql
 ---
 [
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.scalar`: `$args.scalar` doesn't have a field named `blah`",
+        message: "In `@connect(http.headers:)` on `Query.scalar`: `{$args.scalar.blah}` references unknown field `blah` on `$args.scalar`",
         locations: [
             13:47..13:64,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.object`: `InputObject` doesn't have a field named `fieldThatDoesntExist`",
+        message: "In `@connect(http.headers:)` on `Query.object`: `{$args.input.fieldThatDoesntExist}` references unknown field `fieldThatDoesntExist` on `InputObject`",
         locations: [
             23:47..23:79,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Query.enum`: `Enum` doesn't have a field named `cantHaveFields`",
+        message: "In `@connect(http.headers:)` on `Query.enum`: `{$args.enum.cantHaveFields}` references unknown field `cantHaveFields` on `Enum`",
         locations: [
             33:45..33:70,
         ],
     },
     Message {
         code: InvalidHeader,
-        message: "In `@connect(http.headers:)` on `Object.newField`: `$this` doesn't have a field named `fieldThatDoesntExist`",
+        message: "In `@connect(http.headers:)` on `Object.newField`: `{$this.fieldThatDoesntExist}` references unknown field `fieldThatDoesntExist` on `$this`",
         locations: [
             47:47..47:73,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__absolute_connect_url_with_source.graphql.snap

@@ -1,12 +1,12 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templates/absolute_connect_url_with_source.graphql
 ---
 [
     Message {
         code: AbsoluteConnectUrlWithSource,
-        message: "`GET` in `@connect(http:)` on `Query.resources` contains the absolute URL \"http://127.0.0.1/resources\" while also specifying a `source`. Either remove the `source` argument or change the URL to a path.",
+        message: "`@connect(http.GET:)` on `Query.resources` contains the absolute URL \"http://127.0.0.1/resources\" while also specifying a `source`. Either remove the `source` argument or change the URL to a path.",
         locations: [
             12:20..12:48,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__invalid-jsonselection-in-expression.graphql.snap

@@ -6,7 +6,7 @@ input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templ
 [
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.resources`: nom::error::ErrorKind::Eof",
+        message: "In `@connect(http.GET:)` on `Query.resources`: nom::error::ErrorKind::Eof",
         locations: [
             12:27..12:28,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__invalid-path-parameter.graphql.snap

@@ -1,12 +1,12 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templates/invalid-path-parameter.graphql
 ---
 [
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.resources`: Unknown variable",
+        message: "In `@connect(http.GET:)` on `Query.resources`: Unknown variable",
         locations: [
             12:23..12:28,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__invalid_connect_url.graphql.snap

@@ -1,12 +1,12 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templates/invalid_connect_url.graphql
 ---
 [
     Message {
         code: RelativeConnectUrlWithoutSource,
-        message: "`GET` in `@connect(http:)` on `Query.resources` specifies the relative URL \"127.0.0.1\", but no `source` is defined. Either use an absolute URL including scheme (e.g. https://), or add a `@connect__source`.",
+        message: "`@connect(http.GET:)` on `Query.resources` specifies the relative URL \"127.0.0.1\", but no `source` is defined. Either use an absolute URL including scheme (e.g. https://), or add a `@connect__source`.",
         locations: [
             5:47..5:58,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__invalid_connect_url_scheme.graphql.snap

@@ -1,12 +1,12 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templates/invalid_connect_url_scheme.graphql
 ---
 [
     Message {
         code: InvalidUrlScheme,
-        message: "The value \"file://data.json\" for `GET` in `@connect(http:)` on `Query.resources` must be http or https, got file.",
+        message: "The value \"file://data.json\" for `@connect(http.GET:)` on `Query.resources` must be http or https, got file.",
         locations: [
             6:28..6:32,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__invalid_namespace_in_url_template_variables.graphql.snap

@@ -1,26 +1,26 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templates/invalid_namespace_in_url_template_variables.graphql
 ---
 [
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.unknown`: Unknown variable",
+        message: "In `@connect(http.GET:)` on `Query.unknown`: Unknown variable",
         locations: [
             11:31..11:39,
         ],
     },
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.invalid`: $status is not valid here, must be one of $args, $config, $context",
+        message: "In `@connect(http.GET:)` on `Query.invalid`: `{$status.bar}` uses invalid variable `$status`, must be one of $args, $config, $context",
         locations: [
             18:31..18:42,
         ],
     },
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.nodollar`: `config.bar` must start with an argument name, like `$this` or `$args`",
+        message: "In `@connect(http.GET:)` on `Query.nodollar`: `{config.bar}` must start with one of $args, $config, $context",
         locations: [
             25:31..25:41,
         ],

apollo-federation/src/sources/connect/validation/snapshots/validation_tests@uri_templates__invalid_nested_paths_in_url_template_variables.graphql.snap

@@ -1,33 +1,33 @@
 ---
 source: apollo-federation/src/sources/connect/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+expression: "format!(\"{:#?}\", result.errors)"
 input_file: apollo-federation/src/sources/connect/validation/test_data/uri_templates/invalid_nested_paths_in_url_template_variables.graphql
 ---
 [
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.scalar`: `$args.scalar` doesn't have a field named `blah`",
+        message: "In `@connect(http.GET:)` on `Query.scalar`: `{$args.scalar.blah}` references unknown field `blah` on `$args.scalar`",
         locations: [
             10:49..10:66,
         ],
     },
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.object`: `InputObject` doesn't have a field named `fieldThatDoesntExist`",
+        message: "In `@connect(http.GET:)` on `Query.object`: `{$args.input.fieldThatDoesntExist}` references unknown field `fieldThatDoesntExist` on `InputObject`",
         locations: [
             15:49..15:81,
         ],
     },
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.enum`: `Enum` doesn't have a field named `cantHaveFields`",
+        message: "In `@connect(http.GET:)` on `Query.enum`: `{$args.enum.cantHaveFields}` references unknown field `cantHaveFields` on `Enum`",
         locations: [
             20:47..20:72,
         ],
     },
     Message {
         code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Object.newField`: `$this` doesn't have a field named `fieldThatDoesntExist`",
+        message: "In `@connect(http.GET:)` on `Object.newField`: `{$this.fieldThatDoesntExist}` references unknown field `fieldThatDoesntExist` on `$this`",
         locations: [
             29:49..29:75,
         ],