From 7250af065e11fb2724d664f9790abfc0a7454376 Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 27 Jul 2026 13:49:15 +0200 Subject: [PATCH 01/11] feat(output-mapping): AJDA-2946 store description only when table flag is system-managed Table and column descriptions are stored in the native Storage description field through the table-definition update endpoint, driven by the table-level isDescriptionSystemManaged flag added in DMD-1662: - table created by the run: the description is always stored (a fresh table is system-managed by the Storage default), once the load finished and the table surely exists - this also covers tables created by the load job itself - existing table: the description is stored only when the flag is set, so a description managed by the user is never overwritten The existing KBC.description metadata written under the component provider is kept unchanged - it stays as provenance and as a read fallback. --- libs/output-mapping/README.md | 1 + libs/output-mapping/composer.json | 2 +- libs/output-mapping/phpunit.xml.dist | 1 + .../src/DeferredTasks/LoadTableQueue.php | 66 +++++- .../MappingFromConfigurationSchemaColumn.php | 35 ++- .../MappingFromProcessedConfiguration.php | 82 ++++++- .../src/Storage/StoragePreparer.php | 10 + .../src/Storage/TableDescription.php | 55 +++++ .../src/Storage/TableDescriptionModifier.php | 117 ++++++++++ libs/output-mapping/src/Storage/TableInfo.php | 12 + libs/output-mapping/src/TableLoader.php | 21 +- .../DeferredTasks/LoadTableQueueTest.php | 141 ++++++++++++ ...ppingFromConfigurationSchemaColumnTest.php | 24 ++ .../MappingFromProcessedConfigurationTest.php | 130 +++++++++++ .../Storage/TableDescriptionModifierTest.php | 210 ++++++++++++++++++ .../tests/Storage/TableDescriptionTest.php | 62 ++++++ .../tests/Storage/TableInfoTest.php | 14 ++ .../tests/Writer/TableDescriptionTest.php | 117 ++++++++++ 18 files changed, 1090 insertions(+), 10 deletions(-) create mode 100644 libs/output-mapping/src/Storage/TableDescription.php create mode 100644 libs/output-mapping/src/Storage/TableDescriptionModifier.php create mode 100644 libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php create mode 100644 libs/output-mapping/tests/Storage/TableDescriptionTest.php create mode 100644 libs/output-mapping/tests/Writer/TableDescriptionTest.php diff --git a/libs/output-mapping/README.md b/libs/output-mapping/README.md index 434ec69cd..76a62812a 100644 --- a/libs/output-mapping/README.md +++ b/libs/output-mapping/README.md @@ -8,6 +8,7 @@ Output mapping library for Keboola Runner and Workspaces. Processes component ou - Requires: `output-mapping-slice` feature flag, default CSV format (`,` delimiter, `"` enclosure), no custom `columns` mapping - Sliced files must have `columns` or `schema` specified in configuration - Workspace staging (Snowflake/BigQuery): Loads tables directly from workspace database objects (no file upload, no slicing) +- Descriptions: Table and column descriptions (`description`, `schema[].description` or `KBC.description` metadata) are stored in the native Storage description field. A table created by the run always gets its description; on an existing table the description is only stored when the table's `isDescriptionSystemManaged` flag is set, so a description managed by the user is never overwritten. **Files:** - Uploads files as-is to Storage API File Storage (works with all staging types) diff --git a/libs/output-mapping/composer.json b/libs/output-mapping/composer.json index 1b4148dd6..a5f7424b8 100644 --- a/libs/output-mapping/composer.json +++ b/libs/output-mapping/composer.json @@ -35,7 +35,7 @@ "keboola/php-storage-names-sanitizer": "*@dev", "keboola/slicer": "*@dev", "keboola/staging-provider": "*@dev", - "keboola/storage-api-client": "^18.5", + "keboola/storage-api-client": "^18.9", "keboola/storage-api-php-client-branch-wrapper": "^7.0", "microsoft/azure-storage-blob": "^1.5", "psr/log": "^2.0|^3.0", diff --git a/libs/output-mapping/phpunit.xml.dist b/libs/output-mapping/phpunit.xml.dist index 9efa188eb..717bd8d71 100644 --- a/libs/output-mapping/phpunit.xml.dist +++ b/libs/output-mapping/phpunit.xml.dist @@ -39,6 +39,7 @@ tests/Writer/StorageApiHeadlessWriterTest.php tests/Writer/StorageApiLocalTableWriterTest.php tests/Writer/StorageApiSlicedWriterTest.php + tests/Writer/TableDescriptionTest.php tests/Writer/Workspace diff --git a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php index 705ecfd3a..a2e3ce02a 100644 --- a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php +++ b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php @@ -6,6 +6,8 @@ use Keboola\InputMapping\Table\Result\TableInfo; use Keboola\OutputMapping\Exception\InvalidOutputException; +use Keboola\OutputMapping\Storage\TableDescription; +use Keboola\OutputMapping\Storage\TableDescriptionModifier; use Keboola\OutputMapping\Table\Result; use Keboola\OutputMapping\Table\Result\Metrics; use Keboola\StorageApi\ClientException; @@ -20,16 +22,26 @@ class LoadTableQueue /** @var LoadTableTaskInterface[] */ private array $loadTableTasks; + + /** @var array table id => descriptions of a table created by this run */ + private array $createdTableDescriptions; + private Result $tableResult; /** * @param LoadTableTaskInterface[] $loadTableTasks + * @param array $createdTableDescriptions */ - public function __construct(ClientWrapper $clientWrapper, LoggerInterface $logger, array $loadTableTasks) - { + public function __construct( + ClientWrapper $clientWrapper, + LoggerInterface $logger, + array $loadTableTasks, + array $createdTableDescriptions = [], + ) { $this->clientWrapper = $clientWrapper; $this->logger = $logger; $this->loadTableTasks = $loadTableTasks; + $this->createdTableDescriptions = $createdTableDescriptions; $this->tableResult = new Result(); } @@ -93,11 +105,13 @@ public function waitForAll(): array ); } + $tableColumns = null; switch ($jobResult['operationName']) { case 'tableImport': $tableData = $this->clientWrapper->getTableAndFileStorageClient()->getTable( $jobResult['tableId'], ); + $tableColumns = self::extractTableColumns($tableData); $this->tableResult->addTable(new TableInfo($tableData)); $this->tableResult->addGenericVariable( $jobResult['tableId'], @@ -107,14 +121,20 @@ public function waitForAll(): array $jobResults[] = $jobResult; break; case 'tableCreate': - $this->tableResult->addTable( - new TableInfo($this->clientWrapper->getTableAndFileStorageClient()->getTable( - $jobResult['results']['id'], - )), + $tableData = $this->clientWrapper->getTableAndFileStorageClient()->getTable( + $jobResult['results']['id'], ); + $tableColumns = self::extractTableColumns($tableData); + $this->tableResult->addTable(new TableInfo($tableData)); $jobResults[] = $jobResult; break; } + + try { + $this->applyCreatedTableDescriptions($task, $tableColumns); + } catch (InvalidOutputException $e) { + $errors[] = $e->getMessage(); + } } } @@ -126,6 +146,40 @@ public function waitForAll(): array return $jobIds; } + /** + * @param array $tableData table detail as returned by Storage + * @return string[]|null + */ + private static function extractTableColumns(array $tableData): ?array + { + $columns = $tableData['columns'] ?? null; + + return is_array($columns) ? array_map('strval', $columns) : null; + } + + /** + * @param string[]|null $tableColumns + */ + private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?array $tableColumns): void + { + if ($this->createdTableDescriptions === []) { + return; + } + + $tableId = $task->getDestinationTableName(); + $descriptions = $this->createdTableDescriptions[$tableId] ?? null; + if ($descriptions === null) { + return; + } + + // Several sources may be mapped to the same destination table, but the descriptions of a table only + // need to be stored once. + unset($this->createdTableDescriptions[$tableId]); + + $descriptionModifier = new TableDescriptionModifier($this->clientWrapper, $this->logger); + $descriptionModifier->setCreatedTableDescriptions($descriptions, $tableColumns); + } + public function getTaskCount(): int { return count($this->loadTableTasks); diff --git a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php index a689c22c5..1cefd5978 100644 --- a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php +++ b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php @@ -6,6 +6,8 @@ class MappingFromConfigurationSchemaColumn { + private const DESCRIPTION_METADATA_KEY = 'KBC.description'; + public function __construct(private readonly array $mapping) { } @@ -47,8 +49,39 @@ public function getMetadata(): array { $metadata = $this->mapping['metadata'] ?? []; if (isset($this->mapping['description'])) { - $metadata['KBC.description'] = $this->mapping['description']; + $metadata[self::DESCRIPTION_METADATA_KEY] = $this->mapping['description']; } return $metadata; } + + /** + * Description of the column, stored in the native Storage description field. The configuration allows only + * one of the two sources to be used at a time. An empty description is treated as no description, so that + * it never clears a description stored in Storage. + */ + public function getDescription(): ?string + { + if (isset($this->mapping['description'])) { + return self::normalizeDescription($this->mapping['description']); + } + + // metadata is a variableNode in the configuration, so it is not guaranteed to be an array + $metadata = $this->mapping['metadata'] ?? []; + if (is_array($metadata) && isset($metadata[self::DESCRIPTION_METADATA_KEY])) { + return self::normalizeDescription($metadata[self::DESCRIPTION_METADATA_KEY]); + } + + return null; + } + + private static function normalizeDescription(mixed $description): ?string + { + if (!is_scalar($description)) { + return null; + } + + $description = (string) $description; + + return $description !== '' ? $description : null; + } } diff --git a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php index bf41e2197..988d20809 100644 --- a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php +++ b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php @@ -12,6 +12,8 @@ class MappingFromProcessedConfiguration { + private const DESCRIPTION_METADATA_KEY = 'KBC.description'; + private MappingDestination $destination; private MappingFromRawConfigurationAndPhysicalDataWithManifest $source; private array $mapping; @@ -185,11 +187,89 @@ public function getTableMetadata(): array { $metadata = $this->mapping['table_metadata'] ?? []; if (isset($this->mapping['description'])) { - $metadata['KBC.description'] = $this->mapping['description']; + $metadata[self::DESCRIPTION_METADATA_KEY] = $this->mapping['description']; } return $metadata; } + /** + * Description of the table, stored in the native Storage description field. Resolved from the dedicated + * `description` field first, then from the `KBC.description` key of either metadata structure. The + * configuration forbids combining `description` with `table_metadata.KBC.description`. + * + * An empty description is treated as no description, so that it never clears a description stored in + * Storage. + */ + public function getTableDescription(): ?string + { + if (isset($this->mapping['description'])) { + return self::normalizeDescription($this->mapping['description']); + } + + // table_metadata is a variableNode in the configuration, so it is not guaranteed to be an array + $tableMetadata = $this->mapping['table_metadata'] ?? []; + if (is_array($tableMetadata) && isset($tableMetadata[self::DESCRIPTION_METADATA_KEY])) { + return self::normalizeDescription($tableMetadata[self::DESCRIPTION_METADATA_KEY]); + } + + foreach ($this->getMetadata() as $item) { + if (($item['key'] ?? null) === self::DESCRIPTION_METADATA_KEY) { + return self::normalizeDescription($item['value']); + } + } + + return null; + } + + private static function normalizeDescription(mixed $description): ?string + { + if (!is_scalar($description)) { + return null; + } + + $description = (string) $description; + + return $description !== '' ? $description : null; + } + + /** + * Descriptions of the columns, stored in the native Storage description field. Sourced either from the + * schema (which cannot be combined with column_metadata) or from the `KBC.description` key of the column + * metadata. + * + * @return array column name => description + */ + public function getColumnDescriptions(): array + { + $descriptions = []; + + $schema = $this->getSchema(); + if ($schema !== null) { + foreach ($schema as $column) { + $description = $column->getDescription(); + if ($description !== null) { + $descriptions[$column->getName()] = $description; + } + } + + return $descriptions; + } + + foreach ($this->getColumnMetadata() as $columnMetadata) { + foreach ($columnMetadata->getMetadata() as $item) { + if (($item['key'] ?? null) !== self::DESCRIPTION_METADATA_KEY) { + continue; + } + $description = self::normalizeDescription($item['value']); + if ($description !== null) { + $descriptions[$columnMetadata->getColumnName()] = $description; + } + } + } + + return $descriptions; + } + /** @return null|MappingFromConfigurationSchemaColumn[] */ public function getSchema(): ?array { diff --git a/libs/output-mapping/src/Storage/StoragePreparer.php b/libs/output-mapping/src/Storage/StoragePreparer.php index 0ed588c65..a33308a42 100644 --- a/libs/output-mapping/src/Storage/StoragePreparer.php +++ b/libs/output-mapping/src/Storage/StoragePreparer.php @@ -67,6 +67,16 @@ public function prepareStorageBucketAndTable( $destinationTableInfo = $this->getDestinationTableInfoIfExists( $processedSource->getDestination()->getTableId(), ); + + if ($destinationTableInfo !== null) { + // The description of a table which already exists is only updated when it is system-managed. + // Descriptions of tables created by this run are stored once the load finishes, see TableLoader. + $descriptions = TableDescription::createFromMapping($processedSource); + if (!$descriptions->isEmpty()) { + $descriptionModifier = new TableDescriptionModifier($this->clientWrapper, $this->logger); + $descriptionModifier->updateExistingTableDescriptions($destinationTableInfo, $descriptions); + } + } } return new MappingStorageSources($destinationBucket, $destinationTableInfo); diff --git a/libs/output-mapping/src/Storage/TableDescription.php b/libs/output-mapping/src/Storage/TableDescription.php new file mode 100644 index 000000000..273babaff --- /dev/null +++ b/libs/output-mapping/src/Storage/TableDescription.php @@ -0,0 +1,55 @@ + $columnDescriptions column name => description + */ + public function __construct( + private readonly string $tableId, + private readonly ?string $tableDescription, + private readonly array $columnDescriptions, + ) { + } + + public static function createFromMapping(MappingFromProcessedConfiguration $source): self + { + return new self( + $source->getDestination()->getTableId(), + $source->getTableDescription(), + $source->getColumnDescriptions(), + ); + } + + public function getTableId(): string + { + return $this->tableId; + } + + public function getTableDescription(): ?string + { + return $this->tableDescription; + } + + /** + * @return array column name => description + */ + public function getColumnDescriptions(): array + { + return $this->columnDescriptions; + } + + public function isEmpty(): bool + { + return $this->tableDescription === null && $this->columnDescriptions === []; + } +} diff --git a/libs/output-mapping/src/Storage/TableDescriptionModifier.php b/libs/output-mapping/src/Storage/TableDescriptionModifier.php new file mode 100644 index 000000000..801d9fe92 --- /dev/null +++ b/libs/output-mapping/src/Storage/TableDescriptionModifier.php @@ -0,0 +1,117 @@ +isDescriptionSystemManaged()) { + $this->logger->info(sprintf( + 'Description of table "%s" is managed by the user, keeping the current value.', + $tableInfo->getId(), + )); + return; + } + + $this->applyDescriptions($descriptions, $tableInfo->getColumns()); + } + + /** + * Stores descriptions on a table created by the current run. + * + * @param string[]|null $tableColumns columns of the created table, null when the column list is unknown + */ + public function setCreatedTableDescriptions(TableDescription $descriptions, ?array $tableColumns): void + { + $this->applyDescriptions($descriptions, $tableColumns); + } + + /** + * @param string[]|null $tableColumns columns existing in the table, null disables the check + */ + private function applyDescriptions(TableDescription $descriptions, ?array $tableColumns): void + { + $tableDefinitionUpdate = []; + + if ($descriptions->getTableDescription() !== null) { + $tableDefinitionUpdate['description'] = $descriptions->getTableDescription(); + } + + $columns = []; + $missingColumns = []; + foreach ($descriptions->getColumnDescriptions() as $columnName => $columnDescription) { + if ($tableColumns !== null && !in_array($columnName, $tableColumns, true)) { + $missingColumns[] = $columnName; + continue; + } + $columns[] = [ + 'name' => $columnName, + 'description' => $columnDescription, + ]; + } + + if ($missingColumns) { + $this->logger->warning(sprintf( + 'Cannot store description of column(s) "%s" of table "%s", the column(s) do not exist.', + implode('", "', $missingColumns), + $descriptions->getTableId(), + )); + } + + if ($columns) { + $tableDefinitionUpdate['columns'] = $columns; + } + + if (!$tableDefinitionUpdate) { + return; + } + + try { + $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition( + $descriptions->getTableId(), + $tableDefinitionUpdate, + ); + } catch (ClientException $e) { + throw new InvalidOutputException( + sprintf( + 'Cannot update description of table "%s": %s', + $descriptions->getTableId(), + $e->getMessage(), + ), + $e->getCode(), + $e, + ); + } + } +} diff --git a/libs/output-mapping/src/Storage/TableInfo.php b/libs/output-mapping/src/Storage/TableInfo.php index ef427f60b..7230e7154 100644 --- a/libs/output-mapping/src/Storage/TableInfo.php +++ b/libs/output-mapping/src/Storage/TableInfo.php @@ -25,6 +25,18 @@ public function isTyped(): bool return $this->tableInfo['isTyped']; } + /** + * Whether the table description is managed by the system (true) or by the user (false). + * + * Output mapping may store/update a system-managed description but must never overwrite a user-managed + * one (AJDA-2946). Storage defaults the flag to system-managed, which is also assumed when the flag is + * missing from the table detail (Storage version without DMD-1662). + */ + public function isDescriptionSystemManaged(): bool + { + return (bool) ($this->tableInfo['isDescriptionSystemManaged'] ?? true); + } + public function getPrimaryKey(): array { return $this->tableInfo['primaryKey']; diff --git a/libs/output-mapping/src/TableLoader.php b/libs/output-mapping/src/TableLoader.php index 098da20da..63109ca22 100644 --- a/libs/output-mapping/src/TableLoader.php +++ b/libs/output-mapping/src/TableLoader.php @@ -14,6 +14,7 @@ use Keboola\OutputMapping\Staging\StrategyFactory; use Keboola\OutputMapping\Storage\StoragePreparer; use Keboola\OutputMapping\Storage\TableChangesStore; +use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Storage\TableStructureValidatorFactory; use Keboola\OutputMapping\Writer\Helper\Path; use Keboola\OutputMapping\Writer\Table\BranchResolver; @@ -59,6 +60,8 @@ public function uploadTables( } $loadTableTasks = []; + /** @var array $createdTableDescriptions */ + $createdTableDescriptions = []; $tableConfigurationResolver = new TableConfigurationResolver($this->logger); $tableConfigurationValidator = new TableConfigurationValidator($strategy, $configuration); $tableColumnsConfigurationHintsResolver = new TableHintsConfigurationSchemaResolver(); @@ -153,6 +156,17 @@ public function uploadTables( $systemMetadata, ); + if (!$storageSources->didTableExistBefore()) { + // The table is created by this run - either by the table definition created above or by the + // load job itself. Its description is always system-managed (Storage default), so it is stored + // as soon as the load finishes and the table surely exists. Descriptions of tables which + // already existed are handled by StoragePreparer, where the system-managed flag is known. + $descriptions = TableDescription::createFromMapping($processedSource); + if (!$descriptions->isEmpty()) { + $createdTableDescriptions[$descriptions->getTableId()] = $descriptions; + } + } + $loadTableTasks[] = $loadTableTask; } @@ -168,7 +182,12 @@ public function uploadTables( $this->callWorkspaceUnload($strategy); } - $tableQueue = new LoadTableQueue($this->clientWrapper, $this->logger, $loadTableTasks); + $tableQueue = new LoadTableQueue( + $this->clientWrapper, + $this->logger, + $loadTableTasks, + $createdTableDescriptions, + ); $tableQueue->start(); $tableQueue->loadCustomVariables(Path::join( $strategy->getMetadataStorage()->getPath(), diff --git a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php index 94e1cdc30..619befa53 100644 --- a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php +++ b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php @@ -9,6 +9,7 @@ use Keboola\OutputMapping\DeferredTasks\LoadTableQueue; use Keboola\OutputMapping\DeferredTasks\TableWriter\LoadTableTask; use Keboola\OutputMapping\Exception\InvalidOutputException; +use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Table\Result; use Keboola\OutputMapping\Table\Result\Metrics; use Keboola\OutputMapping\Table\Result\TableMetrics; @@ -644,6 +645,146 @@ public function testLoadCustomVariablesSetsVariablesFromValidJson(): void unlink($tmpFile); } + public function testWaitForAllStoresDescriptionsOfCreatedTable(): void + { + $tableId = 'in.c-myBucket.tableCreated'; + + $clientMock = $this->createMock(Client::class); + $clientMock->expects(self::once()) + ->method('getTable') + ->with($tableId) + ->willReturn([ + 'id' => $tableId, + 'displayName' => 'my-name', + 'name' => 'my-name', + 'columns' => ['col1'], + 'lastImportDate' => null, + 'lastChangeDate' => null, + ]) + ; + $clientMock->expects(self::once()) + ->method('updateTableDefinition') + ->with($tableId, [ + 'description' => 'table desc', + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ], + ]) + ->willReturn([]) + ; + + $branchClientMock = $this->createMock(BranchAwareClient::class); + $branchClientMock->expects(self::once()) + ->method('waitForJob') + ->with(123) + ->willReturn([ + 'operationName' => 'tableImport', + 'status' => 'success', + 'tableId' => $tableId, + 'metrics' => [ + 'inBytes' => 0, + 'inBytesUncompressed' => 0, + ], + ]) + ; + + $loadTask = $this->createMock(LoadTableTask::class); + $loadTask->expects(self::once()) + ->method('getStorageJobId') + ->willReturn('123') + ; + $loadTask->expects(self::once()) + ->method('applyMetadata') + ; + $loadTask->expects(self::once()) + ->method('getDestinationTableName') + ->willReturn($tableId) + ; + + $clientWrapperMock = $this->createMock(ClientWrapper::class); + $clientWrapperMock->method('getTableAndFileStorageClient') + ->willReturn($clientMock); + $clientWrapperMock->method('getBranchClient') + ->willReturn($branchClientMock); + + $loadQueue = new LoadTableQueue( + $clientWrapperMock, + new NullLogger(), + [$loadTask], + [$tableId => new TableDescription($tableId, 'table desc', ['col1' => 'col1 desc'])], + ); + $loadQueue->waitForAll(); + } + + public function testWaitForAllReportsFailedDescriptionUpdateAsError(): void + { + $tableId = 'in.c-myBucket.tableCreated'; + + $clientMock = $this->createMock(Client::class); + $clientMock->expects(self::once()) + ->method('getTable') + ->with($tableId) + ->willReturn([ + 'id' => $tableId, + 'displayName' => 'my-name', + 'name' => 'my-name', + 'columns' => ['col1'], + 'lastImportDate' => null, + 'lastChangeDate' => null, + ]) + ; + $clientMock->expects(self::once()) + ->method('updateTableDefinition') + ->willThrowException(new ClientException('Backend does not support definition update', 400)) + ; + + $branchClientMock = $this->createMock(BranchAwareClient::class); + $branchClientMock->expects(self::once()) + ->method('waitForJob') + ->with(123) + ->willReturn([ + 'operationName' => 'tableImport', + 'status' => 'success', + 'tableId' => $tableId, + 'metrics' => [ + 'inBytes' => 0, + 'inBytesUncompressed' => 0, + ], + ]) + ; + + $loadTask = $this->createMock(LoadTableTask::class); + $loadTask->method('getStorageJobId')->willReturn('123'); + $loadTask->method('getDestinationTableName')->willReturn($tableId); + $loadTask->expects(self::once())->method('applyMetadata'); + + $clientWrapperMock = $this->createMock(ClientWrapper::class); + $clientWrapperMock->method('getTableAndFileStorageClient') + ->willReturn($clientMock); + $clientWrapperMock->method('getBranchClient') + ->willReturn($branchClientMock); + + $loadQueue = new LoadTableQueue( + $clientWrapperMock, + new NullLogger(), + [$loadTask], + [$tableId => new TableDescription($tableId, 'table desc', [])], + ); + + try { + $loadQueue->waitForAll(); + self::fail('WaitForAll should fail with InvalidOutputException.'); + } catch (InvalidOutputException $e) { + self::assertSame( + sprintf( + 'Cannot update description of table "%s": Backend does not support definition update', + $tableId, + ), + $e->getMessage(), + ); + } + } + public function testLoadCustomVariablesDoesNothingWhenFileMissing(): void { $clientWrapperMock = $this->createMock(ClientWrapper::class); diff --git a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php index 0bf8b415b..613339a34 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php @@ -22,6 +22,7 @@ public function testMinimalMappingConfiguration(): void self::assertFalse($schemColumn->isDistributionKey()); self::assertFalse($schemColumn->hasMetadata()); self::assertSame([], $schemColumn->getMetadata()); + self::assertNull($schemColumn->getDescription()); } public function testGetters(): void @@ -58,5 +59,28 @@ public function testGetters(): void ], $schemColumn->getMetadata(), ); + self::assertSame('Some description of the newColumn.', $schemColumn->getDescription()); + } + + public function testGetDescriptionFromMetadata(): void + { + $schemColumn = new MappingFromConfigurationSchemaColumn([ + 'name' => 'newColumn', + 'metadata' => [ + 'KBC.description' => 'Description from metadata.', + ], + ]); + + self::assertSame('Description from metadata.', $schemColumn->getDescription()); + } + + public function testGetDescriptionIgnoresNonArrayMetadata(): void + { + $schemColumn = new MappingFromConfigurationSchemaColumn([ + 'name' => 'newColumn', + 'metadata' => 'this is a variableNode, so it may be anything', + ]); + + self::assertNull($schemColumn->getDescription()); } } diff --git a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php index 6de6c3b23..1e10c4765 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php @@ -71,6 +71,136 @@ public function testBasic(): void self::assertEquals(SourceType::WORKSPACE, $mapping->getItemSourceType()); self::assertInstanceOf(MappingDestination::class, $mapping->getDestination()); self::assertNull($mapping->getDeleteWhere()); + self::assertNull($mapping->getTableDescription()); + self::assertSame([], $mapping->getColumnDescriptions()); + } + + /** @dataProvider tableDescriptionProvider */ + public function testGetTableDescription(array $mappingConfiguration, ?string $expectedDescription): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration( + array_merge(['destination' => 'in.c-main.table'], $mappingConfiguration), + $physicalDataWithManifest, + ); + + self::assertSame($expectedDescription, $mapping->getTableDescription()); + } + + public static function tableDescriptionProvider(): Generator + { + yield 'from description field' => [ + 'mappingConfiguration' => ['description' => 'table desc'], + 'expectedDescription' => 'table desc', + ]; + yield 'from table_metadata' => [ + 'mappingConfiguration' => ['table_metadata' => ['KBC.description' => 'table desc']], + 'expectedDescription' => 'table desc', + ]; + yield 'from metadata list' => [ + 'mappingConfiguration' => [ + 'metadata' => [ + ['key' => 'KBC.name', 'value' => 'whatever'], + ['key' => 'KBC.description', 'value' => 'table desc'], + ], + ], + 'expectedDescription' => 'table desc', + ]; + yield 'description field wins over metadata' => [ + 'mappingConfiguration' => [ + 'description' => 'table desc', + 'metadata' => [['key' => 'KBC.description', 'value' => 'metadata desc']], + ], + 'expectedDescription' => 'table desc', + ]; + yield 'no description' => [ + 'mappingConfiguration' => ['table_metadata' => ['key1' => 'val1']], + 'expectedDescription' => null, + ]; + yield 'table_metadata is a variableNode, so it may be anything' => [ + 'mappingConfiguration' => ['table_metadata' => 'not an array'], + 'expectedDescription' => null, + ]; + yield 'empty description is not stored' => [ + 'mappingConfiguration' => ['description' => ''], + 'expectedDescription' => null, + ]; + } + + public function testGetColumnDescriptionsSkipsEmptyDescriptions(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'column_metadata' => [ + 'col1' => [['key' => 'KBC.description', 'value' => '']], + 'col2' => [['key' => 'KBC.description', 'value' => 'col2 desc']], + ], + ], $physicalDataWithManifest); + + self::assertSame(['col2' => 'col2 desc'], $mapping->getColumnDescriptions()); + } + + public function testGetColumnDescriptionsFromSchema(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'schema' => [ + [ + 'name' => 'col1', + 'description' => 'col1 desc', + ], + [ + 'name' => 'col2', + 'metadata' => ['KBC.description' => 'col2 desc'], + ], + [ + 'name' => 'col3', + ], + ], + ], $physicalDataWithManifest); + + self::assertSame( + [ + 'col1' => 'col1 desc', + 'col2' => 'col2 desc', + ], + $mapping->getColumnDescriptions(), + ); + } + + public function testGetColumnDescriptionsFromColumnMetadata(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'column_metadata' => [ + 'col1' => [ + ['key' => 'KBC.datatype.type', 'value' => 'STRING'], + ['key' => 'KBC.description', 'value' => 'col1 desc'], + ], + 'col2' => [ + ['key' => 'KBC.datatype.type', 'value' => 'STRING'], + ], + ], + ], $physicalDataWithManifest); + + self::assertSame(['col1' => 'col1 desc'], $mapping->getColumnDescriptions()); + } + + public function testGetColumnDescriptionsSkipsRestrictedColumns(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'column_metadata' => [ + 'col1' => [['key' => 'KBC.description', 'value' => 'col1 desc']], + '_timestamp' => [['key' => 'KBC.description', 'value' => 'timestamp desc']], + ], + ], $physicalDataWithManifest); + + self::assertSame(['col1' => 'col1 desc'], $mapping->getColumnDescriptions()); } public function testTableMetadata(): void diff --git a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php new file mode 100644 index 000000000..55a290443 --- /dev/null +++ b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php @@ -0,0 +1,210 @@ +createMock(Client::class); + $client->expects($this->once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'description' => 'table desc', + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ['name' => 'col2', 'description' => 'col2 desc'], + ], + ], + ) + ->willReturn([]); + + $logHandler = new TestHandler(); + $logger = new Logger('test', [$logHandler]); + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); + $modifier->updateExistingTableDescriptions( + $this->createTableInfo(true, ['col1', 'col2']), + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc', 'col2' => 'col2 desc']), + ); + + self::assertFalse($logHandler->hasWarningRecords()); + } + + public function testUpdateExistingTableDescriptionsIsSkippedForUserManagedDescription(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->never()) + ->method('updateTableDefinition'); + + $logHandler = new TestHandler(); + $logger = new Logger('test', [$logHandler]); + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); + $modifier->updateExistingTableDescriptions( + $this->createTableInfo(false, ['col1']), + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + ); + + self::assertTrue($logHandler->hasInfoThatContains(sprintf( + 'Description of table "%s" is managed by the user, keeping the current value.', + self::TABLE_ID, + ))); + } + + public function testUpdateExistingTableDescriptionsSkipsMissingColumns(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ], + ], + ) + ->willReturn([]); + + $logHandler = new TestHandler(); + $logger = new Logger('test', [$logHandler]); + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); + $modifier->updateExistingTableDescriptions( + $this->createTableInfo(true, ['col1']), + new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc', 'col2' => 'col2 desc']), + ); + + self::assertTrue($logHandler->hasWarningThatContains(sprintf( + 'Cannot store description of column(s) "col2" of table "%s", the column(s) do not exist.', + self::TABLE_ID, + ))); + } + + public function testUpdateExistingTableDescriptionsWithNothingToStoreDoesNotCallStorage(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->never()) + ->method('updateTableDefinition'); + + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), new Logger('test')); + $modifier->updateExistingTableDescriptions( + $this->createTableInfo(true, ['col1']), + new TableDescription(self::TABLE_ID, null, []), + ); + } + + public function testSetCreatedTableDescriptions(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'description' => 'table desc', + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ], + ], + ) + ->willReturn([]); + + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), new Logger('test')); + $modifier->setCreatedTableDescriptions( + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + ['col1'], + ); + } + + public function testSetCreatedTableDescriptionsWithUnknownColumnList(): void + { + $client = $this->createMock(Client::class); + $client->expects($this->once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ], + ], + ) + ->willReturn([]); + + $logHandler = new TestHandler(); + $logger = new Logger('test', [$logHandler]); + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); + $modifier->setCreatedTableDescriptions( + new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc']), + null, + ); + + self::assertFalse($logHandler->hasWarningRecords()); + } + + public function testStorageErrorIsWrappedInInvalidOutputException(): void + { + $clientException = new ClientException('Table definition update failed', 400); + + $client = $this->createMock(Client::class); + $client->expects($this->once()) + ->method('updateTableDefinition') + ->willThrowException($clientException); + + $modifier = new TableDescriptionModifier($this->createClientWrapper($client), new Logger('test')); + + try { + $modifier->setCreatedTableDescriptions( + new TableDescription(self::TABLE_ID, 'table desc', []), + null, + ); + self::fail('Storing the description should fail with InvalidOutputException.'); + } catch (InvalidOutputException $e) { + self::assertSame( + 'Cannot update description of table "in.c-main.table": Table definition update failed', + $e->getMessage(), + ); + self::assertSame(400, $e->getCode()); + self::assertSame($clientException, $e->getPrevious()); + } + } + + private function createClientWrapper(Client&MockObject $client): ClientWrapper + { + $clientWrapper = $this->createMock(ClientWrapper::class); + $clientWrapper->method('getTableAndFileStorageClient')->willReturn($client); + + return $clientWrapper; + } + + /** + * @param string[] $columns + */ + private function createTableInfo(bool $isDescriptionSystemManaged, array $columns): TableInfo + { + return new TableInfo([ + 'id' => self::TABLE_ID, + 'columns' => $columns, + 'isTyped' => true, + 'primaryKey' => [], + 'isDescriptionSystemManaged' => $isDescriptionSystemManaged, + ]); + } +} diff --git a/libs/output-mapping/tests/Storage/TableDescriptionTest.php b/libs/output-mapping/tests/Storage/TableDescriptionTest.php new file mode 100644 index 000000000..01b49ce42 --- /dev/null +++ b/libs/output-mapping/tests/Storage/TableDescriptionTest.php @@ -0,0 +1,62 @@ +createSourceMock('table desc', ['col1' => 'col1 desc']), + ); + + self::assertSame('in.c-main.table', $descriptions->getTableId()); + self::assertSame('table desc', $descriptions->getTableDescription()); + self::assertSame(['col1' => 'col1 desc'], $descriptions->getColumnDescriptions()); + self::assertFalse($descriptions->isEmpty()); + } + + public function testIsEmptyWithNothingToStore(): void + { + $descriptions = TableDescription::createFromMapping($this->createSourceMock(null, [])); + + self::assertTrue($descriptions->isEmpty()); + } + + public function testIsEmptyWithColumnDescriptionsOnly(): void + { + $descriptions = TableDescription::createFromMapping( + $this->createSourceMock(null, ['col1' => 'col1 desc']), + ); + + self::assertFalse($descriptions->isEmpty()); + } + + /** + * @param array $columnDescriptions + */ + private function createSourceMock( + ?string $tableDescription, + array $columnDescriptions, + ): MappingFromProcessedConfiguration { + $source = $this->createMock(MappingFromProcessedConfiguration::class); + $source->expects($this->once()) + ->method('getDestination') + ->willReturn(new MappingDestination('in.c-main.table')); + $source->expects($this->once()) + ->method('getTableDescription') + ->willReturn($tableDescription); + $source->expects($this->once()) + ->method('getColumnDescriptions') + ->willReturn($columnDescriptions); + + return $source; + } +} diff --git a/libs/output-mapping/tests/Storage/TableInfoTest.php b/libs/output-mapping/tests/Storage/TableInfoTest.php index 5ec1bef70..974c65cfa 100644 --- a/libs/output-mapping/tests/Storage/TableInfoTest.php +++ b/libs/output-mapping/tests/Storage/TableInfoTest.php @@ -22,5 +22,19 @@ public function testBasic(): void $this->assertEquals('tableId', $tableInfo->getId()); $this->assertTrue($tableInfo->isTyped()); $this->assertEquals(['column1'], $tableInfo->getPrimaryKey()); + $this->assertTrue($tableInfo->isDescriptionSystemManaged()); + } + + public function testIsDescriptionSystemManaged(): void + { + $tableInfo = new TableInfo([ + 'id' => 'tableId', + 'columns' => [], + 'isTyped' => true, + 'primaryKey' => [], + 'isDescriptionSystemManaged' => false, + ]); + + $this->assertFalse($tableInfo->isDescriptionSystemManaged()); } } diff --git a/libs/output-mapping/tests/Writer/TableDescriptionTest.php b/libs/output-mapping/tests/Writer/TableDescriptionTest.php new file mode 100644 index 000000000..b32d84020 --- /dev/null +++ b/libs/output-mapping/tests/Writer/TableDescriptionTest.php @@ -0,0 +1,117 @@ +emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + #[NeedsEmptyOutputBucket] + public function testDescriptionIsUpdatedOnSystemManagedTable(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, 'updated table description', 'updated Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetail['isDescriptionSystemManaged']); + self::assertSame('updated table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('updated Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + #[NeedsEmptyOutputBucket] + public function testDescriptionIsNotOverwrittenOnUserManagedTable(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + + // the user takes over the description + $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition($tableId, [ + 'description' => 'description set by the user', + 'isDescriptionSystemManaged' => false, + 'columns' => [ + ['name' => 'Id', 'description' => 'Id description set by the user'], + ], + ]); + + $this->uploadTable($tableId, 'table description from component', 'Id description from component'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertFalse($tableDetail['isDescriptionSystemManaged']); + self::assertSame('description set by the user', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description set by the user', $this->getColumnDescription($tableDetail, 'Id')); + } + + private function uploadTable(string $tableId, string $tableDescription, string $columnDescription): void + { + $root = $this->temp->getTmpFolder(); + file_put_contents($root . '/upload/tableDescription.csv', "\"1\",\"bob\"\n\"2\",\"alice\"\n"); + + $tableQueue = $this->getTableLoader()->uploadTables( + configuration: new OutputMappingSettings( + configuration: [ + 'mapping' => [ + [ + 'source' => 'tableDescription.csv', + 'destination' => $tableId, + 'columns' => ['Id', 'Name'], + 'description' => $tableDescription, + 'column_metadata' => [ + 'Id' => [ + ['key' => 'KBC.description', 'value' => $columnDescription], + ], + ], + ], + ], + ], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + $jobIds = $tableQueue->waitForAll(); + self::assertCount(1, $jobIds); + } + + private function getColumnDescription(array $tableDetail, string $columnName): ?string + { + $definition = $tableDetail['definition'] ?? []; + self::assertIsArray($definition); + $columns = $definition['columns'] ?? []; + self::assertIsArray($columns); + + foreach ($columns as $column) { + self::assertIsArray($column); + if ($column['name'] === $columnName) { + $columnDefinition = $column['definition'] ?? []; + self::assertIsArray($columnDefinition); + + return $columnDefinition['description'] ?? null; + } + } + + return null; + } +} From 5738bffeed7e58d29c68f1e51468f1604d61bb07 Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 27 Jul 2026 15:07:20 +0200 Subject: [PATCH 02/11] fix(output-mapping): AJDA-2946 diff descriptions, gate backend, stop writing KBC.description Review of the previous commit surfaced three defects, all verified against real Storage: - Storage rejects a table-definition patch that carries no effective change with 400 "No table definition changes were provided.", so a second run with an unchanged description killed the whole output mapping - for an existing table even before any data was loaded. The current descriptions are now read from the table definition and only real changes are sent; when nothing changed, no call is made at all. This also removes the per-run cost in the steady state. - The definition-update endpoint exists on Snowflake and BigQuery only, so on any other backend every run with a description ended with 422. Storing the description is now skipped with an info message there. - Writing the native description makes Storage mirror it into a KBC.description metadata row under the `storage` provider, so output mapping's own componentId row was a duplicate - and on a user-managed table it published a competing description, which broke the acceptance criterion. KBC.description is no longer written as component metadata; resolution and removal live in DescriptionHelper. Both call sites now share a single flag-gated entry point taking TableInfo, so the guard cannot be bypassed, and a ClientException >= 500 is propagated instead of being reported as a user error, consistently with the metadata path. --- libs/output-mapping/phpunit.xml.dist | 2 +- .../src/DeferredTasks/LoadTableQueue.php | 26 +- .../Metadata/ColumnsMetadata.php | 10 +- .../MappingFromConfigurationSchemaColumn.php | 33 +-- .../MappingFromProcessedConfiguration.php | 71 +++--- .../src/Storage/StoragePreparer.php | 2 +- .../src/Storage/TableDescriptionModifier.php | 106 +++++---- libs/output-mapping/src/Storage/TableInfo.php | 45 ++++ .../src/Writer/Helper/DescriptionHelper.php | 64 +++++ .../DeferredTasks/LoadTableQueueTest.php | 2 + .../Metadata/SchemaColumnsMetadataTest.php | 12 +- ...ppingFromConfigurationSchemaColumnTest.php | 12 +- .../MappingFromConfigurationSchemaTest.php | 3 +- .../MappingFromProcessedConfigurationTest.php | 52 +++- .../Storage/TableDescriptionModifierTest.php | 225 ++++++++++++------ ...est.php => TableDescriptionWriterTest.php} | 19 +- 16 files changed, 482 insertions(+), 202 deletions(-) create mode 100644 libs/output-mapping/src/Writer/Helper/DescriptionHelper.php rename libs/output-mapping/tests/Writer/{TableDescriptionTest.php => TableDescriptionWriterTest.php} (83%) diff --git a/libs/output-mapping/phpunit.xml.dist b/libs/output-mapping/phpunit.xml.dist index 717bd8d71..a2a7ff179 100644 --- a/libs/output-mapping/phpunit.xml.dist +++ b/libs/output-mapping/phpunit.xml.dist @@ -39,7 +39,7 @@ tests/Writer/StorageApiHeadlessWriterTest.php tests/Writer/StorageApiLocalTableWriterTest.php tests/Writer/StorageApiSlicedWriterTest.php - tests/Writer/TableDescriptionTest.php + tests/Writer/TableDescriptionWriterTest.php tests/Writer/Workspace diff --git a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php index a2e3ce02a..14eb57013 100644 --- a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php +++ b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php @@ -8,6 +8,7 @@ use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Storage\TableDescriptionModifier; +use Keboola\OutputMapping\Storage\TableInfo as StorageTableInfo; use Keboola\OutputMapping\Table\Result; use Keboola\OutputMapping\Table\Result\Metrics; use Keboola\StorageApi\ClientException; @@ -105,13 +106,12 @@ public function waitForAll(): array ); } - $tableColumns = null; + $tableData = null; switch ($jobResult['operationName']) { case 'tableImport': $tableData = $this->clientWrapper->getTableAndFileStorageClient()->getTable( $jobResult['tableId'], ); - $tableColumns = self::extractTableColumns($tableData); $this->tableResult->addTable(new TableInfo($tableData)); $this->tableResult->addGenericVariable( $jobResult['tableId'], @@ -124,14 +124,13 @@ public function waitForAll(): array $tableData = $this->clientWrapper->getTableAndFileStorageClient()->getTable( $jobResult['results']['id'], ); - $tableColumns = self::extractTableColumns($tableData); $this->tableResult->addTable(new TableInfo($tableData)); $jobResults[] = $jobResult; break; } try { - $this->applyCreatedTableDescriptions($task, $tableColumns); + $this->applyCreatedTableDescriptions($task, $tableData); } catch (InvalidOutputException $e) { $errors[] = $e->getMessage(); } @@ -147,20 +146,9 @@ public function waitForAll(): array } /** - * @param array $tableData table detail as returned by Storage - * @return string[]|null + * @param array|null $tableData table detail as returned by Storage after a successful load */ - private static function extractTableColumns(array $tableData): ?array - { - $columns = $tableData['columns'] ?? null; - - return is_array($columns) ? array_map('strval', $columns) : null; - } - - /** - * @param string[]|null $tableColumns - */ - private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?array $tableColumns): void + private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?array $tableData): void { if ($this->createdTableDescriptions === []) { return; @@ -168,7 +156,7 @@ private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?ar $tableId = $task->getDestinationTableName(); $descriptions = $this->createdTableDescriptions[$tableId] ?? null; - if ($descriptions === null) { + if ($descriptions === null || $tableData === null) { return; } @@ -177,7 +165,7 @@ private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?ar unset($this->createdTableDescriptions[$tableId]); $descriptionModifier = new TableDescriptionModifier($this->clientWrapper, $this->logger); - $descriptionModifier->setCreatedTableDescriptions($descriptions, $tableColumns); + $descriptionModifier->updateDescriptions(new StorageTableInfo($tableData), $descriptions); } public function getTaskCount(): int diff --git a/libs/output-mapping/src/DeferredTasks/Metadata/ColumnsMetadata.php b/libs/output-mapping/src/DeferredTasks/Metadata/ColumnsMetadata.php index 2d55d5bc9..bf7f2d188 100644 --- a/libs/output-mapping/src/DeferredTasks/Metadata/ColumnsMetadata.php +++ b/libs/output-mapping/src/DeferredTasks/Metadata/ColumnsMetadata.php @@ -35,7 +35,15 @@ public function apply(Metadata $apiClient, int $bulkSize = 100): void ]; } - $columnsMetadata[$mappingColumnMetadata->getColumnName()] = $columnMetadata; + // a column may end up with no metadata at all, e.g. when its only entry was the description, + // which is stored in the native Storage description field instead + if ($columnMetadata) { + $columnsMetadata[$mappingColumnMetadata->getColumnName()] = $columnMetadata; + } + } + + if (!$columnsMetadata) { + continue; } $options = new TableMetadataUpdateOptions( diff --git a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php index 1cefd5978..fa89ba409 100644 --- a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php +++ b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php @@ -4,10 +4,10 @@ namespace Keboola\OutputMapping\Mapping; +use Keboola\OutputMapping\Writer\Helper\DescriptionHelper; + class MappingFromConfigurationSchemaColumn { - private const DESCRIPTION_METADATA_KEY = 'KBC.description'; - public function __construct(private readonly array $mapping) { } @@ -45,13 +45,13 @@ public function hasMetadata(): bool return !empty($this->getMetadata()); } + /** + * Column metadata without the description, which is stored in the native Storage description field + * instead - see getDescription(). + */ public function getMetadata(): array { - $metadata = $this->mapping['metadata'] ?? []; - if (isset($this->mapping['description'])) { - $metadata[self::DESCRIPTION_METADATA_KEY] = $this->mapping['description']; - } - return $metadata; + return DescriptionHelper::removeDescriptionFromMetadataMap($this->mapping['metadata'] ?? []); } /** @@ -62,26 +62,17 @@ public function getMetadata(): array public function getDescription(): ?string { if (isset($this->mapping['description'])) { - return self::normalizeDescription($this->mapping['description']); + return DescriptionHelper::normalizeDescription($this->mapping['description']); } // metadata is a variableNode in the configuration, so it is not guaranteed to be an array $metadata = $this->mapping['metadata'] ?? []; - if (is_array($metadata) && isset($metadata[self::DESCRIPTION_METADATA_KEY])) { - return self::normalizeDescription($metadata[self::DESCRIPTION_METADATA_KEY]); + if (is_array($metadata) && isset($metadata[DescriptionHelper::DESCRIPTION_METADATA_KEY])) { + return DescriptionHelper::normalizeDescription( + $metadata[DescriptionHelper::DESCRIPTION_METADATA_KEY], + ); } return null; } - - private static function normalizeDescription(mixed $description): ?string - { - if (!is_scalar($description)) { - return null; - } - - $description = (string) $description; - - return $description !== '' ? $description : null; - } } diff --git a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php index 988d20809..bfcd9d957 100644 --- a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php +++ b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php @@ -6,14 +6,13 @@ use Keboola\OutputMapping\Configuration\Table\Configuration; use Keboola\OutputMapping\Configuration\Table\DeduplicationStrategy; +use Keboola\OutputMapping\Writer\Helper\DescriptionHelper; use Keboola\OutputMapping\Writer\Helper\RestrictedColumnsHelper; use Keboola\OutputMapping\Writer\Table\MappingDestination; use Keboola\OutputMapping\Writer\Table\Source\SourceType; class MappingFromProcessedConfiguration { - private const DESCRIPTION_METADATA_KEY = 'KBC.description'; - private MappingDestination $destination; private MappingFromRawConfigurationAndPhysicalDataWithManifest $source; private array $mapping; @@ -131,6 +130,11 @@ public function getColumns(): array } /** + * Column metadata without the description, which is stored in the native Storage description field + * instead - see getColumnDescriptions(). A column whose only metadata was the description is kept with an + * empty metadata list, because the column list of a table is also derived from these keys + * (TableStructureModifier). + * * @return MappingColumnMetadata[] */ public function getColumnMetadata(): array @@ -142,7 +146,10 @@ public function getColumnMetadata(): array $return = []; foreach ($columnMetadataFromConfiguration as $columnName => $metadata) { - $return[] = new MappingColumnMetadata((string) $columnName, $metadata); + $return[] = new MappingColumnMetadata( + (string) $columnName, + DescriptionHelper::removeDescriptionFromMetadataList($metadata), + ); } return $return; @@ -170,12 +177,16 @@ public function getDistributionKey(): array public function hasMetadata(): bool { - return !empty($this->mapping['metadata']); + return !empty($this->getMetadata()); } + /** + * Table metadata without the description, which is stored in the native Storage description field instead + * - see getTableDescription(). + */ public function getMetadata(): array { - return $this->mapping['metadata'] ?? []; + return DescriptionHelper::removeDescriptionFromMetadataList($this->mapping['metadata'] ?? []); } public function hasTableMetadata(): bool @@ -183,13 +194,13 @@ public function hasTableMetadata(): bool return !empty($this->getTableMetadata()); } + /** + * Table metadata without the description, which is stored in the native Storage description field instead + * - see getTableDescription(). + */ public function getTableMetadata(): array { - $metadata = $this->mapping['table_metadata'] ?? []; - if (isset($this->mapping['description'])) { - $metadata[self::DESCRIPTION_METADATA_KEY] = $this->mapping['description']; - } - return $metadata; + return DescriptionHelper::removeDescriptionFromMetadataMap($this->mapping['table_metadata'] ?? []); } /** @@ -203,35 +214,26 @@ public function getTableMetadata(): array public function getTableDescription(): ?string { if (isset($this->mapping['description'])) { - return self::normalizeDescription($this->mapping['description']); + return DescriptionHelper::normalizeDescription($this->mapping['description']); } // table_metadata is a variableNode in the configuration, so it is not guaranteed to be an array $tableMetadata = $this->mapping['table_metadata'] ?? []; - if (is_array($tableMetadata) && isset($tableMetadata[self::DESCRIPTION_METADATA_KEY])) { - return self::normalizeDescription($tableMetadata[self::DESCRIPTION_METADATA_KEY]); + if (is_array($tableMetadata) && isset($tableMetadata[DescriptionHelper::DESCRIPTION_METADATA_KEY])) { + return DescriptionHelper::normalizeDescription( + $tableMetadata[DescriptionHelper::DESCRIPTION_METADATA_KEY], + ); } - foreach ($this->getMetadata() as $item) { - if (($item['key'] ?? null) === self::DESCRIPTION_METADATA_KEY) { - return self::normalizeDescription($item['value']); + foreach ($this->mapping['metadata'] ?? [] as $item) { + if (is_array($item) && ($item['key'] ?? null) === DescriptionHelper::DESCRIPTION_METADATA_KEY) { + return DescriptionHelper::normalizeDescription($item['value'] ?? null); } } return null; } - private static function normalizeDescription(mixed $description): ?string - { - if (!is_scalar($description)) { - return null; - } - - $description = (string) $description; - - return $description !== '' ? $description : null; - } - /** * Descriptions of the columns, stored in the native Storage description field. Sourced either from the * schema (which cannot be combined with column_metadata) or from the `KBC.description` key of the column @@ -255,14 +257,19 @@ public function getColumnDescriptions(): array return $descriptions; } - foreach ($this->getColumnMetadata() as $columnMetadata) { - foreach ($columnMetadata->getMetadata() as $item) { - if (($item['key'] ?? null) !== self::DESCRIPTION_METADATA_KEY) { + // read from the raw configuration - getColumnMetadata() has the description stripped out + $columnMetadataFromConfiguration = $this->mapping['column_metadata'] ? + RestrictedColumnsHelper::removeRestrictedColumnsFromColumnMetadata($this->mapping['column_metadata']) : + []; + + foreach ($columnMetadataFromConfiguration as $columnName => $metadata) { + foreach ($metadata as $item) { + if (!is_array($item) || ($item['key'] ?? null) !== DescriptionHelper::DESCRIPTION_METADATA_KEY) { continue; } - $description = self::normalizeDescription($item['value']); + $description = DescriptionHelper::normalizeDescription($item['value'] ?? null); if ($description !== null) { - $descriptions[$columnMetadata->getColumnName()] = $description; + $descriptions[(string) $columnName] = $description; } } } diff --git a/libs/output-mapping/src/Storage/StoragePreparer.php b/libs/output-mapping/src/Storage/StoragePreparer.php index a33308a42..babca9702 100644 --- a/libs/output-mapping/src/Storage/StoragePreparer.php +++ b/libs/output-mapping/src/Storage/StoragePreparer.php @@ -74,7 +74,7 @@ public function prepareStorageBucketAndTable( $descriptions = TableDescription::createFromMapping($processedSource); if (!$descriptions->isEmpty()) { $descriptionModifier = new TableDescriptionModifier($this->clientWrapper, $this->logger); - $descriptionModifier->updateExistingTableDescriptions($destinationTableInfo, $descriptions); + $descriptionModifier->updateDescriptions($destinationTableInfo, $descriptions); } } } diff --git a/libs/output-mapping/src/Storage/TableDescriptionModifier.php b/libs/output-mapping/src/Storage/TableDescriptionModifier.php index 801d9fe92..1043baf6e 100644 --- a/libs/output-mapping/src/Storage/TableDescriptionModifier.php +++ b/libs/output-mapping/src/Storage/TableDescriptionModifier.php @@ -18,24 +18,36 @@ * - system-managed - output mapping stores/updates the table and column description, * - user-managed - output mapping discards the description it produced so that the value set by the user * is never overwritten. - * - * Tables created by the current run are always system-managed (that is the Storage default), so their - * description is stored without asking. */ class TableDescriptionModifier { + /** + * Backends where Storage implements the table-definition update endpoint. On any other backend the + * description cannot be stored in the native field at all, so it is skipped instead of failing the job. + * Mirrors Keboola\Storage\TablesColumns\DefinitionUpdate\DefinitionUpdate::SUPPORTED_BACKENDS. + */ + private const BACKENDS_SUPPORTING_DEFINITION_UPDATE = [ + 'snowflake', + 'bigquery', + ]; + public function __construct( private readonly ClientWrapper $clientWrapper, private readonly LoggerInterface $logger, ) { } - /** - * Stores descriptions on a table which already existed before this run, unless its description is - * managed by the user. - */ - public function updateExistingTableDescriptions(TableInfo $tableInfo, TableDescription $descriptions): void + public function updateDescriptions(TableInfo $tableInfo, TableDescription $descriptions): void { + if (!in_array($tableInfo->getBucketBackend(), self::BACKENDS_SUPPORTING_DEFINITION_UPDATE, true)) { + $this->logger->info(sprintf( + 'Storing description of table "%s" is not supported on the "%s" backend, skipping it.', + $tableInfo->getId(), + (string) $tableInfo->getBucketBackend(), + )); + return; + } + if (!$tableInfo->isDescriptionSystemManaged()) { $this->logger->info(sprintf( 'Description of table "%s" is managed by the user, keeping the current value.', @@ -44,37 +56,62 @@ public function updateExistingTableDescriptions(TableInfo $tableInfo, TableDescr return; } - $this->applyDescriptions($descriptions, $tableInfo->getColumns()); - } + $tableDefinitionUpdate = $this->buildTableDefinitionUpdate($tableInfo, $descriptions); + if (!$tableDefinitionUpdate) { + // Nothing changed since the last run. Storage rejects a patch without any effective change with + // "No table definition changes were provided." (400), so the call must be skipped entirely. + return; + } - /** - * Stores descriptions on a table created by the current run. - * - * @param string[]|null $tableColumns columns of the created table, null when the column list is unknown - */ - public function setCreatedTableDescriptions(TableDescription $descriptions, ?array $tableColumns): void - { - $this->applyDescriptions($descriptions, $tableColumns); + try { + $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition( + $tableInfo->getId(), + $tableDefinitionUpdate, + ); + } catch (ClientException $e) { + if ($e->getCode() >= 500) { + // Let a Storage outage surface as a retryable application error, consistently with the + // metadata path in LoadTableQueue. + throw $e; + } + + throw new InvalidOutputException( + sprintf( + 'Cannot update description of table "%s": %s', + $tableInfo->getId(), + $e->getMessage(), + ), + $e->getCode(), + $e, + ); + } } /** - * @param string[]|null $tableColumns columns existing in the table, null disables the check + * @return array{description?: string, columns?: list} */ - private function applyDescriptions(TableDescription $descriptions, ?array $tableColumns): void + private function buildTableDefinitionUpdate(TableInfo $tableInfo, TableDescription $descriptions): array { $tableDefinitionUpdate = []; - if ($descriptions->getTableDescription() !== null) { - $tableDefinitionUpdate['description'] = $descriptions->getTableDescription(); + $tableDescription = $descriptions->getTableDescription(); + if ($tableDescription !== null && $tableDescription !== $tableInfo->getDescription()) { + $tableDefinitionUpdate['description'] = $tableDescription; } + $tableColumns = $tableInfo->getColumns(); + $storedColumnDescriptions = $tableInfo->getColumnDescriptions(); + $columns = []; $missingColumns = []; foreach ($descriptions->getColumnDescriptions() as $columnName => $columnDescription) { - if ($tableColumns !== null && !in_array($columnName, $tableColumns, true)) { + if (!in_array($columnName, $tableColumns, true)) { $missingColumns[] = $columnName; continue; } + if ($columnDescription === ($storedColumnDescriptions[$columnName] ?? null)) { + continue; + } $columns[] = [ 'name' => $columnName, 'description' => $columnDescription, @@ -85,7 +122,7 @@ private function applyDescriptions(TableDescription $descriptions, ?array $table $this->logger->warning(sprintf( 'Cannot store description of column(s) "%s" of table "%s", the column(s) do not exist.', implode('", "', $missingColumns), - $descriptions->getTableId(), + $tableInfo->getId(), )); } @@ -93,25 +130,6 @@ private function applyDescriptions(TableDescription $descriptions, ?array $table $tableDefinitionUpdate['columns'] = $columns; } - if (!$tableDefinitionUpdate) { - return; - } - - try { - $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition( - $descriptions->getTableId(), - $tableDefinitionUpdate, - ); - } catch (ClientException $e) { - throw new InvalidOutputException( - sprintf( - 'Cannot update description of table "%s": %s', - $descriptions->getTableId(), - $e->getMessage(), - ), - $e->getCode(), - $e, - ); - } + return $tableDefinitionUpdate; } } diff --git a/libs/output-mapping/src/Storage/TableInfo.php b/libs/output-mapping/src/Storage/TableInfo.php index 7230e7154..fcf202757 100644 --- a/libs/output-mapping/src/Storage/TableInfo.php +++ b/libs/output-mapping/src/Storage/TableInfo.php @@ -41,4 +41,49 @@ public function getPrimaryKey(): array { return $this->tableInfo['primaryKey']; } + + public function getBucketBackend(): ?string + { + $backend = $this->tableInfo['bucket']['backend'] ?? null; + + return is_string($backend) ? $backend : null; + } + + /** + * Description stored in the native Storage description field, or null when the table has none. Read from + * the table definition, the same source input mapping reads. + */ + public function getDescription(): ?string + { + $description = $this->tableInfo['definition']['description'] ?? null; + + return is_string($description) && $description !== '' ? $description : null; + } + + /** + * Descriptions stored in the native Storage description field of the columns, keyed by column name. + * Columns without a description are omitted. + * + * @return array + */ + public function getColumnDescriptions(): array + { + $columns = $this->tableInfo['definition']['columns'] ?? []; + if (!is_array($columns)) { + return []; + } + + $descriptions = []; + foreach ($columns as $column) { + if (!is_array($column) || !isset($column['name'])) { + continue; + } + $description = $column['definition']['description'] ?? null; + if (is_string($description) && $description !== '') { + $descriptions[(string) $column['name']] = $description; + } + } + + return $descriptions; + } } diff --git a/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php b/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php new file mode 100644 index 000000000..41d024ab4 --- /dev/null +++ b/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php @@ -0,0 +1,64 @@ + value metadata map. + * + * @param mixed $metadata the node is a variableNode in the configuration, so it may be anything + * @return array + */ + public static function removeDescriptionFromMetadataMap(mixed $metadata): array + { + if (!is_array($metadata)) { + return []; + } + + unset($metadata[self::DESCRIPTION_METADATA_KEY]); + + return $metadata; + } + + /** + * Removes the description from a list of {key, value} metadata items. + * + * @param array $metadata + * @return list + */ + public static function removeDescriptionFromMetadataList(array $metadata): array + { + return array_values(array_filter( + $metadata, + fn($item): bool => !is_array($item) || ($item['key'] ?? null) !== self::DESCRIPTION_METADATA_KEY, + )); + } +} diff --git a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php index 619befa53..c43235ac3 100644 --- a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php +++ b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php @@ -658,6 +658,7 @@ public function testWaitForAllStoresDescriptionsOfCreatedTable(): void 'displayName' => 'my-name', 'name' => 'my-name', 'columns' => ['col1'], + 'bucket' => ['backend' => 'snowflake'], 'lastImportDate' => null, 'lastChangeDate' => null, ]) @@ -729,6 +730,7 @@ public function testWaitForAllReportsFailedDescriptionUpdateAsError(): void 'displayName' => 'my-name', 'name' => 'my-name', 'columns' => ['col1'], + 'bucket' => ['backend' => 'snowflake'], 'lastImportDate' => null, 'lastChangeDate' => null, ]) diff --git a/libs/output-mapping/tests/DeferredTasks/Metadata/SchemaColumnsMetadataTest.php b/libs/output-mapping/tests/DeferredTasks/Metadata/SchemaColumnsMetadataTest.php index 69acd9bb5..03ac48ade 100644 --- a/libs/output-mapping/tests/DeferredTasks/Metadata/SchemaColumnsMetadataTest.php +++ b/libs/output-mapping/tests/DeferredTasks/Metadata/SchemaColumnsMetadataTest.php @@ -38,8 +38,8 @@ public function applyDataProvider(): Generator 'col2' => [ [ 'columnName' => 'col2', - 'key' => 'KBC.description', - 'value' => 'col2 description', + 'key' => 'key3', + 'value' => 'val3', ], ], ], @@ -68,8 +68,8 @@ public function applyDataProvider(): Generator 'col2' => [ [ 'columnName' => 'col2', - 'key' => 'KBC.description', - 'value' => 'col2 description', + 'key' => 'key3', + 'value' => 'val3', ], ], ], @@ -109,9 +109,13 @@ function (TableMetadataUpdateOptions $options) use ($matcher, $expectedColumnsMe 'key2' => 'val2', ], ]), + // the description is stored in the native Storage description field, so only key3 is written here new MappingFromConfigurationSchemaColumn([ 'name' => 'col2', 'description' => 'col2 description', + 'metadata' => [ + 'key3' => 'val3', + ], ]), new MappingFromConfigurationSchemaColumn([ 'name' => 'col3', diff --git a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php index 613339a34..a67895a12 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php @@ -52,13 +52,8 @@ public function testGetters(): void self::assertTrue($schemColumn->isPrimaryKey()); self::assertTrue($schemColumn->isDistributionKey()); self::assertTrue($schemColumn->hasMetadata()); - self::assertSame( - [ - 'KBC.datatype.type' => 'STRING', - 'KBC.description' => 'Some description of the newColumn.', - ], - $schemColumn->getMetadata(), - ); + // the description is stored in the native Storage description field, not as KBC.description metadata + self::assertSame(['KBC.datatype.type' => 'STRING'], $schemColumn->getMetadata()); self::assertSame('Some description of the newColumn.', $schemColumn->getDescription()); } @@ -72,6 +67,9 @@ public function testGetDescriptionFromMetadata(): void ]); self::assertSame('Description from metadata.', $schemColumn->getDescription()); + // the key is consumed as the description, so it must not be reported as metadata as well + self::assertSame([], $schemColumn->getMetadata()); + self::assertFalse($schemColumn->hasMetadata()); } public function testGetDescriptionIgnoresNonArrayMetadata(): void diff --git a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaTest.php b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaTest.php index 44ed8825f..c099624eb 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaTest.php @@ -68,10 +68,11 @@ public function testFullConfig(): void self::assertTrue($mappingSchema->isPrimaryKey()); self::assertTrue($mappingSchema->isDistributionKey()); self::assertTrue($mappingSchema->hasMetadata()); + // the description is stored in the native Storage description field, not as KBC.description metadata self::assertEquals([ 'key1' => 'value1', 'key2' => 'value2', - 'KBC.description' => 'col1 description', ], $mappingSchema->getMetadata()); + self::assertSame('col1 description', $mappingSchema->getDescription()); } } diff --git a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php index 1e10c4765..ecc627472 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php @@ -219,12 +219,62 @@ public function testTableMetadata(): void $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); $mapping = new MappingFromProcessedConfiguration($mapping, $physicalDataWithManifest); + // the description is stored in the native Storage description field, not as KBC.description metadata self::assertEquals([ 'key1' => 'val1', 'key2' => 'val2', - 'KBC.description' => 'table desc', ], $mapping->getTableMetadata()); self::assertTrue($mapping->hasTableMetadata()); + self::assertSame('table desc', $mapping->getTableDescription()); + } + + public function testDescriptionIsNotReportedAsMetadata(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'table_metadata' => ['KBC.description' => 'table desc'], + 'metadata' => [ + ['key' => 'KBC.description', 'value' => 'table desc'], + ['key' => 'KBC.name', 'value' => 'whatever'], + ], + 'column_metadata' => [ + 'col1' => [ + ['key' => 'KBC.description', 'value' => 'col1 desc'], + ['key' => 'KBC.datatype.type', 'value' => 'STRING'], + ], + // a column whose only metadata is the description must stay in the list, the column list of a + // table is derived from these keys as well + 'col2' => [['key' => 'KBC.description', 'value' => 'col2 desc']], + ], + ], $physicalDataWithManifest); + + self::assertSame([], $mapping->getTableMetadata()); + self::assertFalse($mapping->hasTableMetadata()); + self::assertSame([['key' => 'KBC.name', 'value' => 'whatever']], $mapping->getMetadata()); + self::assertTrue($mapping->hasMetadata()); + + $columnMetadata = $mapping->getColumnMetadata(); + self::assertCount(2, $columnMetadata); + self::assertSame('col1', $columnMetadata[0]->getColumnName()); + self::assertSame([['key' => 'KBC.datatype.type', 'value' => 'STRING']], $columnMetadata[0]->getMetadata()); + self::assertSame('col2', $columnMetadata[1]->getColumnName()); + self::assertSame([], $columnMetadata[1]->getMetadata()); + + self::assertSame('table desc', $mapping->getTableDescription()); + self::assertSame(['col1' => 'col1 desc', 'col2' => 'col2 desc'], $mapping->getColumnDescriptions()); + } + + public function testHasMetadataIsFalseWhenOnlyDescriptionIsSet(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'metadata' => [['key' => 'KBC.description', 'value' => 'table desc']], + ], $physicalDataWithManifest); + + self::assertSame([], $mapping->getMetadata()); + self::assertFalse($mapping->hasMetadata()); } public function testHasSchemaConfiguration(): void diff --git a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php index 55a290443..cb56eb420 100644 --- a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php +++ b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php @@ -4,6 +4,7 @@ namespace Keboola\OutputMapping\Tests\Storage; +use Generator; use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Storage\TableDescriptionModifier; @@ -20,10 +21,21 @@ class TableDescriptionModifierTest extends TestCase { private const TABLE_ID = 'in.c-main.table'; - public function testUpdateExistingTableDescriptions(): void + private TestHandler $logHandler; + private Logger $logger; + + protected function setUp(): void + { + parent::setUp(); + + $this->logHandler = new TestHandler(); + $this->logger = new Logger('test', [$this->logHandler]); + } + + public function testDescriptionsAreStored(): void { $client = $this->createMock(Client::class); - $client->expects($this->once()) + $client->expects(self::once()) ->method('updateTableDefinition') ->with( self::TABLE_ID, @@ -37,106 +49,131 @@ public function testUpdateExistingTableDescriptions(): void ) ->willReturn([]); - $logHandler = new TestHandler(); - $logger = new Logger('test', [$logHandler]); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); - $modifier->updateExistingTableDescriptions( - $this->createTableInfo(true, ['col1', 'col2']), + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1', 'col2']), new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc', 'col2' => 'col2 desc']), ); - self::assertFalse($logHandler->hasWarningRecords()); + self::assertFalse($this->logHandler->hasWarningRecords()); } - public function testUpdateExistingTableDescriptionsIsSkippedForUserManagedDescription(): void + public function testStoringIsSkippedForUserManagedDescription(): void { $client = $this->createMock(Client::class); - $client->expects($this->never()) + $client->expects(self::never()) ->method('updateTableDefinition'); - $logHandler = new TestHandler(); - $logger = new Logger('test', [$logHandler]); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); - $modifier->updateExistingTableDescriptions( - $this->createTableInfo(false, ['col1']), + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1'], isDescriptionSystemManaged: false), new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), ); - self::assertTrue($logHandler->hasInfoThatContains(sprintf( + self::assertTrue($this->logHandler->hasInfoThatContains(sprintf( 'Description of table "%s" is managed by the user, keeping the current value.', self::TABLE_ID, ))); } - public function testUpdateExistingTableDescriptionsSkipsMissingColumns(): void + /** @dataProvider unsupportedBackendProvider */ + public function testStoringIsSkippedOnBackendWithoutDefinitionUpdate(?string $backend): void { $client = $this->createMock(Client::class); - $client->expects($this->once()) - ->method('updateTableDefinition') - ->with( - self::TABLE_ID, - [ - 'columns' => [ - ['name' => 'col1', 'description' => 'col1 desc'], - ], - ], - ) - ->willReturn([]); + $client->expects(self::never()) + ->method('updateTableDefinition'); - $logHandler = new TestHandler(); - $logger = new Logger('test', [$logHandler]); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); - $modifier->updateExistingTableDescriptions( - $this->createTableInfo(true, ['col1']), - new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc', 'col2' => 'col2 desc']), + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1'], backend: $backend), + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), ); - self::assertTrue($logHandler->hasWarningThatContains(sprintf( - 'Cannot store description of column(s) "col2" of table "%s", the column(s) do not exist.', + self::assertTrue($this->logHandler->hasInfoThatContains(sprintf( + 'Storing description of table "%s" is not supported on the', self::TABLE_ID, ))); } - public function testUpdateExistingTableDescriptionsWithNothingToStoreDoesNotCallStorage(): void + public static function unsupportedBackendProvider(): Generator + { + yield 'synapse' => ['backend' => 'synapse']; + yield 'exasol' => ['backend' => 'exasol']; + yield 'teradata' => ['backend' => 'teradata']; + yield 'postgres' => ['backend' => 'postgres']; + yield 'backend missing in the response' => ['backend' => null]; + } + + /** @dataProvider supportedBackendProvider */ + public function testStoringIsPerformedOnSupportedBackend(string $backend): void + { + $client = $this->createMock(Client::class); + $client->expects(self::once()) + ->method('updateTableDefinition') + ->willReturn([]); + + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: [], backend: $backend), + new TableDescription(self::TABLE_ID, 'table desc', []), + ); + } + + public static function supportedBackendProvider(): Generator + { + yield 'snowflake' => ['backend' => 'snowflake']; + yield 'bigquery' => ['backend' => 'bigquery']; + } + + /** + * Storage rejects a patch that carries no effective change with 400 "No table definition changes were + * provided.", so an unchanged description must not be sent at all. + */ + public function testUnchangedDescriptionsAreNotSent(): void { $client = $this->createMock(Client::class); - $client->expects($this->never()) + $client->expects(self::never()) ->method('updateTableDefinition'); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), new Logger('test')); - $modifier->updateExistingTableDescriptions( - $this->createTableInfo(true, ['col1']), - new TableDescription(self::TABLE_ID, null, []), + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo( + columns: ['col1'], + storedTableDescription: 'table desc', + storedColumnDescriptions: ['col1' => 'col1 desc'], + ), + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), ); } - public function testSetCreatedTableDescriptions(): void + public function testOnlyChangedDescriptionsAreSent(): void { $client = $this->createMock(Client::class); - $client->expects($this->once()) + $client->expects(self::once()) ->method('updateTableDefinition') ->with( self::TABLE_ID, [ - 'description' => 'table desc', 'columns' => [ - ['name' => 'col1', 'description' => 'col1 desc'], + ['name' => 'col2', 'description' => 'new col2 desc'], ], ], ) ->willReturn([]); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), new Logger('test')); - $modifier->setCreatedTableDescriptions( - new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), - ['col1'], + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo( + columns: ['col1', 'col2'], + storedTableDescription: 'table desc', + storedColumnDescriptions: ['col1' => 'col1 desc', 'col2' => 'old col2 desc'], + ), + new TableDescription( + self::TABLE_ID, + 'table desc', + ['col1' => 'col1 desc', 'col2' => 'new col2 desc'], + ), ); } - public function testSetCreatedTableDescriptionsWithUnknownColumnList(): void + public function testMissingColumnsAreSkippedWithWarning(): void { $client = $this->createMock(Client::class); - $client->expects($this->once()) + $client->expects(self::once()) ->method('updateTableDefinition') ->with( self::TABLE_ID, @@ -148,32 +185,42 @@ public function testSetCreatedTableDescriptionsWithUnknownColumnList(): void ) ->willReturn([]); - $logHandler = new TestHandler(); - $logger = new Logger('test', [$logHandler]); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), $logger); - $modifier->setCreatedTableDescriptions( - new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc']), - null, + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1']), + new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc', 'col2' => 'col2 desc']), ); - self::assertFalse($logHandler->hasWarningRecords()); + self::assertTrue($this->logHandler->hasWarningThatContains(sprintf( + 'Cannot store description of column(s) "col2" of table "%s", the column(s) do not exist.', + self::TABLE_ID, + ))); + } + + public function testNothingToStoreDoesNotCallStorage(): void + { + $client = $this->createMock(Client::class); + $client->expects(self::never()) + ->method('updateTableDefinition'); + + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1']), + new TableDescription(self::TABLE_ID, null, []), + ); } - public function testStorageErrorIsWrappedInInvalidOutputException(): void + public function testUserErrorIsWrappedInInvalidOutputException(): void { $clientException = new ClientException('Table definition update failed', 400); $client = $this->createMock(Client::class); - $client->expects($this->once()) + $client->expects(self::once()) ->method('updateTableDefinition') ->willThrowException($clientException); - $modifier = new TableDescriptionModifier($this->createClientWrapper($client), new Logger('test')); - try { - $modifier->setCreatedTableDescriptions( + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: []), new TableDescription(self::TABLE_ID, 'table desc', []), - null, ); self::fail('Storing the description should fail with InvalidOutputException.'); } catch (InvalidOutputException $e) { @@ -186,25 +233,65 @@ public function testStorageErrorIsWrappedInInvalidOutputException(): void } } - private function createClientWrapper(Client&MockObject $client): ClientWrapper + public function testApplicationErrorIsPropagated(): void + { + $clientException = new ClientException('Internal Server Error', 500); + + $client = $this->createMock(Client::class); + $client->expects(self::once()) + ->method('updateTableDefinition') + ->willThrowException($clientException); + + try { + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: []), + new TableDescription(self::TABLE_ID, 'table desc', []), + ); + self::fail('Storing the description should fail with ClientException.'); + } catch (ClientException $e) { + self::assertSame($clientException, $e); + } + } + + private function createModifier(Client&MockObject $client): TableDescriptionModifier { $clientWrapper = $this->createMock(ClientWrapper::class); $clientWrapper->method('getTableAndFileStorageClient')->willReturn($client); - return $clientWrapper; + return new TableDescriptionModifier($clientWrapper, $this->logger); } /** * @param string[] $columns + * @param array $storedColumnDescriptions */ - private function createTableInfo(bool $isDescriptionSystemManaged, array $columns): TableInfo - { + private function createTableInfo( + array $columns = [], + bool $isDescriptionSystemManaged = true, + ?string $backend = 'snowflake', + ?string $storedTableDescription = null, + array $storedColumnDescriptions = [], + ): TableInfo { + $definitionColumns = []; + foreach ($columns as $columnName) { + $definitionColumn = ['name' => $columnName]; + if (isset($storedColumnDescriptions[$columnName])) { + $definitionColumn['definition'] = ['description' => $storedColumnDescriptions[$columnName]]; + } + $definitionColumns[] = $definitionColumn; + } + return new TableInfo([ 'id' => self::TABLE_ID, 'columns' => $columns, 'isTyped' => true, 'primaryKey' => [], 'isDescriptionSystemManaged' => $isDescriptionSystemManaged, + 'bucket' => ['backend' => $backend], + 'definition' => [ + 'description' => $storedTableDescription, + 'columns' => $definitionColumns, + ], ]); } } diff --git a/libs/output-mapping/tests/Writer/TableDescriptionTest.php b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php similarity index 83% rename from libs/output-mapping/tests/Writer/TableDescriptionTest.php rename to libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php index b32d84020..69e58864d 100644 --- a/libs/output-mapping/tests/Writer/TableDescriptionTest.php +++ b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php @@ -9,7 +9,7 @@ use Keboola\OutputMapping\Tests\AbstractTestCase; use Keboola\OutputMapping\Tests\Needs\NeedsEmptyOutputBucket; -class TableDescriptionTest extends AbstractTestCase +class TableDescriptionWriterTest extends AbstractTestCase { #[NeedsEmptyOutputBucket] public function testDescriptionIsStoredOnCreatedTable(): void @@ -37,6 +37,23 @@ public function testDescriptionIsUpdatedOnSystemManagedTable(): void self::assertSame('updated Id description', $this->getColumnDescription($tableDetail, 'Id')); } + /** + * Storage rejects a table-definition patch without any effective change with 400 "No table definition + * changes were provided.", so a repeated run with an unchanged description must not send the patch at all. + */ + #[NeedsEmptyOutputBucket] + public function testRepeatedRunWithUnchangedDescriptionSucceeds(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, 'table description', 'Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + #[NeedsEmptyOutputBucket] public function testDescriptionIsNotOverwrittenOnUserManagedTable(): void { From e1be747ab5ac6e81d17e8c07ecbf5548dce47adc Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 27 Jul 2026 15:48:53 +0200 Subject: [PATCH 03/11] test(output-mapping): drop removed backends from the unsupported-backend provider Synapse, Exasol and Teradata are no longer Storage backends - the legacy BucketBackend factory registers Snowflake only and Model_Buckets::availableBackends() lists snowflake, bigquery and postgres. Postgres is the one live bucket backend the table-definition update endpoint does not support, so it is what the gate guards. --- .../tests/Storage/TableDescriptionModifierTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php index cb56eb420..8e244be9d 100644 --- a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php +++ b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php @@ -94,9 +94,8 @@ public function testStoringIsSkippedOnBackendWithoutDefinitionUpdate(?string $ba public static function unsupportedBackendProvider(): Generator { - yield 'synapse' => ['backend' => 'synapse']; - yield 'exasol' => ['backend' => 'exasol']; - yield 'teradata' => ['backend' => 'teradata']; + // postgres is a bucket backend Storage allows (Model_Buckets::availableBackends()) but the + // table-definition update endpoint does not support it yield 'postgres' => ['backend' => 'postgres']; yield 'backend missing in the response' => ['backend' => null]; } From 3534278c9d97e86605e3041df661f3b39c01fac0 Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 27 Jul 2026 16:03:05 +0200 Subject: [PATCH 04/11] refactor(output-mapping): drop the backend gate for storing descriptions Snowflake, BigQuery and Postgres are the only bucket backends Storage still offers, and Postgres is not something output mapping needs to support here, so the gate guarded a case that cannot occur. Removing it takes with it the backend list duplicated from the platform, TableInfo::getBucketBackend() and the fixtures that only existed to feed it. --- .../src/Storage/TableDescriptionModifier.php | 19 ------- libs/output-mapping/src/Storage/TableInfo.php | 7 --- .../DeferredTasks/LoadTableQueueTest.php | 2 - .../Storage/TableDescriptionModifierTest.php | 49 ------------------- 4 files changed, 77 deletions(-) diff --git a/libs/output-mapping/src/Storage/TableDescriptionModifier.php b/libs/output-mapping/src/Storage/TableDescriptionModifier.php index 1043baf6e..751586d94 100644 --- a/libs/output-mapping/src/Storage/TableDescriptionModifier.php +++ b/libs/output-mapping/src/Storage/TableDescriptionModifier.php @@ -21,16 +21,6 @@ */ class TableDescriptionModifier { - /** - * Backends where Storage implements the table-definition update endpoint. On any other backend the - * description cannot be stored in the native field at all, so it is skipped instead of failing the job. - * Mirrors Keboola\Storage\TablesColumns\DefinitionUpdate\DefinitionUpdate::SUPPORTED_BACKENDS. - */ - private const BACKENDS_SUPPORTING_DEFINITION_UPDATE = [ - 'snowflake', - 'bigquery', - ]; - public function __construct( private readonly ClientWrapper $clientWrapper, private readonly LoggerInterface $logger, @@ -39,15 +29,6 @@ public function __construct( public function updateDescriptions(TableInfo $tableInfo, TableDescription $descriptions): void { - if (!in_array($tableInfo->getBucketBackend(), self::BACKENDS_SUPPORTING_DEFINITION_UPDATE, true)) { - $this->logger->info(sprintf( - 'Storing description of table "%s" is not supported on the "%s" backend, skipping it.', - $tableInfo->getId(), - (string) $tableInfo->getBucketBackend(), - )); - return; - } - if (!$tableInfo->isDescriptionSystemManaged()) { $this->logger->info(sprintf( 'Description of table "%s" is managed by the user, keeping the current value.', diff --git a/libs/output-mapping/src/Storage/TableInfo.php b/libs/output-mapping/src/Storage/TableInfo.php index fcf202757..3ede2b931 100644 --- a/libs/output-mapping/src/Storage/TableInfo.php +++ b/libs/output-mapping/src/Storage/TableInfo.php @@ -42,13 +42,6 @@ public function getPrimaryKey(): array return $this->tableInfo['primaryKey']; } - public function getBucketBackend(): ?string - { - $backend = $this->tableInfo['bucket']['backend'] ?? null; - - return is_string($backend) ? $backend : null; - } - /** * Description stored in the native Storage description field, or null when the table has none. Read from * the table definition, the same source input mapping reads. diff --git a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php index c43235ac3..619befa53 100644 --- a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php +++ b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php @@ -658,7 +658,6 @@ public function testWaitForAllStoresDescriptionsOfCreatedTable(): void 'displayName' => 'my-name', 'name' => 'my-name', 'columns' => ['col1'], - 'bucket' => ['backend' => 'snowflake'], 'lastImportDate' => null, 'lastChangeDate' => null, ]) @@ -730,7 +729,6 @@ public function testWaitForAllReportsFailedDescriptionUpdateAsError(): void 'displayName' => 'my-name', 'name' => 'my-name', 'columns' => ['col1'], - 'bucket' => ['backend' => 'snowflake'], 'lastImportDate' => null, 'lastChangeDate' => null, ]) diff --git a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php index 8e244be9d..6db4fbb17 100644 --- a/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php +++ b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php @@ -4,7 +4,6 @@ namespace Keboola\OutputMapping\Tests\Storage; -use Generator; use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Storage\TableDescriptionModifier; @@ -74,52 +73,6 @@ public function testStoringIsSkippedForUserManagedDescription(): void ))); } - /** @dataProvider unsupportedBackendProvider */ - public function testStoringIsSkippedOnBackendWithoutDefinitionUpdate(?string $backend): void - { - $client = $this->createMock(Client::class); - $client->expects(self::never()) - ->method('updateTableDefinition'); - - $this->createModifier($client)->updateDescriptions( - $this->createTableInfo(columns: ['col1'], backend: $backend), - new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), - ); - - self::assertTrue($this->logHandler->hasInfoThatContains(sprintf( - 'Storing description of table "%s" is not supported on the', - self::TABLE_ID, - ))); - } - - public static function unsupportedBackendProvider(): Generator - { - // postgres is a bucket backend Storage allows (Model_Buckets::availableBackends()) but the - // table-definition update endpoint does not support it - yield 'postgres' => ['backend' => 'postgres']; - yield 'backend missing in the response' => ['backend' => null]; - } - - /** @dataProvider supportedBackendProvider */ - public function testStoringIsPerformedOnSupportedBackend(string $backend): void - { - $client = $this->createMock(Client::class); - $client->expects(self::once()) - ->method('updateTableDefinition') - ->willReturn([]); - - $this->createModifier($client)->updateDescriptions( - $this->createTableInfo(columns: [], backend: $backend), - new TableDescription(self::TABLE_ID, 'table desc', []), - ); - } - - public static function supportedBackendProvider(): Generator - { - yield 'snowflake' => ['backend' => 'snowflake']; - yield 'bigquery' => ['backend' => 'bigquery']; - } - /** * Storage rejects a patch that carries no effective change with 400 "No table definition changes were * provided.", so an unchanged description must not be sent at all. @@ -267,7 +220,6 @@ private function createModifier(Client&MockObject $client): TableDescriptionModi private function createTableInfo( array $columns = [], bool $isDescriptionSystemManaged = true, - ?string $backend = 'snowflake', ?string $storedTableDescription = null, array $storedColumnDescriptions = [], ): TableInfo { @@ -286,7 +238,6 @@ private function createTableInfo( 'isTyped' => true, 'primaryKey' => [], 'isDescriptionSystemManaged' => $isDescriptionSystemManaged, - 'bucket' => ['backend' => $backend], 'definition' => [ 'description' => $storedTableDescription, 'columns' => $definitionColumns, From d2b23cb8a4bc7ded5456f2b32dd6acdd68e4daa4 Mon Sep 17 00:00:00 2001 From: zajca Date: Wed, 29 Jul 2026 15:02:02 +0200 Subject: [PATCH 05/11] feat(output-mapping): AJDA-2946 embed descriptions in the create payload A table created by output mapping through the table-definition API now carries its table and column descriptions directly in the create payload, so no extra blocking table-definition update job runs after the load. This covers all three create variants - typed from `schema`, typed from legacy column metadata, and non-typed from manifest `columns`. The deferred path is left only for a table created by the load job itself (CreateAndLoadTableTask), which has no create payload to embed into. LoadTableTaskCreator::create() therefore returns a LoadTableTaskResult carrying the descriptions that could not be embedded, and LoadTableQueue applies just those once the load finished. FailedLoadTableDecider now ignores the KBC.description metadata row under the "storage" provider. A description in the create payload makes Storage write that row at create time, before any data is loaded, so it says nothing about the table having been used and must not block the drop of a failed empty table. A failed load also drops its pending description, so the "was not stored" warning stays reserved for descriptions lost by mistake. --- .../DeferredTasks/FailedLoadTableDecider.php | 11 + .../src/DeferredTasks/LoadTableQueue.php | 39 ++- .../src/LoadTableTaskCreator.php | 47 +++- .../src/LoadTableTaskResult.php | 37 +++ .../src/Storage/TableCreator.php | 17 +- libs/output-mapping/src/TableLoader.php | 22 +- ...eateTableDefinitionDescriptionEnricher.php | 84 +++++++ .../FailedLoadTableDeciderTest.php | 60 ++++- .../DeferredTasks/LoadTableQueueTest.php | 230 +++++++++++++++++- .../tests/LoadTableTaskCreatorTest.php | 158 +++++++++++- .../tests/Storage/TableCreatorTest.php | 8 +- ...TableDefinitionDescriptionEnricherTest.php | 183 ++++++++++++++ 12 files changed, 844 insertions(+), 52 deletions(-) create mode 100644 libs/output-mapping/src/LoadTableTaskResult.php create mode 100644 libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php create mode 100644 libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php diff --git a/libs/output-mapping/src/DeferredTasks/FailedLoadTableDecider.php b/libs/output-mapping/src/DeferredTasks/FailedLoadTableDecider.php index 3d26e8b88..217fab53a 100644 --- a/libs/output-mapping/src/DeferredTasks/FailedLoadTableDecider.php +++ b/libs/output-mapping/src/DeferredTasks/FailedLoadTableDecider.php @@ -4,6 +4,7 @@ namespace Keboola\OutputMapping\DeferredTasks; +use Keboola\OutputMapping\Writer\Helper\DescriptionHelper; use Keboola\StorageApi\ClientException; use Keboola\StorageApiBranch\ClientWrapper; use Psr\Log\LoggerInterface; @@ -29,6 +30,16 @@ public static function decideTableDelete( ); } + // A description passed in the create-table-definition payload makes Storage write a KBC.description + // row under the "storage" provider at create time, i.e. before anything is loaded. It therefore says + // nothing about the table having been used and must not stop the drop of a failed empty table. This + // is intentionally outside the isTyped branch above - the non-typed create-table-definition payload + // (TableDefinitionFromColumns) can carry a description too. + $metadata = array_filter( + $metadata, + fn($m) => $m['key'] !== DescriptionHelper::DESCRIPTION_METADATA_KEY || $m['provider'] !== 'storage', + ); + if ($task->isUsingFreshlyCreatedTable() && // most important ($tableInfo['rowsCount'] === 0 || $tableInfo['rowsCount'] === null) && // seems both are possible 🙄 (count($metadata) === 0) // at this point there should be no metadata, they're set after load diff --git a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php index 14eb57013..724ecb9ce 100644 --- a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php +++ b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php @@ -79,14 +79,20 @@ public function waitForAll(): array $jobResult = $this->clientWrapper->getBranchClient()->waitForJob($jobId); if ($jobResult['status'] === 'error') { + $destinationTableName = $task->getDestinationTableName(); $errors[] = sprintf( 'Failed to load table "%s": %s', - $task->getDestinationTableName(), + $destinationTableName, $jobResult['error']['message'], ); + // The load failed, so there is no table to describe - it is about to be dropped below or it + // keeps the description it already had. Dropping the pending description here keeps it out of + // the "was not stored" warning, which is meant for descriptions lost by mistake. + unset($this->createdTableDescriptions[$destinationTableName]); + if (FailedLoadTableDecider::decideTableDelete($this->logger, $this->clientWrapper, $task)) { $this->clientWrapper->getTableAndFileStorageClient()->dropTable( - $task->getDestinationTableName(), + $destinationTableName, ['force' => true], ); } @@ -106,7 +112,6 @@ public function waitForAll(): array ); } - $tableData = null; switch ($jobResult['operationName']) { case 'tableImport': $tableData = $this->clientWrapper->getTableAndFileStorageClient()->getTable( @@ -125,18 +130,28 @@ public function waitForAll(): array $jobResult['results']['id'], ); $this->tableResult->addTable(new TableInfo($tableData)); + // Only a table created by the load job itself (CreateAndLoadTableTask) can have a + // pending description - every other table is created through a table definition, + // which carries the description in its create payload. + try { + $this->applyCreatedTableDescriptions($task, $tableData); + } catch (InvalidOutputException $e) { + $errors[] = $e->getMessage(); + } $jobResults[] = $jobResult; break; } - - try { - $this->applyCreatedTableDescriptions($task, $tableData); - } catch (InvalidOutputException $e) { - $errors[] = $e->getMessage(); - } } } + if ($this->createdTableDescriptions !== []) { + // Must never happen - a description left here would be silently thrown away. + $this->logger->warning(sprintf( + 'Description of table(s) "%s" was not stored.', + implode('", "', array_keys($this->createdTableDescriptions)), + )); + } + $this->tableResult->setMetrics(new Metrics($jobResults)); if ($errors) { @@ -146,9 +161,9 @@ public function waitForAll(): array } /** - * @param array|null $tableData table detail as returned by Storage after a successful load + * @param array $tableData table detail as returned by Storage after a successful load */ - private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?array $tableData): void + private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, array $tableData): void { if ($this->createdTableDescriptions === []) { return; @@ -156,7 +171,7 @@ private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, ?ar $tableId = $task->getDestinationTableName(); $descriptions = $this->createdTableDescriptions[$tableId] ?? null; - if ($descriptions === null || $tableData === null) { + if ($descriptions === null) { return; } diff --git a/libs/output-mapping/src/LoadTableTaskCreator.php b/libs/output-mapping/src/LoadTableTaskCreator.php index 41baaedc6..2e4aa2ca7 100644 --- a/libs/output-mapping/src/LoadTableTaskCreator.php +++ b/libs/output-mapping/src/LoadTableTaskCreator.php @@ -4,7 +4,6 @@ namespace Keboola\OutputMapping; -use Keboola\OutputMapping\DeferredTasks\LoadTableTaskInterface; use Keboola\OutputMapping\DeferredTasks\TableWriter\CreateAndLoadTableTask; use Keboola\OutputMapping\DeferredTasks\TableWriter\LoadTableTask; use Keboola\OutputMapping\Mapping\MappingFromConfigurationSchemaColumn; @@ -12,6 +11,7 @@ use Keboola\OutputMapping\Mapping\MappingStorageSources; use Keboola\OutputMapping\Storage\NativeTypeDecisionHelper; use Keboola\OutputMapping\Storage\TableCreator; +use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Writer\Table\StrategyInterface; use Keboola\OutputMapping\Writer\Table\TableDefinition\TableDefinitionFactory; use Keboola\OutputMapping\Writer\Table\TableDefinition\TableDefinitionFromColumns; @@ -25,15 +25,22 @@ class LoadTableTaskCreator public function __construct(readonly ClientWrapper $clientWrapper, readonly LoggerInterface $logger) { - $this->tableCreator = new TableCreator($clientWrapper); + $this->tableCreator = new TableCreator($clientWrapper, $logger); } + /** + * @param TableDescription|null $descriptions descriptions produced by output mapping for the destination + * table; they are embedded in the create-table-definition payload whenever the table is created here. + * Descriptions of a table which existed before are handled by StoragePreparer, where the + * `isDescriptionSystemManaged` flag of the table is known. + */ public function create( StrategyInterface $strategy, MappingFromProcessedConfiguration $source, MappingStorageSources $storageSources, OutputMappingSettings $settings, - ): LoadTableTaskInterface { + ?TableDescription $descriptions = null, + ): LoadTableTaskResult { $loadOptions = $this->buildLoadOptions( $source, $strategy, @@ -66,8 +73,12 @@ public function create( $source->getPrimaryKey(), $source->getColumnMetadata(), ); - $this->tableCreator->createTableDefinition($source->getDestination()->getBucketId(), $tableDefinition); - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, true); + $this->tableCreator->createTableDefinition( + $source->getDestination()->getBucketId(), + $tableDefinition, + $descriptions, + ); + return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif ($settings->hasNewNativeTypesFeature() && !$storageSources->didTableExistBefore() && $source->getSchema() @@ -77,8 +88,12 @@ public function create( $source->getSchema(), $storageSources->getBucket()->backend, ); - $this->tableCreator->createTableDefinition($source->getDestination()->getBucketId(), $tableDefinition); - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, true); + $this->tableCreator->createTableDefinition( + $source->getDestination()->getBucketId(), + $tableDefinition, + $descriptions, + ); + return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif (!$storageSources->didTableExistBefore() && $source->hasColumns()) { // tabulka neexistuje a známe sloupce z manifestu - vytváříme ji přes table definition bez typů $tableDefinition = new TableDefinitionFromColumns( @@ -86,11 +101,15 @@ public function create( $source->getColumns(), $source->getPrimaryKey(), ); - $this->tableCreator->createTableDefinition($source->getDestination()->getBucketId(), $tableDefinition); - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, true); + $this->tableCreator->createTableDefinition( + $source->getDestination()->getBucketId(), + $tableDefinition, + $descriptions, + ); + return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif ($storageSources->didTableExistBefore()) { // tabulka existuje takže nahráváme data - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, false); + return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, false)); } else { // tabulka nemá manifest a tím nemá známé columns if ($settings->getTreatValuesAsNull() !== null) { @@ -100,9 +119,13 @@ public function create( $source->getDestination()->getTableName(), )); } - $loadTask = new CreateAndLoadTableTask($source->getDestination(), $loadOptions, true); + // The table is created by the load job itself (Client::queueTableCreate), there is no create + // payload the descriptions could be embedded into - they have to be stored after the load. + return new LoadTableTaskResult( + new CreateAndLoadTableTask($source->getDestination(), $loadOptions, true), + $descriptions !== null && !$descriptions->isEmpty() ? $descriptions : null, + ); } - return $loadTask; } public function buildLoadOptions( diff --git a/libs/output-mapping/src/LoadTableTaskResult.php b/libs/output-mapping/src/LoadTableTaskResult.php new file mode 100644 index 000000000..6bea31d61 --- /dev/null +++ b/libs/output-mapping/src/LoadTableTaskResult.php @@ -0,0 +1,37 @@ +loadTableTask; + } + + public function getDescriptionsNotEmbeddedInCreatePayload(): ?TableDescription + { + return $this->descriptionsNotEmbeddedInCreatePayload; + } +} diff --git a/libs/output-mapping/src/Storage/TableCreator.php b/libs/output-mapping/src/Storage/TableCreator.php index a5b03e783..05727cc82 100644 --- a/libs/output-mapping/src/Storage/TableCreator.php +++ b/libs/output-mapping/src/Storage/TableCreator.php @@ -5,25 +5,40 @@ namespace Keboola\OutputMapping\Storage; use Keboola\OutputMapping\Exception\InvalidOutputException; +use Keboola\OutputMapping\Writer\Table\TableDefinition\CreateTableDefinitionDescriptionEnricher; use Keboola\OutputMapping\Writer\Table\TableDefinitionInterface; use Keboola\StorageApi\ClientException; use Keboola\StorageApiBranch\ClientWrapper; +use Psr\Log\LoggerInterface; class TableCreator { public function __construct( private readonly ClientWrapper $clientWrapper, + private readonly LoggerInterface $logger, ) { } + /** + * The descriptions are part of the create payload, so a table created here already carries them and no + * table-definition update job is needed after the load. The table is brand new, therefore its description + * is always system-managed (Storage default) and there is nothing to diff against. + */ public function createTableDefinition( string $bucketId, TableDefinitionInterface $tableDefinition, + ?TableDescription $descriptions = null, ): string { + $requestData = $tableDefinition->getRequestData(); + if ($descriptions !== null) { + $requestData = (new CreateTableDefinitionDescriptionEnricher($this->logger)) + ->enrich($requestData, $descriptions); + } + try { return $this->clientWrapper->getTableAndFileStorageClient()->createTableDefinition( $bucketId, - $tableDefinition->getRequestData(), + $requestData, ); } catch (ClientException $e) { throw new InvalidOutputException( diff --git a/libs/output-mapping/src/TableLoader.php b/libs/output-mapping/src/TableLoader.php index 63109ca22..33cf7fec0 100644 --- a/libs/output-mapping/src/TableLoader.php +++ b/libs/output-mapping/src/TableLoader.php @@ -141,30 +141,30 @@ public function uploadTables( ); $loadTableTaskCreator = new LoadTableTaskCreator($this->clientWrapper, $this->logger); - $loadTableTask = $loadTableTaskCreator->create( + $loadTableTaskResult = $loadTableTaskCreator->create( $strategy, $processedSource, $storageSources, $configuration, + TableDescription::createFromMapping($processedSource), ); $metadataSetter = new MetadataSetter(); $loadTableTask = $metadataSetter->setTableMetadata( - $loadTableTask, + $loadTableTaskResult->getLoadTableTask(), $processedSource, $storageSources, $systemMetadata, ); - if (!$storageSources->didTableExistBefore()) { - // The table is created by this run - either by the table definition created above or by the - // load job itself. Its description is always system-managed (Storage default), so it is stored - // as soon as the load finishes and the table surely exists. Descriptions of tables which - // already existed are handled by StoragePreparer, where the system-managed flag is known. - $descriptions = TableDescription::createFromMapping($processedSource); - if (!$descriptions->isEmpty()) { - $createdTableDescriptions[$descriptions->getTableId()] = $descriptions; - } + // Descriptions of a table created through a table definition are already part of the create + // payload and descriptions of a table which existed before were applied by StoragePreparer. What + // is left here is the table created by the load job itself, which has no create payload - such a + // description can only be stored once the load finishes and the table surely exists. + $descriptionsToStoreAfterLoad = $loadTableTaskResult->getDescriptionsNotEmbeddedInCreatePayload(); + if ($descriptionsToStoreAfterLoad !== null) { + $createdTableDescriptions[$descriptionsToStoreAfterLoad->getTableId()] = + $descriptionsToStoreAfterLoad; } $loadTableTasks[] = $loadTableTask; diff --git a/libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php b/libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php new file mode 100644 index 000000000..b66d26e33 --- /dev/null +++ b/libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php @@ -0,0 +1,84 @@ + $requestData payload as returned by TableDefinitionInterface::getRequestData() + * @return array + */ + public function enrich(array $requestData, TableDescription $descriptions): array + { + if ($descriptions->isEmpty()) { + return $requestData; + } + + $tableDescription = $descriptions->getTableDescription(); + if ($tableDescription !== null) { + $requestData['description'] = $tableDescription; + } + + $columnDescriptions = $descriptions->getColumnDescriptions(); + if ($columnDescriptions === []) { + return $requestData; + } + + $columns = $requestData['columns'] ?? []; + if (!is_array($columns)) { + return $requestData; + } + + $enrichedColumns = []; + $knownColumns = []; + foreach ($columns as $index => $column) { + $columnName = is_array($column) ? ($column['name'] ?? null) : null; + if (is_string($columnName) && isset($columnDescriptions[$columnName])) { + $knownColumns[] = $columnName; + + $definition = $column['definition'] ?? []; + if (!is_array($definition)) { + $definition = []; + } + $definition['description'] = $columnDescriptions[$columnName]; + $column['definition'] = $definition; + } + $enrichedColumns[$index] = $column; + } + + // A description of a column which is not part of the payload must never append a new column entry - + // that would create a column the data does not have. + $missingColumns = array_diff(array_keys($columnDescriptions), $knownColumns); + if ($missingColumns) { + $this->logger->warning(sprintf( + 'Cannot store description of column(s) "%s" of table "%s", the column(s) do not exist.', + implode('", "', $missingColumns), + $descriptions->getTableId(), + )); + } + + $requestData['columns'] = $enrichedColumns; + + return $requestData; + } +} diff --git a/libs/output-mapping/tests/DeferredTasks/FailedLoadTableDeciderTest.php b/libs/output-mapping/tests/DeferredTasks/FailedLoadTableDeciderTest.php index 9ad1fd665..9a5937ebd 100644 --- a/libs/output-mapping/tests/DeferredTasks/FailedLoadTableDeciderTest.php +++ b/libs/output-mapping/tests/DeferredTasks/FailedLoadTableDeciderTest.php @@ -49,11 +49,69 @@ public function decideProvider(): Generator 'tableInfo' => [ 'rowsCount' => 0, 'isTyped' => false, - 'metadata' => ['a' => 'b'], + 'metadata' => [ + [ + 'key' => 'KBC.createdBy.component.id', + 'value' => 'keboola.ex-db-snowflake', + 'provider' => 'system', + ], + ], + ], + 'freshlyCreated' => true, + 'expectedResult' => false, + ]; + // A description sent in the create-table-definition payload is mirrored by Storage into a + // KBC.description metadata row under the "storage" provider at create time, before any load. + yield 'non-typed table fresh and storage KBC.description metadata' => [ + 'tableInfo' => [ + 'rowsCount' => 0, + 'isTyped' => false, + 'metadata' => [ + [ + 'key' => 'KBC.description', + 'value' => 'table description', + 'provider' => 'storage', + ], + ], + ], + 'freshlyCreated' => true, + 'expectedResult' => true, + ]; + yield 'non-typed table fresh and component KBC.description metadata' => [ + 'tableInfo' => [ + 'rowsCount' => 0, + 'isTyped' => false, + 'metadata' => [ + [ + 'key' => 'KBC.description', + 'value' => 'table description', + 'provider' => 'keboola.ex-db-snowflake', + ], + ], ], 'freshlyCreated' => true, 'expectedResult' => false, ]; + yield 'typed table fresh with storage KBC.dataTypesEnabled and KBC.description metadata' => [ + 'tableInfo' => [ + 'rowsCount' => 0, + 'isTyped' => true, + 'metadata' => [ + [ + 'key' => 'KBC.dataTypesEnabled', + 'value' => true, + 'provider' => 'storage', + ], + [ + 'key' => 'KBC.description', + 'value' => 'table description', + 'provider' => 'storage', + ], + ], + ], + 'freshlyCreated' => true, + 'expectedResult' => true, + ]; yield 'typed table fresh and storage KBC.dataTypesEnabled metadata' => [ 'tableInfo' => [ 'rowsCount' => 0, diff --git a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php index 619befa53..d387331af 100644 --- a/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php +++ b/libs/output-mapping/tests/DeferredTasks/LoadTableQueueTest.php @@ -18,6 +18,8 @@ use Keboola\StorageApi\ClientException; use Keboola\StorageApi\Metadata; use Keboola\StorageApiBranch\ClientWrapper; +use Monolog\Handler\TestHandler; +use Monolog\Logger; use PHPUnit\Framework\TestCase; use Psr\Log\NullLogger; @@ -645,6 +647,11 @@ public function testLoadCustomVariablesSetsVariablesFromValidJson(): void unlink($tmpFile); } + /** + * The deferred path is only reachable for a table created by the load job itself (CreateAndLoadTableTask), + * whose job operation is `tableCreate`. Every other created table carries its description already in the + * create-table-definition payload. + */ public function testWaitForAllStoresDescriptionsOfCreatedTable(): void { $tableId = 'in.c-myBucket.tableCreated'; @@ -678,9 +685,12 @@ public function testWaitForAllStoresDescriptionsOfCreatedTable(): void ->method('waitForJob') ->with(123) ->willReturn([ - 'operationName' => 'tableImport', + 'operationName' => 'tableCreate', 'status' => 'success', - 'tableId' => $tableId, + 'tableId' => null, + 'results' => [ + 'id' => $tableId, + ], 'metrics' => [ 'inBytes' => 0, 'inBytesUncompressed' => 0, @@ -743,9 +753,12 @@ public function testWaitForAllReportsFailedDescriptionUpdateAsError(): void ->method('waitForJob') ->with(123) ->willReturn([ - 'operationName' => 'tableImport', + 'operationName' => 'tableCreate', 'status' => 'success', - 'tableId' => $tableId, + 'tableId' => null, + 'results' => [ + 'id' => $tableId, + ], 'metrics' => [ 'inBytes' => 0, 'inBytesUncompressed' => 0, @@ -785,6 +798,215 @@ public function testWaitForAllReportsFailedDescriptionUpdateAsError(): void } } + /** + * A leftover description means it was never handed over to Storage. It cannot happen today, but it must + * never be dropped silently. + */ + public function testWaitForAllWarnsAboutDescriptionsWhichWereNotStored(): void + { + $tableId = 'in.c-myBucket.tableImported'; + + $clientMock = $this->createMock(Client::class); + $clientMock->expects(self::once()) + ->method('getTable') + ->with($tableId) + ->willReturn([ + 'id' => $tableId, + 'displayName' => 'my-name', + 'name' => 'my-name', + 'columns' => ['col1'], + 'lastImportDate' => null, + 'lastChangeDate' => null, + ]) + ; + $clientMock->expects(self::never()) + ->method('updateTableDefinition') + ; + + $branchClientMock = $this->createMock(BranchAwareClient::class); + $branchClientMock->expects(self::once()) + ->method('waitForJob') + ->with(123) + ->willReturn([ + 'operationName' => 'tableImport', + 'status' => 'success', + 'tableId' => $tableId, + 'metrics' => [ + 'inBytes' => 0, + 'inBytesUncompressed' => 0, + ], + ]) + ; + + $loadTask = $this->createMock(LoadTableTask::class); + $loadTask->expects(self::once())->method('getStorageJobId')->willReturn('123'); + $loadTask->expects(self::once())->method('applyMetadata'); + + $clientWrapperMock = $this->createMock(ClientWrapper::class); + $clientWrapperMock->method('getTableAndFileStorageClient') + ->willReturn($clientMock); + $clientWrapperMock->method('getBranchClient') + ->willReturn($branchClientMock); + + $logHandler = new TestHandler(); + + $loadQueue = new LoadTableQueue( + $clientWrapperMock, + new Logger('test', [$logHandler]), + [$loadTask], + ['in.c-myBucket.leftover' => new TableDescription('in.c-myBucket.leftover', 'table desc', [])], + ); + $loadQueue->waitForAll(); + + self::assertTrue($logHandler->hasWarningThatContains( + 'Description of table(s) "in.c-myBucket.leftover" was not stored.', + )); + } + + /** + * A failed load leaves nothing to describe, so the pending description is expected to disappear without + * the "was not stored" warning - that warning is about descriptions lost by mistake. + */ + public function testWaitForAllDoesNotWarnAboutDescriptionsOfFailedLoad(): void + { + $tableId = 'in.c-myBucket.tableFailed'; + + $branchClientMock = $this->createMock(BranchAwareClient::class); + $branchClientMock->expects(self::once()) + ->method('waitForJob') + ->with(123) + ->willReturn(['status' => 'error', 'error' => ['message' => 'Hi']]) + ; + + $clientMock = $this->createMock(Client::class); + $clientMock->expects(self::once()) + ->method('getTable') + ->with($tableId) + ->willReturn(['rowsCount' => 0, 'metadata' => [], 'isTyped' => false]) + ; + $clientMock->expects(self::once()) + ->method('dropTable') + ->with($tableId, ['force' => true]) + ; + $clientMock->expects(self::never()) + ->method('updateTableDefinition') + ; + + $loadTask = $this->createMock(LoadTableTask::class); + $loadTask->expects(self::once())->method('getStorageJobId')->willReturn('123'); + $loadTask->expects(self::once())->method('isUsingFreshlyCreatedTable')->willReturn(true); + $loadTask->method('getDestinationTableName')->willReturn($tableId); + + $clientWrapperMock = $this->createMock(ClientWrapper::class); + $clientWrapperMock->method('getTableAndFileStorageClient') + ->willReturn($clientMock); + $clientWrapperMock->method('getBranchClient') + ->willReturn($branchClientMock); + + $logHandler = new TestHandler(); + + $loadQueue = new LoadTableQueue( + $clientWrapperMock, + new Logger('test', [$logHandler]), + [$loadTask], + [$tableId => new TableDescription($tableId, 'table desc', ['col1' => 'col1 desc'])], + ); + + try { + $loadQueue->waitForAll(); + self::fail('WaitForAll should fail with InvalidOutputException.'); + } catch (InvalidOutputException $e) { + self::assertSame(sprintf('Failed to load table "%s": Hi', $tableId), $e->getMessage()); + } + + self::assertFalse($logHandler->hasWarningThatContains('was not stored')); + } + + /** + * Several sources may be mapped to the same destination table. The descriptions belong to the table, not + * to the mapping, so they are stored exactly once and nothing is left over afterwards. + */ + public function testWaitForAllStoresDescriptionOnceForTwoTasksWithSameDestination(): void + { + $tableId = 'in.c-myBucket.tableCreated'; + $tableData = [ + 'id' => $tableId, + 'displayName' => 'my-name', + 'name' => 'my-name', + 'columns' => ['col1'], + 'lastImportDate' => null, + 'lastChangeDate' => null, + ]; + + $clientMock = $this->createMock(Client::class); + $clientMock->expects(self::exactly(2)) + ->method('getTable') + ->with($tableId) + ->willReturn($tableData) + ; + $clientMock->expects(self::once()) + ->method('updateTableDefinition') + ->with($tableId, [ + 'description' => 'table desc', + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ], + ]) + ->willReturn([]) + ; + + $jobResults = [ + [ + 'operationName' => 'tableCreate', + 'status' => 'success', + 'tableId' => null, + 'results' => ['id' => $tableId], + 'metrics' => ['inBytes' => 0, 'inBytesUncompressed' => 0], + ], + [ + 'operationName' => 'tableImport', + 'status' => 'success', + 'tableId' => $tableId, + 'metrics' => ['inBytes' => 0, 'inBytesUncompressed' => 0], + ], + ]; + + $branchClientMock = $this->createMock(BranchAwareClient::class); + $branchClientMock->expects(self::exactly(count($jobResults))) + ->method('waitForJob') + ->willReturnCallback(function () use (&$jobResults) { + return array_shift($jobResults); + }) + ; + + $loadTasks = []; + foreach (['123', '456'] as $jobId) { + $loadTask = $this->createMock(LoadTableTask::class); + $loadTask->expects(self::once())->method('getStorageJobId')->willReturn($jobId); + $loadTask->expects(self::once())->method('applyMetadata'); + $loadTask->method('getDestinationTableName')->willReturn($tableId); + $loadTasks[] = $loadTask; + } + + $clientWrapperMock = $this->createMock(ClientWrapper::class); + $clientWrapperMock->method('getTableAndFileStorageClient') + ->willReturn($clientMock); + $clientWrapperMock->method('getBranchClient') + ->willReturn($branchClientMock); + + $logHandler = new TestHandler(); + + $loadQueue = new LoadTableQueue( + $clientWrapperMock, + new Logger('test', [$logHandler]), + $loadTasks, + [$tableId => new TableDescription($tableId, 'table desc', ['col1' => 'col1 desc'])], + ); + $loadQueue->waitForAll(); + + self::assertFalse($logHandler->hasWarningThatContains('was not stored')); + } + public function testLoadCustomVariablesDoesNothingWhenFileMissing(): void { $clientWrapperMock = $this->createMock(ClientWrapper::class); diff --git a/libs/output-mapping/tests/LoadTableTaskCreatorTest.php b/libs/output-mapping/tests/LoadTableTaskCreatorTest.php index 35a9adfd5..32ea55a09 100644 --- a/libs/output-mapping/tests/LoadTableTaskCreatorTest.php +++ b/libs/output-mapping/tests/LoadTableTaskCreatorTest.php @@ -14,6 +14,7 @@ use Keboola\OutputMapping\Mapping\MappingFromRawConfigurationAndPhysicalDataWithManifest; use Keboola\OutputMapping\Mapping\MappingStorageSources; use Keboola\OutputMapping\Storage\BucketInfo; +use Keboola\OutputMapping\Storage\TableDescription; use Keboola\OutputMapping\Tests\AbstractTestCase; use Keboola\OutputMapping\Tests\Needs\NeedsEmptyOutputBucket; use Keboola\OutputMapping\Tests\Needs\NeedsTestTables; @@ -72,13 +73,22 @@ public function testNativeTypeLoadTaskTableNotExists(): void $this->clientWrapper, $this->testLogger, ); - $loadTask = $loadTableTaskCreator->create( + $loadTaskResult = $loadTableTaskCreator->create( strategy: $strategy, source: $source, storageSources: $storageSources, settings: $settings, + descriptions: new TableDescription( + $this->emptyOutputBucketId . '.destinationTable', + 'table desc', + ['col1' => 'col1 desc'], + ), ); + // the descriptions are part of the create payload, nothing is left for after the load + self::assertNull($loadTaskResult->getDescriptionsNotEmbeddedInCreatePayload()); + + $loadTask = $loadTaskResult->getLoadTableTask(); self::assertInstanceOf(LoadTableTask::class, $loadTask); self::assertTrue($loadTask->isUsingFreshlyCreatedTable()); self::assertSame( @@ -90,6 +100,11 @@ public function testNativeTypeLoadTaskTableNotExists(): void ); self::assertTrue($storageTable['isTyped']); + self::assertSame('table desc', $storageTable['definition']['description'] ?? null); + self::assertSame( + ['col1' => 'col1 desc', 'col2' => null], + $this->getColumnDescriptions($storageTable['definition']['columns']), + ); self::assertSame( [ [ @@ -113,7 +128,7 @@ public function testNativeTypeLoadTaskTableNotExists(): void 'canBeFiltered' => true, ], ], - $storageTable['definition']['columns'], + $this->stripColumnDescriptions($storageTable['definition']['columns']), ); } @@ -168,13 +183,22 @@ public function testNewNativeTypeLoadTaskTableNotExists(): void $this->clientWrapper, $this->testLogger, ); - $loadTask = $loadTableTaskCreator->create( + $loadTaskResult = $loadTableTaskCreator->create( strategy: $strategy, source: $source, storageSources: $storageSources, settings: $settings, + descriptions: new TableDescription( + $this->emptyOutputBucketId . '.destinationTable', + 'table desc', + ['col2' => 'col2 desc'], + ), ); + // the descriptions are part of the create payload, nothing is left for after the load + self::assertNull($loadTaskResult->getDescriptionsNotEmbeddedInCreatePayload()); + + $loadTask = $loadTaskResult->getLoadTableTask(); self::assertInstanceOf(LoadTableTask::class, $loadTask); self::assertTrue($loadTask->isUsingFreshlyCreatedTable()); self::assertSame( @@ -186,6 +210,11 @@ public function testNewNativeTypeLoadTaskTableNotExists(): void ); self::assertTrue($storageTable['isTyped']); + self::assertSame('table desc', $storageTable['definition']['description'] ?? null); + self::assertSame( + ['col1' => null, 'col2' => 'col2 desc'], + $this->getColumnDescriptions($storageTable['definition']['columns']), + ); self::assertSame( [ [ @@ -209,7 +238,7 @@ public function testNewNativeTypeLoadTaskTableNotExists(): void 'canBeFiltered' => true, ], ], - $storageTable['definition']['columns'], + $this->stripColumnDescriptions($storageTable['definition']['columns']), ); self::assertSame(['col1'], $storageTable['definition']['primaryKeysNames']); } @@ -242,13 +271,22 @@ public function testLoadTaskTableNotExists(): void $this->clientWrapper, $this->testLogger, ); - $loadTask = $loadTableTaskCreator->create( + $loadTaskResult = $loadTableTaskCreator->create( strategy: $strategy, source: $source, storageSources: $storageSources, settings: $settings, + descriptions: new TableDescription( + $this->emptyOutputBucketId . '.destinationTable', + 'table desc', + ['col1' => 'col1 desc'], + ), ); + // the descriptions are part of the create payload, nothing is left for after the load + self::assertNull($loadTaskResult->getDescriptionsNotEmbeddedInCreatePayload()); + + $loadTask = $loadTaskResult->getLoadTableTask(); self::assertInstanceOf(LoadTableTask::class, $loadTask); self::assertTrue($loadTask->isUsingFreshlyCreatedTable()); self::assertSame( @@ -259,11 +297,17 @@ public function testLoadTaskTableNotExists(): void $this->emptyOutputBucketId.'.destinationTable', ); + // a column definition carrying only a description does not make the table typed self::assertFalse($storageTable['isTyped']); self::assertSame( ['col1', 'col2'], $storageTable['columns'], ); + self::assertSame('table desc', $storageTable['definition']['description'] ?? null); + self::assertSame( + ['col1' => 'col1 desc', 'col2' => null], + $this->getColumnDescriptions($storageTable['definition']['columns']), + ); } #[NeedsTestTables(count: 1)] @@ -292,13 +336,18 @@ public function testLoadTaskTableExists(): void $this->clientWrapper, $this->testLogger, ); - $loadTask = $loadTableTaskCreator->create( + $loadTaskResult = $loadTableTaskCreator->create( strategy: $strategy, source: $source, storageSources: $storageSources, settings: $settings, + descriptions: new TableDescription($this->testBucketId . '.test0', 'table desc', []), ); + // the table existed before, its description was already handled by StoragePreparer + self::assertNull($loadTaskResult->getDescriptionsNotEmbeddedInCreatePayload()); + + $loadTask = $loadTaskResult->getLoadTableTask(); self::assertInstanceOf(LoadTableTask::class, $loadTask); self::assertFalse($loadTask->isUsingFreshlyCreatedTable()); self::assertSame( @@ -333,13 +382,23 @@ public function testLoadTaskTableNotExistsManifestNotExists(): void $this->clientWrapper, $this->testLogger, ); - $loadTask = $loadTableTaskCreator->create( + $descriptions = new TableDescription( + $this->emptyOutputBucketId . '.test0', + 'table desc', + ['col1' => 'col1 desc'], + ); + $loadTaskResult = $loadTableTaskCreator->create( strategy: $strategy, source: $source, storageSources: $storageSources, settings: $settings, + descriptions: $descriptions, ); + // the table is created by the load job itself, there is no create payload to embed the descriptions in + self::assertSame($descriptions, $loadTaskResult->getDescriptionsNotEmbeddedInCreatePayload()); + + $loadTask = $loadTaskResult->getLoadTableTask(); self::assertInstanceOf(CreateAndLoadTableTask::class, $loadTask); self::assertTrue($loadTask->isUsingFreshlyCreatedTable()); self::assertSame( @@ -348,6 +407,91 @@ public function testLoadTaskTableNotExistsManifestNotExists(): void ); } + #[NeedsEmptyOutputBucket] + public function testLoadTaskWithoutManifestAndWithoutDescription(): void + { + $settings = self::createMock(OutputMappingSettings::class); + $settings->expects(self::once())->method('hasNativeTypesFeature')->willReturn(false); + $settings->expects(self::exactly(2))->method('hasNewNativeTypesFeature')->willReturn(false); + + $strategy = self::createMock(LocalTableStrategy::class); + $strategy->expects(self::once()) + ->method('prepareLoadTaskOptions') + ->willReturn([]); + + $source = self::createMock(MappingFromProcessedConfiguration::class); + $source->expects(self::once())->method('hasColumns')->willReturn(false); + $source->expects(self::once())->method('getDestination')->willReturn( + new MappingDestination($this->emptyOutputBucketId . '.test0'), + ); + $source->expects(self::once())->method('getPrimaryKey')->willReturn([]); + + $storageSources = self::createMock(MappingStorageSources::class); + $storageSources->expects(self::exactly(3))->method('didTableExistBefore')->willReturn(false); + + $loadTableTaskCreator = new LoadTableTaskCreator( + $this->clientWrapper, + $this->testLogger, + ); + $loadTaskResult = $loadTableTaskCreator->create( + strategy: $strategy, + source: $source, + storageSources: $storageSources, + settings: $settings, + descriptions: new TableDescription($this->emptyOutputBucketId . '.test0', null, []), + ); + + // there is no description at all, so there is nothing to store after the load either + self::assertNull($loadTaskResult->getDescriptionsNotEmbeddedInCreatePayload()); + self::assertInstanceOf(CreateAndLoadTableTask::class, $loadTaskResult->getLoadTableTask()); + } + + /** + * Storage returns the column description nested in the column definition, splitting it off keeps the + * assertions of the column types independent of the position of the description key. + * + * @param array $columns + * @return array column name => description + */ + private function getColumnDescriptions(array $columns): array + { + $descriptions = []; + foreach ($columns as $column) { + self::assertIsArray($column); + $columnName = $column['name']; + self::assertIsString($columnName); + + $definition = $column['definition'] ?? []; + self::assertIsArray($definition); + $description = $definition['description'] ?? null; + + $descriptions[$columnName] = is_string($description) ? $description : null; + } + + return $descriptions; + } + + /** + * @param array $columns + * @return list + */ + private function stripColumnDescriptions(array $columns): array + { + $strippedColumns = []; + foreach ($columns as $column) { + self::assertIsArray($column); + + $definition = $column['definition'] ?? null; + if (is_array($definition)) { + unset($definition['description']); + $column['definition'] = $definition; + } + $strippedColumns[] = $column; + } + + return $strippedColumns; + } + /** * @dataProvider buildLoadOptionsDataProvider */ diff --git a/libs/output-mapping/tests/Storage/TableCreatorTest.php b/libs/output-mapping/tests/Storage/TableCreatorTest.php index c3a8c1a96..93384d6d7 100644 --- a/libs/output-mapping/tests/Storage/TableCreatorTest.php +++ b/libs/output-mapping/tests/Storage/TableCreatorTest.php @@ -19,7 +19,7 @@ class TableCreatorTest extends AbstractTestCase #[NeedsEmptyOutputBucket] public function testCreateTableDefinition(): void { - $tableCreator = new TableCreator($this->clientWrapper); + $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); $tableDefinition = new TableDefinition( new TableDefinitionColumnFactory([], 'snowflake', true), @@ -60,7 +60,7 @@ public function testCreateTableDefinition(): void #[NeedsEmptyOutputBucket] public function testCreateTableDefinitionErrorHandling(): void { - $tableCreator = new TableCreator($this->clientWrapper); + $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); $tableDefinition = new TableDefinition( new TableDefinitionColumnFactory([], 'snowflake', true), @@ -94,7 +94,7 @@ public function testCreateTableDefinitionErrorHandling(): void #[NeedsEmptyOutputBucket] public function testCreateNonTypedTableFromColumns(): void { - $tableCreator = new TableCreator($this->clientWrapper); + $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); $tableId = $tableCreator->createTableDefinition( $this->emptyOutputBucketId, @@ -113,7 +113,7 @@ public function testCreateNonTypedTableFromColumns(): void #[NeedsEmptyOutputBucket] public function testCreateNonTypedTableFromColumnsErrorHandling(): void { - $tableCreator = new TableCreator($this->clientWrapper); + $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); try { $tableCreator->createTableDefinition( diff --git a/libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php b/libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php new file mode 100644 index 000000000..fbc065fa7 --- /dev/null +++ b/libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php @@ -0,0 +1,183 @@ +logHandler = new TestHandler(); + $this->logger = new Logger('test', [$this->logHandler]); + } + + public function enrichProvider(): Generator + { + yield 'table description only' => [ + 'requestData' => [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1']], + ], + 'descriptions' => new TableDescription(self::TABLE_ID, 'table desc', []), + 'expectedRequestData' => [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1']], + 'description' => 'table desc', + ], + ]; + + yield 'column description on a typed column keeps the type' => [ + 'requestData' => [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [ + [ + 'name' => 'col1', + 'definition' => ['type' => 'VARCHAR', 'nullable' => true], + 'basetype' => 'STRING', + ], + ['name' => 'col2', 'definition' => ['type' => 'NUMBER']], + ], + ], + 'descriptions' => new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc']), + 'expectedRequestData' => [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [ + [ + 'name' => 'col1', + 'definition' => [ + 'type' => 'VARCHAR', + 'nullable' => true, + 'description' => 'col1 desc', + ], + 'basetype' => 'STRING', + ], + ['name' => 'col2', 'definition' => ['type' => 'NUMBER']], + ], + ], + ]; + + yield 'column description on a column without definition' => [ + 'requestData' => [ + 'name' => 'table', + 'primaryKeysNames' => ['col1'], + 'columns' => [ + ['name' => 'col1'], + ['name' => 'col2'], + ], + ], + 'descriptions' => new TableDescription( + self::TABLE_ID, + 'table desc', + ['col1' => 'col1 desc', 'col2' => 'col2 desc'], + ), + 'expectedRequestData' => [ + 'name' => 'table', + 'primaryKeysNames' => ['col1'], + 'columns' => [ + // only a description, no type - the table stays non-typed + ['name' => 'col1', 'definition' => ['description' => 'col1 desc']], + ['name' => 'col2', 'definition' => ['description' => 'col2 desc']], + ], + 'description' => 'table desc', + ], + ]; + + yield 'empty descriptions leave the payload untouched' => [ + 'requestData' => [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1']], + ], + 'descriptions' => new TableDescription(self::TABLE_ID, null, []), + 'expectedRequestData' => [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1']], + ], + ]; + } + + /** @dataProvider enrichProvider */ + public function testEnrich( + array $requestData, + TableDescription $descriptions, + array $expectedRequestData, + ): void { + $enricher = new CreateTableDefinitionDescriptionEnricher($this->logger); + + self::assertSame($expectedRequestData, $enricher->enrich($requestData, $descriptions)); + self::assertFalse($this->logHandler->hasWarningRecords()); + } + + public function testDescriptionOfUnknownColumnIsSkippedAndLogged(): void + { + $enricher = new CreateTableDefinitionDescriptionEnricher($this->logger); + + $requestData = $enricher->enrich( + [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1']], + ], + new TableDescription( + self::TABLE_ID, + null, + ['col1' => 'col1 desc', 'unknown1' => 'unknown1 desc', 'unknown2' => 'unknown2 desc'], + ), + ); + + // a column which is not part of the payload must never be appended, it does not exist in the data + self::assertSame( + [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1', 'definition' => ['description' => 'col1 desc']]], + ], + $requestData, + ); + self::assertTrue($this->logHandler->hasWarningThatContains(sprintf( + 'Cannot store description of column(s) "unknown1", "unknown2" of table "%s", ' + . 'the column(s) do not exist.', + self::TABLE_ID, + ))); + } + + /** + * The payload of a non-typed table must stay a JSON array of objects, so the enrichment must not turn + * the column list into a JSON object. + */ + public function testEnrichedNonTypedPayloadKeepsColumnsAsJsonArray(): void + { + $enricher = new CreateTableDefinitionDescriptionEnricher($this->logger); + + $requestData = $enricher->enrich( + (new TableDefinitionFromColumns('table', ['Id', 'Name'], ['Id']))->getRequestData(), + new TableDescription(self::TABLE_ID, 'table desc', ['Name' => 'Name desc']), + ); + + self::assertSame( + '[{"name":"Id"},{"name":"Name","definition":{"description":"Name desc"}}]', + json_encode($requestData['columns']), + ); + } +} From 84bd8df4372463177cdaa11110dbacdad2985386 Mon Sep 17 00:00:00 2001 From: zajca Date: Wed, 29 Jul 2026 15:02:28 +0200 Subject: [PATCH 06/11] test(output-mapping): cover descriptions against real Storage A review of the description coverage found the create paths well covered but the failure path, the second run and BigQuery untested. Closing those: - a failed load of a freshly created table carrying a description in its create payload must still drop the table; a companion test pins the assumption the drop depends on - that Storage writes the mirrored KBC.description row under the "storage" provider - a table which existed before a failed load keeps its data and description - BigQuery, newly reachable since the backend gate was dropped, on both the create payload and the table-definition update - a second run over a table created by the load job, where the diff reads the stored value back from the table definition - typed tables in both native-types suites: update, user-managed table, and the description of a column added by the same run - descriptions coming from `table_metadata` / `metadata` KBC.description, not only from the dedicated `description` field - a description dropped from the configuration, or sent empty, keeps the stored value instead of clearing it - a description of a column the data does not have is reported and skipped - workspace staging testSaveTableAndColumnMetadata and testLoadTaskTableNotExists asserted the description only through the derived metadata rows, which can no longer tell the native field apart from a row written by output mapping itself; both now assert `definition.description` directly, and the OR-fallback helpers that hid the difference are gone. The main description test threads the test logger through so the user-visible messages are assertable, and pins the table as non-typed so the non-typed path cannot silently stop being exercised. --- .../Storage/BigqueryTableDescriptionTest.php | 146 +++++++ .../tests/Writer/TableDefinitionTest.php | 107 ++++++ .../tests/Writer/TableDefinitionV2Test.php | 184 +++++++++ .../Writer/TableDescriptionWriterTest.php | 361 +++++++++++++++++- .../Writer/Workspace/WriterWorkspaceTest.php | 58 +++ 5 files changed, 848 insertions(+), 8 deletions(-) create mode 100644 libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php diff --git a/libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php b/libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php new file mode 100644 index 000000000..7d751e257 --- /dev/null +++ b/libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php @@ -0,0 +1,146 @@ +emptyBigqueryOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + #[NeedsEmptyBigqueryOutputBucket] + public function testDescriptionIsUpdatedOnExistingTable(): void + { + $tableId = $this->emptyBigqueryOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, 'updated table description', 'updated Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetail['isDescriptionSystemManaged']); + self::assertSame('updated table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('updated Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + /** + * Storage rejects a table-definition patch without any effective change with 400, so an unchanged repeated + * run must not send one. + */ + #[NeedsEmptyBigqueryOutputBucket] + public function testRepeatedRunWithUnchangedDescriptionSucceeds(): void + { + $tableId = $this->emptyBigqueryOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, 'table description', 'Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + private function uploadTable(string $tableId, string $tableDescription, string $columnDescription): void + { + $root = $this->temp->getTmpFolder(); + file_put_contents($root . '/upload/tableDescription.csv', "\"1\",\"bob\"\n\"2\",\"alice\"\n"); + + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( + configuration: new OutputMappingSettings( + configuration: [ + 'mapping' => [ + [ + 'source' => 'tableDescription.csv', + 'destination' => $tableId, + 'description' => $tableDescription, + 'schema' => [ + [ + 'name' => 'Id', + 'data_type' => ['base' => ['type' => 'STRING']], + 'description' => $columnDescription, + ], + [ + 'name' => 'Name', + 'data_type' => ['base' => ['type' => 'STRING']], + ], + ], + ], + ], + ], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_AUTHORITATIVE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + self::assertCount(1, $tableQueue->waitForAll()); + } + + private function getColumnDescription(array $tableDetail, string $columnName): ?string + { + $definition = $tableDetail['definition'] ?? []; + self::assertIsArray($definition); + $columns = $definition['columns'] ?? []; + self::assertIsArray($columns); + + foreach ($columns as $column) { + self::assertIsArray($column); + if ($column['name'] === $columnName) { + $columnDefinition = $column['definition'] ?? []; + self::assertIsArray($columnDefinition); + + return $columnDefinition['description'] ?? null; + } + } + + return null; + } + + protected function initClient(?string $branchId = null): void + { + $clientOptions = (new ClientOptions()) + ->setUrl((string) getenv('BIGQUERY_STORAGE_API_URL')) + ->setToken((string) getenv('BIGQUERY_STORAGE_API_TOKEN')) + ->setAuthType(AuthType::STORAGE_TOKEN) + ->setBranchId($branchId) + ->setBackoffMaxTries(1) + ->setJobPollRetryDelay(function () { + return 1; + }) + ->setUserAgent(implode('::', Test::describe($this))); + $this->clientWrapper = new ClientWrapper($clientOptions); + $tokenInfo = $this->clientWrapper->getBranchClient()->verifyToken(); + print(sprintf( + 'Authorized as "%s (%s)" to project "%s (%s)" at "%s" stack.', + $tokenInfo['description'], + $tokenInfo['id'], + $tokenInfo['owner']['name'], + $tokenInfo['owner']['id'], + $this->clientWrapper->getBranchClient()->getApiUrl(), + )); + } +} diff --git a/libs/output-mapping/tests/Writer/TableDefinitionTest.php b/libs/output-mapping/tests/Writer/TableDefinitionTest.php index aefa55b67..2e1327059 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionTest.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionTest.php @@ -688,4 +688,111 @@ private static function assertTablePrimaryKeyAddJob(array $jobData, array $expec self::assertSame('success', $jobData['status']); self::assertSame($expectedPk, $jobData['operationParams']['columns']); } + + /** + * The legacy native-types path builds the table definition from `column_metadata`, and the descriptions + * ride along in the create payload. A following run finds the table there and goes through the + * table-definition update instead. + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionIsStoredAndUpdatedOnTypedTable(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDefinition'; + + $this->uploadTableWithColumnMetadata($tableId, 'table description', 'Id description'); + + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetails['isTyped']); + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + ['Id' => 'Id description', 'Name' => null], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + + $this->uploadTableWithColumnMetadata($tableId, 'updated table description', 'updated Id description'); + + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetails['isDescriptionSystemManaged']); + self::assertSame('updated table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + ['Id' => 'updated Id description', 'Name' => null], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + + // Storage rejects a patch without any effective change with 400, so an unchanged run must not send one + $this->uploadTableWithColumnMetadata($tableId, 'updated table description', 'updated Id description'); + } + + private function uploadTableWithColumnMetadata( + string $tableId, + string $tableDescription, + string $idDescription, + ): void { + $config = [ + 'source' => 'tableDefinition.csv', + 'destination' => $tableId, + 'columns' => ['Id', 'Name'], + 'description' => $tableDescription, + 'metadata' => [ + [ + 'key' => 'KBC.datatype.backend', + 'value' => 'snowflake', + ], + ], + 'column_metadata' => [ + 'Id' => [ + ['key' => 'KBC.datatype.type', 'value' => Snowflake::TYPE_INTEGER], + ['key' => 'KBC.datatype.basetype', 'value' => 'INTEGER'], + ['key' => 'KBC.description', 'value' => $idDescription], + ], + 'Name' => [ + ['key' => 'KBC.datatype.type', 'value' => Snowflake::TYPE_TEXT], + ['key' => 'KBC.datatype.basetype', 'value' => 'STRING'], + ], + ], + ]; + + file_put_contents( + $this->temp->getTmpFolder() . '/upload/tableDefinition.csv', + "\"1\",\"bob\"\n\"2\",\"alice\"\n", + ); + + $tableQueue = $this->getTableLoader( + logger: $this->testLogger, + strategyFactory: $this->getLocalStagingFactory(logger: $this->testLogger), + )->uploadTables( + configuration: new OutputMappingSettings( + configuration: ['mapping' => [$config]], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_AUTHORITATIVE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + self::assertCount(1, $tableQueue->waitForAll()); + } + + /** + * @param array $columns + * @return array column name => description + */ + private function getColumnDescriptions(array $columns): array + { + $descriptions = []; + foreach ($columns as $column) { + self::assertIsArray($column); + $columnName = $column['name']; + self::assertIsString($columnName); + + $definition = $column['definition'] ?? []; + self::assertIsArray($definition); + $description = $definition['description'] ?? null; + + $descriptions[$columnName] = is_string($description) ? $description : null; + } + + return $descriptions; + } } diff --git a/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php b/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php index 1399843fb..b8aabf260 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php @@ -752,6 +752,190 @@ public function testSaveTableAndColumnMetadata(): void ], $this->getMetadataValues($filteredColumnFooMetadata), ); + + // The metadata rows asserted above are Storage's mirror of the native description field, which is + // where output mapping actually stores it - assert the source, not only the mirror. + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + [ + 'Id' => null, + 'Name' => 'name description', + 'foo' => 'foo description', + ], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + self::assertSame('storage', array_values($filteredTableMetadata)[0]['provider']); + self::assertSame('storage', array_values($filteredColumnNameMetadata)[0]['provider']); + } + + /** + * On the second run the table already exists, so the description goes through StoragePreparer, which diffs + * it against the value stored in the table definition. A typed table exposes it in the same place a + * non-typed one does, otherwise the diff would keep re-sending an unchanged patch. + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionIsUpdatedOnExistingTypedTable(): void + { + $tableId = $this->emptyOutputBucketId . '.test1'; + + $this->uploadTableWithSchema($tableId, 'table description', 'Id description'); + $this->uploadTableWithSchema($tableId, 'updated table description', 'updated Id description'); + + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetails['isTyped']); + self::assertTrue($tableDetails['isDescriptionSystemManaged']); + self::assertSame('updated table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + ['Id' => 'updated Id description', 'Name' => null], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + + // Storage rejects a patch without any effective change with 400, so an unchanged third run must not + // send one at all + $this->uploadTableWithSchema($tableId, 'updated table description', 'updated Id description'); + } + + #[NeedsEmptyOutputBucket] + public function testDescriptionIsNotOverwrittenOnUserManagedTypedTable(): void + { + $tableId = $this->emptyOutputBucketId . '.test1'; + + $this->uploadTableWithSchema($tableId, 'table description', 'Id description'); + + // the user takes over the description + $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition($tableId, [ + 'description' => 'description set by the user', + 'isDescriptionSystemManaged' => false, + 'columns' => [ + ['name' => 'Id', 'description' => 'Id description set by the user'], + ], + ]); + + $this->uploadTableWithSchema($tableId, 'description from component', 'Id description from component'); + + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertFalse($tableDetails['isDescriptionSystemManaged']); + self::assertSame('description set by the user', $tableDetails['definition']['description'] ?? null); + self::assertSame( + ['Id' => 'Id description set by the user', 'Name' => null], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + + self::assertTrue($this->testHandler->hasInfoThatContains(sprintf( + 'Description of table "%s" is managed by the user, keeping the current value.', + $tableId, + ))); + } + + /** + * A column added by the same run must already be part of the table when the descriptions are stored, + * otherwise its description is reported as belonging to a non-existent column and dropped. + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionOfColumnAddedInSecondRun(): void + { + $tableId = $this->emptyOutputBucketId . '.test1'; + + $this->uploadTableWithSchema($tableId, 'table description', 'Id description'); + $this->uploadTableWithSchema($tableId, 'table description', 'Id description', 'foo description'); + + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame( + ['Id' => 'Id description', 'Name' => null, 'foo' => 'foo description'], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + + self::assertFalse($this->testHandler->hasWarningThatContains( + 'Cannot store description of column(s)', + )); + } + + /** + * Writes Id/Name, plus a third `foo` column when its description is given. + */ + private function uploadTableWithSchema( + string $tableId, + ?string $tableDescription, + ?string $idDescription, + ?string $fooDescription = null, + ): void { + $schema = [ + $this->schemaColumn('Id', $idDescription), + $this->schemaColumn('Name'), + ]; + $csv = "\"1\",\"bob\"\n\"2\",\"alice\"\n"; + + if ($fooDescription !== null) { + $schema[] = $this->schemaColumn('foo', $fooDescription); + $csv = "\"1\",\"bob\",\"firstFoo\"\n\"2\",\"alice\",\"secondFoo\"\n"; + } + + $config = [ + 'source' => 'table.csv', + 'destination' => $tableId, + 'schema' => $schema, + ]; + if ($tableDescription !== null) { + $config['description'] = $tableDescription; + } + + file_put_contents($this->temp->getTmpFolder() . '/upload/table.csv', $csv); + + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( + configuration: new OutputMappingSettings( + configuration: ['mapping' => [$config]], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_AUTHORITATIVE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + self::assertCount(1, $tableQueue->waitForAll()); + } + + /** + * @return array + */ + private function schemaColumn(string $name, ?string $description = null): array + { + $column = [ + 'name' => $name, + 'data_type' => [ + 'base' => [ + 'type' => 'STRING', + ], + ], + ]; + if ($description !== null) { + $column['description'] = $description; + } + + return $column; + } + + /** + * @param array $columns + * @return array column name => description + */ + private function getColumnDescriptions(array $columns): array + { + $descriptions = []; + foreach ($columns as $column) { + self::assertIsArray($column); + $columnName = $column['name']; + self::assertIsString($columnName); + + $definition = $column['definition'] ?? []; + self::assertIsArray($definition); + $description = $definition['description'] ?? null; + + $descriptions[$columnName] = is_string($description) ? $description : null; + } + + return $descriptions; } #[NeedsEmptyOutputBucket] diff --git a/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php index 69e58864d..2ff48380d 100644 --- a/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php +++ b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php @@ -4,23 +4,104 @@ namespace Keboola\OutputMapping\Tests\Writer; +use Generator; +use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\OutputMappingSettings; use Keboola\OutputMapping\SystemMetadata; use Keboola\OutputMapping\Tests\AbstractTestCase; use Keboola\OutputMapping\Tests\Needs\NeedsEmptyOutputBucket; +use Keboola\OutputMapping\Writer\Helper\DescriptionHelper; class TableDescriptionWriterTest extends AbstractTestCase { + /** the dedicated `description` field of the mapping */ + private const SOURCE_DEDICATED_FIELD = 'description'; + + /** legacy `KBC.description` in the `table_metadata` key => value map */ + private const SOURCE_TABLE_METADATA = 'table_metadata'; + + /** legacy `KBC.description` in the `metadata` list of {key, value} items */ + private const SOURCE_METADATA_LIST = 'metadata'; + + /** + * The description reaches Storage the same way regardless of which of the three configuration shapes + * carried it, and Storage is the only writer of the mirrored `KBC.description` metadata row. + * + * @dataProvider descriptionSourceProvider + */ #[NeedsEmptyOutputBucket] - public function testDescriptionIsStoredOnCreatedTable(): void + public function testDescriptionIsStoredOnCreatedTable(string $descriptionSource): void { $tableId = $this->emptyOutputBucketId . '.tableDescription'; - $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, 'table description', 'Id description', $descriptionSource); $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + // `columns` in the mapping creates the table through a non-typed table-definition payload; which + // branch of LoadTableTaskCreator runs depends on the project features, so pin it here + self::assertFalse($tableDetail['isTyped']); self::assertSame('table description', $tableDetail['definition']['description'] ?? null); self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + + $this->assertSingleStorageDescriptionRow($tableDetail['metadata'], 'table description'); + } + + public static function descriptionSourceProvider(): Generator + { + yield 'dedicated description field' => ['descriptionSource' => self::SOURCE_DEDICATED_FIELD]; + yield 'table_metadata KBC.description' => ['descriptionSource' => self::SOURCE_TABLE_METADATA]; + yield 'metadata list KBC.description' => ['descriptionSource' => self::SOURCE_METADATA_LIST]; + } + + /** + * A table written without a manifest and without `columns` is created by the load job itself + * (CreateAndLoadTableTask). There is no create-table-definition payload the description could be part of, + * so it is stored once the load finishes and the table surely exists. + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionIsStoredOnTableCreatedByLoadJob(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; + + $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame(['Id', 'Name'], $tableDetail['columns']); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + /** + * The second run finds the table already there, so the description goes through StoragePreparer, which + * diffs it against the stored value read from `definition.description`. A table created by the load job + * must expose the description in the same place as one created through a table definition, otherwise the + * diff always sees null and Storage rejects the unchanged patch with 400. + */ + #[NeedsEmptyOutputBucket] + public function testRepeatedRunOnTableCreatedByLoadJobSucceeds(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; + + $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); + $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + #[NeedsEmptyOutputBucket] + public function testDescriptionIsUpdatedOnTableCreatedByLoadJob(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; + + $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); + $this->uploadTableWithoutColumns($tableId, 'updated table description', 'updated Id description'); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetail['isDescriptionSystemManaged']); + self::assertSame('updated table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('updated Id description', $this->getColumnDescription($tableDetail, 'Id')); } #[NeedsEmptyOutputBucket] @@ -76,25 +157,103 @@ public function testDescriptionIsNotOverwrittenOnUserManagedTable(): void self::assertFalse($tableDetail['isDescriptionSystemManaged']); self::assertSame('description set by the user', $tableDetail['definition']['description'] ?? null); self::assertSame('Id description set by the user', $this->getColumnDescription($tableDetail, 'Id')); + + self::assertTrue($this->testHandler->hasInfoThatContains(sprintf( + 'Description of table "%s" is managed by the user, keeping the current value.', + $tableId, + ))); } - private function uploadTable(string $tableId, string $tableDescription, string $columnDescription): void + /** + * An empty description means "nothing to store", so a run which no longer carries one keeps the value + * already in Storage instead of clearing it. + * + * @dataProvider descriptionRemovedProvider + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionIsKeptWhenNoLongerInConfiguration( + ?string $tableDescription, + ?string $columnDescription, + ): void { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, $tableDescription, $columnDescription); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertTrue($tableDetail['isDescriptionSystemManaged']); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + public static function descriptionRemovedProvider(): Generator { + yield 'description dropped from the configuration' => [ + 'tableDescription' => null, + 'columnDescription' => null, + ]; + yield 'description sent as an empty string' => [ + 'tableDescription' => '', + 'columnDescription' => '', + ]; + } + + /** + * A description of a column the data does not have must never create that column - it is reported and + * skipped, and the rest of the load goes through. + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionOfMissingColumnIsSkippedWithWarning(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description', self::SOURCE_DEDICATED_FIELD, [ + 'nope' => [ + ['key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, 'value' => 'description of nothing'], + ], + ]); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame(['Id', 'Name'], $tableDetail['columns']); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + + self::assertTrue($this->testHandler->hasWarningThatContains( + 'Cannot store description of column(s) "nope"', + )); + } + + /** + * A description in the create-table-definition payload makes Storage write the mirrored + * `KBC.description` metadata row at create time, before anything is loaded. That row must not be mistaken + * for "the table has already been used", otherwise a failed load leaves an empty broken table behind. + */ + #[NeedsEmptyOutputBucket] + public function testFailedLoadDropsFreshlyCreatedTableWithDescription(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescriptionFailedLoad'; + $root = $this->temp->getTmpFolder(); - file_put_contents($root . '/upload/tableDescription.csv', "\"1\",\"bob\"\n\"2\",\"alice\"\n"); + file_put_contents( + $root . '/upload/tableDescriptionFailedLoad.csv', + "\"test\",\"test\"\n\"aabb\",\"ccdd\",\"dddd\"\n", + ); - $tableQueue = $this->getTableLoader()->uploadTables( + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( configuration: new OutputMappingSettings( configuration: [ 'mapping' => [ [ - 'source' => 'tableDescription.csv', + 'source' => 'tableDescriptionFailedLoad.csv', 'destination' => $tableId, 'columns' => ['Id', 'Name'], - 'description' => $tableDescription, + 'description' => 'table description', 'column_metadata' => [ 'Id' => [ - ['key' => 'KBC.description', 'value' => $columnDescription], + [ + 'key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, + 'value' => 'Id description', + ], ], ], ], @@ -108,10 +267,196 @@ private function uploadTable(string $tableId, string $tableDescription, string $ systemMetadata: new SystemMetadata(['componentId' => 'foo']), ); + try { + $tableQueue->waitForAll(); + self::fail('Must throw exception'); + } catch (InvalidOutputException $e) { + self::assertStringContainsString(sprintf('Failed to load table "%s"', $tableId), $e->getMessage()); + } + + self::assertFalse($this->clientWrapper->getTableAndFileStorageClient()->tableExists($tableId)); + self::assertTrue($this->testHandler->hasWarningThatContains( + sprintf('Failed to load table "%s". Dropping table.', $tableId), + )); + // the description was never stored, but it was not lost by mistake either + self::assertFalse($this->testHandler->hasWarningThatContains('was not stored')); + } + + /** + * A table which existed before the failed load keeps its data and its description - only a table freshly + * created by the very same run may be dropped. + */ + #[NeedsEmptyOutputBucket] + public function testFailedLoadKeepsPreExistingTableWithDescription(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + + $root = $this->temp->getTmpFolder(); + file_put_contents( + $root . '/upload/tableDescription.csv', + "\"test\",\"test\"\n\"aabb\",\"ccdd\",\"dddd\"\n", + ); + + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( + configuration: new OutputMappingSettings( + configuration: [ + 'mapping' => [ + [ + 'source' => 'tableDescription.csv', + 'destination' => $tableId, + 'columns' => ['Id', 'Name'], + 'description' => 'table description', + ], + ], + ], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + try { + $tableQueue->waitForAll(); + self::fail('Must throw exception'); + } catch (InvalidOutputException $e) { + self::assertStringContainsString(sprintf('Failed to load table "%s"', $tableId), $e->getMessage()); + } + + self::assertTrue($this->clientWrapper->getTableAndFileStorageClient()->tableExists($tableId)); + self::assertFalse($this->testHandler->hasWarningThatContains('Dropping table')); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); + } + + /** + * @param array|null $extraColumnMetadata + */ + private function uploadTable( + string $tableId, + ?string $tableDescription, + ?string $columnDescription, + string $descriptionSource = self::SOURCE_DEDICATED_FIELD, + ?array $extraColumnMetadata = null, + ): void { + $root = $this->temp->getTmpFolder(); + file_put_contents($root . '/upload/tableDescription.csv', "\"1\",\"bob\"\n\"2\",\"alice\"\n"); + + $mapping = [ + 'source' => 'tableDescription.csv', + 'destination' => $tableId, + 'columns' => ['Id', 'Name'], + ]; + + if ($tableDescription !== null) { + $mapping += match ($descriptionSource) { + self::SOURCE_DEDICATED_FIELD => ['description' => $tableDescription], + self::SOURCE_TABLE_METADATA => [ + 'table_metadata' => [ + DescriptionHelper::DESCRIPTION_METADATA_KEY => $tableDescription, + ], + ], + self::SOURCE_METADATA_LIST => [ + 'metadata' => [ + [ + 'key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, + 'value' => $tableDescription, + ], + ], + ], + default => self::fail(sprintf('Unknown description source "%s".', $descriptionSource)), + }; + } + + $columnMetadata = $extraColumnMetadata ?? []; + if ($columnDescription !== null) { + $columnMetadata['Id'] = [ + ['key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, 'value' => $columnDescription], + ]; + } + if ($columnMetadata !== []) { + $mapping['column_metadata'] = $columnMetadata; + } + + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( + configuration: new OutputMappingSettings( + configuration: ['mapping' => [$mapping]], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + $jobIds = $tableQueue->waitForAll(); self::assertCount(1, $jobIds); } + private function uploadTableWithoutColumns( + string $tableId, + ?string $tableDescription, + ?string $columnDescription, + ): void { + $root = $this->temp->getTmpFolder(); + file_put_contents( + $root . '/upload/tableDescriptionNoManifest.csv', + "\"Id\",\"Name\"\n\"1\",\"bob\"\n\"2\",\"alice\"\n", + ); + + $mapping = [ + 'source' => 'tableDescriptionNoManifest.csv', + 'destination' => $tableId, + ]; + if ($tableDescription !== null) { + $mapping['description'] = $tableDescription; + } + if ($columnDescription !== null) { + $mapping['column_metadata'] = [ + 'Id' => [ + ['key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, 'value' => $columnDescription], + ], + ]; + } + + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( + configuration: new OutputMappingSettings( + configuration: ['mapping' => [$mapping]], + sourcePathPrefix: 'upload', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + self::assertCount(1, $tableQueue->waitForAll()); + } + + /** + * Output mapping stores the description natively and Storage mirrors it into metadata. A second row, or a + * row under any other provider, would mean output mapping wrote the key itself. + * + * @param array $metadata + */ + private function assertSingleStorageDescriptionRow(array $metadata, string $expectedDescription): void + { + $descriptionRows = array_values(array_filter( + $metadata, + fn($row) => is_array($row) && ($row['key'] ?? null) === DescriptionHelper::DESCRIPTION_METADATA_KEY, + )); + + self::assertCount(1, $descriptionRows); + self::assertIsArray($descriptionRows[0]); + self::assertSame('storage', $descriptionRows[0]['provider']); + self::assertSame($expectedDescription, $descriptionRows[0]['value']); + } + private function getColumnDescription(array $tableDetail, string $columnName): ?string { $definition = $tableDetail['definition'] ?? []; diff --git a/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php b/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php index 68e583ad7..f52ff1cf1 100644 --- a/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php +++ b/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php @@ -99,6 +99,64 @@ public function testSnowflakeTableOutputMapping(): void ); } + /** + * The descriptions are stored through the table-definition API regardless of the staging the data came + * from, but workspace unload reaches LoadTableTaskCreator through a different strategy. + */ + #[NeedsTestTables(2), NeedsEmptyOutputBucket] + public function testSnowflakeTableOutputMappingStoresDescription(): void + { + $this->initWorkspace(); + $factory = $this->getWorkspaceStagingFactory(); + + $this->prepareWorkspaceWithTablesClone($this->testBucketId); + + $tableId = $this->emptyOutputBucketId . '.table1a'; + + $config = [ + 'source' => 'table1a', + 'destination' => $tableId, + 'columns' => ['Id', 'Name'], + 'description' => 'table description', + 'column_metadata' => [ + 'Id' => [ + ['key' => 'KBC.description', 'value' => 'Id description'], + ], + ], + ]; + + file_put_contents( + $this->temp->getTmpFolder() . '/table1a.manifest', + (string) json_encode(['columns' => ['Id', 'Name']]), + ); + + $tableQueue = $this->getTableLoader( + logger: $this->testLogger, + strategyFactory: $factory, + )->uploadTables( + configuration: new OutputMappingSettings( + configuration: ['mapping' => [$config]], + sourcePathPrefix: '/', + storageApiToken: $this->clientWrapper->getToken(), + isFailedJob: false, + dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, + ), + systemMetadata: new SystemMetadata(['componentId' => 'foo']), + ); + + self::assertCount(1, $tableQueue->waitForAll()); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + + $idColumn = array_values(array_filter( + $tableDetail['definition']['columns'], + fn($column) => $column['name'] === 'Id', + )); + self::assertCount(1, $idColumn); + self::assertSame('Id description', $idColumn[0]['definition']['description'] ?? null); + } + #[NeedsEmptyOutputBucket] public function testTableOutputMappingMissing(): void { From bf9ef1785203cd57f4530f98d2492eefe9fd07cb Mon Sep 17 00:00:00 2001 From: zajca Date: Thu, 30 Jul 2026 14:48:39 +0200 Subject: [PATCH 07/11] fix(output-mapping): resolve a column description from the first metadata item A metadata list may contain more than one KBC.description item; the schema does not prevent it. getTableDescription() returns on the first match, while getColumnDescriptions() kept overwriting, so the last non-empty value won and the two levels disagreed on the same input. Both now let the first item decide. --- .../MappingFromProcessedConfiguration.php | 2 + .../MappingFromProcessedConfigurationTest.php | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php index bfcd9d957..b6fa332e9 100644 --- a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php +++ b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php @@ -271,6 +271,8 @@ public function getColumnDescriptions(): array if ($description !== null) { $descriptions[(string) $columnName] = $description; } + // the first KBC.description item of a column decides, as in getTableDescription() + break; } } diff --git a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php index ecc627472..726a98337 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php @@ -125,6 +125,24 @@ public static function tableDescriptionProvider(): Generator 'mappingConfiguration' => ['description' => ''], 'expectedDescription' => null, ]; + yield 'the first KBC.description item of the metadata list decides' => [ + 'mappingConfiguration' => [ + 'metadata' => [ + ['key' => 'KBC.description', 'value' => 'first desc'], + ['key' => 'KBC.description', 'value' => 'second desc'], + ], + ], + 'expectedDescription' => 'first desc', + ]; + yield 'an empty first KBC.description item is not overridden by a later one' => [ + 'mappingConfiguration' => [ + 'metadata' => [ + ['key' => 'KBC.description', 'value' => ''], + ['key' => 'KBC.description', 'value' => 'second desc'], + ], + ], + 'expectedDescription' => null, + ]; } public function testGetColumnDescriptionsSkipsEmptyDescriptions(): void @@ -189,6 +207,26 @@ public function testGetColumnDescriptionsFromColumnMetadata(): void self::assertSame(['col1' => 'col1 desc'], $mapping->getColumnDescriptions()); } + public function testGetColumnDescriptionsUseTheFirstDescriptionItemOfAColumn(): void + { + $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); + $mapping = new MappingFromProcessedConfiguration([ + 'destination' => 'in.c-main.table', + 'column_metadata' => [ + 'col1' => [ + ['key' => 'KBC.description', 'value' => 'first desc'], + ['key' => 'KBC.description', 'value' => 'second desc'], + ], + 'col2' => [ + ['key' => 'KBC.description', 'value' => ''], + ['key' => 'KBC.description', 'value' => 'second desc'], + ], + ], + ], $physicalDataWithManifest); + + self::assertSame(['col1' => 'first desc'], $mapping->getColumnDescriptions()); + } + public function testGetColumnDescriptionsSkipsRestrictedColumns(): void { $physicalDataWithManifest = $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class); From 218f198ca643bd0e30a4046956b920cf7dda98ab Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 3 Aug 2026 09:43:43 +0200 Subject: [PATCH 08/11] refactor(output-mapping): address review of the description handling - The create payload is built by the table definition again. TableDefinitionInterface gained setDescriptions(), each implementation renders the descriptions in its own getRequestData(), so what is sent to Storage stays in one place. TableCreator is back to its original signature and the standalone request-data enricher is gone. - A metadata map which is not an object is reported instead of being silently dropped; the node is a variableNode, so the value really can be anything and swallowing it hid a configuration error. Resolution of the description now goes through DescriptionHelper in every path, and the helper has unit tests. - TableDescriptionModifier wraps only a 4xx as a user error; a connection error carries no HTTP status, so the previous >= 500 check turned it into one. - Dropped the redundant early exit in LoadTableQueue::applyCreatedTableDescriptions(). --- .../src/DeferredTasks/LoadTableQueue.php | 4 - .../src/LoadTableTaskCreator.php | 38 ++-- .../MappingFromConfigurationSchemaColumn.php | 18 +- .../MappingFromProcessedConfiguration.php | 38 ++-- .../src/Storage/TableCreator.php | 28 +-- .../src/Storage/TableDescriptionModifier.php | 5 +- .../src/Writer/Helper/DescriptionHelper.php | 71 ++++++- ...eateTableDefinitionDescriptionEnricher.php | 84 -------- .../Table/TableDefinition/TableDefinition.php | 6 +- .../TableDefinitionDescriptionsTrait.php | 81 ++++++++ .../TableDefinitionFromColumns.php | 6 +- .../TableDefinitionFromSchema.php | 7 +- .../Writer/Table/TableDefinitionInterface.php | 9 + ...ppingFromConfigurationSchemaColumnTest.php | 12 +- .../MappingFromProcessedConfigurationTest.php | 22 ++- .../tests/Storage/TableCreatorTest.php | 8 +- .../Writer/Helper/DescriptionHelperTest.php | 187 ++++++++++++++++++ ...TableDefinitionDescriptionEnricherTest.php | 183 ----------------- .../TableDefinitionDescriptionsTest.php | 175 ++++++++++++++++ 19 files changed, 615 insertions(+), 367 deletions(-) delete mode 100644 libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php create mode 100644 libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionDescriptionsTrait.php create mode 100644 libs/output-mapping/tests/Writer/Helper/DescriptionHelperTest.php delete mode 100644 libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php create mode 100644 libs/output-mapping/tests/Writer/Table/TableDefinition/TableDefinitionDescriptionsTest.php diff --git a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php index 724ecb9ce..530831a25 100644 --- a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php +++ b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php @@ -165,10 +165,6 @@ public function waitForAll(): array */ private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, array $tableData): void { - if ($this->createdTableDescriptions === []) { - return; - } - $tableId = $task->getDestinationTableName(); $descriptions = $this->createdTableDescriptions[$tableId] ?? null; if ($descriptions === null) { diff --git a/libs/output-mapping/src/LoadTableTaskCreator.php b/libs/output-mapping/src/LoadTableTaskCreator.php index 2e4aa2ca7..88aa68a5e 100644 --- a/libs/output-mapping/src/LoadTableTaskCreator.php +++ b/libs/output-mapping/src/LoadTableTaskCreator.php @@ -16,6 +16,7 @@ use Keboola\OutputMapping\Writer\Table\TableDefinition\TableDefinitionFactory; use Keboola\OutputMapping\Writer\Table\TableDefinition\TableDefinitionFromColumns; use Keboola\OutputMapping\Writer\Table\TableDefinitionFromSchema\TableDefinitionFromSchema; +use Keboola\OutputMapping\Writer\Table\TableDefinitionInterface; use Keboola\StorageApiBranch\ClientWrapper; use Psr\Log\LoggerInterface; @@ -25,7 +26,7 @@ class LoadTableTaskCreator public function __construct(readonly ClientWrapper $clientWrapper, readonly LoggerInterface $logger) { - $this->tableCreator = new TableCreator($clientWrapper, $logger); + $this->tableCreator = new TableCreator($clientWrapper); } /** @@ -73,11 +74,7 @@ public function create( $source->getPrimaryKey(), $source->getColumnMetadata(), ); - $this->tableCreator->createTableDefinition( - $source->getDestination()->getBucketId(), - $tableDefinition, - $descriptions, - ); + $this->createTableDefinition($source, $tableDefinition, $descriptions); return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif ($settings->hasNewNativeTypesFeature() && !$storageSources->didTableExistBefore() && @@ -88,11 +85,7 @@ public function create( $source->getSchema(), $storageSources->getBucket()->backend, ); - $this->tableCreator->createTableDefinition( - $source->getDestination()->getBucketId(), - $tableDefinition, - $descriptions, - ); + $this->createTableDefinition($source, $tableDefinition, $descriptions); return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif (!$storageSources->didTableExistBefore() && $source->hasColumns()) { // tabulka neexistuje a známe sloupce z manifestu - vytváříme ji přes table definition bez typů @@ -101,11 +94,7 @@ public function create( $source->getColumns(), $source->getPrimaryKey(), ); - $this->tableCreator->createTableDefinition( - $source->getDestination()->getBucketId(), - $tableDefinition, - $descriptions, - ); + $this->createTableDefinition($source, $tableDefinition, $descriptions); return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif ($storageSources->didTableExistBefore()) { // tabulka existuje takže nahráváme data @@ -128,6 +117,23 @@ public function create( } } + private function createTableDefinition( + MappingFromProcessedConfiguration $source, + TableDefinitionInterface $tableDefinition, + ?TableDescription $descriptions, + ): void { + // The table is brand new, therefore its description is always system-managed (Storage default) and + // there is nothing to diff against. + if ($descriptions !== null && !$descriptions->isEmpty()) { + $tableDefinition->setDescriptions($descriptions, $this->logger); + } + + $this->tableCreator->createTableDefinition( + $source->getDestination()->getBucketId(), + $tableDefinition, + ); + } + public function buildLoadOptions( MappingFromProcessedConfiguration $source, StrategyInterface $strategy, diff --git a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php index fa89ba409..91beba3eb 100644 --- a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php +++ b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php @@ -51,7 +51,10 @@ public function hasMetadata(): bool */ public function getMetadata(): array { - return DescriptionHelper::removeDescriptionFromMetadataMap($this->mapping['metadata'] ?? []); + return DescriptionHelper::removeDescriptionFromMetadataMap( + $this->mapping['metadata'] ?? [], + 'schema.metadata', + ); } /** @@ -65,14 +68,9 @@ public function getDescription(): ?string return DescriptionHelper::normalizeDescription($this->mapping['description']); } - // metadata is a variableNode in the configuration, so it is not guaranteed to be an array - $metadata = $this->mapping['metadata'] ?? []; - if (is_array($metadata) && isset($metadata[DescriptionHelper::DESCRIPTION_METADATA_KEY])) { - return DescriptionHelper::normalizeDescription( - $metadata[DescriptionHelper::DESCRIPTION_METADATA_KEY], - ); - } - - return null; + return DescriptionHelper::getDescriptionFromMetadataMap( + $this->mapping['metadata'] ?? [], + 'schema.metadata', + ); } } diff --git a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php index b6fa332e9..0341e8573 100644 --- a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php +++ b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php @@ -200,7 +200,10 @@ public function hasTableMetadata(): bool */ public function getTableMetadata(): array { - return DescriptionHelper::removeDescriptionFromMetadataMap($this->mapping['table_metadata'] ?? []); + return DescriptionHelper::removeDescriptionFromMetadataMap( + $this->mapping['table_metadata'] ?? [], + 'table_metadata', + ); } /** @@ -217,21 +220,15 @@ public function getTableDescription(): ?string return DescriptionHelper::normalizeDescription($this->mapping['description']); } - // table_metadata is a variableNode in the configuration, so it is not guaranteed to be an array - $tableMetadata = $this->mapping['table_metadata'] ?? []; - if (is_array($tableMetadata) && isset($tableMetadata[DescriptionHelper::DESCRIPTION_METADATA_KEY])) { - return DescriptionHelper::normalizeDescription( - $tableMetadata[DescriptionHelper::DESCRIPTION_METADATA_KEY], - ); - } - - foreach ($this->mapping['metadata'] ?? [] as $item) { - if (is_array($item) && ($item['key'] ?? null) === DescriptionHelper::DESCRIPTION_METADATA_KEY) { - return DescriptionHelper::normalizeDescription($item['value'] ?? null); - } + $tableMetadataDescription = DescriptionHelper::getDescriptionFromMetadataMap( + $this->mapping['table_metadata'] ?? [], + 'table_metadata', + ); + if ($tableMetadataDescription !== null) { + return $tableMetadataDescription; } - return null; + return DescriptionHelper::getDescriptionFromMetadataList($this->mapping['metadata'] ?? []); } /** @@ -263,16 +260,9 @@ public function getColumnDescriptions(): array []; foreach ($columnMetadataFromConfiguration as $columnName => $metadata) { - foreach ($metadata as $item) { - if (!is_array($item) || ($item['key'] ?? null) !== DescriptionHelper::DESCRIPTION_METADATA_KEY) { - continue; - } - $description = DescriptionHelper::normalizeDescription($item['value'] ?? null); - if ($description !== null) { - $descriptions[(string) $columnName] = $description; - } - // the first KBC.description item of a column decides, as in getTableDescription() - break; + $description = DescriptionHelper::getDescriptionFromMetadataList($metadata); + if ($description !== null) { + $descriptions[(string) $columnName] = $description; } } diff --git a/libs/output-mapping/src/Storage/TableCreator.php b/libs/output-mapping/src/Storage/TableCreator.php index 05727cc82..b140f66f2 100644 --- a/libs/output-mapping/src/Storage/TableCreator.php +++ b/libs/output-mapping/src/Storage/TableCreator.php @@ -5,40 +5,22 @@ namespace Keboola\OutputMapping\Storage; use Keboola\OutputMapping\Exception\InvalidOutputException; -use Keboola\OutputMapping\Writer\Table\TableDefinition\CreateTableDefinitionDescriptionEnricher; use Keboola\OutputMapping\Writer\Table\TableDefinitionInterface; use Keboola\StorageApi\ClientException; use Keboola\StorageApiBranch\ClientWrapper; -use Psr\Log\LoggerInterface; class TableCreator { - public function __construct( - private readonly ClientWrapper $clientWrapper, - private readonly LoggerInterface $logger, - ) { + public function __construct(private readonly ClientWrapper $clientWrapper) + { } - /** - * The descriptions are part of the create payload, so a table created here already carries them and no - * table-definition update job is needed after the load. The table is brand new, therefore its description - * is always system-managed (Storage default) and there is nothing to diff against. - */ - public function createTableDefinition( - string $bucketId, - TableDefinitionInterface $tableDefinition, - ?TableDescription $descriptions = null, - ): string { - $requestData = $tableDefinition->getRequestData(); - if ($descriptions !== null) { - $requestData = (new CreateTableDefinitionDescriptionEnricher($this->logger)) - ->enrich($requestData, $descriptions); - } - + public function createTableDefinition(string $bucketId, TableDefinitionInterface $tableDefinition): string + { try { return $this->clientWrapper->getTableAndFileStorageClient()->createTableDefinition( $bucketId, - $requestData, + $tableDefinition->getRequestData(), ); } catch (ClientException $e) { throw new InvalidOutputException( diff --git a/libs/output-mapping/src/Storage/TableDescriptionModifier.php b/libs/output-mapping/src/Storage/TableDescriptionModifier.php index 751586d94..2199eaf2a 100644 --- a/libs/output-mapping/src/Storage/TableDescriptionModifier.php +++ b/libs/output-mapping/src/Storage/TableDescriptionModifier.php @@ -50,8 +50,9 @@ public function updateDescriptions(TableInfo $tableInfo, TableDescription $descr $tableDefinitionUpdate, ); } catch (ClientException $e) { - if ($e->getCode() >= 500) { - // Let a Storage outage surface as a retryable application error, consistently with the + if ($e->getCode() < 400 || $e->getCode() >= 500) { + // Only a 4xx is the caller's fault. A Storage outage and a connection error (which carries no + // HTTP status, i.e. code 0) both stay a retryable application error, consistently with the // metadata path in LoadTableQueue. throw $e; } diff --git a/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php b/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php index 41d024ab4..774e75fb3 100644 --- a/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php +++ b/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php @@ -4,6 +4,8 @@ namespace Keboola\OutputMapping\Writer\Helper; +use Keboola\OutputMapping\Exception\InvalidOutputException; + /** * The table/column description is stored in the native Storage description field, not as `KBC.description` * metadata (AJDA-2946). Storage itself mirrors the native value into a `KBC.description` metadata row under @@ -35,19 +37,33 @@ public static function normalizeDescription(mixed $description): ?string * Removes the description from a key => value metadata map. * * @param mixed $metadata the node is a variableNode in the configuration, so it may be anything + * @param string $configurationNode name of the configuration node, for the error message * @return array */ - public static function removeDescriptionFromMetadataMap(mixed $metadata): array + public static function removeDescriptionFromMetadataMap(mixed $metadata, string $configurationNode): array { - if (!is_array($metadata)) { - return []; - } + $metadata = self::assertMetadataMap($metadata, $configurationNode); unset($metadata[self::DESCRIPTION_METADATA_KEY]); return $metadata; } + /** + * Reads the description from a key => value metadata map. + * + * @param mixed $metadata the node is a variableNode in the configuration, so it may be anything + * @param string $configurationNode name of the configuration node, for the error message + */ + public static function getDescriptionFromMetadataMap(mixed $metadata, string $configurationNode): ?string + { + $metadata = self::assertMetadataMap($metadata, $configurationNode); + + return isset($metadata[self::DESCRIPTION_METADATA_KEY]) + ? self::normalizeDescription($metadata[self::DESCRIPTION_METADATA_KEY]) + : null; + } + /** * Removes the description from a list of {key, value} metadata items. * @@ -58,7 +74,52 @@ public static function removeDescriptionFromMetadataList(array $metadata): array { return array_values(array_filter( $metadata, - fn($item): bool => !is_array($item) || ($item['key'] ?? null) !== self::DESCRIPTION_METADATA_KEY, + fn($item): bool => !self::isDescriptionItem($item), )); } + + /** + * Reads the description from a list of {key, value} metadata items. The first matching item decides, the + * schema does not prevent a list from carrying more than one. + * + * @param array $metadata + */ + public static function getDescriptionFromMetadataList(array $metadata): ?string + { + foreach ($metadata as $item) { + if (self::isDescriptionItem($item)) { + return self::normalizeDescription($item['value'] ?? null); + } + } + + return null; + } + + /** + * @phpstan-assert-if-true array{key: string, value?: mixed} $item + */ + private static function isDescriptionItem(mixed $item): bool + { + return is_array($item) && ($item['key'] ?? null) === self::DESCRIPTION_METADATA_KEY; + } + + /** + * A metadata map comes from a variableNode, so the configuration cannot guarantee its type. A value which + * is not a map is a configuration error and must be reported, not silently dropped. + * + * @return array + */ + private static function assertMetadataMap(mixed $metadata, string $configurationNode): array + { + if (!is_array($metadata)) { + throw new InvalidOutputException(sprintf( + 'Configuration node "%s" must be an object, "%s" given.', + $configurationNode, + get_debug_type($metadata), + )); + } + + /** @var array $metadata */ + return $metadata; + } } diff --git a/libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php b/libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php deleted file mode 100644 index b66d26e33..000000000 --- a/libs/output-mapping/src/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricher.php +++ /dev/null @@ -1,84 +0,0 @@ - $requestData payload as returned by TableDefinitionInterface::getRequestData() - * @return array - */ - public function enrich(array $requestData, TableDescription $descriptions): array - { - if ($descriptions->isEmpty()) { - return $requestData; - } - - $tableDescription = $descriptions->getTableDescription(); - if ($tableDescription !== null) { - $requestData['description'] = $tableDescription; - } - - $columnDescriptions = $descriptions->getColumnDescriptions(); - if ($columnDescriptions === []) { - return $requestData; - } - - $columns = $requestData['columns'] ?? []; - if (!is_array($columns)) { - return $requestData; - } - - $enrichedColumns = []; - $knownColumns = []; - foreach ($columns as $index => $column) { - $columnName = is_array($column) ? ($column['name'] ?? null) : null; - if (is_string($columnName) && isset($columnDescriptions[$columnName])) { - $knownColumns[] = $columnName; - - $definition = $column['definition'] ?? []; - if (!is_array($definition)) { - $definition = []; - } - $definition['description'] = $columnDescriptions[$columnName]; - $column['definition'] = $definition; - } - $enrichedColumns[$index] = $column; - } - - // A description of a column which is not part of the payload must never append a new column entry - - // that would create a column the data does not have. - $missingColumns = array_diff(array_keys($columnDescriptions), $knownColumns); - if ($missingColumns) { - $this->logger->warning(sprintf( - 'Cannot store description of column(s) "%s" of table "%s", the column(s) do not exist.', - implode('", "', $missingColumns), - $descriptions->getTableId(), - )); - } - - $requestData['columns'] = $enrichedColumns; - - return $requestData; - } -} diff --git a/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinition.php b/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinition.php index 860db3070..679333a5a 100644 --- a/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinition.php +++ b/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinition.php @@ -8,6 +8,8 @@ class TableDefinition implements TableDefinitionInterface { + use TableDefinitionDescriptionsTrait; + public function __construct( private readonly TableDefinitionColumnFactory $tableDefinitionColumnFactory, ) { @@ -57,10 +59,10 @@ public function getRequestData(): array foreach ($this->columns as $column) { $columns[] = $column->toArray(); } - return [ + return $this->withDescriptions([ 'name' => $this->tableName, 'primaryKeysNames' => $this->primaryKeysNames, 'columns' => $columns, - ]; + ]); } } diff --git a/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionDescriptionsTrait.php b/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionDescriptionsTrait.php new file mode 100644 index 000000000..4beb1fdc0 --- /dev/null +++ b/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionDescriptionsTrait.php @@ -0,0 +1,81 @@ +descriptions = $descriptions; + $this->descriptionsLogger = $logger; + } + + /** + * Called by getRequestData() of the implementing definition, so that the payload it returns is always the + * one really sent to Storage. + * + * @param array $requestData + * @return array + */ + private function withDescriptions(array $requestData): array + { + if ($this->descriptions === null || $this->descriptions->isEmpty()) { + return $requestData; + } + + $tableDescription = $this->descriptions->getTableDescription(); + if ($tableDescription !== null) { + $requestData['description'] = $tableDescription; + } + + $columnDescriptions = $this->descriptions->getColumnDescriptions(); + if ($columnDescriptions === []) { + return $requestData; + } + + $knownColumns = []; + foreach ($requestData['columns'] as $index => $column) { + $columnName = $column['name'] ?? null; + if (!is_string($columnName) || !isset($columnDescriptions[$columnName])) { + continue; + } + $knownColumns[] = $columnName; + + $definition = $column['definition'] ?? []; + $definition['description'] = $columnDescriptions[$columnName]; + $requestData['columns'][$index]['definition'] = $definition; + } + + // A description of a column which is not part of the payload must never append a new column entry - + // that would create a column the data does not have. + $missingColumns = array_diff(array_keys($columnDescriptions), $knownColumns); + if ($missingColumns) { + $this->descriptionsLogger?->warning(sprintf( + 'Cannot store description of column(s) "%s" of table "%s", the column(s) do not exist.', + implode('", "', $missingColumns), + $this->descriptions->getTableId(), + )); + } + + return $requestData; + } +} diff --git a/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionFromColumns.php b/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionFromColumns.php index 434039faa..799bef6fe 100644 --- a/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionFromColumns.php +++ b/libs/output-mapping/src/Writer/Table/TableDefinition/TableDefinitionFromColumns.php @@ -16,6 +16,8 @@ */ class TableDefinitionFromColumns implements TableDefinitionInterface { + use TableDefinitionDescriptionsTrait; + /** * @param string[] $columns * @param string[] $primaryKeysNames @@ -29,14 +31,14 @@ public function __construct( public function getRequestData(): array { - return [ + return $this->withDescriptions([ 'name' => $this->tableName, 'primaryKeysNames' => array_values($this->primaryKeysNames), 'columns' => array_map( static fn(string $columnName): array => ['name' => $columnName], array_values($this->columns), ), - ]; + ]); } public function getTableName(): string diff --git a/libs/output-mapping/src/Writer/Table/TableDefinitionFromSchema/TableDefinitionFromSchema.php b/libs/output-mapping/src/Writer/Table/TableDefinitionFromSchema/TableDefinitionFromSchema.php index ed9ad3e05..3d93cccc9 100644 --- a/libs/output-mapping/src/Writer/Table/TableDefinitionFromSchema/TableDefinitionFromSchema.php +++ b/libs/output-mapping/src/Writer/Table/TableDefinitionFromSchema/TableDefinitionFromSchema.php @@ -5,10 +5,13 @@ namespace Keboola\OutputMapping\Writer\Table\TableDefinitionFromSchema; use Keboola\OutputMapping\Mapping\MappingFromConfigurationSchemaColumn; +use Keboola\OutputMapping\Writer\Table\TableDefinition\TableDefinitionDescriptionsTrait; use Keboola\OutputMapping\Writer\Table\TableDefinitionInterface; class TableDefinitionFromSchema implements TableDefinitionInterface { + use TableDefinitionDescriptionsTrait; + private array $primaryKeys = []; private array $columns = []; @@ -32,11 +35,11 @@ public function addColumn(MappingFromConfigurationSchemaColumn $column, string $ public function getRequestData(): array { - return [ + return $this->withDescriptions([ 'name' => $this->tableName, 'primaryKeysNames' => $this->primaryKeys, 'columns' => $this->columns, - ]; + ]); } public function getTableName(): string diff --git a/libs/output-mapping/src/Writer/Table/TableDefinitionInterface.php b/libs/output-mapping/src/Writer/Table/TableDefinitionInterface.php index cac9670f3..26bedf48b 100644 --- a/libs/output-mapping/src/Writer/Table/TableDefinitionInterface.php +++ b/libs/output-mapping/src/Writer/Table/TableDefinitionInterface.php @@ -4,9 +4,18 @@ namespace Keboola\OutputMapping\Writer\Table; +use Keboola\OutputMapping\Storage\TableDescription; +use Psr\Log\LoggerInterface; + interface TableDefinitionInterface { public function getRequestData(): array; public function getTableName(): string; + + /** + * Descriptions to be rendered into the payload returned by getRequestData(), so that a table created from + * this definition carries them right away. + */ + public function setDescriptions(TableDescription $descriptions, LoggerInterface $logger): void; } diff --git a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php index a67895a12..99d1f5df0 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php @@ -4,6 +4,7 @@ namespace Keboola\OutputMapping\Tests\Mapping; +use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\Mapping\MappingFromConfigurationSchemaColumn; use PHPUnit\Framework\TestCase; @@ -72,13 +73,20 @@ public function testGetDescriptionFromMetadata(): void self::assertFalse($schemColumn->hasMetadata()); } - public function testGetDescriptionIgnoresNonArrayMetadata(): void + /** + * The node is a variableNode, so a non-object value reaches the code. Dropping the metadata silently would + * hide the configuration error, so it is reported instead. + */ + public function testNonObjectMetadataIsReported(): void { $schemColumn = new MappingFromConfigurationSchemaColumn([ 'name' => 'newColumn', 'metadata' => 'this is a variableNode, so it may be anything', ]); - self::assertNull($schemColumn->getDescription()); + $this->expectException(InvalidOutputException::class); + $this->expectExceptionMessage('Configuration node "schema.metadata" must be an object, "string" given.'); + + $schemColumn->getDescription(); } } diff --git a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php index 726a98337..3c5d37261 100644 --- a/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php +++ b/libs/output-mapping/tests/Mapping/MappingFromProcessedConfigurationTest.php @@ -6,6 +6,7 @@ use Generator; use Keboola\OutputMapping\Configuration\Table\DeduplicationStrategy; +use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\Mapping\MappingFromProcessedConfiguration; use Keboola\OutputMapping\Mapping\MappingFromRawConfigurationAndPhysicalData; use Keboola\OutputMapping\Mapping\MappingFromRawConfigurationAndPhysicalDataWithManifest; @@ -87,6 +88,23 @@ public function testGetTableDescription(array $mappingConfiguration, ?string $ex self::assertSame($expectedDescription, $mapping->getTableDescription()); } + /** + * `table_metadata` is a variableNode, so a non-object value reaches the code. Dropping it silently would + * hide the configuration error, so it is reported instead. + */ + public function testNonObjectTableMetadataIsReported(): void + { + $mapping = new MappingFromProcessedConfiguration( + ['destination' => 'in.c-main.table', 'table_metadata' => 'not an array'], + $this->createMock(MappingFromRawConfigurationAndPhysicalDataWithManifest::class), + ); + + $this->expectException(InvalidOutputException::class); + $this->expectExceptionMessage('Configuration node "table_metadata" must be an object, "string" given.'); + + $mapping->getTableDescription(); + } + public static function tableDescriptionProvider(): Generator { yield 'from description field' => [ @@ -117,10 +135,6 @@ public static function tableDescriptionProvider(): Generator 'mappingConfiguration' => ['table_metadata' => ['key1' => 'val1']], 'expectedDescription' => null, ]; - yield 'table_metadata is a variableNode, so it may be anything' => [ - 'mappingConfiguration' => ['table_metadata' => 'not an array'], - 'expectedDescription' => null, - ]; yield 'empty description is not stored' => [ 'mappingConfiguration' => ['description' => ''], 'expectedDescription' => null, diff --git a/libs/output-mapping/tests/Storage/TableCreatorTest.php b/libs/output-mapping/tests/Storage/TableCreatorTest.php index 93384d6d7..c3a8c1a96 100644 --- a/libs/output-mapping/tests/Storage/TableCreatorTest.php +++ b/libs/output-mapping/tests/Storage/TableCreatorTest.php @@ -19,7 +19,7 @@ class TableCreatorTest extends AbstractTestCase #[NeedsEmptyOutputBucket] public function testCreateTableDefinition(): void { - $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); + $tableCreator = new TableCreator($this->clientWrapper); $tableDefinition = new TableDefinition( new TableDefinitionColumnFactory([], 'snowflake', true), @@ -60,7 +60,7 @@ public function testCreateTableDefinition(): void #[NeedsEmptyOutputBucket] public function testCreateTableDefinitionErrorHandling(): void { - $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); + $tableCreator = new TableCreator($this->clientWrapper); $tableDefinition = new TableDefinition( new TableDefinitionColumnFactory([], 'snowflake', true), @@ -94,7 +94,7 @@ public function testCreateTableDefinitionErrorHandling(): void #[NeedsEmptyOutputBucket] public function testCreateNonTypedTableFromColumns(): void { - $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); + $tableCreator = new TableCreator($this->clientWrapper); $tableId = $tableCreator->createTableDefinition( $this->emptyOutputBucketId, @@ -113,7 +113,7 @@ public function testCreateNonTypedTableFromColumns(): void #[NeedsEmptyOutputBucket] public function testCreateNonTypedTableFromColumnsErrorHandling(): void { - $tableCreator = new TableCreator($this->clientWrapper, $this->testLogger); + $tableCreator = new TableCreator($this->clientWrapper); try { $tableCreator->createTableDefinition( diff --git a/libs/output-mapping/tests/Writer/Helper/DescriptionHelperTest.php b/libs/output-mapping/tests/Writer/Helper/DescriptionHelperTest.php new file mode 100644 index 000000000..1a043309d --- /dev/null +++ b/libs/output-mapping/tests/Writer/Helper/DescriptionHelperTest.php @@ -0,0 +1,187 @@ + ['description' => 'some description', 'expected' => 'some description']; + yield 'empty string is no description' => ['description' => '', 'expected' => null]; + yield 'whitespace is a description' => ['description' => ' ', 'expected' => ' ']; + yield 'null' => ['description' => null, 'expected' => null]; + yield 'integer is cast' => ['description' => 42, 'expected' => '42']; + yield 'zero is cast' => ['description' => 0, 'expected' => '0']; + yield 'false is no description' => ['description' => false, 'expected' => null]; + yield 'array' => ['description' => ['nope'], 'expected' => null]; + } + + public function testRemoveDescriptionFromMetadataMap(): void + { + $metadata = [ + 'KBC.description' => 'some description', + 'KBC.datatype.type' => 'VARCHAR', + ]; + + self::assertSame( + ['KBC.datatype.type' => 'VARCHAR'], + DescriptionHelper::removeDescriptionFromMetadataMap($metadata, 'table_metadata'), + ); + } + + public function testRemoveDescriptionFromMetadataMapKeepsMapWithoutDescription(): void + { + $metadata = ['KBC.datatype.type' => 'VARCHAR']; + + self::assertSame($metadata, DescriptionHelper::removeDescriptionFromMetadataMap($metadata, 'metadata')); + } + + public function testGetDescriptionFromMetadataMap(): void + { + self::assertSame( + 'some description', + DescriptionHelper::getDescriptionFromMetadataMap( + ['KBC.description' => 'some description'], + 'table_metadata', + ), + ); + } + + /** + * @dataProvider metadataMapWithoutDescriptionProvider + */ + public function testGetDescriptionFromMetadataMapWithoutDescription(array $metadata): void + { + self::assertNull(DescriptionHelper::getDescriptionFromMetadataMap($metadata, 'table_metadata')); + } + + public static function metadataMapWithoutDescriptionProvider(): Generator + { + yield 'empty map' => ['metadata' => []]; + yield 'other keys only' => ['metadata' => ['KBC.datatype.type' => 'VARCHAR']]; + yield 'empty description' => ['metadata' => ['KBC.description' => '']]; + } + + /** + * The node is a variableNode in the configuration, so a non-object value reaches the code. It is a + * configuration error and must be reported instead of silently dropping the metadata. + * + * @dataProvider notAMetadataMapProvider + */ + public function testMetadataMapMustBeAnObject(mixed $metadata, string $expectedType): void + { + $this->expectException(InvalidOutputException::class); + $this->expectExceptionMessage(sprintf( + 'Configuration node "table_metadata" must be an object, "%s" given.', + $expectedType, + )); + + DescriptionHelper::removeDescriptionFromMetadataMap($metadata, 'table_metadata'); + } + + /** + * @dataProvider notAMetadataMapProvider + */ + public function testMetadataMapMustBeAnObjectWhenReadingDescription(mixed $metadata, string $expectedType): void + { + $this->expectException(InvalidOutputException::class); + $this->expectExceptionMessage(sprintf( + 'Configuration node "table_metadata" must be an object, "%s" given.', + $expectedType, + )); + + DescriptionHelper::getDescriptionFromMetadataMap($metadata, 'table_metadata'); + } + + public static function notAMetadataMapProvider(): Generator + { + yield 'string' => ['metadata' => 'some description', 'expectedType' => 'string']; + yield 'integer' => ['metadata' => 42, 'expectedType' => 'int']; + yield 'boolean' => ['metadata' => true, 'expectedType' => 'bool']; + } + + public function testRemoveDescriptionFromMetadataList(): void + { + $metadata = [ + ['key' => 'KBC.datatype.type', 'value' => 'VARCHAR'], + ['key' => 'KBC.description', 'value' => 'some description'], + ['key' => 'KBC.datatype.nullable', 'value' => true], + ]; + + self::assertSame( + [ + ['key' => 'KBC.datatype.type', 'value' => 'VARCHAR'], + ['key' => 'KBC.datatype.nullable', 'value' => true], + ], + DescriptionHelper::removeDescriptionFromMetadataList($metadata), + ); + } + + public function testRemoveDescriptionFromMetadataListRemovesEveryDescriptionItem(): void + { + $metadata = [ + ['key' => 'KBC.description', 'value' => 'first'], + ['key' => 'KBC.description', 'value' => 'second'], + ]; + + self::assertSame([], DescriptionHelper::removeDescriptionFromMetadataList($metadata)); + } + + public function testGetDescriptionFromMetadataList(): void + { + $metadata = [ + ['key' => 'KBC.datatype.type', 'value' => 'VARCHAR'], + ['key' => 'KBC.description', 'value' => 'some description'], + ]; + + self::assertSame('some description', DescriptionHelper::getDescriptionFromMetadataList($metadata)); + } + + /** + * The schema does not prevent a list from carrying more than one description; the first item decides, so + * that the table and the column level agree on the same input. + */ + public function testGetDescriptionFromMetadataListReadsTheFirstItem(): void + { + $metadata = [ + ['key' => 'KBC.description', 'value' => 'first'], + ['key' => 'KBC.description', 'value' => 'second'], + ]; + + self::assertSame('first', DescriptionHelper::getDescriptionFromMetadataList($metadata)); + } + + /** + * @dataProvider metadataListWithoutDescriptionProvider + */ + public function testGetDescriptionFromMetadataListWithoutDescription(array $metadata): void + { + self::assertNull(DescriptionHelper::getDescriptionFromMetadataList($metadata)); + } + + public static function metadataListWithoutDescriptionProvider(): Generator + { + yield 'empty list' => ['metadata' => []]; + yield 'other keys only' => ['metadata' => [['key' => 'KBC.datatype.type', 'value' => 'VARCHAR']]]; + yield 'empty description' => ['metadata' => [['key' => 'KBC.description', 'value' => '']]]; + yield 'first item empty' => ['metadata' => [ + ['key' => 'KBC.description', 'value' => ''], + ['key' => 'KBC.description', 'value' => 'second'], + ]]; + } +} diff --git a/libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php b/libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php deleted file mode 100644 index fbc065fa7..000000000 --- a/libs/output-mapping/tests/Writer/Table/TableDefinition/CreateTableDefinitionDescriptionEnricherTest.php +++ /dev/null @@ -1,183 +0,0 @@ -logHandler = new TestHandler(); - $this->logger = new Logger('test', [$this->logHandler]); - } - - public function enrichProvider(): Generator - { - yield 'table description only' => [ - 'requestData' => [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [['name' => 'col1']], - ], - 'descriptions' => new TableDescription(self::TABLE_ID, 'table desc', []), - 'expectedRequestData' => [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [['name' => 'col1']], - 'description' => 'table desc', - ], - ]; - - yield 'column description on a typed column keeps the type' => [ - 'requestData' => [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [ - [ - 'name' => 'col1', - 'definition' => ['type' => 'VARCHAR', 'nullable' => true], - 'basetype' => 'STRING', - ], - ['name' => 'col2', 'definition' => ['type' => 'NUMBER']], - ], - ], - 'descriptions' => new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc']), - 'expectedRequestData' => [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [ - [ - 'name' => 'col1', - 'definition' => [ - 'type' => 'VARCHAR', - 'nullable' => true, - 'description' => 'col1 desc', - ], - 'basetype' => 'STRING', - ], - ['name' => 'col2', 'definition' => ['type' => 'NUMBER']], - ], - ], - ]; - - yield 'column description on a column without definition' => [ - 'requestData' => [ - 'name' => 'table', - 'primaryKeysNames' => ['col1'], - 'columns' => [ - ['name' => 'col1'], - ['name' => 'col2'], - ], - ], - 'descriptions' => new TableDescription( - self::TABLE_ID, - 'table desc', - ['col1' => 'col1 desc', 'col2' => 'col2 desc'], - ), - 'expectedRequestData' => [ - 'name' => 'table', - 'primaryKeysNames' => ['col1'], - 'columns' => [ - // only a description, no type - the table stays non-typed - ['name' => 'col1', 'definition' => ['description' => 'col1 desc']], - ['name' => 'col2', 'definition' => ['description' => 'col2 desc']], - ], - 'description' => 'table desc', - ], - ]; - - yield 'empty descriptions leave the payload untouched' => [ - 'requestData' => [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [['name' => 'col1']], - ], - 'descriptions' => new TableDescription(self::TABLE_ID, null, []), - 'expectedRequestData' => [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [['name' => 'col1']], - ], - ]; - } - - /** @dataProvider enrichProvider */ - public function testEnrich( - array $requestData, - TableDescription $descriptions, - array $expectedRequestData, - ): void { - $enricher = new CreateTableDefinitionDescriptionEnricher($this->logger); - - self::assertSame($expectedRequestData, $enricher->enrich($requestData, $descriptions)); - self::assertFalse($this->logHandler->hasWarningRecords()); - } - - public function testDescriptionOfUnknownColumnIsSkippedAndLogged(): void - { - $enricher = new CreateTableDefinitionDescriptionEnricher($this->logger); - - $requestData = $enricher->enrich( - [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [['name' => 'col1']], - ], - new TableDescription( - self::TABLE_ID, - null, - ['col1' => 'col1 desc', 'unknown1' => 'unknown1 desc', 'unknown2' => 'unknown2 desc'], - ), - ); - - // a column which is not part of the payload must never be appended, it does not exist in the data - self::assertSame( - [ - 'name' => 'table', - 'primaryKeysNames' => [], - 'columns' => [['name' => 'col1', 'definition' => ['description' => 'col1 desc']]], - ], - $requestData, - ); - self::assertTrue($this->logHandler->hasWarningThatContains(sprintf( - 'Cannot store description of column(s) "unknown1", "unknown2" of table "%s", ' - . 'the column(s) do not exist.', - self::TABLE_ID, - ))); - } - - /** - * The payload of a non-typed table must stay a JSON array of objects, so the enrichment must not turn - * the column list into a JSON object. - */ - public function testEnrichedNonTypedPayloadKeepsColumnsAsJsonArray(): void - { - $enricher = new CreateTableDefinitionDescriptionEnricher($this->logger); - - $requestData = $enricher->enrich( - (new TableDefinitionFromColumns('table', ['Id', 'Name'], ['Id']))->getRequestData(), - new TableDescription(self::TABLE_ID, 'table desc', ['Name' => 'Name desc']), - ); - - self::assertSame( - '[{"name":"Id"},{"name":"Name","definition":{"description":"Name desc"}}]', - json_encode($requestData['columns']), - ); - } -} diff --git a/libs/output-mapping/tests/Writer/Table/TableDefinition/TableDefinitionDescriptionsTest.php b/libs/output-mapping/tests/Writer/Table/TableDefinition/TableDefinitionDescriptionsTest.php new file mode 100644 index 000000000..74b742284 --- /dev/null +++ b/libs/output-mapping/tests/Writer/Table/TableDefinition/TableDefinitionDescriptionsTest.php @@ -0,0 +1,175 @@ +logHandler = new TestHandler(); + $this->logger = new Logger('test', [$this->logHandler]); + } + + public function testRequestDataWithoutDescriptionsIsUnchanged(): void + { + $tableDefinition = new TableDefinitionFromColumns('table', ['col1', 'col2'], []); + + self::assertSame( + [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1'], ['name' => 'col2']], + ], + $tableDefinition->getRequestData(), + ); + } + + public function testEmptyDescriptionsAddNothing(): void + { + $tableDefinition = new TableDefinitionFromColumns('table', ['col1'], []); + $tableDefinition->setDescriptions(new TableDescription(self::TABLE_ID, null, []), $this->logger); + + self::assertSame( + [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1']], + ], + $tableDefinition->getRequestData(), + ); + } + + /** + * A `definition` holding only a description carries no type, so the table stays non-typed. + */ + public function testDescriptionsOnANonTypedDefinition(): void + { + $tableDefinition = new TableDefinitionFromColumns('table', ['col1', 'col2'], ['col1']); + $tableDefinition->setDescriptions( + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + $this->logger, + ); + + self::assertSame( + [ + 'name' => 'table', + 'primaryKeysNames' => ['col1'], + 'columns' => [ + ['name' => 'col1', 'definition' => ['description' => 'col1 desc']], + ['name' => 'col2'], + ], + 'description' => 'table desc', + ], + $tableDefinition->getRequestData(), + ); + } + + public function testDescriptionOnATypedColumnKeepsTheType(): void + { + $tableDefinition = new TableDefinition(new TableDefinitionColumnFactory([], 'snowflake', false)); + $tableDefinition->setTableName('table'); + $tableDefinition->addColumn('col1', (new GenericStorage('varchar', ['length' => '25']))->toMetadata()); + $tableDefinition->addColumn('col2', (new GenericStorage('int'))->toMetadata()); + $tableDefinition->setDescriptions( + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + $this->logger, + ); + + $requestData = $tableDefinition->getRequestData(); + + self::assertSame('table desc', $requestData['description']); + self::assertSame( + ['name' => 'col1', 'basetype' => 'STRING', 'definition' => ['description' => 'col1 desc']], + $requestData['columns'][0], + ); + self::assertSame(['name' => 'col2', 'basetype' => 'INTEGER'], $requestData['columns'][1]); + } + + public function testDescriptionsOnADefinitionFromSchema(): void + { + $tableDefinition = new TableDefinitionFromSchema( + 'table', + [ + new MappingFromConfigurationSchemaColumn([ + 'name' => 'col1', + 'data_type' => ['base' => ['type' => 'STRING']], + ]), + ], + 'snowflake', + ); + $tableDefinition->setDescriptions( + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + $this->logger, + ); + + $requestData = $tableDefinition->getRequestData(); + + self::assertSame('table desc', $requestData['description']); + self::assertSame('col1 desc', $requestData['columns'][0]['definition']['description']); + self::assertSame('STRING', $requestData['columns'][0]['basetype']); + } + + /** + * A description of a column which is not part of the payload must never append a new column entry - that + * would create a column the data does not have. + */ + public function testDescriptionOfAnUnknownColumnIsSkippedWithWarning(): void + { + $tableDefinition = new TableDefinitionFromColumns('table', ['col1'], []); + $tableDefinition->setDescriptions( + new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc', 'nope' => 'nope desc']), + $this->logger, + ); + + self::assertSame( + [ + 'name' => 'table', + 'primaryKeysNames' => [], + 'columns' => [['name' => 'col1', 'definition' => ['description' => 'col1 desc']]], + ], + $tableDefinition->getRequestData(), + ); + + self::assertTrue($this->logHandler->hasWarningThatContains(sprintf( + 'Cannot store description of column(s) "nope" of table "%s", the column(s) do not exist.', + self::TABLE_ID, + ))); + } + + public function testKnownColumnsAreNotReportedAsMissing(): void + { + $tableDefinition = new TableDefinitionFromColumns('table', ['col1'], []); + $tableDefinition->setDescriptions( + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + $this->logger, + ); + + $tableDefinition->getRequestData(); + + self::assertFalse($this->logHandler->hasWarningThatContains('Cannot store description of column(s)')); + } +} From 3f4582f484eec2e34486512b562f9f0d85b9adbe Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 3 Aug 2026 09:43:43 +0200 Subject: [PATCH 09/11] test(output-mapping): fold the description coverage into existing tests The dedicated functional tests repeated setups the suite already pays for. The coverage moves to tests which run the same paths anyway, so it costs no extra Storage jobs: - BigqueryTableDescriptionTest is gone; there is no BigQuery/Snowflake difference in what output mapping does, and TableDefinitionV2BigQueryTest already runs the same table twice - the typed create/update paths are asserted in testWriterCreateTableDefinition and testWriterUpdateTableDefinitionWithBaseTypes, which store the description on a table that exists before the run - testAddMissingColumnTableDefinition covers a column added by the same run, so its description must not be reported as belonging to a non-existent column - the workspace path is asserted in testSnowflakeTableOutputMapping instead of a second test paying for its own initWorkspace() - TableDescriptionWriterTest keeps only what needs real Storage: 11 methods down to 6, with the repeated-unchanged-run case folded into the update test The three configuration shapes carrying a description are resolution logic and stay covered by the unit tests. --- .../Storage/BigqueryTableDescriptionTest.php | 146 -------------- .../tests/Writer/TableDefinitionTest.php | 118 +++-------- .../Writer/TableDefinitionV2BigQueryTest.php | 13 ++ .../tests/Writer/TableDefinitionV2Test.php | 163 ++------------- .../Writer/TableDescriptionWriterTest.php | 189 ++---------------- .../Writer/Workspace/WriterWorkspaceTest.php | 59 +----- 6 files changed, 92 insertions(+), 596 deletions(-) delete mode 100644 libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php diff --git a/libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php b/libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php deleted file mode 100644 index 7d751e257..000000000 --- a/libs/output-mapping/tests/Storage/BigqueryTableDescriptionTest.php +++ /dev/null @@ -1,146 +0,0 @@ -emptyBigqueryOutputBucketId . '.tableDescription'; - - $this->uploadTable($tableId, 'table description', 'Id description'); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertSame('table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - - #[NeedsEmptyBigqueryOutputBucket] - public function testDescriptionIsUpdatedOnExistingTable(): void - { - $tableId = $this->emptyBigqueryOutputBucketId . '.tableDescription'; - - $this->uploadTable($tableId, 'table description', 'Id description'); - $this->uploadTable($tableId, 'updated table description', 'updated Id description'); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertTrue($tableDetail['isDescriptionSystemManaged']); - self::assertSame('updated table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('updated Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - - /** - * Storage rejects a table-definition patch without any effective change with 400, so an unchanged repeated - * run must not send one. - */ - #[NeedsEmptyBigqueryOutputBucket] - public function testRepeatedRunWithUnchangedDescriptionSucceeds(): void - { - $tableId = $this->emptyBigqueryOutputBucketId . '.tableDescription'; - - $this->uploadTable($tableId, 'table description', 'Id description'); - $this->uploadTable($tableId, 'table description', 'Id description'); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertSame('table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - - private function uploadTable(string $tableId, string $tableDescription, string $columnDescription): void - { - $root = $this->temp->getTmpFolder(); - file_put_contents($root . '/upload/tableDescription.csv', "\"1\",\"bob\"\n\"2\",\"alice\"\n"); - - $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( - configuration: new OutputMappingSettings( - configuration: [ - 'mapping' => [ - [ - 'source' => 'tableDescription.csv', - 'destination' => $tableId, - 'description' => $tableDescription, - 'schema' => [ - [ - 'name' => 'Id', - 'data_type' => ['base' => ['type' => 'STRING']], - 'description' => $columnDescription, - ], - [ - 'name' => 'Name', - 'data_type' => ['base' => ['type' => 'STRING']], - ], - ], - ], - ], - ], - sourcePathPrefix: 'upload', - storageApiToken: $this->clientWrapper->getToken(), - isFailedJob: false, - dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_AUTHORITATIVE, - ), - systemMetadata: new SystemMetadata(['componentId' => 'foo']), - ); - - self::assertCount(1, $tableQueue->waitForAll()); - } - - private function getColumnDescription(array $tableDetail, string $columnName): ?string - { - $definition = $tableDetail['definition'] ?? []; - self::assertIsArray($definition); - $columns = $definition['columns'] ?? []; - self::assertIsArray($columns); - - foreach ($columns as $column) { - self::assertIsArray($column); - if ($column['name'] === $columnName) { - $columnDefinition = $column['definition'] ?? []; - self::assertIsArray($columnDefinition); - - return $columnDefinition['description'] ?? null; - } - } - - return null; - } - - protected function initClient(?string $branchId = null): void - { - $clientOptions = (new ClientOptions()) - ->setUrl((string) getenv('BIGQUERY_STORAGE_API_URL')) - ->setToken((string) getenv('BIGQUERY_STORAGE_API_TOKEN')) - ->setAuthType(AuthType::STORAGE_TOKEN) - ->setBranchId($branchId) - ->setBackoffMaxTries(1) - ->setJobPollRetryDelay(function () { - return 1; - }) - ->setUserAgent(implode('::', Test::describe($this))); - $this->clientWrapper = new ClientWrapper($clientOptions); - $tokenInfo = $this->clientWrapper->getBranchClient()->verifyToken(); - print(sprintf( - 'Authorized as "%s (%s)" to project "%s (%s)" at "%s" stack.', - $tokenInfo['description'], - $tokenInfo['id'], - $tokenInfo['owner']['name'], - $tokenInfo['owner']['id'], - $this->clientWrapper->getBranchClient()->getApiUrl(), - )); - } -} diff --git a/libs/output-mapping/tests/Writer/TableDefinitionTest.php b/libs/output-mapping/tests/Writer/TableDefinitionTest.php index 2e1327059..43b328f3b 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionTest.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionTest.php @@ -89,6 +89,9 @@ public function testWriterCreateTableDefinition( $value = is_string($value) ? sprintf($value, $this->emptyOutputBucketId) : $value; }); $config = $configTemplate; + // the descriptions ride along in the create payload of the table definition + $config['description'] = 'table description'; + $config['column_metadata']['Id'][] = ['key' => 'KBC.description', 'value' => 'Id description']; $root = $this->temp->getTmpFolder(); file_put_contents( @@ -123,6 +126,12 @@ public function testWriterCreateTableDefinition( self::assertDataTypeDefinition($tableDetails['columnMetadata']['Name'], $expectedTypes['Name']); self::assertDataTypeDefinition($tableDetails['columnMetadata']['birthweight'], $expectedTypes['birthweight']); self::assertDataTypeDefinition($tableDetails['columnMetadata']['created'], $expectedTypes['created']); + + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + 'Id description', + $this->getColumnDescriptions($tableDetails['definition']['columns'])['Id'], + ); } public function configProvider(): iterable @@ -236,8 +245,13 @@ public function testWriterUpdateTableDefinitionWithBaseTypes(bool $incrementalFl 'incremental' => $incrementalFlag, 'columns' => ['Id', 'Name', 'birthweight', 'created'], 'primary_key' => ['Id', 'Name'], + // the table exists before the run, so the descriptions go through the table-definition update + 'description' => 'table description', 'column_metadata' => [ - 'Id' => $idDatatype->toMetadata(), + 'Id' => array_merge( + $idDatatype->toMetadata(), + [['key' => 'KBC.description', 'value' => 'Id description']], + ), 'Name' => $nameDatatype->toMetadata(), 'birthweight' => $birthweightDatatype->toMetadata(), 'created' => $created->toMetadata(), @@ -296,6 +310,16 @@ function (array $job) use ($runId) { }, ); + // StoragePreparer stores the descriptions through a table-definition update before the load + self::assertCount(1, array_filter( + $writerJobs, + fn(array $job) => $job['operationName'] === 'tableDefinitionUpdate', + )); + $writerJobs = array_filter( + $writerJobs, + fn(array $job) => $job['operationName'] !== 'tableDefinitionUpdate', + ); + self::assertCount(4, $writerJobs); // tableColumnAdd jobs @@ -345,6 +369,13 @@ function (array $job) use ($runId) { 'length' => null, 'nullable' => true, ]); + + self::assertTrue($tableDetails['isDescriptionSystemManaged']); + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + 'Id description', + $this->getColumnDescriptions($tableDetails['definition']['columns'])['Id'], + ); } /** @@ -689,91 +720,6 @@ private static function assertTablePrimaryKeyAddJob(array $jobData, array $expec self::assertSame($expectedPk, $jobData['operationParams']['columns']); } - /** - * The legacy native-types path builds the table definition from `column_metadata`, and the descriptions - * ride along in the create payload. A following run finds the table there and goes through the - * table-definition update instead. - */ - #[NeedsEmptyOutputBucket] - public function testDescriptionIsStoredAndUpdatedOnTypedTable(): void - { - $tableId = $this->emptyOutputBucketId . '.tableDefinition'; - - $this->uploadTableWithColumnMetadata($tableId, 'table description', 'Id description'); - - $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertTrue($tableDetails['isTyped']); - self::assertSame('table description', $tableDetails['definition']['description'] ?? null); - self::assertSame( - ['Id' => 'Id description', 'Name' => null], - $this->getColumnDescriptions($tableDetails['definition']['columns']), - ); - - $this->uploadTableWithColumnMetadata($tableId, 'updated table description', 'updated Id description'); - - $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertTrue($tableDetails['isDescriptionSystemManaged']); - self::assertSame('updated table description', $tableDetails['definition']['description'] ?? null); - self::assertSame( - ['Id' => 'updated Id description', 'Name' => null], - $this->getColumnDescriptions($tableDetails['definition']['columns']), - ); - - // Storage rejects a patch without any effective change with 400, so an unchanged run must not send one - $this->uploadTableWithColumnMetadata($tableId, 'updated table description', 'updated Id description'); - } - - private function uploadTableWithColumnMetadata( - string $tableId, - string $tableDescription, - string $idDescription, - ): void { - $config = [ - 'source' => 'tableDefinition.csv', - 'destination' => $tableId, - 'columns' => ['Id', 'Name'], - 'description' => $tableDescription, - 'metadata' => [ - [ - 'key' => 'KBC.datatype.backend', - 'value' => 'snowflake', - ], - ], - 'column_metadata' => [ - 'Id' => [ - ['key' => 'KBC.datatype.type', 'value' => Snowflake::TYPE_INTEGER], - ['key' => 'KBC.datatype.basetype', 'value' => 'INTEGER'], - ['key' => 'KBC.description', 'value' => $idDescription], - ], - 'Name' => [ - ['key' => 'KBC.datatype.type', 'value' => Snowflake::TYPE_TEXT], - ['key' => 'KBC.datatype.basetype', 'value' => 'STRING'], - ], - ], - ]; - - file_put_contents( - $this->temp->getTmpFolder() . '/upload/tableDefinition.csv', - "\"1\",\"bob\"\n\"2\",\"alice\"\n", - ); - - $tableQueue = $this->getTableLoader( - logger: $this->testLogger, - strategyFactory: $this->getLocalStagingFactory(logger: $this->testLogger), - )->uploadTables( - configuration: new OutputMappingSettings( - configuration: ['mapping' => [$config]], - sourcePathPrefix: 'upload', - storageApiToken: $this->clientWrapper->getToken(), - isFailedJob: false, - dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_AUTHORITATIVE, - ), - systemMetadata: new SystemMetadata(['componentId' => 'foo']), - ); - - self::assertCount(1, $tableQueue->waitForAll()); - } - /** * @param array $columns * @return array column name => description diff --git a/libs/output-mapping/tests/Writer/TableDefinitionV2BigQueryTest.php b/libs/output-mapping/tests/Writer/TableDefinitionV2BigQueryTest.php index 798ea0fca..f5cf33d7b 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionV2BigQueryTest.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionV2BigQueryTest.php @@ -109,9 +109,11 @@ public function testWriteTableOutputMappingExistingTable(): void [ 'source' => 'tableDefinition.csv', 'destination' => $this->emptyBigqueryOutputBucketId . '.tableDefinitionBackendType', + 'description' => 'table description', 'schema' => [ [ 'name' => 'Id', + 'description' => 'Id description', 'data_type' => [ 'base' => [ 'type' => BaseType::NUMERIC, @@ -196,6 +198,17 @@ public function testWriteTableOutputMappingExistingTable(): void self::assertCount(1, $tables); self::assertEquals($this->emptyBigqueryOutputBucketId . '.tableDefinitionBackendType', $tables[0]['id']); self::assertNotEmpty($jobIds[0]); + + // The descriptions ride along in the create payload of the first run. The second run finds the table + // there and must not send an unchanged table-definition patch, which Storage rejects with 400. + $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tables[0]['id']); + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + $idColumn = array_values(array_filter( + $tableDetails['definition']['columns'], + fn($column) => $column['name'] === 'Id', + )); + self::assertCount(1, $idColumn); + self::assertSame('Id description', $idColumn[0]['definition']['description'] ?? null); } public function configProvider(): iterable diff --git a/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php b/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php index b8aabf260..d3a5c6e52 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionV2Test.php @@ -245,9 +245,12 @@ public function testAddMissingColumnTableDefinition(): void $config = [ 'source' => 'tableDefinition.csv', 'destination' => $this->emptyOutputBucketId . '.tableDefinition', + // the table exists before the run, so the descriptions go through the table-definition update + 'description' => 'table description', 'schema' => [ [ 'name' => 'Id', + 'description' => 'Id description', 'data_type' => [ 'base' => [ 'type' => BaseType::NUMERIC, @@ -264,6 +267,7 @@ public function testAddMissingColumnTableDefinition(): void ], [ 'name' => 'newColumn', + 'description' => 'newColumn description', 'data_type' => [ 'base' => [ 'type' => BaseType::STRING, @@ -281,7 +285,7 @@ public function testAddMissingColumnTableDefinition(): void EOT, ); - $tableQueue = $this->getTableLoader()->uploadTables( + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( configuration: new OutputMappingSettings( configuration: ['mapping' => [$config]], sourcePathPrefix: 'upload', @@ -304,6 +308,16 @@ public function testAddMissingColumnTableDefinition(): void 'nullable' => true, ], ); + + // A column added by the same run is already part of the table when the descriptions are stored, so its + // description must not be reported as belonging to a non-existent column. + self::assertTrue($tableDetails['isDescriptionSystemManaged']); + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + ['Id' => 'Id description', 'Name' => null, 'newColumn' => 'newColumn description'], + $this->getColumnDescriptions($tableDetails['definition']['columns']), + ); + self::assertFalse($this->testHandler->hasWarningThatContains('Cannot store description of column(s)')); } public function configProvider(): iterable @@ -769,153 +783,6 @@ public function testSaveTableAndColumnMetadata(): void self::assertSame('storage', array_values($filteredColumnNameMetadata)[0]['provider']); } - /** - * On the second run the table already exists, so the description goes through StoragePreparer, which diffs - * it against the value stored in the table definition. A typed table exposes it in the same place a - * non-typed one does, otherwise the diff would keep re-sending an unchanged patch. - */ - #[NeedsEmptyOutputBucket] - public function testDescriptionIsUpdatedOnExistingTypedTable(): void - { - $tableId = $this->emptyOutputBucketId . '.test1'; - - $this->uploadTableWithSchema($tableId, 'table description', 'Id description'); - $this->uploadTableWithSchema($tableId, 'updated table description', 'updated Id description'); - - $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertTrue($tableDetails['isTyped']); - self::assertTrue($tableDetails['isDescriptionSystemManaged']); - self::assertSame('updated table description', $tableDetails['definition']['description'] ?? null); - self::assertSame( - ['Id' => 'updated Id description', 'Name' => null], - $this->getColumnDescriptions($tableDetails['definition']['columns']), - ); - - // Storage rejects a patch without any effective change with 400, so an unchanged third run must not - // send one at all - $this->uploadTableWithSchema($tableId, 'updated table description', 'updated Id description'); - } - - #[NeedsEmptyOutputBucket] - public function testDescriptionIsNotOverwrittenOnUserManagedTypedTable(): void - { - $tableId = $this->emptyOutputBucketId . '.test1'; - - $this->uploadTableWithSchema($tableId, 'table description', 'Id description'); - - // the user takes over the description - $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition($tableId, [ - 'description' => 'description set by the user', - 'isDescriptionSystemManaged' => false, - 'columns' => [ - ['name' => 'Id', 'description' => 'Id description set by the user'], - ], - ]); - - $this->uploadTableWithSchema($tableId, 'description from component', 'Id description from component'); - - $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertFalse($tableDetails['isDescriptionSystemManaged']); - self::assertSame('description set by the user', $tableDetails['definition']['description'] ?? null); - self::assertSame( - ['Id' => 'Id description set by the user', 'Name' => null], - $this->getColumnDescriptions($tableDetails['definition']['columns']), - ); - - self::assertTrue($this->testHandler->hasInfoThatContains(sprintf( - 'Description of table "%s" is managed by the user, keeping the current value.', - $tableId, - ))); - } - - /** - * A column added by the same run must already be part of the table when the descriptions are stored, - * otherwise its description is reported as belonging to a non-existent column and dropped. - */ - #[NeedsEmptyOutputBucket] - public function testDescriptionOfColumnAddedInSecondRun(): void - { - $tableId = $this->emptyOutputBucketId . '.test1'; - - $this->uploadTableWithSchema($tableId, 'table description', 'Id description'); - $this->uploadTableWithSchema($tableId, 'table description', 'Id description', 'foo description'); - - $tableDetails = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertSame( - ['Id' => 'Id description', 'Name' => null, 'foo' => 'foo description'], - $this->getColumnDescriptions($tableDetails['definition']['columns']), - ); - - self::assertFalse($this->testHandler->hasWarningThatContains( - 'Cannot store description of column(s)', - )); - } - - /** - * Writes Id/Name, plus a third `foo` column when its description is given. - */ - private function uploadTableWithSchema( - string $tableId, - ?string $tableDescription, - ?string $idDescription, - ?string $fooDescription = null, - ): void { - $schema = [ - $this->schemaColumn('Id', $idDescription), - $this->schemaColumn('Name'), - ]; - $csv = "\"1\",\"bob\"\n\"2\",\"alice\"\n"; - - if ($fooDescription !== null) { - $schema[] = $this->schemaColumn('foo', $fooDescription); - $csv = "\"1\",\"bob\",\"firstFoo\"\n\"2\",\"alice\",\"secondFoo\"\n"; - } - - $config = [ - 'source' => 'table.csv', - 'destination' => $tableId, - 'schema' => $schema, - ]; - if ($tableDescription !== null) { - $config['description'] = $tableDescription; - } - - file_put_contents($this->temp->getTmpFolder() . '/upload/table.csv', $csv); - - $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( - configuration: new OutputMappingSettings( - configuration: ['mapping' => [$config]], - sourcePathPrefix: 'upload', - storageApiToken: $this->clientWrapper->getToken(), - isFailedJob: false, - dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_AUTHORITATIVE, - ), - systemMetadata: new SystemMetadata(['componentId' => 'foo']), - ); - - self::assertCount(1, $tableQueue->waitForAll()); - } - - /** - * @return array - */ - private function schemaColumn(string $name, ?string $description = null): array - { - $column = [ - 'name' => $name, - 'data_type' => [ - 'base' => [ - 'type' => 'STRING', - ], - ], - ]; - if ($description !== null) { - $column['description'] = $description; - } - - return $column; - } - /** * @param array $columns * @return array column name => description diff --git a/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php index 2ff48380d..9ed120f8c 100644 --- a/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php +++ b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php @@ -4,7 +4,6 @@ namespace Keboola\OutputMapping\Tests\Writer; -use Generator; use Keboola\OutputMapping\Exception\InvalidOutputException; use Keboola\OutputMapping\OutputMappingSettings; use Keboola\OutputMapping\SystemMetadata; @@ -14,27 +13,16 @@ class TableDescriptionWriterTest extends AbstractTestCase { - /** the dedicated `description` field of the mapping */ - private const SOURCE_DEDICATED_FIELD = 'description'; - - /** legacy `KBC.description` in the `table_metadata` key => value map */ - private const SOURCE_TABLE_METADATA = 'table_metadata'; - - /** legacy `KBC.description` in the `metadata` list of {key, value} items */ - private const SOURCE_METADATA_LIST = 'metadata'; - /** - * The description reaches Storage the same way regardless of which of the three configuration shapes - * carried it, and Storage is the only writer of the mirrored `KBC.description` metadata row. - * - * @dataProvider descriptionSourceProvider + * Storage is the only writer of the mirrored `KBC.description` metadata row - output mapping stores the + * description natively and never writes the key itself. */ #[NeedsEmptyOutputBucket] - public function testDescriptionIsStoredOnCreatedTable(string $descriptionSource): void + public function testDescriptionIsStoredOnCreatedTable(): void { $tableId = $this->emptyOutputBucketId . '.tableDescription'; - $this->uploadTable($tableId, 'table description', 'Id description', $descriptionSource); + $this->uploadTable($tableId, 'table description', 'Id description'); $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); // `columns` in the mapping creates the table through a non-typed table-definition payload; which @@ -46,69 +34,18 @@ public function testDescriptionIsStoredOnCreatedTable(string $descriptionSource) $this->assertSingleStorageDescriptionRow($tableDetail['metadata'], 'table description'); } - public static function descriptionSourceProvider(): Generator - { - yield 'dedicated description field' => ['descriptionSource' => self::SOURCE_DEDICATED_FIELD]; - yield 'table_metadata KBC.description' => ['descriptionSource' => self::SOURCE_TABLE_METADATA]; - yield 'metadata list KBC.description' => ['descriptionSource' => self::SOURCE_METADATA_LIST]; - } - - /** - * A table written without a manifest and without `columns` is created by the load job itself - * (CreateAndLoadTableTask). There is no create-table-definition payload the description could be part of, - * so it is stored once the load finishes and the table surely exists. - */ - #[NeedsEmptyOutputBucket] - public function testDescriptionIsStoredOnTableCreatedByLoadJob(): void - { - $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; - - $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertSame(['Id', 'Name'], $tableDetail['columns']); - self::assertSame('table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - /** * The second run finds the table already there, so the description goes through StoragePreparer, which - * diffs it against the stored value read from `definition.description`. A table created by the load job - * must expose the description in the same place as one created through a table definition, otherwise the - * diff always sees null and Storage rejects the unchanged patch with 400. + * diffs it against the stored value read from `definition.description`. Storage rejects a table-definition + * patch without any effective change with 400 "No table definition changes were provided.", so a repeated + * run with an unchanged description must not send the patch at all. */ - #[NeedsEmptyOutputBucket] - public function testRepeatedRunOnTableCreatedByLoadJobSucceeds(): void - { - $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; - - $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); - $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertSame('table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - - #[NeedsEmptyOutputBucket] - public function testDescriptionIsUpdatedOnTableCreatedByLoadJob(): void - { - $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; - - $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); - $this->uploadTableWithoutColumns($tableId, 'updated table description', 'updated Id description'); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertTrue($tableDetail['isDescriptionSystemManaged']); - self::assertSame('updated table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('updated Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - #[NeedsEmptyOutputBucket] public function testDescriptionIsUpdatedOnSystemManagedTable(): void { $tableId = $this->emptyOutputBucketId . '.tableDescription'; + $this->uploadTable($tableId, 'table description', 'Id description'); $this->uploadTable($tableId, 'table description', 'Id description'); $this->uploadTable($tableId, 'updated table description', 'updated Id description'); @@ -119,18 +56,21 @@ public function testDescriptionIsUpdatedOnSystemManagedTable(): void } /** - * Storage rejects a table-definition patch without any effective change with 400 "No table definition - * changes were provided.", so a repeated run with an unchanged description must not send the patch at all. + * A table written without a manifest and without `columns` is created by the load job itself + * (CreateAndLoadTableTask). There is no create-table-definition payload the description could be part of, + * so it is stored once the load finishes and the table surely exists. It must land in the same place a + * table definition puts it, otherwise the diff of the next run would keep re-sending an unchanged patch. */ #[NeedsEmptyOutputBucket] - public function testRepeatedRunWithUnchangedDescriptionSucceeds(): void + public function testDescriptionIsStoredOnTableCreatedByLoadJob(): void { - $tableId = $this->emptyOutputBucketId . '.tableDescription'; + $tableId = $this->emptyOutputBucketId . '.tableDescriptionNoManifest'; - $this->uploadTable($tableId, 'table description', 'Id description'); - $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); + $this->uploadTableWithoutColumns($tableId, 'table description', 'Id description'); $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); + self::assertSame(['Id', 'Name'], $tableDetail['columns']); self::assertSame('table description', $tableDetail['definition']['description'] ?? null); self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); } @@ -167,18 +107,14 @@ public function testDescriptionIsNotOverwrittenOnUserManagedTable(): void /** * An empty description means "nothing to store", so a run which no longer carries one keeps the value * already in Storage instead of clearing it. - * - * @dataProvider descriptionRemovedProvider */ #[NeedsEmptyOutputBucket] - public function testDescriptionIsKeptWhenNoLongerInConfiguration( - ?string $tableDescription, - ?string $columnDescription, - ): void { + public function testDescriptionIsKeptWhenNoLongerInConfiguration(): void + { $tableId = $this->emptyOutputBucketId . '.tableDescription'; $this->uploadTable($tableId, 'table description', 'Id description'); - $this->uploadTable($tableId, $tableDescription, $columnDescription); + $this->uploadTable($tableId, null, null); $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); self::assertTrue($tableDetail['isDescriptionSystemManaged']); @@ -186,18 +122,6 @@ public function testDescriptionIsKeptWhenNoLongerInConfiguration( self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); } - public static function descriptionRemovedProvider(): Generator - { - yield 'description dropped from the configuration' => [ - 'tableDescription' => null, - 'columnDescription' => null, - ]; - yield 'description sent as an empty string' => [ - 'tableDescription' => '', - 'columnDescription' => '', - ]; - } - /** * A description of a column the data does not have must never create that column - it is reported and * skipped, and the rest of the load goes through. @@ -207,7 +131,7 @@ public function testDescriptionOfMissingColumnIsSkippedWithWarning(): void { $tableId = $this->emptyOutputBucketId . '.tableDescription'; - $this->uploadTable($tableId, 'table description', 'Id description', self::SOURCE_DEDICATED_FIELD, [ + $this->uploadTable($tableId, 'table description', 'Id description', [ 'nope' => [ ['key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, 'value' => 'description of nothing'], ], @@ -282,58 +206,6 @@ public function testFailedLoadDropsFreshlyCreatedTableWithDescription(): void self::assertFalse($this->testHandler->hasWarningThatContains('was not stored')); } - /** - * A table which existed before the failed load keeps its data and its description - only a table freshly - * created by the very same run may be dropped. - */ - #[NeedsEmptyOutputBucket] - public function testFailedLoadKeepsPreExistingTableWithDescription(): void - { - $tableId = $this->emptyOutputBucketId . '.tableDescription'; - - $this->uploadTable($tableId, 'table description', 'Id description'); - - $root = $this->temp->getTmpFolder(); - file_put_contents( - $root . '/upload/tableDescription.csv', - "\"test\",\"test\"\n\"aabb\",\"ccdd\",\"dddd\"\n", - ); - - $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( - configuration: new OutputMappingSettings( - configuration: [ - 'mapping' => [ - [ - 'source' => 'tableDescription.csv', - 'destination' => $tableId, - 'columns' => ['Id', 'Name'], - 'description' => 'table description', - ], - ], - ], - sourcePathPrefix: 'upload', - storageApiToken: $this->clientWrapper->getToken(), - isFailedJob: false, - dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, - ), - systemMetadata: new SystemMetadata(['componentId' => 'foo']), - ); - - try { - $tableQueue->waitForAll(); - self::fail('Must throw exception'); - } catch (InvalidOutputException $e) { - self::assertStringContainsString(sprintf('Failed to load table "%s"', $tableId), $e->getMessage()); - } - - self::assertTrue($this->clientWrapper->getTableAndFileStorageClient()->tableExists($tableId)); - self::assertFalse($this->testHandler->hasWarningThatContains('Dropping table')); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); - self::assertSame('table description', $tableDetail['definition']['description'] ?? null); - self::assertSame('Id description', $this->getColumnDescription($tableDetail, 'Id')); - } - /** * @param array|null $extraColumnMetadata */ @@ -341,7 +213,6 @@ private function uploadTable( string $tableId, ?string $tableDescription, ?string $columnDescription, - string $descriptionSource = self::SOURCE_DEDICATED_FIELD, ?array $extraColumnMetadata = null, ): void { $root = $this->temp->getTmpFolder(); @@ -354,23 +225,7 @@ private function uploadTable( ]; if ($tableDescription !== null) { - $mapping += match ($descriptionSource) { - self::SOURCE_DEDICATED_FIELD => ['description' => $tableDescription], - self::SOURCE_TABLE_METADATA => [ - 'table_metadata' => [ - DescriptionHelper::DESCRIPTION_METADATA_KEY => $tableDescription, - ], - ], - self::SOURCE_METADATA_LIST => [ - 'metadata' => [ - [ - 'key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, - 'value' => $tableDescription, - ], - ], - ], - default => self::fail(sprintf('Unknown description source "%s".', $descriptionSource)), - }; + $mapping['description'] = $tableDescription; } $columnMetadata = $extraColumnMetadata ?? []; diff --git a/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php b/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php index f52ff1cf1..94a1185bc 100644 --- a/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php +++ b/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php @@ -35,6 +35,14 @@ public function testSnowflakeTableOutputMapping(): void 'destination' => $this->emptyOutputBucketId . '.table1a', 'incremental' => true, 'columns' => ['Id'], + // the descriptions are stored the same way regardless of the staging the data came from, but + // workspace unload reaches LoadTableTaskCreator through a different strategy + 'description' => 'table description', + 'column_metadata' => [ + 'Id' => [ + ['key' => 'KBC.description', 'value' => 'Id description'], + ], + ], ], [ 'source' => 'table2a', @@ -97,58 +105,11 @@ public function testSnowflakeTableOutputMapping(): void '"id3","name3","foo3","bar3"', ], ); - } - - /** - * The descriptions are stored through the table-definition API regardless of the staging the data came - * from, but workspace unload reaches LoadTableTaskCreator through a different strategy. - */ - #[NeedsTestTables(2), NeedsEmptyOutputBucket] - public function testSnowflakeTableOutputMappingStoresDescription(): void - { - $this->initWorkspace(); - $factory = $this->getWorkspaceStagingFactory(); - - $this->prepareWorkspaceWithTablesClone($this->testBucketId); - - $tableId = $this->emptyOutputBucketId . '.table1a'; - - $config = [ - 'source' => 'table1a', - 'destination' => $tableId, - 'columns' => ['Id', 'Name'], - 'description' => 'table description', - 'column_metadata' => [ - 'Id' => [ - ['key' => 'KBC.description', 'value' => 'Id description'], - ], - ], - ]; - - file_put_contents( - $this->temp->getTmpFolder() . '/table1a.manifest', - (string) json_encode(['columns' => ['Id', 'Name']]), - ); - $tableQueue = $this->getTableLoader( - logger: $this->testLogger, - strategyFactory: $factory, - )->uploadTables( - configuration: new OutputMappingSettings( - configuration: ['mapping' => [$config]], - sourcePathPrefix: '/', - storageApiToken: $this->clientWrapper->getToken(), - isFailedJob: false, - dataTypeSupport: OutputMappingSettings::DATA_TYPES_SUPPORT_NONE, - ), - systemMetadata: new SystemMetadata(['componentId' => 'foo']), + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable( + $this->emptyOutputBucketId . '.table1a', ); - - self::assertCount(1, $tableQueue->waitForAll()); - - $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable($tableId); self::assertSame('table description', $tableDetail['definition']['description'] ?? null); - $idColumn = array_values(array_filter( $tableDetail['definition']['columns'], fn($column) => $column['name'] === 'Id', From 1efaf136a31c755af429135b9f628c732c3736ad Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 3 Aug 2026 10:04:10 +0200 Subject: [PATCH 10/11] test(output-mapping): keep the legacy typed update test free of descriptions Storing a description issues a table-definition update, and on the CI project that makes Storage report the `created` column as TIMESTAMP_LTZ instead of TIMESTAMP under the "storage" provider, so testWriterUpdateTableDefinitionWithBaseTypes started failing on an assertion unrelated to descriptions. The update path is not specific to how the table definition was built at create time - it always goes through StoragePreparer - and testAddMissingColumnTableDefinition in the new-native-types suite already covers it on an existing typed table. The create payload stays asserted in testWriterCreateTableDefinition. --- .../tests/Writer/TableDefinitionTest.php | 24 +------------------ 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/libs/output-mapping/tests/Writer/TableDefinitionTest.php b/libs/output-mapping/tests/Writer/TableDefinitionTest.php index 43b328f3b..35febc95d 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionTest.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionTest.php @@ -245,13 +245,8 @@ public function testWriterUpdateTableDefinitionWithBaseTypes(bool $incrementalFl 'incremental' => $incrementalFlag, 'columns' => ['Id', 'Name', 'birthweight', 'created'], 'primary_key' => ['Id', 'Name'], - // the table exists before the run, so the descriptions go through the table-definition update - 'description' => 'table description', 'column_metadata' => [ - 'Id' => array_merge( - $idDatatype->toMetadata(), - [['key' => 'KBC.description', 'value' => 'Id description']], - ), + 'Id' => $idDatatype->toMetadata(), 'Name' => $nameDatatype->toMetadata(), 'birthweight' => $birthweightDatatype->toMetadata(), 'created' => $created->toMetadata(), @@ -310,16 +305,6 @@ function (array $job) use ($runId) { }, ); - // StoragePreparer stores the descriptions through a table-definition update before the load - self::assertCount(1, array_filter( - $writerJobs, - fn(array $job) => $job['operationName'] === 'tableDefinitionUpdate', - )); - $writerJobs = array_filter( - $writerJobs, - fn(array $job) => $job['operationName'] !== 'tableDefinitionUpdate', - ); - self::assertCount(4, $writerJobs); // tableColumnAdd jobs @@ -369,13 +354,6 @@ function (array $job) use ($runId) { 'length' => null, 'nullable' => true, ]); - - self::assertTrue($tableDetails['isDescriptionSystemManaged']); - self::assertSame('table description', $tableDetails['definition']['description'] ?? null); - self::assertSame( - 'Id description', - $this->getColumnDescriptions($tableDetails['definition']['columns'])['Id'], - ); } /** From 1fc736cddb0f5b1cedb7f38cb47ce7367dac458f Mon Sep 17 00:00:00 2001 From: zajca Date: Mon, 3 Aug 2026 11:15:20 +0200 Subject: [PATCH 11/11] test(output-mapping): assert the datatype metadata Storage really returns Reinstates the description coverage in testWriterUpdateTableDefinitionWithBaseTypes. Storing a column description on a typed table makes Storage re-read every column from the backend and rewrite its datatype metadata - the description-only shortcut in TableDefinitionUpdateService is gated on a non-typed table, so a typed one takes the syncTypedColumnInfo() branch. `created` is added by basetype, where Storage records the requested Snowflake alias TIMESTAMP with no length; the re-read reports what the column really is, TIMESTAMP_LTZ with length 9. The expectation now says that, rather than the value that only held until something forced a re-read. --- .../tests/Writer/TableDefinitionTest.php | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/libs/output-mapping/tests/Writer/TableDefinitionTest.php b/libs/output-mapping/tests/Writer/TableDefinitionTest.php index 35febc95d..5986331ca 100644 --- a/libs/output-mapping/tests/Writer/TableDefinitionTest.php +++ b/libs/output-mapping/tests/Writer/TableDefinitionTest.php @@ -245,8 +245,13 @@ public function testWriterUpdateTableDefinitionWithBaseTypes(bool $incrementalFl 'incremental' => $incrementalFlag, 'columns' => ['Id', 'Name', 'birthweight', 'created'], 'primary_key' => ['Id', 'Name'], + // the table exists before the run, so the descriptions go through the table-definition update + 'description' => 'table description', 'column_metadata' => [ - 'Id' => $idDatatype->toMetadata(), + 'Id' => array_merge( + $idDatatype->toMetadata(), + [['key' => 'KBC.description', 'value' => 'Id description']], + ), 'Name' => $nameDatatype->toMetadata(), 'birthweight' => $birthweightDatatype->toMetadata(), 'created' => $created->toMetadata(), @@ -305,6 +310,16 @@ function (array $job) use ($runId) { }, ); + // StoragePreparer stores the descriptions through a table-definition update before the load + self::assertCount(1, array_filter( + $writerJobs, + fn(array $job) => $job['operationName'] === 'tableDefinitionUpdate', + )); + $writerJobs = array_filter( + $writerJobs, + fn(array $job) => $job['operationName'] !== 'tableDefinitionUpdate', + ); + self::assertCount(4, $writerJobs); // tableColumnAdd jobs @@ -349,11 +364,23 @@ function (array $job) use ($runId) { 'length' => '38,9', 'nullable' => true, ]); + // A column description on a typed table makes Storage re-read every column from the backend and + // rewrite its datatype metadata (TableDefinitionUpdateService::updateTableDefinitionOnStorageBackend() + // takes the syncTypedColumnInfo() branch, the description-only shortcut is gated on a non-typed + // table). `created` was added by basetype, where Storage recorded the requested Snowflake alias + // TIMESTAMP; the re-read reports the type the column really has. self::assertDataTypeDefinition($tableDetails['columnMetadata']['created'], [ - 'type' => Snowflake::TYPE_TIMESTAMP, - 'length' => null, + 'type' => Snowflake::TYPE_TIMESTAMP_LTZ, + 'length' => '9', 'nullable' => true, ]); + + self::assertTrue($tableDetails['isDescriptionSystemManaged']); + self::assertSame('table description', $tableDetails['definition']['description'] ?? null); + self::assertSame( + 'Id description', + $this->getColumnDescriptions($tableDetails['definition']['columns'])['Id'], + ); } /**