Use `SchemaTypeRef` instead of `Node<ObjectType>` in `ConnectedElement` #8626

benjamn · 2025-11-17T22:19:39Z

This PR represents a combination/cherry-picking of several commits from my abstract types PR #8143 (which I am currently attempting to simplify for easier review).

A SchemaTypeRef is a reference to a &Schema as well as the &Name and &ExtendedType of some named type within the schema, all with the same 'schema lifetime. The SchemaTypeRef::new method returns an Option<SchemaTypeRef> to guarantee that SchemaTypeRef references are only created for actual schema types, so if you have a SchemaTypeRef you can be sure the type it represents exists in the schema.

Because SchemaTypeRef has access to the Schema (not just the individual element), it can perform operations that are aware of other types within the same schema, like finding all the implementing/member types of an interface or union.

Further, since SchemaTypeRef can represent any named type in the schema, not just ObjectType, these changes should lay some of the necessary groundwork for working with abstract types, where the type in question might be an InterfaceType or UnionType (not just ObjectType).

After some earlier attempts, I've decided it's important to use a type like SchemaTypeRef that still implements the Copy trait (by holding only references), rather than (say) ExtendedType (which must be Cloned), so we don't have to update all the data structures that assume they can implement Copy while using ConnectedElement.

apollo-librarian · 2025-11-17T22:19:47Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: fe2cf9a941104165200cc725
Build Logs: View logs

naomijub · 2025-11-18T15:34:05Z

apollo-federation/src/connectors/validation/connect/entity.rs

-                } else {
-                    Some(Err(Message {
+        for arg in field.arguments.iter() {
+            // if let Some(input_type) = self.schema.types.get(arg.ty.inner_named_type()) {


naomijub · 2025-11-18T15:39:25Z

apollo-federation/src/connectors/validation/connect/entity.rs

@@ -247,82 +262,128 @@ impl<'schema> ArgumentVisitor<'schema> {
    fn enter_root_group(


I feel this function could be simplified

naomijub · 2025-11-18T15:44:05Z

...shots/validation_tests@keys_and_entities__invalid__entity_true_returning_scalar.graphql.snap

@@ -1,6 +1,7 @@
 ---
 source: apollo-federation/src/connectors/validation/mod.rs
-expression: "format!(\"{:#?}\", errors)"
+assertion_line: 325
+expression: "format!(\"{:#?}\", result.errors)"


instead of doing this you can do use assert_debug_snapshot

naomijub · 2025-11-18T15:48:08Z

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

-            Connect::find_on_type(node, schema, all_source_names);
-        connects.extend(connects_for_type);
-        messages.extend(messages_for_type);
+    for (type_name, extended_type) in schema.types.iter().filter(|(_, ty)| !ty.is_built_in()) {


I feel this for-loop could be simplified with something like:

pseudo code

let (connects, messages): (Vec<_>, Vec<_>) = schema .types .iter() .filter(|(_, ty)| !ty.is_built_in() && // maybe split in 2 filters? matches!( ty, ExtendedType::Object(_) | ExtendedType::Interface(_) | ExtendedType::Union(_) )) .filter_map(|(type_name, _)| { SchemaTypeRef::new(schema.schema, type_name) }) .map(|type_ref| Connect::find_on_type(type_ref, schema, all_source_names)) .unzip();

Advantage is that you do a single allocation to the vector instead of extending it multiple times

naomijub · 2025-11-18T15:50:54Z

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

        schema: &'schema SchemaInfo,
        source_names: &'schema [SourceName],
    ) -> (Vec<Self>, Vec<Message>) {
        let object_category = if schema
            .schema_definition
            .query
            .as_ref()
-            .is_some_and(|query| query.name == object.name)
+            .is_some_and(|query| query.name == *type_ref.name())


why not something like:

.is_some_and(|query| &query.name == type_ref.name())

naomijub · 2025-11-18T15:58:24Z

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

            // mark the field with a @connect directive as seen
            seen.push(ResolvedField {
-                object_name: parent_type.name.clone(),
+                object_name: parent_type.name().clone(),


too many clones on this field, parent_type.name().clone(), would it make sense to use it as ref? or wrap it in an Arc?

naomijub · 2025-11-18T16:02:38Z

apollo-federation/src/connectors/schema_type_ref.rs

+                    .last()
+                    .and_then(|parent_name| SchemaTypeRef::new(self.0, parent_name))
+                    .map(|parent_ref| parent_ref.is_abstract())
+                    .unwrap_or(false);


unwrap or default is equals to false

naomijub · 2025-11-18T16:03:38Z

apollo-federation/src/connectors/schema_type_ref.rs

+        self.2
+    }
+
+    #[allow(dead_code)]


why so much dead code?

maybe use #[expect(dead_code)] instead of allow, so we can remove the flag when it is actually in use

naomijub · 2025-11-18T16:07:29Z

apollo-federation/src/connectors/schema_type_ref.rs

+            .into_iter()
+            .flat_map(|ty| match ty {
+                ExtendedType::Object(o) => {
+                    let mut map = IndexMap::default();


ExtendedType::Object(o) => { o.fields .get(field_name) .map(|field_def| { std::iter::once((o.name.clone(), field_def)) .collect::<IndexMap<_, _>>() }) .unwrap_or_default() }

or, if you prefer if let

ExtendedType::Object(o) => { if let Some(field_def) = o.fields.get(field_name) { std::iter::once((o.name.clone(), field_def)) .collect() } else { IndexMap::new() } }

naomijub · 2025-11-18T16:08:32Z

apollo-federation/src/connectors/schema_type_ref.rs

+                    map
+                }
+
+                ExtendedType::Interface(i) => {


maybe something like this here as well:

Pseudo codde

ExtendedType::Interface(i) => { self.0 .implementers_map() .get(i.name.as_str()) .map(|implementers| { implementers .objects .iter() .chain(&implementers.interfaces) .filter_map(|name| SchemaTypeRef::new(self.0, name.as_str())) .flat_map(|tref| tref.get_fields(field_name).into_iter()) .collect::<IndexMap<_, _>>() }) .unwrap_or_default() }

naomijub · 2025-11-18T17:49:59Z

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

        let coordinate = ConnectDirectiveCoordinate {
            element: ConnectedElement::Field {
-                parent_type: object,
+                parent_type: SchemaTypeRef::from_node(&schema, object).unwrap(),


can this panic?

benjamn · 2025-11-25T16:39:11Z

@copilot I basically agree with everything @naomijub suggested above. Can you draft a commit that addresses everything unambiguous and a comment that explains anything you couldn't handle?

Copilot · 2025-11-25T16:39:20Z

@benjamn I've opened a new pull request, #8676, to work on those changes. Once the pull request is ready, I'll request review from you.

A SchemaTypeRef is a reference to a &Schema as well as the &Name and &ExtendedType of some named type within the schema, all with the same 'schema lifetime. The SchemaTypeRef::new method returns an Option<SchemaTypeRef> to guarantee that SchemaTypeRef references are only created for actual schema types. Because SchemaTypeRef has access to the Schema (not just an individual element), it can perform operations that are aware of other types within the same schema, like finding all the implementing/member types of an interface or union. Because SchemaTypeRef can represent any named type in the schema, not just ObjectType, these changes should lay some of the necessary groundwork for working with abstract types, where the type in question might be an InterfaceType or UnionType. After some earlier attempts, I've decided it's important to use a type like SchemaTypeRef that still implements the Copy trait, rather than (say) ExtendedType, so we don't have to update all the data structures that assume they can implement Copy while using ConnectedElement.

Co-authored-by: copilot-swe-agent[bot] <[email protected]>

benjamn self-assigned this Nov 17, 2025

benjamn requested a review from a team as a November 17, 2025 22:19

benjamn force-pushed the benjamn/SchemaTypeRef-instead-of-Node-ObjectType branch from f2a3d0f to a93d0b6 Compare November 17, 2025 22:23

benjamn requested review from andrewmcgivery, dylan-apollo, lennyburdette and naomijub November 17, 2025 22:33

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

benjamn requested review from a team as code owners November 18, 2025 15:30

benjamn force-pushed the benjamn/SchemaTypeRef-instead-of-Node-ObjectType branch from a93d0b6 to f3aafe4 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

Copilot AI mentioned this pull request Nov 25, 2025

Address review feedback for SchemaTypeRef changes #8676

Merged

9 tasks

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

benjamn force-pushed the benjamn/SchemaTypeRef-instead-of-Node-ObjectType branch from a77dfd2 to 8d679e0 Compare December 4, 2025 20:11

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

benjamn force-pushed the benjamn/SchemaTypeRef-instead-of-Node-ObjectType branch from 8d679e0 to eaf069f Compare December 4, 2025 20:56

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/SchemaTypeRef-instead-of-Node-ObjectType branch from eaf069f to e16d56d Compare December 4, 2025 22:13

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/SchemaTypeRef-instead-of-Node-ObjectType branch from e16d56d to 9f44e1c Compare December 5, 2025 14:33

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

benjamn force-pushed the benjamn/SchemaTypeRef-instead-of-Node-ObjectType branch from 9f44e1c to 41756d9 Compare December 5, 2025 21:51

benjamn and others added 2 commits December 8, 2025 10:30

Address review feedback for SchemaTypeRef changes (#8676)

56ab2fc

Co-authored-by: copilot-swe-agent[bot] <[email protected]>

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

benjamn force-pushed the benjamn/SchemaTypeRef-instead-of-Node-ObjectType branch from 41756d9 to 56ab2fc Compare December 8, 2025 15:37

		@@ -247,82 +262,128 @@ impl<'schema> ArgumentVisitor<'schema> {
		fn enter_root_group(

Use SchemaTypeRef instead of Node<ObjectType> in ConnectedElement #8626

Are you sure you want to change the base?

Use SchemaTypeRef instead of Node<ObjectType> in ConnectedElement #8626

Uh oh!

Conversation

benjamn commented Nov 17, 2025

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!

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!

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!

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 commented Nov 25, 2025

Uh oh!

Copilot AI commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use `SchemaTypeRef` instead of `Node<ObjectType>` in `ConnectedElement` #8626

Use `SchemaTypeRef` instead of `Node<ObjectType>` in `ConnectedElement` #8626

apollo-librarian bot commented Nov 17, 2025 •

edited

Loading