self-hosted/joplin
1
0
Fork 0
mirror of https://github.com/laurent22/joplin.git synced 2026-06-18 20:16:34 +02:00
Code Issues Releases Activity

Server: Fixes #12984: Improve handling of concurrent deletion requests for the same item (#13092)

Browse Source
This commit is contained in:
Henry Heino
2025-09-08 12:03:20 +01:00
committed by GitHub
parent f6b3f9860c
commit 5492ce55fa
6 changed files with 82 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/server/src/models/ItemModel.test.ts
+10
View File
@@ -634,4 +634,14 @@ describe('ItemModel', () => {
expect(jopItems.map(it => it.title).sort()).toEqual(['Folder 1', 'Note 1', 'Note 2']);
});
test('should allow ignoring no-op deletions', async () => {
await createUserAndSession(1);
await expect((async () => {
await models().item().delete('00000000000000000000000000000003', { allowNoOp: false });
})()).rejects.toThrow();
await models().item().delete('00000000000000000000000000000003', { allowNoOp: true });
});
});
packages/server/src/models/ItemModel.ts
+3 -3
View File
@@ -909,7 +909,7 @@ export default class ItemModel extends BaseModel<Item> {
}, 'ItemModel::delete');
}
public async deleteForUser(userId: Uuid, item: Item): Promise<void> {
public async deleteForUser(userId: Uuid, item: Item, options: DeleteOptions = {}): Promise<void> {
if (this.isRootSharedFolder(item)) {
const share = await this.models().share().byItemId(item.id);
if (!share) throw new Error(`Cannot find share associated with item ${item.id}`);
@@ -920,10 +920,10 @@ export default class ItemModel extends BaseModel<Item> {
await this.models().shareUser().delete(userShare.id);
} else if (share.owner_id === userId) {
// Delete the share for everyone
await this.delete(item.id);
await this.delete(item.id, options);
}
} else {
await this.delete(item.id);
await this.delete(item.id, options);
}
}
packages/server/src/routes/api/items.test.ts
+45 -1
View File
@@ -1,4 +1,4 @@
import { beforeAllDb, afterAllTests, beforeEachDb, createUserAndSession, models, createItem, makeTempFileWithContent, makeNoteSerializedBody, createItemTree, expectHttpError, createNote, expectNoHttpError, getItem } from '../../utils/testing/testUtils';
import { beforeAllDb, afterAllTests, beforeEachDb, createUserAndSession, models, createItem, makeTempFileWithContent, makeNoteSerializedBody, createItemTree, expectHttpError, createNote, expectNoHttpError, getItem, deleteItem, createBaseAppContext } from '../../utils/testing/testUtils';
import { NoteEntity } from '@joplin/lib/services/database/types';
import { ModelType } from '@joplin/lib/BaseModel';
import { deleteApi, getApi, putApi } from '../../utils/testing/apiUtils';
@@ -126,6 +126,23 @@ describe('api/items', () => {
expect(ids.sort()).toEqual(['000000000000000000000000000000F2', '00000000000000000000000000000002'].sort());
});
test('delete should not error if an item does not exist', async () => {
const { user, session } = await createUserAndSession(1, true);
const tree = {
'000000000000000000000000000000F1': { },
};
const itemModel = models().item();
await createItemTree(user.id, '', tree);
await deleteApi(session.id, 'items/root:/12345600000000000000000000000000.md:');
// Should not have deleted the folder
expect((await itemModel.all()).length).toBe(1);
expect((await itemModel.all())[0].jop_id).toBe('000000000000000000000000000000F1');
});
test('should get back the serialized note', async () => {
const { session } = await createUserAndSession(1, true);
@@ -397,4 +414,31 @@ describe('api/items', () => {
);
});
test('should support multiple delete requests for the same item at the same time', async () => {
const { user: user1, session: session1 } = await createUserAndSession(1);
await createItemTree(user1.id, '', {
'000000000000000000000000000000F1': {
'00000000000000000000000000000001': null,
'00000000000000000000000000000002': null,
'00000000000000000000000000000003': null,
},
});
const baseAppContext = await createBaseAppContext();
// Should not fail
await Promise.all([
deleteItem(session1.id, '00000000000000000000000000000001', baseAppContext),
deleteItem(session1.id, '00000000000000000000000000000001', baseAppContext),
deleteItem(session1.id, '00000000000000000000000000000002', baseAppContext),
deleteItem(session1.id, '00000000000000000000000000000002', baseAppContext),
]);
// Should have deleted the items
expect(await models().item().loadByJopId(user1.id, '00000000000000000000000000000001')).toBeNull();
expect(await models().item().loadByJopId(user1.id, '00000000000000000000000000000002')).toBeNull();
// Should not have deleted the other item
expect(await models().item().loadByJopId(user1.id, '00000000000000000000000000000003')).toBeTruthy();
});
});
packages/server/src/routes/api/items.ts
+7 -1
View File
@@ -108,7 +108,13 @@ router.del('api/items/:id', async (path: SubPath, ctx: AppContext) => {
} else {
const item = await itemFromPath(ctx.joplin.owner.id, ctx.joplin.models.item(), path);
await ctx.joplin.models.item().checkIfAllowed(ctx.joplin.owner, AclAction.Delete, item);
await ctx.joplin.models.item().deleteForUser(ctx.joplin.owner.id, item);
await ctx.joplin.models.item().deleteForUser(
ctx.joplin.owner.id,
item,
// Even though item has already been fetched, a no-op can
// still happen if two users try to delete the item at the same time.
{ allowNoOp: true },
);
}
} catch (error) {
if (error instanceof ErrorNotFound) {
packages/server/src/utils/testing/apiUtils.ts
+5
View File
@@ -6,6 +6,10 @@ interface ExecRequestOptions {
filePath?: string;
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
query?: Record<string, any>;
// Providing a baseAppContext allows skipping some amount of setup logic that can cause issues when
// called many times concurrently.
baseAppContext?: AppContext;
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
@@ -70,6 +74,7 @@ export async function execRequestC(sessionId: string, method: string, path: stri
if (options.filePath) appContextOptions.request.files = { file: { filepath: options.filePath } };
if (options.query) appContextOptions.request.query = options.query;
if (options.baseAppContext) appContextOptions.baseAppContext = options.baseAppContext;
const context = await koaAppContext(appContextOptions);
await routeHandler(context);
packages/server/src/utils/testing/testUtils.ts
+12 -6
View File
@@ -177,6 +177,8 @@ export interface AppContextTestOptions {
sessionId?: string;
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
request?: any;
baseAppContext?: AppContext;
}
export function msleep(ms: number) {
@@ -206,6 +208,12 @@ export function msleep(ms: number) {
});
}
export const createBaseAppContext = () => {
const appLogger = Logger.create('AppTest');
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
return setupAppContext({} as any, Env.Dev, db_, dbSlave_, () => appLogger);
};
export async function koaAppContext(options: AppContextTestOptions = null): Promise<AppContext> {
if (!db_) throw new Error('Database must be initialized first');
@@ -238,15 +246,13 @@ export async function koaAppContext(options: AppContextTestOptions = null): Prom
const req = httpMocks.createRequest(reqOptions);
req.__isMocked = true;
const appLogger = Logger.create('AppTest');
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
const baseAppContext = await setupAppContext({} as any, Env.Dev, db_, dbSlave_, () => appLogger);
const baseAppContext = options.baseAppContext ?? await createBaseAppContext();
// Set type to "any" because the Koa context has many properties and we
// don't need to mock all of them.
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
const appContext: any = {
joplinBase: baseAppContext.joplinBase,
baseAppContext,
joplin: {
...baseAppContext.joplinBase,
@@ -406,8 +412,8 @@ export async function updateItem(sessionId: string, path: string, content: strin
return models().item().load(item.id);
}
export async function deleteItem(sessionId: string, jopId: string): Promise<void> {
await deleteApi(sessionId, `items/root:/${jopId}.md:`);
export async function deleteItem(sessionId: string, jopId: string, baseAppContext?: AppContext): Promise<void> {
await deleteApi(sessionId, `items/root:/${jopId}.md:`, { baseAppContext });
}
export async function createNote(sessionId: string, note: NoteEntity): Promise<Item> {