From 877b7ad0dff0945c436fec2449378de042f92931 Mon Sep 17 00:00:00 2001 From: Hemachandar <132386067+hmacr@users.noreply.github.com> Date: Thu, 27 Mar 2025 15:20:40 +0530 Subject: [PATCH] fix: Handle index collision when creating a collection (#8803) * fix: Handle index collision when creating a collection * move to sequelize hooks * index maxLen parity between api and model * remove beforeUpdate hook * use common indexLen in model * beforeUpdate hook.. * test --- server/models/Collection.ts | 47 ++++++++++++++++++- .../api/collections/collections.test.ts | 26 ++++++++++ server/routes/api/collections/collections.ts | 38 +++++++-------- server/utils/removeIndexCollision.ts | 6 +-- server/validation.ts | 2 +- 5 files changed, 90 insertions(+), 29 deletions(-) diff --git a/server/models/Collection.ts b/server/models/Collection.ts index bd61d24711..3c15ffcacd 100644 --- a/server/models/Collection.ts +++ b/server/models/Collection.ts @@ -1,4 +1,5 @@ /* eslint-disable lines-between-class-members */ +import fractionalIndex from "fractional-index"; import find from "lodash/find"; import findIndex from "lodash/findIndex"; import remove from "lodash/remove"; @@ -11,6 +12,8 @@ import { InferAttributes, InferCreationAttributes, EmptyResultError, + type CreateOptions, + type UpdateOptions, } from "sequelize"; import { Sequelize, @@ -32,6 +35,8 @@ import { BeforeDestroy, IsDate, AllowNull, + BeforeCreate, + BeforeUpdate, } from "sequelize-typescript"; import isUUID from "validator/lib/isUUID"; import type { CollectionSort, ProsemirrorData } from "@shared/types"; @@ -41,7 +46,9 @@ import { sortNavigationNodes } from "@shared/utils/collections"; import slugify from "@shared/utils/slugify"; import { CollectionValidation } from "@shared/validations"; import { ValidationError } from "@server/errors"; +import removeIndexCollision from "@server/utils/removeIndexCollision"; import { generateUrlId } from "@server/utils/url"; +import { ValidateIndex } from "@server/validation"; import Document from "./Document"; import FileOperation from "./FileOperation"; import Group from "./Group"; @@ -217,8 +224,8 @@ class Collection extends ParanoidModel< color: string | null; @Length({ - max: 256, - msg: `index must be 256 characters or less`, + max: ValidateIndex.maxLength, + msg: `index must be ${ValidateIndex.maxLength} characters or less`, }) @Column index: string | null; @@ -324,6 +331,30 @@ class Collection extends ParanoidModel< } } + @BeforeCreate + static async setIndex(model: Collection, options: CreateOptions) { + if (model.index) { + model.index = await removeIndexCollision(model.teamId, model.index, { + transaction: options.transaction, + }); + return; + } + + const firstCollectionForTeam = await this.findOne({ + where: { + teamId: model.teamId, + }, + order: [ + // using LC_COLLATE:"C" because we need byte order to drive the sorting + Sequelize.literal('"collection"."index" collate "C"'), + ["updatedAt", "DESC"], + ], + ...options, + }); + + model.index = fractionalIndex(null, firstCollectionForTeam?.index ?? null); + } + @AfterCreate static async onAfterCreate( model: Collection, @@ -343,6 +374,18 @@ class Collection extends ParanoidModel< }); } + @BeforeUpdate + static async checkIndex( + model: Collection, + options: UpdateOptions + ) { + if (model.index && model.changed("index")) { + model.index = await removeIndexCollision(model.teamId, model.index, { + transaction: options.transaction, + }); + } + } + // associations @BelongsTo(() => FileOperation, "importId") diff --git a/server/routes/api/collections/collections.test.ts b/server/routes/api/collections/collections.test.ts index 1fc40f1a3d..492e7ff727 100644 --- a/server/routes/api/collections/collections.test.ts +++ b/server/routes/api/collections/collections.test.ts @@ -1327,6 +1327,32 @@ describe("#collections.create", () => { expect(body.policies[0].abilities.read).toBeTruthy(); }); + it("should ensure unique index across the team", async () => { + const team = await buildTeam(); + const [adminA, adminB] = await Promise.all([ + buildAdmin({ teamId: team.id }), + buildAdmin({ teamId: team.id }), + ]); + + const resA = await server.post("/api/collections.create", { + body: { + token: adminA.getJwtToken(), + name: "Test A", + }, + }); + const resB = await server.post("/api/collections.create", { + body: { + token: adminB.getJwtToken(), + name: "Test B", + }, + }); + const [bodyA, bodyB] = await Promise.all([resA.json(), resB.json()]); + + expect(resA.status).toEqual(200); + expect(resB.status).toEqual(200); + expect(bodyA.data.index).not.toEqual(bodyB.data.index); + }); + it("if index collision, should updated index of other collection", async () => { const user = await buildUser(); const createdCollectionAResponse = await server.post( diff --git a/server/routes/api/collections/collections.ts b/server/routes/api/collections/collections.ts index cab3aa048a..86af4a9fa6 100644 --- a/server/routes/api/collections/collections.ts +++ b/server/routes/api/collections/collections.ts @@ -1,4 +1,3 @@ -import fractionalIndex from "fractional-index"; import invariant from "invariant"; import Router from "koa-router"; import { Sequelize, Op, WhereOptions } from "sequelize"; @@ -42,7 +41,6 @@ import { import { APIContext } from "@server/types"; import { RateLimiterStrategy } from "@server/utils/RateLimiter"; import { collectionIndexing } from "@server/utils/indexing"; -import removeIndexCollision from "@server/utils/removeIndexCollision"; import pagination from "../middlewares/pagination"; import * as T from "./schema"; @@ -55,23 +53,21 @@ router.post( transaction(), async (ctx: APIContext) => { const { transaction } = ctx.state; - const { name, color, description, data, permission, sharing, icon, sort } = - ctx.input.body; - let { index } = ctx.input.body; + const { + name, + color, + description, + data, + permission, + sharing, + icon, + sort, + index, + } = ctx.input.body; const { user } = ctx.state.auth; authorize(user, "createCollection", user.team); - if (index) { - index = await removeIndexCollision(user.teamId, index, { transaction }); - } else { - const first = await Collection.findFirstCollectionForUser(user, { - attributes: ["id", "index"], - transaction, - }); - index = fractionalIndex(null, first ? first.index : null); - } - const collection = Collection.build({ name, content: data, @@ -959,18 +955,16 @@ router.post( transaction(), async (ctx: APIContext) => { const { transaction } = ctx.state; - const { id } = ctx.input.body; - let { index } = ctx.input.body; + const { id, index } = ctx.input.body; const { user } = ctx.state.auth; - const collection = await Collection.findByPk(id, { + let collection = await Collection.findByPk(id, { transaction, lock: transaction.LOCK.UPDATE, }); authorize(user, "move", collection); - index = await removeIndexCollision(user.teamId, index, { transaction }); - await collection.update( + collection = await collection.update( { index, }, @@ -982,14 +976,14 @@ router.post( name: "collections.move", collectionId: collection.id, data: { - index, + index: collection.index, }, }); ctx.body = { success: true, data: { - index, + index: collection.index, }, }; } diff --git a/server/utils/removeIndexCollision.ts b/server/utils/removeIndexCollision.ts index a17010c757..7f78cc502c 100644 --- a/server/utils/removeIndexCollision.ts +++ b/server/utils/removeIndexCollision.ts @@ -1,5 +1,5 @@ import fractionalIndex from "fractional-index"; -import { Op, Sequelize, type FindOptions } from "sequelize"; +import { Sequelize, type FindOptions } from "sequelize"; import Collection from "@server/models/Collection"; /** @@ -31,9 +31,7 @@ export default async function removeIndexCollision( where: { teamId, deletedAt: null, - index: { - [Op.gt]: index, - }, + index: Sequelize.literal(`"collection"."index" collate "C" > '${index}'`), }, attributes: ["id", "index"], limit: 1, diff --git a/server/validation.ts b/server/validation.ts index d1f9228d7a..ec46b884ba 100644 --- a/server/validation.ts +++ b/server/validation.ts @@ -232,7 +232,7 @@ export class ValidateDocumentId { export class ValidateIndex { public static regex = new RegExp("^[\x20-\x7E]+$"); public static message = "Must be between x20 to x7E ASCII"; - public static maxLength = 100; + public static maxLength = 256; } export class ValidateURL {