Query Graph Traversal: Part 2 #161
Conversation
sachindshinde
force-pushed
the
sachin/query-graph-traversal-part-2
branch
3 times, most recently
from
a2cec85 to
4e422fa
Compare
sachindshinde
force-pushed
the
sachin/query-graph-traversal-part-2
branch
from
4e422fa to
a688086
Compare
| @@ -141,3 +142,21 @@ pub(crate) fn directive_required_boolean_argument( | |||
| .into() | |||
| }) | |||
| } | |||
|
|
|||
| pub(crate) fn directive_optional_variable_boolean_argument( | |||
There was a problem hiding this comment.
I’m thinking we could use a trait for this kind of conversion, but probably not in this PR. (Rather in a larger design for apollographql/apollo-rs#717)
There was a problem hiding this comment.
Yeah, having something cleaner/simpler here would be nice (ideally with some way to handle custom scalars).
| pub(crate) fn unsatisfied_conditions() -> Self { | ||
| Self { | ||
| satisfied: false, | ||
| cost: -1, |
There was a problem hiding this comment.
Why is cost signed? What does negative cost mean? If -1 is the only negative value with special meaning different from "minus one", should it be Option::None instead?
There was a problem hiding this comment.
It should never be used directly if it's -1 I think. I ended up refactoring this into an enum instead, so cost vanishes in the unsatisfied case.
src/query_graph/graph_path.rs
Outdated
| /// If the trigger of the last edge in the `edges` array was an operation element with a | ||
| /// `@defer` application, then the arguments of that application. | ||
| defer_on_tail: Option<DeferDirectiveArguments>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] |
There was a problem hiding this comment.
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | |
| #[derive(Debug, Clone, PartialEq, Eq, Hash, derive_more::From)] |
We already depend on this derive macro crate, which I think can replace the two impls below.
| // atomically increment global counter | ||
| Self(NEXT_OVERRIDE_ID.fetch_add(1, atomic::Ordering::AcqRel)) |
There was a problem hiding this comment.
Do we ever plan on (de)serializing something that contains OverrideId? That could lead to collisions. Maybe add a comment on the struct to warn against adding derive(Serialize, Deserialize) without changing new() to do something else like UUIDs.
There was a problem hiding this comment.
Nah, we shouldn't be serializing/deserializing these AFAIK. Will add a warning in a comment.
src/query_graph/mod.rs
Outdated
| // PORT_NOTE: The JS codebase found all matching candidates and asserted there was only one. | ||
| // We consider it unlikely for there to be more than one candidate given all the code paths | ||
| // that create edges, so we've removed the assert here and return on the first match. |
There was a problem hiding this comment.
We could still assert (or debug assert) there isn’t a second candidate without collecting all of them:
let mut iter = /* … */;
if let Some(first) = iter.next() {
debug_assert!(iter.next().is_none(), "unexpectedly found multiple candidates")
// …
}There was a problem hiding this comment.
That's a nice pattern, will add the debug_assert!()!
| _source: &NodeStr, | ||
| _interface_field_definition_position: InterfaceFieldDefinitionPosition, | ||
| ) -> Result<bool, FederationError> { | ||
| todo!() |
There was a problem hiding this comment.
we’ll definitely need to grep the entire code base for FIXME/TODO/todo! at some point
There was a problem hiding this comment.
Yeah, most of the todo!()s in this PR are just blocked by not having an easy way to get subgraph info, so I'm going to file a follow-up PR for that.
| pub(crate) fn new() -> Self { | ||
| // atomically increment global counter | ||
| Self(NEXT_ID.fetch_add(1, atomic::Ordering::AcqRel)) |
There was a problem hiding this comment.
Same as above for OverrideId re serialization
src/query_plan/operation.rs
Outdated
| pub(crate) fn defer_directive_arguments( | ||
| &self, | ||
| ) -> Result<Option<DeferDirectiveArguments>, FederationError> { | ||
| if let Some(directive) = self.directives.get(&name!("defer")) { |
There was a problem hiding this comment.
| if let Some(directive) = self.directives.get(&name!("defer")) { | |
| if let Some(directive) = self.directives.get("defer") { |
I think this should be equivalent
sachindshinde
force-pushed
the
sachin/query-graph-traversal-part-2
branch
from
f31778b to
84b2483
Compare
Fixes #155