diff --git a/.changeset/silly-baths-design.md b/.changeset/silly-baths-design.md new file mode 100644 index 0000000000..066c227eae --- /dev/null +++ b/.changeset/silly-baths-design.md @@ -0,0 +1,8 @@ +--- +"@dataplan/pg": patch +"grafast": patch +"postgraphile": patch +--- + +Fix a bug where `trap()` would prevent connection-capable list steps from +resolving correctly (often replacing them with `null`). diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.deopt.mermaid b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.deopt.mermaid new file mode 100644 index 0000000000..5d71c6ae45 --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.deopt.mermaid @@ -0,0 +1,73 @@ +%%{init: {'themeVariables': { 'fontSize': '12px'}}}%% +graph TD + classDef path fill:#eee,stroke:#000,color:#000 + classDef plan fill:#fff,stroke-width:1px,color:#000 + classDef itemplan fill:#fff,stroke-width:2px,color:#000 + classDef unbatchedplan fill:#dff,stroke-width:1px,color:#000 + classDef sideeffectplan fill:#fcc,stroke-width:2px,color:#000 + classDef bucket fill:#f6f6f6,color:#000,stroke-width:2px,text-align:left + + subgraph "Buckets for queries/trap/trapped-empty-pg-select" + Bucket0("Bucket 0 (root)

1: Error[27], Error[34]
ᐳ: 6, 10, 11, 14, 7, 12, 15
2: InhibitIf[8], InhibitIf[16]
3: PgSelect[9], PgSelect[17]
4: PgSelectRows[35], PgSelectRows[37]
5: __Flag[36], __Flag[38]"):::bucket + Bucket1("Bucket 1 (listItem)

ROOT __Item{1}ᐸ36ᐳ[39]"):::bucket + Bucket2("Bucket 2 (listItem)

ROOT __Item{2}ᐸ38ᐳ[41]"):::bucket + Bucket3("Bucket 3 (nullableBoundary)
Deps: 40

ROOT PgSelectSingle{1}ᐸmessagesᐳ[40]"):::bucket + Bucket4("Bucket 4 (nullableBoundary)
Deps: 42

ROOT PgSelectSingle{2}ᐸmessagesᐳ[42]"):::bucket + end + Bucket0 --> Bucket1 & Bucket2 + Bucket1 --> Bucket3 + Bucket2 --> Bucket4 + + %% plan dependencies + InhibitIf8[["InhibitIf[8∈0] ➊
ᐸ6, if(7)ᐳ
More deps:
- Constantᐸ[]ᐳ[6]"]]:::plan + Lambda7{{"Lambda[7∈0] ➊
ᐸisEmptyᐳ
More deps:
- Constantᐸ[]ᐳ[6]"}}:::plan + Lambda7 --> InhibitIf8 + PgSelect9[["PgSelect[9∈0] ➊
ᐸmessagesᐳ"]]:::plan + Object12{{"Object[12∈0] ➊
ᐸ{pgSettings,withPgClient}ᐳ"}}:::plan + Object12 & InhibitIf8 --> PgSelect9 + Access10{{"Access[10∈0] ➊
ᐸ2.pgSettingsᐳ"}}:::plan + Access11{{"Access[11∈0] ➊
ᐸ2.withPgClientᐳ"}}:::plan + Access10 & Access11 --> Object12 + InhibitIf16[["InhibitIf[16∈0] ➊
ᐸ14, if(15)ᐳ
More deps:
- Constantᐸ[ 'ca70ca70-0000-0000-0000-a11ce000ca70' ]ᐳ[14]"]]:::plan + Lambda15{{"Lambda[15∈0] ➊
ᐸisEmptyᐳ
More deps:
- Constantᐸ[ 'ca70ca70-0000-0000-0000-a11ce000ca70' ]ᐳ[14]"}}:::plan + Lambda15 --> InhibitIf16 + PgSelect17[["PgSelect[17∈0] ➊
ᐸmessagesᐳ"]]:::plan + Object12 & InhibitIf16 --> PgSelect17 + __Value2["__Value[2∈0] ➊
ᐸcontextᐳ"]:::plan + __Value2 --> Access10 + __Value2 --> Access11 + PgSelectRows35[["PgSelectRows[35∈0] ➊"]]:::plan + PgSelect9 --> PgSelectRows35 + __Flag36[["__Flag[36∈0] ➊
ᐸ35, trapInhibited, onReject: INHIBITᐳ"]]:::plan + PgSelectRows35 -->|"trapError;trapInhibited"| __Flag36 + PgSelectRows37[["PgSelectRows[37∈0] ➊"]]:::plan + PgSelect17 --> PgSelectRows37 + __Flag38[["__Flag[38∈0] ➊
ᐸ37, trapInhibited, onReject: INHIBITᐳ"]]:::plan + PgSelectRows37 -->|"trapError;trapInhibited"| __Flag38 + Error27[["Error[27∈0] ➊"]]:::unbatchedplan + Error34[["Error[34∈0] ➊"]]:::unbatchedplan + __Item39[/"__Item[39∈1]
ᐸ36ᐳ"\]:::itemplan + __Flag36 ==> __Item39 + PgSelectSingle40{{"PgSelectSingle[40∈1]
ᐸmessagesᐳ"}}:::plan + __Item39 --> PgSelectSingle40 + __Item41[/"__Item[41∈2]
ᐸ38ᐳ"\]:::itemplan + __Flag38 ==> __Item41 + PgSelectSingle42{{"PgSelectSingle[42∈2]
ᐸmessagesᐳ"}}:::plan + __Item41 --> PgSelectSingle42 + PgClassExpression43{{"PgClassExpression[43∈3]
ᐸ__messages__.”id”ᐳ"}}:::plan + PgSelectSingle40 --> PgClassExpression43 + PgClassExpression44{{"PgClassExpression[44∈4]
ᐸ__messages__.”id”ᐳ"}}:::plan + PgSelectSingle42 --> PgClassExpression44 + + %% define steps + classDef bucket0 stroke:#696969 + class Bucket0,__Value2,Lambda7,InhibitIf8,PgSelect9,Access10,Access11,Object12,Lambda15,InhibitIf16,PgSelect17,Error27,Error34,PgSelectRows35,__Flag36,PgSelectRows37,__Flag38 bucket0 + classDef bucket1 stroke:#00bfff + class Bucket1,__Item39,PgSelectSingle40 bucket1 + classDef bucket2 stroke:#7f007f + class Bucket2,__Item41,PgSelectSingle42 bucket2 + classDef bucket3 stroke:#ffa500 + class Bucket3,PgClassExpression43 bucket3 + classDef bucket4 stroke:#0000ff + class Bucket4,PgClassExpression44 bucket4 + diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.deopt.sql b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.deopt.sql new file mode 100644 index 0000000000..528042e4f0 --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.deopt.sql @@ -0,0 +1,10 @@ +select + __messages__."id" as "0" +from app_public.messages as __messages__ +where + ( + __messages__.id = any($1::"uuid"[]) + ) and ( + true /* authorization checks */ + ) +order by __messages__."id" asc; diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.errors.json5 b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.errors.json5 new file mode 100644 index 0000000000..869c06f83d --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.errors.json5 @@ -0,0 +1,26 @@ +[ + { + message: 'connection() cannot be used with trap() or inhibit...() at this time.', + locations: [ + { + line: 26, + column: 3, + }, + ], + path: [ + 'connection', + ], + }, + { + message: 'connection() cannot be used with trap() or inhibit...() at this time.', + locations: [ + { + line: 8, + column: 3, + }, + ], + path: [ + 'emptyConnection', + ], + }, +] \ No newline at end of file diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.json5 b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.json5 new file mode 100644 index 0000000000..4747756426 --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.json5 @@ -0,0 +1,10 @@ +{ + emptyList: [], + list: [ + { + id: "ca70ca70-0000-0000-0000-a11ce000ca70", + }, + ], + emptyConnection: null, + connection: null, +} diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.mermaid b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.mermaid new file mode 100644 index 0000000000..5d71c6ae45 --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.mermaid @@ -0,0 +1,73 @@ +%%{init: {'themeVariables': { 'fontSize': '12px'}}}%% +graph TD + classDef path fill:#eee,stroke:#000,color:#000 + classDef plan fill:#fff,stroke-width:1px,color:#000 + classDef itemplan fill:#fff,stroke-width:2px,color:#000 + classDef unbatchedplan fill:#dff,stroke-width:1px,color:#000 + classDef sideeffectplan fill:#fcc,stroke-width:2px,color:#000 + classDef bucket fill:#f6f6f6,color:#000,stroke-width:2px,text-align:left + + subgraph "Buckets for queries/trap/trapped-empty-pg-select" + Bucket0("Bucket 0 (root)

1: Error[27], Error[34]
ᐳ: 6, 10, 11, 14, 7, 12, 15
2: InhibitIf[8], InhibitIf[16]
3: PgSelect[9], PgSelect[17]
4: PgSelectRows[35], PgSelectRows[37]
5: __Flag[36], __Flag[38]"):::bucket + Bucket1("Bucket 1 (listItem)

ROOT __Item{1}ᐸ36ᐳ[39]"):::bucket + Bucket2("Bucket 2 (listItem)

ROOT __Item{2}ᐸ38ᐳ[41]"):::bucket + Bucket3("Bucket 3 (nullableBoundary)
Deps: 40

ROOT PgSelectSingle{1}ᐸmessagesᐳ[40]"):::bucket + Bucket4("Bucket 4 (nullableBoundary)
Deps: 42

ROOT PgSelectSingle{2}ᐸmessagesᐳ[42]"):::bucket + end + Bucket0 --> Bucket1 & Bucket2 + Bucket1 --> Bucket3 + Bucket2 --> Bucket4 + + %% plan dependencies + InhibitIf8[["InhibitIf[8∈0] ➊
ᐸ6, if(7)ᐳ
More deps:
- Constantᐸ[]ᐳ[6]"]]:::plan + Lambda7{{"Lambda[7∈0] ➊
ᐸisEmptyᐳ
More deps:
- Constantᐸ[]ᐳ[6]"}}:::plan + Lambda7 --> InhibitIf8 + PgSelect9[["PgSelect[9∈0] ➊
ᐸmessagesᐳ"]]:::plan + Object12{{"Object[12∈0] ➊
ᐸ{pgSettings,withPgClient}ᐳ"}}:::plan + Object12 & InhibitIf8 --> PgSelect9 + Access10{{"Access[10∈0] ➊
ᐸ2.pgSettingsᐳ"}}:::plan + Access11{{"Access[11∈0] ➊
ᐸ2.withPgClientᐳ"}}:::plan + Access10 & Access11 --> Object12 + InhibitIf16[["InhibitIf[16∈0] ➊
ᐸ14, if(15)ᐳ
More deps:
- Constantᐸ[ 'ca70ca70-0000-0000-0000-a11ce000ca70' ]ᐳ[14]"]]:::plan + Lambda15{{"Lambda[15∈0] ➊
ᐸisEmptyᐳ
More deps:
- Constantᐸ[ 'ca70ca70-0000-0000-0000-a11ce000ca70' ]ᐳ[14]"}}:::plan + Lambda15 --> InhibitIf16 + PgSelect17[["PgSelect[17∈0] ➊
ᐸmessagesᐳ"]]:::plan + Object12 & InhibitIf16 --> PgSelect17 + __Value2["__Value[2∈0] ➊
ᐸcontextᐳ"]:::plan + __Value2 --> Access10 + __Value2 --> Access11 + PgSelectRows35[["PgSelectRows[35∈0] ➊"]]:::plan + PgSelect9 --> PgSelectRows35 + __Flag36[["__Flag[36∈0] ➊
ᐸ35, trapInhibited, onReject: INHIBITᐳ"]]:::plan + PgSelectRows35 -->|"trapError;trapInhibited"| __Flag36 + PgSelectRows37[["PgSelectRows[37∈0] ➊"]]:::plan + PgSelect17 --> PgSelectRows37 + __Flag38[["__Flag[38∈0] ➊
ᐸ37, trapInhibited, onReject: INHIBITᐳ"]]:::plan + PgSelectRows37 -->|"trapError;trapInhibited"| __Flag38 + Error27[["Error[27∈0] ➊"]]:::unbatchedplan + Error34[["Error[34∈0] ➊"]]:::unbatchedplan + __Item39[/"__Item[39∈1]
ᐸ36ᐳ"\]:::itemplan + __Flag36 ==> __Item39 + PgSelectSingle40{{"PgSelectSingle[40∈1]
ᐸmessagesᐳ"}}:::plan + __Item39 --> PgSelectSingle40 + __Item41[/"__Item[41∈2]
ᐸ38ᐳ"\]:::itemplan + __Flag38 ==> __Item41 + PgSelectSingle42{{"PgSelectSingle[42∈2]
ᐸmessagesᐳ"}}:::plan + __Item41 --> PgSelectSingle42 + PgClassExpression43{{"PgClassExpression[43∈3]
ᐸ__messages__.”id”ᐳ"}}:::plan + PgSelectSingle40 --> PgClassExpression43 + PgClassExpression44{{"PgClassExpression[44∈4]
ᐸ__messages__.”id”ᐳ"}}:::plan + PgSelectSingle42 --> PgClassExpression44 + + %% define steps + classDef bucket0 stroke:#696969 + class Bucket0,__Value2,Lambda7,InhibitIf8,PgSelect9,Access10,Access11,Object12,Lambda15,InhibitIf16,PgSelect17,Error27,Error34,PgSelectRows35,__Flag36,PgSelectRows37,__Flag38 bucket0 + classDef bucket1 stroke:#00bfff + class Bucket1,__Item39,PgSelectSingle40 bucket1 + classDef bucket2 stroke:#7f007f + class Bucket2,__Item41,PgSelectSingle42 bucket2 + classDef bucket3 stroke:#ffa500 + class Bucket3,PgClassExpression43 bucket3 + classDef bucket4 stroke:#0000ff + class Bucket4,PgClassExpression44 bucket4 + diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.sql b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.sql new file mode 100644 index 0000000000..528042e4f0 --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.sql @@ -0,0 +1,10 @@ +select + __messages__."id" as "0" +from app_public.messages as __messages__ +where + ( + __messages__.id = any($1::"uuid"[]) + ) and ( + true /* authorization checks */ + ) +order by __messages__."id" asc; diff --git a/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.test.graphql b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.test.graphql new file mode 100644 index 0000000000..6f1bb8c0e9 --- /dev/null +++ b/grafast/dataplan-pg/__tests__/queries/trap/trapped-empty-pg-select.test.graphql @@ -0,0 +1,54 @@ +## expect(errors).toHaveLength(2) +## expect(data.emptyList).toEqual([]) +## expect(data.list).toEqual([{ id: "ca70ca70-0000-0000-0000-a11ce000ca70" }]) +#> dontLogErrors: true +{ + emptyList: trappedMessages(ids: []) { + id + } + list: trappedMessages(ids: ["ca70ca70-0000-0000-0000-a11ce000ca70"]) { + id + } + emptyConnection: trappedMessagesConnection(ids: [], first: 1) { + edges { + cursor + node { + id + } + } + nodes { + id + } + pageInfo { + hasNextPage + hasPreviousPage + startCursor + endCursor + } + totalCount + } + connection: trappedMessagesConnection( + ids: [ + "ca70ca70-0000-0000-0000-a11ce000ca70" + "ca70ca70-0000-0000-0000-b0b00000ca70" + ] + first: 1 + ) { + edges { + cursor + node { + id + } + } + nodes { + id + } + pageInfo { + hasNextPage + hasPreviousPage + startCursor + endCursor + } + totalCount + } +} diff --git a/grafast/dataplan-pg/src/examples/exampleSchema.ts b/grafast/dataplan-pg/src/examples/exampleSchema.ts index a7b6622250..40b00fe44f 100644 --- a/grafast/dataplan-pg/src/examples/exampleSchema.ts +++ b/grafast/dataplan-pg/src/examples/exampleSchema.ts @@ -45,6 +45,7 @@ import { graphqlHasStreamDefer, GraphQLStreamDirective, groupBy, + inhibitOnEmpty, inhibitOnNull, lambda, listen, @@ -53,6 +54,8 @@ import { newObjectTypeBuilder, object, Step, + trap, + TRAP_INHIBITED, } from "grafast"; import type { GraphQLOutputType } from "grafast/graphql"; import { @@ -4047,6 +4050,103 @@ export function makeExampleSchema( ), }, + trappedMessages: { + type: new GraphQLList(Message), + args: { + ids: { + type: new GraphQLNonNull( + new GraphQLList(new GraphQLNonNull(GraphQLString)), + ), + }, + }, + plan: EXPORTABLE( + ( + inhibitOnEmpty, + listOfCodec, + messageResource, + trap, + TRAP_INHIBITED, + TYPES, + ) => + function plan(_$root, { $ids }) { + const $nonEmptyIds = inhibitOnEmpty($ids); + const $messages = messageResource.find(); + $messages.where( + (sql) => + sql`${$messages.alias}.id = any(${$messages.placeholder($nonEmptyIds, listOfCodec(TYPES.uuid))})`, + ); + return trap($messages, TRAP_INHIBITED, { + valueForInhibited: "EMPTY_LIST", + }); + }, + [ + inhibitOnEmpty, + listOfCodec, + messageResource, + trap, + TRAP_INHIBITED, + TYPES, + ], + ), + }, + + trappedMessagesConnection: { + type: MessagesConnection, + args: { + ids: { + type: new GraphQLNonNull( + new GraphQLList(new GraphQLNonNull(GraphQLString)), + ), + }, + first: { + type: GraphQLInt, + applyPlan: EXPORTABLE( + () => + function plan(_$root, $connection: MessageConnectionStep, arg) { + $connection.setFirst(arg.getRaw()); + }, + [], + ), + }, + }, + plan: EXPORTABLE( + ( + connection, + inhibitOnEmpty, + listOfCodec, + messageResource, + sql, + trap, + TRAP_INHIBITED, + TYPES, + ) => + function plan(_$root, { $ids }) { + const $nonEmptyIds = inhibitOnEmpty($ids); + const $messages = messageResource.find(); + // Prevent deduplication with trappedMessages + $messages.where(sql`true /* connection */`); + $messages.where( + (sql) => + sql`${$messages}.id = any(${$messages.placeholder($nonEmptyIds, listOfCodec(TYPES.uuid))})`, + ); + const $trappedMessages = trap($messages, TRAP_INHIBITED, { + valueForInhibited: "EMPTY_LIST", + }); + return connection($trappedMessages); + }, + [ + connection, + inhibitOnEmpty, + listOfCodec, + messageResource, + sql, + trap, + TRAP_INHIBITED, + TYPES, + ], + ), + }, + uniqueAuthorCount: { type: GraphQLInt, args: { diff --git a/grafast/grafast/src/steps/__flag.ts b/grafast/grafast/src/steps/__flag.ts index 825648373c..809c346d29 100644 --- a/grafast/grafast/src/steps/__flag.ts +++ b/grafast/grafast/src/steps/__flag.ts @@ -17,10 +17,13 @@ import type { ExecutionDetails, ExecutionEntryFlags, GrafastResultsList, + Maybe, } from "../interfaces.ts"; import { isListCapableStep, Step } from "../step.ts"; import { sudo } from "../utils.ts"; import type { __ItemStep } from "./__item.ts"; +import type { StepWithItems } from "./connection.ts"; +import { itemsOrStep } from "./connection.ts"; // PUBLIC FLAGS export const TRAP_ERROR = FLAG_ERROR as ExecutionEntryFlags; @@ -91,7 +94,10 @@ function resolveTrapValue(tv: TrapValue): ResolvedTrapValue { } } -export class __FlagStep extends Step> { +export class __FlagStep + extends Step> + implements StepWithItems +{ static $$export = { moduleName: "grafast", exportName: "__FlagStep", @@ -104,16 +110,24 @@ export class __FlagStep extends Step> { private valueForInhibited: ResolvedTrapValue; private valueForError: ResolvedTrapValue; private canBeInlined: boolean; + private baseOptions: Omit; constructor(step: TStep, options: FlagStepOptions) { super(); const { acceptFlags = DEFAULT_ACCEPT_FLAGS, onReject, dataOnly, - if: $cond, valueForInhibited = "PASS_THROUGH", valueForError = "PASS_THROUGH", + if: $cond, } = options; + this.baseOptions = { + acceptFlags, + onReject, + dataOnly, + valueForInhibited, + valueForError, + }; this.forbiddenFlags = ALL_FLAGS & ~acceptFlags; this.onRejectReturnValue = onReject == null ? $$inhibit : flagError(onReject, step.id); @@ -155,12 +169,31 @@ export class __FlagStep extends Step> { [$$deepDepSkip](): Step { return this.getDepOptions(0).step; } + listItem?: ($item: __ItemStep) => Step; // Copied over listItem if the dependent step is a list capable step _listItem($item: __ItemStep) { const $dep = this.dependencies[0]; return isListCapableStep($dep) ? $dep.listItem($item) : $item; } + + /** + * Makes `__FlagStep` compatible with `ConnectionStep`; importantly, this + * copies our flagging over to the derived step. + */ + public items(): Step> { + const $dep = this.getDepOptions(0).step; + const $items = itemsOrStep($dep); + if ($dep === $items) { + // If the underlying step didn't use `.items()` then we don't need to + // re-wrap, avoid creating more steps and just return ourself. + return this; + } + const $if = + this.ifDep != null ? this.getDepOptions(this.ifDep).step : undefined; + return new __FlagStep($items, { ...this.baseOptions, if: $if }); + } + /** Return inlining instructions if we can be inlined. @internal */ inline( options: Omit, diff --git a/grafast/grafast/src/steps/connection.ts b/grafast/grafast/src/steps/connection.ts index 48ec1216c6..f6ae208554 100644 --- a/grafast/grafast/src/steps/connection.ts +++ b/grafast/grafast/src/steps/connection.ts @@ -20,6 +20,7 @@ import { maybeArraysMatch, terminateIterable, } from "../utils.ts"; +import { __FlagStep } from "./__flag.ts"; import { access } from "./access.ts"; import { constant, ConstantStep } from "./constant.ts"; import { each } from "./each.ts"; @@ -1205,6 +1206,11 @@ export function connection< TCursorValue, TCollectionStep > { + if (step instanceof __FlagStep) { + throw new Error( + "connection() cannot be used with trap() or inhibit...() at this time.", + ); + } if (typeof params === "function" || params?.nodePlan || params?.cursorPlan) { throw new Error( `connection() was completely overhauled during the beta; this usage is no longer supported. Usage is much more straightforward now.`, @@ -1231,7 +1237,7 @@ export function connection< return $connection; } -interface StepWithItems extends Step { +export interface StepWithItems extends Step { items(): Step>>; } export type ItemsStep> =