Skip to content

Commit

Permalink
Wrap dead-end logic with closures (#3068)
Browse files Browse the repository at this point in the history
While looking at satisfiability validations, @duckki noticed that we were throwing away dead-end/unadvanceable data. I wanted to check whether we could use closures to avoid executing dead-logic entirely unless it's needed (when no options are found for a path), and it turns out it's possible (specifically, the closure's state ends up being immutable, so delaying execution shouldn't change behavior). This PR accordingly wraps the dead-end logic with closures, and only executes them when needed for generating satisfiability errors.
  • Loading branch information
sachindshinde authored Jul 8, 2024
1 parent 860aace commit 38debcf
Show file tree
Hide file tree
Showing 3 changed files with 241 additions and 190 deletions.
6 changes: 6 additions & 0 deletions .changeset/afraid-phones-reflect.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@apollo/composition": patch
"@apollo/query-graphs": patch
---

Error messages are now lazily evaluated for satisfiability validations.
9 changes: 5 additions & 4 deletions composition-js/src/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,14 @@ import {
Transition,
QueryGraphState,
Unadvanceables,
isUnadvanceable,
Unadvanceable,
noConditionsResolution,
TransitionPathWithLazyIndirectPaths,
RootVertex,
simpleValidationConditionResolver,
ConditionResolver,
UnadvanceableClosures,
isUnadvanceableClosures,
} from "@apollo/query-graphs";
import { CompositionHint, HINTS } from "./hints";
import { ASTNode, GraphQLError, print } from "graphql";
Expand Down Expand Up @@ -486,7 +487,7 @@ export class ValidationState {
const transition = supergraphEdge.transition;
const targetType = supergraphEdge.tail.type;
const newSubgraphPathInfos: SubgraphPathInfo[] = [];
const deadEnds: Unadvanceables[] = [];
const deadEnds: UnadvanceableClosures[] = [];
// If the edge has an override condition, we should capture it in the state so
// that we can ignore later edges that don't satisfy the condition.
const newOverrideConditions = new Map([...this.selectedOverrideConditions]);
Expand All @@ -504,7 +505,7 @@ export class ValidationState {
targetType,
newOverrideConditions,
);
if (isUnadvanceable(options)) {
if (isUnadvanceableClosures(options)) {
deadEnds.push(options);
continue;
}
Expand Down Expand Up @@ -533,7 +534,7 @@ export class ValidationState {
}
const newPath = this.supergraphPath.add(transition, supergraphEdge, noConditionsResolution);
if (newSubgraphPathInfos.length === 0) {
return { error: satisfiabilityError(newPath, this.subgraphPathInfos.map((p) => p.path.path), deadEnds) };
return { error: satisfiabilityError(newPath, this.subgraphPathInfos.map((p) => p.path.path), deadEnds.map((d) => d.toUnadvanceables())) };
}

const updatedState = new ValidationState(
Expand Down
Loading

0 comments on commit 38debcf

Please sign in to comment.