Update usage of `shape` crate for 0.7.0-preview.n releases #8616

benjamn · 2025-11-14T19:51:19Z

This PR cherry-picks the crucial [email protected] version update from PR #8143 as a standalone commit (note: previously known as [email protected]), before introducing any abstract types-related changes, which should make these Shape-related updates much easier to review.

This update benefits all ConnectSpec versions, while still allowing us to preserve backwards/forwards compatibility for older versions (connect/v0.3 and earlier). It would be a shame to have to maintain two slightly different shape APIs (colliding on the same cargo package name) in order to keep connect/v0.3 and earlier working as before. Fortunately, we do not need to maintain a fork like that at the level of the shape library.

One of the goals of this commit is to show how limited/reasonable the updates are, because the new functionality coming in [email protected] does not radically alter the behavior of the library (at least not in the ways we were previously using it). Notable (expected) changes: we've started using a shape.locations() method instead of accessing/mutating the shape.locations vector directly, the new shape name tracking system allows better error messages, and shape pretty-printing may now include newlines/indentation for readability.

If this PR looks good and it meets the needs of the Router connectors implementation, I hope it can lend general confidence/support to the [email protected] minor version update, even though I know most of the team has not had a chance to review the upstream changes in detail.

github-actions · 2025-11-14T19:51:29Z

@benjamn, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

dylan-apollo · 2025-11-14T20:32:38Z

apollo-federation/src/connectors/json_selection/methods/public/entries.rs

            {
-                input_shape
-                    .locations
-                    .extend(method_name.shape_location(context.source_id()));
-                input_shape.locations
+                let mut locations = input_shape.locations().cloned().collect::<IndexSet<_>>();
+                locations.extend(method_name.shape_location(context.source_id()));
+                locations.into_iter()
            },


Suggested change

{

input_shape

.locations

.extend(method_name.shape_location(context.source_id()));

input_shape.locations

let mut locations = input_shape.locations().cloned().collect::<IndexSet<_>>();

locations.extend(method_name.shape_location(context.source_id()));

locations.into_iter()

},

input_shape

.locations()

.cloned()

.chain(method_name.shape_location(context.source_id())),

Saves 1 or 2 allocations. Unless you needed the deduplication from the IndexSet

dylan-apollo · 2025-11-14T20:34:08Z

apollo-federation/src/connectors/json_selection/methods/public/filter.rs

@@ -162,7 +162,7 @@ fn filter_shape(
        );
    }

-    Shape::list(input_shape.any_item([]), input_shape.locations)


Unfortunate that we have to do all this extra cloning now since it's behind a &self method. Why was the change needed?

The internal representation of metadata (including locations) is completely different now. Directly accessing the vector struct field is not an option any more.

That said, I would very much like to make all Shape::helper methods take a sequence of locations: impl IntoIter<Item = &Location> (and clone them if they need to be stored) rather than Item = Location. I think that would help prevent cloning, and it matches what shape.locations() returns.

From my scan of the changes, most of the places we were using it we were cloning anyway. So saving the couple of places we didn't need to is not worth stressing over

dylan-apollo · 2025-11-14T20:35:49Z

apollo-federation/src/connectors/json_selection/methods/public/size.rs

            {
-                input_shape
-                    .locations
-                    .extend(method_name.shape_location(context.source_id()));
-                input_shape.locations
+                let mut locations = input_shape.locations().cloned().collect::<IndexSet<_>>();
+                locations.extend(method_name.shape_location(context.source_id()));
+                locations.into_iter()
            },


Suggested change

{

input_shape

.locations

.extend(method_name.shape_location(context.source_id()));

input_shape.locations

let mut locations = input_shape.locations().cloned().collect::<IndexSet<_>>();

locations.extend(method_name.shape_location(context.source_id()));

locations.into_iter()

},

input_shape

.locations()

.cloned()

.chain(method_name.shape_location(context.source_id())),

dylan-apollo · 2025-11-14T20:37:52Z

apollo-federation/src/connectors/validation/snapshots/[email protected]

@@ -6,7 +6,7 @@ input_file: apollo-federation/src/connectors/validation/test_data/env-vars.graph
 [
    Message {
        code: InvalidHeader,
-        message: "In `@source(http.headers:)`: object values aren't valid here",
+        message: "In `@source(http.headers:)`: expected `One<Float, Bool, String, null, None>` rejects `Dict<String>`",


I worry this message is less actionable for the user, since they don't know (though might intuit) what One<> and rejects means.

Maybe, but it's much better information about what went wrong, once you get a little bit familiar with it. Saying nothing about the expected shape (and very little about the received shape) was unhelpful before.

We can workshop how we display these errors later, but I don't want to go back to an opaque error out of some vague fear of confusing beginners (with accurate information they do genuinely need).

I like that this gives more information so I think that is more helpful... I suspect it's just the wording that could be improved here.

Is there an option to tweak the wording so it's something like this?

Got unexpected 'Dict<String>', expected one of `Float, Bool, String, null, None`

Ok, thanks for the feedback on this!

If you have a look at aeb5a9b, I think I've taken the main line of the error message back to the previous style (adding a short name for the expected shape), with a \n-separated second line that gives the full Details: ... (but without the extra explain_mismatch logic, just the pretty-printed expected/received shapes).

I think this is at least as useful as the previous error message now, and potentially much more useful, especially for LLMs trying to understand the errors.

...on/snapshots/validation_tests@headers__invalid_nested_paths_in_header_variables.graphql.snap

apollo-federation/src/connectors/validation/expression.rs

andrewmcgivery · 2025-11-14T22:22:49Z

apollo-federation/src/connectors/json_selection/methods/public/as.rs

@@ -323,12 +323,12 @@ mod tests {

        assert_eq!(
            selection!("person->as", spec).shape().pretty_print(),
-            "Error<\"Method ->as requires one or two arguments (got 0)\", $root.person>",
+            "Error<\n  \"Method ->as requires one or two arguments (got 0)\",\n  $root.person,\n>",


Confused by these changes... are the \n line breaks in the error message?

This is how we pretty-print a ShapeCase::Error (and yes, [email protected] introduced pretty printing with line breaks for overlong lines), but it's not necessarily what we would put into a higher-level error message. In practice, we'd iterate over shape.errors() and use the error.message string from each one (like "Method ->as requires one or two arguments (got 0)"), so the \ns you see here wouldn't matter.

andrewmcgivery · 2025-11-14T22:26:41Z

apollo-federation/src/connectors/json_selection/apply_to.rs

@@ -1315,7 +1315,7 @@ impl ApplyToInternal for SubSelection {

        // Build up the merged object shape using Shape::all to merge the
        // individual named_selection object shapes.
-        let mut all_shape = Shape::none();
+        let mut all_shape = Shape::unknown([]);


This seems like a pretty major difference to go from none to unknown. 😅

Is the comment above it still accurate?

Mostly just flagging this as a "I don't know enough about what is going on here to say this is fine or not fine... but being cautious" haha

Good question/caution!

To explain this change specifically, all_shape serves as an empty starting shape that gets repeatedly merged with the output of each named selection (typically an object, even for single properties), ultimately generating a single object shape (or union of object shapes) representing all their contributions.

The Shape::unknown([]) shape works well for this, given the way it disappears from intersections (All<Unknown, T> => T, or All<Unknown, T, U> => All<T, U>), which is why this change is safe.

Previously I was using Shape::none() because I mistakenly thought it had the same behavior as Shape::unknown([]). Specifically, I thought None should be automatically dropped from All<None, ...> intersections during simplification. If that was true/appropriate, then Shape::none() would behave basically like Shape::unknown([]) does (for this code's needs). Since then, I've realized None should not disappear from intersections, since it provides important information, often making the intersection unsatisfiable, which is noteworthy.

So, long story short, we're now using Unknown because it always worked, and not using None because it only seemed to work before I fixed a bug.

andrewmcgivery · 2025-11-14T22:30:05Z

...c/connectors/validation/snapshots/validation_tests@uri_templates__invalid_types.graphql.snap

@@ -6,16 +6,15 @@ input_file: apollo-federation/src/connectors/validation/test_data/uri_templates/
 [
    Message {
        code: InvalidUrl,
-        message: "In `GET` in `@connect(http:)` on `Query.argIsArray`: array values aren't valid here",
+        message: "In `GET` in `@connect(http:)` on `Query.argIsArray`: expected `One<Float, Bool, String, null, None>` rejects `One<List<One<String, null>>, null>` (because `One<Float, Bool, String, null, None>` rejects `List<One<String, null>>`)",


I'm very confused by this message 😆 This again is probably a wording problem more than anything but this is breaking my brain trying to read it lol

Here's the new format for this particular error (after aeb5a9b):

In `GET` in `@connect(http:)` on `Query.argIsArray`: expected union but received incompatible union Details: `One<Float, Bool, String, null, None>` does not accept `One<List<One<String, null>>, null>`

Note that using (just) the short name union leads to a collision here that makes the error nearly useless without the extra Details: ... line. To anticipate collisions like this, I put in the word "incompatible" so it's clear the two things are probably different/incompatible even though they happen to have the same short name. However, in cases where the short name is different, that bit of information (the first line of the error) is usually more important than the Details. To balance usefulness and verbosity of the Details line, I also removed the trailing (because ...) part.

I'm happy to keep workshopping this error format, but I would submit it's already an extremely helpful error, especially to an LLM.

apollo-librarian · 2025-11-17T20:59:49Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 1e69c6e325f0cd0bfda1986d
Build Logs: View logs

#8616 (comment) The first line is still an improvement over the previous logic because it gives the short names of _both_ `mismatch.expected` and `mismatch.received`, rather than just the received shape name. The `Details: {} did not accept {}` line is important for providing more exact information, including explaining situations where the short names collide (two "object" shapes might be incompatible for any number of reasons, for example). The structure of this shape display syntax should also be helpful for LLMs to make sense of these errors, whereas the previous errors were rather opaque.

naomijub · 2025-11-18T17:57:14Z

apollo-federation/src/connectors/json_selection/methods/public/as.rs

        assert_eq!(
            selection!("person->as", spec).shape().pretty_print(),
-            "Error<\"Method ->as requires one or two arguments (got 0)\", $root.person>",
+            "Error<\n  \"Method ->as requires one or two arguments (got 0)\",\n  $root.person,\n>",


Maybe use r#""# syntax for those blocks with \n:

r#"Error< "Method ->as requires one or two arguments (got 0)", $root.person, >"#

and get for free removing \"

#8616 (comment) The first line is still an improvement over the previous logic because it gives the short names of _both_ `mismatch.expected` and `mismatch.received`, rather than just the received shape name. The `Details: {} did not accept {}` line is important for providing more exact information, including explaining situations where the short names collide (two "object" shapes might be incompatible for any number of reasons, for example). The structure of this shape display syntax should also be helpful for LLMs to make sense of these errors, whereas the previous errors were rather opaque.

This PR proposes a fix for an apparent bug where the connectors expansion code was creating `@join__directive` directives with empty `graphs: []` arguments, accounting for (the only) composition errors in at least 161/4530 supergraphs in our test corpus: - Before (v2.12.1): 4283 successful, 247 failed - After (this fix, based on current dev): 4444 successful, 86 failed In an even more recent snapshot of this corpus, with 5741 supergraphs, this fix reduced the number of failures from 707 to 236 failures, and that only represents graphs where this empty `@join__directive(graphs: [])` bug was the _only_ problem, suggesting this particular problem is increasing in frequency and probably confusing new developers. This code was originally related to license enforcement, but I believe the plan of record is for connectors to be a free feature, so we can probably do without any license enforcement here. I'm certainly open to alternatives, including explanations of existing behavior, but this seems like an accidental regression that is worth fixing before (or as part of) the next connectors-related release.

We are updating the `shape` crate before introducing any abstract types-related changes, because this update benefits all connect spec versions, while still allowing us to preserve backwards/forwards compatibility for older versions (connect/v0.3 and earlier). It would be a shame to have to maintain two slightly different `shape` APIs (hogging the same cargo package name) in order to keep connect/v0.3 and earlier working as before. One of the goals of this commit is to show how limited/reasonable the updates are, because the new functionality coming in [email protected] does not radically alter the behavior of the library (at least not in the ways we were previously using it). Some notable (expected) changes: we've started using a `shape.locations()` method instead of accessing/mutating the `shape.locations` vector directly, the new shape name tracking system allows better error messages, and shape pretty-printing may now include newlines for readability.

#8616 (comment) The first line is still an improvement over the previous logic because it gives the short names of _both_ `mismatch.expected` and `mismatch.received`, rather than just the received shape name. The `Details: {} did not accept {}` line is important for providing more exact information, including explaining situations where the short names collide (two "object" shapes might be incompatible for any number of reasons, for example). The structure of this shape display syntax should also be helpful for LLMs to make sense of these errors, whereas the previous errors were rather opaque.

benjamn self-assigned this Nov 14, 2025

benjamn requested review from a team as code owners November 14, 2025 19:51

benjamn requested review from andrewmcgivery, dylan-apollo, lennyburdette and naomijub November 14, 2025 19:51

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from f063150 to 07d35ec Compare November 14, 2025 19:57

dylan-apollo reviewed Nov 14, 2025

View reviewed changes

andrewmcgivery reviewed Nov 14, 2025

View reviewed changes

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 7d7f6dd to aeb5a9b Compare November 17, 2025 16:39

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from aeb5a9b to 1deaed9 Compare November 17, 2025 20:59

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 1deaed9 to 10ed3b5 Compare November 18, 2025 15:30

benjamn mentioned this pull request Nov 18, 2025

Adapt connectors expansion and validation to handle abstract types (interfaces and unions) #8143

Draft

naomijub reviewed Nov 18, 2025

View reviewed changes

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 10ed3b5 to 19253d8 Compare December 4, 2025 20:10

benjamn changed the base branch from dev to benjamn/fix-empty-join__directive-graphs December 4, 2025 20:10

benjamn force-pushed the benjamn/fix-empty-join__directive-graphs branch from cc1f10b to 9df22d9 Compare December 4, 2025 20:56

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 19253d8 to 71872d5 Compare December 4, 2025 20:56

benjamn changed the title ~~Update usage of shape crate for 0.7.0-preview.1+ releases~~ Update usage of shape crate for 0.7.0-preview.n releases Dec 4, 2025

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 71872d5 to 4fd9902 Compare December 4, 2025 22:13

benjamn force-pushed the benjamn/fix-empty-join__directive-graphs branch from 9df22d9 to 717b043 Compare December 5, 2025 14:33

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 4fd9902 to 9504be3 Compare December 5, 2025 14:33

benjamn force-pushed the benjamn/fix-empty-join__directive-graphs branch from 717b043 to c8f8920 Compare December 5, 2025 21:51

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 9504be3 to 6a6cfea Compare December 5, 2025 21:51

benjamn added 3 commits December 8, 2025 10:05

benjamn force-pushed the benjamn/fix-empty-join__directive-graphs branch from c8f8920 to f87bcbd Compare December 8, 2025 15:37

benjamn force-pushed the benjamn/update-shape-to-0.7.0-preview branch from 6a6cfea to 1ce8d37 Compare December 8, 2025 15:37

Base automatically changed from benjamn/fix-empty-join__directive-graphs to dev December 8, 2025 16:46

Update usage of shape crate for 0.7.0-preview.n releases #8616

Are you sure you want to change the base?

Update usage of shape crate for 0.7.0-preview.n releases #8616

Conversation

benjamn commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamn Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamn Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewmcgivery Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamn Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

benjamn Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apollo-librarian bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview has no changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Update usage of `shape` crate for 0.7.0-preview.n releases #8616

Update usage of `shape` crate for 0.7.0-preview.n releases #8616

benjamn commented Nov 14, 2025 •

edited

Loading

benjamn Nov 14, 2025 •

edited

Loading

benjamn Nov 14, 2025 •

edited

Loading

andrewmcgivery Nov 14, 2025 •

edited

Loading

benjamn Nov 14, 2025 •

edited

Loading

benjamn Nov 17, 2025 •

edited

Loading

apollo-librarian bot commented Nov 17, 2025 •

edited

Loading