Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of empty typed columns in Alter Table MySQL query and sanitize data-dictionary field names #3844

Merged
merged 17 commits into from
Nov 1, 2022

Conversation

clayliddell
Copy link
Contributor

@clayliddell clayliddell commented Oct 7, 2022

This PR is intended to clean up some bugs in the AlterTableQuery/MySQLQuery class. The changes in this PR are:

  1. Converting all empty non-text table values to NULL on import to avoid typing related errors in MySQL.
  2. Sanitizing all field names and index field names in data-dictionaries to properly reflect their table equivalents (this should allow content creators to specify the CSV column name inside of data-dictionaries instead of the serialized table column name).
  • Test coverage exists
  • Documentation exists

QA Steps

  • Create a data-dictionary with the following columns and types:
  • name: number type: number format: default
  • name: integer type: integer format: default
  • name: date type: date format: %m/%d/%Y
  • name: datetime type: datetime format: %m/%d/%Y %I:%M %p
  • name: year type: year format: %Y
  • name: yearmonth type: yearmonth format: %Y/%m
  • name: boolean type: boolean format: default
  • Configure the created data-dictionary as the sitewide data-dictionary.
  • Create a remote dataset from: https://gist.githubusercontent.com/clayliddell/111a59210d5d416af4d938bcb725be27/raw/bb2cc37f58a2c86944fe5c63b317b8f962be1fcf/dd-null-test.csv
  • Ensure that it imports and displays correctly without problem (and with null values for non-text fields).

@janette
Copy link
Member

janette commented Oct 10, 2022

The fix is relying on datastore_mysql_import, this is not enabled by default as it depends on specific db config.

Error: Class 'Drupal\datastore_mysql_import\Service\MysqlImport' not found in /var/www/html/dkan/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php on line 109 #0 /var/www/html/dkan/modules/datastore/src/DataDictionary/AlterTableQuery/MySQLQuery.php(76): Drupal\datastore\DataDictionary\AlterTableQuery\MySQLQuery->sanitizeFields(Array)

When that module is enabled, all but the time columns import correctly.
The 'time' column errors with the following:

[error]  SQLSTATE[HY000]: General error: 1411 Incorrect datetime value: '12:21 AM' for function str_to_date: UPDATE "datastore_e7fb0581f6133041c4c5b94c26246488" SET "time"=STR_TO_DATE(time, :date_format); Array
(
    [:date_format] => %H:%I %p
)

image

sometimes the error looks like this

[error]  SQLSTATE[HY000]: General error: 1411 Incorrect datetime value: '12:21 AM' for function str_to_date: UPDATE "datastore_0c163e1d562132d8323cc2ce3e31f839" SET "time"=STR_TO_DATE(time, :date_format); Array
(
    [:date_format] => %I:%i %p
)

And if the format is left at 'default'

SQLSTATE[22007]: Invalid datetime format: 1292 Incorrect time value: '12:21 AM' for column 'time' at row 1

Copy link
Member

@janette janette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need more information on how time values are imported.

@@ -4,6 +4,7 @@

use Drupal\Core\Database\StatementInterface;

use Drupal\datastore_mysql_import\Service\MysqlImport;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is not enabled by default.

Copy link
Member

@janette janette Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the sanitizing and truncating to common/src/Storage/AbstractDatabaseTable.php datastore/src/Storage/DatabaseTable.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OR maybe the sanitizing/truncating of things should be in Drupal\datastore\Plugin\QueueWorker\ImportJob.php

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved sanitizing/truncating methods to ImportJob.

@paul-m paul-m self-assigned this Oct 12, 2022
Copy link
Member

@janette janette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent

@janette janette merged commit c98455b into 2.x Nov 1, 2022
@janette janette deleted the fix-empty-and-sanitize branch May 19, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants