Remove then insert (#275)

* fixed an issue in the cache when inserting a record that was just deleted from a connection

* adding to a single list does not undo the whole operation

* added changeset

.changeset/lucky-poems-notice.md

@@ -0,0 +1,5 @@
+---
+'houdini': patch
+---
+
+Fixed edge cases involving adding, removing, and deleting records back to back from in-memory cache"

src/runtime/cache/cache.ts

@@ -919,7 +919,7 @@ class CacheInternal {
 				innerType = typename as string
 			}
 
-			// build up an
+			// if this isn't an embedded reference, use the entry's id in the link list
 			if (!embedded) {
 				const id = this.id(innerType, entry as {})
 				if (id) {
@@ -955,7 +955,7 @@ class CacheInternal {
 
 		// if there is only one layer in the cache, clean up the data
 		if (this.storage.layerCount === 1) {
-			this.storage.topLayer.applyDeletes()
+			this.storage.topLayer.removeUndefinedFields()
 		}
 	}
 }

src/runtime/cache/lists.ts

@@ -253,7 +253,7 @@ export class List {
 		}
 
 		// if the id is not contained in the list, dont notify anyone
-		const value = this.cache._internal_unstable.storage.get(parentID, targetKey)
+		let value = this.cache._internal_unstable.storage.get(parentID, targetKey)
 			.value as LinkedList
 		if (!value || !value.includes(targetID)) {
 			return
@@ -275,6 +275,30 @@ export class List {
 		// remove the target from the parent
 		this.cache._internal_unstable.storage.remove(parentID, targetKey, targetID)
 
+		// if we are removing an id from a connection then the id we were given points to an edge
+		if (this.connection) {
+			// grab the index we are removing
+			const [, field, index] = targetID.match(/(.*)\[(\d+)\]$/) || []
+			if (!field || !index) {
+				throw new Error('Could not find id of edge')
+			}
+			const newIDs = []
+
+			// every element in the linked list after the element we removed needs to have an updated id
+			for (let i = parseInt(index) + 1; i < value.length; i++) {
+				const to = `${field}[${i - 1}]`
+				newIDs.push(to)
+				this.cache._internal_unstable.storage.replaceID({
+					from: `${field}[${i}]`,
+					to,
+				})
+			}
+
+			value = value.slice(0, parseInt(index)).concat(newIDs)
+
+			this.cache._internal_unstable.storage.writeLink(parentID, targetKey, value)
+		}
+
 		// notify the subscribers about the change
 		for (const spec of subscribers) {
 			// trigger the update
@@ -287,11 +311,6 @@ export class List {
 			)
 		}
 
-		// if we are removing from a connection, delete the embedded edge holding the record
-		if (this.connection) {
-			this.cache._internal_unstable.storage.delete(targetID)
-		}
-
 		// return true if we deleted something
 		return true
 	}

src/runtime/cache/storage.ts

@@ -58,6 +58,12 @@ export class InMemoryStorage {
 		throw new Error('Could not find layer with id: ' + id)
 	}
 
+	replaceID(replacement: { from: string; to: string }) {
+		for (const layer of this.data) {
+			layer.replaceID(replacement)
+		}
+	}
+
 	get(
 		id: string,
 		field: string
@@ -83,7 +89,13 @@ export class InMemoryStorage {
 			const layer = this.data[i]
 			const [layerValue, kind] = layer.get(id, field)
 			const layerOperations = layer.getOperations(id, field) || []
-			layer.deletedIDs.forEach((v) => operations.remove.add(v))
+			layer.deletedIDs.forEach((v) => {
+				// if the layer wants to undo a delete for the id
+				if (layer.operations[v]?.undoDeletesInList?.includes(field)) {
+					return
+				}
+				operations.remove.add(v)
+			})
 
 			// if the layer does not contain a value for the field, move on
 			if (typeof layerValue === 'undefined' && layerOperations.length === 0) {
@@ -303,11 +315,13 @@ export class Layer {
 			const fieldOperations = this.operations[id]?.fields[field]
 
 			// if the operation was globally deleted
-			if (this.operations[value]?.deleted) {
+			if (this.operations[value]?.deleted || this.deletedIDs.has(value)) {
 				// undo the delete
-				// NOTE: this will bring it back to the all lists just because we inserted it
-				// to one containing the link
-				delete this.operations[value].deleted
+				this.operations[value] = {
+					...this.operations[value],
+					undoDeletesInList: [...(this.operations[id]?.undoDeletesInList || []), field],
+				}
+
 				// the value could have been removed specifically from the list
 			} else if (value && fieldOperations?.length > 0) {
 				// if we have a field operation to remove the list, undo the operation
@@ -343,7 +357,17 @@ export class Layer {
 		this.deletedIDs = new Set<string>()
 	}
 
-	applyDeletes() {
+	replaceID({ from, to }: { from: string; to: string }) {
+		// any fields that existing in from, assign to to
+		this.fields[to] = this.fields[from]
+		this.links[to] = this.links[from]
+		this.operations[to] = this.operations[from] || { fields: {} }
+		if (this.deletedIDs.has(from)) {
+			this.deletedIDs.add(to)
+		}
+	}
+
+	removeUndefinedFields() {
 		// any field that's marked as undefined needs to be deleted
 		for (const [id, fields] of Object.entries(this.fields)) {
 			for (const [field, value] of Object.entries(fields)) {
@@ -376,6 +400,8 @@ export class Layer {
 			[id]: {
 				...this.operations[id],
 				deleted: true,
+				// reapply any delete undos
+				undoDeletesInList: [],
 			},
 		}
 
@@ -429,14 +455,17 @@ export class Layer {
 			}
 
 			// if we are applying
-			if (ops.deleted) {
+			if (ops?.deleted) {
 				delete this.fields[id]
 				delete this.links[id]
 			}
 		}
 
 		// copy the field values
 		for (const [id, values] of Object.entries(layer.fields)) {
+			if (!values) {
+				continue
+			}
 			// we do have a record matching this id, copy the individual fields
 			for (const [field, value] of Object.entries(values)) {
 				this.writeField(id, field, value)
@@ -445,6 +474,9 @@ export class Layer {
 
 		// copy the link values
 		for (const [id, values] of Object.entries(layer.links)) {
+			if (!values) {
+				continue
+			}
 			// we do have a record matching this id, copy the individual links
 			for (const [field, value] of Object.entries(values)) {
 				this.writeLink(id, field, value)
@@ -479,6 +511,7 @@ type LinkMap = EntityMap<string | null | LinkedList>
 type OperationMap = {
 	[id: string]: {
 		deleted?: boolean
+		undoDeletesInList?: string[]
 		fields: { [field: string]: ListOperation[] }
 	}
 }

src/runtime/cache/tests/list.test.ts

@@ -463,10 +463,146 @@ test('remove from connection', function () {
 	expect(cache._internal_unstable.subscriptions.get('User:2', 'firstName')).toHaveLength(0)
 	// but we're still subscribing to user 3
 	expect(cache._internal_unstable.subscriptions.get('User:3', 'firstName')).toHaveLength(1)
-	// make sure we marked the corresponding edge for deletion
-	expect(
-		cache._internal_unstable.storage.topLayer.operations['User:1.friends.edges[0]'].deleted
-	).toBeTruthy()
+})
+
+test('element removed from list can be added back', function () {
+	// instantiate a cache
+	const cache = new Cache(config)
+
+	const selection: SubscriptionSelection = {
+		viewer: {
+			type: 'User',
+			keyRaw: 'viewer',
+			fields: {
+				id: {
+					type: 'ID',
+					keyRaw: 'id',
+				},
+				friends: {
+					type: 'User',
+					keyRaw: 'friends',
+					list: {
+						name: 'All_Users',
+						connection: true,
+						type: 'User',
+					},
+					fields: {
+						edges: {
+							type: 'UserEdge',
+							keyRaw: 'edges',
+							fields: {
+								node: {
+									type: 'Node',
+									keyRaw: 'node',
+									abstract: true,
+									fields: {
+										__typename: {
+											type: 'String',
+											keyRaw: '__typename',
+										},
+										id: {
+											type: 'ID',
+											keyRaw: 'id',
+										},
+										firstName: {
+											type: 'String',
+											keyRaw: 'firstName',
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	// start off associated with one object
+	cache.write({
+		selection,
+		data: {
+			viewer: {
+				id: '1',
+				friends: {
+					edges: [
+						{
+							node: {
+								__typename: 'User',
+								id: '2',
+								firstName: 'jane2',
+							},
+						},
+						{
+							node: {
+								__typename: 'User',
+								id: '3',
+								firstName: 'jane',
+							},
+						},
+					],
+				},
+			},
+		},
+	})
+
+	// a function to spy on that will play the role of set
+	const set = jest.fn()
+
+	// subscribe to the fields
+	cache.subscribe({
+		rootType: 'Query',
+		set,
+		selection: selection,
+	})
+
+	// remove user 2 from the list
+	cache.list('All_Users').remove({
+		id: '2',
+	})
+
+	cache.list('All_Users').append(
+		{
+			id: {
+				keyRaw: 'id',
+				type: 'String',
+			},
+			firstName: {
+				keyRaw: 'firstName',
+				type: 'String',
+			},
+		},
+		{
+			__typename: 'User',
+			id: '2',
+			firstName: 'jane2',
+		},
+		{}
+	)
+
+	expect(set).toHaveBeenNthCalledWith(2, {
+		viewer: {
+			id: '1',
+			friends: {
+				edges: [
+					{
+						node: {
+							__typename: 'User',
+							id: '3',
+							firstName: 'jane',
+						},
+					},
+					{
+						node: {
+							__typename: 'User',
+							id: '2',
+							firstName: 'jane2',
+						},
+					},
+				],
+			},
+		},
+	})
 })
 
 test('append in connection', function () {

src/runtime/cache/tests/storage.test.ts

@@ -181,6 +181,30 @@ describe('in memory layers', function () {
 		expect(storage.get('User:1', 'firstName').value).toBeUndefined()
 	})
 
+	test('can overwrite deletes for a specific link list', function () {
+		const storage = new InMemoryStorage()
+
+		// add a base layer with some value
+		storage.writeLink('User:1', 'friends', ['User:2'])
+
+		// add an optimistic layer that deletes the first entry
+		const layer = storage.createLayer(true)
+		layer.delete('User:2')
+
+		// make sure its removed
+		expect(storage.get('User:1', 'friends').value).toEqual([])
+
+		// resolve the optimistic layer
+		storage.resolveLayer(layer.id)
+		// sanity check
+		expect(storage.get('User:1', 'friends').value).toEqual([])
+
+		// add the entry back to the list
+		storage.writeLink('User:1', 'friends', ['User:2'])
+
+		expect(storage.get('User:1', 'friends').value).toEqual(['User:2'])
+	})
+
 	test('deleting specific fields removes the field', function () {
 		const storage = new InMemoryStorage()
 
@@ -196,7 +220,7 @@ describe('in memory layers', function () {
 
 		// delete the value
 		storage.deleteField('User:1', 'firstName')
-		storage.topLayer.applyDeletes()
+		storage.topLayer.removeUndefinedFields()
 
 		// look up the value now that it's been deleted
 		expect(storage.get('User:1', 'firstName')).toEqual({
@@ -223,7 +247,7 @@ describe('in memory layers', function () {
 
 		// delete the value
 		storage.deleteField('User:1', 'firstName')
-		storage.topLayer.applyDeletes()
+		storage.topLayer.removeUndefinedFields()
 
 		// look up the value now that it's been deleted
 		expect(storage.get('User:1', 'firstName')).toEqual({