Skip to content

feat(charts): add extensible extra metadata column to slices #41200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bsovran wants to merge 2 commits into
base: master
Choose a base branch
from
+51 −0
Open

feat(charts): add extensible extra metadata column to slices #41200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
897c634
feat(charts): add extensible extra metadata column to slices
bsovran Jun 18, 2026
2133aa4
test(charts): fix chart export tests and round-trip for extra column
bsovran Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset/charts/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1735,6 +1735,7 @@ class ImportV1ChartSchema(Schema):
viz_type = fields.String(required=True)
params = fields.Dict()
query_context = fields.String(allow_none=True, validate=utils.validate_json)
extra = fields.String(allow_none=True, validate=utils.validate_json)

@sadpandajoe sadpandajoe Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds extra to the import schema, but the REST API schemas don't declare it: ChartPostSchema (line 225), ChartPutSchema (line 287), and ChartEntityResponseSchema (line 204). As written, extra gets silently dropped on POST/PUT /api/v1/chart/ and is absent from the GET response, so the column is only reachable through import/export.

Is REST read/write in scope for this PR? If so, add extra to those three schemas the same way query_context is wired (fields.String + utils.validate_json). If this is intentionally import-only groundwork for the SIP, can you note that in the PR description so the REST follow-up doesn't get lost?

cache_timeout = fields.Integer(allow_none=True)
uuid = fields.UUID(required=True)
version = fields.String(required=True)
Expand Down
45 changes: 45 additions & 0 deletions superset/migrations/versions/2026-06-17_16-58_9d4b2e8c1f0a_add_extra_column_to_slices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# 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.
"""add extra column to slices

Revision ID: 9d4b2e8c1f0a
Revises: 78a40c08b4be
Create Date: 2026-06-17 16:58:00.000000

"""

import sqlalchemy as sa

from superset.migrations.shared.utils import add_columns, drop_columns
from superset.utils.core import MediumText

# revision identifiers, used by Alembic.
revision = "9d4b2e8c1f0a"
down_revision = "78a40c08b4be"


def upgrade() -> None:
"""Add the nullable ``extra`` column to ``slices``."""
add_columns(
"slices",
sa.Column("extra", MediumText(), nullable=True),
)


def downgrade() -> None:
"""Drop the ``extra`` column from ``slices``."""
drop_columns("slices", "extra")
3 changes: 3 additions & 0 deletions superset/models/slice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class Slice( # pylint: disable=too-many-public-methods
viz_type = Column(String(250))
params = Column(utils.MediumText())
query_context = Column(utils.MediumText())
extra = Column(utils.MediumText())

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The new chart metadata field is persisted on the model but not copied when charts are cloned, so dashboard duplication will silently drop extra metadata on the duplicated charts. Update the chart cloning path to include this new field so cloned charts preserve the same metadata as the source chart. [incomplete implementation]

Severity Level: Major ⚠️ 
- ⚠️ Dashboard copy with duplicate charts drops chart extra metadata.
- ⚠️ Cloned charts diverge from originals in stored configuration.
Steps of Reproduction ✅ 
1. In a running Superset instance with this PR applied, pick an existing dashboard with
charts and ensure at least one chart has `Slice.extra` set (e.g. via a shell or test
fixture): `original_slc.extra = '{"foo": "bar"}'` for a `Slice` instance attached to a
dashboard. The `Slice` model (including the new `extra` column) is defined in
`superset/models/slice.py:17-27` as seen in the class definition.

2. Trigger a dashboard copy operation with chart duplication enabled. In production this
is done via the dashboard copy API which uses `CopyDashboardCommand`
(`superset/commands/dashboard/copy.py:6-15`) and `DashboardDAO.copy_dashboard`. In tests
the same path is exercised directly in
`superset/tests/integration_tests/dashboards/dao_tests.py:27-42`, where
`DashboardDAO.copy_dashboard(original_dash, dash_data)` is called with
`dash_data["duplicate_slices"] = True`.

3. During the copy operation, `DashboardDAO.copy_dashboard` in
`superset/daos/dashboard.py:368-46` iterates over `original_dash.slices` and, when
`duplicate_slices` is true, calls `new_slice = slc.clone()` at line 386 for each chart,
then adds each `new_slice` to the session and associates it with the new dashboard.

4. The `Slice.clone` method in `superset/models/slice.py:151-42` constructs a new `Slice`
with only `slice_name`, `datasource_id`, `datasource_type`, `datasource_name`, `viz_type`,
`params`, `description`, and `cache_timeout`, and does not copy the `extra` field. As a
result, each cloned chart's `extra` column remains NULL/empty even though the source
chart's `extra` contained metadata, so the duplicated dashboard's charts silently lose
their `extra` metadata.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset/models/slice.py
**Line:** 85:85
**Comment:**
	*Incomplete Implementation: The new chart metadata field is persisted on the model but not copied when charts are cloned, so dashboard duplication will silently drop `extra` metadata on the duplicated charts. Update the chart cloning path to include this new field so cloned charts preserve the same metadata as the source chart.

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
👍 | 👎

@sadpandajoe sadpandajoe Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why MediumText() here instead of Text? SqlaTable.extra, the precedent this is following, is Column(Text). I can see the argument for matching params/query_context on this model instead, just want to confirm the divergence from the sibling extra field is deliberate.

description = Column(Text)
cache_timeout = Column(Integer)
perm = Column(String(1000))
Expand Down Expand Up @@ -134,6 +135,7 @@ class Slice( # pylint: disable=too-many-public-methods
"viz_type",
"params",
"query_context",
"extra",
"cache_timeout",
]
export_parent = "table"
Expand All @@ -156,6 +158,7 @@ def clone(self) -> Slice:
params=self.params,
description=self.description,
cache_timeout=self.cache_timeout,
extra=self.extra,
)

@renders("datasource_name")
Expand Down
2 changes: 2 additions & 0 deletions tests/integration_tests/charts/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def test_export_chart_command(self, mock_g):
"uuid": str(example_chart.uuid),
"version": "1.0.0",
"query_context": None,
"extra": None,

@sadpandajoe sadpandajoe Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both additions here (and the key-order one at line 156) only assert that the extra key appears in the export. Nothing exercises a non-null value surviving a round-trip, or clone() carrying extra across. Could you add a positive round-trip test (set extra='{"foo":"bar"}', export, re-import, assert it's preserved) and a clone() assertion (assert slice.clone().extra == slice.extra)? The second one would lock in the behavior the static-analysis bot flagged as missing.

}

@patch("superset.utils.core.g")
Expand Down Expand Up @@ -152,6 +153,7 @@ def test_export_chart_command_key_order(self, mock_g):
"viz_type",
"params",
"query_context",
"extra",
"cache_timeout",
"uuid",
"version",
Expand Down
Loading