Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish the review of TODOs for the Beta #417

Merged
merged 53 commits into from
Jul 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
af956e8
Demote
benjie Jul 14, 2023
698874e
Implement an LRU
benjie Jul 14, 2023
7f58aa1
Rename inputPlan to inputStep
benjie Jul 14, 2023
6852c9f
Move the query cache onto the schema itself, so multiple schemas can …
benjie Jul 14, 2023
d5fd4b4
Move all Grafast*Extensions into Grafast namespace for easier extensi…
benjie Jul 14, 2023
edfdd10
Install query cache into schema extensions
benjie Jul 14, 2023
4861bde
Update TODOs
benjie Jul 14, 2023
0a8a046
Fix type of context option
benjie Jul 14, 2023
cf3d519
Pass all args to context callback
benjie Jul 14, 2023
516226b
Address TODOs
benjie Jul 14, 2023
ce071ab
Address more TODOs
benjie Jul 14, 2023
8b132c4
Remove duplicate behavior
benjie Jul 14, 2023
14db578
Embed NodeID Codec directly into handler, remove all separate handling
benjie Jul 14, 2023
e7dd2e0
docs(changeset): `codec` is now baked into NodeId handlers (rather th…
benjie Jul 14, 2023
1a4f4ea
Update docs
benjie Jul 14, 2023
1cade8c
Demote
benjie Jul 14, 2023
8a86e83
Move AccessStep finalization to finalize lifecycle method
benjie Jul 14, 2023
c4923da
Overhaul operation plan caching
benjie Jul 14, 2023
a2e856f
Hide secrets from plan when using constant
benjie Jul 14, 2023
15fe85b
Demote
benjie Jul 14, 2023
8f7117a
Make constant steps private by default, then mark a lot of them as no…
benjie Jul 14, 2023
341cb54
Demote
benjie Jul 14, 2023
cfda9d1
Lint fixes
benjie Jul 14, 2023
b41f349
hasNext is not data
benjie Jul 17, 2023
73e2113
Demote
benjie Jul 17, 2023
7049fab
🤷‍♂️
benjie Jul 17, 2023
ca3d639
Demote
benjie Jul 17, 2023
42a3f5f
Seems fine whree it is...
benjie Jul 17, 2023
24884f2
Address some TODOs
benjie Jul 17, 2023
e345e5b
Rename postgraphilePresetAmber to PostGraphileAmberPreset
benjie Jul 17, 2023
8fa6f45
V4 preset should use amber preset
benjie Jul 17, 2023
4203029
Don't break streamed list items
benjie Jul 17, 2023
8c58934
Demote
benjie Jul 17, 2023
fe616f1
Revert "V4 preset should use amber preset"
benjie Jul 17, 2023
9055776
Windows compat
benjie Jul 17, 2023
728a500
TODO -> NOTE
benjie Jul 17, 2023
0ee7894
Switch JWT type parser out for shared parser
benjie Jul 17, 2023
ddd0d95
Update a couple notes
benjie Jul 17, 2023
2532946
Rename deepEval to applyTransforms
benjie Jul 17, 2023
4b7a0ff
Demote
benjie Jul 17, 2023
76efcd3
Lint fixes
benjie Jul 17, 2023
1882e01
docs(changeset): `constant(foo)` no longer adds the value of `foo` to…
benjie Jul 17, 2023
8816723
docs(changeset): `deepEval` has been renamed to `applyTransforms`
benjie Jul 17, 2023
9327b97
Remove redundant argument
benjie Jul 17, 2023
502b233
docs(changeset): `preset.grafast.context` second parameter is no long…
benjie Jul 17, 2023
47f6f01
docs(changeset): Fix planning such that OutputPlan optimizations base…
benjie Jul 17, 2023
620f9e0
docs(changeset): Grafast now supports `operationsCacheMaxLength` and …
benjie Jul 17, 2023
ff4395b
docs(changeset): Grafast operation cache now tied to the schema, so m…
benjie Jul 17, 2023
e5012f9
docs(changeset): Move `GrafastFieldExtensions` to `Grafast.FieldExten…
benjie Jul 17, 2023
9a324f1
docs(changeset): Use an LRU for caching the getter from options - pre…
benjie Jul 17, 2023
f115b6f
docs(changeset): Export `parseDatabaseIdentifier`
benjie Jul 17, 2023
dc94b4a
docs(changeset): Rename 'postgraphilePresetAmber' to 'PostGraphileAmb…
benjie Jul 17, 2023
cf0940a
Tiny fixes
benjie Jul 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .changeset/early-teachers-fry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"graphile-build-pg": patch
"graphile-build": patch
"postgraphile": patch
---

`codec` is now baked into NodeId handlers (rather than using `codecName` and
looking that up in `codecs`). All related APIs have thus simplified, e.g. the
step `node(codecs, handler, $id)` is now `node(handler, $id)`, etc. TypeScript
should point out any issues you have hopefully.
8 changes: 8 additions & 0 deletions .changeset/five-moles-study.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"postgraphile": patch
"grafast": patch
---

Grafast now supports `operationsCacheMaxLength` and
`operationOperationPlansCacheMaxLength` configuration via
`schema.extensions.grafast.*`. Currently undocumented.
5 changes: 5 additions & 0 deletions .changeset/giant-spoons-give.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"graphile-build-pg": patch
---

Export `parseDatabaseIdentifier`
8 changes: 8 additions & 0 deletions .changeset/hot-snails-complain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"grafast": patch
---

`constant(foo)` no longer adds the value of `foo` to the plan diagram unless you
pass `true` as the second option (`constant(foo, true)`) or `foo` is something
very basic like `null`/`undefined`/`true`/`false`. This is to protect your
secrets.
8 changes: 8 additions & 0 deletions .changeset/khaki-lies-destroy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@dataplan/pg": patch
"graphile-build-pg": patch
"postgraphile": patch
"grafast": patch
---

`deepEval` has been renamed to `applyTransforms`
7 changes: 7 additions & 0 deletions .changeset/light-ducks-burn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"grafast": patch
---

Move `GrafastFieldExtensions` to `Grafast.FieldExtensions` and the same for most
other `Grafast*Extensions` interfaces. This is to make TypeScript declaration
merging easier.
6 changes: 6 additions & 0 deletions .changeset/loud-fishes-draw.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"grafast": patch
---

Fix planning such that OutputPlan optimizations based on `AccessPlan` can happen
after the AccessPlan is optimized.
7 changes: 7 additions & 0 deletions .changeset/real-moons-relate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"postgraphile": patch
"grafast": patch
---

Grafast operation cache now tied to the schema, so multiple schemas will not
cause degraded performance from clearing the cache.
9 changes: 9 additions & 0 deletions .changeset/real-shrimps-juggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"postgraphile": patch
"grafast": patch
---

`preset.grafast.context` second parameter is no longer the existing GraphQL
context, but instead the GraphQL request details (which contains the
`contextValue`). If you were using this (unlikely), add `.contextValue` to usage
of the second argument.
5 changes: 5 additions & 0 deletions .changeset/sharp-turkeys-remember.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"postgraphile": patch
---

Rename 'postgraphilePresetAmber' to 'PostGraphileAmberPreset'
6 changes: 6 additions & 0 deletions .changeset/small-panthers-work.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@grafserv/persisted": patch
---

Use an LRU for caching the getter from options - prevents memory exhaustion in
the case of poorly written consumer code.
2 changes: 1 addition & 1 deletion grafast/dataplan-pg/src/datasource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ export class PgResource<
step:
stepOrConstant instanceof ExecutableStep
? stepOrConstant
: constant(stepOrConstant),
: constant(stepOrConstant, false),
codec,
matches: (alias: SQL) =>
typeof attribute.expression === "function"
Expand Down
20 changes: 11 additions & 9 deletions grafast/dataplan-pg/src/examples/exampleSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1638,7 +1638,9 @@ export function makeExampleSchema(
* because it's not being ran as part of a Grafast planning context - hence
* the `if`.
*/
const $person = registry.pgResources.people.get({ person_id: constant(1) });
const $person = registry.pgResources.people.get({
person_id: constant(1, false),
});
const $posts = $person.manyRelation("posts");
const $post = $posts.single();
const $id = $post.get("post_id");
Expand Down Expand Up @@ -4139,7 +4141,7 @@ export function makeExampleSchema(
function plan(_$root, { $id }) {
const $item: SingleTableItemStep = singleTableItemsResource.get({
id: $id as ExecutableStep<number>,
type: constant("TOPIC"),
type: constant("TOPIC", false),
});
return $item;
},
Expand Down Expand Up @@ -4451,7 +4453,7 @@ export function makeExampleSchema(
attribute: "cvss_score",
callback: (alias) =>
sql`${alias} > ${$vulnerabilities.placeholder(
constant(6),
constant(6, false),
TYPES.float,
)}`,
});
Expand Down Expand Up @@ -4884,7 +4886,7 @@ export function makeExampleSchema(
function plan(_$root, fieldArgs) {
const $item = pgInsertSingle(relationalItemsResource, {
type: constant`POST`,
author_id: constant(2),
author_id: constant(2, false),
});
const $itemId = $item.get("id");
const $post = pgInsertSingle(relationalPostsResource, {
Expand Down Expand Up @@ -4941,13 +4943,13 @@ export function makeExampleSchema(
for (let i = 0; i < 3; i++) {
const $item = pgInsertSingle(relationalItemsResource, {
type: constant`POST`,
author_id: constant(2),
author_id: constant(2, false),
});
const $itemId = $item.get("id");
$post = pgInsertSingle(relationalPostsResource, {
id: $itemId,
title: constant(`Post #${i + 1}`),
description: constant(`Desc ${i + 1}`),
title: constant(`Post #${i + 1}`, false),
description: constant(`Desc ${i + 1}`, false),
note: constant(null),
});
}
Expand Down Expand Up @@ -4983,11 +4985,11 @@ export function makeExampleSchema(
sql`interfaces_and_unions.insert_post(${authorId.placeholder}, ${title.placeholder})`,
args: [
{
step: constant(2),
step: constant(2, false),
pgCodec: TYPES.int,
},
{
step: constant(`Computed post #${i + 1}`),
step: constant(`Computed post #${i + 1}`, false),
pgCodec: TYPES.text,
},
],
Expand Down
4 changes: 2 additions & 2 deletions grafast/dataplan-pg/src/steps/pgSelect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ import {
__ItemStep,
__TrackedValueStep,
access,
applyTransforms,
arrayOfLength,
ConnectionStep,
constant,
ConstantStep,
deepEval,
ExecutableStep,
exportAs,
first,
Expand Down Expand Up @@ -786,7 +786,7 @@ export class PgSelectStep<
);
}

const $evalledStep = deepEval($step);
const $evalledStep = applyTransforms($step);

const dependencyIndex = this.addDependency($evalledStep);
const symbol = Symbol(`step-${$step.id}`);
Expand Down
2 changes: 1 addition & 1 deletion grafast/dataplan-pg/src/steps/pgUnionAll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1519,7 +1519,7 @@ and ${condition(i + 1)}`}
optimize() {
if (this.memberDigests.length === 0) {
// We have no implementations, we'll never return anything
return constant([]);
return constant([], false);
}

this.planLimitAndOffset();
Expand Down
8 changes: 4 additions & 4 deletions grafast/grafast/src/args.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@ export function hookArgs(
const finalize = (args: ExecutionArgs) => {
const userContext = resolvedPreset.grafast?.context;
if (typeof userContext === "function") {
const result = userContext(ctx, args.contextValue as Record<string, any>);
const result = userContext(ctx, args);
if (isPromiseLike(result)) {
// Deliberately shadowed 'result'
return result.then((result) => {
Object.assign(args.contextValue as Record<string, any>, result);
Object.assign(args.contextValue as Partial<Grafast.Context>, result);
return args;
});
} else {
Object.assign(args.contextValue as Record<string, any>, result);
Object.assign(args.contextValue as Partial<Grafast.Context>, result);
return args;
}
} else if (typeof userContext === "object" && userContext !== null) {
Object.assign(args.contextValue as Record<string, any>, userContext);
Object.assign(args.contextValue as Partial<Grafast.Context>, userContext);
return args;
} else {
return args;
Expand Down
36 changes: 22 additions & 14 deletions grafast/grafast/src/engine/OperationPlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
object,
SafeError,
} from "../index.js";
import { inputPlan } from "../input.js";
import { inputStep } from "../input.js";
import { inspect } from "../inspect.js";
import type {
FieldPlanResolver,
Expand Down Expand Up @@ -387,17 +387,9 @@ export class OperationPlan {
this.checkTimeout();
this.lap("optimizeSteps");

// Replace access plans with direct access, etc
te.batch(() => {
this.optimizeOutputPlans();
});

this.checkTimeout();
this.lap("optimizeOutputPlans");

this.phase = "finalize";

this.stepTracker.finalize();
this.stepTracker.finalizeSteps();

// Get rid of steps that are no longer needed after optimising outputPlans
// (we shouldn't see any new steps or dependencies after here)
Expand All @@ -420,6 +412,22 @@ export class OperationPlan {

this.lap("finalizeSteps");

// Replace access plans with direct access, etc (must come after finalizeSteps)
te.batch(() => {
this.optimizeOutputPlans();
});

this.checkTimeout();
this.lap("optimizeOutputPlans");

// AccessSteps may have been removed
this.stepTracker.treeShakeSteps();

this.checkTimeout();
this.lap("treeShakeSteps", "optimizeOutputPlans");

this.stepTracker.finalizeOutputPlans();

te.batch(() => {
this.finalizeLayerPlans();
});
Expand Down Expand Up @@ -1169,6 +1177,7 @@ export class OperationPlan {
// ENHANCEMENT: should we do `step = parentStep.object()` (i.e.
// `$pgSelectSingle.record()`) or similar for "opaque" steps to become
// suitable for consumption by resolvers?
// Maybe `parentStep.forResolver()` or `parentStep.hydrate()` or `parentStep.toFullObject()`?
step = parentStep;
}

Expand Down Expand Up @@ -1701,9 +1710,8 @@ export class OperationPlan {
}
for (const t of allPossibleObjectTypes) {
if (!layerPlan.reason.typeNames.includes(t.name)) {
// TODO: do I need to do anything extra here? Since we're re-using an
// existing LayerPlan, we should be careful to ensure none of the
// previous assumptions have been broken.
// Since we're re-using an existing LayerPlan, we should be careful to
// ensure none of the previous assumptions have been broken.
layerPlan.reason.typeNames.push(t.name);
}
}
Expand Down Expand Up @@ -1885,7 +1893,7 @@ export class OperationPlan {
const argumentValue = argumentValues?.find(
(v) => v.name.value === argumentName,
);
const argumentPlan = inputPlan(
const argumentPlan = inputStep(
this,
argumentType,
argumentValue?.value,
Expand Down
2 changes: 1 addition & 1 deletion grafast/grafast/src/engine/OutputPlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1230,7 +1230,7 @@ const arrayExecutorString_nonNullable_streaming = makeArrayExecutor(
* workaround until we can afford the time to write our own introspection
* execution.
*
* TODO: write our own introspection execution!
* ENHANCE: write our own introspection execution!
*/
const introspect = (
root: PayloadRoot,
Expand Down
39 changes: 27 additions & 12 deletions grafast/grafast/src/engine/StepTracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,34 @@ export class StepTracker {
return (this.stepById as ExecutableStep[]).slice(oldStepCount);
}

/** Called when OperationPlan enters finalize phase. */
public finalize() {
const forbidden = () => {
throw new Error(`Forbidden after 'finalize' phase`);
private forbid(
key: keyof {
[K in keyof StepTracker as StepTracker[K] extends (...args: any[]) => any
? K
: never]: any;
},
) {
this[key] = () => {
throw new Error(
`StepTracker.${key as string}(...) is forbidden after 'finalize' phase`,
);
};
this.addStep = forbidden;
this.addLayerPlan = forbidden;
this.addOutputPlan = forbidden;
this.deleteLayerPlan = forbidden;
this.addStepDependency = forbidden;
this.setOutputPlanRootStep = forbidden;
this.setLayerPlanRootStep = forbidden;
this.replaceStep = forbidden;
}

/** Called when OperationPlan enters finalize phase. */
public finalizeSteps() {
this.forbid("addStep");
this.forbid("addLayerPlan");
this.forbid("addOutputPlan");
this.forbid("deleteLayerPlan");
this.forbid("addStepDependency");
this.forbid("setLayerPlanRootStep");
this.forbid("replaceStep");
}

/** Called when OperationPlan is about to finalize the output plans. */
public finalizeOutputPlans() {
this.forbid("setOutputPlanRootStep");
}

public addStep($step: ExecutableStep): number {
Expand Down
Loading
Loading