fix(sqllab): correct URL format for SQL Lab permalinks

[LevisNgigi]

fix(sqllab): correct URL format for SQL Lab permalinks

SUMMARY

This PR fixes an issue from this PR by ensuring that permalinks use the correct /sqllab/p/{key} format instead of ?key={key} while still using url_for. Previously , url_for("SqllabView.root", key=key, _external=True) generated URLs in the query parameter format (?key={key}), which caused issues when sharing permalinks across users.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

1. Log in and save a query in SQL Lab.
2. Copy the query URL using the "Copy Query URL" option.
3. Share the URL with a different user (one with the sqllab role and access to the dataset).
4. Have the other user try to load the shared query URL.
5. Observe that the second user with sqllab_role can successfully load the shared query.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Unsafe URL Pattern Replacement ▹ view	✅
	Incorrect API Documentation ▹ view	✅

Files scanned

​

File Path	Reviewed
superset-frontend/src/dashboard/components/nativeFilters/state.ts	✅
superset/sqllab/permalink/api.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ [email protected]

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.46%. Comparing base (76d897e) to head (a8e947f).
Report is 1419 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/views/sqllab.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #32154       +/-   ##
===========================================
+ Coverage   60.48%   83.46%   +22.98%     
===========================================
  Files        1931      545     -1386     
  Lines       76236    39047    -37189     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32592    -13522     
+ Misses      28017     6455    -21562     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.50% <71.42%> (-0.67%)	⬇️
javascript	?
mysql	75.87% <85.71%> (?)
postgres	75.94% <85.71%> (?)
presto	53.03% <71.42%> (-0.77%)	⬇️
python	83.46% <85.71%> (+19.98%)	⬆️
sqlite	75.45% <85.71%> (?)
unit	61.01% <71.42%> (+3.38%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[betodealmeida]

Thanks for fixing this! I suggested a more robust solution.

[betodealmeida]

Oops, my bad. Looking at the view:

    @expose("/", methods=["GET", "POST"])
    @expose("/p/<string:permalink>/", methods=["GET"])
    @has_access
    @permission_name("read")
    @event_logger.log_this
    def root(self, **kwargs: Any) -> FlaskResponse:
        payload = {}
        if form_data := request.form.get("form_data"):
            with contextlib.suppress(json.JSONDecodeError):
                payload["requested_query"] = json.loads(form_data)
        return self.render_app_template(payload)

Looks like this is what we want:

Suggested change

	url = url_for("SqllabView.root", key=key, _external=True)
	if "/sqllab/?key=" in url:
	url = url.replace("/sqllab/?key=", "/sqllab/p/")
	url = url_for("SqllabView.root", permalink=key, _external=True)

In general, we should always build internal URLs using url_for.

[LevisNgigi]

So after testing this locally it still will not work.It displays the same query despite copying different queries.

[betodealmeida]

Can you try this:

    @expose("/", methods=["GET", "POST"], endpoint="root")  # <= here
    @expose("/p/<string:permalink>/", methods=["GET"], endpoint="root_p")  # <= here
    @has_access
    @permission_name("read")
    @event_logger.log_this
    def root(self, **kwargs: Any) -> FlaskResponse:
        payload = {}
        if form_data := request.form.get("form_data"):
            with contextlib.suppress(json.JSONDecodeError):
                payload["requested_query"] = json.loads(form_data)
        return self.render_app_template(payload)

And then do:

url = url_for("SqllabView.root_p", permalink=key, _external=True)

[LevisNgigi]

@expose("/", methods=["GET", "POST"], endpoint="root")
TypeError: expose() got an unexpected keyword argument 'endpoint' after adding the endpoint.

[LevisNgigi]

Updated the pr and separated permalink_view from root and works well.

[justinpark]

LGTM! Thanks for hotfix

Green. Ready to go.