codeant-ai-for-open-source[bot] commented on code in PR #41176:
URL: https://github.com/apache/superset/pull/41176#discussion_r3500206634
##########
superset/daos/dataset.py:
##########
@@ -275,6 +275,103 @@ def update(
return super().update(item, attributes)
+ @classmethod
+ def _validate_column_date_formats(
+ cls, property_columns: list[dict[str, Any]]
+ ) -> None:
+ for column in property_columns:
+ if column.get("python_date_format") is None:
+ continue
+ if not
DatasetDAO.validate_python_date_format(column["python_date_format"]):
+ raise ValueError(
+ "python_date_format is an invalid date/timestamp format."
+ )
+
+ @classmethod
+ def _override_columns(
+ cls, model: SqlaTable, property_columns: list[dict[str, Any]]
+ ) -> None:
+ """Replace columns by natural key (``column_name``) — update in place
+ rather than delete-and-reinsert.
+
+ SPIKE (full-Continuum): the previous
+ delete-and-reinsert pattern produced overlapping shadow rows in
+ ``table_columns_version`` (the same ``column_name`` had a DELETE
+ shadow at tx N alongside an INSERT shadow at tx N for a fresh PK).
+ Continuum's ``Reverter`` couldn't unwind this on restore: its flush
+ ordering inserts the historical row before deleting the live one,
+ hitting the ``UNIQUE (table_id, column_name)`` constraint mid-flush
+ (ADR-004 Failure 1).
+
+ The natural-key upsert keeps PKs stable across metadata refresh.
+ Continuum captures only real field changes; new columns get plain
+ INSERT shadows; removed columns get plain DELETE shadows. No
+ natural-key collisions, so Reverter can restore cleanly.
+
+ Behaviour change vs. the previous implementation: PKs of unchanged
+ columns are preserved. Charts that reference columns by their
+ ``id`` continue to work across a metadata refresh — previously
+ such references would be invalidated.
+ """
+ existing_by_name = {c.column_name: c for c in model.columns}
+ incoming_by_name = {p["column_name"]: p for p in property_columns}
+
+ # Identity is the natural key here, never the payload's ``id``:
+ # setattr-ing an incoming ``id`` onto a name-matched row would
+ # rewrite a live primary key, and a renamed column whose payload
+ # still carries its old ``id`` would INSERT with a live PK while
+ # the old-named row is deleted in the same flush — INSERTs flush
+ # before DELETEs, so that collides on the PK / UNIQUE(table_id,
+ # column_name) constraints. ``table_id`` is pinned to *model*.
+ protected_keys = ("id", "table_id")
+
+ # Update columns present in both: in-place setattr.
+ for name, col in existing_by_name.items():
+ if name in incoming_by_name:
+ for key, value in incoming_by_name[name].items():
+ if key not in protected_keys:
+ setattr(col, key, value)
+
+ # Insert columns present only in incoming.
+ for name, properties in incoming_by_name.items():
+ if name not in existing_by_name:
+ cleaned = {
+ key: value
+ for key, value in properties.items()
+ if key not in protected_keys
+ }
+ db.session.add(TableColumn(**{**cleaned, "table_id":
model.id}))
+
+ # Delete columns present only in existing.
+ for name, col in existing_by_name.items():
+ if name not in incoming_by_name:
+ db.session.delete(col)
Review Comment:
**Suggestion:** In override mode, new columns are added before removed
columns are deleted. When a rename is represented as delete+insert and the
metadata DB uses case-insensitive uniqueness for `column_name`, the flush order
(INSERT before DELETE) can hit `UNIQUE (table_id, column_name)` and fail the
save. Ensure conflicting old rows are deleted/flushed before inserts, or handle
case-insensitive natural-key matching during override. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Override column updates fail for case-only column renames.
⚠️ Users see generic 422 error, not clear validation.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset to use a metadata database where the
table_columns.column_name field
is compared case-insensitively under the UniqueConstraint defined in
superset/connectors/sqla/models.py lines 943-947, and create a dataset with
a column named
for example Foo.
2. Issue an HTTP PUT to the dataset update API endpoint in
superset/datasets/api.py
(method starting near line 480) targeting that dataset, with query parameter
override_columns set to true and a columns payload that represents renaming
Foo to foo by
including only a column entry whose column_name is foo and omitting the old
Foo entry, so
the rename is encoded as delete plus insert.
3. The request is handled by UpdateDatasetCommand.run in
superset/commands/dataset/update.py lines 83-97, which calls
_validate_semantics and
_validate_columns; because override_columns is true, the uniqueness check on
new column
names at lines 65-72 of _validate_columns is skipped, so the rename proceeds
without a
preemptive duplicate-name error.
4. DatasetDAO.update in superset/daos/dataset.py lines 71-37 calls
update_columns with
override_columns true; update_columns then calls _override_columns, which
builds
existing_by_name and incoming_by_name at lines 77-79, treats Foo and foo as
different
keys, adds a new TableColumn for foo at lines 96-104, and only afterwards
schedules
deletion of the old Foo at lines 106-109; when SQLAlchemy flushes, it
inserts the new foo
row before deleting Foo, and in a case-insensitive metadata database this
violates the
UniqueConstraint on table_id and column_name, raising an IntegrityError and
causing the
dataset update to fail with a generic error instead of performing the
intended rename.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d02f1ec4ce5842d2bfb830c9d2a47254&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d02f1ec4ce5842d2bfb830c9d2a47254&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 336:348
**Comment:**
*Logic Error: In override mode, new columns are added before removed
columns are deleted. When a rename is represented as delete+insert and the
metadata DB uses case-insensitive uniqueness for `column_name`, the flush order
(INSERT before DELETE) can hit `UNIQUE (table_id, column_name)` and fail the
save. Ensure conflicting old rows are deleted/flushed before inserts, or handle
case-insensitive natural-key matching during override.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=78d845b90cfcd61a40aa5a67befdf7527b18d9b4a4ea584347199114881a9ce0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=78d845b90cfcd61a40aa5a67befdf7527b18d9b4a4ea584347199114881a9ce0&reaction=dislike'>👎</a>
--
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]
