fix(permalink): parsing urlParams as well #24160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24160 +/- ##
=======================================
Coverage 68.26% 68.27%
=======================================
Files 1952 1952
Lines 75360 75367 +7
Branches 8208 8208
=======================================
+ Hits 51448 51455 +7
Misses 21806 21806
Partials 2106 2106
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -1959,7 +1959,7 @@ def dashboard_permalink( # pylint: disable=no-self-use | |||
| dashboard_id, state = value["dashboardId"], value.get("state", {}) | |||
| url = f"/superset/dashboard/{dashboard_id}?permalink_key={key}" | |||
| if url_params := state.get("urlParams"): | |||
| params = parse.urlencode(url_params) | |||
| params = parse.urlencode(dict(url_params)) | |||
There was a problem hiding this comment.
I'm surprised this wasn't caught by mypy or related tests. Can you double check that the the types are accurate, and see if the tests are correct?
There was a problem hiding this comment.
@villebro Yes, but at the superset/views/core.py:836 you're parsing url_params the same way using dict
if key is not None:
command = GetExplorePermalinkCommand(key)
try:
permalink_value = command.run()
if permalink_value:
state = permalink_value["state"]
initial_form_data = state["formData"]
url_params = state.get("urlParams")
if url_params:
initial_form_data["url_params"] = dict(url_params)
else:
return json_error_response(
_("Error: permalink state not found"), status=404
)
except (ChartNotFoundError, ExplorePermalinkGetFailedError) as ex:
flash(__("Error: %(msg)s", msg=ex.message), "danger")
return redirect("/chart/list/")There was a problem hiding this comment.
@villebro, I don't see any test cases that test urlParams in the permalink state.
The type of urlParams is Optional[List[Tuple[str, str]]], which means that parsing urlParams using dict is the correct (and works on me)
There was a problem hiding this comment.
@villebro So the urlencode function taking dictionary, it means that we need to make dictionary from urlParams that have type List[Tuple[str, str]] (as you doing it in superset/views/core.py:836)
There was a problem hiding this comment.
The reason we want to keep it as a list/tuple of tuples is because url params can contain duplicate keys (this would be lost is it were converted to a dict). I believe this may be a regression from #23888 , which has lost some context on tuples, which has caused them to become lists, which isn't compatible with urlencode.
I think the correct solution is to decode the persisted JSON object with the relevant marshmallow schema, which could be implemented with a new MarshmallowCodec. I can open a PR with the required changes. Alternatively, feel free to take a stab at it if you want!
There was a problem hiding this comment.
Please open PR by yourself!
There was a problem hiding this comment.
I'll do that, thanks for bringing this to our attention!
There was a problem hiding this comment.
@Always-prog I was able to reproduce the issue, thanks again!
There was a problem hiding this comment.
@villebro you are welcome!
SUMMARY
When reading urlParams from a permalink, an error occurs because the
urlencodefunction expects a dictionary, but urlParams is a 2D array. To resolve this, we simply need to usedict(urlParams)and it will work correctly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION