codeant-ai-for-open-source[bot] commented on code in PR #36679:
URL: https://github.com/apache/superset/pull/36679#discussion_r2624135008
##########
superset/datasets/schemas.py:
##########
@@ -257,6 +257,7 @@ def fix_extra(self, data: dict[str, Any], **kwargs: Any) ->
dict[str, Any]:
expression = fields.String(allow_none=True)
description = fields.String(allow_none=True)
python_date_format = fields.String(allow_none=True)
+ datetime_format = fields.String(allow_none=True)
Review Comment:
**Suggestion:** The import schema for columns now accepts any string for
`datetime_format` without validating its length or format, which can lead to
values longer than the 100-character database column or formats that later fail
validation in the update API, causing runtime errors or inconsistent behavior
between import and edit flows. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
datetime_format = fields.String(
allow_none=True, validate=[Length(1, 100),
validate_python_date_format]
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The import schema currently allows any string for datetime_format while
elsewhere (DatasetColumnsPutSchema) the field is validated with Length(1, 100)
and validate_python_date_format. That mismatch can let overly long or invalid
formats enter the system during import and later fail when the same dataset is
edited or persisted, causing runtime errors or data inconsistency. Adding the
same validators restores consistency and prevents invalid data from being
imported. This fixes a real data-validation issue, not just style.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/datasets/schemas.py
**Line:** 260:260
**Comment:**
*Logic Error: The import schema for columns now accepts any string for
`datetime_format` without validating its length or format, which can lead to
values longer than the 100-character database column or formats that later fail
validation in the update API, causing runtime errors or inconsistent behavior
between import and edit flows.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -702,19 +671,43 @@ def test_import_dataset_managed_externally(
assert sqla_table.external_url == "https://example.org/my_table";
+def test_import_dataset_column_datetime_format(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Test importing a dataset with a column including a datetime format.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ for column in config["columns"]:
+ column["datetime_format"] = "%Y-%m-%d"
+
+ schema = ImportV1DatasetSchema()
+ dataset_config = schema.load(config)
+
+ dataset_config["database_id"] = database.id
+
+ sqla_table = import_dataset(dataset_config)
+ for column in sqla_table.columns:
+ assert column.datetime_format == "%Y-%m-%d"
Review Comment:
**Suggestion:** The new test checks `column.datetime_format`, but the
SQLAlchemy model exposes the date format as `python_date_format`, so this
assertion will raise an AttributeError or validate the wrong field instead of
confirming that the imported `datetime_format` config is correctly mapped.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
assert column.python_date_format == "%Y-%m-%d"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR adds a test that checks `column.datetime_format`, but elsewhere in
this test module (and in the project's models) the column-level date format is
surfaced as `python_date_format` (see other assertions in this file that use
`column.python_date_format`). As written, the assertion will raise
AttributeError or incorrectly pass/fail because `datetime_format` is not the
model attribute. Changing the assertion to `column.python_date_format`
correctly verifies that the imported `datetime_format` configuration was mapped
to the model attribute. This fixes a real test bug rather than a cosmetic
change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/datasets/commands/importers/v1/import_test.py
**Line:** 701:701
**Comment:**
*Logic Error: The new test checks `column.datetime_format`, but the
SQLAlchemy model exposes the date format as `python_date_format`, so this
assertion will raise an AttributeError or validate the wrong field instead of
confirming that the imported `datetime_format` config is correctly mapped.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
