Skip to content

Commit

Permalink
Merge pull request #4807 from markusguenther/bugfix/4804
Browse files Browse the repository at this point in the history 
BUGFIX: Skip node data processing for invalid properties
  • Loading branch information
@ahaeslich
ahaeslich authored Dec 19, 2023
2 parents 4326f8d + 77bcc07 commit 0861612
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 28 deletions.
7 changes: 6 additions & 1 deletion Neos.ContentRepository.LegacyNodeMigration/Classes/NodeDataToAssetsProcessor.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ public function run(): ProcessorResult
continue;
}
foreach ($properties as $propertyName => $propertyValue) {
$propertyType = $nodeType->getPropertyType($propertyName);
try {
$propertyType = $nodeType->getPropertyType($propertyName);
} catch (\InvalidArgumentException $exception) {
$this->dispatch(Severity::WARNING, 'Skipped node data processing for the property "%s". The property name is not part of the NodeType schema for the NodeType "%s". (Node: %s)', $propertyName, $nodeType->name->value, $nodeDataRow['identifier']);
continue;
}
foreach ($this->extractAssetIdentifiers($propertyType, $propertyValue) as $assetId) {
if (array_key_exists($assetId, $this->processedAssetIds)) {
continue;
Expand Down
7 changes: 6 additions & 1 deletion Neos.ContentRepository.LegacyNodeMigration/Classes/NodeDataToEventsProcessor.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,12 @@ public function extractPropertyValuesAndReferences(array $nodeDataRow, NodeType
}

foreach ($decodedProperties as $propertyName => $propertyValue) {
$type = $nodeType->getPropertyType($propertyName);
try {
$type = $nodeType->getPropertyType($propertyName);
} catch (\InvalidArgumentException $exception) {
$this->dispatch(Severity::WARNING, 'Skipped node data processing for the property "%s". The property name is not part of the NodeType schema for the NodeType "%s". (Node: %s)', $propertyName, $nodeType->name->value, $nodeDataRow['identifier']);
continue;
}

if ($type === 'reference' || $type === 'references') {
if (!empty($propertyValue)) {
Expand Down
20 changes: 19 additions & 1 deletion Neos.ContentRepository.LegacyNodeMigration/Tests/Behavior/Bootstrap/FeatureContext.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ class FeatureContext implements Context
*/
private array $loggedErrors = [];

/**
* @var array<string>
*/
private array $loggedWarnings = [];

private ContentRepository $contentRepository;

protected ContentRepositoryRegistry $contentRepositoryRegistry;
Expand Down Expand Up @@ -141,6 +146,8 @@ public function iRunTheEventMigration(string $contentStream = null): void
$migration->onMessage(function (Severity $severity, string $message) {
if ($severity === Severity::ERROR) {
$this->loggedErrors[] = $message;
} elseif ($severity === Severity::WARNING) {
$this->loggedWarnings[] = $message;
}
});
$this->lastMigrationResult = $migration->run();
Expand Down Expand Up @@ -186,12 +193,21 @@ public function iExpectTheFollowingEventsToBeExported(TableNode $table): void
/**
* @Then I expect the following errors to be logged
*/
public function iExpectTheFollwingErrorsToBeLogged(TableNode $table): void
public function iExpectTheFollowingErrorsToBeLogged(TableNode $table): void
{
Assert::assertSame($table->getColumn(0), $this->loggedErrors, 'Expected logged errors do not match');
$this->loggedErrors = [];
}

/**
* @Then I expect the following warnings to be logged
*/
public function iExpectTheFollowingWarningsToBeLogged(TableNode $table): void
{
Assert::assertSame($table->getColumn(0), $this->loggedWarnings, 'Expected logged warnings do not match');
$this->loggedWarnings = [];
}

/**
* @Then I expect a MigrationError
* @Then I expect a MigrationError with the message
Expand Down Expand Up @@ -307,6 +323,8 @@ public function findAssetById(string $assetId): SerializedAsset|SerializedImageV
$migration->onMessage(function (Severity $severity, string $message) {
if ($severity === Severity::ERROR) {
$this->loggedErrors[] = $message;
} elseif ($severity === Severity::WARNING) {
$this->loggedWarnings[] = $message;
}
});
$this->lastMigrationResult = $migration->run();
Expand Down
34 changes: 22 additions & 12 deletions Neos.ContentRepository.LegacyNodeMigration/Tests/Behavior/Features/Assets.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ Feature: Export of used Assets, Image Variants and Persistent Resources

Scenario: Exporting an Image Variant includes the original Image asset as well
When I have the following node data rows:
| Identifier | Path | Node Type | Properties |
| sites-node-id | /sites | unstructured | |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"string": "asset:\/\/variant1"} |
| Identifier | Path | Node Type | Properties |
| sites-node-id | /sites | unstructured | |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"string": "asset:\/\/variant1"} |
And I run the asset migration
Then I expect the following Assets to be exported:
"""
Expand Down Expand Up @@ -87,12 +87,12 @@ Feature: Export of used Assets, Image Variants and Persistent Resources

Scenario: Assets and image variants are only exported once each
When I have the following node data rows:
| Identifier | Path | Node Type | Dimension Values | Properties |
| sites-node-id | /sites | unstructured | | |
| site-node-id | /sites/test-site | Some.Package:Homepage | | {"string": "asset:\/\/asset1"} |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"language": ["ch"]} | {"image": {"__flow_object_type": "Neos\\Media\\Domain\\Model\\Image", "__identifier": "asset2"}} |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"language": ["en"]} | {"assets": [{"__flow_object_type": "Neos\\Media\\Domain\\Model\\Document", "__identifier": "asset3"}, {"__flow_object_type": "Neos\\Media\\Domain\\Model\\Image", "__identifier": "asset2"}, {"__flow_object_type": "Neos\\Media\\Domain\\Model\\ImageVariant", "__identifier": "variant1"}]} |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"language": ["de"]} | {"string": "some text with an <a href=\"asset:\/\/asset4\">asset link</a>"} |
| Identifier | Path | Node Type | Dimension Values | Properties |
| sites-node-id | /sites | unstructured | | |
| site-node-id | /sites/test-site | Some.Package:Homepage | | {"string": "asset:\/\/asset1"} |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"language": ["ch"]} | {"image": {"__flow_object_type": "Neos\\Media\\Domain\\Model\\Image", "__identifier": "asset2"}} |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"language": ["en"]} | {"assets": [{"__flow_object_type": "Neos\\Media\\Domain\\Model\\Document", "__identifier": "asset3"}, {"__flow_object_type": "Neos\\Media\\Domain\\Model\\Image", "__identifier": "asset2"}, {"__flow_object_type": "Neos\\Media\\Domain\\Model\\ImageVariant", "__identifier": "variant1"}]} |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"language": ["de"]} | {"string": "some text with an <a href=\"asset:\/\/asset4\">asset link</a>"} |
And I run the asset migration
Then I expect the following Assets to be exported:
"""
Expand Down Expand Up @@ -178,12 +178,22 @@ Feature: Export of used Assets, Image Variants and Persistent Resources

Scenario: Referring to non-existing asset
When I have the following node data rows:
| Identifier | Path | Node Type | Properties |
| sites-node-id | /sites | unstructured | |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"string": "asset:\/\/non-existing-asset"} |
| Identifier | Path | Node Type | Properties |
| sites-node-id | /sites | unstructured | |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"string": "asset:\/\/non-existing-asset"} |
And I run the asset migration
Then I expect no Assets to be exported
And I expect no ImageVariants to be exported
And I expect no PersistentResources to be exported
And I expect the following errors to be logged
| Failed to extract assets of property "string" of node "site-node-id" (type: "Some.Package:Homepage"): Failed to find mock asset with id "non-existing-asset" |

Scenario: Nodes with properties that are not part of the node type schema (see https://github.com/neos/neos-development-collection/issues/4804)
When I have the following node data rows:
| Identifier | Path | Node Type | Properties |
| sites-node-id | /sites | unstructured | |
| site-node-id | /sites/test-site | Some.Package:Homepage | {"unknownProperty": "asset:\/\/variant1"} |
And I run the asset migration
Then I expect no Assets to be exported
And I expect the following warnings to be logged
| Skipped node data processing for the property "unknownProperty". The property name is not part of the NodeType schema for the NodeType "Some.Package:Homepage". (Node: site-node-id) |
Loading

0 comments on commit 0861612

Please sign in to comment.