Update searchVector at label identifier update for custom fields #7588

ijreilly · 2024-10-11T10:40:03Z

By default, when custom fields are created, a searchVector field is created based on the "name" field, which is also the label identifier by default.
When this label identifier is updated, we want to update the searchVector field to use this field as searchable field instead, if it is of "searchable type" (today it is only possible to select a text or number field as label identifier, while number fields are not searchable).

…label-identifier

ijreilly · 2024-10-11T10:43:11Z

packages/twenty-server/src/engine/metadata-modules/search/search.service.ts

+    );
+
+    // index needs to be recreated as typeorm deletes then recreates searchVector column at alter
+    await this.indexMetadataService.createIndex(


@Weiko what happens in that typeOrm deletes then recreates searchVector column during update (cf PostGresQuerryRunner - changeColumn method - L751:

if (oldColumn.type !== newColumn.type || oldColumn.length !== newColumn.length || newColumn.isArray !== oldColumn.isArray || (!oldColumn.generatedType && newColumn.generatedType === "STORED") || (oldColumn.asExpression !== newColumn.asExpression && newColumn.generatedType === "STORED")) { // To avoid data conversion, we just recreate column await this.dropColumn(table, oldColumn); await this.addColumn(table, newColumn); // update cloned table clonedTable = table.clone(); } ```) So the index on the table is deleted as well, but the index metadata is untouched, creating a temporary inconsistancy. When I recreate the index right after, nothing new is saved on the index metadata table as the same information are to be saved, and the index is re-created on the table, so we are back to a consistent state. Does that sound too fragile? This could also be a potential candidate for another patch

Interesting, thanks for the details! LGTM.

greptile-apps

PR Summary

This pull request enhances the search functionality for custom fields by updating the searchVector when the label identifier is modified.

Introduced new SearchModule and SearchService to handle search vector updates
Updated FieldMetadataService and ObjectMetadataService to integrate with the new search functionality
Modified WorkspaceMigrationRunnerService to support generated columns for searchVector
Added utility functions in is-searchable-field.util.ts to determine searchable field types
Improved getTsVectorColumnExpressionFromFields to filter and validate searchable fields

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

greptile-apps · 2024-10-11T10:42:22Z

...src/engine/metadata-modules/workspace-migration/factories/ts-vector-column-action.factory.ts

+      {
+        action: WorkspaceMigrationColumnActionType.ALTER,
+        currentColumnDefinition: {
+          columnName: currentFieldMetadata.name,


style: consider using computeColumnName(currentFieldMetadata) for consistency with handleCreateAction

greptile-apps · 2024-10-11T10:42:22Z

...src/engine/metadata-modules/workspace-migration/factories/ts-vector-column-action.factory.ts

+          defaultValue: undefined,
+        },
+        alteredColumnDefinition: {
+          columnName: alteredFieldMetadata.name,


style: consider using computeColumnName(alteredFieldMetadata) for consistency with handleCreateAction

greptile-apps · 2024-10-11T10:43:16Z

...gine/workspace-manager/workspace-sync-metadata/utils/get-ts-vector-column-expression.util.ts

+  const filteredFieldsUsedForSearch = fieldsUsedForSearch.filter((field) =>
+    isSearchableFieldType(field.type),
+  );


logic: This filtering is redundant since the input type is already constrained to SearchableFieldType

greptile-apps · 2024-10-11T10:43:17Z

...gine/workspace-manager/workspace-sync-metadata/utils/get-ts-vector-column-expression.util.ts

  const columnExpressions = fieldsUsedForSearch.flatMap(
    getColumnExpressionsFromField,
  );


logic: Use filteredFieldsUsedForSearch instead of fieldsUsedForSearch here

greptile-apps · 2024-10-11T10:43:17Z

...gine/workspace-manager/workspace-sync-metadata/utils/get-ts-vector-column-expression.util.ts

+  if (filteredFieldsUsedForSearch.length < 1) {
+    throw new Error('No searchable fields found');
+  }


style: Consider using a more specific error type, such as WorkspaceMigrationException

greptile-apps · 2024-10-11T10:43:39Z

...erver/src/engine/workspace-manager/workspace-sync-metadata/utils/is-searchable-field.util.ts

+const SEARCHABLE_FIELD_TYPES = [
+  FieldMetadataType.TEXT,
+  FieldMetadataType.FULL_NAME,
+  FieldMetadataType.EMAILS,
+  FieldMetadataType.ADDRESS,
+  FieldMetadataType.LINKS,
+] as const;


style: Consider using an enum or object for SEARCHABLE_FIELD_TYPES to improve type safety and maintainability

ijreilly · 2024-10-14T09:21:30Z

...er/src/database/commands/upgrade-version/0-31/0-32/0-32-simplify-search-vector-expression.ts

+      this.logger.log(`Running command for workspace ${workspaceId}`);
+
+      try {
+        const searchVectorFields = await this.fieldMetadataRepository.findBy({


@Weiko I could add some very specific logic here to filter out fields whose entry in _typeorm_generated_columns_and_materialized_views has a value containing "CASE" (it would imply declaring/creating the repository etc for this entity that we kind of decided to set aside so far). This would make the command idempotent, while it is not completely the case here - if we run the command multiple times the output will be the same but we will have re-created the column and index.

I think it's fine recreating everything as it will be executed only once. We can even go further and truncate the _typeorm_generated_columns_and_materialized_views table

Good idea, as we just discussed I will do the truncate manually on the db so I can first test on a workspace independently.

ijreilly · 2024-10-14T09:22:16Z

...database/commands/upgrade-version/0-31/0-32/0-32-simplify-search-vector-expression.module.ts

+import { WorkspaceMigrationRunnerModule } from 'src/engine/workspace-manager/workspace-migration-runner/workspace-migration-runner.module';
+import { WorkspaceSyncMetadataCommandsModule } from 'src/engine/workspace-manager/workspace-sync-metadata/commands/workspace-sync-metadata-commands.module';
+
+@Module({


So far I did not added this command to an upgrade-0.32 set of command as we would only want to run it twice and not to run it for self hosting

…label-identifier

Weiko · 2024-10-14T12:47:40Z

...er/src/database/commands/upgrade-version/0-31/0-32/0-32-simplify-search-vector-expression.ts

+      this.logger.log(`Running command for workspace ${workspaceId}`);
+
+      try {
+        const searchVectorFields = await this.fieldMetadataRepository.findBy({


I think it's fine recreating everything as it will be executed only once. We can even go further and truncate the _typeorm_generated_columns_and_materialized_views table

Weiko · 2024-10-14T12:51:27Z

packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts

+          workspaceId,
+        );
+
+      if (isSearchEnabled && isWorkspaceMigratedForSearch) {


TODO: remove feature flag in next PR, shouldn't be necessary anymore

done in this PR !

packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts

packages/twenty-server/src/engine/metadata-modules/search/search.service.ts

…label-identifier

Weiko · 2024-10-15T14:29:30Z

packages/twenty-server/src/engine/metadata-modules/index-metadata/index-metadata.service.ts

-        ...(isDefined(indexType) ? { indexType: indexType } : {}),
-        isCustom: isCustom,
-      });
+      result = await this.indexMetadataRepository.upsert(


Should we rename the method upsertIndex? @ijreilly

I wouldn't - it still creates the index, the upsert only concerns the entry in metadata table. but a migration to create the index is added anyway

You are right @ijreilly!

Weiko

LGTM, good work!

ijreilly added 4 commits October 7, 2024 11:06

wip

c0668c6

Merge branch 'main' of github.com:twentyhq/twenty into search-update-…

2b239cd

…label-identifier

Add generatedType in migration

7531e4f

Recreate index at search vector update

4abd180

ijreilly commented Oct 11, 2024

View reviewed changes

ijreilly requested a review from Weiko October 11, 2024 10:43

greptile-apps bot reviewed Oct 11, 2024

View reviewed changes

charlesBochet added the -PR: awaiting review label Oct 11, 2024

charlesBochet assigned Weiko and ijreilly Oct 11, 2024

Add command to simplify searchVector expression + refactor

6bb9aaa

ijreilly commented Oct 14, 2024

View reviewed changes

ijreilly added 2 commits October 14, 2024 11:32

Merge branch 'main' of github.com:twentyhq/twenty into search-update-…

524002d

…label-identifier

Fix tests

f9f8f38

Weiko reviewed Oct 14, 2024

View reviewed changes

ijreilly added 5 commits October 15, 2024 15:07

Remove feature flags

c9a8603

Add await and detail more in error message

552ecee

change command name

ac4589a

Merge branch 'main' of github.com:twentyhq/twenty into search-update-…

a8650a1

…label-identifier

Perform upsert at index creation and add constraint on indexMetadata

a795da8

Weiko reviewed Oct 15, 2024

View reviewed changes

Weiko approved these changes Oct 15, 2024

View reviewed changes

ijreilly merged commit 1de7391 into main Oct 15, 2024
14 of 16 checks passed

ijreilly deleted the search-update-label-identifier branch October 15, 2024 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update searchVector at label identifier update for custom fields #7588

Update searchVector at label identifier update for custom fields #7588

ijreilly commented Oct 11, 2024

ijreilly Oct 11, 2024

Weiko Oct 14, 2024 •

edited

Loading

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

ijreilly Oct 14, 2024

Weiko Oct 14, 2024

ijreilly Oct 15, 2024

ijreilly Oct 14, 2024

Weiko Oct 14, 2024

Weiko Oct 14, 2024

ijreilly Oct 15, 2024

Weiko Oct 15, 2024

ijreilly Oct 15, 2024

Weiko Oct 15, 2024

Weiko left a comment

Update searchVector at label identifier update for custom fields #7588

Update searchVector at label identifier update for custom fields #7588

Conversation

ijreilly commented Oct 11, 2024

Choose a reason for hiding this comment

Weiko Oct 14, 2024 • edited Loading

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

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

Weiko left a comment

Choose a reason for hiding this comment

Weiko Oct 14, 2024 •

edited

Loading