feat: populate relation join column #10212
Conversation
TODOs/FIXMEs:
|
magrinj
force-pushed
the
feat/populate-relation-join-column
branch
from
a0efa3e to
ecb8f88
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces significant changes to the field metadata and relation handling system. Here's a summary of the key changes:
The PR moves the MigrateRelationsToFieldMetadataCommand from version 0.41 to 0.42 while introducing support for relation join columns and improved type safety.
Key points to consider:
- Dependency injection will fail in 0.41 upgrade module since the command is still referenced but removed from providers
- The removal of 'default' type option across multiple files could break existing code that relies on this type
- The PR introduces a new
IsExactlytype utility but weakens type safety in some places by replacingsatisfieswithasassertions - The PR title/description doesn't accurately reflect the scope of changes being made to the metadata system
- The changes to upgrade version handling (0.41 -> 0.42) could cause issues for users following specific upgrade paths
The changes appear technically sound but require careful consideration of backwards compatibility and upgrade paths.
29 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| 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'; |
There was a problem hiding this comment.
logic: Command imported from 0.42 but used in 0.41 upgrade. This could cause version compatibility issues if 0.42 features are required.
| @@ -7,6 +7,8 @@ import { MigrateRichTextFieldCommand } from 'src/database/commands/upgrade-versi | |||
| import { UpgradeTo0_42Command } from 'src/database/commands/upgrade-version/0-42/0-42-upgrade-version.command'; | |||
| import { FeatureFlag } from 'src/engine/core-modules/feature-flag/feature-flag.entity'; | |||
| import { FeatureFlagModule } from 'src/engine/core-modules/feature-flag/feature-flag.module'; | |||
| import { MigrateRelationsToFieldMetadataCommand } from 'src/database/commands/upgrade-version/0-42/0-42-migrate-relations-to-field-metadata.command'; | |||
| import { TypeORMModule } from 'src/database/typeorm/typeorm.module'; | |||
There was a problem hiding this comment.
logic: TypeORMModule is imported but not used in the module's imports array. Either add it to imports or remove if not needed.
| import { TypeORMModule } from 'src/database/typeorm/typeorm.module'; |
| isFieldMetadataOfType(fieldMetadata, FieldMetadataType.UUID), | ||
| // TODO: Fix this, it's working in other places but not here | ||
| ) as FieldMetadataEntity<FieldMetadataType.UUID>[]; |
There was a problem hiding this comment.
logic: The TODO indicates isFieldMetadataOfType is not working here but is working elsewhere. This inconsistency needs investigation as it could lead to incorrect type filtering. Consider removing the type assertion if the function is fixed.
| @Field(() => FieldMetadataType) | ||
| type: FieldMetadataType; | ||
| type: T; |
There was a problem hiding this comment.
logic: Changing type to T ensures type-safety between the generic parameter and the actual field type. However, this could potentially break existing code that expects type to be FieldMetadataType.
| @@ -59,7 +59,7 @@ export class FieldMetadataEntity< | |||
| nullable: false, | |||
| type: 'varchar', | |||
| }) | |||
| type: FieldMetadataType; | |||
| type: T; | |||
There was a problem hiding this comment.
logic: Changing type to T could cause runtime type checking issues if T doesn't match the actual database value. Consider adding a runtime type guard.
| dataType: NumberDataType.INT, | ||
| } satisfies FieldMetadataSettings<FieldMetadataType.NUMBER>; | ||
| } as FieldMetadataSettings<FieldMetadataType.NUMBER>; |
There was a problem hiding this comment.
style: Using as instead of satisfies removes compile-time type checking. Consider keeping satisfies for better type safety.
Fix twentyhq/core-team-issues#241 (comment)