sadpandajoe commented on code in PR #41200:
URL: https://github.com/apache/superset/pull/41200#discussion_r3463023184
##########
superset/charts/schemas.py:
##########
@@ -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)
Review Comment:
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?
##########
superset/models/slice.py:
##########
@@ -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())
Review Comment:
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.
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -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,
Review Comment:
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.
--
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]
