From b79761ce44b5cfc3bd8a683906144e6c4b72259d Mon Sep 17 00:00:00 2001 From: Daniel Cousens <413395+dcousens@users.noreply.github.com> Date: Thu, 28 Sep 2023 12:35:06 +1000 Subject: [PATCH 1/5] reduce relationship field error verbosity --- .../src/fields/types/relationship/index.ts | 2 +- .../src/lib/core/resolve-relationships.ts | 21 +++++-------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/packages/core/src/fields/types/relationship/index.ts b/packages/core/src/fields/types/relationship/index.ts index c8e36c26233..c25da9826fd 100644 --- a/packages/core/src/fields/types/relationship/index.ts +++ b/packages/core/src/fields/types/relationship/index.ts @@ -93,7 +93,7 @@ export const relationship = const foreignList = lists[foreignListKey]; if (!foreignList) { throw new Error( - `Unable to resolve list '${foreignListKey}' for field ${listKey}.${fieldKey}` + `${listKey}.${fieldKey} points to ${ref}, but ${ref} doesn't exist`, ); } const foreignListTypes = foreignList.types; diff --git a/packages/core/src/lib/core/resolve-relationships.ts b/packages/core/src/lib/core/resolve-relationships.ts index 9060677c5e6..7d4241b1bc1 100644 --- a/packages/core/src/lib/core/resolve-relationships.ts +++ b/packages/core/src/lib/core/resolve-relationships.ts @@ -130,31 +130,20 @@ export function resolveRelationships( const foreignField = foreignUnresolvedList.fields[field.field]?.dbField; if (!foreignField) { throw new Error( - `The relationship field at ${localRef} points to ${foreignRef} but no field at ${foreignRef} exists` + `${localRef} points to ${foreignRef}, but ${foreignRef} doesn't exist` ); } if (foreignField.kind !== 'relation') { throw new Error( - `The relationship field at ${localRef} points to ${foreignRef} but ${foreignRef} is not a relationship field` + `${localRef} points to ${foreignRef}, but ${foreignRef} is not a relationship field` ); } - if (foreignField.list !== listKey) { + const actualRef = `${foreignField.list}.${foreignField.field}` + if (actualRef !== localRef) { throw new Error( - `The relationship field at ${localRef} points to ${foreignRef} but ${foreignRef} points to the list ${foreignField.list} rather than ${listKey}` - ); - } - - if (foreignField.field === undefined) { - throw new Error( - `The relationship field at ${localRef} points to ${foreignRef}, ${localRef} points to ${listKey} correctly but does not point to the ${fieldPath} field when it should` - ); - } - - if (foreignField.field !== fieldPath) { - throw new Error( - `The relationship field at ${localRef} points to ${foreignRef}, ${localRef} points to ${listKey} correctly but points to the ${foreignField.field} field instead of ${fieldPath}` + `${foreignRef} points to ${actualRef}, but ${localRef} expects a two-way relationship` ); } From c9e13422f37003053d4df26adb472837e77717da Mon Sep 17 00:00:00 2001 From: Daniel Cousens <413395+dcousens@users.noreply.github.com> Date: Thu, 28 Sep 2023 12:34:43 +1000 Subject: [PATCH 2/5] update tests for new errors --- ...mentation.test.ts => relationship.test.ts} | 148 ++++++++++++++---- 1 file changed, 119 insertions(+), 29 deletions(-) rename tests/api-tests/fields/types/{fixtures/relationship/implementation.test.ts => relationship.test.ts} (52%) diff --git a/tests/api-tests/fields/types/fixtures/relationship/implementation.test.ts b/tests/api-tests/fields/types/relationship.test.ts similarity index 52% rename from tests/api-tests/fields/types/fixtures/relationship/implementation.test.ts rename to tests/api-tests/fields/types/relationship.test.ts index 16f1203937d..af706e26af0 100644 --- a/tests/api-tests/fields/types/fixtures/relationship/implementation.test.ts +++ b/tests/api-tests/fields/types/relationship.test.ts @@ -1,12 +1,13 @@ import { assertInputObjectType, printType, assertObjectType, parse } from 'graphql'; import { createSystem, initConfig } from '@keystone-6/core/system'; +import type { ListSchemaConfig } from '@keystone-6/core/types'; import { config, list } from '@keystone-6/core'; import { text, relationship } from '@keystone-6/core/fields'; import { allowAll } from '@keystone-6/core/access'; const fieldKey = 'foo'; -const getSchema = (field: any) => { +function getSchema (field: { ref: string, many?: boolean }) { return createSystem( initConfig( config({ @@ -16,42 +17,42 @@ const getSchema = (field: any) => { Test: list({ access: allowAll, fields: { - [fieldKey]: field, + [fieldKey]: relationship(field), }, }), }, }) ) ).graphQLSchema; -}; +} describe('Type Generation', () => { test('inputs for relationship fields in create args', () => { - const relMany = getSchema(relationship({ many: true, ref: 'Zip' })); + const relMany = getSchema(({ many: true, ref: 'Zip' })); expect( assertInputObjectType(relMany.getType('TestCreateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToManyForCreateInput'); - const relSingle = getSchema(relationship({ many: false, ref: 'Zip' })); + const relSingle = getSchema(({ many: false, ref: 'Zip' })); expect( assertInputObjectType(relSingle.getType('TestCreateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToOneForCreateInput'); }); test('inputs for relationship fields in update args', () => { - const relMany = getSchema(relationship({ many: true, ref: 'Zip' })); + const relMany = getSchema(({ many: true, ref: 'Zip' })); expect( assertInputObjectType(relMany.getType('TestUpdateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToManyForUpdateInput'); - const relSingle = getSchema(relationship({ many: false, ref: 'Zip' })); + const relSingle = getSchema(({ many: false, ref: 'Zip' })); expect( assertInputObjectType(relSingle.getType('TestUpdateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToOneForUpdateInput'); }); test('to-one for create relationship nested mutation input', () => { - const schema = getSchema(relationship({ many: false, ref: 'Zip' })); + const schema = getSchema(({ many: false, ref: 'Zip' })); expect(printType(schema.getType('ZipRelateToOneForCreateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToOneForCreateInput { @@ -62,7 +63,7 @@ describe('Type Generation', () => { }); test('to-one for update relationship nested mutation input', () => { - const schema = getSchema(relationship({ many: false, ref: 'Zip' })); + const schema = getSchema(({ many: false, ref: 'Zip' })); expect(printType(schema.getType('ZipRelateToOneForUpdateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToOneForUpdateInput { @@ -74,7 +75,7 @@ describe('Type Generation', () => { }); test('to-many for create relationship nested mutation input', () => { - const schema = getSchema(relationship({ many: true, ref: 'Zip' })); + const schema = getSchema(({ many: true, ref: 'Zip' })); expect(printType(schema.getType('ZipRelateToManyForCreateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToManyForCreateInput { @@ -85,7 +86,7 @@ describe('Type Generation', () => { }); test('to-many for update relationship nested mutation input', () => { - const schema = getSchema(relationship({ many: true, ref: 'Zip' })); + const schema = getSchema(({ many: true, ref: 'Zip' })); expect(printType(schema.getType('ZipRelateToManyForUpdateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToManyForUpdateInput { @@ -98,7 +99,7 @@ describe('Type Generation', () => { }); test('to-one relationships cannot be filtered at the field level', () => { - const schema = getSchema(relationship({ many: false, ref: 'Zip' })); + const schema = getSchema(({ many: false, ref: 'Zip' })); expect( ( @@ -119,7 +120,7 @@ describe('Type Generation', () => { }); test('to-many relationships can be filtered at the field level', () => { - const schema = getSchema(relationship({ many: true, ref: 'Zip' })); + const schema = getSchema(({ many: true, ref: 'Zip' })); expect(printType(schema.getType('Test')!)).toMatchInlineSnapshot(` "type Test { @@ -131,20 +132,109 @@ describe('Type Generation', () => { }); }); -describe('Referenced list errors', () => { - test('throws when list not found', async () => { - expect(() => getSchema(relationship({ ref: 'DoesNotExist' }))).toThrow( - "Unable to resolve list 'DoesNotExist' for field Test.foo" - ); - }); - - test('does not throw when no two way relationship specified', async () => { - getSchema(relationship({ many: true, ref: 'Zip' })); - }); - - test('throws when field on list not found', async () => { - expect(() => getSchema(relationship({ many: true, ref: 'Zip.bar' }))).toThrow( - 'The relationship field at Test.foo points to Zip.bar but no field at Zip.bar exists' - ); - }); +describe('Reference errors', () => { + function tryf (lists: ListSchemaConfig) { + return createSystem( + initConfig( + config({ + db: { url: 'file:./thing.db', provider: 'sqlite' }, + lists, + }) + ) + ).graphQLSchema; + }; + + const fixtures = { + 'list not found': { + lists: { + Foo: list({ + access: allowAll, + fields: { + bar: relationship({ ref: 'Abc.def' }) + } + }), + }, + error: `Foo.bar points to Abc.def, but Abc.def doesn't exist`, + }, + 'field not found': { + lists: { + Foo: list({ + access: allowAll, + fields: { + bar: relationship({ ref: 'Abc.def' }) + } + }), + Abc: list({ + access: allowAll, + fields: {} + }), + }, + error: `Foo.bar points to Abc.def, but Abc.def doesn't exist`, + }, + '2-way not 2-way': { + lists: { + Foo: list({ + access: allowAll, + fields: { + bar: relationship({ ref: 'Abc.def' }) + } + }), + Abc: list({ + access: allowAll, + fields: { + def: relationship({ ref: 'Foo.bazzz' }) + } + }), + }, + error: `Abc.def points to Foo.bazzz, but Foo.bar expects a two-way relationship`, + }, + 'field wrong type': { + lists: { + Foo: list({ + access: allowAll, + fields: { + bar: relationship({ ref: 'Abc.def' }) + } + }), + Abc: list({ + access: allowAll, + fields: { + def: text() + } + }), + }, + error: `Foo.bar points to Abc.def, but Abc.def is not a relationship field`, + }, + '1-way relationships': { + lists: { + Foo: list({ + access: allowAll, + fields: { + bar: relationship({ ref: 'Abc' }) + } + }), + Abc: list({ + access: allowAll, + fields: {} + }), + }, + error: null + }, + } + + for (const [description, { lists, error }] of Object.entries(fixtures)) { + if (error) { + test(`throws for ${description}`, () => { + expect(() => { + tryf(lists) + }).toThrow(error); + }); + } else { + test(`does not throw for ${description}`, () => { + expect(() => { + tryf(lists) + }).not.toThrow(); + }); + } + } }); From 35d9b5aa1b51baefaa9dcd0d86b9702d11048349 Mon Sep 17 00:00:00 2001 From: Daniel Cousens <413395+dcousens@users.noreply.github.com> Date: Thu, 28 Sep 2023 12:35:50 +1000 Subject: [PATCH 3/5] prettier --- .../src/fields/types/relationship/index.ts | 4 +- .../src/lib/core/resolve-relationships.ts | 6 +- .../fields/types/relationship.test.ts | 66 +++++++++---------- 3 files changed, 36 insertions(+), 40 deletions(-) diff --git a/packages/core/src/fields/types/relationship/index.ts b/packages/core/src/fields/types/relationship/index.ts index c25da9826fd..8c94c982326 100644 --- a/packages/core/src/fields/types/relationship/index.ts +++ b/packages/core/src/fields/types/relationship/index.ts @@ -92,9 +92,7 @@ export const relationship = const [foreignListKey, foreignFieldKey] = ref.split('.'); const foreignList = lists[foreignListKey]; if (!foreignList) { - throw new Error( - `${listKey}.${fieldKey} points to ${ref}, but ${ref} doesn't exist`, - ); + throw new Error(`${listKey}.${fieldKey} points to ${ref}, but ${ref} doesn't exist`); } const foreignListTypes = foreignList.types; diff --git a/packages/core/src/lib/core/resolve-relationships.ts b/packages/core/src/lib/core/resolve-relationships.ts index 7d4241b1bc1..ce11ed2a9af 100644 --- a/packages/core/src/lib/core/resolve-relationships.ts +++ b/packages/core/src/lib/core/resolve-relationships.ts @@ -129,9 +129,7 @@ export function resolveRelationships( alreadyResolvedTwoSidedRelationships.add(foreignRef); const foreignField = foreignUnresolvedList.fields[field.field]?.dbField; if (!foreignField) { - throw new Error( - `${localRef} points to ${foreignRef}, but ${foreignRef} doesn't exist` - ); + throw new Error(`${localRef} points to ${foreignRef}, but ${foreignRef} doesn't exist`); } if (foreignField.kind !== 'relation') { @@ -140,7 +138,7 @@ export function resolveRelationships( ); } - const actualRef = `${foreignField.list}.${foreignField.field}` + const actualRef = `${foreignField.list}.${foreignField.field}`; if (actualRef !== localRef) { throw new Error( `${foreignRef} points to ${actualRef}, but ${localRef} expects a two-way relationship` diff --git a/tests/api-tests/fields/types/relationship.test.ts b/tests/api-tests/fields/types/relationship.test.ts index af706e26af0..5ca705faae3 100644 --- a/tests/api-tests/fields/types/relationship.test.ts +++ b/tests/api-tests/fields/types/relationship.test.ts @@ -7,7 +7,7 @@ import { allowAll } from '@keystone-6/core/access'; const fieldKey = 'foo'; -function getSchema (field: { ref: string, many?: boolean }) { +function getSchema(field: { ref: string; many?: boolean }) { return createSystem( initConfig( config({ @@ -28,31 +28,31 @@ function getSchema (field: { ref: string, many?: boolean }) { describe('Type Generation', () => { test('inputs for relationship fields in create args', () => { - const relMany = getSchema(({ many: true, ref: 'Zip' })); + const relMany = getSchema({ many: true, ref: 'Zip' }); expect( assertInputObjectType(relMany.getType('TestCreateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToManyForCreateInput'); - const relSingle = getSchema(({ many: false, ref: 'Zip' })); + const relSingle = getSchema({ many: false, ref: 'Zip' }); expect( assertInputObjectType(relSingle.getType('TestCreateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToOneForCreateInput'); }); test('inputs for relationship fields in update args', () => { - const relMany = getSchema(({ many: true, ref: 'Zip' })); + const relMany = getSchema({ many: true, ref: 'Zip' }); expect( assertInputObjectType(relMany.getType('TestUpdateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToManyForUpdateInput'); - const relSingle = getSchema(({ many: false, ref: 'Zip' })); + const relSingle = getSchema({ many: false, ref: 'Zip' }); expect( assertInputObjectType(relSingle.getType('TestUpdateInput')).getFields().foo.type.toString() ).toEqual('ZipRelateToOneForUpdateInput'); }); test('to-one for create relationship nested mutation input', () => { - const schema = getSchema(({ many: false, ref: 'Zip' })); + const schema = getSchema({ many: false, ref: 'Zip' }); expect(printType(schema.getType('ZipRelateToOneForCreateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToOneForCreateInput { @@ -63,7 +63,7 @@ describe('Type Generation', () => { }); test('to-one for update relationship nested mutation input', () => { - const schema = getSchema(({ many: false, ref: 'Zip' })); + const schema = getSchema({ many: false, ref: 'Zip' }); expect(printType(schema.getType('ZipRelateToOneForUpdateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToOneForUpdateInput { @@ -75,7 +75,7 @@ describe('Type Generation', () => { }); test('to-many for create relationship nested mutation input', () => { - const schema = getSchema(({ many: true, ref: 'Zip' })); + const schema = getSchema({ many: true, ref: 'Zip' }); expect(printType(schema.getType('ZipRelateToManyForCreateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToManyForCreateInput { @@ -86,7 +86,7 @@ describe('Type Generation', () => { }); test('to-many for update relationship nested mutation input', () => { - const schema = getSchema(({ many: true, ref: 'Zip' })); + const schema = getSchema({ many: true, ref: 'Zip' }); expect(printType(schema.getType('ZipRelateToManyForUpdateInput')!)).toMatchInlineSnapshot(` "input ZipRelateToManyForUpdateInput { @@ -99,7 +99,7 @@ describe('Type Generation', () => { }); test('to-one relationships cannot be filtered at the field level', () => { - const schema = getSchema(({ many: false, ref: 'Zip' })); + const schema = getSchema({ many: false, ref: 'Zip' }); expect( ( @@ -120,7 +120,7 @@ describe('Type Generation', () => { }); test('to-many relationships can be filtered at the field level', () => { - const schema = getSchema(({ many: true, ref: 'Zip' })); + const schema = getSchema({ many: true, ref: 'Zip' }); expect(printType(schema.getType('Test')!)).toMatchInlineSnapshot(` "type Test { @@ -133,7 +133,7 @@ describe('Type Generation', () => { }); describe('Reference errors', () => { - function tryf (lists: ListSchemaConfig) { + function tryf(lists: ListSchemaConfig) { return createSystem( initConfig( config({ @@ -142,7 +142,7 @@ describe('Reference errors', () => { }) ) ).graphQLSchema; - }; + } const fixtures = { 'list not found': { @@ -150,8 +150,8 @@ describe('Reference errors', () => { Foo: list({ access: allowAll, fields: { - bar: relationship({ ref: 'Abc.def' }) - } + bar: relationship({ ref: 'Abc.def' }), + }, }), }, error: `Foo.bar points to Abc.def, but Abc.def doesn't exist`, @@ -161,12 +161,12 @@ describe('Reference errors', () => { Foo: list({ access: allowAll, fields: { - bar: relationship({ ref: 'Abc.def' }) - } + bar: relationship({ ref: 'Abc.def' }), + }, }), Abc: list({ access: allowAll, - fields: {} + fields: {}, }), }, error: `Foo.bar points to Abc.def, but Abc.def doesn't exist`, @@ -176,14 +176,14 @@ describe('Reference errors', () => { Foo: list({ access: allowAll, fields: { - bar: relationship({ ref: 'Abc.def' }) - } + bar: relationship({ ref: 'Abc.def' }), + }, }), Abc: list({ access: allowAll, fields: { - def: relationship({ ref: 'Foo.bazzz' }) - } + def: relationship({ ref: 'Foo.bazzz' }), + }, }), }, error: `Abc.def points to Foo.bazzz, but Foo.bar expects a two-way relationship`, @@ -193,14 +193,14 @@ describe('Reference errors', () => { Foo: list({ access: allowAll, fields: { - bar: relationship({ ref: 'Abc.def' }) - } + bar: relationship({ ref: 'Abc.def' }), + }, }), Abc: list({ access: allowAll, fields: { - def: text() - } + def: text(), + }, }), }, error: `Foo.bar points to Abc.def, but Abc.def is not a relationship field`, @@ -210,29 +210,29 @@ describe('Reference errors', () => { Foo: list({ access: allowAll, fields: { - bar: relationship({ ref: 'Abc' }) - } + bar: relationship({ ref: 'Abc' }), + }, }), Abc: list({ access: allowAll, - fields: {} + fields: {}, }), }, - error: null + error: null, }, - } + }; for (const [description, { lists, error }] of Object.entries(fixtures)) { if (error) { test(`throws for ${description}`, () => { expect(() => { - tryf(lists) + tryf(lists); }).toThrow(error); }); } else { test(`does not throw for ${description}`, () => { expect(() => { - tryf(lists) + tryf(lists); }).not.toThrow(); }); } From 7d4e2eb1da66904931daac629f74dbff71a59261 Mon Sep 17 00:00:00 2001 From: Daniel Cousens <413395+dcousens@users.noreply.github.com> Date: Fri, 29 Sep 2023 09:28:48 +1000 Subject: [PATCH 4/5] use alternate wording for 2 way relationship errors, add missing conflict test --- .../src/lib/core/resolve-relationships.ts | 4 ++-- .../fields/types/relationship.test.ts | 21 +++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/core/src/lib/core/resolve-relationships.ts b/packages/core/src/lib/core/resolve-relationships.ts index ce11ed2a9af..086e2987763 100644 --- a/packages/core/src/lib/core/resolve-relationships.ts +++ b/packages/core/src/lib/core/resolve-relationships.ts @@ -138,10 +138,10 @@ export function resolveRelationships( ); } - const actualRef = `${foreignField.list}.${foreignField.field}`; + const actualRef = foreignField.field ? `${foreignField.list}.${foreignField.field}` : foreignField.list; if (actualRef !== localRef) { throw new Error( - `${foreignRef} points to ${actualRef}, but ${localRef} expects a two-way relationship` + `${localRef} points to ${foreignRef}, ${foreignRef} points to ${actualRef}, expected ${foreignRef} to point to ${localRef}` ); } diff --git a/tests/api-tests/fields/types/relationship.test.ts b/tests/api-tests/fields/types/relationship.test.ts index 5ca705faae3..e21188cfaea 100644 --- a/tests/api-tests/fields/types/relationship.test.ts +++ b/tests/api-tests/fields/types/relationship.test.ts @@ -171,7 +171,24 @@ describe('Reference errors', () => { }, error: `Foo.bar points to Abc.def, but Abc.def doesn't exist`, }, - '2-way not 2-way': { + '1-way / 2-way conflict': { + lists: { + Foo: list({ + access: allowAll, + fields: { + bar: relationship({ ref: 'Abc.def' }), + }, + }), + Abc: list({ + access: allowAll, + fields: { + def: relationship({ ref: 'Foo' }), + }, + }), + }, + error: `Foo.bar points to Abc.def, Abc.def points to Foo, expected Abc.def to point to Foo.bar`, + }, + '3-way / 2-way conflict': { lists: { Foo: list({ access: allowAll, @@ -186,7 +203,7 @@ describe('Reference errors', () => { }, }), }, - error: `Abc.def points to Foo.bazzz, but Foo.bar expects a two-way relationship`, + error: `Foo.bar points to Abc.def, Abc.def points to Foo.bazzz, expected Abc.def to point to Foo.bar`, }, 'field wrong type': { lists: { From 65f0bc0e659a97c882c925cc7a7a17dee4c45902 Mon Sep 17 00:00:00 2001 From: Daniel Cousens <413395+dcousens@users.noreply.github.com> Date: Fri, 29 Sep 2023 10:54:10 +1000 Subject: [PATCH 5/5] prettier --- packages/core/src/lib/core/resolve-relationships.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/src/lib/core/resolve-relationships.ts b/packages/core/src/lib/core/resolve-relationships.ts index 086e2987763..e69bd75ba14 100644 --- a/packages/core/src/lib/core/resolve-relationships.ts +++ b/packages/core/src/lib/core/resolve-relationships.ts @@ -138,7 +138,9 @@ export function resolveRelationships( ); } - const actualRef = foreignField.field ? `${foreignField.list}.${foreignField.field}` : foreignField.list; + const actualRef = foreignField.field + ? `${foreignField.list}.${foreignField.field}` + : foreignField.list; if (actualRef !== localRef) { throw new Error( `${localRef} points to ${foreignRef}, ${foreignRef} points to ${actualRef}, expected ${foreignRef} to point to ${localRef}`