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

Use same sanitization when comparing properties #4233

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

janette
Copy link
Member

@janette janette commented Jul 19, 2024

When applying date formats defined in the data dictionary we are comparing
the dictionary metadata to the query properties that have been sanitized, so we must also sanitize the metadata fields.
""start_date"" !== "start_date"

Warning: Trying to access array offset on value of type null in Drupal\common\Storage\SelectFactory->addDateExpressions() (line 108 of /mnt/www/html/datamedicaidstg/docroot/modules/contrib/dkan/mo
dules/common/src/Storage/SelectFactory.php)
Warning: Undefined array key "start_date" in Drupal\common\Storage\SelectFactory->addDateExpressions() (line 108 of /mnt/www/html/datamedicaidstg/docroot/modules/contrib/dkan/modules/common/src/Storage/SelectFactory.php)

Steps to reproduce

  • checkout 2.x
  • Enable dblog
  • Add a data dictionary defining a data type = date, format = other, Other format = %m/%d/%Y, OR use the provided POST below.
POST https://dkan.ddev.site/api/1/metastore/schemas/data-dictionary/items HTTP/1.1
Content-Type: application/json
Authorization: Basic admin:admin

{
  "data": {
    "title": "Test Dictionary",
    "fields": [
      {
        "name": "string",
        "title": "String example",
        "type": "string",
        "format": "default"
      },
      {
        "name": "number",
        "title": "Number",
        "type": "number",
        "format": "default"
      },
      {
        "name": "start_date",
        "title": "Start Date",
        "type": "date",
        "format": "%m/%d/%Y"
      }
    ]
  }
}
  • Create a dataset with the attached data file as your resource. Enter the data dictionary endpoint that you just created into the distribution Data Dictionary field
    --> test-data.csv
  • Add "application/vnd.tableschema+json" as Data Dictionary Type
  • Save and run the import queues
ddev drush queue:run localize_import
ddev drush queue:run datastore_import
ddev drush queue:run post_import

QA Steps

@janette janette requested a review from paul-m July 19, 2024 04:37
@paul-m
Copy link
Contributor

paul-m commented Jul 19, 2024

Here's what I did:

git checkout update-addDateExpressions
ddev dkan-site-install
ddev drush pm-enable dblog
ddev drush pm-enable sample_content
ddev drush dkan:sample-content:create
# Use UI to create a data dictionary for 'date' field.
# Use UI to set the site to use the distribution dictionary.
# Use UI to link gold prices dataset to the data dictionary, using the URL from the data dictionary edit page.
ddev drush cron
ddev drush cron

This yields this error for all datasets:

No data-dictionary found for resource with id "0310f6c2d28bbee5de1211e5b5bb0b1c" and version "1721400760".

If we use a sitewide dictionary, we get this error:

SQLSTATE[HY000]: General error: 1411 Incorrect datetime value: '1950-01-01' for function str_to_date: UPDATE "datastore_eec0e7275ffb7442a6083ce82cddeff3" SET "date"=STR_TO_DATE(date, :date_format); Array ( [:date_format] => %m/%d/%Y )

Unfortunately, we get the same result on 2.x, in addition to this branch, so clearly the repro steps are not great.

Copy link
Contributor

@dmundra dmundra left a comment

Choose a reason for hiding this comment

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

@janette and @paul-m the addition of the test 'testDownloadWithDataDictionary' works well to test the CSV export has the correct date format.

@dmundra dmundra merged commit 46ba5e4 into 2.x Sep 6, 2024
11 checks passed
@janette
Copy link
Member Author

janette commented Sep 9, 2024

This is a short term fix. Will refactor the structural and escaping aspects of normalizeProperty() into two separate methods to replace this change.

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.

3 participants