Supports federation 2.3 in the router #2489

pcmanus · 2023-01-27T15:09:20Z

The query planner in Federation 2.3 adds a new concept of "data rewrites" for fetches in order to support both @interfaceObject and to support the fix for apollographql/federation#1257.

Those "rewrites" describe simple updates that need to be performed either on the inputs (the "representations" passed to _entities; need to rewrite the __typename when sending queries to an @interfaceObject) or the output of a fetch (needed when a field has been aliased to permit the subgraph query to be valid, but that field needs to be "un-aliased" to its original name after the fetch).

This commit implements those rewrites.

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

Exceptions

Note any exceptions here

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

Some of the paths returned by the query planner may contain some "fragment", which are really meant to act as type conditions. That is, for some data like: ```json { "x": [ { "__typename": A, "a": 0 }, { "__typename": B, "a": 1 }, { "__typename": A, "a": 2 }, ] "y": 42 } ``` and a path `["x", "... on A", "a"]`, then the intend is that this selects the following data: ```json { "x": [ { "__typename": A, "a": 0 }, { "__typename": A, "a": 2 }, ] } ``` but skips the `B` entity in particular. That logic is currently not implemented, and while fragments in path are parsed, they are otherwise ignored (and so the code currently ends up selecting the `B` entity). The only place where such fragments may appear in paths currently is in deferred nodes, where the paths are used to select what should be in a particular defer response. I believe (I haven't taken the time to test it yet) that this mean the code may currently sometimes include data in a given deferred response that should be sent in a different response. This commit fix the code that handles fragments in path so that they are taken account as explained in the example above. Unfortunately, this doesn't quite fix the issue with deferred response mentioned above (it should in some cases, but not all of them) because the code currently filters responses _before_ it tries to apply those path selections, which means that in many cases the "__typename" fields are filtered out and thus cannot be used to decide if an object matches the path fragment or not (the code thus default to include the object to mimick the existing behaviour, but this may be incorrect in some cases). However, those fragments in path are heavily used in the changes introduced to the query planner by federation 2.3, so while this patch does not quite fix the defer issue, it is a step forward and is necessary in preparation for supporting federation 2.3.

pcmanus · 2023-01-27T15:12:13Z

This PR is technically on top of #2485, though that later PR is rebased on dev, while this current PR is currently rebased on 1.10.0-alpha.0 because it needs the updated router bridge, and so I temporarily cherry-picked the commit of #2485 here. But it definitively make sense to review that other PR first, and to rebase this afterward.

The query planner in Federation 2.3 adds a new concept of "data rewrites" for fetches in order to support both `@interfaceObject` and to support the fix for apollographql/federation#1257. Those "rewrites" describe simple updates that need to be performed either on the inputs (the "representations" passed to `_entities`; need to rewrite the `__typename` when sending queries to an `@interfaceObject`) or the output of a fetch (needed when a field has been aliased to permit the subgraph query to be valid, but that field needs to be "un-aliased" to its original name after the fetch). This commit implements those rewrites.

Geal

Looks good! This needs a merge from dev to resolve the conflicts with 09cff54 and then that will be good to go

apollo-router/src/query_planner/rewrites.rs

bnjjj · 2023-01-31T10:53:36Z

apollo-router/src/services/supergraph_service.rs

+            schema
+              @link(url: "https://specs.apollo.dev/link/v1.0")
+              @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)
+            {
+              query: Query
+            }
+
+            directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE


It's taking a lot of lines in our file, maybe for later we should refactor this and put all these schemas in separate files in testdata directory and use include_str! macro to have a cleaner file.

Happy to change it if you prefer, it's fairly quick. I mostly followed the pattern of the previous tests in this file.

Though just for the conversion, I tend to find it easier to maintain tests when they are somewhat self-contained, and you don't have to navigate multiple files to understand what is going on. But it's probably more of a personal preference and not trying to push anything really.

bnjjj · 2023-01-31T10:54:59Z

apollo-router/src/json_ext.rs

@@ -528,7 +665,10 @@ impl Path {
                    } else if s == "@" {
                        PathElement::Flatten
                    } else {
-                        PathElement::Key(s.to_string())
+                        s.strip_prefix("... on ").map_or_else(


Just a nit: that could be helpful to put "... on " in a constant somewhere to avoid typo and forget the trailing whitespace somewhere

Good call. Pushed the quick change.

Geal · 2023-01-31T13:37:31Z

I merged dev and took care of the conflict

abernix · 2023-01-31T14:22:19Z

Looks like cargo fmt check failed on this one.

…oducted by `@interfaceObject` The introduction of `@interfaceObject` in federation 2.3 has for consequence that a subgraph (that uses an `@interfaceObject`) may return in its response a `__typename` that corresponds to an interface type in the supergraph (but which, locally to that subgraph, is an object type). The idea is that if that `__typename` is requested, then the query planner will ensure that another follow-up fetch will override that value so it maps to a proper object type. However, in some cases, that `__typename` is not queried, nor is there another reason to resolve the actual underlying implementation type of the underlying object (meaning that only fields of the interface are queried), and in that case the `__typename` may still point to the interface type when `format_response` is called. Unfortunately, that method currently nullify the whole object in such case, which is not the behaviour we want and was not caught in apollographql#2489.

… cases The introduction of `@interfaceObject` in federation 2.3 has for consequence that a subgraph (that uses an `@interfaceObject`) may return in its response a `__typename` that corresponds to an interface type in the supergraph (but which, locally to that subgraph, is an object type). The idea is that if that `__typename` is requested, then the query planner will ensure that another follow-up fetch will override that value so it maps to a proper object type. However, in some cases, that `__typename` is not queried, nor is there another reason to resolve the actual underlying implementation type of the underlying object (meaning that only fields of the interface are queried), and in that case the `__typename` may still point to the interface type when `format_response` is called. Unfortunately, that method currently nullify the whole object in such case, which is not the behaviour we want and was not caught in #2489.

[#2489](#2489) introduced automatic aliasing rules to support `@interfaceObject`. These rules now properly apply to lists.

pcmanus force-pushed the federation23-support branch from 2a0aa0b to 36651cc Compare January 27, 2023 15:13

abernix requested review from Geal, o0Ignition0o and bnjjj and removed request for Geal and o0Ignition0o January 27, 2023 15:17

Geal suggested changes Jan 30, 2023

View reviewed changes

apollo-router/src/query_planner/rewrites.rs Show resolved Hide resolved

bnjjj approved these changes Jan 31, 2023

View reviewed changes

Geal mentioned this pull request Jan 31, 2023

Fix handling of fragments in handling of Path #2485

Merged

6 tasks

Merge branch '1.10.0-alpha.0' into federation23-support

10e161d

Geal self-requested a review January 31, 2023 13:36

pcmanus added 3 commits January 31, 2023 15:07

[Review feedback] Extract repeated string in constant

71e7d62

Add changelog entry

1b65e7a

Fix built?

b3b46d7

Keep linter happy

bd74e68

Geal approved these changes Jan 31, 2023

View reviewed changes

Geal merged commit f255fa3 into apollographql:1.10.0-alpha.0 Jan 31, 2023

abernix mentioned this pull request Feb 1, 2023

prep release: v1.10.0 #2516

Merged

pcmanus mentioned this pull request Feb 2, 2023

fix: __typename when formatting response in some @interfaceObject cases #2530

Merged

6 tasks

Geal mentioned this pull request Feb 9, 2023

modifying documents in compiler context apollographql/apollo-rs#420

Closed

o0Ignition0o added a commit that referenced this pull request Apr 15, 2024

Apply alias rewrites to arrays

b5568be

[#2489](#2489) introduced automatic aliasing rules to support `@interfaceObject`. These rules now properly apply to lists.

o0Ignition0o mentioned this pull request Apr 15, 2024

Apply alias rewrites to arrays #4958

Merged

o0Ignition0o added a commit that referenced this pull request Apr 16, 2024

Apply alias rewrites to arrays (#4958)

af404f2

[#2489](#2489) introduced automatic aliasing rules to support `@interfaceObject`. These rules now properly apply to lists.

abernix mentioned this pull request Apr 22, 2024

prep release: v1.45.0 #4995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supports federation 2.3 in the router #2489

Supports federation 2.3 in the router #2489

pcmanus commented Jan 27, 2023

pcmanus commented Jan 27, 2023

Geal left a comment

bnjjj Jan 31, 2023

pcmanus Jan 31, 2023

bnjjj Jan 31, 2023

pcmanus Jan 31, 2023

Geal commented Jan 31, 2023

abernix commented Jan 31, 2023

Supports federation 2.3 in the router #2489

Supports federation 2.3 in the router #2489

Conversation

pcmanus commented Jan 27, 2023

pcmanus commented Jan 27, 2023

Geal left a comment

Choose a reason for hiding this comment

bnjjj Jan 31, 2023

Choose a reason for hiding this comment

pcmanus Jan 31, 2023

Choose a reason for hiding this comment

bnjjj Jan 31, 2023

Choose a reason for hiding this comment

pcmanus Jan 31, 2023

Choose a reason for hiding this comment

Geal commented Jan 31, 2023

abernix commented Jan 31, 2023