Add unique indexes and indexes for composite types #7162

FelixMalfait · 2024-09-19T16:52:13Z

Add support for indexes on composite fields and unicity constraint on indexes

This pull request includes several changes across multiple files to improve error handling, enforce unique constraints, and update database migrations. The most important changes include updating error messages for snack bars, adding a new command to enforce unique constraints, and updating database migrations to include new fields and constraints.

FelixMalfait · 2024-10-06T10:35:02Z

Todo:

check that having 2 null-s isn't considered as a unicity constraint violation
update GraphqlQueryUpdateManyResolverService.validate, GraphqlQueryUpdateOneResolverService.validate and
GraphqlQueryCreateManyResolverService.deleteMany to perform a pre-check there and throw a user-friendly unicity violation error at application level and not wait for the error to happen at the DB level (small negative impact on performance but that seems a better approach no @Weiko ?)
finish update/migration commands
not important / after merging this, create an issue to see if we can learn from that unicity constraint error and build a mini DTO-framework like email-format validation or things like that in that validate method (?)

FelixMalfait · 2024-10-07T11:43:35Z

packages/twenty-front/src/modules/object-metadata/hooks/useFindManyObjectMetadataItems.ts

-          variant: SnackBarVariant.Error,
-        },
-      );
+      enqueueSnackBar(`${error.message}`, {


was too dev-oriented

FelixMalfait · 2024-10-07T11:43:40Z

packages/twenty-front/src/modules/object-record/hooks/useFindDuplicateRecords.ts

-              variant: SnackBarVariant.Error,
-            },
-          );
+          enqueueSnackBar(`Error finding duplicates:", ${error.message}`, {


was too dev-oriented

FelixMalfait · 2024-10-07T11:44:07Z

packages/twenty-front/src/modules/error-handler/components/PromiseRejectionEffect.tsx

@@ -20,7 +20,7 @@ export const PromiseRejectionEffect = () => {
          },
        );
      } else {
-        enqueueSnackBar(`Error: ${event.reason}`, {
+        enqueueSnackBar(`${error.message}`, {


red color and message usually already convey the fact that it's an error, and there's limited space

FelixMalfait · 2024-10-07T11:45:48Z

packages/twenty-server/src/database/typeorm/metadata/migrations/1726757368824-migrationDebt.ts

@@ -0,0 +1,53 @@
+import { MigrationInterface, QueryRunner } from 'typeorm';
+
+export class MigrationDebt1726757368824 implements MigrationInterface {


This is not directly related to this PR but it reflects that migrations <> typeorm entities were out of sync. I ran this a few weeks ago when opening the PR but it seems the debt hasn't been picked up by anyone since

Thanks! Ideally we should enforce a rule (danger?)
If *.entity.ts files are updated, a migration should be generated 🤔

.../src/database/typeorm/metadata/migrations/1726762935841-addColumnNameToIndexFieldMetadata.ts

FelixMalfait · 2024-10-07T11:47:15Z

...server/src/database/commands/upgrade-version/0-31/0-31-enforce-unique-constraints.command.ts

+import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager';
+
+@Command({
+  name: 'upgrade-0.31:enforce-unique-constraints',


This isn't ready to be reviewed yet

Weiko · 2024-10-07T14:30:10Z

packages/twenty-server/src/database/typeorm/metadata/migrations/1726757368824-migrationDebt.ts

@@ -0,0 +1,53 @@
+import { MigrationInterface, QueryRunner } from 'typeorm';
+
+export class MigrationDebt1726757368824 implements MigrationInterface {


Thanks! Ideally we should enforce a rule (danger?)
If *.entity.ts files are updated, a migration should be generated 🤔

...atabase/typeorm/metadata/migrations/1726762935841-addCompostiveColumnToIndexFieldMetadata.ts

...ql/workspace-query-runner/utils/workspace-query-runner-graphql-api-exception-handler.util.ts

packages/twenty-server/src/engine/twenty-orm/decorators/workspace-column-index.decorator.ts

Weiko · 2024-10-10T15:56:08Z

packages/twenty-front/src/modules/object-metadata/components/ObjectMetadataItemsLoadEffect.tsx

 import { WorkspaceActivationStatus } from '~/generated/graphql';
 import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems';
 import { isDeeplyEqual } from '~/utils/isDeeplyEqual';
 import { isUndefinedOrNull } from '~/utils/isUndefinedOrNull';

-const filterTsVectorFields = (


I think we want to keep that?

Oh I see it seems you handled it differently 👍

Weiko · 2024-10-10T16:11:21Z

.../src/engine/api/graphql/workspace-resolver-builder/factories/create-many-resolver.factory.ts

@@ -40,7 +40,7 @@ export class CreateManyResolverFactory

        return await this.graphqlQueryRunnerService.createMany(args, options);
      } catch (error) {
-        workspaceQueryRunnerGraphqlApiExceptionHandler(error);
+        workspaceQueryRunnerGraphqlApiExceptionHandler(error, context);


should we use internalContext here as well to keep it consistent and avoid issues with _context?

Weiko · 2024-10-10T16:13:50Z

...wenty-server/src/engine/api/graphql/workspace-schema-builder/services/type-mapper.service.ts

@@ -114,6 +115,7 @@ export class TypeMapperService {
      [FieldMetadataType.RAW_JSON, RawJsonFilterType],
      [FieldMetadataType.RICH_TEXT, StringFilterType],
      [FieldMetadataType.ARRAY, ArrayFilterType],
+      [FieldMetadataType.TS_VECTOR, StringFilterType],


I'm not sure we want that already. TS_VECTOR is dedicated to searchVector field and search is only applied in the search resolver (which does not contain filter). We might want to combine filters later on but I'm not sure this change is needed? This won't be properly reflected in the API at least, your filter won't work as expected with a StringFilter on a tsvector column

greptile-apps

PR Summary

This pull request adds support for unique indexes and indexes on composite fields, focusing on enhancing data integrity and query performance. Key changes include:

Added indexMetadatas field to ObjectMetadataItem type, supporting index information retrieval
Introduced new types IndexMetadataItem and IndexFieldMetadataItem for handling index-related data
Updated GraphQL queries and schemas to include index metadata and uniqueness constraints
Modified database migrations to add isUnique and indexWhereClause fields to index metadata
Enhanced error handling in GraphQL resolvers for unique constraint violations
Removed filtering of TsVector fields in ObjectMetadataItemsLoadEffect, potentially exposing them in the UI

_{30 file(s) reviewed, 10 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-11T13:38:57Z

packages/twenty-front/src/modules/object-metadata/hooks/useFindManyObjectMetadataItems.ts

 import { useQuery } from '@apollo/client';
+import { useMemo } from 'react';


style: Import order changed. Consider using a consistent import order throughout the codebase.

greptile-apps · 2024-10-11T13:42:29Z

packages/twenty-front/src/modules/object-metadata/validation-schemas/indexMetadataItemSchema.ts

+  indexType: z.nativeEnum(IndexType),
+  indexWhereClause: z.string().nullable(),
+  isUnique: z.boolean(),
+  objectMetadata: z.any(),


style: objectMetadata: z.any() is too permissive. Define a more specific schema

greptile-apps · 2024-10-11T13:44:27Z

packages/twenty-front/src/modules/object-record/hooks/useFindDuplicateRecords.ts

+          enqueueSnackBar(`Error finding duplicates:", ${error.message}`, {
+            variant: SnackBarVariant.Error,
+          });


style: Error message simplified, but consider including the object name for context

greptile-apps · 2024-10-11T13:44:55Z

packages/twenty-front/src/modules/object-record/hooks/useHandleFindManyRecordsError.ts

+    enqueueSnackBar(`${error.message}`, {
+      variant: SnackBarVariant.Error,
+    });


style: Consider adding a generic prefix to error messages for consistency

greptile-apps · 2024-10-11T13:47:23Z

packages/twenty-front/src/modules/object-record/record-field/utils/isFieldValueEmpty.ts

+  if (isFieldTsVector(fieldDefinition)) {
+    return false;
+  }


logic: TsVector fields are always considered non-empty. Ensure this behavior is intended and doesn't cause unexpected side effects in other parts of the application.

greptile-apps · 2024-10-11T13:47:24Z

packages/twenty-front/src/modules/object-record/record-field/utils/isFieldValueEmpty.ts

  throw new Error(
    `Entity field type not supported in isFieldValueEmpty : ${fieldDefinition.type}}`,
  );


style: Consider using a more specific error type or including the field type in the error message for easier debugging.

packages/twenty-front/src/modules/ui/field/display/components/EllipsisDisplay.tsx

charlesBochet

Big one! Let's iterate from there!

...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts

Add unique indexes

05c3ff1

FelixMalfait changed the title ~~Add unique indexes~~ Add unique indexes and indexes for composite types Sep 19, 2024

FelixMalfait added 4 commits September 19, 2024 19:00

Linter

b668d45

Improve frontend error messages

3d5e149

Merge branch 'main' into add-unique-indexes

4e79c01

Begin migration command

5d265fb

Weiko assigned Weiko and unassigned Weiko Oct 3, 2024

FelixMalfait added 2 commits October 6, 2024 10:14

Merge main and attempt to fix huge conflicts

3255104

Add additional constraints

3fdffce

charlesBochet mentioned this pull request Oct 7, 2024

Release 0.31 #7459

Closed

4 tasks

FelixMalfait added 2 commits October 7, 2024 13:42

Error handling

3b7d6cd

Merge branch 'main' into add-unique-indexes

5cfa045

FelixMalfait commented Oct 7, 2024

View reviewed changes

.../src/database/typeorm/metadata/migrations/1726762935841-addColumnNameToIndexFieldMetadata.ts Outdated Show resolved Hide resolved

FelixMalfait commented Oct 7, 2024

View reviewed changes

Remove dead code

d125250

FelixMalfait force-pushed the add-unique-indexes branch from 8a743f6 to d125250 Compare October 7, 2024 11:55

Weiko reviewed Oct 7, 2024

View reviewed changes

FelixMalfait added 7 commits October 7, 2024 17:01

Partial index for empty string + rename workspacindexcolumn

33bfe5c

Dont overload cache

fede32a

Fixes

e03888e

Drop composite column

43727a3

Display indexes on the frontend

58522fb

Improve settings display, dont hide ts vector

42d0dcd

Style update

a740bc3

FelixMalfait added 9 commits October 9, 2024 22:32

begin test fix

fb2c6a4

Lint

b4f1b5a

Fix some tests

6c147f9

IsUnique decorator and property on fields

8d33bbb

Merge main

6573253

Progress on WorkspaceUnique

f80414a

Enable workflow by default in local env

8f4471e

Fix a test

5540edb

Disable create index

ef7ecfc

Weiko reviewed Oct 10, 2024

View reviewed changes

FelixMalfait added 7 commits October 10, 2024 18:58

Fix

d84812f

Fix a test unrelated to this PR changes

1d3a5a7

Not possible to remove ts_vector from sort/filter

794e0d6

Remove console log

6920a70

Merge branch 'main' into add-unique-indexes

9beeaf6

Fix a few tests

3410bf9

Improve command

3f54928

FelixMalfait marked this pull request as ready for review October 11, 2024 13:34

greptile-apps bot reviewed Oct 11, 2024

View reviewed changes

FelixMalfait and others added 6 commits October 11, 2024 15:55

Add feature flag

886a7e9

Fix lessThan operator not applying column case

75ce755

Feature flag

a7ba38e

Merge branch 'main' into add-unique-indexes

d537979

Setup 0.32 column folder

cbb1eeb

Add tests to pass coverage

77b719c

charlesBochet approved these changes Oct 13, 2024

View reviewed changes

Fix tests

249da24

charlesBochet merged commit b792d2a into main Oct 13, 2024
8 of 9 checks passed

charlesBochet deleted the add-unique-indexes branch October 13, 2024 08:21

github-advanced-security bot found potential problems Oct 13, 2024

View reviewed changes

...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts Dismissed Show dismissed Hide dismissed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unique indexes and indexes for composite types #7162

Add unique indexes and indexes for composite types #7162

FelixMalfait commented Sep 19, 2024 •

edited

Loading

FelixMalfait commented Oct 6, 2024 •

edited

Loading

FelixMalfait Oct 7, 2024

FelixMalfait Oct 7, 2024

FelixMalfait Oct 7, 2024

FelixMalfait Oct 7, 2024

Weiko Oct 7, 2024

FelixMalfait Oct 7, 2024

Weiko Oct 7, 2024

Weiko Oct 10, 2024

Weiko Oct 10, 2024

Weiko Oct 10, 2024

Weiko Oct 10, 2024

greptile-apps bot left a comment

greptile-apps bot Oct 11, 2024

greptile-apps bot Oct 11, 2024

greptile-apps bot Oct 11, 2024

greptile-apps bot Oct 11, 2024

greptile-apps bot Oct 11, 2024

greptile-apps bot Oct 11, 2024

charlesBochet left a comment

		@@ -0,0 +1,53 @@
		import { MigrationInterface, QueryRunner } from 'typeorm';

		export class MigrationDebt1726757368824 implements MigrationInterface {

		import { useQuery } from '@apollo/client';
		import { useMemo } from 'react';

Add unique indexes and indexes for composite types #7162

Add unique indexes and indexes for composite types #7162

Conversation

FelixMalfait commented Sep 19, 2024 • edited Loading

Error Handling Improvements:

New Command for Unique Constraints:

Database Migrations:

GraphQL Exception Handling:

FelixMalfait commented Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

FelixMalfait commented Sep 19, 2024 •

edited

Loading

FelixMalfait commented Oct 6, 2024 •

edited

Loading