fix: Ch31968query context #17600
Merged
fix: Ch31968query context #17600
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AAfghahi
force-pushed
the
ch31968queryContext
branch
2 times, most recently
from
7c72eb9
to
409c74b
Compare
superset/charts/commands/export.py
Outdated
| @@ -63,6 +63,9 @@ def _export(model: Slice) -> Iterator[Tuple[str, str]]: | |||
| except json.decoder.JSONDecodeError: | |||
| logger.info("Unable to decode `params` field: %s", payload["params"]) | |||
|
|
|||
| if payload.get("query_context"): | |||
There was a problem hiding this comment.
payload = {
key: value
for key, value in payload.items()
if key not in REMOVE_KEYS
}| @@ -96,10 +96,5 @@ def _import( | |||
| } | |||
| ) | |||
| config["params"].update({"datasource": dataset.uid}) | |||
| if config["query_context"]: | |||
There was a problem hiding this comment.
We need to delete query_context if it exists:
# fix old exports before PR
if config["query_context"]:
config["query_context"] = NoneThere was a problem hiding this comment.
Would it be better if this was
if config["query_context"]:
del config["query_context"]
AAfghahi
force-pushed
the
ch31968queryContext
branch
2 times, most recently
from
42c4714
to
60d09c6
Compare
AAfghahi
force-pushed
the
ch31968queryContext
branch
5 times, most recently
from
d250bbf
to
d712364
Compare
eschutho
approved these changes
Dec 1, 2021
There was a problem hiding this comment.
LGTM but could also use a review from @betodealmeida who has more context on this part of the codebase.
AAfghahi
force-pushed
the
ch31968queryContext
branch
from
d712364
to
6fa51cd
Compare
eschutho
reviewed
Dec 1, 2021
| try: | ||
| query_context = slc.get_query_context() | ||
| except DatasetNotFoundError: | ||
| query_context = None |
There was a problem hiding this comment.
@AAfghahi and I also talked about maybe writing a script/migration as a follow up that will remove any existing query_context fields from charts if they contain an invalid dataset id. Either way, it seems that if query context is failing in any way here, that it would be just as fine not to use it.
betodealmeida
approved these changes
Dec 1, 2021
eschutho
reviewed
Dec 1, 2021
| @@ -191,34 +190,6 @@ def test_import_v1_chart(self, mock_g): | |||
| ) | |||
| assert dataset.table_name == "imported_dataset" | |||
| assert chart.table == dataset | |||
| assert json.loads(chart.query_context) == { | |||
There was a problem hiding this comment.
also @AAfghahi is going to fast-follow with a test for these changes.
sadpandajoe
pushed a commit
to preset-io/superset
that referenced
this pull request
Dec 1, 2021
* a lot of console logs * import and export of query context (cherry picked from commit d7e3a60)
|
🏷️ 2021.46 |
9 tasks
mistercrunch
added
the
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
The were some recent changes that are causing an error when reading a query context that has a dataset id tied to a dataset that does not exist. In the case where we found this error, the invalid dataset id was caused by exporting query_context which was then exporting the dataset id instead of a uuid or something that could be transferred from one superset instance to another. So this PR removes the query_context from export and import and also does not throw an error if a chart tries to read the query_context and hits a dataset not found error.
In the near future, we should think about cleaning up any existing query contexts in a migration or script that point to a dataset that does not exist.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
n/a
TESTING INSTRUCTIONS
export a dashboard with charts that have query_context and import them. When loading the dashboard in the new instance, there should be no errors.
ADDITIONAL INFORMATION