Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: workspace sync #3505

Merged
merged 26 commits into from
Jan 30, 2024
Merged

feat: workspace sync #3505

merged 26 commits into from
Jan 30, 2024

Conversation

magrinj
Copy link
Member

@magrinj magrinj commented Jan 17, 2024

This PR introduces a comprehensive refactoring of the workspace:sync-metadata command, alongside addressing various bug fixes. The refactoring aimed to enhance code modularity, readability, and maintainability. Key changes include the creation of new directory structures and classes, each serving a distinct purpose in the metadata synchronization process.

  1. Comparators Directory: This directory contains class designed to compare standard objects against their database counterparts. The primary function of this class is to effectively identify differences.

  2. Factories Directory: The factories directory goal is to contain object generation class. It contains logic to create new objects based on provided data. A typical use case involves creating Standard objects derived from decorators.

  3. Services Directory: Just here for more readability.

  4. Storage Directory: This directory serves as a memory storage for the create, update, and delete collections. Its primary purpose is to support operations like the dropping of fields, objects, or relations, thereby providing a robust mechanism for managing changes within the metadata environment.

Fix #3442

@magrinj magrinj force-pushed the feat/workspace-sync branch from ae63f94 to e809d04 Compare January 18, 2024 14:55
@magrinj magrinj changed the title feat: wip workspace sync feat: workspace sync Jan 25, 2024
@magrinj magrinj marked this pull request as ready for review January 25, 2024 11:43
@@ -113,6 +113,9 @@ export class WorkspaceMigrationRunnerService {
tableMigration?.columns,
);
break;
case 'drop':
await queryRunner.dropTable(`${schemaName}.${tableMigration.name}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we handle dropping tables that are referenced in other tables with non-nullable FK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually dropping is still a work in progress, I just commit the change on this part but doesn't implement it.
But you're point is good, we'll have to drop the other table before

(field) => field.name === fieldName,
);
const originalFieldMetadata = originalObjectMetadata.fields.find(
(field) => field.name === fieldName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a ticket here #3621 in preparation. We will want to use something else here as we already discussed 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! I had planned to create this kind of ticket tomorrow 🙂


for (const difference of fieldMetadataDifference) {
const fieldName = difference.path[0];
// Object shouldn't have thousands of fields, so we can use find here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:Otherwise we can also create a map<fieldName, field> for faster access?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Weiko Yes I think of it too, but I think it's a bit too much and add more variables and "complexity"

import { FeatureFlagEntity } from 'src/core/feature-flag/feature-flag.entity';

@Injectable()
export class FeatureFlagFactory {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify with the pattern we want to use. Here it seems that's something we would have originally simply put in the FeatureFlagService with a method taking a workspaceId as a parameter. What's the benefit here? Factories usually intervene when we want to instantiate complex objects so I'm wondering if this is necessary here? Or at least let's decide a pattern here because I know we don't do that for metadata module for example 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my mind, factories can be used when the create or modify an object based on other object, and we're creating a map of feature flags [key: value], it's why I've created a factory.
But if we think it can be used in other part of the code we can move it into FeatureFlagService

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To your last point: I think we will want this function in other places. Not fully sure though since usually we want to query over a specific featureFlag key and we don't need all of them but could be useful I guess 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can move it later if we need it ?

@magrinj magrinj requested a review from Weiko January 30, 2024 07:39
Copy link
Contributor

github-actions bot commented Jan 30, 2024

TODOs/FIXMEs:

  • // TODO: Should be done in a transaction: packages/twenty-server/src/workspace/workspace-migration-runner/workspace-migration-runner.service.ts

Generated by 🚫 dangerJS against 9f0d97e

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@charlesBochet charlesBochet merged commit 73f6876 into main Jan 30, 2024
12 of 13 checks passed
@charlesBochet charlesBochet deleted the feat/workspace-sync branch January 30, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Timebox] Add ability to apply migration properly on standard object
3 participants