feat: populate join column for new relation #9974

magrinj · 2025-02-03T12:32:02Z

No description provided.

...c/database/commands/upgrade-version/0-41/0-41-migrate-relations-to-field-metadata.command.ts

charlesBochet

Agree on what's in the PR for now, happy to re-review it when it makes sense

greptile-apps

PR Summary

This PR implements significant changes to handle relation field metadata and join columns more effectively. It involves a major TypeScript upgrade and extensive type system improvements.

Key changes:

Moves MigrateRelationsToFieldMetadataCommand from v0.41 to v0.42 upgrade module with enhanced join column handling
Introduces new isFieldMetadataOfType utilities replacing isRelationFieldMetadata for better type safety
Removes 'default' type option from field metadata interfaces in favor of strict FieldMetadataType
Adds joinColumnName property to FieldMetadataRelationSettings for explicit join column configuration
Updates TypeScript target to ES2018 across packages to support newer language features

The changes appear well-structured but require careful testing of upgrade paths and existing code compatibility due to the removal of the 'default' type option.

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

greptile-apps · 2025-02-13T15:26:41Z

...ges/twenty-server/src/database/commands/upgrade-version/0-41/0-41-upgrade-version.command.ts

 import { RemoveDuplicateMcmasCommand } from 'src/database/commands/upgrade-version/0-41/0-41-remove-duplicate-mcmas';
 import { SeedWorkflowViewsCommand } from 'src/database/commands/upgrade-version/0-41/0-41-seed-workflow-views.command';
+import { MigrateRelationsToFieldMetadataCommand } from 'src/database/commands/upgrade-version/0-42/0-42-migrate-relations-to-field-metadata.command';


logic: The migration command has been moved from 0.41 to 0.42 but is still being used in the 0.41 upgrade. This could cause version compatibility issues if there are dependencies between migrations.

greptile-apps · 2025-02-13T15:26:48Z

packages/twenty-server/package.json

@@ -78,7 +78,7 @@
    "rimraf": "^5.0.5",
    "twenty-emails": "workspace:*",
    "twenty-shared": "workspace:*",
-    "typescript": "5.3.3"
+    "typescript": "5.7.2"


logic: TypeScript 5.7.2 requires Node.js >= 18.18.0, but the engine specification on line 84 requires ^18.17.1. This could lead to compatibility issues.

Suggested change

"typescript": "5.7.2"

"node": "^18.18.0",

greptile-apps · 2025-02-13T15:28:34Z

...c/database/commands/upgrade-version/0-42/0-42-migrate-relations-to-field-metadata.command.ts

+    const joinColumnFieldMetadata = joinColumnFieldMetadataCollection.find(
+      (joinColumnFieldMetadata) =>
+        joinColumnFieldMetadata.name === `${fieldMetadata.name}Id`,
+    );


style: Join column matching relies on exact name pattern. Consider adding a more flexible matching mechanism or validation for edge cases

greptile-apps · 2025-02-13T15:30:54Z

...er/src/engine/metadata-modules/field-metadata/interfaces/field-metadata-options.interface.ts

+  IsExactly<T, FieldMetadataType> extends true
+    ? FieldMetadataDefaultOption[] | FieldMetadataComplexOption[]


logic: This condition will only be true when T is exactly FieldMetadataType, not when T is a specific type from FieldMetadataType. This may cause unexpected behavior when used with specific field types.

greptile-apps · 2025-02-13T15:31:15Z

...r/src/engine/metadata-modules/field-metadata/interfaces/field-metadata-settings.interface.ts

+  IsExactly<T, FieldMetadataType> extends true
+    ? FieldMetadataDefaultSettings
+    : T extends keyof FieldMetadataSettingsMapping
+      ? FieldMetadataSettingsMapping[T] & FieldMetadataDefaultSettings
+      : never;


logic: the new type checking logic using IsExactly will return FieldMetadataDefaultSettings for exact FieldMetadataType matches, which is different from the previous behavior where it would return never. This could cause type mismatches in existing code

greptile-apps · 2025-02-13T15:33:12Z

...-server/src/engine/metadata-modules/remote-server/remote-table/utils/udt-name-mapper.util.ts

        dataType: NumberDataType.INT,
-      } satisfies FieldMetadataSettings<FieldMetadataType.NUMBER>;
+      } as FieldMetadataSettings<FieldMetadataType.NUMBER>;


style: Using 'as' type assertion instead of 'satisfies' reduces type safety. Consider keeping 'satisfies' to ensure type correctness at compile time.

greptile-apps · 2025-02-13T15:34:33Z

packages/twenty-server/src/engine/seeder/metadata-seeds/survey-results-metadata-seeds.ts

        dataType: NumberDataType.FLOAT,
        decimals: 6,
        type: 'percentage',


logic: decimals is set to 6 for percentage but label indicates 3 decimals

Suggested change

dataType: NumberDataType.FLOAT,

decimals: 6,

type: 'percentage',

dataType: NumberDataType.FLOAT,

decimals: 3,

type: 'percentage',

greptile-apps · 2025-02-13T15:35:28Z

packages/twenty-server/src/engine/utils/isUUIDFieldMetadata.util.ts

+import { FieldMetadataInterface } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata.interface';
+
+export const isUUIDFieldMetadata = (
+  fieldMetadata: FieldMetadataInterface<any>,


style: Using any here loses type safety. Consider using FieldMetadataType instead, similar to other type guard utilities in the codebase.

greptile-apps · 2025-02-13T15:36:05Z

packages/twenty-shared/src/types/IsExactly.ts

+export type IsExactly<T, U> = [T] extends [U]
+  ? [U] extends [T]
+    ? true
+    : false
+  : false;


style: Consider adding JSDoc comments to explain the purpose and behavior of this type utility, including examples of usage.

Suggested change

export type IsExactly<T, U> = [T] extends [U]

? [U] extends [T]

? true

: false

: false;

/**

* Checks if two types are exactly the same.

*

* @example

* type A = IsExactly<string, string>; // true

* type B = IsExactly<string | number, string>; // false

* type C = IsExactly<{ a: string }, { a: string }>; // true

*/

export type IsExactly<T, U> = [T] extends [U]

? [U] extends [T]

? true

: false

: false;

greptile-apps · 2025-02-13T15:36:45Z

tsconfig.base.json

@@ -8,7 +8,7 @@
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "importHelpers": true,
-    "target": "es2015",
+    "target": "ES2018",
    "module": "esnext",
    "lib": ["es2020", "dom"],


style: lib includes es2020 but target is ES2018. Consider aligning these versions to avoid potential issues with TypeScript's type checking.

Suggested change

"lib": ["es2020", "dom"],

"lib": ["es2018", "dom"],

fix

charlesBochet reviewed Feb 11, 2025

View reviewed changes

...c/database/commands/upgrade-version/0-41/0-41-migrate-relations-to-field-metadata.command.ts Show resolved Hide resolved

charlesBochet reviewed Feb 11, 2025

View reviewed changes

charlesBochet added the -PR: awaiting author label Feb 11, 2025

charlesBochet assigned magrinj Feb 11, 2025

magrinj force-pushed the feat/relation-populate-join-column branch 4 times, most recently from 3b8b4d5 to 9b88290 Compare February 13, 2025 15:15

magrinj marked this pull request as ready for review February 13, 2025 15:24

greptile-apps bot reviewed Feb 13, 2025

View reviewed changes

magrinj added 11 commits February 14, 2025 09:45

feat: populate join column for new relation

c321de5

fix: typing issue

2af4c0b

fix: typecheck issue

5d484e6

feat: move to 0.42

1cc8209

wip

ae832e1

fix: missing module

5b1db9c

wip

61e878a

wip

7cc53b8

wip

0b42f67

fix: typing issue

5aa691e

fix: try to bump typescript

256ad8f

fix

magrinj force-pushed the feat/relation-populate-join-column branch from 03ea600 to 256ad8f Compare February 14, 2025 08:45

magrinj closed this Feb 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: populate join column for new relation #9974

feat: populate join column for new relation #9974

magrinj commented Feb 3, 2025

charlesBochet left a comment

greptile-apps bot left a comment

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

greptile-apps bot Feb 13, 2025

		IsExactly<T, FieldMetadataType> extends true
		? FieldMetadataDefaultOption[] \| FieldMetadataComplexOption[]

feat: populate join column for new relation #9974

feat: populate join column for new relation #9974

Conversation

magrinj commented Feb 3, 2025

charlesBochet left a 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 Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment