diff --git a/src/DependencyInjection/migration.xml b/src/DependencyInjection/migration.xml index b008b1b85..071e8101c 100644 --- a/src/DependencyInjection/migration.xml +++ b/src/DependencyInjection/migration.xml @@ -426,6 +426,9 @@ + + + diff --git a/src/Exception/MigrationException.php b/src/Exception/MigrationException.php index ff1c28442..65bbf8a1c 100644 --- a/src/Exception/MigrationException.php +++ b/src/Exception/MigrationException.php @@ -95,7 +95,7 @@ class MigrationException extends HttpException final public const INVALID_ID = 'SWAG_MIGRATION__INVALID_ID'; - public const DUPLICATE_SOURCE_CONNECTION = 'SWAG_MIGRATION__DUPLICATE_SOURCE_CONNECTION'; + final public const DUPLICATE_SOURCE_CONNECTION = 'SWAG_MIGRATION__DUPLICATE_SOURCE_CONNECTION'; public static function associationEntityRequiredMissing(string $entity, string $missingEntity): self { diff --git a/src/Migration/Mapping/MappingService.php b/src/Migration/Mapping/MappingService.php index d84ab7b04..dd1e30812 100644 --- a/src/Migration/Mapping/MappingService.php +++ b/src/Migration/Mapping/MappingService.php @@ -216,6 +216,15 @@ public function getMappings(string $connectionId, string $entityName, array $ids public function hasValidMappingByEntityId(string $connectionId, string $entityName, string $entityId, Context $context): bool { + // check in write array first to avoid unnecessary db calls and find not yet written mappings + foreach ($this->writeArray as $writeMapping) { + if ($writeMapping['connectionId'] !== $connectionId || $writeMapping['entityId'] !== $entityId) { + continue; + } + + return $writeMapping['oldIdentifier'] !== null; + } + $criteria = new Criteria(); $criteria->addFilter( new EqualsFilter('connectionId', $connectionId), diff --git a/src/Migration/Service/MediaFileProcessorService.php b/src/Migration/Service/MediaFileProcessorService.php index d4c8c2c56..ce3983fcc 100644 --- a/src/Migration/Service/MediaFileProcessorService.php +++ b/src/Migration/Service/MediaFileProcessorService.php @@ -108,6 +108,7 @@ private function getMediaFiles(MigrationContextInterface $migrationContext): arr ->from('swag_migration_media_file') ->where('run_id = :runId') ->andWhere('written = 1') + ->andWhere('processed = 0') ->orderBy('entity, file_size') ->setFirstResult($migrationContext->getOffset()) ->setMaxResults($migrationContext->getLimit()) diff --git a/src/Migration/Service/MigrationDataWriter.php b/src/Migration/Service/MigrationDataWriter.php index c62982dd0..40c9e5cf4 100644 --- a/src/Migration/Service/MigrationDataWriter.php +++ b/src/Migration/Service/MigrationDataWriter.php @@ -73,6 +73,7 @@ public function writeData(MigrationContextInterface $migrationContext, Context $ $criteria->addFilter(new EqualsFilter('entity', $dataSet::getEntity())); $criteria->addFilter(new EqualsFilter('runId', $migrationContext->getRunUuid())); $criteria->addFilter(new EqualsFilter('convertFailure', false)); + $criteria->addFilter(new EqualsFilter('written', false)); $criteria->setOffset($migrationContext->getOffset()); $criteria->setLimit($migrationContext->getLimit()); $criteria->addSorting(new FieldSorting('autoIncrement', FieldSorting::ASCENDING)); @@ -133,7 +134,7 @@ public function writeData(MigrationContextInterface $migrationContext, Context $ } unset($data); - return $migrationData->getTotal(); + return $migrationData->count(); } catch (WriteException $exception) { $this->handleWriteException( $exception, @@ -165,7 +166,7 @@ public function writeData(MigrationContextInterface $migrationContext, Context $ $context ); - return $migrationData->getTotal(); + return $migrationData->count(); } /** diff --git a/src/Migration/Validation/Log/MigrationValidationUnexpectedFieldLog.php b/src/Migration/Validation/Log/MigrationValidationUnexpectedFieldLog.php index d363bf846..491f5bc83 100644 --- a/src/Migration/Validation/Log/MigrationValidationUnexpectedFieldLog.php +++ b/src/Migration/Validation/Log/MigrationValidationUnexpectedFieldLog.php @@ -15,7 +15,7 @@ { public function isUserFixable(): bool { - return true; + return false; } public function getLevel(): string diff --git a/src/Migration/Validation/MigrationValidationService.php b/src/Migration/Validation/MigrationValidationService.php index e1d939c2a..d783ce48c 100644 --- a/src/Migration/Validation/MigrationValidationService.php +++ b/src/Migration/Validation/MigrationValidationService.php @@ -7,11 +7,13 @@ namespace SwagMigrationAssistant\Migration\Validation; +use Doctrine\DBAL\Connection; use Shopware\Core\Framework\Context; +use Shopware\Core\Framework\DataAbstractionLayer\CompiledFieldCollection; use Shopware\Core\Framework\DataAbstractionLayer\DefinitionInstanceRegistry; -use Shopware\Core\Framework\DataAbstractionLayer\Field\Field; use Shopware\Core\Framework\DataAbstractionLayer\Field\FkField; use Shopware\Core\Framework\DataAbstractionLayer\Field\Flag\Required; +use Shopware\Core\Framework\DataAbstractionLayer\Field\StorageAware; use Shopware\Core\Framework\DataAbstractionLayer\Write\Command\WriteCommandQueue; use Shopware\Core\Framework\DataAbstractionLayer\Write\DataStack\KeyValuePair; use Shopware\Core\Framework\DataAbstractionLayer\Write\EntityExistence; @@ -32,21 +34,33 @@ use SwagMigrationAssistant\Migration\Validation\Log\MigrationValidationMissingRequiredFieldLog; use SwagMigrationAssistant\Migration\Validation\Log\MigrationValidationUnexpectedFieldLog; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; +use Symfony\Contracts\Service\ResetInterface; /** * @internal */ #[Package('fundamentals@after-sales')] -readonly class MigrationValidationService +class MigrationValidationService implements ResetInterface { + /** + * @var array> + */ + private array $requiredColumnsCache = []; + public function __construct( - private DefinitionInstanceRegistry $definitionRegistry, - private EventDispatcherInterface $eventDispatcher, - private LoggingServiceInterface $loggingService, - private MappingServiceInterface $mappingService, + private readonly DefinitionInstanceRegistry $definitionRegistry, + private readonly EventDispatcherInterface $eventDispatcher, + private readonly LoggingServiceInterface $loggingService, + private readonly MappingServiceInterface $mappingService, + private readonly Connection $connection, ) { } + public function reset(): void + { + $this->requiredColumnsCache = []; + } + /** * @param array|null $convertedEntity * @param array $sourceData @@ -83,7 +97,7 @@ public function validate( } catch (\Throwable $exception) { $validationContext->getValidationResult()->addLog( MigrationLogBuilder::fromMigrationContext($validationContext->getMigrationContext()) - ->withEntityName($validationContext->getEntityDefinition()->getEntityName()) + ->withEntityName($entityDefinition->getEntityName()) ->withSourceData($validationContext->getSourceData()) ->withConvertedData($validationContext->getConvertedData()) ->withExceptionMessage($exception->getMessage()) @@ -106,25 +120,39 @@ public function validate( return $validationContext->getValidationResult(); } + /** + * Validates that all required fields are present and that no unexpected fields exist. + * Required fields are determined by checking which database columns are non-nullable without a default value + */ private function validateEntityStructure(MigrationValidationContext $validationContext): void { - $fields = $validationContext->getEntityDefinition()->getFields(); + $entityDefinition = $validationContext->getEntityDefinition(); - $requiredFields = array_values(array_map( - static fn (Field $field) => $field->getPropertyName(), - $fields->filterByFlag(Required::class)->getElements() - )); + $fields = $entityDefinition->getFields(); + $entityName = $entityDefinition->getEntityName(); - $convertedFieldNames = array_keys($validationContext->getConvertedData()); - $missingRequiredFields = array_diff($requiredFields, $convertedFieldNames); + $convertedData = $validationContext->getConvertedData(); + $validationResult = $validationContext->getValidationResult(); + + $requiredDatabaseColumns = $this->getRequiredDatabaseColumns($entityName); + $requiredFields = $this->filterRequiredFields( + $fields, + $requiredDatabaseColumns + ); + + $convertedFieldNames = array_keys($convertedData); + $missingRequiredFields = array_diff( + $requiredFields, + $convertedFieldNames + ); foreach ($missingRequiredFields as $missingField) { - $validationContext->getValidationResult()->addLog( + $validationResult->addLog( MigrationLogBuilder::fromMigrationContext($validationContext->getMigrationContext()) - ->withEntityName($validationContext->getEntityDefinition()->getEntityName()) + ->withEntityName($entityName) ->withFieldName($missingField) - ->withConvertedData($validationContext->getConvertedData()) - ->withEntityId($validationContext->getConvertedData()['id'] ?? null) + ->withConvertedData($convertedData) + ->withEntityId($convertedData['id'] ?? null) ->build(MigrationValidationMissingRequiredFieldLog::class) ); } @@ -132,47 +160,61 @@ private function validateEntityStructure(MigrationValidationContext $validationC $unexpectedFields = array_diff($convertedFieldNames, array_keys($fields->getElements())); foreach ($unexpectedFields as $unexpectedField) { - $validationContext->getValidationResult()->addLog( + $validationResult->addLog( MigrationLogBuilder::fromMigrationContext($validationContext->getMigrationContext()) - ->withEntityName($validationContext->getEntityDefinition()->getEntityName()) + ->withEntityName($entityName) ->withFieldName($unexpectedField) - ->withConvertedData($validationContext->getConvertedData()) - ->withEntityId($validationContext->getConvertedData()['id'] ?? null) + ->withConvertedData($convertedData) + ->withEntityId($convertedData['id'] ?? null) ->build(MigrationValidationUnexpectedFieldLog::class) ); } } + /** + * Validates that all field values conform to their field definitions by attempting to serialize them. + */ private function validateFields(MigrationValidationContext $validationContext): void { - $fields = $validationContext->getEntityDefinition()->getFields(); + $entityDefinition = $validationContext->getEntityDefinition(); + $fields = $entityDefinition->getFields(); + + $convertedData = $validationContext->getConvertedData(); + $validationResult = $validationContext->getValidationResult(); - if (!isset($validationContext->getConvertedData()['id'])) { + $id = $convertedData['id'] ?? null; + + if ($id === null) { throw MigrationException::unexpectedNullValue('id'); } - if (!Uuid::isValid($validationContext->getConvertedData()['id'])) { - throw MigrationException::invalidId($validationContext->getConvertedData()['id'], $validationContext->getEntityDefinition()->getEntityName()); + if (!Uuid::isValid($id)) { + throw MigrationException::invalidId($id, $entityDefinition->getEntityName()); } $entityExistence = EntityExistence::createForEntity( - $validationContext->getEntityDefinition()->getEntityName(), - ['id' => $validationContext->getConvertedData()['id']], + $entityDefinition->getEntityName(), + ['id' => $id], ); $parameters = new WriteParameterBag( - $validationContext->getEntityDefinition(), + $entityDefinition, WriteContext::createFromContext($validationContext->getContext()), '', new WriteCommandQueue(), ); - foreach ($validationContext->getConvertedData() as $fieldName => $value) { + foreach ($convertedData as $fieldName => $value) { if (!$fields->has($fieldName)) { continue; } $field = clone $fields->get($fieldName); + + /** + * The required flag controls flow in AbstractFieldSerializer::requiresValidation(). + * Without it, the serializer will skip validation for the field. + */ $field->setFlags(new Required()); $keyValue = new KeyValuePair( @@ -185,47 +227,41 @@ private function validateFields(MigrationValidationContext $validationContext): $serializer = $field->getSerializer(); \iterator_to_array($serializer->encode($field, $entityExistence, $keyValue, $parameters), false); } catch (\Throwable $e) { - $validationContext->getValidationResult()->addLog( + $validationResult->addLog( MigrationLogBuilder::fromMigrationContext($validationContext->getMigrationContext()) - ->withEntityName($validationContext->getEntityDefinition()->getEntityName()) + ->withEntityName($entityDefinition->getEntityName()) ->withFieldName($fieldName) ->withConvertedData([$fieldName => $value]) ->withSourceData($validationContext->getSourceData()) ->withExceptionMessage($e->getMessage()) ->withExceptionTrace($e->getTrace()) - ->withEntityId($validationContext->getConvertedData()['id'] ?? null) + ->withEntityId($id) ->build(MigrationValidationInvalidFieldValueLog::class) ); } } } + /** + * Validates that all foreign key fields reference existing entities by checking the mapping service. + */ private function validateAssociations(MigrationValidationContext $validationContext): void { - $fields = $validationContext->getEntityDefinition()->getFields(); + $entityDefinition = $validationContext->getEntityDefinition(); + $fkFields = $entityDefinition->getFields()->filterInstance(FkField::class); - $fkFields = array_values(array_map( - static fn (Field $field) => $field->getPropertyName(), - $fields->filterInstance(FkField::class)->getElements() - )); + $convertedData = $validationContext->getConvertedData(); + $validationResult = $validationContext->getValidationResult(); - foreach ($fkFields as $fkFieldName) { - if (!isset($validationContext->getConvertedData()[$fkFieldName])) { - continue; - } + /** @var FkField $fkField */ + foreach ($fkFields as $fkField) { + $fkFieldName = $fkField->getPropertyName(); + $fkValue = $convertedData[$fkFieldName] ?? null; - $fkValue = $validationContext->getConvertedData()[$fkFieldName]; - - if ($fkValue === '') { + if ($fkValue === null || $fkValue === '') { continue; } - $fkField = $fields->get($fkFieldName); - - if (!$fkField instanceof FkField) { - throw MigrationException::unexpectedNullValue($fkFieldName); - } - $referenceEntity = $fkField->getReferenceEntity(); if (!$referenceEntity) { @@ -240,16 +276,69 @@ private function validateAssociations(MigrationValidationContext $validationCont ); if (!$hasMapping) { - $validationContext->getValidationResult()->addLog( + $validationResult->addLog( MigrationLogBuilder::fromMigrationContext($validationContext->getMigrationContext()) - ->withEntityName($validationContext->getEntityDefinition()->getEntityName()) + ->withEntityName($entityDefinition->getEntityName()) ->withFieldName($fkFieldName) ->withConvertedData([$fkFieldName => $fkValue]) ->withSourceData($validationContext->getSourceData()) - ->withEntityId($validationContext->getConvertedData()['id'] ?? null) + ->withEntityId($convertedData['id'] ?? null) ->build(MigrationValidationInvalidForeignKeyLog::class) ); } } } + + /** + * @param array $requiredDbColumns + * + * @return array + */ + private function filterRequiredFields(CompiledFieldCollection $fields, array $requiredDbColumns): array + { + $requiredFields = []; + + foreach ($fields->filterByFlag(Required::class) as $field) { + if (!($field instanceof StorageAware)) { + $requiredFields[] = $field->getPropertyName(); + + continue; + } + + if (!\in_array($field->getStorageName(), $requiredDbColumns, true)) { + continue; + } + + $requiredFields[] = $field->getPropertyName(); + } + + return $requiredFields; + } + + /** + * Gets the list of required database columns for the given entity and caches the result for future calls. + * A required database column is defined as a column that is non-nullable, has no default value, and is not auto-incrementing. + * + * @return list + */ + private function getRequiredDatabaseColumns(string $entityName): array + { + if (isset($this->requiredColumnsCache[$entityName])) { + return $this->requiredColumnsCache[$entityName]; + } + + $this->requiredColumnsCache[$entityName] = []; + + $columns = $this->connection + ->createSchemaManager() + ->listTableColumns($entityName); + + foreach ($columns as $column) { + if ($column->getNotnull() && $column->getDefault() === null && !$column->getAutoincrement()) { + $this->requiredColumnsCache[$entityName][] = $column->getName(); + } + } + + return $this->requiredColumnsCache[$entityName]; + } } diff --git a/tests/MigrationServicesTrait.php b/tests/MigrationServicesTrait.php index 9135d0557..ab565ba14 100644 --- a/tests/MigrationServicesTrait.php +++ b/tests/MigrationServicesTrait.php @@ -7,6 +7,7 @@ namespace SwagMigrationAssistant\Test; +use Doctrine\DBAL\Connection; use Psr\Log\NullLogger; use Shopware\Core\Checkout\Cart\Tax\TaxCalculator; use Shopware\Core\Checkout\Order\Aggregate\OrderDelivery\OrderDeliveryStates; @@ -212,6 +213,7 @@ protected function getMigrationDataConverter( $this->getContainer()->get('event_dispatcher'), $loggingService, $mappingService, + $this->getContainer()->get(Connection::class), ); return new MigrationDataConverter( @@ -221,7 +223,7 @@ protected function getMigrationDataConverter( $loggingService, $dataDefinition, new DummyMappingService(), - $validationService + $validationService, ); } diff --git a/tests/integration/Migration/Validation/MigrationValidationServiceTest.php b/tests/integration/Migration/Validation/MigrationValidationServiceTest.php index fa4748df4..da00c5600 100644 --- a/tests/integration/Migration/Validation/MigrationValidationServiceTest.php +++ b/tests/integration/Migration/Validation/MigrationValidationServiceTest.php @@ -17,6 +17,7 @@ use Shopware\Core\Framework\Log\Package; use Shopware\Core\Framework\Test\TestCaseBase\IntegrationTestBehaviour; use Shopware\Core\Framework\Uuid\Uuid; +use SwagMigrationAssistant\Exception\MigrationException; use SwagMigrationAssistant\Migration\Connection\SwagMigrationConnectionEntity; use SwagMigrationAssistant\Migration\Logging\SwagMigrationLoggingCollection; use SwagMigrationAssistant\Migration\Logging\SwagMigrationLoggingDefinition; @@ -47,6 +48,8 @@ class MigrationValidationServiceTest extends TestCase private const CONNECTION_ID = '01991554142d73348ea58793d98f1989'; + private MigrationContext $migrationContext; + private MigrationValidationService $validationService; /** @@ -76,7 +79,21 @@ protected function setUp(): void $this->mappingRepo = static::getContainer()->get(SwagMigrationMappingDefinition::ENTITY_NAME . '.repository'); $this->context = Context::createDefaultContext(); + $connection = new SwagMigrationConnectionEntity(); + $connection->setId(self::CONNECTION_ID); + $connection->setProfileName(Shopware54Profile::PROFILE_NAME); + $connection->setGatewayName(DummyLocalGateway::GATEWAY_NAME); + $this->runId = Uuid::randomHex(); + + $this->migrationContext = new MigrationContext( + $connection, + new Shopware54Profile(), + new DummyLocalGateway(), + null, + $this->runId, + ); + static::getContainer()->get('swag_migration_connection.repository')->create( [ [ @@ -134,21 +151,8 @@ public function testShouldEarlyReturnNullWhenConvertedDataIsEmpty(): void #[DataProvider('entityStructureAndFieldProvider')] public function testShouldValidateStructureAndFieldsValues(array $convertedData, array $expectedLogs): void { - $connection = new SwagMigrationConnectionEntity(); - $connection->setId(self::CONNECTION_ID); - $connection->setProfileName(Shopware54Profile::PROFILE_NAME); - $connection->setGatewayName(DummyLocalGateway::GATEWAY_NAME); - - $migrationContext = new MigrationContext( - $connection, - new Shopware54Profile(), - new DummyLocalGateway(), - null, - $this->runId, - ); - $result = $this->validationService->validate( - $migrationContext, + $this->migrationContext, $this->context, $convertedData, SwagMigrationLoggingDefinition::ENTITY_NAME, @@ -170,6 +174,96 @@ public function testShouldValidateStructureAndFieldsValues(array $convertedData, static::assertSame($expectedLogs, $logCodes); } + public function testShouldFilterNullableFields(): void + { + $result = $this->validationService->validate( + $this->migrationContext, + $this->context, + [ + 'id' => Uuid::randomHex(), + ], + ProductDefinition::ENTITY_NAME, + [] + ); + + static::assertInstanceOf(MigrationValidationResult::class, $result); + + $missingFields = \array_map(fn ($log) => $log->getFieldName(), $result->getLogs()); + static::assertCount(3, $missingFields); + + $expectedMissingFields = [ + 'active', // has no required flag + 'price', // is nullable, but has required flag + 'cmsPageVersionId', // has default value, but has required flag + ]; + + static::assertCount( + 0, + \array_intersect($expectedMissingFields, $missingFields) + ); + } + + public function testShouldLogWhenEntityHasNowId(): void + { + $result = $this->validationService->validate( + $this->migrationContext, + $this->context, + [ + 'level' => 'error', + 'code' => 'some_code', + 'userFixable' => true, + 'createdAt' => (new \DateTime())->format(\DATE_ATOM), + ], + SwagMigrationLoggingDefinition::ENTITY_NAME, + [] + ); + + static::assertInstanceOf(MigrationValidationResult::class, $result); + + $logs = \array_filter($result->getLogs(), fn ($log) => $log instanceof MigrationValidationExceptionLog); + static::assertCount(1, $logs); + + $exceptionLog = array_values($logs)[0]; + static::assertInstanceOf(MigrationValidationExceptionLog::class, $exceptionLog); + + static::assertSame( + MigrationException::unexpectedNullValue('id')->getMessage(), + $exceptionLog->getExceptionMessage() + ); + } + + public function testShouldLogWhenEntityHasInvalidId(): void + { + $id = 'invalid-uuid'; + + $result = $this->validationService->validate( + $this->migrationContext, + $this->context, + [ + 'id' => $id, + 'level' => 'error', + 'code' => 'some_code', + 'userFixable' => true, + 'createdAt' => (new \DateTime())->format(\DATE_ATOM), + ], + SwagMigrationLoggingDefinition::ENTITY_NAME, + [] + ); + + static::assertInstanceOf(MigrationValidationResult::class, $result); + + $logs = \array_filter($result->getLogs(), fn ($log) => $log instanceof MigrationValidationExceptionLog); + static::assertCount(1, $logs); + + $exceptionLog = array_values($logs)[0]; + static::assertInstanceOf(MigrationValidationExceptionLog::class, $exceptionLog); + + static::assertSame( + MigrationException::invalidId($id, SwagMigrationLoggingDefinition::ENTITY_NAME)->getMessage(), + $exceptionLog->getExceptionMessage(), + ); + } + /** * @param array $convertedData * @param array> $mappings @@ -178,25 +272,12 @@ public function testShouldValidateStructureAndFieldsValues(array $convertedData, #[DataProvider('associationProvider')] public function testValidateAssociations(array $convertedData, array $mappings, array $expectedLogs): void { - $connection = new SwagMigrationConnectionEntity(); - $connection->setId(self::CONNECTION_ID); - $connection->setProfileName(Shopware54Profile::PROFILE_NAME); - $connection->setGatewayName(DummyLocalGateway::GATEWAY_NAME); - - $migrationContext = new MigrationContext( - $connection, - new Shopware54Profile(), - new DummyLocalGateway(), - null, - $this->runId, - ); - if (!empty($mappings)) { $this->mappingRepo->create($mappings, $this->context); } $result = $this->validationService->validate( - $migrationContext, + $this->migrationContext, $this->context, $convertedData, SwagMigrationLoggingDefinition::ENTITY_NAME,