Skip to content

Commit

Permalink
refactor: cleaning up
Browse files Browse the repository at this point in the history
  • Loading branch information
@duckki
duckki committed Jan 28, 2025
1 parent fdb9a02 commit fb9e5d2
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 96 deletions.
17 changes: 2 additions & 15 deletions apollo-federation/src/correctness/query_plan_analysis.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ fn interpret_fetch_node(
fetch: &FetchNode,
) -> Result<ResponseShape, String> {
let mut result = if let Some(_requires) = &fetch.requires {
// TODO: check requires
// for required_selection in requires {
// let Selection::InlineFragment(inline) = required_selection else {
// return Err(internal_error!(
Expand Down Expand Up @@ -489,13 +490,6 @@ fn interpret_plan_node_at_path(
.join(".")
)));
};
// TODO: pass the `type_cond`
// println!("here: interpret_plan_node_at_path");
// println!(
// "type_cond {type_cond} vs sub-state type {}",
// sub_state.default_type_condition()
// );
// println!("state: {state}\npath: {path:?}\nnode: {node}");
let updated_sub_state = interpret_plan_node_at_path(
schema,
sub_state,
Expand Down Expand Up @@ -548,10 +542,6 @@ fn interpret_flatten_node(
conditions: &[Literal],
flatten: &FlattenNode,
) -> Result<ResponseShape, String> {
// println!(
// "interpret_flatten_node: path: {:?}\nstate: {state}\n",
// flatten.path
// );
let result = interpret_plan_node_at_path(
schema,
state,
Expand All @@ -563,10 +553,7 @@ fn interpret_flatten_node(
let Some(result) = result else {
// `flatten.path` is addressing a non-existing response object.
// Ideally, this should not happen, but QP may try to fetch infeasible selections.
// println!(
// "warning: Response shape does not have a matching path: {:?}\nstate: {state}",
// flatten.path,
// );
// TODO: Report this as a over-fetching later.
return Ok(state.clone());
};
Ok(result.simplify_boolean_conditions())
Expand Down
110 changes: 52 additions & 58 deletions apollo-federation/src/correctness/response_shape.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ fn get_interface_implementers<'a>(
}

/// Does `x` implies `y`? (`x`'s possible types is a subset of `y`'s possible types)
/// - All type-definition positions are in the API schema.
/// - All type-definition positions are in the given schema.
// Note: Similar to `runtime_types_intersect` (avoids using `possible_runtime_types`)
fn runtime_types_implies(
x: &CompositeTypeDefinitionPosition,
Expand Down Expand Up @@ -132,15 +132,15 @@ fn get_ground_types(
// That can happen when a more specific type condition is computed
// than the one that was explicitly provided.
#[derive(Debug, Clone)]
struct AppliedTypeCondition(Vec<CompositeTypeDefinitionPosition>);
struct DisplayTypeCondition(Vec<CompositeTypeDefinitionPosition>);

impl AppliedTypeCondition {
impl DisplayTypeCondition {
fn new(ty: CompositeTypeDefinitionPosition) -> Self {
AppliedTypeCondition(vec![ty])
DisplayTypeCondition(vec![ty])
}

fn deduced() -> Self {
AppliedTypeCondition(Vec::new())
DisplayTypeCondition(Vec::new())
}

/// Construct a new type condition with a named type condition added.
Expand All @@ -166,10 +166,11 @@ impl AppliedTypeCondition {
}
buf.push(ty);
buf.sort_by(|a, b| a.type_name().cmp(b.type_name()));
Ok(AppliedTypeCondition(buf))
Ok(DisplayTypeCondition(buf))
}
}

/// Aggregated type conditions that are normalized for comparison
#[derive(Debug, Clone)]
pub struct NormalizedTypeCondition {
// The set of object types that are used for comparison.
Expand All @@ -179,7 +180,7 @@ pub struct NormalizedTypeCondition {
ground_set: Vec<ObjectTypeDefinitionPosition>,

// Simplified type condition for display.
for_display: AppliedTypeCondition,
for_display: DisplayTypeCondition,
}

impl PartialEq for NormalizedTypeCondition {
Expand All @@ -196,25 +197,52 @@ impl std::hash::Hash for NormalizedTypeCondition {
}
}

// Public accessors
// Public constructors & accessors
impl NormalizedTypeCondition {
/// precondition: `self` and `other` are not empty.
pub fn implies(&self, other: &Self) -> bool {
self.ground_set.iter().all(|t| other.ground_set.contains(t))
/// Construct a new type condition with a single named type condition.
pub(crate) fn from_type_name(
name: Name,
schema: &ValidFederationSchema,
) -> Result<Self, FederationError> {
let ty: CompositeTypeDefinitionPosition = schema.get_type(name)?.try_into()?;
Ok(NormalizedTypeCondition {
ground_set: get_ground_types(&ty, schema)?,
for_display: DisplayTypeCondition::new(ty),
})
}

pub(crate) fn ground_set(&self) -> &[ObjectTypeDefinitionPosition] {
&self.ground_set
pub(crate) fn from_object_type(ty: &ObjectTypeDefinitionPosition) -> Self {
NormalizedTypeCondition {
ground_set: vec![ty.clone()],
for_display: DisplayTypeCondition::new(ty.clone().into()),
}
}

pub(crate) fn from_object_types(
types: impl Iterator<Item = ObjectTypeDefinitionPosition>,
) -> Result<Self, FederationError> {
let mut ground_set: Vec<_> = types.collect();
ground_set.sort_by(|a, b| a.type_name.cmp(&b.type_name));
Ok(NormalizedTypeCondition {
ground_set,
for_display: DisplayTypeCondition::deduced(),
})
}

pub(crate) fn from_ground_type(ty: &ObjectTypeDefinitionPosition) -> Self {
/// Special constructor with empty conditions (logically contains *all* types).
/// - Used for the `_Entity` type.
pub(crate) fn unconstrained() -> Self {
NormalizedTypeCondition {
ground_set: vec![ty.clone()],
for_display: AppliedTypeCondition::new(ty.clone().into()),
ground_set: Vec::new(),
for_display: DisplayTypeCondition::deduced(),
}
}

/// is this type condition represented by a single named type?
pub(crate) fn ground_set(&self) -> &[ObjectTypeDefinitionPosition] {
&self.ground_set
}

/// Is this type condition represented by a single named type?
pub fn is_named_type(&self, type_name: &Name) -> bool {
// Check the display type first.
let Some((first, rest)) = self.for_display.0.split_first() else {
Expand All @@ -230,32 +258,14 @@ impl NormalizedTypeCondition {
};
rest.is_empty() && first.type_name == *type_name
}
}

impl NormalizedTypeCondition {
/// Construct a new type condition with a single named type condition.
pub(crate) fn from_type_name(
name: Name,
schema: &ValidFederationSchema,
) -> Result<Self, FederationError> {
let ty: CompositeTypeDefinitionPosition = schema.get_type(name)?.try_into()?;
Ok(NormalizedTypeCondition {
ground_set: get_ground_types(&ty, schema)?,
for_display: AppliedTypeCondition::new(ty),
})
}

pub(crate) fn from_object_types(
types: impl Iterator<Item = ObjectTypeDefinitionPosition>,
) -> Result<Self, FederationError> {
let mut ground_set: Vec<_> = types.collect();
ground_set.sort_by(|a, b| a.type_name.cmp(&b.type_name));
Ok(NormalizedTypeCondition {
ground_set,
for_display: AppliedTypeCondition::deduced(),
})
/// precondition: `self` and `other` are not empty.
pub fn implies(&self, other: &Self) -> bool {
self.ground_set.iter().all(|t| other.ground_set.contains(t))
}
}

impl NormalizedTypeCondition {
/// Construct a new type condition with a named type condition added.
fn add_type_name(
&self,
Expand All @@ -270,7 +280,7 @@ impl NormalizedTypeCondition {
// - Just returns `other`, since _Entity ∩ other = other.
return Ok(Some(NormalizedTypeCondition {
ground_set: other_types,
for_display: AppliedTypeCondition::new(other_ty),
for_display: DisplayTypeCondition::new(other_ty),
}));
}
let ground_set: Vec<ObjectTypeDefinitionPosition> = self
Expand All @@ -296,15 +306,6 @@ impl NormalizedTypeCondition {
}
}

/// Special constructor with empty conditions (logically contains *all* types).
/// - Used for the `_Entity` type.
pub(crate) fn unconstrained() -> Self {
NormalizedTypeCondition {
ground_set: Vec::new(),
for_display: AppliedTypeCondition::deduced(),
}
}

fn field_type_condition(
&self,
field: &Field,
Expand Down Expand Up @@ -618,7 +619,6 @@ pub struct DefinitionVariant {
sub_selection_response_shape: Option<ResponseShape>,
}

// Public accessors & constructors
impl DefinitionVariant {
pub fn boolean_clause(&self) -> &Clause {
&self.boolean_clause
Expand Down Expand Up @@ -665,7 +665,6 @@ pub struct PossibleDefinitionsPerTypeCondition {
// - Note: The Boolean conditions between variants may not be mutually exclusive.
}

// Public accessors & constructors
impl PossibleDefinitionsPerTypeCondition {
pub fn field_selection_key(&self) -> &FieldSelectionKey {
&self.field_selection_key
Expand All @@ -681,9 +680,7 @@ impl PossibleDefinitionsPerTypeCondition {
conditional_variants: new_variants,
}
}
}

impl PossibleDefinitionsPerTypeCondition {
pub(crate) fn insert_variant(
&mut self,
variant: DefinitionVariant,
Expand Down Expand Up @@ -807,7 +804,6 @@ pub struct ResponseShape {
definitions_per_response_key: IndexMap</*response_key*/ Name, PossibleDefinitions>,
}

// Public accessors
impl ResponseShape {
pub fn default_type_condition(&self) -> &Name {
&self.default_type_condition
Expand All @@ -834,9 +830,7 @@ impl ResponseShape {
.insert(response_key, value)
.is_some()
}
}

impl ResponseShape {
pub fn new(default_type_condition: Name) -> Self {
ResponseShape {
default_type_condition,
Expand Down Expand Up @@ -1173,7 +1167,7 @@ pub fn compute_response_shape_for_inline_fragment(
// ResponseShape display
// - This section is only for display and thus untrusted.

impl fmt::Display for AppliedTypeCondition {
impl fmt::Display for DisplayTypeCondition {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
if self.0.is_empty() {
return write!(f, "<deduced>");
Expand Down
2 changes: 1 addition & 1 deletion apollo-federation/src/correctness/response_shape_compare.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ fn compare_possible_definitions<'a, T: PathConstraint<'a>>(
let ground_set_iter = this_cond.ground_set().iter();
let mut ground_set_iter = ground_set_iter.filter(|ty| path_constraint.allows(ty));
ground_set_iter.try_for_each(|ground_ty| {
let filter_cond = NormalizedTypeCondition::from_ground_type(ground_ty);
let filter_cond = NormalizedTypeCondition::from_object_type(ground_ty);
let other_def =
merge_definitions_for_type_condition(other, &filter_cond).map_err(|e| {
e.add_description(&format!(
Expand Down
44 changes: 22 additions & 22 deletions apollo-federation/src/correctness/subgraph_constraint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub(crate) struct SubgraphConstraint<'a> {
subgraph_types: IndexSet<ObjectTypeDefinitionPosition>,
}

/// Is the object type resolvable in the subgraph schema?
fn is_resolvable(
ty_pos: &ObjectTypeDefinitionPosition,
schema: &ValidFederationSchema,
Expand All @@ -52,9 +53,10 @@ impl<'a> SubgraphConstraint<'a> {
pub(crate) fn at_root(
subgraphs_by_name: &'a IndexMap<Arc<str>, ValidFederationSchema>,
) -> Self {
let all_subgraphs = subgraphs_by_name.keys().cloned().collect();
SubgraphConstraint {
subgraphs_by_name,
possible_subgraphs: subgraphs_by_name.keys().cloned().collect(),
possible_subgraphs: all_subgraphs,
subgraph_types: Default::default(),
}
}
Expand All @@ -78,9 +80,9 @@ impl<'a> SubgraphConstraint<'a> {
Ok(result)
}

// (Parent type & field type consistency in subgraphs) Considering the parent types in
// `self.subgraph_types` and their possible subgraphs, find all object types that the field
// can resolve to.
// (Parent type & field type consistency in subgraphs) Considering the field's possible parent
// types ( `self.subgraph_types`) and their possible entity subgraphs, find all object types
// that the field can resolve to.
fn subgraph_types_for_field(&self, field_name: &str) -> Result<Self, FederationError> {
let mut possible_subgraphs = IndexSet::default();
let mut subgraph_types = IndexSet::default();
Expand Down Expand Up @@ -117,24 +119,6 @@ impl<'a> SubgraphConstraint<'a> {
}

impl<'a> PathConstraint<'a> for SubgraphConstraint<'a> {
/// Is `ty` allowed under the subgraph constraint?
fn allows(&self, ty: &ObjectTypeDefinitionPosition) -> bool {
self.subgraph_types.is_empty() || self.subgraph_types.contains(ty)
}

/// Is `defs` feasible under the subgraph constraint?
fn allows_any(&self, defs: &PossibleDefinitions) -> bool {
if self.subgraph_types.is_empty() {
return true;
}
let intersects = |ground_set: &[ObjectTypeDefinitionPosition]| {
// See if `self.subgraph_types` and `ground_set` have any intersection.
ground_set.iter().any(|ty| self.subgraph_types.contains(ty))
};
defs.iter()
.any(|(type_cond, _)| intersects(type_cond.ground_set()))
}

fn under_type_condition(&self, type_cond: &NormalizedTypeCondition) -> Self {
SubgraphConstraint {
subgraphs_by_name: self.subgraphs_by_name,
Expand All @@ -152,4 +136,20 @@ impl<'a> PathConstraint<'a> for SubgraphConstraint<'a> {
))
})
}

fn allows(&self, ty: &ObjectTypeDefinitionPosition) -> bool {
self.subgraph_types.is_empty() || self.subgraph_types.contains(ty)
}

fn allows_any(&self, defs: &PossibleDefinitions) -> bool {
if self.subgraph_types.is_empty() {
return true;
}
let intersects = |ground_set: &[ObjectTypeDefinitionPosition]| {
// See if `self.subgraph_types` and `ground_set` have any intersection.
ground_set.iter().any(|ty| self.subgraph_types.contains(ty))
};
defs.iter()
.any(|(type_cond, _)| intersects(type_cond.ground_set()))
}
}

0 comments on commit fb9e5d2

Please sign in to comment.