Fix generated list operation fragments with custom keys (HoudiniGraph…

…ql#773)

* 🐛 FIX: obj identification

* 🚸 UPDATE: don't log paginate error on a list dir

* ✏️ DOC: tentative

* 👌 UPDATE: even better tests with extra field

* tweak variable name

Co-authored-by: Alec Aivazis <[email protected]>

.changeset/old-trees-tap.md

@@ -0,0 +1,5 @@
+---
+'houdini': patch
+---
+
+object identification takes care of custom IDs in list directive now

packages/houdini/src/codegen/generators/artifacts/artifacts.test.ts

@@ -1956,7 +1956,7 @@ describe('mutation artifacts', function () {
 			export default {
 			    name: "A",
 			    kind: "HoudiniMutation",
-			    hash: "2a821dc72c7f92a67ff9fcef569c4cadaf9852662035b32d015c4af67e57aca3",
+			    hash: "e01f8a23cc33c10c4ee3745c041ee97f428b3b4676a5d8d681124f75b09306da",
 
 			    raw: \`mutation A {
 			  addFriend {
@@ -1970,7 +1970,6 @@ describe('mutation artifacts', function () {
 			fragment All_Users_toggle on User {
 			  firstName
 			  id
-			  id
 			}
 			\`,
 

packages/houdini/src/codegen/generators/definitions/schema.test.ts

@@ -152,20 +152,123 @@ test('list operations are included', async function () {
 	// read the documents file
 	expect(graphql.parse((await fs.readFile(config.definitionsDocumentsPath))!))
 		.toMatchInlineSnapshot(`
-		fragment Friends_insert on User {
-		  id
-		}
+			fragment Friends_insert on User {
+			  id
+			}
+
+			fragment Friends_toggle on User {
+			  id
+			}
+
+			fragment Friends_remove on User {
+			  id
+			}
+		`)
+})
+
+test('list operations are included but delete directive should not be in when we have Custom Ids', async function () {
+	const docs: CollectedGraphQLDocument[] = [
+		mockCollectedDoc(
+			`query TestQuery { usersByCursor @list(name: "Friends") { edges { node { id } } } }`
+		),
+		mockCollectedDoc(`fragment TestFragment on User { firstName }`),
+		mockCollectedDoc(`query CustomIdList { customIdList @list(name: "theList") { foo }}`),
+	]
+
+	// execute the generator
+	await runPipeline(config, docs)
+
+	// read the schema file
+	expect(graphql.parse((await fs.readFile(config.definitionsSchemaPath))!))
+		.toMatchInlineSnapshot(`
+			enum CachePolicy {
+			  CacheAndNetwork
+			  CacheOnly
+			  CacheOrNetwork
+			  NetworkOnly
+			}
 
-		fragment Friends_toggle on User {
-		  id
-		  id
-		}
+			"""
+			@list is used to mark a field for the runtime as a place to add or remove
+			entities in mutations
+			"""
+			directive @list(name: String!, connection: Boolean) on FIELD
 
-		fragment Friends_remove on User {
-		  id
-		}
+			"""
+			@paginate is used to to mark a field for pagination.
+			More info in the [doc](https://houdinigraphql.com/guides/pagination).
+			"""
+			directive @paginate(name: String) on FIELD
+
+			"""@prepend is used to tell the runtime to add the result to the end of the list"""
+			directive @prepend on FRAGMENT_SPREAD
+
+			"""@append is used to tell the runtime to add the result to the start of the list"""
+			directive @append on FRAGMENT_SPREAD
+
+			"""@allLists is used to tell the runtime to add the result to all list"""
+			directive @allLists on FRAGMENT_SPREAD
 
-	`)
+			"""
+			@parentID is used to provide a parentID without specifying position or in situations
+			where it doesn't make sense (eg when deleting a node.)
+			"""
+			directive @parentID(value: ID!) on FRAGMENT_SPREAD
+
+			"""@when is used to provide a conditional or in situations where it doesn't make sense (eg when removing or deleting a node.)"""
+			directive @when on FRAGMENT_SPREAD
+
+			"""@when_not is used to provide a conditional or in situations where it doesn't make sense (eg when removing or deleting a node.)"""
+			directive @when_not on FRAGMENT_SPREAD
+
+			"""@arguments is used to define the arguments of a fragment"""
+			directive @arguments on FRAGMENT_DEFINITION
+
+			"""@cache is used to specify cache rules for a query"""
+			directive @cache(policy: CachePolicy, partial: Boolean) on QUERY
+
+			"""@manual_load is used to disable automatic fetch (no load, no auto fetch in component), you will have to do it manually."""
+			directive @manual_load on QUERY
+
+			"""@mask_enable to enable masking on fragment (overwriting the global conf)"""
+			directive @mask_enable on FRAGMENT_SPREAD
+
+			"""@mask_disable to disable masking on fragment (overwriting the global conf)"""
+			directive @mask_disable on FRAGMENT_SPREAD
+
+			directive @User_delete repeatable on FIELD
+		`)
+
+	// read the documents file
+	expect(graphql.parse((await fs.readFile(config.definitionsDocumentsPath))!))
+		.toMatchInlineSnapshot(`
+			fragment Friends_insert on User {
+			  id
+			}
+
+			fragment Friends_toggle on User {
+			  id
+			}
+
+			fragment Friends_remove on User {
+			  id
+			}
+
+			fragment theList_insert on CustomIdType {
+			  foo
+			  bar
+			}
+
+			fragment theList_toggle on CustomIdType {
+			  foo
+			  bar
+			}
+
+			fragment theList_remove on CustomIdType {
+			  foo
+			  bar
+			}
+		`)
 })
 
 test("writing twice doesn't duplicate definitions", async function () {

packages/houdini/src/codegen/transforms/list.ts

@@ -10,6 +10,7 @@ import {
 } from '../../lib'
 import { ArtifactKind } from '../../runtime/lib/types'
 import { TypeWrapper, unwrapType } from '../utils'
+import { objectIdentificationSelection } from '../utils/objectIdentificationSelection'
 import { pageInfoSelection } from './paginate'
 
 // addListFragments adds fragments for the fields tagged with @list
@@ -183,14 +184,19 @@ export default async function addListFragments(
 	}
 
 	// we need to add a delete directive for every type that is the target of a list
-	const listTargets = [
+	const validDeletes = [
 		...new Set(
 			Object.values(lists).map(({ type }) => {
 				// only consider object types
 				if (!(type instanceof graphql.GraphQLObjectType)) {
 					return ''
 				}
 
+				// only consider when we have "id" or a single customId, if not, go out
+				if (config.keyFieldsForType(type.name).length !== 1) {
+					return ''
+				}
+
 				return type.name
 			})
 		).values(),
@@ -222,24 +228,20 @@ export default async function addListFragments(
 						selections: [...selection.selections],
 					}
 
-					// is there no id selection
+					// is there no custom ids selection
 					if (
 						schemaType &&
 						fragmentSelection &&
 						!fragmentSelection?.selections.find(
-							(field) => field.kind === 'Field' && field.name.value === 'id'
+							(field) =>
+								field.kind === 'Field' &&
+								config.keyFieldsForType(type.name).includes(field.name.value)
 						)
 					) {
 						// add the id field to the selection
 						fragmentSelection.selections = [
 							...fragmentSelection.selections,
-							{
-								kind: graphql.Kind.FIELD,
-								name: {
-									kind: graphql.Kind.NAME,
-									value: 'id',
-								},
-							},
+							...objectIdentificationSelection(config, type),
 						]
 					}
 
@@ -272,19 +274,7 @@ export default async function addListFragments(
 							kind: graphql.Kind.FRAGMENT_DEFINITION,
 							// in order to insert an item into this list, it must
 							// have the same selection as the field
-							selectionSet: {
-								...fragmentSelection,
-								selections: [
-									...fragmentSelection.selections,
-									{
-										kind: graphql.Kind.FIELD,
-										name: {
-											kind: graphql.Kind.NAME,
-											value: 'id',
-										},
-									},
-								],
-							},
+							selectionSet: fragmentSelection,
 							typeCondition: {
 								kind: graphql.Kind.NAMED_TYPE,
 								name: {
@@ -303,15 +293,7 @@ export default async function addListFragments(
 							// deleting an entity just takes its id and the parent
 							selectionSet: {
 								kind: graphql.Kind.SELECTION_SET,
-								selections: [
-									{
-										kind: graphql.Kind.FIELD,
-										name: {
-											kind: graphql.Kind.NAME,
-											value: 'id',
-										},
-									},
-								],
+								selections: [...objectIdentificationSelection(config, type)],
 							},
 							typeCondition: {
 								kind: graphql.Kind.NAMED_TYPE,
@@ -325,7 +307,7 @@ export default async function addListFragments(
 				}
 			) as graphql.DefinitionNode[]
 		).concat(
-			...listTargets.map<graphql.DirectiveDefinitionNode>((typeName) => ({
+			...validDeletes.map<graphql.DirectiveDefinitionNode>((typeName) => ({
 				kind: graphql.Kind.DIRECTIVE_DEFINITION,
 				name: {
 					kind: graphql.Kind.NAME,

packages/houdini/src/codegen/transforms/lists.test.ts

@@ -1,6 +1,8 @@
-import { test, expect } from 'vitest'
+import * as graphql from 'graphql'
+import { expect, test } from 'vitest'
 
 import { runPipeline } from '../../codegen'
+import { fs } from '../../lib'
 import { mockCollectedDoc, testConfig } from '../../test'
 
 test('insert fragments on query selection set', async function () {
@@ -456,7 +458,64 @@ test('cannot use list directive if id is not a valid field', async function () {
 
 	// run the pipeline
 	const config = testConfig()
-	await expect(runPipeline(config, docs)).rejects.toBeTruthy()
+
+	let nbError = 0
+	// run the pipeline
+	try {
+		await runPipeline(config, docs)
+	} catch (error: unknown) {
+		nbError++
+		// @ts-ignore
+		expect(error[0].description).toMatchInlineSnapshot(
+			'"@list on \\u001b[32mLegend\\u001b[37m\\u001b[0m as a configuration issue. Object identification missing: \\"\\u001b[33mid\\u001b[37m\\u001b[0m\\". Check \'Custom IDs\' if needed."'
+		)
+	}
+	expect(nbError).toBe(1)
+	// expect(docs[0]).toMatchInlineSnapshot(``)
+})
+
+test('list with Custom Ids & an extra field', async function () {
+	const docs = [
+		mockCollectedDoc(
+			`
+			query CustomIdList {
+				customIdList @list(name: "theList") {
+					foo
+					dummy
+				}
+			}
+		`
+		),
+	]
+
+	const config = testConfig()
+
+	// run the pipeline
+	await runPipeline(config, docs)
+
+	const content = await fs.readFile(config.definitionsDocumentsPath)
+
+	// We want Obj Identification & the extra field on insert & toggle
+	expect(graphql.parse(content!)).toMatchInlineSnapshot(
+		`
+		fragment theList_insert on CustomIdType {
+		  foo
+		  dummy
+		  bar
+		}
+
+		fragment theList_toggle on CustomIdType {
+		  foo
+		  dummy
+		  bar
+		}
+
+		fragment theList_remove on CustomIdType {
+		  foo
+		  bar
+		}
+	`
+	)
 })
 
 test('paginate with name also gets treated as a list', async function () {