fix(sqllab): access issue:#28596

[leoguo1024]

fix sqllab access issue. Fixes #28596

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.78%. Comparing base (76d897e) to head (b35e4e4). Report is 1763 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29311       +/-   ##
===========================================
+ Coverage   60.48%   77.78%   +17.29%     
===========================================
  Files        1931      518     -1413     
  Lines       76236    37548    -38688     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    29206    -16908     
+ Misses      28017     8342    -19675     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.94% <ø> (-0.22%)	:arrow_down:
javascript	?
mysql	77.22% <ø> (?)
postgres	77.33% <ø> (?)
presto	53.55% <ø> (-0.25%)	:arrow_down:
python	77.78% <ø> (+14.27%)	:arrow_up:
sqlite	76.79% <ø> (?)
unit	?

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[villebro]

@leoguo1024 please provide a more verbose description, explaining the rationale behind the changes. In particular, the fact that the test extract_tables_from_jinja_sql is passing both before and after this change make feel we need to revisit the tests introduced in #27470. Tagging @john-bodley as he introduced the new function and tests.

[Hen0k]

Looking forward to this getting fixed. All my users are unable to access data through the SQL lab. The latest version that doesn't have this issue is 3.1.1

[Hen0k]

Hey @rusackas @john-bodley got any suggestions for merging this?

[Hen0k]

Hey @leoguo1024

I am still stuck with this issue. got any recommendations?

[villebro]

Echoing @john-bodley's comment here - we really do need to do template processing here, in case there's jinja logic in the query. @leoguo1024 @Hen0k can you check if this issue is still relevant on master branch and/or the latest 4.1 release candidate? If so I can help figure out what's wrong here..

[Hen0k]

Hey @villebro @rukshn This is now solved in rc2 for 4.1

After using the latest image, I am able to run queries like before.

Just to recap the issue was with users that only had access to some superset dataset not being able to run any query on the sql_lab. But this works when I provide the user permission to the entire database. But that is not ideal for my situation where access needed to be limited.

Now, the users with the sql_lab role are able to make queries with the datasets they have access to (based on other roles) like before.

[vicky-sai]

Can we merge this as this is very important for all of the users here ?

[giftig]

@vicky-sai in its current state it has important outstanding comments as above -- it's broken the build and an important test is failing, and it needs a better explanation of what it's attempting to do (which might also clarify how it can be properly fixed).

However it looks like @Hen0k has verified that this issue has already been resolved in 4.1rc2. I'd suggest seeing if that build works for you too. If it's fixed in the release candidate we can just close this PR.

[rusackas]

This still has outstanding comments, and needs a rebase. I'd love to see this merged, but it needs to be brought up to speed so it's mergeable. Any help would be appreciated on this front, as I'd love to close the related issue.

[giftig]

Given this PR is a one-liner which seems to cause more problems than it fixes and is now very stale, I think we can just close this one; it would be easier to raise a fresh PR than build on this one imo.