mikebridge commented on code in PR #41076:
URL: https://github.com/apache/superset/pull/41076#discussion_r3574957700
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -201,7 +205,31 @@ def _import(
overwrite
or (dashboard.id, chart_id) not in
existing_relationships
):
- dashboard_chart_ids.append((dashboard.id, chart_id))
+ target_chart_ids.append(chart_id)
+
+ if overwrite:
+ # Replace the dashboard's chart membership entirely.
+ dashboard.slices = (
+ db.session.query(Slice)
+ .filter(Slice.id.in_(target_chart_ids))
+ .all()
+ if target_chart_ids
+ else []
+ )
+ # Flush eagerly so the M2M rows land in
+ # ``dashboard_slices`` before any subsequent
+ # autoflush fires an inner-flush event handler
+ # that would reset the relationship change.
+ db.session.flush()
+ elif target_chart_ids:
+ # Append only the new associations to existing ones.
+ new_slices = (
+ db.session.query(Slice)
+ .filter(Slice.id.in_(target_chart_ids))
+ .all()
+ )
Review Comment:
Addressed by subsequent commits; the flagged code is no longer current after
the rebase onto master. Resolving as outdated.
##########
superset/commands/importers/v1/utils.py:
##########
@@ -387,6 +388,37 @@ def safe_insert_dashboard_chart_relationships(
)
+def _prime_versioning_unit_of_work() -> None:
+ """Ensure Continuum has a unit-of-work for the current connection.
+
+ ``dashboard_slices`` is a Continuum-tracked (versioned) association
+ table, so a raw Core INSERT/DELETE on it fires Continuum's engine-level
+ ``before_execute`` listener, which looks up a unit-of-work for the
+ connection and raises ``KeyError`` when none is registered (the same
+ failure class the dashboard test factory hit). The normal import flow
+ registers one via prior ORM flushes, so this is belt-and-suspenders for
+ a bulk relationship insert that might run before any flush on the
+ connection. No-op (the listener is detached) when version capture is
+ disabled, which is the shipped default; never allowed to break an import.
+ """
+ try:
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import versioning_manager
+
+ # Mirror the exact condition Continuum's track_association_operations
+ # listener uses to decide whether it acts (versioning OR
+ # native_versioning), so the prime can't skip while the listener runs.
+ options = versioning_manager.options
Review Comment:
Addressed by subsequent commits; the flagged code is no longer current after
the rebase onto master. Resolving as outdated.
##########
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}
Review Comment:
Addressed by subsequent commits; the flagged code is no longer current after
the rebase onto master. Resolving as outdated.
##########
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")
Review Comment:
Addressed by subsequent commits; the flagged code is no longer current after
the rebase onto master. Resolving as outdated.
##########
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}
Review Comment:
Addressed by subsequent commits; the flagged code is no longer current after
the rebase onto master. Resolving as outdated.
##########
superset/daos/dataset.py:
##########
@@ -363,6 +411,9 @@ def update_metrics(
- If a metric Dict does not have an `id` then we create a new metric.
- If there are extra metrics on the metadata db that are not defined
on the List
then we delete.
+
+ Uses individual ORM operations (not bulk) so that SQLAlchemy-Continuum
+ can capture each row change in the version history.
"""
metrics_by_id = {metric.id: metric for metric in model.metrics}
Review Comment:
Addressed by subsequent commits; the flagged code is no longer current after
the rebase onto master. Resolving as outdated.
--
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]
