Use prepared statements + add tests for record position (#4451)

Use prepared statements + add tests

Co-authored-by: Thomas Trompette <thomast@twenty.com>

packages/twenty-server/src/workspace/workspace-query-builder/factories/__tests__/record-position-query.factory.spec.ts

@@ -16,35 +16,73 @@ describe('RecordPositionQueryFactory', () => {
   });
 
   describe('create', () => {
-    it('should return a string with the position when positionValue is first', async () => {
+    describe('createForGet', () => {
-      const positionValue = 'first';
+      it('should return a string with the position when positionValue is first', async () => {
+        const positionValue = 'first';
 
-      const result = await factory.create(
+        const result = await factory.create(
-        RecordPositionQueryType.GET,
+          RecordPositionQueryType.GET,
-        positionValue,
+          positionValue,
-        objectMetadataItem,
+          objectMetadataItem,
-        dataSourceSchema,
+          dataSourceSchema,
-      );
+        );
 
-      expect(result).toEqual(
+        expect(result).toEqual(
-        `SELECT position FROM workspace_test."company"
+          `SELECT position FROM workspace_test."company"
             WHERE "position" IS NOT NULL ORDER BY "position" ASC LIMIT 1`,
-      );
+        );
+      });
+
+      it('should return a string with the position when positionValue is last', async () => {
+        const positionValue = 'last';
+
+        const result = await factory.create(
+          RecordPositionQueryType.GET,
+          positionValue,
+          objectMetadataItem,
+          dataSourceSchema,
+        );
+
+        expect(result).toEqual(
+          `SELECT position FROM workspace_test."company"
+            WHERE "position" IS NOT NULL ORDER BY "position" DESC LIMIT 1`,
+        );
+      });
     });
 
-    it('should return a string with the position when positionValue is last', async () => {
+    it('should return a string with the position when positionValue is a number', async () => {
-      const positionValue = 'last';
+      const positionValue = 1;
+
+      try {
+        await factory.create(
+          RecordPositionQueryType.GET,
+          positionValue,
+          objectMetadataItem,
+          dataSourceSchema,
+        );
+      } catch (error) {
+        expect(error.message).toEqual(
+          'RecordPositionQueryType.GET requires positionValue to be a number',
+        );
+      }
+    });
+  });
+
+  describe('createForUpdate', () => {
+    it('should return a string when RecordPositionQueryType is UPDATE', async () => {
+      const positionValue = 1;
 
       const result = await factory.create(
-        RecordPositionQueryType.GET,
+        RecordPositionQueryType.UPDATE,
         positionValue,
         objectMetadataItem,
         dataSourceSchema,
       );
 
       expect(result).toEqual(
-        `SELECT position FROM workspace_test."company"
+        `UPDATE workspace_test."company"
-            WHERE "position" IS NOT NULL ORDER BY "position" DESC LIMIT 1`,
+            SET "position" = $1
+            WHERE "id" = $2`,
       );
     });
   });

packages/twenty-server/src/workspace/workspace-query-builder/factories/record-position-query.factory.ts

@@ -12,7 +12,6 @@ export class RecordPositionQueryFactory {
     positionValue: 'first' | 'last' | number,
     objectMetadata: { isCustom: boolean; nameSingular: string },
     dataSourceSchema: string,
-    recordId?: string,
   ): Promise<string> {
     const name =
       (objectMetadata.isCustom ? '_' : '') + objectMetadata.nameSingular;
@@ -27,24 +26,7 @@ export class RecordPositionQueryFactory {
 
         return this.createForGet(positionValue, name, dataSourceSchema);
       case RecordPositionQueryType.UPDATE:
-        if (typeof positionValue !== 'number') {
+        return this.createForUpdate(name, dataSourceSchema);
-          throw new Error(
-            'RecordPositionQueryType.UPDATE requires positionValue to be a number',
-          );
-        }
-
-        if (!recordId) {
-          throw new Error(
-            'RecordPositionQueryType.UPDATE requires recordId to be defined',
-          );
-        }
-
-        return this.createForUpdate(
-          positionValue,
-          name,
-          dataSourceSchema,
-          recordId,
-        );
       default:
         throw new Error('Invalid RecordPositionQueryType');
     }
@@ -62,13 +44,11 @@ export class RecordPositionQueryFactory {
   }
 
   private async createForUpdate(
-    positionValue: number,
     name: string,
     dataSourceSchema: string,
-    recordId: string,
   ): Promise<string> {
     return `UPDATE ${dataSourceSchema}."${name}"
-            SET "position" = ${positionValue}
+            SET "position" = $1
-            WHERE "id" = '${recordId}'`;
+            WHERE "id" = $2`;
   }
 }

packages/twenty-server/src/workspace/workspace-query-runner/factories/__tests__/record-position.factory.spec.ts

@@ -0,0 +1,64 @@
+import { TestingModule, Test } from '@nestjs/testing';
+
+import { WorkspaceDataSourceService } from 'src/workspace/workspace-datasource/workspace-datasource.service';
+import { RecordPositionQueryFactory } from 'src/workspace/workspace-query-builder/factories/record-position-query.factory';
+import { RecordPositionFactory } from 'src/workspace/workspace-query-runner/factories/record-position.factory';
+
+describe('RecordPositionFactory', () => {
+  const recordPositionQueryFactory = {
+    create: jest.fn().mockResolvedValue('query'),
+  };
+
+  const workspaceDataSourceService = {
+    getSchemaName: jest.fn().mockReturnValue('schemaName'),
+    executeRawQuery: jest.fn().mockResolvedValue([{ position: 1 }]),
+  };
+
+  let factory: RecordPositionFactory;
+
+  beforeEach(async () => {
+    const module: TestingModule = await Test.createTestingModule({
+      providers: [
+        RecordPositionFactory,
+        {
+          provide: RecordPositionQueryFactory,
+          useValue: recordPositionQueryFactory,
+        },
+        {
+          provide: WorkspaceDataSourceService,
+          useValue: workspaceDataSourceService,
+        },
+      ],
+    }).compile();
+
+    factory = module.get<RecordPositionFactory>(RecordPositionFactory);
+  });
+
+  it('should be defined', () => {
+    expect(factory).toBeDefined();
+  });
+
+  describe('create', () => {
+    const objectMetadata = { isCustom: false, nameSingular: 'company' };
+    const workspaceId = 'workspaceId';
+
+    it('should return the value when value is a number', async () => {
+      const value = 1;
+      const result = await factory.create(value, objectMetadata, workspaceId);
+
+      expect(result).toEqual(value);
+    });
+    it('should return the existing position / 2 when value is first', async () => {
+      const value = 'first';
+      const result = await factory.create(value, objectMetadata, workspaceId);
+
+      expect(result).toEqual(0.5);
+    });
+    it('should return the existing position + 1 when value is last', async () => {
+      const value = 'last';
+      const result = await factory.create(value, objectMetadata, workspaceId);
+
+      expect(result).toEqual(2);
+    });
+  });
+});

packages/twenty-server/src/workspace/workspace-query-runner/services/record-position-backfill-service.ts

@@ -36,12 +36,11 @@ export class RecordPositionBackfillService {
       position,
       objectMetadata as ObjectMetadataInterface,
       dataSourceSchema,
-      recordId,
     );
 
     this.workspaceDataSourceService.executeRawQuery(
       query,
-      [],
+      [position, recordId],
       workspaceId,
       undefined,
     );