feat: Adding support for new FieldMetadataType with Postgres enums #2674

magrinj · 2023-11-23T14:14:42Z

Description

This PR introduces support for a new FieldMetadataType, utilizing Postgres enums to enhance our data modeling capabilities. We have introduced three new types: RATING, SELECT, and MULTI_SELECT.

Key features

Creation and Update of FieldMetadata:

We can now create or update FieldMetadata with additional options for these enum types. For instance, when creating a field with the type MULTI_SELECT, the options include properties like position, color, label, and value.

Example GraphQL mutation for creating a FieldMetadata:

mutation CreateOneField {
  createOneField(input: {
    field: {
      name: "multiSelect", 
      label: "Multi Select", 
      type: MULTI_SELECT, 
      objectMetadataId: "0053a033-e2a7-44c6-a2ab-34437bc82c46", 
      options: [
        {position: 0, color: "#fff", label: "First", value: "MULTI_FIRST"}, 
        {position: 1, color: "#fff", label: "Second", value: "MULTI_SECOND"}
      ]
    }
  }) {
    id
    options
  }
}

Options update
- The backend generates a unique id for each option within a FieldMetadata type. This id is crucial for the updateOneField mutation, ensuring a seamless transition between old and new enum values.

Utilization in Document queries:

When a field is established, it can now include the appropriate enum value. Here are examples of mutations for inserting enum values into a document:

mutation MyMutation {
  createDocument(data: {multiSelect: [MULTI_FIRST], name: "MultiFirst"}) {
    id
    name
    multiSelect
    createdAt
    deletedAt
    updatedAt
  }
}

Or

mutation MyMutation {
  createDocument(data: {select: FIRST, name: "First"}) {
    id
    name
    select
    createdAt
    deletedAt
    updatedAt
  }
}

Frontend Integration:
- The frontend is required to map FieldMetadata options with the enum values. This mapping will deduce properties such as color and position, facilitating the way enums are updated in all the views.

Known issues

Filtering on multiple choices enum doesn't work, it's due to pg_graphql that is not handling them properly supabase/pg_graphql#458

github-actions · 2023-11-29T11:01:24Z

TODOs/FIXMEs:

// TODO: Add validation for this but we don't have the type actually: server/src/metadata/field-metadata/dtos/update-field.input.ts

Generated by 🚫 dangerJS against 75054de

magrinj · 2023-11-29T11:03:30Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


Temporary workaround as composite type doesn't use the correct type in database, TEXT should be text not varchar.
cc @Weiko

Ok, we will need to migrate our DB though (to change the metadata). cc @charlesBochet

If it's a temporary workaround, maybe we should actually use TEXT and run the migration to have the final result directly? (which is having text in the workspace schema I guess?). Except if we want to handle SHORT_TEXT type or something and use varchar

Tbh, I'm pretty sure varchar is the same as text for postgres if you don't define a particular size

Yes see here

The TEXT data type can store a string with unlimited length. If you do not specify the n integer for the VARCHAR data type, it behaves like the TEXT datatype. The performance of the VARCHAR (without the size n) and TEXT are the same.

https://www.postgresqltutorial.com/postgresql-tutorial/postgresql-char-varchar-text/

Do we want to store currency in an enum in the future?
Plus: data is more robust
Minus: migration when geopolitics evolves :D

Regarding varchar vs text, I don't think we should use both. I think varchar should be good, and agree we should avoid generating migrations if possible

So not sure to understand the motivation behind this change

@Weiko: Ok good point that Postgres handle varchar without length in the same way as text.
I'm ok with avoiding migrations, but I don't really like to have inconsistence between the metadata and what is actually stored in the database.

For better explanation @charlesBochet, instead of always redefining the composite types like Link or Currency, I've moved the composite types definitions that initially was in the schema-builder into the field-metadata.
That way we're using the same object to define them. But it involve creating the FieldMetadata for the inner fields of composite types, and that was manually done before by manually putting the type varchar.
But actually varchar is not mapped in our FieldMetadataType.
The first thing I does is putting the type FieldMetadataType.TEXT for this fields, but this one is converted to a text column type in Postgres not a varchar.
It's why I've created a temporary type to avoid breaking changes, but from what @Weiko says, I can still put TEXT, we'll just have some inconsistency but it should work the same.

Do we want to store currency in an enum in the future? Plus: data is more robust Minus: migration when geopolitics evolves :D

It's something that we can consider

server/src/metadata/workspace-migration/factories/abstract.factory.ts

server/src/metadata/workspace-migration/factories/enum-column-action.factory.ts

server/src/workspace/workspace-migration-runner/services/workspace-migration-enum.service.ts

magrinj · 2023-11-29T11:07:27Z

server/src/workspace/workspace-migration-runner/workspace-migration-runner.service.ts

+  ) {
+    const enumValues = migrationColumn.enum;
+
+    // TODO: Maybe we can do something better if we can recreate the old `TableColumn` object


Try to rely on TypeORM by recreating the old TableColumn.
This is needed to support updating other properties at the same time.
In this PR we can't change the options of a FieldMetadata with the defaultValue at the same time.

Could you elaborate more here? What happens if we do both at same time?

If we currently do both at the same time, TypeORM migrations will brakes enum migrations an drop the enum column before recreating it, so all the data will be lost

server/src/database/typeorm/metadata/migrations/1700663879152-addEnumOptions.ts

Weiko · 2023-11-29T15:48:56Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


Ok, we will need to migrate our DB though (to change the metadata). cc @charlesBochet

Weiko · 2023-11-29T15:50:39Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


If it's a temporary workaround, maybe we should actually use TEXT and run the migration to have the final result directly? (which is having text in the workspace schema I guess?). Except if we want to handle SHORT_TEXT type or something and use varchar

Weiko · 2023-11-29T15:51:33Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


Tbh, I'm pretty sure varchar is the same as text for postgres if you don't define a particular size

Weiko · 2023-11-29T15:51:50Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


Yes see here

The TEXT data type can store a string with unlimited length. If you do not specify the n integer for the VARCHAR data type, it behaves like the TEXT datatype. The performance of the VARCHAR (without the size n) and TEXT are the same.

https://www.postgresqltutorial.com/postgresql-tutorial/postgresql-char-varchar-text/

Weiko · 2023-11-29T15:52:37Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


So instead maybe we can keep varchar, that would avoid unnecessary migrations @magrinj @charlesBochet
Ideally we should keep things consistent but looks like in this case it's fine

Weiko · 2023-11-29T15:55:07Z

server/src/metadata/workspace-migration/factories/abstract.factory.ts

+} from 'src/metadata/workspace-migration/workspace-migration.entity';
+import { FieldMetadataType } from 'src/metadata/field-metadata/field-metadata.entity';
+
+export class AbstractFactory<T extends FieldMetadataType | 'default'>


Can we use a less generic name instead of AbstractFactory? WorkspaceMigrationFactory or something like that?

@Weiko Yes we can, but actually AbstractFactory should only be used inside this folder.
I'll rename it WorkspaceColumnActionAbstractFactory

Weiko · 2023-11-29T16:02:11Z

server/src/workspace/workspace-migration-runner/services/workspace-migration-enum.service.ts

+import { WorkspaceMigrationColumnAlter } from 'src/metadata/workspace-migration/workspace-migration.entity';
+
+@Injectable()
+export class WorkspaceMigrationEnumService {


Should we prefix custom enums with "_"? @charlesBochet

I think it will because the name is based on: ${tableName}_${migrationColumn.columnName}

You mean because of table name? What if we add a custom enum on a standard object? As for the column name then yes we just to make sure we put an _ there when creating the migration

@Weiko Actually enum names are based on the field name, so if the field is _myField the enum will be something like _myTable__myField_enum

charlesBochet

Another masterpiece!

charlesBochet · 2023-11-29T16:16:48Z

infra/dev/postgres/Dockerfile

@@ -3,7 +3,7 @@ ARG PG_MAIN_VERSION=14
 FROM postgres:${PG_MAIN_VERSION} as postgres

 ARG PG_MAIN_VERSION
-ARG PG_GRAPHQL_VERSION=1.3.0
+ARG PG_GRAPHQL_VERSION=1.4.2


I will also need to update our local install script

server/src/database/typeorm/metadata/migrations/1700663879152-addEnumOptions.ts

charlesBochet · 2023-11-29T16:34:27Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


Do we want to store currency in an enum in the future?
Plus: data is more robust
Minus: migration when geopolitics evolves :D

charlesBochet · 2023-11-29T16:36:24Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


Regarding varchar vs text, I don't think we should use both. I think varchar should be good, and agree we should avoid generating migrations if possible

charlesBochet · 2023-11-29T16:36:36Z

server/src/metadata/field-metadata/composite-types/currency.composite-type.ts

@@ -22,7 +22,7 @@ export const currencyObjectDefinition = {
    } satisfies FieldMetadataInterface,
    {
      id: 'currencyCode',
-      type: FieldMetadataType.TEXT,
+      type: FieldMetadataType.VARCHAR,


So not sure to understand the motivation behind this change

charlesBochet · 2023-11-29T16:46:21Z

server/src/metadata/field-metadata/field-metadata.service.ts

+    const updatedFieldMetadata = await super.updateOne(id, record);
+
+    if (record.options || record.defaultValue) {
+      await this.workspaceMigrationService.createCustomMigration(


I am not sure about the createCustomMigration API long term. I feel that we should maybe provide something more restrictive: createColumn, udpateColumnType, updateEnumOptions...

We can let it like this for now

charlesBochet · 2023-11-29T16:49:30Z

server/src/workspace/workspace-migration-runner/services/workspace-migration-enum.service.ts

+import { WorkspaceMigrationColumnAlter } from 'src/metadata/workspace-migration/workspace-migration.entity';
+
+@Injectable()
+export class WorkspaceMigrationEnumService {


I think it will because the name is based on: ${tableName}_${migrationColumn.columnName}

charlesBochet · 2023-11-29T16:51:05Z

server/src/workspace/workspace-migration-runner/services/workspace-migration-enum.service.ts

+    );
+
+    // Update existing rows to handle missing values
+    await this.handleMissingEnumValues(


this is the first migration that will touch the data. I am very curious to see how it behaves on production.
On a regular project I would avoid that, but as we are doing migration per workspace, it's actually great. It will help migrating between versions of twenty handling data migration too and we will be able to test it on test workspaces, tier 3 workspaces before trying it on more important workspaces or clients!

charlesBochet · 2023-11-29T16:51:50Z

server/src/workspace/workspace-migration-runner/services/workspace-migration-enum.service.ts

+    );
+  }
+
+  private async renameEnumType(


so much complexity, but all steps make sense to me

charlesBochet · 2023-11-29T16:53:05Z

server/src/workspace/workspace-migration-runner/workspace-migration-runner.service.ts

+  ) {
+    const enumValues = migrationColumn.enum;
+
+    // TODO: Maybe we can do something better if we can recreate the old `TableColumn` object


Could you elaborate more here? What happens if we do both at same time?

feat: wip enum type feat: try to alter enum feat: wip enum feat: wip enum feat: schema-builder can handle enum fix: return default value in field metadata response

This reverts commit 3857539.

magrinj force-pushed the feat/enums branch from e29afc2 to a8a1ec1 Compare November 23, 2023 14:32

charlesBochet added the PR: wip label Nov 24, 2023

magrinj force-pushed the feat/enums branch from 41c75d8 to 7f4b350 Compare November 27, 2023 16:55

charlesBochet assigned magrinj Nov 28, 2023

charlesBochet added the PR: awaiting author label Nov 28, 2023

magrinj force-pushed the feat/enums branch from 4c7eef7 to 0aeefd5 Compare November 29, 2023 10:32

magrinj force-pushed the feat/enums branch from e6d5476 to cae314a Compare November 29, 2023 11:02

magrinj commented Nov 29, 2023

View reviewed changes

magrinj changed the title ~~Feat/enums~~ feat: Adding support for new FieldMetadataType with Postgres enums Nov 29, 2023

magrinj marked this pull request as ready for review November 29, 2023 14:23

magrinj requested review from charlesBochet and Weiko November 29, 2023 14:26

magrinj removed PR: wip PR: awaiting author labels Nov 29, 2023

This was linked to issues Nov 29, 2023

feat: support SELECT type on per tenant back-end #2445

Closed

feat: add new RATING type #2459

Closed

magrinj force-pushed the feat/enums branch from 9385db5 to 7a80ce6 Compare November 29, 2023 14:32

magrinj mentioned this pull request Nov 29, 2023

Update migrations alter table behaviors #2767

Closed

3 tasks

Weiko reviewed Nov 29, 2023

View reviewed changes

charlesBochet approved these changes Nov 29, 2023

View reviewed changes

charlesBochet added the PR: awaiting author label Nov 29, 2023

magrinj added 6 commits November 30, 2023 09:28

feat: add enum type (RATING, SELECT, MULTI_SELECT)

e54346a

feat: wip enum type feat: try to alter enum feat: wip enum feat: wip enum feat: schema-builder can handle enum fix: return default value in field metadata response

fix: create fieldMedata with options

f21f618

fix: lint issues

4c2df50

fix: rename abstract factory

7cee95e

feat: drop PHONE and EMAIL fieldMetadata types

3857539

feat: drop VARCHAR fieldMetadata type and rely on TEXT

04a3b11

magrinj force-pushed the feat/enums branch from 7a80ce6 to 04a3b11 Compare November 30, 2023 08:37

Revert "feat: drop PHONE and EMAIL fieldMetadata types"

75054de

This reverts commit 3857539.

magrinj merged commit 6e6f0af into main Nov 30, 2023
8 of 13 checks passed

magrinj deleted the feat/enums branch November 30, 2023 14:24

charlesBochet mentioned this pull request Dec 8, 2023

Release 0.2.1 #2879

Merged

feat: Adding support for new FieldMetadataType with Postgres enums #2674

feat: Adding support for new FieldMetadataType with Postgres enums #2674

Conversation

magrinj commented Nov 23, 2023 • edited

Description

Key features

Known issues

github-actions bot commented Nov 29, 2023 • edited

TODOs/FIXMEs:

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

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

Choose a reason for hiding this comment

magrinj Nov 30, 2023 • edited

Choose a reason for hiding this comment

charlesBochet left a 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

Choose a reason for hiding this comment

magrinj commented Nov 23, 2023 •

edited

github-actions bot commented Nov 29, 2023 •

edited

magrinj Nov 30, 2023 •

edited