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

@@ -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<Collection>) {
+    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<Collection>
+  ) {
+    if (model.index && model.changed("index")) {
+      model.index = await removeIndexCollision(model.teamId, model.index, {
+        transaction: options.transaction,
+      });
+    }
+  }
+
   // associations
 
   @BelongsTo(() => FileOperation, "importId")

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(

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<T.CollectionsCreateReq>) => {
     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<T.CollectionsMoveReq>) => {
     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,
       },
     };
   }

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,

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 {