chore: update field with primary key attribute (#3852)

* chore: update field with primary key attribute

* chore: test

* fix: build

* chore: test

* chore: test

* chore: test

* chore: test

* chore: test

* fix: test

* fix: test

* fix: test

* chore: unique index name

* fix: test

* chore: test

packages/core/database/src/__tests__/sync/primary-key.test.ts

@@ -75,6 +75,7 @@ describe('primary key', () => {
     await assertPrimaryKey('someField', true);
   });
 });
+
 describe.skipIf(process.env['DB_DIALECT'] === 'sqlite')('primary key not in sqlite', () => {
   let db: Database;
 

packages/core/database/src/collection.ts

@@ -357,10 +357,6 @@ export class Collection<
       );
     }
 
-    if (this.options.autoGenId !== false && options.primaryKey) {
-      this.model.removeAttribute('id');
-    }
-
     this.removeField(name);
     this.fields.set(name, field);
     this.emit('field.afterAdd', field);

packages/core/database/src/sync-runner.ts

@@ -12,6 +12,8 @@ export class SyncRunner {
   private readonly database: Database;
   private tableDescMap = {};
 
+  private uniqueAttributes: string[] = [];
+
   constructor(private model: typeof Model) {
     this.collection = model.collection;
     this.database = model.database;
@@ -63,6 +65,16 @@ export class SyncRunner {
       throw e;
     }
 
+    try {
+      const beforeColumns = await this.queryInterface.describeTable(this.tableName, options);
+      await this.handlePrimaryKeyBeforeSync(beforeColumns, options);
+      await this.handleUniqueFieldBeforeSync(beforeColumns, options);
+    } catch (e) {
+      if (!e.message.includes('No description found')) {
+        throw e;
+      }
+    }
+
     const syncResult = await this.performSync(options);
     const columns = await this.queryInterface.describeTable(this.tableName, options);
 
@@ -73,11 +85,48 @@ export class SyncRunner {
     return syncResult;
   }
 
-  async handlePrimaryKey(columns, options) {
+  async handleUniqueFieldBeforeSync(beforeColumns, options) {
-    if (!this.database.inDialect('postgres')) {
+    if (!this.database.inDialect('sqlite')) {
       return;
     }
+    // find new attributes with unique true
+    const newAttributes = Object.keys(this.rawAttributes).filter((key) => {
+      return !Object.keys(beforeColumns).includes(this.rawAttributes[key].field) && this.rawAttributes[key].unique;
+    });
 
+    this.uniqueAttributes = newAttributes;
+
+    // set unique false for new attributes to skip sequelize sync error
+    for (const newAttribute of newAttributes) {
+      this.rawAttributes[newAttribute].unique = false;
+    }
+  }
+
+  async handlePrimaryKeyBeforeSync(columns, options) {
+    const columnsBePrimaryKey = Object.keys(columns)
+      .filter((key) => {
+        return columns[key].primaryKey == true;
+      })
+      .sort();
+
+    const columnsWillBePrimaryKey = Object.keys(this.rawAttributes)
+      .filter((key) => {
+        return this.rawAttributes[key].primaryKey == true;
+      })
+      .map((key) => {
+        return this.rawAttributes[key].field;
+      })
+      .sort();
+
+    if (columnsBePrimaryKey.length == 1 && !columnsWillBePrimaryKey.includes(columnsBePrimaryKey[0])) {
+      // remove primary key
+      if (this.database.inDialect('mariadb', 'mysql')) {
+        await this.sequelize.query(`ALTER TABLE ${this.collection.quotedTableName()} DROP PRIMARY KEY;`, options);
+      }
+    }
+  }
+
+  async handlePrimaryKey(columns, options) {
     try {
       const columnsBePrimaryKey = Object.keys(columns)
         .filter((key) => {
@@ -95,22 +144,32 @@ export class SyncRunner {
         .sort();
 
       if (columnsWillBePrimaryKey.length == 0) {
-        // skip if no primary key
         return;
       }
 
-      if (JSON.stringify(columnsBePrimaryKey) != JSON.stringify(columnsWillBePrimaryKey)) {
+      if (
-        await this.queryInterface.addConstraint(this.tableName, {
+        columnsWillBePrimaryKey.length == 1 &&
-          type: 'primary key',
+        JSON.stringify(columnsBePrimaryKey) != JSON.stringify(columnsWillBePrimaryKey)
-          fields: columnsWillBePrimaryKey,
+      ) {
-          name: `${this.collection.tableName()}_${columnsWillBePrimaryKey.join('_')}_pk`,
+        if (this.database.inDialect('mariadb', 'mysql')) {
-          transaction: options?.transaction,
+          await this.sequelize.query(
-        });
+            `ALTER TABLE ${this.collection.quotedTableName()} ADD PRIMARY KEY (${columnsWillBePrimaryKey[0]});`,
+            options,
+          );
+        } else {
+          await this.queryInterface.addConstraint(this.tableName, {
+            type: 'primary key',
+            fields: columnsWillBePrimaryKey,
+            name: `${this.collection.tableName()}_${columnsWillBePrimaryKey.join('_')}_pk`,
+            transaction: options?.transaction,
+          });
+        }
       }
     } catch (e) {
       if (e.message.includes('No description found')) {
         return;
       }
+      throw e;
     }
   }
 
@@ -176,6 +235,10 @@ export class SyncRunner {
   }
 
   async handleUniqueIndex(options) {
+    for (const uniqueAttribute of this.uniqueAttributes) {
+      this.rawAttributes[uniqueAttribute].unique = true;
+    }
+
     const existsIndexes: any = await this.queryInterface.showIndex(this.collection.getTableNameWithSchema(), options);
     const existsUniqueIndexes = existsIndexes.filter((index) => index.unique);
 
@@ -225,6 +288,7 @@ export class SyncRunner {
         await this.queryInterface.addIndex(this.tableName, [this.rawAttributes[uniqueAttribute].field], {
           unique: true,
           transaction: options?.transaction,
+          name: `${this.collection.tableName()}_${this.rawAttributes[uniqueAttribute].field}_uk`,
         });
       }
     }

packages/core/server/src/__tests__/gateway.test.ts

@@ -196,12 +196,16 @@ describe('gateway', () => {
           plugins: ['nocobase'],
         });
         await waitSecond();
+
         await app.runAsCLI(['install'], {
           from: 'user',
+          throwError: true,
         });
+
         await app.runAsCLI(['start'], {
           from: 'user',
         });
+
         await waitSecond();
         clearMessages();
       });

packages/plugins/@nocobase/plugin-collection-manager/src/server/__tests__/primary-key.test.ts

@@ -1,4 +1,4 @@
-import { Database } from '@nocobase/database';
+import { Database, HasManyRepository } from '@nocobase/database';
 import { MockServer } from '@nocobase/test';
 import { createApp } from './index';
 
@@ -45,4 +45,161 @@ describe('primary key test', function () {
     const errorMessage = response.body.errors[0].message;
     expect(errorMessage).toContain('already has primary key');
   });
+
+  it('should throw error when update field in collection that already has primary key', async () => {
+    await db.getRepository('collections').create({
+      values: {
+        name: 'posts',
+        fields: [
+          {
+            name: 'id',
+            type: 'bigInt',
+            primaryKey: true,
+          },
+          {
+            name: 'title',
+            type: 'string',
+          },
+          {
+            name: 'name',
+            type: 'string',
+          },
+        ],
+      },
+      context: {},
+    });
+
+    let err;
+    try {
+      await db.getRepository<HasManyRepository>('collections.fields', 'posts').update({
+        filterByTk: 'title',
+        values: {
+          primaryKey: true,
+        },
+        context: {},
+      });
+    } catch (e) {
+      err = e;
+    }
+
+    expect(err).toBeDefined();
+  });
+
+  it.skipIf(process.env['DB_DIALECT'] === 'sqlite')('should add new primary key', async () => {
+    await db.getRepository('collections').create({
+      values: {
+        name: 'posts',
+        fields: [
+          {
+            name: 'id',
+            type: 'bigInt',
+            primaryKey: true,
+          },
+          {
+            name: 'name',
+            type: 'string',
+          },
+        ],
+      },
+      context: {},
+    });
+
+    const response1 = await app
+      .agent()
+      .resource('collections.fields', 'posts')
+      .update({
+        filterByTk: 'id',
+        values: {
+          primaryKey: false,
+        },
+      });
+
+    expect(response1.statusCode).toBe(200);
+
+    const model = db.getCollection('posts').model;
+    expect(model.rawAttributes['id'].primaryKey).toBe(false);
+
+    const response2 = await app
+      .agent()
+      .resource('collections.fields', 'posts')
+      .create({
+        values: {
+          primaryKey: true,
+          name: 'title',
+          type: 'string',
+        },
+      });
+
+    expect(response2.statusCode).toBe(200);
+
+    expect(model.rawAttributes['title'].primaryKey).toBe(true);
+    expect(model.rawAttributes['id'].primaryKey).toBe(false);
+  });
+
+  it.skipIf(process.env['DB_DIALECT'] === 'sqlite')('should update new primary key', async () => {
+    await db.getRepository('collections').create({
+      values: {
+        name: 'posts',
+        fields: [
+          {
+            name: 'id',
+            type: 'bigInt',
+            primaryKey: true,
+          },
+          {
+            name: 'name',
+            type: 'string',
+          },
+        ],
+      },
+      context: {},
+    });
+
+    const response1 = await app
+      .agent()
+      .resource('collections.fields', 'posts')
+      .update({
+        filterByTk: 'id',
+        values: {
+          primaryKey: false,
+        },
+      });
+
+    expect(response1.statusCode).toBe(200);
+
+    const model = db.getCollection('posts').model;
+    expect(model.rawAttributes['id'].primaryKey).toBe(false);
+
+    const response2 = await app
+      .agent()
+      .resource('collections.fields', 'posts')
+      .create({
+        values: {
+          name: 'title',
+          type: 'string',
+        },
+      });
+
+    expect(response2.statusCode).toBe(200);
+
+    const response3 = await app
+      .agent()
+      .resource('collections.fields', 'posts')
+      .update({
+        filterByTk: 'title',
+        values: {
+          primaryKey: true,
+        },
+      });
+
+    expect(response3.statusCode).toBe(200);
+
+    expect(model.rawAttributes['title'].primaryKey).toBe(true);
+
+    const tableInfo = await db.sequelize
+      .getQueryInterface()
+      .describeTable(db.getCollection('posts').getTableNameWithSchema());
+
+    expect(tableInfo.title.primaryKey).toBe(true);
+  });
 });

packages/plugins/@nocobase/plugin-collection-manager/src/server/__tests__/resources/collections.test.ts

@@ -101,7 +101,7 @@ describe('collections', () => {
     expect(count).toBe(0);
   });
 
-  test('remove collection 3', async () => {
+  test.skipIf(process.env['DB_DIALECT'] === 'sqlite')('remove collection 3', async () => {
     await app
       .agent()
       .resource('collections')

packages/plugins/@nocobase/plugin-collection-manager/src/server/hooks/afterCreateForForeignKeyField.ts

@@ -189,6 +189,8 @@ export function afterCreateForForeignKeyField(db: Database) {
   return async (model, options) => {
     try {
       await hook(model, options);
-    } catch (error) {}
+    } catch (error) {
+      console.log(error);
+    }
   };
 }

packages/plugins/@nocobase/plugin-collection-manager/src/server/hooks/afterCreateForReverseField.ts

@@ -8,6 +8,7 @@ export function afterCreateForReverseField(db: Database) {
     if (!reverseKey) {
       return;
     }
+
     const reverse = await Field.model.findByPk(reverseKey, { transaction });
     await reverse.update({ reverseKey: model.get('key') }, { hooks: false, transaction });
   };

packages/plugins/@nocobase/plugin-collection-manager/src/server/hooks/beforeCreateForValidateField.ts

@@ -25,3 +25,25 @@ export function beforeCreateForValidateField(db: Database) {
     }
   };
 }
+
+export function beforeUpdateForValidateField(db: Database) {
+  return async (model, { transaction }) => {
+    const isPrimaryKey = model.get('primaryKey');
+    if (isPrimaryKey) {
+      const collection = db.getCollection(model.get('collectionName'));
+      if (!collection) {
+        return;
+      }
+
+      const primaryKey = collection.model.primaryKeyAttribute;
+
+      if (primaryKey !== model.get('name') && collection.model.rawAttributes[primaryKey]) {
+        throw new Error(
+          `update field ${model.get('name')} failed, collection ${
+            collection.name
+          } already has primary key ${primaryKey}`,
+        );
+      }
+    }
+  };
+}

packages/plugins/@nocobase/plugin-collection-manager/src/server/models/field.ts

@@ -62,40 +62,6 @@ export class FieldModel extends MagicAttributeModel {
     });
   }
 
-  async migrate({ isNew, ...options }: MigrateOptions = {}) {
-    let field;
-    try {
-      field = await this.load({
-        transaction: options.transaction,
-      });
-
-      if (!field) {
-        return;
-      }
-      const collection = this.getFieldCollection();
-
-      if (isNew && collection.model.rawAttributes[this.get('name')] && this.get('unique')) {
-        // trick: set unique to false to avoid auto sync unique index
-        collection.model.rawAttributes[this.get('name')].unique = false;
-      }
-
-      await field.sync(options);
-
-      if (isNew && this.get('unique')) {
-        await this.syncUniqueIndex({
-          transaction: options.transaction,
-        });
-      }
-    } catch (error) {
-      // field sync failed, delete from memory
-      if (isNew && field) {
-        // update field should not remove field from memory
-        field.remove();
-      }
-      throw error;
-    }
-  }
-
   async remove(options?: any) {
     const collection = this.getFieldCollection();
 

packages/plugins/@nocobase/plugin-collection-manager/src/server/server.ts

@@ -13,7 +13,7 @@ import {
   beforeDestroyForeignKey,
   beforeInitOptions,
 } from './hooks';
-import { beforeCreateForValidateField } from './hooks/beforeCreateForValidateField';
+import { beforeCreateForValidateField, beforeUpdateForValidateField } from './hooks/beforeCreateForValidateField';
 import { beforeCreateForViewCollection } from './hooks/beforeCreateForViewCollection';
 import { CollectionModel, FieldModel } from './models';
 import collectionActions from './resourcers/collections';
@@ -120,21 +120,11 @@ export class CollectionManagerPlugin extends Plugin {
     this.app.db.on('fields.beforeCreate', beforeCreateForValidateField(this.app.db));
 
     this.app.db.on('fields.afterCreate', afterCreateForReverseField(this.app.db));
-
+    this.app.db.on('fields.beforeUpdate', beforeUpdateForValidateField(this.app.db));
-    this.app.db.on('fields.afterCreate', async (model: FieldModel, { context, transaction }) => {
-      if (context) {
-        await model.migrate({
-          isNew: true,
-          transaction,
-        });
-      }
-    });
-
-    // after migrate
-    this.app.db.on('fields.afterCreate', afterCreateForForeignKeyField(this.app.db));
 
     this.app.db.on('fields.beforeUpdate', async (model, options) => {
       const newValue = options.values;
+
       if (
         model.get('reverseKey') &&
         lodash.get(newValue, 'reverseField') &&
@@ -147,6 +137,7 @@ export class CollectionManagerPlugin extends Plugin {
           throw new Error('cant update field without a reverseField key');
         }
       }
+
       // todo: 目前只支持一对多
       if (model.get('sortable') && model.get('type') === 'hasMany') {
         model.set('sortBy', model.get('foreignKey') + 'Sort');
@@ -185,9 +176,36 @@ export class CollectionManagerPlugin extends Plugin {
       }
     });
 
-    this.app.db.on('fields.afterSaveWithAssociations', async (model: FieldModel, { context, transaction }) => {
+    const afterCreateForForeignKeyFieldHook = afterCreateForForeignKeyField(this.app.db);
+
+    this.app.db.on('fields.afterCreate', async (model: FieldModel, options) => {
+      const { context, transaction } = options;
       if (context) {
         await model.load({ transaction });
+        await afterCreateForForeignKeyFieldHook(model, options);
+      }
+    });
+
+    this.app.db.on('fields.afterUpdate', async (model: FieldModel, options) => {
+      const { context, transaction } = options;
+      if (context) {
+        await model.load({ transaction });
+      }
+    });
+
+    this.app.db.on('fields.afterSaveWithAssociations', async (model: FieldModel, options) => {
+      const { context, transaction } = options;
+      if (context) {
+        const collection = this.app.db.getCollection(model.get('collectionName'));
+        const syncOptions = {
+          transaction,
+          force: false,
+          alter: {
+            drop: false,
+          },
+        };
+
+        await collection.sync(syncOptions);
       }
     });
 

packages/plugins/@nocobase/plugin-custom-request/src/server/collections/customRequest.ts

@@ -3,6 +3,7 @@ import { defineCollection } from '@nocobase/database';
 export default defineCollection({
   dumpRules: 'required',
   name: 'customRequests',
+  autoGenId: false,
   fields: [
     {
       type: 'uid',