Added logic to parse @context in FederatedQueryGraphBuilder::build
#6223
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
|
CI performance tests
|
TylerBloom
force-pushed
the
tylerb/port-context-in-federate-subgraphs
branch
from
2ce6517
to
a757dfd
Compare
TylerBloom
force-pushed
the
router-528-impl-set-context
branch
from
cf7c4fe
to
95a8db5
Compare
TylerBloom
force-pushed
the
tylerb/port-context-in-federate-subgraphs
branch
from
a6f22b7
to
76f8cad
Compare
| // fromContext in that subgraph, ya? | ||
| .find(|(dir, _)| **dir == FROM_CONTEXT_DIRECTIVE_NAME) | ||
| else { | ||
| continue; |
There was a problem hiding this comment.
One not about this: This is silencing potential errors. There could be @context in the subgraph but not @fromContext (or worse, visa versa). This simply ignore that. I'm comfortable with this because composition should catch that, but it would also be reasonable track this better and raise an error.
| subgraph_to_args: IndexMap<Arc<str>, Vec<ObjectFieldArgumentDefinitionPosition>>, | ||
| subgraph_to_arg_indices: | ||
| IndexMap<Arc<str>, IndexMap<ObjectFieldArgumentDefinitionPosition, String>>, |
There was a problem hiding this comment.
I want to revisit this was we near the end of porting this feature. These fields contain the same data, in the same order. The only difference is one also contains identifier strings. We should be able to remove subgraph_to_args. For now, keeping it will make porting things easier.
TylerBloom
requested review from
dariuszkuc,
sachindshinde,
goto-bus-stop,
SimonSapin,
lrlna and
duckki
as code owners
| .root_kinds_to_nodes_by_source | ||
| .iter() | ||
| .filter(|(source, _)| **source != self.base.query_graph.current_source) | ||
| .flat_map(|(_, root_kind_to_notes)| root_kind_to_notes.keys().copied()) |
There was a problem hiding this comment.
typo
| .flat_map(|(_, root_kind_to_notes)| root_kind_to_notes.keys().copied()) | |
| .flat_map(|(_, root_kind_to_nodes)| root_kind_to_nodes.keys().copied()) |
| .referencers() | ||
| .directives | ||
| .iter() | ||
| // TODO: I think I need to get the subgraph data which contains the name of the |
There was a problem hiding this comment.
I think so? Because the directive could be renamed in the subgraph, I think we want to use the ContextSpecDefinition to get the name, but I'm not totally sure of the mechanics.
| // For @context | ||
| // subgraph -> referencers -> iter through object_types and interface_types | ||
| let subgraph_data = self.subgraphs.get(source)?; | ||
| let mut context_name_to_types: HashMap<&str, HashSet<&ObjectTypeDefinitionPosition>> = |
There was a problem hiding this comment.
We have to build a similar HashMap for interfaces and unions? Or can we do it in the same map?
| context_name_to_types | ||
| .entry(application.name) | ||
| .or_default() | ||
| .insert(object_def_pos); |
There was a problem hiding this comment.
I like how elegant this is in Rust.
|
|
||
| // Collect data for @fromContext | ||
| let coordinate_map = coordinate_map.entry(subgraph_name.clone()).or_default(); | ||
| for object_field_arg in &from_context_refs.object_field_arguments { |
There was a problem hiding this comment.
It's nice that the way we've architected this, we don't have to do some of the checking that's done in the JS code.
There are still a few TODOs. The regular expression that is used will fail to build because looking around is not supported. The logic inside the
simpleTraversalfor the context has also not been ported. Lastly, the definition position types might be off and need to be tested.