add createMany fields to fieldMetadataService to batch field creation #9957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Weiko
changed the title
|
Nice! |
Weiko
commented
Jan 31, 2025
| await queryRunner.startTransaction(); | ||
|
|
||
| try { | ||
| const fieldMetadataRepository = |
There was a problem hiding this comment.
A lot of things could be refactored there, this method has many responsibilities and we could at least create a few reusable methods. I will probably not do that in this PR and only move the logic from createOne 👀
![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 optimizes field metadata creation by introducing batch processing capabilities in the FieldMetadataService.
- Added new
createManymethod inpackages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.tsto handle bulk field creation - Refactored
DataSeedWorkspaceCommandandSeederServiceto use batch creation instead of individual field creation - Reduced cache recomputation from 35 to 7 times during seeding by grouping field creations by object metadata
- Implemented single transaction handling for multiple field creations to improve database performance
- Added batch processing for view field creation while maintaining existing validation and error handling
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-server/src/database/commands/data-seed-dev-workspace.command.ts
Show resolved
Hide resolved
Comment on lines
797
to
800
| const view = await workspaceQueryRunner?.query( | ||
| `SELECT id FROM ${dataSourceMetadata.schema}."view" | ||
| WHERE "objectMetadataId" = '${createdFieldMetadata.objectMetadataId}'`, | ||
| ); |
There was a problem hiding this comment.
logic: SQL injection vulnerability in template string. Should use parameterized queries instead of string interpolation.
Suggested change
| const view = await workspaceQueryRunner?.query( | |
| `SELECT id FROM ${dataSourceMetadata.schema}."view" | |
| WHERE "objectMetadataId" = '${createdFieldMetadata.objectMetadataId}'`, | |
| ); | |
| const view = await workspaceQueryRunner?.query( | |
| 'SELECT id FROM $1:name."view" WHERE "objectMetadataId" = $2', | |
| [dataSourceMetadata.schema, createdFieldMetadata.objectMetadataId] | |
| ); |
There was a problem hiding this comment.
unlikely, those 2 variables are treated above. However I simply moved the existing code while I could have used repository pattern instead
packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts
Outdated
Show resolved
Hide resolved
…batch-transaction-and-cache-invalidation
ijreilly
approved these changes
Feb 3, 2025
packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts
Show resolved
Hide resolved
Weiko
deleted the
c--add-create-many-field-metadata-method-to-batch-transaction-and-cache-invalidation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Not exposed in the API yet, this new method allows us to reduce the time to create multiple fields at once, mostly during seeding. This allows us to batch transactions and avoid recomputing the cache everytime.
With this change, we recompute the cache 7 times instead of 35 during seeding. We could do the same for objects.