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..a2a7ff179 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/TableDescriptionWriterTest.php tests/Writer/Workspace 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 705ecfd3a..530831a25 100644 --- a/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php +++ b/libs/output-mapping/src/DeferredTasks/LoadTableQueue.php @@ -6,6 +6,9 @@ use Keboola\InputMapping\Table\Result\TableInfo; 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; @@ -20,16 +23,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(); } @@ -66,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], ); } @@ -107,17 +126,32 @@ 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'], ); + $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; } } } + 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) { @@ -126,6 +160,25 @@ public function waitForAll(): array return $jobIds; } + /** + * @param array $tableData table detail as returned by Storage after a successful load + */ + private function applyCreatedTableDescriptions(LoadTableTaskInterface $task, array $tableData): void + { + $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->updateDescriptions(new StorageTableInfo($tableData), $descriptions); + } + public function getTaskCount(): int { return count($this->loadTableTasks); 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/LoadTableTaskCreator.php b/libs/output-mapping/src/LoadTableTaskCreator.php index 41baaedc6..88aa68a5e 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,10 +11,12 @@ 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; use Keboola\OutputMapping\Writer\Table\TableDefinitionFromSchema\TableDefinitionFromSchema; +use Keboola\OutputMapping\Writer\Table\TableDefinitionInterface; use Keboola\StorageApiBranch\ClientWrapper; use Psr\Log\LoggerInterface; @@ -28,12 +29,19 @@ public function __construct(readonly ClientWrapper $clientWrapper, readonly Logg $this->tableCreator = new TableCreator($clientWrapper); } + /** + * @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 +74,8 @@ public function create( $source->getPrimaryKey(), $source->getColumnMetadata(), ); - $this->tableCreator->createTableDefinition($source->getDestination()->getBucketId(), $tableDefinition); - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, true); + $this->createTableDefinition($source, $tableDefinition, $descriptions); + return new LoadTableTaskResult(new LoadTableTask($source->getDestination(), $loadOptions, true)); } elseif ($settings->hasNewNativeTypesFeature() && !$storageSources->didTableExistBefore() && $source->getSchema() @@ -77,8 +85,8 @@ public function create( $source->getSchema(), $storageSources->getBucket()->backend, ); - $this->tableCreator->createTableDefinition($source->getDestination()->getBucketId(), $tableDefinition); - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, true); + $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ů $tableDefinition = new TableDefinitionFromColumns( @@ -86,11 +94,11 @@ public function create( $source->getColumns(), $source->getPrimaryKey(), ); - $this->tableCreator->createTableDefinition($source->getDestination()->getBucketId(), $tableDefinition); - $loadTask = new LoadTableTask($source->getDestination(), $loadOptions, true); + $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 - $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 +108,30 @@ 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; + } + + 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( 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/Mapping/MappingFromConfigurationSchemaColumn.php b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php index a689c22c5..91beba3eb 100644 --- a/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php +++ b/libs/output-mapping/src/Mapping/MappingFromConfigurationSchemaColumn.php @@ -4,6 +4,8 @@ namespace Keboola\OutputMapping\Mapping; +use Keboola\OutputMapping\Writer\Helper\DescriptionHelper; + class MappingFromConfigurationSchemaColumn { public function __construct(private readonly array $mapping) @@ -43,12 +45,32 @@ 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'] ?? []; + return DescriptionHelper::removeDescriptionFromMetadataMap( + $this->mapping['metadata'] ?? [], + 'schema.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'])) { - $metadata['KBC.description'] = $this->mapping['description']; + return DescriptionHelper::normalizeDescription($this->mapping['description']); } - return $metadata; + + 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 bf41e2197..0341e8573 100644 --- a/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php +++ b/libs/output-mapping/src/Mapping/MappingFromProcessedConfiguration.php @@ -6,6 +6,7 @@ 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; @@ -129,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 @@ -140,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; @@ -168,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 @@ -181,13 +194,79 @@ 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'] ?? []; + return DescriptionHelper::removeDescriptionFromMetadataMap( + $this->mapping['table_metadata'] ?? [], + 'table_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'])) { - $metadata['KBC.description'] = $this->mapping['description']; + return DescriptionHelper::normalizeDescription($this->mapping['description']); + } + + $tableMetadataDescription = DescriptionHelper::getDescriptionFromMetadataMap( + $this->mapping['table_metadata'] ?? [], + 'table_metadata', + ); + if ($tableMetadataDescription !== null) { + return $tableMetadataDescription; + } + + return DescriptionHelper::getDescriptionFromMetadataList($this->mapping['metadata'] ?? []); + } + + /** + * 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; + } + + // 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) { + $description = DescriptionHelper::getDescriptionFromMetadataList($metadata); + if ($description !== null) { + $descriptions[(string) $columnName] = $description; + } } - return $metadata; + + return $descriptions; } /** @return null|MappingFromConfigurationSchemaColumn[] */ diff --git a/libs/output-mapping/src/Storage/StoragePreparer.php b/libs/output-mapping/src/Storage/StoragePreparer.php index 0ed588c65..babca9702 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->updateDescriptions($destinationTableInfo, $descriptions); + } + } } return new MappingStorageSources($destinationBucket, $destinationTableInfo); diff --git a/libs/output-mapping/src/Storage/TableCreator.php b/libs/output-mapping/src/Storage/TableCreator.php index a5b03e783..b140f66f2 100644 --- a/libs/output-mapping/src/Storage/TableCreator.php +++ b/libs/output-mapping/src/Storage/TableCreator.php @@ -11,15 +11,12 @@ class TableCreator { - public function __construct( - private readonly ClientWrapper $clientWrapper, - ) { + public function __construct(private readonly ClientWrapper $clientWrapper) + { } - public function createTableDefinition( - string $bucketId, - TableDefinitionInterface $tableDefinition, - ): string { + public function createTableDefinition(string $bucketId, TableDefinitionInterface $tableDefinition): string + { try { return $this->clientWrapper->getTableAndFileStorageClient()->createTableDefinition( $bucketId, 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..2199eaf2a --- /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; + } + + $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; + } + + try { + $this->clientWrapper->getTableAndFileStorageClient()->updateTableDefinition( + $tableInfo->getId(), + $tableDefinitionUpdate, + ); + } catch (ClientException $e) { + 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; + } + + throw new InvalidOutputException( + sprintf( + 'Cannot update description of table "%s": %s', + $tableInfo->getId(), + $e->getMessage(), + ), + $e->getCode(), + $e, + ); + } + } + + /** + * @return array{description?: string, columns?: list} + */ + private function buildTableDefinitionUpdate(TableInfo $tableInfo, TableDescription $descriptions): array + { + $tableDefinitionUpdate = []; + + $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 (!in_array($columnName, $tableColumns, true)) { + $missingColumns[] = $columnName; + continue; + } + if ($columnDescription === ($storedColumnDescriptions[$columnName] ?? null)) { + 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), + $tableInfo->getId(), + )); + } + + if ($columns) { + $tableDefinitionUpdate['columns'] = $columns; + } + + return $tableDefinitionUpdate; + } +} diff --git a/libs/output-mapping/src/Storage/TableInfo.php b/libs/output-mapping/src/Storage/TableInfo.php index ef427f60b..3ede2b931 100644 --- a/libs/output-mapping/src/Storage/TableInfo.php +++ b/libs/output-mapping/src/Storage/TableInfo.php @@ -25,8 +25,58 @@ 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']; } + + /** + * 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/TableLoader.php b/libs/output-mapping/src/TableLoader.php index 098da20da..33cf7fec0 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(); @@ -138,21 +141,32 @@ 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, ); + // 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; } @@ -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/src/Writer/Helper/DescriptionHelper.php b/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php new file mode 100644 index 000000000..774e75fb3 --- /dev/null +++ b/libs/output-mapping/src/Writer/Helper/DescriptionHelper.php @@ -0,0 +1,125 @@ + 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, string $configurationNode): array + { + $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. + * + * @param array $metadata + * @return list + */ + public static function removeDescriptionFromMetadataList(array $metadata): array + { + return array_values(array_filter( + $metadata, + 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/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/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 94e1cdc30..d387331af 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; @@ -17,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; @@ -644,6 +647,366 @@ 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'; + + $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' => 'tableCreate', + 'status' => 'success', + 'tableId' => null, + 'results' => [ + 'id' => $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' => 'tableCreate', + 'status' => 'success', + 'tableId' => null, + 'results' => [ + 'id' => $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(), + ); + } + } + + /** + * 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/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/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/Mapping/MappingFromConfigurationSchemaColumnTest.php b/libs/output-mapping/tests/Mapping/MappingFromConfigurationSchemaColumnTest.php index 0bf8b415b..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; @@ -22,6 +23,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 @@ -51,12 +53,40 @@ 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.', + // 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()); + } + + public function testGetDescriptionFromMetadata(): void + { + $schemColumn = new MappingFromConfigurationSchemaColumn([ + 'name' => 'newColumn', + 'metadata' => [ + 'KBC.description' => 'Description from metadata.', ], - $schemColumn->getMetadata(), - ); + ]); + + 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()); + } + + /** + * 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', + ]); + + $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/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 6de6c3b23..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; @@ -71,6 +72,187 @@ 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()); + } + + /** + * `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' => [ + '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 'empty description is not stored' => [ + '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 + { + $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 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); + $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 @@ -89,12 +271,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 new file mode 100644 index 000000000..6db4fbb17 --- /dev/null +++ b/libs/output-mapping/tests/Storage/TableDescriptionModifierTest.php @@ -0,0 +1,247 @@ +logHandler = new TestHandler(); + $this->logger = new Logger('test', [$this->logHandler]); + } + + public function testDescriptionsAreStored(): void + { + $client = $this->createMock(Client::class); + $client->expects(self::once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'description' => 'table desc', + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ['name' => 'col2', 'description' => 'col2 desc'], + ], + ], + ) + ->willReturn([]); + + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1', 'col2']), + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc', 'col2' => 'col2 desc']), + ); + + self::assertFalse($this->logHandler->hasWarningRecords()); + } + + public function testStoringIsSkippedForUserManagedDescription(): void + { + $client = $this->createMock(Client::class); + $client->expects(self::never()) + ->method('updateTableDefinition'); + + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1'], isDescriptionSystemManaged: false), + new TableDescription(self::TABLE_ID, 'table desc', ['col1' => 'col1 desc']), + ); + + self::assertTrue($this->logHandler->hasInfoThatContains(sprintf( + 'Description of table "%s" is managed by the user, keeping the current value.', + self::TABLE_ID, + ))); + } + + /** + * 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(self::never()) + ->method('updateTableDefinition'); + + $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 testOnlyChangedDescriptionsAreSent(): void + { + $client = $this->createMock(Client::class); + $client->expects(self::once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'columns' => [ + ['name' => 'col2', 'description' => 'new col2 desc'], + ], + ], + ) + ->willReturn([]); + + $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 testMissingColumnsAreSkippedWithWarning(): void + { + $client = $this->createMock(Client::class); + $client->expects(self::once()) + ->method('updateTableDefinition') + ->with( + self::TABLE_ID, + [ + 'columns' => [ + ['name' => 'col1', 'description' => 'col1 desc'], + ], + ], + ) + ->willReturn([]); + + $this->createModifier($client)->updateDescriptions( + $this->createTableInfo(columns: ['col1']), + new TableDescription(self::TABLE_ID, null, ['col1' => 'col1 desc', 'col2' => 'col2 desc']), + ); + + 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 testUserErrorIsWrappedInInvalidOutputException(): void + { + $clientException = new ClientException('Table definition update failed', 400); + + $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 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()); + } + } + + 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 new TableDescriptionModifier($clientWrapper, $this->logger); + } + + /** + * @param string[] $columns + * @param array $storedColumnDescriptions + */ + private function createTableInfo( + array $columns = [], + bool $isDescriptionSystemManaged = true, + ?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, + 'definition' => [ + 'description' => $storedTableDescription, + 'columns' => $definitionColumns, + ], + ]); + } +} 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/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/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)')); + } +} diff --git a/libs/output-mapping/tests/Writer/TableDefinitionTest.php b/libs/output-mapping/tests/Writer/TableDefinitionTest.php index aefa55b67..5986331ca 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 @@ -340,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'], + ); } /** @@ -688,4 +724,26 @@ private static function assertTablePrimaryKeyAddJob(array $jobData, array $expec self::assertSame('success', $jobData['status']); self::assertSame($expectedPk, $jobData['operationParams']['columns']); } + + /** + * @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/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 1399843fb..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 @@ -752,6 +766,43 @@ 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']); + } + + /** + * @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 new file mode 100644 index 000000000..9ed120f8c --- /dev/null +++ b/libs/output-mapping/tests/Writer/TableDescriptionWriterTest.php @@ -0,0 +1,334 @@ +emptyOutputBucketId . '.tableDescription'; + + $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 + // 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'); + } + + /** + * 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`. 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 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'); + + $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')); + } + + /** + * 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 testDescriptionIsStoredOnTableCreatedByLoadJob(): 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(['Id', 'Name'], $tableDetail['columns']); + self::assertSame('table description', $tableDetail['definition']['description'] ?? null); + self::assertSame('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')); + + self::assertTrue($this->testHandler->hasInfoThatContains(sprintf( + 'Description of table "%s" is managed by the user, keeping the current value.', + $tableId, + ))); + } + + /** + * 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. + */ + #[NeedsEmptyOutputBucket] + public function testDescriptionIsKeptWhenNoLongerInConfiguration(): void + { + $tableId = $this->emptyOutputBucketId . '.tableDescription'; + + $this->uploadTable($tableId, 'table description', 'Id description'); + $this->uploadTable($tableId, null, null); + + $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')); + } + + /** + * 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', [ + '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/tableDescriptionFailedLoad.csv', + "\"test\",\"test\"\n\"aabb\",\"ccdd\",\"dddd\"\n", + ); + + $tableQueue = $this->getTableLoader(logger: $this->testLogger)->uploadTables( + configuration: new OutputMappingSettings( + configuration: [ + 'mapping' => [ + [ + 'source' => 'tableDescriptionFailedLoad.csv', + 'destination' => $tableId, + 'columns' => ['Id', 'Name'], + 'description' => 'table description', + 'column_metadata' => [ + 'Id' => [ + [ + 'key' => DescriptionHelper::DESCRIPTION_METADATA_KEY, + 'value' => 'Id 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::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')); + } + + /** + * @param array|null $extraColumnMetadata + */ + private function uploadTable( + string $tableId, + ?string $tableDescription, + ?string $columnDescription, + ?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['description'] = $tableDescription; + } + + $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'] ?? []; + 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; + } +} diff --git a/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php b/libs/output-mapping/tests/Writer/Workspace/WriterWorkspaceTest.php index 68e583ad7..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,6 +105,17 @@ public function testSnowflakeTableOutputMapping(): void '"id3","name3","foo3","bar3"', ], ); + + $tableDetail = $this->clientWrapper->getTableAndFileStorageClient()->getTable( + $this->emptyOutputBucketId . '.table1a', + ); + 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]