Reorder validation execution to match migration action order (#22200)
## Summary Reorders the validation execution sequence in the workspace entity migration builder to match the actual execution order of migration actions (delete → create → update). This ensures that optimistic entity maps accurately simulate the post-migration state during validation. ## Key Changes - **Moved creation validation before update validation** in `WorkspaceEntityMigrationBuilderService`: Creation validation now executes immediately after deletion validation, allowing updates to reference entities created in the same migration without validators needing to peek into to-be-created maps. - **Removed `remainingFlatEntityMapsToValidate` parameter from update validation**: Since creation validation now completes before update validation begins, the optimistic maps already contain all created entities. Updates can safely reference newly created entities through the optimistic maps without needing access to remaining-to-create maps. - **Simplified `FlatNavigationMenuItemValidatorService`**: Removed the logic that combined remaining-to-create maps with optimistic maps, now relying solely on the optimistic maps which contain all previously validated creations. - **Updated type definition**: Modified `FlatEntityUpdateValidationArgs` type to exclude `remainingFlatEntityMapsToValidate` since it's no longer needed. ## Implementation Details This change enables a more intuitive validation flow where: 1. Deletions are validated first 2. Creations are validated next (in topological order for self-referential FKs) 3. Updates are validated last (can safely reference newly created entities) The optimistic maps are progressively built during creation validation, so by the time update validation runs, they faithfully represent the post-migration state, eliminating the need for validators to access separate remaining-to-create maps. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/22200?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This commit is contained in:
+88
-90
@@ -185,102 +185,13 @@ export abstract class WorkspaceEntityMigrationBuilderService<
|
||||
);
|
||||
this.logger.perfTime(
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'update validation',
|
||||
);
|
||||
|
||||
for (const flatEntityToUpdateUniversalIdentifier in updatedFlatEntityMaps.byUniversalIdentifier) {
|
||||
const flatEntityUpdate =
|
||||
updatedFlatEntityMaps.byUniversalIdentifier[
|
||||
flatEntityToUpdateUniversalIdentifier
|
||||
];
|
||||
|
||||
if (!isDefined(flatEntityUpdate)) {
|
||||
throw new FlatEntityMapsException(
|
||||
'Could not find flat entity updates in maps dispatcher should never occur',
|
||||
FlatEntityMapsExceptionCode.ENTITY_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
const validationResult = await this.validateFlatEntityUpdate({
|
||||
flatEntityUpdate: flatEntityUpdate.update,
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps,
|
||||
workspaceId,
|
||||
buildOptions,
|
||||
additionalCacheDataMaps,
|
||||
universalIdentifier: flatEntityToUpdateUniversalIdentifier,
|
||||
remainingFlatEntityMapsToValidate: createdFlatEntityMaps,
|
||||
});
|
||||
|
||||
if (validationResult.status === 'fail') {
|
||||
allValidationResult.push(validationResult);
|
||||
continue;
|
||||
}
|
||||
|
||||
const existingFlatEntity = findFlatEntityByUniversalIdentifier<
|
||||
MetadataUniversalFlatEntity<T>
|
||||
>({
|
||||
universalIdentifier: flatEntityToUpdateUniversalIdentifier,
|
||||
flatEntityMaps:
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps[flatEntityMapsKey],
|
||||
});
|
||||
|
||||
if (!isDefined(existingFlatEntity)) {
|
||||
throw new FlatEntityMapsException(
|
||||
'Existing flat entity to update post successful validation is not defined, should never occur',
|
||||
FlatEntityMapsExceptionCode.ENTITY_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
const updatedFlatEntity: MetadataUniversalFlatEntity<T> = {
|
||||
...existingFlatEntity,
|
||||
...flatEntityUpdate.update,
|
||||
};
|
||||
|
||||
const diff = Object.fromEntries(
|
||||
Object.entries(flatEntityUpdate.update).map(([key, after]) => [
|
||||
key,
|
||||
{
|
||||
before:
|
||||
existingFlatEntity[key as keyof MetadataUniversalFlatEntity<T>],
|
||||
after,
|
||||
},
|
||||
]),
|
||||
) as UniversalFlatEntityDiff<T>;
|
||||
|
||||
replaceUniversalFlatEntityInUniversalFlatEntityMapsThroughMutationOrThrow(
|
||||
{
|
||||
universalFlatEntity: updatedFlatEntity,
|
||||
universalFlatEntityMapsToMutate:
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps[flatEntityMapsKey],
|
||||
},
|
||||
);
|
||||
|
||||
actionsResult.update.push(
|
||||
...(Array.isArray(validationResult.action)
|
||||
? validationResult.action
|
||||
: [validationResult.action]
|
||||
).map((action) => ({
|
||||
...action,
|
||||
flatEntity: updatedFlatEntity,
|
||||
diff,
|
||||
})),
|
||||
);
|
||||
}
|
||||
|
||||
this.logger.perfTimeEnd(
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'update validation',
|
||||
'creation validation',
|
||||
);
|
||||
|
||||
const remainingFlatEntityMapsToCreate = structuredClone(
|
||||
createdFlatEntityMaps,
|
||||
);
|
||||
|
||||
this.logger.perfTime(
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'creation validation',
|
||||
);
|
||||
|
||||
const sortedCreateUniversalIdentifiers =
|
||||
topologicallySortUniversalFlatEntitiesForSelfReferentialFks({
|
||||
metadataName: this.metadataName,
|
||||
@@ -352,6 +263,93 @@ export abstract class WorkspaceEntityMigrationBuilderService<
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'creation validation',
|
||||
);
|
||||
this.logger.perfTime(
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'update validation',
|
||||
);
|
||||
|
||||
for (const flatEntityToUpdateUniversalIdentifier in updatedFlatEntityMaps.byUniversalIdentifier) {
|
||||
const flatEntityUpdate =
|
||||
updatedFlatEntityMaps.byUniversalIdentifier[
|
||||
flatEntityToUpdateUniversalIdentifier
|
||||
];
|
||||
|
||||
if (!isDefined(flatEntityUpdate)) {
|
||||
throw new FlatEntityMapsException(
|
||||
'Could not find flat entity updates in maps dispatcher should never occur',
|
||||
FlatEntityMapsExceptionCode.ENTITY_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
const validationResult = await this.validateFlatEntityUpdate({
|
||||
flatEntityUpdate: flatEntityUpdate.update,
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps,
|
||||
workspaceId,
|
||||
buildOptions,
|
||||
additionalCacheDataMaps,
|
||||
universalIdentifier: flatEntityToUpdateUniversalIdentifier,
|
||||
});
|
||||
|
||||
if (validationResult.status === 'fail') {
|
||||
allValidationResult.push(validationResult);
|
||||
continue;
|
||||
}
|
||||
|
||||
const existingFlatEntity = findFlatEntityByUniversalIdentifier<
|
||||
MetadataUniversalFlatEntity<T>
|
||||
>({
|
||||
universalIdentifier: flatEntityToUpdateUniversalIdentifier,
|
||||
flatEntityMaps:
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps[flatEntityMapsKey],
|
||||
});
|
||||
|
||||
if (!isDefined(existingFlatEntity)) {
|
||||
throw new FlatEntityMapsException(
|
||||
'Existing flat entity to update post successful validation is not defined, should never occur',
|
||||
FlatEntityMapsExceptionCode.ENTITY_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
const updatedFlatEntity: MetadataUniversalFlatEntity<T> = {
|
||||
...existingFlatEntity,
|
||||
...flatEntityUpdate.update,
|
||||
};
|
||||
|
||||
const diff = Object.fromEntries(
|
||||
Object.entries(flatEntityUpdate.update).map(([key, after]) => [
|
||||
key,
|
||||
{
|
||||
before:
|
||||
existingFlatEntity[key as keyof MetadataUniversalFlatEntity<T>],
|
||||
after,
|
||||
},
|
||||
]),
|
||||
) as UniversalFlatEntityDiff<T>;
|
||||
|
||||
replaceUniversalFlatEntityInUniversalFlatEntityMapsThroughMutationOrThrow(
|
||||
{
|
||||
universalFlatEntity: updatedFlatEntity,
|
||||
universalFlatEntityMapsToMutate:
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps[flatEntityMapsKey],
|
||||
},
|
||||
);
|
||||
|
||||
actionsResult.update.push(
|
||||
...(Array.isArray(validationResult.action)
|
||||
? validationResult.action
|
||||
: [validationResult.action]
|
||||
).map((action) => ({
|
||||
...action,
|
||||
flatEntity: updatedFlatEntity,
|
||||
diff,
|
||||
})),
|
||||
);
|
||||
}
|
||||
|
||||
this.logger.perfTimeEnd(
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'update validation',
|
||||
);
|
||||
this.logger.perfTimeEnd(
|
||||
`EntityBuilder ${this.metadataName}`,
|
||||
'entity processing',
|
||||
|
||||
+1
-1
@@ -5,7 +5,7 @@ import { type UniversalFlatEntityValidationArgs } from 'src/engine/workspace-man
|
||||
|
||||
export type FlatEntityUpdateValidationArgs<T extends AllMetadataName> = Omit<
|
||||
UniversalFlatEntityValidationArgs<T>,
|
||||
'flatEntityToValidate'
|
||||
'flatEntityToValidate' | 'remainingFlatEntityMapsToValidate'
|
||||
> & {
|
||||
flatEntityUpdate: UniversalFlatEntityUpdate<T>;
|
||||
universalIdentifier: string;
|
||||
|
||||
+2
-12
@@ -284,7 +284,6 @@ export class FlatNavigationMenuItemValidatorService {
|
||||
optimisticFlatEntityMapsAndRelatedFlatEntityMaps: {
|
||||
flatNavigationMenuItemMaps: optimisticFlatNavigationMenuItemMaps,
|
||||
},
|
||||
remainingFlatEntityMapsToValidate,
|
||||
}: FlatEntityUpdateValidationArgs<
|
||||
typeof ALL_METADATA_NAME.navigationMenuItem
|
||||
>): FailedFlatEntityValidation<'navigationMenuItem', 'update'> {
|
||||
@@ -353,21 +352,12 @@ export class FlatNavigationMenuItemValidatorService {
|
||||
|
||||
const newFolderUniversalIdentifier = folderUniversalIdentifierUpdate;
|
||||
|
||||
const combinedFlatNavigationMenuItemMaps: MetadataUniversalFlatEntityMaps<
|
||||
typeof ALL_METADATA_NAME.navigationMenuItem
|
||||
> = {
|
||||
byUniversalIdentifier: {
|
||||
...remainingFlatEntityMapsToValidate.byUniversalIdentifier,
|
||||
...optimisticFlatNavigationMenuItemMaps.byUniversalIdentifier,
|
||||
},
|
||||
};
|
||||
|
||||
const circularDependencyErrors = this.getCircularDependencyValidationErrors(
|
||||
{
|
||||
navigationMenuItemUniversalIdentifier:
|
||||
fromFlatNavigationMenuItem.universalIdentifier,
|
||||
folderUniversalIdentifier: newFolderUniversalIdentifier,
|
||||
flatNavigationMenuItemMaps: combinedFlatNavigationMenuItemMaps,
|
||||
flatNavigationMenuItemMaps: optimisticFlatNavigationMenuItemMaps,
|
||||
},
|
||||
);
|
||||
|
||||
@@ -378,7 +368,7 @@ export class FlatNavigationMenuItemValidatorService {
|
||||
const referencedParentNavigationMenuItem =
|
||||
findFlatEntityByUniversalIdentifier({
|
||||
universalIdentifier: newFolderUniversalIdentifier,
|
||||
flatEntityMaps: combinedFlatNavigationMenuItemMaps,
|
||||
flatEntityMaps: optimisticFlatNavigationMenuItemMaps,
|
||||
});
|
||||
|
||||
if (!isDefined(referencedParentNavigationMenuItem)) {
|
||||
|
||||
Reference in New Issue
Block a user