From 3b670d85800504a835ce0d83585e64b7f834d8d6 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Wed, 26 Feb 2025 11:31:52 +0000 Subject: [PATCH] Be stricter about types and usage of getDep/getDepOptions --- .../dataplan-pg/src/steps/pgPolymorphic.ts | 13 ++++-- grafast/dataplan-pg/src/steps/pgSelect.ts | 2 +- grafast/dataplan-pg/src/steps/pgUnionAll.ts | 2 +- grafast/grafast/src/engine/OutputPlan.ts | 2 +- grafast/grafast/src/interfaces.ts | 15 ++++-- grafast/grafast/src/step.ts | 42 ++++++++++++++--- grafast/grafast/src/steps/__flag.ts | 46 ++++++++++++------- .../grafast/src/steps/__inputDynamicScalar.ts | 2 +- grafast/grafast/src/steps/__trackedValue.ts | 2 +- grafast/grafast/src/steps/access.ts | 8 ++-- grafast/grafast/src/steps/connection.ts | 4 +- grafast/grafast/src/steps/listTransform.ts | 8 +++- grafast/grafast/src/steps/object.ts | 13 ++++-- .../grafast/src/steps/polymorphicBranch.ts | 2 +- 14 files changed, 111 insertions(+), 50 deletions(-) diff --git a/grafast/dataplan-pg/src/steps/pgPolymorphic.ts b/grafast/dataplan-pg/src/steps/pgPolymorphic.ts index 4b030dd1a..8b6df00ce 100644 --- a/grafast/dataplan-pg/src/steps/pgPolymorphic.ts +++ b/grafast/dataplan-pg/src/steps/pgPolymorphic.ts @@ -82,12 +82,13 @@ export class PgPolymorphicStep< }) as any; } - itemPlan(): TItemStep { - return this.getDep(this.itemStepId); + private itemPlan(): TItemStep { + return this.getDepOptions(this.itemStepId).step; } - typeSpecifierPlan(): TTypeSpecifierStep { - return this.getDep(this.typeSpecifierStepId); + private typeSpecifierPlan(): TTypeSpecifierStep { + return this.getDepOptions(this.typeSpecifierStepId) + .step; } planForType(type: GraphQLObjectType): ExecutableStep { @@ -101,7 +102,9 @@ export class PgPolymorphicStep< ).join("', '")}'`, ); } - return spec.plan(this.typeSpecifierPlan(), this.itemPlan()); + const $typeSpecifier = this.typeSpecifierPlan(); + const $item = this.itemPlan(); + return spec.plan($typeSpecifier, $item); } private getTypeNameFromSpecifier(specifier: TTypeSpecifier) { diff --git a/grafast/dataplan-pg/src/steps/pgSelect.ts b/grafast/dataplan-pg/src/steps/pgSelect.ts index c6a4681a2..faeb43f23 100644 --- a/grafast/dataplan-pg/src/steps/pgSelect.ts +++ b/grafast/dataplan-pg/src/steps/pgSelect.ts @@ -1644,7 +1644,7 @@ export class PgSelectRowsStep< } public getClassStep(): PgSelectStep { - return this.getDep>(0); + return this.getDepOptions>(0).step; } listItem(itemPlan: Step) { diff --git a/grafast/dataplan-pg/src/steps/pgUnionAll.ts b/grafast/dataplan-pg/src/steps/pgUnionAll.ts index 9280eee65..234c5543a 100644 --- a/grafast/dataplan-pg/src/steps/pgUnionAll.ts +++ b/grafast/dataplan-pg/src/steps/pgUnionAll.ts @@ -1096,7 +1096,7 @@ export class PgUnionAllRowsStep< this.addDependency($pgUnionAll); } public getClassStep(): PgUnionAllStep { - return this.getDep>(0); + return this.getDepOptions>(0).step; } listItem(itemPlan: Step) { diff --git a/grafast/grafast/src/engine/OutputPlan.ts b/grafast/grafast/src/engine/OutputPlan.ts index 4e7aea7c9..195cc4b49 100644 --- a/grafast/grafast/src/engine/OutputPlan.ts +++ b/grafast/grafast/src/engine/OutputPlan.ts @@ -466,7 +466,7 @@ export class OutputPlan { | undefined = ($root.unbatchedExecute! as any)[expressionSymbol]; if (expressionDetails !== undefined) { // @ts-ignore - const $parent: Step = $root.getDep(0); + const { step: $parent } = $root.getDepOptions(0); this.layerPlan.operationPlan.stepTracker.setOutputPlanRootStep( this, $parent, diff --git a/grafast/grafast/src/interfaces.ts b/grafast/grafast/src/interfaces.ts index 24d7fa448..bce96486e 100644 --- a/grafast/grafast/src/interfaces.ts +++ b/grafast/grafast/src/interfaces.ts @@ -788,7 +788,7 @@ export interface LocationDetails { } export type UnwrapPlanTuple = { - [Index in keyof TIn]: TIn[Index] extends Step ? U : never; + [Index in keyof TIn]: DataFromStep; }; export type NotVariableValueNode = Exclude; @@ -809,14 +809,21 @@ export type Maybe = T | null | undefined; export * from "./planJSONInterfaces.js"; -export interface AddDependencyOptions { - step: Step; +export interface AddDependencyOptions { + step: TStep; skipDeduplication?: boolean; /** @defaultValue `FLAG_NULL` */ acceptFlags?: ExecutionEntryFlags; - onReject?: null | Error | undefined; + onReject?: Maybe; nonUnaryMessage?: ($dependent: Step, $dependency: Step) => string; } + +export interface DependencyOptions { + step: TStep; + acceptFlags: ExecutionEntryFlags; + onReject: Maybe; +} + /** * @internal */ diff --git a/grafast/grafast/src/step.ts b/grafast/grafast/src/step.ts index 5854d3668..a163e8fe0 100644 --- a/grafast/grafast/src/step.ts +++ b/grafast/grafast/src/step.ts @@ -20,6 +20,7 @@ import { getDebug } from "./global.js"; import { inspect } from "./inspect.js"; import type { AddDependencyOptions, + DependencyOptions, ExecutionDetails, ExecutionEntryFlags, ExecutionResults, @@ -36,7 +37,7 @@ import { ALL_FLAGS, DEFAULT_FORBIDDEN_FLAGS, } from "./interfaces.js"; -import type { __ItemStep } from "./steps/index.js"; +import type { __FlagStep, __ItemStep } from "./steps/index.js"; import { stepADependsOnStepB, stepAMayDependOnStepB } from "./utils.js"; /** @@ -167,8 +168,11 @@ export /* abstract */ class Step { /** * What execution entry flags we can't handle for the given indexed dependency * (default = this.defaultForbiddenFlags) + * + * @internal */ protected readonly dependencyForbiddenFlags: ReadonlyArray; + /** @internal */ protected readonly dependencyOnReject: ReadonlyArray< Error | null | undefined >; @@ -347,25 +351,49 @@ export /* abstract */ class Step { return this.layerPlan.getStep(id, this); } - protected getDepOptions(depId: number): AddDependencyOptions { - const step = this.dependencies[depId]; + protected getDepOptions( + depId: number, + ): DependencyOptions { + const step = this.dependencies[depId] as TStep; const forbiddenFlags = this.dependencyForbiddenFlags[depId]; const onReject = this.dependencyOnReject[depId]; const acceptFlags = ALL_FLAGS & ~forbiddenFlags; return { step, acceptFlags, onReject }; } - protected getDep(_depId: number): T { + protected getDep( + _depId: number, + ): TStep | __FlagStep; + protected getDep( + _depId: number, + throwOnFlagged: true, + ): TStep; + protected getDep( + _depId: number, + _throwOnFlagged = false, + ): TStep | __FlagStep { // This gets replaced when `__FlagStep` is loaded. Were we on ESM we could // just put the code here, but since we're not we have to avoid the // circular dependency. throw new Error(`Grafast failed to load correctly`); } - protected maybeGetDep( + protected maybeGetDep( + depId: number | null | undefined, + ): TStep | __FlagStep | null; + protected maybeGetDep( + depId: number | null | undefined, + throwOnFlagged: true, + ): TStep | null; + protected maybeGetDep( depId: number | null | undefined, - ): T | null { - return depId == null ? null : this.getDep(depId); + throwOnFlagged = false, + ): TStep | __FlagStep | null { + return depId == null + ? null + : throwOnFlagged + ? this.getDep(depId, true) + : this.getDep(depId); } protected getDepOrConstant( diff --git a/grafast/grafast/src/steps/__flag.ts b/grafast/grafast/src/steps/__flag.ts index 77f628786..97cdb4da6 100644 --- a/grafast/grafast/src/steps/__flag.ts +++ b/grafast/grafast/src/steps/__flag.ts @@ -3,6 +3,7 @@ import { $$inhibit, flagError, SafeError } from "../error.js"; import { inspect } from "../inspect.js"; import type { AddDependencyOptions, + DataFromStep, ExecutionDetails, ExecutionEntryFlags, GrafastResultsList, @@ -89,7 +90,7 @@ function resolveTrapValue(tv: TrapValue): ResolvedTrapValue { } } -export class __FlagStep extends Step { +export class __FlagStep extends Step> { static $$export = { moduleName: "grafast", exportName: "__FlagStep", @@ -102,7 +103,7 @@ export class __FlagStep extends Step { private valueForInhibited: ResolvedTrapValue; private valueForError: ResolvedTrapValue; private canBeInlined: boolean; - constructor(step: Step, options: FlagStepOptions) { + constructor(step: TStep, options: FlagStepOptions) { super(); const { acceptFlags = DEFAULT_ACCEPT_FLAGS, @@ -207,8 +208,8 @@ export class __FlagStep extends Step { } public execute( - _details: ExecutionDetails<[data: TData, cond?: boolean]>, - ): GrafastResultsList { + _details: ExecutionDetails<[data: DataFromStep, cond?: boolean]>, + ): GrafastResultsList> { throw new Error(`${this} not finalized?`); } @@ -222,7 +223,7 @@ export class __FlagStep extends Step { } private fancyExecute( - details: ExecutionDetails<[data: TData, cond?: boolean]>, + details: ExecutionDetails<[data: DataFromStep, cond?: boolean]>, ): any { const dataEv = details.values[0]!; const condEv = @@ -268,7 +269,7 @@ export class __FlagStep extends Step { // Checks already performed via addDependency, just pass everything through. Should have been inlined! private passThroughExecute( - details: ExecutionDetails<[data: TData, cond?: boolean]>, + details: ExecutionDetails<[data: DataFromStep, cond?: boolean]>, ): any { const ev = details.values[0]; if (ev.isBatch) { @@ -284,11 +285,11 @@ export class __FlagStep extends Step { * Example use case: get user by id, but id is null: no need to fetch the user * since we know they won't exist. */ -export function inhibitOnNull( - $step: Step, +export function inhibitOnNull( + $step: TStep, options?: { if?: FlagStepOptions["if"] }, ) { - return new __FlagStep($step, { + return new __FlagStep($step, { ...options, acceptFlags: DEFAULT_ACCEPT_FLAGS & ~FLAG_NULL, }); @@ -299,20 +300,20 @@ export function inhibitOnNull( * that represents a Post instead: throw error to tell user they've sent invalid * data. */ -export function assertNotNull( - $step: Step, +export function assertNotNull( + $step: TStep, message: string, options?: { if?: FlagStepOptions["if"] }, ) { - return new __FlagStep($step, { + return new __FlagStep($step, { ...options, acceptFlags: DEFAULT_ACCEPT_FLAGS & ~FLAG_NULL, onReject: new SafeError(message), }); } -export function trap( - $step: Step, +export function trap( + $step: TStep, acceptFlags: ExecutionEntryFlags, options?: { valueForInhibited?: FlagStepOptions["valueForInhibited"]; @@ -320,18 +321,31 @@ export function trap( if?: FlagStepOptions["if"]; }, ) { - return new __FlagStep($step, { + return new __FlagStep($step, { ...options, acceptFlags: (acceptFlags & TRAPPABLE_FLAGS) | FLAG_NULL, }); } // Have to overwrite the getDep method due to circular dependency -(Step.prototype as any).getDep = function (this: Step, depId: number) { +(Step.prototype as any).getDep = function ( + this: Step, + depId: number, + throwOnFlagged = false, +) { const { step, acceptFlags, onReject } = this.getDepOptions(depId); if (acceptFlags === DEFAULT_ACCEPT_FLAGS && onReject == null) { return step; } else { + if (throwOnFlagged) { + throw new Error( + `When retrieving dependency ${step} of ${this}, the dependency is flagged as ${digestAcceptFlags( + acceptFlags, + )}/onReject=${String( + onReject, + )}. Please use \`this.getDepOptions(depId)\` instead, and handle the flags`, + ); + } // Return a __FlagStep around options.step so that all the options are preserved. return new __FlagStep(step, { acceptFlags, onReject }); } diff --git a/grafast/grafast/src/steps/__inputDynamicScalar.ts b/grafast/grafast/src/steps/__inputDynamicScalar.ts index 6772f56a9..cda7a7566 100644 --- a/grafast/grafast/src/steps/__inputDynamicScalar.ts +++ b/grafast/grafast/src/steps/__inputDynamicScalar.ts @@ -132,7 +132,7 @@ export class __InputDynamicScalarStep< /** @internal */ eval(): TLeaf { const variableValues = this.variableNames.map((variableName, i) => - this.getDep<__TrackedValueStep>(i).eval(), + this.getDep<__TrackedValueStep>(i, true).eval(), ); return this.valueFromValues(variableValues); } diff --git a/grafast/grafast/src/steps/__trackedValue.ts b/grafast/grafast/src/steps/__trackedValue.ts index 1d34da19d..4705bb06a 100644 --- a/grafast/grafast/src/steps/__trackedValue.ts +++ b/grafast/grafast/src/steps/__trackedValue.ts @@ -166,7 +166,7 @@ export class __TrackedValueStep< } private getValuePlan() { - return this.getDep<__ValueStep | AccessStep>(0); + return this.getDep<__ValueStep | AccessStep>(0, true); } /** diff --git a/grafast/grafast/src/steps/access.ts b/grafast/grafast/src/steps/access.ts index 334f8e304..af18f2266 100644 --- a/grafast/grafast/src/steps/access.ts +++ b/grafast/grafast/src/steps/access.ts @@ -160,12 +160,12 @@ export class AccessStep extends UnbatchedStep { } toStringMeta(): string { - return `${chalk.bold.yellow(String(this.getDep(0).id))}.${this.path - .map((p) => String(p)) - .join(".")}`; + return `${chalk.bold.yellow( + String(this.getDepOptions(0).step.id), + )}.${this.path.map((p) => String(p)).join(".")}`; } - getParentStep(): Step { + getParentStep() { return this.getDep(0); } diff --git a/grafast/grafast/src/steps/connection.ts b/grafast/grafast/src/steps/connection.ts index df867aa39..00ef958d1 100644 --- a/grafast/grafast/src/steps/connection.ts +++ b/grafast/grafast/src/steps/connection.ts @@ -186,7 +186,7 @@ export class ConnectionStep< }); } public getBefore(): TCursorStep | null { - return this.maybeGetDep(this._beforeDepId); + return this.maybeGetDep(this._beforeDepId, true); } public setBefore($beforePlan: Step) { if ($beforePlan instanceof ConstantStep && $beforePlan.data == null) { @@ -203,7 +203,7 @@ export class ConnectionStep< }); } public getAfter(): TCursorStep | null { - return this.maybeGetDep(this._afterDepId); + return this.maybeGetDep(this._afterDepId, true); } public setAfter($afterPlan: Step) { if ($afterPlan instanceof ConstantStep && $afterPlan.data == null) { diff --git a/grafast/grafast/src/steps/listTransform.ts b/grafast/grafast/src/steps/listTransform.ts index 6616b39f6..9061346a9 100644 --- a/grafast/grafast/src/steps/listTransform.ts +++ b/grafast/grafast/src/steps/listTransform.ts @@ -10,7 +10,11 @@ import { import type { LayerPlanReasonSubroutine } from "../engine/LayerPlan.js"; import { LayerPlan } from "../engine/LayerPlan.js"; import { withGlobalLayerPlan } from "../engine/lib/withGlobalLayerPlan.js"; -import type { ConnectionCapableStep, ExecutionDetails } from "../index.js"; +import type { + __FlagStep, + ConnectionCapableStep, + ExecutionDetails, +} from "../index.js"; import type { GrafastResultsList } from "../interfaces.js"; import { $$deepDepSkip } from "../interfaces.js"; import type { ListCapableStep } from "../step.js"; @@ -186,7 +190,7 @@ export class __ListTransformStep< } getListStep(): TListStep { - return this.getDep(this.rawListStepDepId); + return this.getDepOptions(this.rawListStepDepId).step; } [$$deepDepSkip]() { diff --git a/grafast/grafast/src/steps/object.ts b/grafast/grafast/src/steps/object.ts index 0d2097410..00a7d7d2f 100644 --- a/grafast/grafast/src/steps/object.ts +++ b/grafast/grafast/src/steps/object.ts @@ -97,10 +97,15 @@ export class ObjectStep< this.addDependency({ step: plan, skipDeduplication: true }); } + getStepForKey(key: TKey): TPlans[TKey]; + getStepForKey( + key: TKey, + allowMissing: true, + ): TPlans[TKey] | null; getStepForKey( key: TKey, allowMissing = false, - ): TKey extends keyof TPlans ? TPlans[TKey] : null { + ): TPlans[TKey] | null { const idx = this.keys.indexOf(key as string); if (idx < 0) { if (!allowMissing) { @@ -110,9 +115,9 @@ export class ObjectStep< )}' - we have no such key`, ); } - return null as any; + return null; } - return this.getDep(idx) as any; + return this.getDepOptions(idx).step; } toStringMeta(): string { @@ -316,7 +321,7 @@ ${inner} )}'; supported keys: '${this.keys.join("', '")}'`, ); } - return this.getDep(index); + return this.getDepOptions(index).step; } } diff --git a/grafast/grafast/src/steps/polymorphicBranch.ts b/grafast/grafast/src/steps/polymorphicBranch.ts index 89e9a0de5..0555db455 100644 --- a/grafast/grafast/src/steps/polymorphicBranch.ts +++ b/grafast/grafast/src/steps/polymorphicBranch.ts @@ -54,7 +54,7 @@ export class PolymorphicBranchStep planForType(objectType: GraphQLObjectType): Step { const matcher = this.matchers[objectType.name]; - const $step = this.getDep(0); + const $step = this.getDep(0, true); if (matcher) { if (typeof matcher.plan === "function") { return matcher.plan($step);