From bbc04901c92245d339963a50439be7116d54e46c Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 10:42:36 -0500 Subject: [PATCH 01/15] Fix empty dd columns and sanitize dd field names --- .../src/Service/MysqlImport.php | 2 +- .../AlterTableQuery/MySQLQuery.php | 105 +++++++++++++++++- 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php index 67a55d539d..f50445a4fd 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php @@ -274,7 +274,7 @@ public function sanitizeDescription(string $column) { * @returns string * Sanitized column name. */ - protected function sanitizeHeader(string $column): string { + public static function sanitizeHeader(string $column): string { // Replace all spaces and newline characters with underscores since they are // not supported. $column = preg_replace('/(?: |\r\n|\r|\n)/', '_', $column); diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 621a516a43..67b7fd4542 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -4,6 +4,7 @@ use Drupal\Core\Database\StatementInterface; +use Drupal\datastore_mysql_import\Service\MysqlImport; use Drupal\datastore\DataDictionary\AlterTableQueryBase; use Drupal\datastore\DataDictionary\AlterTableQueryInterface; use Drupal\datastore\DataDictionary\IncompatibleTypeException; @@ -13,6 +14,17 @@ */ class MySQLQuery extends AlterTableQueryBase implements AlterTableQueryInterface { + /** + * Date time specific types. + * + * @var string[] + */ + protected const DATE_TIME_TYPES = [ + 'DATE', + 'TIME', + 'DATETIME', + ]; + /** * Default type to use for fields if no data-dictionary type is specified. * @@ -60,7 +72,15 @@ class MySQLQuery extends AlterTableQueryBase implements AlterTableQueryInterface * {@inheritdoc} */ public function doExecute(): void { + // Sanitize field names to match database field names. + $this->fields = $this->sanitizeFields($this->fields); + // Filter out fields which are not present in the database table. $this->fields = $this->mergeFields($this->fields, $this->table); + + // Sanitize index field names to match database field names. + $this->indexes = $this->sanitizeIndexes($this->indexes); + // Filter out indexes with fields which are not present in the database + // table. $this->indexes = $this->mergeIndexes($this->indexes, $this->table); // Build and execute SQL commands to prepare for table alter. @@ -72,6 +92,54 @@ public function doExecute(): void { $command->execute(); } + /** + * Sanitize field names. + * + * @param array $fields + * Query fields. + * + * @return array + * Query fields list with sanitized field names. + */ + protected function sanitizeFields(array $fields): array { + // Iterate through field list... + foreach (array_keys($fields) as $key) { + // Create reference to index field name. + $field_name = &$fields[$key]['name']; + // Sanitize field name. + $field_name = MysqlImport::sanitizeHeader($field_name); + } + + return $fields; + } + + /** + * Sanitize index field names. + * + * @param array $indexes + * Query indexes. + * + * @return array + * Query indexes list with sanitized index field names. + */ + protected function sanitizeIndexes(array $indexes): array { + // Iterate through index list... + foreach (array_keys($indexes) as $index_key) { + // Create reference to index field list. + $index_fields = &$indexes[$index_key]['fields']; + + // Iterate through index field list... + foreach (array_keys($index_fields) as $field_key) { + // Create reference to index field name. + $field_name = &$index_fields[$field_key]['name']; + // Sanitize field name. + $field_name = MysqlImport::sanitizeHeader($field_name); + } + } + + return $indexes; + } + /** * Remove query fields not found in the given table and copy over names. * @@ -218,22 +286,49 @@ protected function buildPreAlterCommands(array $query_fields, string $table): ar // Build pre-alter commands for each query field. foreach ($query_fields as ['name' => $col, 'type' => $type, 'format' => $format]) { + // Extract base type from full MySQL column type. + $base_type = $this->getBaseType($type); + + // Replace empty strings with NULL for non-text columns to prevent + // misc. errors (i.e. STR_TO_DATE function related and "Incorrect + // `type` value" errors). + if (!in_array($base_type, self::STRING_FIELD_TYPES, TRUE)) { + $pre_alter_cmds[] = $this->connection->update($table)->condition($col, '')->expression($col, 'NULL'); + } + // If this field is a date field, and a valid format is provided; update // the format of the date fields to ISO-8601 before importing into MySQL. - if ($type === 'date' && !empty($format) && $format !== 'default') { + if (in_array($base_type, self::DATE_TIME_TYPES, TRUE) && !empty($format) && $format !== 'default') { $mysql_date_format = $this->dateFormatConverter->convert($format); - // Replace empty date strings with NULL to prevent STR_TO_DATE errors. - $pre_alter_cmds[] = $this->connection->update($table)->condition($col, '')->expression($col, 'NULL'); // Convert date formats for date column. $pre_alter_cmds[] = $this->connection->update($table)->expression($col, "STR_TO_DATE({$col}, :date_format)", [ ':date_format' => $mysql_date_format, ]); } + + // Convert strings 'true' and 'false' to '1' and '0' for boolean fields. + if ($base_type === 'BOOL') { + $pre_alter_cmds[] = $this->connection->update($table)->condition("UPPER({$col})", 'FALSE')->expression($col, '0'); + $pre_alter_cmds[] = $this->connection->update($table)->condition("UPPER({$col})", 'TRUE')->expression($col, '1'); + } } return $pre_alter_cmds; } + /** + * Extract base type from full MySQL type. + * + * @param string $type + * Full MySQL type - e.g. "DECIMAL(12, 3)". + * + * @return string + * Base MySQL type - e.g. "DECIMAL". + */ + protected static function getBaseType(string $type): string { + return strtok($type, '('); + } + /** * Build alter command to modify table column data types. * @@ -379,10 +474,8 @@ protected function getIndexType(string $frictionless_type): string { * Formatted index field option string. */ protected function buildIndexFieldOption(string $name, ?int $length, string $table, string $type): string { - // Extract base type from full MySQL type. - $base_type = strtok($type, '('); // If this field is a string type, determine what it's length should be... - if (in_array($base_type, self::STRING_FIELD_TYPES)) { + if (in_array($this->getBaseType($type), self::STRING_FIELD_TYPES)) { // Initialize length to the default index field length if not set. $length ??= self::DEFAULT_INDEX_FIELD_LENGTH; // Retrieve the max length for this table column. From 08c562f2327f38f8450c7c0f20266d3118182f2f Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 12:00:02 -0500 Subject: [PATCH 02/15] Fix bool pre-alter commands --- .../src/DataDictionary/AlterTableQuery/MySQLQuery.php | 4 ++-- .../datastore/tests/src/Functional/DictionaryEnforcerTest.php | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 67b7fd4542..7039711c01 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -308,8 +308,8 @@ protected function buildPreAlterCommands(array $query_fields, string $table): ar // Convert strings 'true' and 'false' to '1' and '0' for boolean fields. if ($base_type === 'BOOL') { - $pre_alter_cmds[] = $this->connection->update($table)->condition("UPPER({$col})", 'FALSE')->expression($col, '0'); - $pre_alter_cmds[] = $this->connection->update($table)->condition("UPPER({$col})", 'TRUE')->expression($col, '1'); + $pre_alter_cmds[] = $this->connection->update($table)->where("UPPER({$col}) = :value", [':value' => 'FALSE'])->expression($col, '0'); + $pre_alter_cmds[] = $this->connection->update($table)->where("UPPER({$col}) = :value", [':value' => 'TRUE'])->expression($col, '1'); } } diff --git a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php index 271d9cb69e..3f6e0055eb 100644 --- a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php +++ b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php @@ -166,10 +166,11 @@ public function testDictionaryEnforcement(): void { // Build dataset. $dataset_id = $this->uuid->generate(); $dataset = $this->validMetadataFactory->get($this->getDataset($dataset_id, 'Test ' . $dataset_id, [$this->resourceUrl], TRUE), 'dataset'); - // Create datset. + // Create dataset. $this->metastore->post('dataset', $dataset); $this->metastore->publish('dataset', $dataset_id); + print_r(`drush queue:list`); // Run cron to import dataset into datastore. $this->cron->run(); // Run cron to apply data-dictionary. From 8f04b0402dc5014835ac1883e58e1c2c31cfa95b Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 12:28:00 -0500 Subject: [PATCH 03/15] CodeClimate - reducing cognitive complexity --- .../AlterTableQuery/MySQLQuery.php | 65 +++++++++++++++---- 1 file changed, 53 insertions(+), 12 deletions(-) diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 7039711c01..fb522e4a12 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -79,8 +79,7 @@ public function doExecute(): void { // Sanitize index field names to match database field names. $this->indexes = $this->sanitizeIndexes($this->indexes); - // Filter out indexes with fields which are not present in the database - // table. + // Filter out indexes with fields which are not present in the table. $this->indexes = $this->mergeIndexes($this->indexes, $this->table); // Build and execute SQL commands to prepare for table alter. @@ -296,26 +295,68 @@ protected function buildPreAlterCommands(array $query_fields, string $table): ar $pre_alter_cmds[] = $this->connection->update($table)->condition($col, '')->expression($col, 'NULL'); } - // If this field is a date field, and a valid format is provided; update - // the format of the date fields to ISO-8601 before importing into MySQL. - if (in_array($base_type, self::DATE_TIME_TYPES, TRUE) && !empty($format) && $format !== 'default') { - $mysql_date_format = $this->dateFormatConverter->convert($format); - // Convert date formats for date column. - $pre_alter_cmds[] = $this->connection->update($table)->expression($col, "STR_TO_DATE({$col}, :date_format)", [ - ':date_format' => $mysql_date_format, - ]); + // If this field is a date field, and + if (in_array($base_type, self::DATE_TIME_TYPES, TRUE)) { + $pre_alter_cmds = array_merge($pre_alter_cmds, $this->buildDatePreAlterCommands($table, $col, $format)); } // Convert strings 'true' and 'false' to '1' and '0' for boolean fields. if ($base_type === 'BOOL') { - $pre_alter_cmds[] = $this->connection->update($table)->where("UPPER({$col}) = :value", [':value' => 'FALSE'])->expression($col, '0'); - $pre_alter_cmds[] = $this->connection->update($table)->where("UPPER({$col}) = :value", [':value' => 'TRUE'])->expression($col, '1'); + $pre_alter_cmds = array_merge($pre_alter_cmds, $this->buildBoolPreAlterCommands($table, $col)); } } return $pre_alter_cmds; } + /** + * Build pre-alter commands for date fields. + * + * Update format of the date fields to ISO-8601 before importing into MySQL. + * + * @param string $table + * Table name. + * @param string $column + * Table column. + * @param string $format + * Field frictionless date format. + * + * @return \Drupal\Core\Database\Query\Update[] + * Pre-alter update DB queries. + */ + protected function buildDatePreAlterCommands(string $table, string $column, string $format): array { + $pre_alter_cmds = []; + + // If a valid format is provided... + if (!empty($format) && $format !== 'default') { + $mysql_date_format = $this->dateFormatConverter->convert($format); + // Convert date formats for date column. + $pre_alter_cmds[] = $this->connection->update($table)->expression($column, "STR_TO_DATE({$column}, :date_format)", [ + ':date_format' => $mysql_date_format, + ]); + } + + return $pre_alter_cmds; + } + + /** + * Build pre-alter commands for boolean fields. + * + * @param string $table + * Table name. + * @param string $column + * Table column. + * + * @return \Drupal\Core\Database\Query\Update[] + * Pre-alter update DB queries. + */ + protected function buildBoolPreAlterCommands(string $table, string $column): array { + return [ + $this->connection->update($table)->where("UPPER({$column}) = :value", [':value' => 'FALSE'])->expression($column, '0'), + $this->connection->update($table)->where("UPPER({$column}) = :value", [':value' => 'TRUE'])->expression($column, '1'), + ]; + } + /** * Extract base type from full MySQL type. * From 0ba97836b1753013f7697ce10ed9ff48d7573d8c Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 12:30:17 -0500 Subject: [PATCH 04/15] Fix documentation --- .../src/DataDictionary/AlterTableQuery/MySQLQuery.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index fb522e4a12..140be677af 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -295,12 +295,12 @@ protected function buildPreAlterCommands(array $query_fields, string $table): ar $pre_alter_cmds[] = $this->connection->update($table)->condition($col, '')->expression($col, 'NULL'); } - // If this field is a date field, and + // Build pre-alter commands for date fields. if (in_array($base_type, self::DATE_TIME_TYPES, TRUE)) { $pre_alter_cmds = array_merge($pre_alter_cmds, $this->buildDatePreAlterCommands($table, $col, $format)); } - // Convert strings 'true' and 'false' to '1' and '0' for boolean fields. + // Build pre-alter commands for boolean fields. if ($base_type === 'BOOL') { $pre_alter_cmds = array_merge($pre_alter_cmds, $this->buildBoolPreAlterCommands($table, $col)); } @@ -342,6 +342,8 @@ protected function buildDatePreAlterCommands(string $table, string $column, stri /** * Build pre-alter commands for boolean fields. * + * Convert strings 'true' and 'false' to '1' and '0' for boolean fields + * * @param string $table * Table name. * @param string $column From c5c563d860619843c87c874473326b7a69a7388e Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 12:49:22 -0500 Subject: [PATCH 05/15] CodeClimate - documentation --- .../datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 140be677af..19526706e3 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -342,7 +342,7 @@ protected function buildDatePreAlterCommands(string $table, string $column, stri /** * Build pre-alter commands for boolean fields. * - * Convert strings 'true' and 'false' to '1' and '0' for boolean fields + * Convert strings 'true' and 'false' to '1' and '0' for boolean fields. * * @param string $table * Table name. From ae485773470d288c66908c53c7611df6b477ef98 Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 16:37:53 -0500 Subject: [PATCH 06/15] Fix broken tests & add test coverage --- .../src/Service/MysqlImport.php | 10 +- .../AlterTableQuery/MySQLQuery.php | 96 ++++++++++--------- modules/datastore/tests/data/data-dict.csv | 1 + .../src/Functional/DictionaryEnforcerTest.php | 3 +- 4 files changed, 58 insertions(+), 52 deletions(-) diff --git a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php index f50445a4fd..5e0901fcff 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php @@ -229,11 +229,11 @@ public function generateTableSpec(array $columns): array { foreach ($columns as $column) { // Sanitize the supplied table header to generate a unique column name; // null-coalesce potentially NULL column names to empty strings. - $name = $this->sanitizeHeader($column ?? ''); + $name = self::sanitizeHeader($column ?? ''); // Truncate the generated table column name, if necessary, to fit the max // column length. - $name = $this->truncateHeader($name); + $name = self::truncateHeader($name); // Generate unique numeric suffix for the header if a header already // exists with the same name. @@ -244,7 +244,7 @@ public function generateTableSpec(array $columns): array { $spec[$name] = [ 'type' => "text", - 'description' => $this->sanitizeDescription($column ?? ''), + 'description' => self::sanitizeDescription($column ?? ''), ]; } @@ -260,7 +260,7 @@ public function generateTableSpec(array $columns): array { * @return string * Column name on single line. */ - public function sanitizeDescription(string $column) { + public static function sanitizeDescription(string $column) { $trimmed = array_filter(array_map('trim', explode("\n", $column))); return implode(" ", $trimmed); } @@ -304,7 +304,7 @@ public static function sanitizeHeader(string $column): string { * @returns string * Truncated column name. */ - protected function truncateHeader(string $column): string { + protected static function truncateHeader(string $column): string { // If the supplied table column name is longer than the max column length, // truncate the column name to 5 characters under the max length and // substitute the truncated characters with a unique hash. diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 19526706e3..4caad62dbb 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -204,7 +204,7 @@ protected function getTableColsAndComments(string $table): array { } /** - * Get MySQL equivalent of the given Frictionless "Table Schema" type. + * Build full MySQL equivalent of the given Frictionless "Table Schema" type. * * @param string $frictionless_type * Frictionless "Table Schema" data type. @@ -214,59 +214,76 @@ protected function getTableColsAndComments(string $table): array { * MySQL table to get type for. * * @return string - * MySQL data type. + * Full MySQL data type. */ protected function getFieldType(string $frictionless_type, string $column, string $table): string { + // Determine MySQL base type. + $base_mysql_type = $this->getMySQLType($frictionless_type); // Build the MySQL type argument list. - $args = $this->buildTypeArgs($frictionless_type, $column, $table); + $args = $this->buildTypeArgs($base_mysql_type, $column, $table); + $args_str = !empty($args) ? '(' . implode(', ', $args) . ')' : ''; // Build full MySQL type. + return $base_mysql_type . $args_str; + } + + /** + * Get base MySQL equivalent of the given Frictionless "Table Schema" type. + * + * @param string $frictionless_type + * Frictionless "Table Schema" data type. + * + * @return string + * Base MySQL data type. + */ + protected function getMySQLType(string $frictionless_type): string { return ([ - 'string' => (fn () => 'TEXT'), - 'number' => (fn ($args) => "DECIMAL({$args['size']}, {$args['decimal']})"), - 'integer' => (fn () => 'INT'), - 'date' => (fn () => 'DATE'), - 'time' => (fn () => 'TIME'), - 'datetime' => (fn () => 'DATETIME'), - 'year' => (fn () => 'YEAR'), - 'yearmonth' => (fn () => 'TINYTEXT'), - 'boolean' => (fn () => 'BOOL'), - 'object' => (fn () => 'TEXT'), - 'geopoint' => (fn () => 'TEXT'), - 'geojson' => (fn () => 'TEXT'), - 'array' => (fn () => 'TEXT'), - 'duration' => (fn () => 'TINYTEXT'), - ])[$frictionless_type]($args); + 'string' => 'TEXT', + 'number' => 'DECIMAL', + 'integer' => 'INT', + 'date' => 'DATE', + 'time' => 'TIME', + 'datetime' => 'DATETIME', + 'year' => 'YEAR', + 'yearmonth' => 'TINYTEXT', + 'boolean' => 'BOOL', + 'object' => 'TEXT', + 'geopoint' => 'TEXT', + 'geojson' => 'TEXT', + 'array' => 'TEXT', + 'duration' => 'TINYTEXT', + ])[$frictionless_type]; } /** - * Build MySQL type arg list for the given Frictionless "Table Schema" type. + * Build MySQL type arg list for MySQL type. * * @param string $type - * Frictionless "Table Schema" data type. + * MySQL data type. * @param string $column * Column name. * @param string $table * Table name. * + * @return array + * MySQL type arguments. + * * @throws Drupal\datastore\DataDictionary\IncompatibleTypeException * When incompatible data is found in the table for the specified type. */ protected function buildTypeArgs(string $type, string $column, string $table): array { - $args = []; - - // If this field is a number field, build decimal and size arguments for - // MySQL type. - if ($type === 'number') { + // If this field is a DECIMAL field, build decimal and size arguments. + if ($type === 'DECIMAL') { $non_decimals = $this->connection->query("SELECT MAX(LENGTH(TRIM(LEADING '-' FROM SUBSTRING_INDEX({$column}, '.', 1)))) FROM {{$table}};")->fetchField(); - $args['decimal'] = $this->connection->query("SELECT MAX(LENGTH(SUBSTRING_INDEX({$column}, '.', -1))) FROM {{$table}};")->fetchField(); - $args['size'] = $non_decimals + $args['decimal']; - if ($args['size'] > self::DECIMAL_MAX_SIZE || $args['decimal'] > self::DECIMAL_MAX_DECIMAL) { + $decimal = $this->connection->query("SELECT MAX(LENGTH(SUBSTRING_INDEX({$column}, '.', -1))) FROM {{$table}};")->fetchField(); + $size = $non_decimals + $decimal; + if ($size > self::DECIMAL_MAX_SIZE || $decimal > self::DECIMAL_MAX_DECIMAL) { throw new IncompatibleTypeException("Decimal values found in column too large for DECIMAL type; please use type 'string' for column '{$column}'"); } + return [$size, $decimal]; } - return $args; + return []; } /** @@ -285,8 +302,8 @@ protected function buildPreAlterCommands(array $query_fields, string $table): ar // Build pre-alter commands for each query field. foreach ($query_fields as ['name' => $col, 'type' => $type, 'format' => $format]) { - // Extract base type from full MySQL column type. - $base_type = $this->getBaseType($type); + // Determine base MySQL type for Frictionless column type. + $base_type = $this->getMySQLType($type); // Replace empty strings with NULL for non-text columns to prevent // misc. errors (i.e. STR_TO_DATE function related and "Incorrect @@ -359,19 +376,6 @@ protected function buildBoolPreAlterCommands(string $table, string $column): arr ]; } - /** - * Extract base type from full MySQL type. - * - * @param string $type - * Full MySQL type - e.g. "DECIMAL(12, 3)". - * - * @return string - * Base MySQL type - e.g. "DECIMAL". - */ - protected static function getBaseType(string $type): string { - return strtok($type, '('); - } - /** * Build alter command to modify table column data types. * @@ -517,8 +521,10 @@ protected function getIndexType(string $frictionless_type): string { * Formatted index field option string. */ protected function buildIndexFieldOption(string $name, ?int $length, string $table, string $type): string { + // Extract base type from full MySQL type ("DECIMAL(12, 3)" -> "DECIMAL"). + $base_type = strtok($type, '('); // If this field is a string type, determine what it's length should be... - if (in_array($this->getBaseType($type), self::STRING_FIELD_TYPES)) { + if (in_array($base_type, self::STRING_FIELD_TYPES)) { // Initialize length to the default index field length if not set. $length ??= self::DEFAULT_INDEX_FIELD_LENGTH; // Retrieve the max length for this table column. diff --git a/modules/datastore/tests/data/data-dict.csv b/modules/datastore/tests/data/data-dict.csv index 440b21730a..876f556450 100644 --- a/modules/datastore/tests/data/data-dict.csv +++ b/modules/datastore/tests/data/data-dict.csv @@ -1,3 +1,4 @@ a,b,c,d 1,02/23/1978,9.07,efghijk +,,, 2,09/12/2009,2.37,abcde diff --git a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php index 3f6e0055eb..65c979e731 100644 --- a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php +++ b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php @@ -170,7 +170,6 @@ public function testDictionaryEnforcement(): void { $this->metastore->post('dataset', $dataset); $this->metastore->publish('dataset', $dataset_id); - print_r(`drush queue:list`); // Run cron to import dataset into datastore. $this->cron->run(); // Run cron to apply data-dictionary. @@ -229,7 +228,7 @@ public function testDictionaryEnforcement(): void { 'd', ], ], - 'numOfRows' => 2, + 'numOfRows' => 3, ], $result); } From 8d9a7ae3382ddbe4681117f0b33210a5b1fa8d9a Mon Sep 17 00:00:00 2001 From: Clayton Liddell Date: Fri, 7 Oct 2022 16:48:53 -0500 Subject: [PATCH 07/15] CodeClimate --- .../src/DataDictionary/AlterTableQuery/MySQLQuery.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 4caad62dbb..5c9537ffa1 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -218,7 +218,7 @@ protected function getTableColsAndComments(string $table): array { */ protected function getFieldType(string $frictionless_type, string $column, string $table): string { // Determine MySQL base type. - $base_mysql_type = $this->getMySQLType($frictionless_type); + $base_mysql_type = $this->getBaseType($frictionless_type); // Build the MySQL type argument list. $args = $this->buildTypeArgs($base_mysql_type, $column, $table); $args_str = !empty($args) ? '(' . implode(', ', $args) . ')' : ''; @@ -236,7 +236,7 @@ protected function getFieldType(string $frictionless_type, string $column, strin * @return string * Base MySQL data type. */ - protected function getMySQLType(string $frictionless_type): string { + protected function getBaseType(string $frictionless_type): string { return ([ 'string' => 'TEXT', 'number' => 'DECIMAL', @@ -303,7 +303,7 @@ protected function buildPreAlterCommands(array $query_fields, string $table): ar // Build pre-alter commands for each query field. foreach ($query_fields as ['name' => $col, 'type' => $type, 'format' => $format]) { // Determine base MySQL type for Frictionless column type. - $base_type = $this->getMySQLType($type); + $base_type = $this->getBaseType($type); // Replace empty strings with NULL for non-text columns to prevent // misc. errors (i.e. STR_TO_DATE function related and "Incorrect From c3a5d71a0d34935fecf32c0ea74343f9278fd03f Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Wed, 19 Oct 2022 15:15:25 -0700 Subject: [PATCH 08/15] some work on tests --- .../src/Factory/MysqlImportFactory.php | 2 - .../src/Service/MysqlImport.php | 2 - .../src/Unit/Service/MysqlImportTest.php | 21 ++++++++- .../datastore/src/Commands/PurgeCommands.php | 2 - .../src/Controller/ImportController.php | 2 - modules/datastore/src/Drush.php | 2 - .../EventSubscriber/DatastoreSubscriber.php | 1 - .../datastore/src/Service/Factory/Import.php | 2 - .../Form/DkanSqlEndpointSettingsForm.php | 2 - modules/datastore/tests/data/data-dict.csv | 8 ++-- .../src/Functional/DictionaryEnforcerTest.php | 46 +++++++++++++------ .../AlterTableQuery/MySQLQueryTest.php | 33 +++++++++++++ phpunit.xml | 38 ++++----------- 13 files changed, 100 insertions(+), 61 deletions(-) diff --git a/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php b/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php index 88a9519cc5..8ac23c831e 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php @@ -10,8 +10,6 @@ /** * Importer factory. - * - * @codeCoverageIgnore */ class MysqlImportFactory implements ImportFactoryInterface { diff --git a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php index 5e0901fcff..cac4c13d32 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php @@ -10,8 +10,6 @@ /** * Expiremental MySQL LOAD DATA importer. - * - * @codeCoverageIgnore */ class MysqlImport extends ImportJob { diff --git a/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php b/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php index aa80d139de..022e25d6b4 100644 --- a/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php +++ b/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php @@ -25,7 +25,7 @@ use Symfony\Component\HttpFoundation\RequestStack; /** - * + * @coversDefaultClass Drupal\datastore_mysql_import\Service\MysqlImport */ class MysqlImportTest extends TestCase { @@ -34,6 +34,10 @@ class MysqlImportTest extends TestCase { /** * Test spec generation. + * + * @covers ::generateTableSpec + * @covers ::sanitizeHeader + * @covers ::truncateHeader */ public function testGenerateTableSpec() { $mysqlImporter = $this->getMysqlImporter(); @@ -124,6 +128,21 @@ public function testMysqlImporterWithCSVFileWithNewLinesInHeaders() { ])); } + public function sanitizeHeaderProvider() { + return [ + 'reserved_word' => ['accessible', '_accessible'], + 'numeric' => [1, '_1'], + ]; + } + + /** + * @dataProvider sanitizeHeaderProvider + * @covers ::sanitizeHeader + */ + public function testSanitizeHeader($column, $expected) { + $this->assertEquals(MysqlImport::sanitizeHeader($column), $expected); + } + protected function getMysqlImporter() { $resource = new DataResource(self::HOST . '/text.csv', 'text/csv'); $databaseTableFactory = $this->getDatabaseTableFactoryMock(); diff --git a/modules/datastore/src/Commands/PurgeCommands.php b/modules/datastore/src/Commands/PurgeCommands.php index e4e3805e1d..7747beff07 100644 --- a/modules/datastore/src/Commands/PurgeCommands.php +++ b/modules/datastore/src/Commands/PurgeCommands.php @@ -8,8 +8,6 @@ /** * Datastore-related Drush commands. - * - * @codeCoverageIgnore */ class PurgeCommands extends DrushCommands { diff --git a/modules/datastore/src/Controller/ImportController.php b/modules/datastore/src/Controller/ImportController.php index d7af540ddc..48839fd411 100644 --- a/modules/datastore/src/Controller/ImportController.php +++ b/modules/datastore/src/Controller/ImportController.php @@ -16,8 +16,6 @@ * Class Api. * * @package Drupal\datastore - * - * @codeCoverageIgnore */ class ImportController implements ContainerInjectionInterface { use JsonResponseTrait; diff --git a/modules/datastore/src/Drush.php b/modules/datastore/src/Drush.php index 82a180ce32..9b2120e0e6 100755 --- a/modules/datastore/src/Drush.php +++ b/modules/datastore/src/Drush.php @@ -11,8 +11,6 @@ /** * Drush commands for controlling the datastore. - * - * @codeCoverageIgnore */ class Drush extends DrushCommands { /** diff --git a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php index b10c4086c7..fdd58d1885 100644 --- a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php +++ b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php @@ -77,7 +77,6 @@ public function __construct(ConfigFactoryInterface $config_factory, LoggerChanne /** * Inherited. * - * @codeCoverageIgnore * @inheritdoc */ public static function getSubscribedEvents() { diff --git a/modules/datastore/src/Service/Factory/Import.php b/modules/datastore/src/Service/Factory/Import.php index a149518ea7..de624469ea 100644 --- a/modules/datastore/src/Service/Factory/Import.php +++ b/modules/datastore/src/Service/Factory/Import.php @@ -8,8 +8,6 @@ /** * Create an importer object for a given resource. - * - * @codeCoverageIgnore */ class Import implements ImportFactoryInterface { diff --git a/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php b/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php index 935cf95968..e8ef6283c7 100644 --- a/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php +++ b/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php @@ -12,8 +12,6 @@ * * @package Drupal\sql_endpoint\Form * @todo Remove - * - * @codeCoverageIgnore */ class DkanSqlEndpointSettingsForm extends ConfigFormBase { diff --git a/modules/datastore/tests/data/data-dict.csv b/modules/datastore/tests/data/data-dict.csv index 876f556450..aee7816fa4 100644 --- a/modules/datastore/tests/data/data-dict.csv +++ b/modules/datastore/tests/data/data-dict.csv @@ -1,4 +1,4 @@ -a,b,c,d -1,02/23/1978,9.07,efghijk -,,, -2,09/12/2009,2.37,abcde +a,b,c,d,e +1,02/23/1978,9.07,efghijk,0 +,,,, +2,09/12/2009,2.37,abcde,1 diff --git a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php index 65c979e731..e75619d7e7 100644 --- a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php +++ b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php @@ -19,6 +19,7 @@ * @group datastore */ class DictionaryEnforcerTest extends ExistingSiteBase { + use GetDataTrait; /** @@ -96,14 +97,17 @@ public function setUp(): void { $this->uuid = \Drupal::service('uuid'); $this->validMetadataFactory = ServiceTest::getValidMetadataFactory($this); $this->webServiceApi = ImportController::create(\Drupal::getContainer()); - $this->datasetStorage = \Drupal::service('dkan.metastore.storage')->getInstance('dataset'); + $this->datasetStorage = \Drupal::service('dkan.metastore.storage') + ->getInstance('dataset'); // Copy resource file to uploads directory. $file_system = \Drupal::service('file_system'); $upload_path = $file_system->realpath(self::UPLOAD_LOCATION); $file_system->prepareDirectory($upload_path, FileSystemInterface::CREATE_DIRECTORY); $file_system->copy(self::TEST_DATA_PATH . self::RESOURCE_FILE, $upload_path); // Create resource URL. - $this->resourceUrl = \Drupal::service('stream_wrapper_manager')->getViaUri(self::UPLOAD_LOCATION . self::RESOURCE_FILE)->getExternalUrl(); + $this->resourceUrl = \Drupal::service('stream_wrapper_manager') + ->getViaUri(self::UPLOAD_LOCATION . self::RESOURCE_FILE) + ->getExternalUrl(); } /** @@ -134,6 +138,11 @@ public function testDictionaryEnforcement(): void { 'title' => 'D', 'type' => 'string', ], + [ + 'name' => 'e', + 'title' => 'E', + 'type' => 'boolean', + ], ]; $indexes = [ [ @@ -152,20 +161,26 @@ public function testDictionaryEnforcement(): void { 'type' => 'fulltext', ], ]; - $data_dict = $this->validMetadataFactory->get($this->getDataDictionary($fields, $indexes, $dict_id), 'data-dictionary'); + $dd = $this->getDataDictionary($fields, $indexes, $dict_id); + $data_dict = $this->validMetadataFactory->get($dd, 'data-dictionary'); // Create data-dictionary. $this->metastore->post('data-dictionary', $data_dict); $this->metastore->publish('data-dictionary', $dict_id); // Set global data-dictinary in metastore config. - $metastore_config = \Drupal::configFactory()->getEditable('metastore.settings'); + $metastore_config = \Drupal::configFactory() + ->getEditable('metastore.settings'); $metastore_config->set('data_dictionary_mode', DataDictionaryDiscovery::MODE_SITEWIDE); $metastore_config->set('data_dictionary_sitewide', $dict_id); $metastore_config->save(); // Build dataset. $dataset_id = $this->uuid->generate(); - $dataset = $this->validMetadataFactory->get($this->getDataset($dataset_id, 'Test ' . $dataset_id, [$this->resourceUrl], TRUE), 'dataset'); + $dataset = $this->validMetadataFactory->get( + $this->getDataset( + $dataset_id, 'Test ' . $dataset_id, [$this->resourceUrl], TRUE + ), 'dataset' + ); // Create dataset. $this->metastore->post('dataset', $dataset); $this->metastore->publish('dataset', $dataset_id); @@ -182,7 +197,7 @@ public function testDictionaryEnforcement(): void { $request = Request::create('http://blah/api'); // Retrieve schema for dataset resource. $response = $this->webServiceApi->summary($dist_id, $request); - $result = json_decode($response->getContent(), true); + $result = json_decode($response->getContent(), TRUE); // Validate schema. $this->assertEquals([ @@ -191,15 +206,15 @@ public function testDictionaryEnforcement(): void { 'record_number' => [ 'type' => 'serial', 'length' => 10, - 'unsigned' => true, - 'not null' => true, + 'unsigned' => TRUE, + 'not null' => TRUE, 'mysql_type' => 'int', ], 'a' => [ - 'type' => 'int', - 'length' => 11, - 'mysql_type' => 'int', - ], + 'type' => 'int', + 'length' => 11, + 'mysql_type' => 'int', + ], 'b' => [ 'type' => 'varchar', 'mysql_type' => 'date', @@ -216,6 +231,11 @@ public function testDictionaryEnforcement(): void { 'mysql_type' => 'text', 'description' => 'D', ], + 'e' => [ + 'type' => 'boolean', + 'mysql_type' => 'BOOL', + 'description' => 'E', + ], ], 'indexes' => [ 'index_a' => [ @@ -228,7 +248,7 @@ public function testDictionaryEnforcement(): void { 'd', ], ], - 'numOfRows' => 3, + 'numOfRows' => 2, ], $result); } diff --git a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php index 8fa05357a8..ff0ef1d05a 100644 --- a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php +++ b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php @@ -18,6 +18,8 @@ /** * Unit tests for Drupal\datastore\DataDictionary\AlterTableQuery\MySQLQuery. + * + * @coversDefaultClass Drupal\datastore\DataDictionary\AlterTableQuery\MySQLQuery */ class MySQLQueryTest extends TestCase { @@ -37,6 +39,8 @@ public function setUp(): void { /** * Build MySQLQuery arguments. + * + * @todo Include an update() mock for buildBoolPreAlterCommands(). */ public function buildConnectionChain(): Chain { return (new Chain($this)) @@ -48,6 +52,7 @@ public function buildConnectionChain(): Chain { 'foo' => 'Foo', 'bar' => 'Bar', 'baz' => 'Baz', + 'boolz' => 'Boolz', ]) ->add(StatementInterface::class, 'fetchField', (new Sequence()) ->add('5') @@ -69,6 +74,7 @@ public function buildMySQLQuery(Connection $connection, ?string $table = NULL, ? ['name' => 'foo', 'type' => 'string', 'format' => 'default', 'title' => 'Foo'], ['name' => 'bar', 'type' => 'number', 'format' => 'default', 'title' => 'Bar'], ['name' => 'baz', 'type' => 'date', 'format' => '%Y-%m-%d', 'title' => 'Baz'], + ['name' => 'boolz', 'type' => 'boolean', 'format' => 'default', 'title' => 'Boolz'], ]; $dictionary_indexes ??= [ ['name' => 'index1', 'type' => 'index', 'description' => 'Fizz', 'fields' => [ @@ -134,4 +140,31 @@ public function testExecuteWithTooLargeDecimal(): void { $mysql_query->execute(); } + public function baseTypeProvider() { + return [ + 'string' => ['string', 'TEXT'], + // @todo getBaseType() performs no type checking, has no error result. + 'getBaseType-does-no-error-checking' => ['not-a-frictionless-type', NULL], + ]; + } + + /** + * @dataProvider baseTypeProvider + * @covers ::getBaseType + */ + public function _testGetBaseType($frictionless_type, $expected_type) { + // Instantiate a MySQL query object. + $connection_chain = $this->buildConnectionChain(); + $table = 'datastore_' . uniqid(); + $mysql_query = $this->buildMySQLQuery($connection_chain->getMock(), $table); + + // getBaseType() is protected. + $get_test_base = new \ReflectionMethod($mysql_query, 'getBaseType'); + $get_test_base->setAccessible(TRUE); + $this->assertEquals( + $expected_type, + $get_test_base->invokeArgs($mysql_query, [$frictionless_type]) + ); + } + } diff --git a/phpunit.xml b/phpunit.xml index ab5bce7831..3ec3332fd3 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,47 +1,29 @@ - - modules/common/src - modules/metastore/src - modules/metastore/modules/metastore/src - modules/metastore/modules/metastore_search/src - modules/metastore/modules/metastore_admin/src - modules/datastore/src - modules/datastore/modules/datastore_mysql_import/src - modules/frontend/src - modules/harvest/src - modules/harvest/modules/harvest_dashboard/src - modules/json_form_widget/src + . - modules/common/src/Tests + modules/common/tests + modules/datastore/tests + modules/dkan_js_frontend/tests + modules/metastore/tests + modules/sample_content ./ ./ - modules/common/tests/src - modules/metastore/tests/src - modules/metastore/modules/metastore/tests/src - modules/metastore/modules/metastore_search/tests/src - modules/metastore/modules/metastore_admin/tests/src - modules/datastore/tests/src - modules/datastore/modules/datastore_mysql_import/tests/src - modules/frontend/tests/src - modules/dkan_js_frontend/tests/src - modules/harvest/tests/src - modules/harvest/modules/harvest_dashboard/tests/src - modules/json_form_widget/tests/src - tests/src + . tests/src/Unit From 61d463ed908999198d15cb2929474fb721392af9 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 20 Oct 2022 10:56:26 -0700 Subject: [PATCH 09/15] tests --- .../AlterTableQuery/MySQLQuery.php | 44 +++++++----- .../src/Functional/DictionaryEnforcerTest.php | 11 +-- .../AlterTableQuery/MySQLQueryTest.php | 69 ++++++++++++++++--- 3 files changed, 91 insertions(+), 33 deletions(-) diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index 5c9537ffa1..b1c998759e 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -68,6 +68,30 @@ class MySQLQuery extends AlterTableQueryBase implements AlterTableQueryInterface 'BLOB', ]; + /** + * Mapping between frictionless types and SQL types. + * + * Frictionless type is key, SQL is value. + * + * @var string[] + */ + protected static $frictionlessTypes = [ + 'string' => 'TEXT', + 'number' => 'DECIMAL', + 'integer' => 'INT', + 'date' => 'DATE', + 'time' => 'TIME', + 'datetime' => 'DATETIME', + 'year' => 'YEAR', + 'yearmonth' => 'TINYTEXT', + 'boolean' => 'BOOL', + 'object' => 'TEXT', + 'geopoint' => 'TEXT', + 'geojson' => 'TEXT', + 'array' => 'TEXT', + 'duration' => 'TINYTEXT', + ]; + /** * {@inheritdoc} */ @@ -237,22 +261,10 @@ protected function getFieldType(string $frictionless_type, string $column, strin * Base MySQL data type. */ protected function getBaseType(string $frictionless_type): string { - return ([ - 'string' => 'TEXT', - 'number' => 'DECIMAL', - 'integer' => 'INT', - 'date' => 'DATE', - 'time' => 'TIME', - 'datetime' => 'DATETIME', - 'year' => 'YEAR', - 'yearmonth' => 'TINYTEXT', - 'boolean' => 'BOOL', - 'object' => 'TEXT', - 'geopoint' => 'TEXT', - 'geojson' => 'TEXT', - 'array' => 'TEXT', - 'duration' => 'TINYTEXT', - ])[$frictionless_type]; + if ($sql_type = static::$frictionlessTypes[$frictionless_type] ?? FALSE) { + return $sql_type; + } + throw new \InvalidArgumentException($frictionless_type . ' is not a valid frictionless type.'); } /** diff --git a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php index e75619d7e7..f6da17ebd8 100644 --- a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php +++ b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php @@ -161,8 +161,7 @@ public function testDictionaryEnforcement(): void { 'type' => 'fulltext', ], ]; - $dd = $this->getDataDictionary($fields, $indexes, $dict_id); - $data_dict = $this->validMetadataFactory->get($dd, 'data-dictionary'); + $data_dict = $this->validMetadataFactory->get($this->getDataDictionary($fields, $indexes, $dict_id), 'data-dictionary'); // Create data-dictionary. $this->metastore->post('data-dictionary', $data_dict); $this->metastore->publish('data-dictionary', $dict_id); @@ -176,11 +175,7 @@ public function testDictionaryEnforcement(): void { // Build dataset. $dataset_id = $this->uuid->generate(); - $dataset = $this->validMetadataFactory->get( - $this->getDataset( - $dataset_id, 'Test ' . $dataset_id, [$this->resourceUrl], TRUE - ), 'dataset' - ); + $dataset = $this->validMetadataFactory->get($this->getDataset($dataset_id, 'Test ' . $dataset_id, [$this->resourceUrl], TRUE), 'dataset'); // Create dataset. $this->metastore->post('dataset', $dataset); $this->metastore->publish('dataset', $dataset_id); @@ -248,7 +243,7 @@ public function testDictionaryEnforcement(): void { 'd', ], ], - 'numOfRows' => 2, + 'numOfRows' => 3, ], $result); } diff --git a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php index ff0ef1d05a..49f518b2a3 100644 --- a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php +++ b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php @@ -152,19 +152,70 @@ public function baseTypeProvider() { * @dataProvider baseTypeProvider * @covers ::getBaseType */ - public function _testGetBaseType($frictionless_type, $expected_type) { - // Instantiate a MySQL query object. - $connection_chain = $this->buildConnectionChain(); - $table = 'datastore_' . uniqid(); - $mysql_query = $this->buildMySQLQuery($connection_chain->getMock(), $table); + public function testGetBaseType($frictionless_type, $expected_type) { + // Mock an object so we don't have to set up dependencies. + $mysql_query = $this->getMockBuilder(MySQLQuery::class) + ->disableOriginalConstructor() + // Don't mock the method we're testing. + ->setMethodsExcept(['getBaseType']) + ->getMock(); // getBaseType() is protected. $get_test_base = new \ReflectionMethod($mysql_query, 'getBaseType'); $get_test_base->setAccessible(TRUE); - $this->assertEquals( - $expected_type, - $get_test_base->invokeArgs($mysql_query, [$frictionless_type]) - ); + + // Handle the case of exceptions. + if ($expected_type === NULL) { + $this->expectException(\InvalidArgumentException::class); + } + + $sql_type = $get_test_base->invokeArgs($mysql_query, [$frictionless_type]); + if ($expected_type !== NULL) { + $this->assertEquals($expected_type, $sql_type); + } + } + + /** + * Make sure that buildBoolPreAlterCommands calls buildBoolPreAlterCommands. + * + * @covers ::buildBoolPreAlterCommands + * + * @todo This is brittle, feel free to replace. + */ + public function testBuildPreAlterCommandsForBool() { + // Our test data. + $query_fields = [[ + 'name' => 'col', + 'type' => 'boolean', + 'format' => 'default', + 'title' => 'Boolz', + ]]; + + // Mock an object so we can call buildPreAlterCommands() against it. + $mysql_query = $this->getMockBuilder(MySQLQuery::class) + ->disableOriginalConstructor() + // Mock buildBoolPreAlterCommands() so we can set expectations on it. + ->onlyMethods(['buildBoolPreAlterCommands']) + // Don't mock these methods because they're called from + // buildPreAlterCommands() + ->setMethodsExcept(['buildPreAlterCommands', 'getBaseType']) + ->getMock(); + $mysql_query->expects($this->once()) + ->method('buildBoolPreAlterCommands') + ->with( + $this->equalTo('table'), + $this->equalTo('col') + ); + + // Mock a connection so buildPreAlterCommands() can call it. + $query_connection = new \ReflectionProperty($mysql_query, 'connection'); + $query_connection->setAccessible(TRUE); + $query_connection->setValue($mysql_query, $this->buildConnectionChain()->getMock()); + + // Set up the method so we can run it. + $build_pre_alter_command = new \ReflectionMethod($mysql_query, 'buildPreAlterCommands'); + $build_pre_alter_command->setAccessible(TRUE); + $build_pre_alter_command->invokeArgs($mysql_query, [$query_fields, 'table']); } } From 0f935cc4e1be245b0e89f2c0e4b7d526d54e0524 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 20 Oct 2022 11:27:40 -0700 Subject: [PATCH 10/15] re-ignore coverage --- .../datastore_mysql_import/src/Factory/MysqlImportFactory.php | 2 ++ modules/datastore/src/Commands/PurgeCommands.php | 2 ++ modules/datastore/src/Controller/ImportController.php | 2 ++ modules/datastore/src/Drush.php | 2 ++ modules/datastore/src/EventSubscriber/DatastoreSubscriber.php | 1 + modules/datastore/src/Service/Factory/Import.php | 2 ++ .../src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php | 2 ++ 7 files changed, 13 insertions(+) diff --git a/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php b/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php index 8ac23c831e..88a9519cc5 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php @@ -10,6 +10,8 @@ /** * Importer factory. + * + * @codeCoverageIgnore */ class MysqlImportFactory implements ImportFactoryInterface { diff --git a/modules/datastore/src/Commands/PurgeCommands.php b/modules/datastore/src/Commands/PurgeCommands.php index 7747beff07..e4e3805e1d 100644 --- a/modules/datastore/src/Commands/PurgeCommands.php +++ b/modules/datastore/src/Commands/PurgeCommands.php @@ -8,6 +8,8 @@ /** * Datastore-related Drush commands. + * + * @codeCoverageIgnore */ class PurgeCommands extends DrushCommands { diff --git a/modules/datastore/src/Controller/ImportController.php b/modules/datastore/src/Controller/ImportController.php index 48839fd411..d7af540ddc 100644 --- a/modules/datastore/src/Controller/ImportController.php +++ b/modules/datastore/src/Controller/ImportController.php @@ -16,6 +16,8 @@ * Class Api. * * @package Drupal\datastore + * + * @codeCoverageIgnore */ class ImportController implements ContainerInjectionInterface { use JsonResponseTrait; diff --git a/modules/datastore/src/Drush.php b/modules/datastore/src/Drush.php index 9b2120e0e6..82a180ce32 100755 --- a/modules/datastore/src/Drush.php +++ b/modules/datastore/src/Drush.php @@ -11,6 +11,8 @@ /** * Drush commands for controlling the datastore. + * + * @codeCoverageIgnore */ class Drush extends DrushCommands { /** diff --git a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php index fdd58d1885..b10c4086c7 100644 --- a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php +++ b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php @@ -77,6 +77,7 @@ public function __construct(ConfigFactoryInterface $config_factory, LoggerChanne /** * Inherited. * + * @codeCoverageIgnore * @inheritdoc */ public static function getSubscribedEvents() { diff --git a/modules/datastore/src/Service/Factory/Import.php b/modules/datastore/src/Service/Factory/Import.php index de624469ea..a149518ea7 100644 --- a/modules/datastore/src/Service/Factory/Import.php +++ b/modules/datastore/src/Service/Factory/Import.php @@ -8,6 +8,8 @@ /** * Create an importer object for a given resource. + * + * @codeCoverageIgnore */ class Import implements ImportFactoryInterface { diff --git a/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php b/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php index e8ef6283c7..935cf95968 100644 --- a/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php +++ b/modules/datastore/src/SqlEndpoint/Form/DkanSqlEndpointSettingsForm.php @@ -12,6 +12,8 @@ * * @package Drupal\sql_endpoint\Form * @todo Remove + * + * @codeCoverageIgnore */ class DkanSqlEndpointSettingsForm extends ConfigFormBase { From c9a2906c88290f76277242fce8c855444491f52b Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 20 Oct 2022 12:02:27 -0700 Subject: [PATCH 11/15] expectations --- .../tests/src/Functional/DictionaryEnforcerTest.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php index f6da17ebd8..e5bcac8e79 100644 --- a/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php +++ b/modules/datastore/tests/src/Functional/DictionaryEnforcerTest.php @@ -196,7 +196,7 @@ public function testDictionaryEnforcement(): void { // Validate schema. $this->assertEquals([ - 'numOfColumns' => 5, + 'numOfColumns' => 6, 'columns' => [ 'record_number' => [ 'type' => 'serial', @@ -227,9 +227,11 @@ public function testDictionaryEnforcement(): void { 'description' => 'D', ], 'e' => [ - 'type' => 'boolean', - 'mysql_type' => 'BOOL', + 'type' => 'int', + 'mysql_type' => 'tinyint', 'description' => 'E', + 'length' => 1, + 'size' => 'tiny', ], ], 'indexes' => [ From e1e05ab5be993995d0064e72c25cb26b0fca4ee2 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 20 Oct 2022 12:59:34 -0700 Subject: [PATCH 12/15] refactored sanitizers and truncator to ImportJob --- .../src/Service/MysqlImport.php | 133 +------ .../src/Unit/Service/MysqlImportTest.php | 15 - .../AlterTableQuery/MySQLQuery.php | 6 +- .../src/Plugin/QueueWorker/ImportJob.php | 359 ++++++++++++++++++ .../Unit/Plugin/QueueWorker/ImportJobTest.php | 51 +++ 5 files changed, 417 insertions(+), 147 deletions(-) diff --git a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php index cac4c13d32..3d924ca897 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Service/MysqlImport.php @@ -24,65 +24,6 @@ class MysqlImport extends ImportJob { '\n' => "\n", ]; - /** - * The maximum length of a MySQL table column name. - * - * @var int - */ - protected const MAX_COLUMN_LENGTH = 64; - - /** - * List of reserved words in MySQL 5.6-8 and MariaDB. - * - * @var string[] - */ - protected const RESERVED_WORDS = ['accessible', 'add', 'all', 'alter', 'analyze', - 'and', 'as', 'asc', 'asensitive', 'before', 'between', 'bigint', 'binary', - 'blob', 'both', 'by', 'call', 'cascade', 'case', 'change', 'char', - 'character', 'check', 'collate', 'column', 'condition', 'constraint', - 'continue', 'convert', 'create', 'cross', 'cube', 'cume_dist', - 'current_date', 'current_role', 'current_time', 'current_timestamp', - 'current_user', 'cursor', 'database', 'databases', 'day_hour', - 'day_microsecond', 'day_minute', 'day_second', 'dec', 'decimal', 'declare', - 'default', 'delayed', 'delete', 'dense_rank', 'desc', 'describe', - 'deterministic', 'distinct', 'distinctrow', 'div', 'do_domain_ids', - 'double', 'drop', 'dual', 'each', 'else', 'elseif', 'empty', 'enclosed', - 'escaped', 'except', 'exists', 'exit', 'explain', 'false', 'fetch', - 'first_value', 'float', 'float4', 'float8', 'for', 'force', 'foreign', - 'from', 'fulltext', 'function', 'general', 'generated', 'get', 'grant', - 'group', 'grouping', 'groups', 'having', 'high_priority', 'hour_microsecond', - 'hour_minute', 'hour_second', 'if', 'ignore', 'ignore_domain_ids', - 'ignore_server_ids', 'in', 'index', 'infile', 'inner', 'inout', - 'insensitive', 'insert', 'int', 'int1', 'int2', 'int3', 'int4', 'int8', - 'integer', 'intersect', 'interval', 'into', 'io_after_gtids', - 'io_before_gtids', 'is', 'iterate', 'join', 'json_table', 'key', 'keys', - 'kill', 'lag', 'last_value', 'lateral', 'lead', 'leading', 'leave', 'left', - 'like', 'limit', 'linear', 'lines', 'load', 'localtime', 'localtimestamp', - 'lock', 'long', 'longblob', 'longtext', 'loop', 'low_priority', - 'master_bind', 'master_heartbeat_period', 'master_ssl_verify_server_cert', - 'match', 'maxvalue', 'mediumblob', 'mediumint', 'mediumtext', 'middleint', - 'minute_microsecond', 'minute_second', 'mod', 'modifies', 'natural', 'not', - 'no_write_to_binlog', 'nth_value', 'ntile', 'null', 'numeric', 'of', - 'offset', 'on', 'optimize', 'optimizer_costs', 'option', 'optionally', - 'or', 'order', 'out', 'outer', 'outfile', 'over', 'page_checksum', - 'parse_vcol_expr', 'partition', 'percent_rank', 'position', 'precision', - 'primary', 'procedure', 'purge', 'range', 'rank', 'read', 'reads', - 'read_write', 'real', 'recursive', 'references', 'ref_system_id', 'regexp', - 'release', 'rename', 'repeat', 'replace', 'require', 'resignal', - 'restrict', 'return', 'returning', 'revoke', 'right', 'rlike', 'row', - 'row_number', 'rows', 'schema', 'schemas', 'second_microsecond', 'select', - 'sensitive', 'separator', 'set', 'show', 'signal', 'slow', 'smallint', - 'spatial', 'specific', 'sql', 'sql_big_result', 'sql_calc_found_rows', - 'sqlexception', 'sql_small_result', 'sqlstate', 'sqlwarning', 'ssl', - 'starting', 'stats_auto_recalc', 'stats_persistent', 'stats_sample_pages', - 'stored', 'straight_join', 'system', 'table', 'terminated', 'then', - 'tinyblob', 'tinyint', 'tinytext', 'to', 'trailing', 'trigger', 'true', - 'undo', 'union', 'unique', 'unlock', 'unsigned', 'update', 'usage', 'use', - 'using', 'utc_date', 'utc_time', 'utc_timestamp', 'values', 'varbinary', - 'varchar', 'varcharacter', 'varying', 'virtual', 'when', 'where', 'while', - 'window', 'with', 'write', 'xor', 'year_month', 'zerofill', - ]; - /** * Override. * @@ -227,94 +168,28 @@ public function generateTableSpec(array $columns): array { foreach ($columns as $column) { // Sanitize the supplied table header to generate a unique column name; // null-coalesce potentially NULL column names to empty strings. - $name = self::sanitizeHeader($column ?? ''); + $name = ImportJob::sanitizeHeader($column ?? ''); // Truncate the generated table column name, if necessary, to fit the max // column length. - $name = self::truncateHeader($name); + $name = ImportJob::truncateHeader($name); // Generate unique numeric suffix for the header if a header already // exists with the same name. for ($i = 2; isset($spec[$name]); $i++) { $suffix = '_' . $i; - $name = substr($name, 0, self::MAX_COLUMN_LENGTH - strlen($suffix)) . $suffix; + $name = substr($name, 0, ImportJob::MAX_COLUMN_LENGTH - strlen($suffix)) . $suffix; } $spec[$name] = [ 'type' => "text", - 'description' => self::sanitizeDescription($column ?? ''), + 'description' => ImportJob::sanitizeDescription($column ?? ''), ]; } return $spec; } - /** - * Transform possible multiline string to single line for description. - * - * @param string $column - * Column name. - * - * @return string - * Column name on single line. - */ - public static function sanitizeDescription(string $column) { - $trimmed = array_filter(array_map('trim', explode("\n", $column))); - return implode(" ", $trimmed); - } - - /** - * Sanitize table column name according to the MySQL supported characters. - * - * @param string $column - * The column name being sanitized. - * - * @returns string - * Sanitized column name. - */ - public static function sanitizeHeader(string $column): string { - // Replace all spaces and newline characters with underscores since they are - // not supported. - $column = preg_replace('/(?: |\r\n|\r|\n)/', '_', $column); - // Strip unsupported characters from the header. - $column = preg_replace('/[^A-Za-z0-9_]/', '', $column); - // Trim underscores from the beginning and end of the column name. - $column = trim($column, '_'); - // Convert the column name to lowercase. - $column = strtolower($column); - - if (is_numeric($column) || in_array($column, self::RESERVED_WORDS)) { - // Prepend "_" to column name that are not allowed in MySQL - // This can be dropped after move to Drupal 9. - // @see https://github.com/GetDKAN/dkan/issues/3606 - $column = '_' . $column; - } - - return $column; - } - - /** - * Truncate column name if longer than the max column length for the database. - * - * @param string $column - * The column name being truncated. - * - * @returns string - * Truncated column name. - */ - protected static function truncateHeader(string $column): string { - // If the supplied table column name is longer than the max column length, - // truncate the column name to 5 characters under the max length and - // substitute the truncated characters with a unique hash. - if (strlen($column) > self::MAX_COLUMN_LENGTH) { - $field = substr($column, 0, self::MAX_COLUMN_LENGTH - 5); - $hash = substr(md5($column), 0, 4); - $column = $field . '_' . $hash; - } - - return $column; - } - /** * Construct a SQL file import statement using the given file information. * diff --git a/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php b/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php index 022e25d6b4..a3f1aaa7bf 100644 --- a/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php +++ b/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php @@ -128,21 +128,6 @@ public function testMysqlImporterWithCSVFileWithNewLinesInHeaders() { ])); } - public function sanitizeHeaderProvider() { - return [ - 'reserved_word' => ['accessible', '_accessible'], - 'numeric' => [1, '_1'], - ]; - } - - /** - * @dataProvider sanitizeHeaderProvider - * @covers ::sanitizeHeader - */ - public function testSanitizeHeader($column, $expected) { - $this->assertEquals(MysqlImport::sanitizeHeader($column), $expected); - } - protected function getMysqlImporter() { $resource = new DataResource(self::HOST . '/text.csv', 'text/csv'); $databaseTableFactory = $this->getDatabaseTableFactoryMock(); diff --git a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php index b1c998759e..7a8de94f7b 100644 --- a/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php +++ b/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php @@ -4,7 +4,7 @@ use Drupal\Core\Database\StatementInterface; -use Drupal\datastore_mysql_import\Service\MysqlImport; +use Drupal\datastore\Plugin\QueueWorker\ImportJob; use Drupal\datastore\DataDictionary\AlterTableQueryBase; use Drupal\datastore\DataDictionary\AlterTableQueryInterface; use Drupal\datastore\DataDictionary\IncompatibleTypeException; @@ -130,7 +130,7 @@ protected function sanitizeFields(array $fields): array { // Create reference to index field name. $field_name = &$fields[$key]['name']; // Sanitize field name. - $field_name = MysqlImport::sanitizeHeader($field_name); + $field_name = ImportJob::sanitizeHeader($field_name); } return $fields; @@ -156,7 +156,7 @@ protected function sanitizeIndexes(array $indexes): array { // Create reference to index field name. $field_name = &$index_fields[$field_key]['name']; // Sanitize field name. - $field_name = MysqlImport::sanitizeHeader($field_name); + $field_name = ImportJob::sanitizeHeader($field_name); } } diff --git a/modules/datastore/src/Plugin/QueueWorker/ImportJob.php b/modules/datastore/src/Plugin/QueueWorker/ImportJob.php index 2c38058d2b..92c2ebc60a 100644 --- a/modules/datastore/src/Plugin/QueueWorker/ImportJob.php +++ b/modules/datastore/src/Plugin/QueueWorker/ImportJob.php @@ -13,6 +13,299 @@ */ class ImportJob extends AbstractPersistentJob { + /** + * The maximum length of a MySQL table column name. + * + * @var int + */ + protected const MAX_COLUMN_LENGTH = 64; + + /** + * List of reserved words in MySQL 5.6-8 and MariaDB. + * + * @var string[] + */ + protected const RESERVED_WORDS = [ + 'accessible', + 'add', + 'all', + 'alter', + 'analyze', + 'and', + 'as', + 'asc', + 'asensitive', + 'before', + 'between', + 'bigint', + 'binary', + 'blob', + 'both', + 'by', + 'call', + 'cascade', + 'case', + 'change', + 'char', + 'character', + 'check', + 'collate', + 'column', + 'condition', + 'constraint', + 'continue', + 'convert', + 'create', + 'cross', + 'cube', + 'cume_dist', + 'current_date', + 'current_role', + 'current_time', + 'current_timestamp', + 'current_user', + 'cursor', + 'database', + 'databases', + 'day_hour', + 'day_microsecond', + 'day_minute', + 'day_second', + 'dec', + 'decimal', + 'declare', + 'default', + 'delayed', + 'delete', + 'dense_rank', + 'desc', + 'describe', + 'deterministic', + 'distinct', + 'distinctrow', + 'div', + 'do_domain_ids', + 'double', + 'drop', + 'dual', + 'each', + 'else', + 'elseif', + 'empty', + 'enclosed', + 'escaped', + 'except', + 'exists', + 'exit', + 'explain', + 'false', + 'fetch', + 'first_value', + 'float', + 'float4', + 'float8', + 'for', + 'force', + 'foreign', + 'from', + 'fulltext', + 'function', + 'general', + 'generated', + 'get', + 'grant', + 'group', + 'grouping', + 'groups', + 'having', + 'high_priority', + 'hour_microsecond', + 'hour_minute', + 'hour_second', + 'if', + 'ignore', + 'ignore_domain_ids', + 'ignore_server_ids', + 'in', + 'index', + 'infile', + 'inner', + 'inout', + 'insensitive', + 'insert', + 'int', + 'int1', + 'int2', + 'int3', + 'int4', + 'int8', + 'integer', + 'intersect', + 'interval', + 'into', + 'io_after_gtids', + 'io_before_gtids', + 'is', + 'iterate', + 'join', + 'json_table', + 'key', + 'keys', + 'kill', + 'lag', + 'last_value', + 'lateral', + 'lead', + 'leading', + 'leave', + 'left', + 'like', + 'limit', + 'linear', + 'lines', + 'load', + 'localtime', + 'localtimestamp', + 'lock', + 'long', + 'longblob', + 'longtext', + 'loop', + 'low_priority', + 'master_bind', + 'master_heartbeat_period', + 'master_ssl_verify_server_cert', + 'match', + 'maxvalue', + 'mediumblob', + 'mediumint', + 'mediumtext', + 'middleint', + 'minute_microsecond', + 'minute_second', + 'mod', + 'modifies', + 'natural', + 'not', + 'no_write_to_binlog', + 'nth_value', + 'ntile', + 'null', + 'numeric', + 'of', + 'offset', + 'on', + 'optimize', + 'optimizer_costs', + 'option', + 'optionally', + 'or', + 'order', + 'out', + 'outer', + 'outfile', + 'over', + 'page_checksum', + 'parse_vcol_expr', + 'partition', + 'percent_rank', + 'position', + 'precision', + 'primary', + 'procedure', + 'purge', + 'range', + 'rank', + 'read', + 'reads', + 'read_write', + 'real', + 'recursive', + 'references', + 'ref_system_id', + 'regexp', + 'release', + 'rename', + 'repeat', + 'replace', + 'require', + 'resignal', + 'restrict', + 'return', + 'returning', + 'revoke', + 'right', + 'rlike', + 'row', + 'row_number', + 'rows', + 'schema', + 'schemas', + 'second_microsecond', + 'select', + 'sensitive', + 'separator', + 'set', + 'show', + 'signal', + 'slow', + 'smallint', + 'spatial', + 'specific', + 'sql', + 'sql_big_result', + 'sql_calc_found_rows', + 'sqlexception', + 'sql_small_result', + 'sqlstate', + 'sqlwarning', + 'ssl', + 'starting', + 'stats_auto_recalc', + 'stats_persistent', + 'stats_sample_pages', + 'stored', + 'straight_join', + 'system', + 'table', + 'terminated', + 'then', + 'tinyblob', + 'tinyint', + 'tinytext', + 'to', + 'trailing', + 'trigger', + 'true', + 'undo', + 'union', + 'unique', + 'unlock', + 'unsigned', + 'update', + 'usage', + 'use', + 'using', + 'utc_date', + 'utc_time', + 'utc_timestamp', + 'values', + 'varbinary', + 'varchar', + 'varcharacter', + 'varying', + 'virtual', + 'when', + 'where', + 'while', + 'window', + 'with', + 'write', + 'xor', + 'year_month', + 'zerofill', + ]; + /** * Storage class. * @@ -60,6 +353,72 @@ protected function __construct(string $identifier, $storage, array $config = NUL $this->resource = $config['resource']; } + /** + * Transform possible multiline string to single line for description. + * + * @param string $column + * Column name. + * + * @return string + * Column name on single line. + */ + public static function sanitizeDescription(string $column) { + $trimmed = array_filter(array_map('trim', explode("\n", $column))); + return implode(" ", $trimmed); + } + + /** + * Sanitize table column name according to the MySQL supported characters. + * + * @param string $column + * The column name being sanitized. + * + * @returns string + * Sanitized column name. + */ + public static function sanitizeHeader(string $column): string { + // Replace all spaces and newline characters with underscores since they are + // not supported. + $column = preg_replace('/(?: |\r\n|\r|\n)/', '_', $column); + // Strip unsupported characters from the header. + $column = preg_replace('/[^A-Za-z0-9_]/', '', $column); + // Trim underscores from the beginning and end of the column name. + $column = trim($column, '_'); + // Convert the column name to lowercase. + $column = strtolower($column); + + if (is_numeric($column) || in_array($column, ImportJob::RESERVED_WORDS)) { + // Prepend "_" to column name that are not allowed in MySQL + // This can be dropped after move to Drupal 9. + // @see https://github.com/GetDKAN/dkan/issues/3606 + $column = '_' . $column; + } + + return $column; + } + + /** + * Truncate column name if longer than the max column length for the database. + * + * @param string $column + * The column name being truncated. + * + * @returns string + * Truncated column name. + */ + public static function truncateHeader(string $column): string { + // If the supplied table column name is longer than the max column length, + // truncate the column name to 5 characters under the max length and + // substitute the truncated characters with a unique hash. + if (strlen($column) > ImportJob::MAX_COLUMN_LENGTH) { + $field = substr($column, 0, ImportJob::MAX_COLUMN_LENGTH - 5); + $hash = substr(md5($column), 0, 4); + $column = $field . '_' . $hash; + } + + return $column; + } + /** * Get the storage object. */ diff --git a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php b/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php index e86e37131c..63929fb1c3 100644 --- a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php +++ b/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php @@ -238,4 +238,55 @@ public function testNonStorage() { ]); } + public function sanitizeDescriptionProvider() { + return [ + 'multiline' => ["Multi\nLine", 'Multi Line'], + ]; + } + + /** + * @dataProvider sanitizeDescriptionProvider + * @covers \Drupal\datastore\Plugin\QueueWorker\ImportJob::sanitizeDescription + */ + public function testSanitizeDescription($column, $expected) { + $this->assertEquals($expected, ImportJob::sanitizeDescription($column)); + } + + public function sanitizeHeaderProvider() { + return [ + 'reserved_word' => ['accessible', '_accessible'], + 'numeric' => [1, '_1'], + ]; + } + + /** + * @dataProvider sanitizeHeaderProvider + * @covers \Drupal\datastore\Plugin\QueueWorker\ImportJob::sanitizeHeader + */ + public function testSanitizeHeader($column, $expected) { + $this->assertEquals($expected, ImportJob::sanitizeHeader($column)); + } + + public function truncateHeaderProvider() { + $max_length = 64; + return [ + 'max_length' => [ + str_repeat('a', $max_length), + $max_length, + ], + 'longer_length' => [ + str_repeat('b', $max_length + 1), + $max_length, + ], + ]; + } + + /** + * @dataProvider truncateHeaderProvider + * @covers \Drupal\datastore\Plugin\QueueWorker\ImportJob::truncateHeader + */ + public function testTruncateHeader($column, $expected) { + $this->assertEquals($expected, strlen(ImportJob::truncateHeader($column))); + } + } From 2b62779a9c3bdce2d56175afe147c42c92098cc0 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 20 Oct 2022 13:06:18 -0700 Subject: [PATCH 13/15] CS the CS --- .../src/Plugin/QueueWorker/ImportJob.php | 322 +++--------------- 1 file changed, 44 insertions(+), 278 deletions(-) diff --git a/modules/datastore/src/Plugin/QueueWorker/ImportJob.php b/modules/datastore/src/Plugin/QueueWorker/ImportJob.php index 92c2ebc60a..2625b2a23f 100644 --- a/modules/datastore/src/Plugin/QueueWorker/ImportJob.php +++ b/modules/datastore/src/Plugin/QueueWorker/ImportJob.php @@ -26,284 +26,50 @@ class ImportJob extends AbstractPersistentJob { * @var string[] */ protected const RESERVED_WORDS = [ - 'accessible', - 'add', - 'all', - 'alter', - 'analyze', - 'and', - 'as', - 'asc', - 'asensitive', - 'before', - 'between', - 'bigint', - 'binary', - 'blob', - 'both', - 'by', - 'call', - 'cascade', - 'case', - 'change', - 'char', - 'character', - 'check', - 'collate', - 'column', - 'condition', - 'constraint', - 'continue', - 'convert', - 'create', - 'cross', - 'cube', - 'cume_dist', - 'current_date', - 'current_role', - 'current_time', - 'current_timestamp', - 'current_user', - 'cursor', - 'database', - 'databases', - 'day_hour', - 'day_microsecond', - 'day_minute', - 'day_second', - 'dec', - 'decimal', - 'declare', - 'default', - 'delayed', - 'delete', - 'dense_rank', - 'desc', - 'describe', - 'deterministic', - 'distinct', - 'distinctrow', - 'div', - 'do_domain_ids', - 'double', - 'drop', - 'dual', - 'each', - 'else', - 'elseif', - 'empty', - 'enclosed', - 'escaped', - 'except', - 'exists', - 'exit', - 'explain', - 'false', - 'fetch', - 'first_value', - 'float', - 'float4', - 'float8', - 'for', - 'force', - 'foreign', - 'from', - 'fulltext', - 'function', - 'general', - 'generated', - 'get', - 'grant', - 'group', - 'grouping', - 'groups', - 'having', - 'high_priority', - 'hour_microsecond', - 'hour_minute', - 'hour_second', - 'if', - 'ignore', - 'ignore_domain_ids', - 'ignore_server_ids', - 'in', - 'index', - 'infile', - 'inner', - 'inout', - 'insensitive', - 'insert', - 'int', - 'int1', - 'int2', - 'int3', - 'int4', - 'int8', - 'integer', - 'intersect', - 'interval', - 'into', - 'io_after_gtids', - 'io_before_gtids', - 'is', - 'iterate', - 'join', - 'json_table', - 'key', - 'keys', - 'kill', - 'lag', - 'last_value', - 'lateral', - 'lead', - 'leading', - 'leave', - 'left', - 'like', - 'limit', - 'linear', - 'lines', - 'load', - 'localtime', - 'localtimestamp', - 'lock', - 'long', - 'longblob', - 'longtext', - 'loop', - 'low_priority', - 'master_bind', - 'master_heartbeat_period', - 'master_ssl_verify_server_cert', - 'match', - 'maxvalue', - 'mediumblob', - 'mediumint', - 'mediumtext', - 'middleint', - 'minute_microsecond', - 'minute_second', - 'mod', - 'modifies', - 'natural', - 'not', - 'no_write_to_binlog', - 'nth_value', - 'ntile', - 'null', - 'numeric', - 'of', - 'offset', - 'on', - 'optimize', - 'optimizer_costs', - 'option', - 'optionally', - 'or', - 'order', - 'out', - 'outer', - 'outfile', - 'over', - 'page_checksum', - 'parse_vcol_expr', - 'partition', - 'percent_rank', - 'position', - 'precision', - 'primary', - 'procedure', - 'purge', - 'range', - 'rank', - 'read', - 'reads', - 'read_write', - 'real', - 'recursive', - 'references', - 'ref_system_id', - 'regexp', - 'release', - 'rename', - 'repeat', - 'replace', - 'require', - 'resignal', - 'restrict', - 'return', - 'returning', - 'revoke', - 'right', - 'rlike', - 'row', - 'row_number', - 'rows', - 'schema', - 'schemas', - 'second_microsecond', - 'select', - 'sensitive', - 'separator', - 'set', - 'show', - 'signal', - 'slow', - 'smallint', - 'spatial', - 'specific', - 'sql', - 'sql_big_result', - 'sql_calc_found_rows', - 'sqlexception', - 'sql_small_result', - 'sqlstate', - 'sqlwarning', - 'ssl', - 'starting', - 'stats_auto_recalc', - 'stats_persistent', - 'stats_sample_pages', - 'stored', - 'straight_join', - 'system', - 'table', - 'terminated', - 'then', - 'tinyblob', - 'tinyint', - 'tinytext', - 'to', - 'trailing', - 'trigger', - 'true', - 'undo', - 'union', - 'unique', - 'unlock', - 'unsigned', - 'update', - 'usage', - 'use', - 'using', - 'utc_date', - 'utc_time', - 'utc_timestamp', - 'values', - 'varbinary', - 'varchar', - 'varcharacter', - 'varying', - 'virtual', - 'when', - 'where', - 'while', - 'window', - 'with', - 'write', - 'xor', - 'year_month', - 'zerofill', + 'accessible', 'add', 'all', 'alter', 'analyze', 'and', 'as', 'asc', + 'asensitive', 'before', 'between', 'bigint', 'binary', 'blob', 'both', 'by', + 'call', 'cascade', 'case', 'change', 'char', 'character', 'check', + 'collate', 'column', 'condition', 'constraint', 'continue', 'convert', + 'create', 'cross', 'cube', 'cume_dist', 'current_date', 'current_role', + 'current_time', 'current_timestamp', 'current_user', 'cursor', 'database', + 'databases', 'day_hour', 'day_microsecond', 'day_minute', 'day_second', + 'dec', 'decimal', 'declare', 'default', 'delayed', 'delete', 'dense_rank', + 'desc', 'describe', 'deterministic', 'distinct', 'distinctrow', 'div', + 'do_domain_ids', 'double', 'drop', 'dual', 'each', 'else', 'elseif', + 'empty', 'enclosed', 'escaped', 'except', 'exists', 'exit', 'explain', + 'false', 'fetch', 'first_value', 'float', 'float4', 'float8', 'for', + 'force', 'foreign', 'from', 'fulltext', 'function', 'general', 'generated', + 'get', 'grant', 'group', 'grouping', 'groups', 'having', 'high_priority', + 'hour_microsecond', 'hour_minute', 'hour_second', 'if', 'ignore', + 'ignore_domain_ids', 'ignore_server_ids', 'in', 'index', 'infile', 'inner', + 'inout', 'insensitive', 'insert', 'int', 'int1', 'int2', 'int3', 'int4', + 'int8', 'integer', 'intersect', 'interval', 'into', 'io_after_gtids', + 'io_before_gtids', 'is', 'iterate', 'join', 'json_table', 'key', 'keys', + 'kill', 'lag', 'last_value', 'lateral', 'lead', 'leading', 'leave', 'left', + 'like', 'limit', 'linear', 'lines', 'load', 'localtime', 'localtimestamp', + 'lock', 'long', 'longblob', 'longtext', 'loop', 'low_priority', + 'master_bind', 'master_heartbeat_period', 'master_ssl_verify_server_cert', + 'match', 'maxvalue', 'mediumblob', 'mediumint', 'mediumtext', 'middleint', + 'minute_microsecond', 'minute_second', 'mod', 'modifies', 'natural', 'not', + 'no_write_to_binlog', 'nth_value', 'ntile', 'null', 'numeric', 'of', + 'offset', 'on', 'optimize', 'optimizer_costs', 'option', 'optionally', 'or', + 'order', 'out', 'outer', 'outfile', 'over', 'page_checksum', + 'parse_vcol_expr', 'partition', 'percent_rank', 'position', 'precision', + 'primary', 'procedure', 'purge', 'range', 'rank', 'read', 'reads', + 'read_write', 'real', 'recursive', 'references', 'ref_system_id', 'regexp', + 'release', 'rename', 'repeat', 'replace', 'require', 'resignal', 'restrict', + 'return', 'returning', 'revoke', 'right', 'rlike', 'row', 'row_number', + 'rows', 'schema', 'schemas', 'second_microsecond', 'select', 'sensitive', + 'separator', 'set', 'show', 'signal', 'slow', 'smallint', 'spatial', + 'specific', 'sql', 'sql_big_result', 'sql_calc_found_rows', 'sqlexception', + 'sql_small_result', 'sqlstate', 'sqlwarning', 'ssl', 'starting', + 'stats_auto_recalc', 'stats_persistent', 'stats_sample_pages', 'stored', + 'straight_join', 'system', 'table', 'terminated', 'then', 'tinyblob', + 'tinyint', 'tinytext', 'to', 'trailing', 'trigger', 'true', 'undo', 'union', + 'unique', 'unlock', 'unsigned', 'update', 'usage', 'use', 'using', + 'utc_date', 'utc_time', 'utc_timestamp', 'values', 'varbinary', 'varchar', + 'varcharacter', 'varying', 'virtual', 'when', 'where', 'while', 'window', + 'with', 'write', 'xor', 'year_month', 'zerofill', ]; /** From 50cd93794ca711488723d580fabc793900aa7a62 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 20 Oct 2022 13:43:46 -0700 Subject: [PATCH 14/15] cleanups --- .../tests/src/Unit/Service/MysqlImportTest.php | 6 +----- .../Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php b/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php index a3f1aaa7bf..aa80d139de 100644 --- a/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php +++ b/modules/datastore/modules/datastore_mysql_import/tests/src/Unit/Service/MysqlImportTest.php @@ -25,7 +25,7 @@ use Symfony\Component\HttpFoundation\RequestStack; /** - * @coversDefaultClass Drupal\datastore_mysql_import\Service\MysqlImport + * */ class MysqlImportTest extends TestCase { @@ -34,10 +34,6 @@ class MysqlImportTest extends TestCase { /** * Test spec generation. - * - * @covers ::generateTableSpec - * @covers ::sanitizeHeader - * @covers ::truncateHeader */ public function testGenerateTableSpec() { $mysqlImporter = $this->getMysqlImporter(); diff --git a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php index 49f518b2a3..7589232e13 100644 --- a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php +++ b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php @@ -143,7 +143,6 @@ public function testExecuteWithTooLargeDecimal(): void { public function baseTypeProvider() { return [ 'string' => ['string', 'TEXT'], - // @todo getBaseType() performs no type checking, has no error result. 'getBaseType-does-no-error-checking' => ['not-a-frictionless-type', NULL], ]; } From 276d3aa79685d4b0d5fba28c4d38afcfe95f78e4 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Fri, 21 Oct 2022 12:21:40 -0700 Subject: [PATCH 15/15] revert bool away --- .../Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php index 7589232e13..cea5f3554a 100644 --- a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php +++ b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php @@ -39,8 +39,6 @@ public function setUp(): void { /** * Build MySQLQuery arguments. - * - * @todo Include an update() mock for buildBoolPreAlterCommands(). */ public function buildConnectionChain(): Chain { return (new Chain($this)) @@ -52,7 +50,6 @@ public function buildConnectionChain(): Chain { 'foo' => 'Foo', 'bar' => 'Bar', 'baz' => 'Baz', - 'boolz' => 'Boolz', ]) ->add(StatementInterface::class, 'fetchField', (new Sequence()) ->add('5') @@ -74,7 +71,6 @@ public function buildMySQLQuery(Connection $connection, ?string $table = NULL, ? ['name' => 'foo', 'type' => 'string', 'format' => 'default', 'title' => 'Foo'], ['name' => 'bar', 'type' => 'number', 'format' => 'default', 'title' => 'Bar'], ['name' => 'baz', 'type' => 'date', 'format' => '%Y-%m-%d', 'title' => 'Baz'], - ['name' => 'boolz', 'type' => 'boolean', 'format' => 'default', 'title' => 'Boolz'], ]; $dictionary_indexes ??= [ ['name' => 'index1', 'type' => 'index', 'description' => 'Fizz', 'fields' => [