codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3500183195
##########
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)
+
+ @classmethod
+ def _upsert_columns(
+ cls, model: SqlaTable, property_columns: list[dict[str, Any]]
+ ) -> None:
+ columns_by_id = {column.id: column for column in model.columns}
+ property_columns_by_id = {
+ properties["id"]: properties
+ for properties in property_columns
+ if "id" in properties
+ }
+
+ for properties in property_columns:
+ if "id" not in properties:
+ db.session.add(TableColumn(**{**properties, "table_id":
model.id}))
+
+ for properties in property_columns_by_id.values():
+ col = columns_by_id[properties["id"]]
+ for key, value in properties.items():
+ setattr(col, key, value)
Review Comment:
**Suggestion:** This direct dictionary lookup can raise a `KeyError` and
fail the whole update when a column id is stale or disappears between
validation and write (concurrent updates/deletes). Resolve ids defensively (for
example, use `.get()` and raise a controlled validation/command error) instead
of indexing the map unconditionally. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset update API can 500 on concurrent column edits.
- ⚠️ Users may see opaque internal errors saving datasets.
- ⚠️ Background jobs updating datasets risk unexpected crashes.
- ⚠️ Version-history transactions may be left partially applied.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a dataset update via the public REST endpoint `PUT
/api/v1/dataset/<pk>`
implemented by `DatasetRestApi.put` in `superset/datasets/api.py:398-437`,
sending a JSON
body that includes a `columns` array with at least one entry carrying an
`id` field
corresponding to an existing column on the dataset.
2. The request is handled by `UpdateDatasetCommand.run` in
`superset/commands/dataset/update.py:24-37`, which first calls
`self.validate()` and then,
if no validation errors are accumulated, calls
`DatasetDAO.update(self._model,
attributes=self._properties)`.
3. During `validate()`, `_validate_semantics` (in
`superset/commands/dataset/update.py:7-17`) calls `_validate_columns` (lines
42-55), which
builds `columns_ids = [column["id"] for column in columns if "id" in
column]` and checks
those against the database via
`DatasetDAO.validate_columns_exist(self._model_id,
columns_ids)`. This ensures the IDs are valid at validation time but does
not lock them
against concurrent changes.
4. After validation succeeds, but before `_upsert_columns` loads
`model.columns`, a
concurrent client issues another `PUT /api/v1/dataset/<pk>` (or other
operation) that
deletes one of the validated columns (e.g. via a request omitting that
column so it is
removed), and commits; under the default READ COMMITTED isolation this makes
the column
row disappear from subsequent queries in our transaction.
5. Back in the original request, `DatasetDAO.update` in
`superset/daos/dataset.py:250-273`
sees `"columns"` in `attributes` and calls `DatasetDAO.update_columns(item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")))`,
which for `override_columns=False` dispatches to `_upsert_columns` at
`superset/daos/dataset.py:101-120`.
6. Inside `_upsert_columns`, `columns_by_id = {column.id: column for column
in
model.columns}` is built from the now-current database view (which no longer
includes the
concurrently-deleted column), while `property_columns_by_id` still contains
that stale
`id`. When the loop `for properties in property_columns_by_id.values(): col =
columns_by_id[properties["id"]]` executes at
`superset/daos/dataset.py:105-118`, the
lookup `columns_by_id[properties["id"]]` raises a `KeyError`, because the
`id` is no
longer present in `columns_by_id`.
7. The `UpdateDatasetCommand.run` transaction decorator in
`superset/commands/dataset/update.py:24-33` is configured to catch only
`SQLAlchemyError`
and `ValueError` and re-raise `DatasetUpdateFailedError`, so the `KeyError`
from
`_upsert_columns` is not translated and bubbles up as an uncaught exception,
causing an
HTTP 500 instead of a controlled validation error from the dataset update
API.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=16103ccbc3eb4da6b920672474eda68c&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=16103ccbc3eb4da6b920672474eda68c&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:** 365:368
**Comment:**
*Race Condition: This direct dictionary lookup can raise a `KeyError`
and fail the whole update when a column id is stale or disappears between
validation and write (concurrent updates/deletes). Resolve ids defensively (for
example, use `.get()` and raise a controlled validation/command error) instead
of indexing the map unconditionally.
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%2F41075&comment_hash=fa4e686b6884e47b0d89d33092486175d1da2147827b04c30f19fe65f46a8577&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=fa4e686b6884e47b0d89d33092486175d1da2147827b04c30f19fe65f46a8577&reaction=dislike'>👎</a>
##########
superset/datasets/api.py:
##########
@@ -440,17 +503,69 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(SqlaTable, pk)
+
try:
+ # Two commands, two commits, two Continuum transactions for an
+ # ``override_columns`` save — deliberately NOT merged into one
+ # transaction. A single-transaction design was attempted and
+ # reverted: ``DBEventLogger`` writes request logs through the
+ # SHARED scoped session and calls ``commit()`` /
+ # ``rollback()`` on it mid-request (superset/utils/log.py),
+ # so any save held uncommitted across a logged sub-action can
+ # be committed half-done (Postgres/MySQL) or rolled back
+ # entirely on a transient logger failure (SQLite's
+ # "database is locked"). Until the event logger gets its own
+ # session, per-command commit boundaries are the only shape
+ # whose failure modes are honest. Consequence the
+ # version-history UI must tolerate: one logical save can
+ # surface as two version transactions stamped the same second.
changed_model = UpdateDatasetCommand(pk, item,
override_columns).run()
+ # Capture the post-update identifiers BEFORE the refresh:
+ # RefreshDatasetCommand commits its own transaction, so reading
+ # afterwards would attribute the refresh's version to the
+ # user's update (and old→new would span two transactions).
+ new_info = current_entity_version_info(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ etag_version_uuid = new_info.version_uuid
if override_columns:
RefreshDatasetCommand(pk).run()
- response = self.response(200, id=changed_model.id, result=item)
+ # The ETag must reflect the entity's *current live* version,
+ # which after the refresh is the refresh's transaction —
+ # re-read it rather than reusing the pre-refresh uuid.
+ etag_version_uuid = current_entity_etag_uuid(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ response = self.response(
+ 200,
+ id=changed_model.id,
+ result=item,
+ old_version=old_info.version,
+ new_version=new_info.version,
+ old_transaction_id=old_info.transaction_id,
+ new_transaction_id=new_info.transaction_id,
+ old_version_uuid=old_info.version_uuid,
+ new_version_uuid=new_info.version_uuid,
+ )
+ set_version_etag(response, etag_version_uuid)
except DatasetNotFoundError:
response = self.response_404()
except DatasetForbiddenError:
response = self.response_403()
except DatasetInvalidError as ex:
response = self.response_422(message=ex.normalized_messages())
+ except DatasetRefreshFailedError as ex:
+ logger.error(
+ "Error refreshing dataset during update %s: %s",
+ self.__class__.__name__,
+ str(ex),
+ exc_info=True,
+ )
+ response = self.response_422(message=str(ex))
Review Comment:
**Suggestion:** If `RefreshDatasetCommand` fails after
`UpdateDatasetCommand` has already committed, the endpoint returns 422 even
though the dataset update succeeded and is persisted. This creates a
false-failure contract that can trigger duplicate retries and user confusion.
Handle this as a partial-success path (or make both operations atomic) so the
HTTP status reflects the committed write state. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dataset update persists despite HTTP 422 error response.
- ⚠️ Clients may retry writes causing duplicate logical updates.
- ⚠️ User-facing UI misreports success of dataset edits.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger the dataset update endpoint `DatasetRestApi.put` in
`superset/datasets/api.py`
(PR hunk starting at line 493) by issuing an HTTP PUT to
`/api/v1/dataset/{pk}` with a
valid JSON body and query parameter `override_columns=true`, so the code
executes
`UpdateDatasetCommand(pk, item, override_columns).run()` at line 527
followed by
`RefreshDatasetCommand(pk).run()` at line 536.
2. From `superset/commands/dataset/update.py:24-37` observe that
`UpdateDatasetCommand.run()` is wrapped in a `@transaction` decorator and,
on success,
commits the dataset changes via `DatasetDAO.update(...)`, meaning the
dataset update is
durably persisted before any refresh logic runs.
3. Inspect `superset/commands/dataset/refresh.py:12-33` where
`RefreshDatasetCommand.run()` is also wrapped in its own
`@transaction(on_error=...,
reraise=DatasetRefreshFailedError)`; if `fetch_metadata()` or subsequent
datetime format
detection throws (for example due to a broken database connection or invalid
metadata),
the decorator raises `DatasetRefreshFailedError` after its separate
transaction, leaving
the earlier update transaction committed.
4. In `DatasetRestApi.put` at `superset/datasets/api.py` PR lines 561-568,
see that
`DatasetRefreshFailedError` is caught and logged, and the method returns
`self.response_422(message=str(ex))`; reproducing this by deliberately
causing
`RefreshDatasetCommand.run()` to fail (while keeping the update valid)
yields an HTTP 422
to the client even though a follow-up GET to the same dataset shows that the
main update
was persisted, demonstrating the false-failure contract described in the
suggestion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=874103ed86874f1ca56fe636d19e9568&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=874103ed86874f1ca56fe636d19e9568&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/datasets/api.py
**Line:** 535:568
**Comment:**
*Logic Error: If `RefreshDatasetCommand` fails after
`UpdateDatasetCommand` has already committed, the endpoint returns 422 even
though the dataset update succeeded and is persisted. This creates a
false-failure contract that can trigger duplicate retries and user confusion.
Handle this as a partial-success path (or make both operations atomic) so the
HTTP status reflects the committed write state.
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%2F41075&comment_hash=6024d9b7864ad24519ef92fb7fbde98f4c6e7ef74a468a91d9ece0d88a02ec48&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=6024d9b7864ad24519ef92fb7fbde98f4c6e7ef74a468a91d9ece0d88a02ec48&reaction=dislike'>👎</a>
##########
superset/daos/dataset.py:
##########
@@ -373,28 +424,22 @@ def update_metrics(
if "id" in properties
}
- db.session.bulk_insert_mappings(
- SqlMetric,
- [
- {**properties, "table_id": model.id}
- for properties in property_metrics
- if "id" not in properties
- ],
- )
-
- db.session.bulk_update_mappings(
- SqlMetric,
- [
- {**metrics_by_id[properties["id"]].__dict__, **properties}
- for properties in property_metrics_by_id.values()
- ],
- )
-
- db.session.query(SqlMetric).filter(
- SqlMetric.id.in_(
- {metric.id for metric in model.metrics} -
property_metrics_by_id.keys()
- )
- ).delete(synchronize_session="fetch")
+ # Insert new metrics
+ for properties in property_metrics:
+ if "id" not in properties:
+ db.session.add(SqlMetric(**{**properties, "table_id":
model.id}))
+
+ # Update existing metrics
+ for properties in property_metrics_by_id.values():
+ metric = metrics_by_id[properties["id"]]
+ for key, value in properties.items():
+ setattr(metric, key, value)
Review Comment:
**Suggestion:** This has the same failure mode for metrics: indexing
`metrics_by_id` without checking existence can raise `KeyError` at runtime when
ids are stale or concurrently removed. Convert this to a guarded lookup and
surface a controlled domain validation error instead of an uncaught exception.
[race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset metric updates can 500 on concurrent metric deletes.
- ⚠️ Users may lose edits when metrics vanish mid-save.
- ⚠️ Automated tools updating metrics may experience unexpected failures.
- ⚠️ Error handling inconsistent with other dataset validation paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the dataset update endpoint `PUT /api/v1/dataset/<pk>` handled by
`DatasetRestApi.put` in `superset/datasets/api.py:398-437`, providing a JSON
body that
includes a `metrics` array, with at least one metric carrying an `id`
corresponding to an
existing metric on the dataset.
2. The request flows into `UpdateDatasetCommand.run`
(`superset/commands/dataset/update.py:24-37`), which calls `self.validate()`
and then
`DatasetDAO.update(self._model, attributes=self._properties)` on success.
3. During `validate()`, `_validate_semantics`
(`superset/commands/dataset/update.py:7-17`)
calls `_validate_metrics` (`superset/commands/dataset/update.py:66-77`),
which builds
`metrics_ids = [metric["id"] for metric in metrics if "id" in metric]` and
checks them via
`DatasetDAO.validate_metrics_exist(self._model_id, metrics_ids)`. This
ensures the IDs
exist at validation time but does not prevent concurrent removal between
validation and
write.
4. While the first request is in flight after validation, a concurrent
client issues
another `PUT /api/v1/dataset/<pk>` (or other dataset modification) that
deletes one of the
validated metrics and commits. Under the database isolation level,
subsequent ORM loads in
the first transaction can observe the updated state without that metric row.
5. Back in the original request, `DatasetDAO.update` in
`superset/daos/dataset.py:250-273`
sees `"metrics"` in `attributes` and calls `DatasetDAO.update_metrics(item,
attributes.pop("metrics"))`, which executes the logic in `update_metrics` at
`superset/daos/dataset.py:32-74`.
6. Inside `update_metrics`, `metrics_by_id = {metric.id: metric for metric in
model.metrics}` is built from the current relationship state, which now
omits the
concurrently-deleted metric, while `property_metrics_by_id` still contains
that stale
`id`. When the loop `for properties in property_metrics_by_id.values():
metric =
metrics_by_id[properties["id"]]` runs at `superset/daos/dataset.py:51-68`,
the lookup
`metrics_by_id[properties["id"]]` raises a `KeyError` because the metric
`id` is missing
from `metrics_by_id`.
7. The `UpdateDatasetCommand.run` transaction decorator in
`superset/commands/dataset/update.py:24-33` only catches `SQLAlchemyError`
and
`ValueError`, so the `KeyError` from `update_metrics` is not converted into a
domain-specific error and instead propagates as an uncaught exception,
resulting in an
HTTP 500 on the dataset metrics update.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=75547ac433c84bf3902bf2535608617a&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=75547ac433c84bf3902bf2535608617a&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:** 433:436
**Comment:**
*Race Condition: This has the same failure mode for metrics: indexing
`metrics_by_id` without checking existence can raise `KeyError` at runtime when
ids are stale or concurrently removed. Convert this to a guarded lookup and
surface a controlled domain validation error instead of an uncaught exception.
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%2F41075&comment_hash=c656437f1f88716735cac8c44cf760693d4ca780300f2c4d4493eead02c4cba0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=c656437f1f88716735cac8c44cf760693d4ca780300f2c4d4493eead02c4cba0&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]
