Server: Process orphaned items

packages/server/src/models/ItemModel.test.ts

@@ -497,68 +497,65 @@ describe('ItemModel', () => {
 		expect(emptyOnes.length).toBe(4);
 	});
 
-	// test('should stop importing item if it has been deleted', async function() {
-	// 	const { user: user1 } = await createUserAndSession(1);
+	// Case where an item is orphaned by the associated user has since then been
+	// deleted.
+	test('should process orphaned items - 1', async () => {
+		const { user: user1 } = await createUserAndSession(1);
+		const { user: user2 } = await createUserAndSession(2);
 
-	// 	const tempDir1 = await tempDir('storage1');
+		await createItemTree3(user1.id, '', '', [
+			{
+				id: '000000000000000000000000000000F1',
+			},
+		]);
 
-	// 	const driver = await loadStorageDriver({
-	// 		type: StorageDriverType.Filesystem,
-	// 		path: tempDir1,
-	// 	}, db());
+		await createItemTree3(user2.id, '', '', [
+			{
+				id: '000000000000000000000000000000F2',
+			},
+		]);
 
-	// 	let waitWrite = false;
-	// 	const previousWrite = driver.write.bind(driver);
-	// 	driver.write = async (itemId:string, content:Buffer, context: Context) => {
-	// 		return new Promise((resolve) => {
-	// 			const iid = setInterval(async () => {
-	// 				if (waitWrite) return;
-	// 				clearInterval(iid);
-	// 				await previousWrite(itemId, content, context);
-	// 				resolve(null);
-	// 			}, 10);
-	// 		});
-	// 	}
+		await db()('users').where('id', '=', user1.id).delete();
+		await db()('user_items').where('user_id', '=', user1.id).delete();
 
-	// 	await models().item().saveFromRawContent(user1, {
-	// 		body: Buffer.from(JSON.stringify({ 'version': 1 })),
-	// 		name: 'info.json',
-	// 	});
+		expect(await models().item().count()).toBe(2);
 
-	// 	const item = (await models().item().all())[0];
+		await models().item().processOrphanedItems();
 
-	// 	const promise = models().item().importContentToStorage(driver);
-	// 	waitWrite = false;
-	// 	await promise;
+		expect(await models().item().count()).toBe(1);
 
-	// 	expect(await driver.exists(item.id, { models: models() })).toBe(true);
+		const item = await models().item().all();
+		expect(item[0].name).toBe('000000000000000000000000000000F2.md');
+	});
 
+	// Case where an item is orphaned and the associated user is still prsent.
+	test('should process orphaned items - 2', async () => {
+		const { user: user1 } = await createUserAndSession(1);
+		const { user: user2 } = await createUserAndSession(2);
 
+		await createItemTree3(user1.id, '', '', [
+			{
+				id: '000000000000000000000000000000F1',
+			},
+		]);
 
+		await createItemTree3(user2.id, '', '', [
+			{
+				id: '000000000000000000000000000000F2',
+			},
+		]);
 
+		await db()('user_items').where('user_id', '=', user1.id).delete();
 
+		expect(await models().userItem().count()).toBe(1);
 
+		await models().item().processOrphanedItems();
 
-	// 	{
-	// 		const result = await models().item().saveFromRawContent(user1, {
-	// 			body: Buffer.from(JSON.stringify({ 'version': 2 })),
-	// 			name: 'info2.json',
-	// 		});
+		expect(await models().item().count()).toBe(2);
+		expect(await models().userItem().count()).toBe(2);
 
-	// 		const item2 = result['info2.json'].item;
-
-	// 		waitWrite = true;
-	// 		const promise = models().item().importContentToStorage(driver);
-
-	// 		await msleep(100);
-
-	// 		await models().item().delete(item2.id);
-
-	// 		waitWrite = false;
-	// 		await promise;
-
-	// 		expect(await driver.exists(item2.id, { models: models() })).toBe(false);
-	// 	}
-	// });
+		expect((await models().userItem().byUserId(user1.id)).length).toBe(1);
+		expect((await models().userItem().byUserId(user2.id)).length).toBe(1);
+	});
 
 });

packages/server/src/models/ItemModel.ts

@@ -899,6 +899,44 @@ export default class ItemModel extends BaseModel<Item> {
 		}, 'ItemModel::makeTestItems');
 	}
 
+	// I hate that this hack is necessary but it seems certain items end up with
+	// no associated user_items in the database. Thus when the user tries to
+	// update the items, the system thinks it's a new one, but then the query
+	// fails due to the UNIQUE constraints. This is now mitigated by making
+	// these items as "rejectedByTarget", which move them out of the way so that
+	// the rest of the items can be synced.
+	//
+	// To be safe we should however fix these orphaned items and that's what
+	// this function is doing.
+	//
+	// Why this happens is unclear. It's probably related to sharing notes,
+	// maybe moving them from one folder to another, then unsharing, etc. It
+	// can't be replicated so far. On Joplin Cloud it happens on only 0.0008% of
+	// items, so a simple processing task like this one is sufficient for now
+	// but it would be nice to get to the bottom of this bug.
+	public processOrphanedItems = async () => {
+		await this.withTransaction(async () => {
+			const orphanedItems: Item[] = await this.db(this.tableName)
+				.select(['items.id', 'items.owner_id'])
+				.leftJoin('user_items', 'user_items.item_id', 'items.id')
+				.whereNull('user_items.user_id');
+
+			const userIds: string[] = orphanedItems.map(i => i.owner_id);
+			const users = await this.models().user().loadByIds(userIds, { fields: ['id'] });
+
+			for (const orphanedItem of orphanedItems) {
+				if (!users.find(u => u.id)) {
+					// The user may have been deleted since then. In that case, we
+					// simply delete the orphaned item.
+					await this.delete(orphanedItem.id);
+				} else {
+					// Otherwise we add it back to the user's collection
+					await this.models().userItem().add(orphanedItem.owner_id, orphanedItem.id);
+				}
+			}
+		}, 'ItemModel::processOrphanedItems');
+	};
+
 	// This method should be private because items should only be saved using
 	// saveFromRawContent, which is going to deal with the content driver. But
 	// since it's used in various test units, it's kept public for now.
@@ -935,7 +973,7 @@ export default class ItemModel extends BaseModel<Item> {
 			} catch (error) {
 				if (isUniqueConstraintError(error)) {
 					modelLogger.error(`Unique constraint error on item: ${JSON.stringify({ id: item.id, name: item.name, jop_id: item.jop_id, owner_id: item.owner_id })}`, error);
-					throw new ErrorConflict(`This item is already present and cannot be added again: ${item.jop_id}`);
+					throw new ErrorConflict(`This item is already present and cannot be added again: ${item.name}`);
 				} else {
 					throw error;
 				}

packages/server/src/routes/api/items.test.ts

@@ -9,10 +9,10 @@ import { resourceBlobPath } from '../../utils/joplinUtils';
 import { ErrorForbidden, ErrorPayloadTooLarge } from '../../utils/errors';
 import { PaginatedResults } from '../../models/utils/pagination';
 
-describe('api_items', () => {
+describe('api/items', () => {
 
 	beforeAll(async () => {
-		await beforeAllDb('api_items');
+		await beforeAllDb('api/items');
 	});
 
 	afterAll(async () => {

packages/server/src/services/TaskService.ts

@@ -26,6 +26,7 @@ export const taskIdToLabel = (taskId: TaskId): string => {
 		[TaskId.CompressOldChanges]: _('Compress old changes'),
 		[TaskId.ProcessUserDeletions]: _('Process user deletions'),
 		[TaskId.AutoAddDisabledAccountsForDeletion]: _('Auto-add disabled accounts for deletion'),
+		[TaskId.ProcessOrphanedItems]: 'Process orphaned items',
 	};
 
 	const s = strings[taskId];

packages/server/src/services/database/types.ts

@@ -123,6 +123,7 @@ export enum TaskId {
 	CompressOldChanges = 7,
 	ProcessUserDeletions = 8,
 	AutoAddDisabledAccountsForDeletion = 9,
+	ProcessOrphanedItems = 10,
 }
 
 // AUTO-GENERATED-TYPES

packages/server/src/utils/setupTaskService.ts

@@ -53,6 +53,13 @@ export default async function(env: Env, models: Models, config: Config, services
 			schedule: '0 */6 * * *',
 			run: (models: Models) => models.session().deleteExpiredSessions(),
 		},
+
+		{
+			id: TaskId.ProcessOrphanedItems,
+			description: taskIdToLabel(TaskId.ProcessOrphanedItems),
+			schedule: '15 * * * *',
+			run: (models: Models) => models.item().processOrphanedItems(),
+		},
 	];
 
 	if (config.USER_DATA_AUTO_DELETE_ENABLED) {