feat: workspace health #3344

magrinj · 2024-01-10T09:08:25Z

Introducing the workspace:health Command

In this PR, we're introducing a new command: workspace:health. This command is designed to assess the health of a specified workspace by examining its metadata and database structure.

Key Features

Comprehensive Validation: Ensures each object corresponds to a table, each field aligns with a database column, and all metadata are valid based on their types.
Flexible Modes: Offers different levels of checking, including structure, metadata, or both.
Verbose Output Option: Provides detailed issue reporting for in-depth analysis.

How It Works

Run this command in your terminal as follows:

$ yarn command workspace:health --workspace-id 20202020-1c25-4d02-bf25-6aeccf7ea419

Customizable Parameters

Mode Selection (-m, --mode [all | structure | metadata]):
- all: Checks both structure and metadata.
- structure: Focuses on the database table structure based on metadata.
- metadata: Concentrates on the validity of metadata.
Verbose Output (-v, --verbose):
- When enabled, displays detailed information about all detected issues within the terminal. If not used, only a brief summary message is shown.

Warning

Relations are not yet checked

Weiko

That's great! I feel like we will benefit a lot from unit tests for this part but does not have to be in this PR 🙂

Weiko · 2024-01-11T13:05:50Z

...ages/twenty-server/src/workspace/workspace-health/services/object-metadata-health.service.ts

+        message: `Table ${objectMetadata.targetTableName} not found in schema ${schemaName}`,
+      });
+
+      return issues;


Probably unnecessary since we return issues after this condition anyway?

It's here in case we add more check after this condition, if table doesn't exist we don't have to check anything else

Alright 👍

Weiko · 2024-01-11T14:39:23Z

packages/twenty-server/src/metadata/field-metadata/utils/serialize-default-value.ts

-        return 'now()';
-      default:
-        throw new BadRequestException('Invalid dynamic default value type');
+    const serializedTypeDefaultValue = serializeTypeDefaultValue(defaultValue);


I find the naming a bit confusing because this function is already serialising default value based on their types.
I know why you did that though 😅. Maybe we should have named

{ type: something }

to

{ strategy: something }

Anyway, not a big issue, just wanted to mention it.

Yes I agree with that, manipulating it doesn't seems really nice with that name, but it's better to change this in another PR 🙂

packages/twenty-server/src/workspace/workspace-health/commands/workspace-health.command.ts

Weiko · 2024-01-11T14:42:03Z

.../twenty-server/src/workspace/workspace-health/interfaces/workspace-health-issue.interface.ts

+import { FieldMetadataEntity } from 'src/metadata/field-metadata/field-metadata.entity';
+import { ObjectMetadataEntity } from 'src/metadata/object-metadata/object-metadata.entity';
+
+export enum WorkspaceHealthIssueType {


Great! I can already see us benefiting a lot from this for debugging 💪

packages/twenty-server/src/workspace/workspace-health/services/database-structure.service.ts

Weiko · 2024-01-11T14:56:25Z

packages/twenty-server/src/workspace/workspace-health/services/database-structure.service.ts

+      defaultValue && 'value' in defaultValue ? defaultValue.value : null;
+
+    if (typeof value === 'number') {
+      return value.toString();


This is because default values are stored as "strings" in DB for numbers?

Don't really know if they are stored as string, but in my structure query they are strings

Weiko · 2024-01-11T14:58:19Z

packages/twenty-server/src/workspace/workspace-health/services/field-metadata-health.service.ts

+      );
+
+    if (!workspaceTableColumns) {
+      throw new NotFoundException(


nit: Not for this PR but we should probably not return httpExceptions in this kind of code. Actually we should probably not throw httpExceptions in services at all. Since we don't have our own Error classes yet I think it's fine for now but just wanted to mention it!

Not sure about that, in Nest.JS documentation it's advised to use HttpException for Rest or GraphQL API, we should I guess throw class errors in service and wrap them inside the controller or resolver, but it's going to be a lot verbose 😅

I see. Yes I'm sure we can find a way to make this less verbose though, I find it weird to have HttpExceptions inside commands where http is not exposed. 🤔 Let's discuss this later 👍

packages/twenty-server/src/workspace/workspace-health/services/field-metadata-health.service.ts

Weiko · 2024-01-11T15:11:14Z

packages/twenty-server/src/workspace/workspace-health/workspace-health.service.ts

+    }
+
+    // Try to connect to the data source
+    await this.typeORMService.connectToDataSource(dataSourceMetadata);


Why do we need to connect to the workspace datasource here? This is to check if this settings are valid and that we can connect later?

Yes exactly, it's just to check if the connection to this workspace is working properly

Weiko

LGTM 💪 !

charlesBochet added the PR: wip label Jan 10, 2024

magrinj force-pushed the feat/workspace-health branch 2 times, most recently from 9bc6052 to ad0fd72 Compare January 11, 2024 09:11

magrinj added 7 commits January 11, 2024 11:45

feat: wip workspace health

f4154f1

feat: split structure and metadata check

4f662e8

feat: check default value structure health

780a200

feat: check targetColumnMap structure health

ae6047d

fix: composite types doesn't have default value properly defined

2409544

feat: check default value structure health

c7a48d6

feat: check options structure health

8aa9d9d

magrinj force-pushed the feat/workspace-health branch from 465b8d2 to 8aa9d9d Compare January 11, 2024 10:45

magrinj added 3 commits January 11, 2024 11:56

fix: verbose option not working properly

39e4f3a

fix: word issue

d8dd17d

fix: tests

12abb40

magrinj added PR: awaiting review and removed PR: wip labels Jan 11, 2024

magrinj marked this pull request as ready for review January 11, 2024 11:14

magrinj requested review from charlesBochet and Weiko January 11, 2024 11:15

magrinj linked an issue Jan 11, 2024 that may be closed by this pull request

[Timebox] feat: workspace health check #3001

Closed

charlesBochet assigned Weiko Jan 11, 2024

Weiko reviewed Jan 11, 2024

View reviewed changes

magrinj added 3 commits January 11, 2024 16:26

fix: remove console.log

38e099a

fix: TRUE and FALSE instead of YES and NO

ebaa607

fix: fieldMetadataType instead of type

010eec0

Weiko approved these changes Jan 11, 2024

View reviewed changes

magrinj merged commit 5f0c9f6 into main Jan 11, 2024
13 checks passed

magrinj deleted the feat/workspace-health branch January 11, 2024 15:41

magrinj mentioned this pull request Jan 12, 2024

Support relation health check in workspace health command #3396

Closed

magrinj mentioned this pull request Feb 2, 2024

feat: add fix options to workspace health check command #3773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: workspace health #3344

feat: workspace health #3344

magrinj commented Jan 10, 2024 •

edited

Weiko left a comment

Weiko Jan 11, 2024

magrinj Jan 11, 2024

Weiko Jan 11, 2024

Weiko Jan 11, 2024

magrinj Jan 11, 2024

Weiko Jan 11, 2024

Weiko Jan 11, 2024

magrinj Jan 11, 2024

Weiko Jan 11, 2024

magrinj Jan 11, 2024

Weiko Jan 11, 2024

Weiko Jan 11, 2024

magrinj Jan 11, 2024

Weiko left a comment

feat: workspace health #3344

feat: workspace health #3344

Conversation

magrinj commented Jan 10, 2024 • edited

Key Features

How It Works

Customizable Parameters

Warning

Weiko 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

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

magrinj commented Jan 10, 2024 •

edited