fix: change the validation logic for python_date_format (#25510)

Co-authored-by: John Bodley <john.bodley@gmail.com>

UPDATING.md

@@ -44,6 +44,7 @@ assists people when migrating to a new version.
 - [26462](https://github.com/apache/superset/issues/26462): Removes the Profile feature given that it's not actively maintained and not widely used.
 - [26377](https://github.com/apache/superset/pull/26377): Removes the deprecated Redirect API that supported short URLs used before the permalink feature.
 - [26329](https://github.com/apache/superset/issues/26329): Removes the deprecated `DASHBOARD_NATIVE_FILTERS` feature flag. The previous value of the feature flag was `True` and now the feature is permanently enabled.
+- [25510](https://github.com/apache/superset/pull/25510): Reenforces that any newly defined Python data format (other than epoch) must adhere to the ISO 8601 standard (enforced by way of validation at the API and database level) after a previous relaxation to include slashes in addition to dashes. From now on when specifying new columns, dataset owners will need to use a SQL expression instead to convert their string columns of the form %Y/%m/%d etc. to a `DATE`, `DATETIME`, etc. type.
 
 ### Potential Downtime
 

superset-frontend/src/components/Datasource/DatasourceEditor.jsx

@@ -329,7 +329,7 @@ function ColumnCollectionTable({
               control={
                 <TextControl
                   controlId="python_date_format"
-                  placeholder="%Y/%m/%d"
+                  placeholder="%Y-%m-%d"
                 />
               }
             />

superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx

@@ -96,7 +96,7 @@ describe('DatasourceEditor', () => {
 
     const inputLabel = screen.getByPlaceholderText('Label');
     const inputDescription = screen.getByPlaceholderText('Description');
-    const inputDtmFormat = screen.getByPlaceholderText('%Y/%m/%d');
+    const inputDtmFormat = screen.getByPlaceholderText('%Y-%m-%d');
     const inputCertifiedBy = screen.getByPlaceholderText('Certified by');
     const inputCertDetails = screen.getByPlaceholderText(
       'Certification details',

superset/daos/dataset.py

@@ -17,12 +17,15 @@
 from __future__ import annotations
 
 import logging
+from datetime import datetime
 from typing import Any
 
+import dateutil.parser
 from sqlalchemy.exc import SQLAlchemyError
 
 from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
 from superset.daos.base import BaseDAO
+from superset.daos.exceptions import DAOUpdateFailedError
 from superset.extensions import db
 from superset.models.core import Database
 from superset.models.dashboard import Dashboard
@@ -150,6 +153,17 @@ class DatasetDAO(BaseDAO[SqlaTable]):
         ).all()
         return len(dataset_query) == 0
 
+    @staticmethod
+    def validate_python_date_format(dt_format: str) -> bool:
+        if dt_format in ("epoch_s", "epoch_ms"):
+            return True
+        try:
+            dt_str = datetime.now().strftime(dt_format)
+            dateutil.parser.isoparse(dt_str)
+            return True
+        except ValueError:
+            return False
+
     @classmethod
     def update(
         cls,
@@ -193,6 +207,18 @@ class DatasetDAO(BaseDAO[SqlaTable]):
         then we delete.
         """
 
+        for column in property_columns:
+            if (
+                "python_date_format" in column
+                and column["python_date_format"] is not None
+            ):
+                if not DatasetDAO.validate_python_date_format(
+                    column["python_date_format"]
+                ):
+                    raise DAOUpdateFailedError(
+                        "python_date_format is an invalid date/timestamp format."
+                    )
+
         if override_columns:
             db.session.query(TableColumn).filter(
                 TableColumn.table_id == model.id

superset/datasets/schemas.py

@@ -15,9 +15,10 @@
 # specific language governing permissions and limitations
 # under the License.
 import json
-import re
+from datetime import datetime
 from typing import Any
 
+from dateutil.parser import isoparse
 from flask_babel import lazy_gettext as _
 from marshmallow import fields, pre_load, Schema, ValidationError
 from marshmallow.validate import Length
@@ -43,26 +44,25 @@ openapi_spec_methods_override = {
 }
 
 
-def validate_python_date_format(value: str) -> None:
-    regex = re.compile(
-        r"""
-        ^(
-            epoch_s|epoch_ms|
-            (?P<date>%Y([-/]%m([-/]%d)?)?)([\sT](?P<time>%H(:%M(:%S(\.%f)?)?)?))?
-        )$
-        """,
-        re.VERBOSE,
-    )
-    match = regex.match(value or "")
-    if not match:
-        raise ValidationError([_("Invalid date/timestamp format")])
+def validate_python_date_format(dt_format: str) -> bool:
+    if dt_format in ("epoch_s", "epoch_ms"):
+        return True
+    try:
+        dt_str = datetime.now().strftime(dt_format)
+        isoparse(dt_str)
+    except ValueError as ex:
+        raise ValidationError([_("Invalid date/timestamp format")]) from ex
+    return True
 
 
 class DatasetColumnsPutSchema(Schema):
     id = fields.Integer(required=False)
     column_name = fields.String(required=True, validate=Length(1, 255))
     type = fields.String(allow_none=True)
-    advanced_data_type = fields.String(allow_none=True, validate=Length(1, 255))
+    advanced_data_type = fields.String(
+        allow_none=True,
+        validate=Length(1, 255),
+    )
     verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)})
     description = fields.String(allow_none=True)
     expression = fields.String(allow_none=True)

tests/unit_tests/config_test.py

@@ -211,7 +211,6 @@ def test_python_date_format_by_column_name(
         "dttm_columns": {
             "id": {"python_date_format": "epoch_ms"},
             "dttm": {"python_date_format": "epoch_s"},
-            "duration_ms": {"python_date_format": "invalid"},
         },
     }
     mocker.patch(
@@ -228,7 +227,6 @@ def test_python_date_format_by_column_name(
         return_value=[
             {"column_name": "id", "type": "INTEGER", "is_dttm": False},
             {"column_name": "dttm", "type": "INTEGER", "is_dttm": False},
-            {"column_name": "duration_ms", "type": "INTEGER", "is_dttm": False},
         ],
     )
 
@@ -242,12 +240,6 @@ def test_python_date_format_by_column_name(
     assert dttm_col.is_dttm
     assert dttm_col.python_date_format == "epoch_s"
 
-    duration_ms_col = [c for c in test_table.columns if c.column_name == "duration_ms"][
-        0
-    ]
-    assert duration_ms_col.is_dttm
-    assert duration_ms_col.python_date_format == "invalid"
-
 
 def test_expression_by_column_name(
     mocker: MockerFixture,

tests/unit_tests/datasets/schema_tests.py

@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# pylint: disable=import-outside-toplevel, invalid-name, unused-argument, redefined-outer-name
+import pytest
+from marshmallow import ValidationError
+
+from superset.datasets.schemas import validate_python_date_format
+
+
+# pylint: disable=too-few-public-methods
+@pytest.mark.parametrize(
+    "payload",
+    [
+        "epoch_ms",
+        "epoch_s",
+        "%Y-%m-%dT%H:%M:%S.%f",
+        "%Y%m%d",
+    ],
+)
+def test_validate_python_date_format(payload) -> None:
+    assert validate_python_date_format(payload)
+
+
+@pytest.mark.parametrize(
+    "payload",
+    [
+        "%d%m%Y",
+        "%Y/%m/%dT%H:%M:%S.%f",
+    ],
+)
+def test_validate_python_date_format_raises(payload) -> None:
+    with pytest.raises(ValidationError):
+        validate_python_date_format(payload)