From 7051698ba60d31974d2c1ad92d3026508662e48a Mon Sep 17 00:00:00 2001 From: Tim Griesser Date: Tue, 21 Feb 2023 12:18:08 -0500 Subject: [PATCH 1/8] failing tests for id coercion --- .../examples/global-connection-fields/schema/numbers.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts b/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts index bc838d94e..d2c90ba6a 100644 --- a/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts +++ b/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts @@ -5,6 +5,9 @@ class NumberThing { id: number; constructor(n: number) { + if (typeof n !== 'number') { + throw new Error(`Expected NumberThing to receive number, saw ${typeof n} ${n}`); + } this.id = n; } } @@ -13,6 +16,9 @@ class BatchLoadableNumberThing { id: number; constructor(n: number) { + if (typeof n !== 'number') { + throw new Error(`Expected BatchLoadableNumberThing to receive number, saw ${typeof n} ${n}`); + } this.id = n; } } From 9761028ba0b7f47f1df317a5149ec22e0cc79f92 Mon Sep 17 00:00:00 2001 From: Tim Griesser Date: Tue, 21 Feb 2023 12:29:43 -0500 Subject: [PATCH 2/8] Throw on schema numbers --- .../examples/global-connection-fields/schema/numbers.ts | 6 ++++-- .../plugin-relay/tests/examples/relay/schema/numbers.ts | 8 ++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts b/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts index d2c90ba6a..12618e4f3 100644 --- a/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts +++ b/packages/plugin-relay/tests/examples/global-connection-fields/schema/numbers.ts @@ -6,7 +6,7 @@ class NumberThing { constructor(n: number) { if (typeof n !== 'number') { - throw new Error(`Expected NumberThing to receive number, saw ${typeof n} ${n}`); + throw new TypeError(`Expected NumberThing to receive number, saw ${typeof n} ${n}`); } this.id = n; } @@ -17,7 +17,9 @@ class BatchLoadableNumberThing { constructor(n: number) { if (typeof n !== 'number') { - throw new Error(`Expected BatchLoadableNumberThing to receive number, saw ${typeof n} ${n}`); + throw new TypeError( + `Expected BatchLoadableNumberThing to receive number, saw ${typeof n} ${n}`, + ); } this.id = n; } diff --git a/packages/plugin-relay/tests/examples/relay/schema/numbers.ts b/packages/plugin-relay/tests/examples/relay/schema/numbers.ts index d80b301e0..e7a5846a4 100644 --- a/packages/plugin-relay/tests/examples/relay/schema/numbers.ts +++ b/packages/plugin-relay/tests/examples/relay/schema/numbers.ts @@ -5,6 +5,9 @@ class NumberThing { id: number; constructor(n: number) { + if (typeof n !== 'number') { + throw new TypeError(`Expected NumberThing to receive number, saw ${typeof n} ${n}`); + } this.id = n; } } @@ -13,6 +16,11 @@ class BatchLoadableNumberThing { id: number; constructor(n: number) { + if (typeof n !== 'number') { + throw new TypeError( + `Expected BatchLoadableNumberThing to receive number, saw ${typeof n} ${n}`, + ); + } this.id = n; } } From 50850af39b740b650872417daae025a82975f2f5 Mon Sep 17 00:00:00 2001 From: Tim Griesser Date: Tue, 21 Feb 2023 12:54:19 -0500 Subject: [PATCH 3/8] fix some of the failures --- packages/plugin-relay/src/input-field-builder.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/plugin-relay/src/input-field-builder.ts b/packages/plugin-relay/src/input-field-builder.ts index b3906bbe2..ec1d47200 100644 --- a/packages/plugin-relay/src/input-field-builder.ts +++ b/packages/plugin-relay/src/input-field-builder.ts @@ -37,7 +37,7 @@ inputFieldBuilder.globalIDList = function globalIDList[] )?.map((type: ObjectRef) => ({ typename: this.builder.configStore.getTypeConfig(type).name, - parse: type instanceof NodeRef ? type.parseId : undefined, + parseId: type instanceof NodeRef ? type.parseId : undefined, })) ?? null, }, }) as never; @@ -60,7 +60,7 @@ inputFieldBuilder.globalID = function globalID( (Array.isArray(forTypes) ? forTypes : [forTypes])) as ObjectRef[] )?.map((type: ObjectRef) => ({ typename: this.builder.configStore.getTypeConfig(type).name, - parse: type instanceof NodeRef ? type.parseId : undefined, + parseId: type instanceof NodeRef ? type.parseId : undefined, })) ?? null, }, }) as unknown as InputFieldRef< From 405be796e5b3b049bb501107e607ff58f324dd7d Mon Sep 17 00:00:00 2001 From: Tim Griesser Date: Tue, 21 Feb 2023 13:07:58 -0500 Subject: [PATCH 4/8] fix additional failures --- packages/deno/packages/plugin-relay/field-builder.ts | 2 +- packages/deno/packages/plugin-relay/index.ts | 2 +- .../deno/packages/plugin-relay/input-field-builder.ts | 4 ++-- packages/deno/packages/plugin-tracing/README.md | 8 +------- packages/plugin-relay/src/field-builder.ts | 2 +- packages/plugin-relay/src/index.ts | 2 +- 6 files changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/deno/packages/plugin-relay/field-builder.ts b/packages/deno/packages/plugin-relay/field-builder.ts index 686e8bf04..8061eec2d 100644 --- a/packages/deno/packages/plugin-relay/field-builder.ts +++ b/packages/deno/packages/plugin-relay/field-builder.ts @@ -80,7 +80,7 @@ fieldBuilderProto.nodeList = function nodeList({ ids, ...options }) { const globalIds = rawIds.map((id) => typeof id === "string" ? internalDecodeGlobalID(this.builder, id, context, info, true) : id && { - id: String(id.id), + id: id.id, typename: this.builder.configStore.getTypeConfig(id.type).name, }); return resolveNodes(this.builder, context, info, globalIds); diff --git a/packages/deno/packages/plugin-relay/index.ts b/packages/deno/packages/plugin-relay/index.ts index 577b60483..cfb2cd8b0 100644 --- a/packages/deno/packages/plugin-relay/index.ts +++ b/packages/deno/packages/plugin-relay/index.ts @@ -25,7 +25,7 @@ export class PothosRelayPlugin extends BasePlugin internalDecodeGlobalID(this.builder, String(globalID), ctx, info, Array.isArray(mappings.value) ? mappings.value : false)); + const argMapper = createInputValueMapper(argMappings, (globalID, mappings, ctx: Types["Context"], info: GraphQLResolveInfo) => internalDecodeGlobalID(this.builder, String(globalID), ctx, info, Array.isArray(mappings.value) ? mappings.value : true)); return (parent, args, context, info) => resolver(parent, argMapper(args, undefined, context, info), context, info); } override wrapSubscribe(subscribe: GraphQLFieldResolver | undefined, fieldConfig: PothosOutputFieldConfig): GraphQLFieldResolver | undefined { diff --git a/packages/deno/packages/plugin-relay/input-field-builder.ts b/packages/deno/packages/plugin-relay/input-field-builder.ts index 57e36103e..3132ff5a9 100644 --- a/packages/deno/packages/plugin-relay/input-field-builder.ts +++ b/packages/deno/packages/plugin-relay/input-field-builder.ts @@ -15,7 +15,7 @@ inputFieldBuilder.globalIDList = function globalIDList[])?.map((type: ObjectRef) => ({ typename: this.builder.configStore.getTypeConfig(type).name, - parse: type instanceof NodeRef ? type.parseId : undefined, + parseId: type instanceof NodeRef ? type.parseId : undefined, })) ?? null, }, }) as never; @@ -29,7 +29,7 @@ inputFieldBuilder.globalID = function globalID({ for: forTy relayGlobalIDFor: ((forTypes && (Array.isArray(forTypes) ? forTypes : [forTypes])) as ObjectRef[])?.map((type: ObjectRef) => ({ typename: this.builder.configStore.getTypeConfig(type).name, - parse: type instanceof NodeRef ? type.parseId : undefined, + parseId: type instanceof NodeRef ? type.parseId : undefined, })) ?? null, }, }) as unknown as InputFieldRef> as never; diff --git a/packages/deno/packages/plugin-tracing/README.md b/packages/deno/packages/plugin-tracing/README.md index a6ac1c782..486be3320 100644 --- a/packages/deno/packages/plugin-tracing/README.md +++ b/packages/deno/packages/plugin-tracing/README.md @@ -767,13 +767,7 @@ import { schema } from './schema'; const yoga = createYoga({ schema, - plugins: [ - useSentry({ - // Disable resolver tracking since this is covered by the pothos tracing plugin - // If all resolvers are being traced, you could use the Sentry envelop plug instead of the pothos tracing plugin - trackResolvers: false, - }), - ], + plugins: [useSentry({})], }); const server = createServer(yoga); diff --git a/packages/plugin-relay/src/field-builder.ts b/packages/plugin-relay/src/field-builder.ts index 0480eac8f..eee2723e4 100644 --- a/packages/plugin-relay/src/field-builder.ts +++ b/packages/plugin-relay/src/field-builder.ts @@ -165,7 +165,7 @@ fieldBuilderProto.nodeList = function nodeList({ ids, ...options }) { typeof id === 'string' ? internalDecodeGlobalID(this.builder, id, context, info, true) : id && { - id: String(id.id), + id: id.id, typename: this.builder.configStore.getTypeConfig(id.type).name, }, ); diff --git a/packages/plugin-relay/src/index.ts b/packages/plugin-relay/src/index.ts index fbfc23c21..2d01a96ae 100644 --- a/packages/plugin-relay/src/index.ts +++ b/packages/plugin-relay/src/index.ts @@ -47,7 +47,7 @@ export class PothosRelayPlugin extends BasePlugin Date: Tue, 21 Feb 2023 13:27:15 -0500 Subject: [PATCH 5/8] don't force rawID to be a string --- packages/plugin-relay/src/field-builder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin-relay/src/field-builder.ts b/packages/plugin-relay/src/field-builder.ts index eee2723e4..23f8b314d 100644 --- a/packages/plugin-relay/src/field-builder.ts +++ b/packages/plugin-relay/src/field-builder.ts @@ -128,7 +128,7 @@ fieldBuilderProto.node = function node({ id, ...options }) { typeof rawID === 'string' ? internalDecodeGlobalID(this.builder, rawID, context, info, true) : rawID && { - id: String(rawID.id), + id: rawID.id, typename: this.builder.configStore.getTypeConfig(rawID.type).name, }; From c32a1a0babea0ea9d033da6dab15c24d44d63738 Mon Sep 17 00:00:00 2001 From: Tim Griesser Date: Tue, 21 Feb 2023 13:27:58 -0500 Subject: [PATCH 6/8] add deno script --- packages/deno/packages/plugin-relay/field-builder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/deno/packages/plugin-relay/field-builder.ts b/packages/deno/packages/plugin-relay/field-builder.ts index 8061eec2d..d8ec8c8f3 100644 --- a/packages/deno/packages/plugin-relay/field-builder.ts +++ b/packages/deno/packages/plugin-relay/field-builder.ts @@ -55,7 +55,7 @@ fieldBuilderProto.node = function node({ id, ...options }) { const globalID = typeof rawID === "string" ? internalDecodeGlobalID(this.builder, rawID, context, info, true) : rawID && { - id: String(rawID.id), + id: rawID.id, typename: this.builder.configStore.getTypeConfig(rawID.type).name, }; return (await resolveNodes(this.builder, context, info, [globalID]))[0]; From 07d8df5ce8ccfb256bc4b5c076fb59692bc9a1b8 Mon Sep 17 00:00:00 2001 From: Michael Hayes Date: Thu, 23 Feb 2023 11:19:32 -1000 Subject: [PATCH 7/8] add tests for more id parsing cases --- packages/plugin-relay/src/index.ts | 14 +++----- packages/plugin-relay/src/schema-builder.ts | 14 ++++++-- .../tests/__snapshots__/index.test.ts.snap | 8 +++++ .../tests/examples/relay/schema/numbers.ts | 35 +++++++++++++++++++ packages/plugin-relay/tests/index.test.ts | 30 ++++++++++++++++ 5 files changed, 89 insertions(+), 12 deletions(-) diff --git a/packages/plugin-relay/src/index.ts b/packages/plugin-relay/src/index.ts index 2d01a96ae..25928321a 100644 --- a/packages/plugin-relay/src/index.ts +++ b/packages/plugin-relay/src/index.ts @@ -27,9 +27,9 @@ export class PothosRelayPlugin extends BasePlugin { const argMappings = mapInputFields(fieldConfig.args, this.buildCache, (inputField) => { if (inputField.extensions?.isRelayGlobalID) { - return (inputField.extensions?.relayGlobalIDFor ?? true) as - | true - | { typename: string; parseId: (id: string, ctx: object) => unknown }[]; + return (inputField.extensions?.relayGlobalIDFor ?? + inputField.extensions?.alwaysParse ?? + false) as boolean | { typename: string; parseId: (id: string, ctx: object) => unknown }[]; } return null; @@ -42,13 +42,7 @@ export class PothosRelayPlugin extends BasePlugin - internalDecodeGlobalID( - this.builder, - String(globalID), - ctx, - info, - Array.isArray(mappings.value) ? mappings.value : true, - ), + internalDecodeGlobalID(this.builder, String(globalID), ctx, info, mappings.value ?? false), ); return (parent, args, context, info) => diff --git a/packages/plugin-relay/src/schema-builder.ts b/packages/plugin-relay/src/schema-builder.ts index 0d2f656f4..bfab4d94b 100644 --- a/packages/plugin-relay/src/schema-builder.ts +++ b/packages/plugin-relay/src/schema-builder.ts @@ -158,7 +158,12 @@ schemaBuilderProto.nodeInterfaceRef = function nodeInterfaceRef() { ...this.options.relayOptions.nodeQueryOptions, type: ref as InterfaceRef, args: { - id: t.arg.globalID({ required: true }), + id: t.arg.globalID({ + required: true, + extensions: { + alwaysParse: true, + }, + }), }, resolve: resolveNodeFn ? (root, args, context, info) => @@ -198,7 +203,12 @@ schemaBuilderProto.nodeInterfaceRef = function nodeInterfaceRef() { ...this.options.relayOptions.nodesQueryOptions, type: [ref], args: { - ids: t.arg.globalIDList({ required: true }), + ids: t.arg.globalIDList({ + required: true, + extensions: { + alwaysParse: true, + }, + }), }, resolve: resolveNodesFn ? (root, args, context, info) => diff --git a/packages/plugin-relay/tests/__snapshots__/index.test.ts.snap b/packages/plugin-relay/tests/__snapshots__/index.test.ts.snap index 45ac44025..ea0bd4526 100644 --- a/packages/plugin-relay/tests/__snapshots__/index.test.ts.snap +++ b/packages/plugin-relay/tests/__snapshots__/index.test.ts.snap @@ -46,6 +46,13 @@ input GlobalIDInput { otherList: [OtherInput!] = [{someField: \\"abc\\"}] } +type IDResult { + arg: String! + id: String! + idType: String! + typename: String! +} + type Mutation { answerPoll(answer: Int!, id: ID!): Poll! createPoll(answers: [String!]!, question: String!): Poll! @@ -128,6 +135,7 @@ type PollAnswersUsingOffsetConnectionEdge { type Query { batchNumbers(after: ID, before: ID, first: Int, last: Int): QueryBatchNumbersConnection! cursorConnection(after: ID, before: ID, first: Int, last: Int): QueryCursorConnection! + echoIDs(genericNumberThingID: ID!, globalID: ID!, numberThingID: ID!): [IDResult!]! extraNode: Node inputGlobalID(id: ID!, inputObj: GlobalIDInput!, normalId: ID!): String! moreNodes: [Node]! diff --git a/packages/plugin-relay/tests/examples/relay/schema/numbers.ts b/packages/plugin-relay/tests/examples/relay/schema/numbers.ts index e7a5846a4..cd151ecc5 100644 --- a/packages/plugin-relay/tests/examples/relay/schema/numbers.ts +++ b/packages/plugin-relay/tests/examples/relay/schema/numbers.ts @@ -236,3 +236,38 @@ builder.queryField('numberThingsByIDs', (t) => resolve: (root, args) => args.ids.map(({ id }) => new NumberThing(id)), }), ); + +const IDResult = builder + .objectRef<{ + id: unknown; + typename: string; + arg: string; + }>('IDResult') + .implement({ + fields: (t) => ({ + id: t.string({ + resolve: (n) => String(n.id), + }), + typename: t.exposeString('typename', {}), + arg: t.exposeString('arg', {}), + idType: t.string({ + resolve: (n) => typeof n.id, + }), + }), + }); + +builder.queryField('echoIDs', (t) => + t.field({ + type: [IDResult], + args: { + globalID: t.arg.globalID({ required: true }), + numberThingID: t.arg.globalID({ required: true, for: [NumberThingRef] }), + genericNumberThingID: t.arg.globalID({ required: true, for: [NumberThing] }), + }, + resolve: (_, args) => [ + { ...args.globalID, arg: 'globalID' }, + { ...args.numberThingID, arg: 'numberThingID' }, + { ...args.genericNumberThingID, arg: 'genericNumberThingID' }, + ], + }), +); diff --git a/packages/plugin-relay/tests/index.test.ts b/packages/plugin-relay/tests/index.test.ts index eb9f6322d..d362bd7e1 100644 --- a/packages/plugin-relay/tests/index.test.ts +++ b/packages/plugin-relay/tests/index.test.ts @@ -529,6 +529,16 @@ describe('relay example schema', () => { id number } + echoIDs( + globalID: "TnVtYmVyOjE=" + numberThingID: "TnVtYmVyOjE=" + genericNumberThingID: "TnVtYmVyOjE=" + ) { + id + typename + arg + idType + } } `; @@ -541,6 +551,26 @@ describe('relay example schema', () => { expect(result).toMatchInlineSnapshot(` { "data": { + "echoIDs": [ + { + "arg": "globalID", + "id": "1", + "idType": "string", + "typename": "Number", + }, + { + "arg": "numberThingID", + "id": "1", + "idType": "number", + "typename": "Number", + }, + { + "arg": "genericNumberThingID", + "id": "1", + "idType": "string", + "typename": "Number", + }, + ], "invalid": null, "invalidList": null, "numberThingByID": { From d2b02b79ab3ae029f70b451c75ef4aaacdeb616d Mon Sep 17 00:00:00 2001 From: Michael Hayes Date: Thu, 23 Feb 2023 11:30:10 -1000 Subject: [PATCH 8/8] add changeset --- .changeset/eight-ligers-develop.md | 6 ++++++ packages/deno/packages/plugin-relay/index.ts | 6 ++++-- .../deno/packages/plugin-relay/schema-builder.ts | 14 ++++++++++++-- packages/plugin-relay/src/index.ts | 2 +- packages/plugin-relay/src/schema-builder.ts | 4 ++-- 5 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 .changeset/eight-ligers-develop.md diff --git a/.changeset/eight-ligers-develop.md b/.changeset/eight-ligers-develop.md new file mode 100644 index 000000000..38bea0586 --- /dev/null +++ b/.changeset/eight-ligers-develop.md @@ -0,0 +1,6 @@ +--- +'@pothos/plugin-relay': patch +'@pothos/deno': patch +--- + +Fix a few issues with globalID parsing diff --git a/packages/deno/packages/plugin-relay/index.ts b/packages/deno/packages/plugin-relay/index.ts index cfb2cd8b0..19b501c50 100644 --- a/packages/deno/packages/plugin-relay/index.ts +++ b/packages/deno/packages/plugin-relay/index.ts @@ -15,7 +15,9 @@ export class PothosRelayPlugin extends BasePlugin, fieldConfig: PothosOutputFieldConfig): GraphQLFieldResolver { const argMappings = mapInputFields(fieldConfig.args, this.buildCache, (inputField) => { if (inputField.extensions?.isRelayGlobalID) { - return (inputField.extensions?.relayGlobalIDFor ?? true) as true | { + return (inputField.extensions?.relayGlobalIDFor ?? + inputField.extensions?.relayGlobalIDAlwaysParse ?? + false) as boolean | { typename: string; parseId: (id: string, ctx: object) => unknown; }[]; @@ -25,7 +27,7 @@ export class PothosRelayPlugin extends BasePlugin internalDecodeGlobalID(this.builder, String(globalID), ctx, info, Array.isArray(mappings.value) ? mappings.value : true)); + const argMapper = createInputValueMapper(argMappings, (globalID, mappings, ctx: Types["Context"], info: GraphQLResolveInfo) => internalDecodeGlobalID(this.builder, String(globalID), ctx, info, mappings.value ?? false)); return (parent, args, context, info) => resolver(parent, argMapper(args, undefined, context, info), context, info); } override wrapSubscribe(subscribe: GraphQLFieldResolver | undefined, fieldConfig: PothosOutputFieldConfig): GraphQLFieldResolver | undefined { diff --git a/packages/deno/packages/plugin-relay/schema-builder.ts b/packages/deno/packages/plugin-relay/schema-builder.ts index 57230920b..4d0aa554b 100644 --- a/packages/deno/packages/plugin-relay/schema-builder.ts +++ b/packages/deno/packages/plugin-relay/schema-builder.ts @@ -101,7 +101,12 @@ schemaBuilderProto.nodeInterfaceRef = function nodeInterfaceRef() { ...this.options.relayOptions.nodeQueryOptions, type: ref as InterfaceRef, args: { - id: t.arg.globalID({ required: true }), + id: t.arg.globalID({ + required: true, + extensions: { + relayGlobalIDAlwaysParse: true, + }, + }), }, resolve: resolveNodeFn ? (root, args, context, info) => resolveNodeFn(root, args as { @@ -134,7 +139,12 @@ schemaBuilderProto.nodeInterfaceRef = function nodeInterfaceRef() { ...this.options.relayOptions.nodesQueryOptions, type: [ref], args: { - ids: t.arg.globalIDList({ required: true }), + ids: t.arg.globalIDList({ + required: true, + extensions: { + relayGlobalIDAlwaysParse: true, + }, + }), }, resolve: resolveNodesFn ? (root, args, context, info) => resolveNodesFn(root, args as { diff --git a/packages/plugin-relay/src/index.ts b/packages/plugin-relay/src/index.ts index 25928321a..245863b7c 100644 --- a/packages/plugin-relay/src/index.ts +++ b/packages/plugin-relay/src/index.ts @@ -28,7 +28,7 @@ export class PothosRelayPlugin extends BasePlugin { if (inputField.extensions?.isRelayGlobalID) { return (inputField.extensions?.relayGlobalIDFor ?? - inputField.extensions?.alwaysParse ?? + inputField.extensions?.relayGlobalIDAlwaysParse ?? false) as boolean | { typename: string; parseId: (id: string, ctx: object) => unknown }[]; } diff --git a/packages/plugin-relay/src/schema-builder.ts b/packages/plugin-relay/src/schema-builder.ts index bfab4d94b..b7f06e831 100644 --- a/packages/plugin-relay/src/schema-builder.ts +++ b/packages/plugin-relay/src/schema-builder.ts @@ -161,7 +161,7 @@ schemaBuilderProto.nodeInterfaceRef = function nodeInterfaceRef() { id: t.arg.globalID({ required: true, extensions: { - alwaysParse: true, + relayGlobalIDAlwaysParse: true, }, }), }, @@ -206,7 +206,7 @@ schemaBuilderProto.nodeInterfaceRef = function nodeInterfaceRef() { ids: t.arg.globalIDList({ required: true, extensions: { - alwaysParse: true, + relayGlobalIDAlwaysParse: true, }, }), },