Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add Postgres SQL validator #11538

Merged
merged 6 commits into from
Dec 5, 2020

Conversation

betodealmeida
Copy link
Member

SUMMARY

This PR adds a SQL validator for Postgres queries in SQL Lab. It uses the pgsanity module, which parses the query, providing syntax validation without having to talk to the database.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

2020-11-03-125407_410x126_scrot

TEST PLAN

Create a superset_config.py:

SQLALCHEMY_DATABASE_URI = "postgresql://superset:superset@localhost/test" 

FEATURE_FLAGS: Dict[str, Any] = {
    "SQL_VALIDATORS_BY_ENGINE": {"postgresql": "PostgreSQLValidator"},
}

And tested a few queries.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Nov 3, 2020

Codecov Report

Merging #11538 (8dbbfb2) into master (6f2e36d) will decrease coverage by 5.65%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11538      +/-   ##
==========================================
- Coverage   66.62%   60.96%   -5.66%     
==========================================
  Files         873      888      +15     
  Lines       41870    43324    +1454     
  Branches     3847     3817      -30     
==========================================
- Hits        27894    26412    -1482     
- Misses      13875    16912    +3037     
+ Partials      101        0     -101     
Flag Coverage Δ
cypress 55.36% <ø> (-0.46%) ⬇️
javascript ?
python 63.86% <54.54%> (+1.70%) ⬆️

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

Impacted Files Coverage Δ
superset/sql_validators/__init__.py 80.00% <50.00%> (ø)
superset/sql_validators/postgres.py 50.00% <50.00%> (ø)
superset/config.py 90.07% <100.00%> (-0.08%) ⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...components/AdhocFilterEditPopoverSqlTabContent.jsx 7.14% <0.00%> (-92.86%) ⬇️
...ntend/src/views/CRUD/annotation/AnnotationList.tsx 1.36% <0.00%> (-90.94%) ⬇️
... and 489 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f2e36d...8dbbfb2. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it! LGTM, but bonus points for a test if it's not too much trouble.

name = "PostgreSQLValidator"

@classmethod
def validate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add unit tests

@betodealmeida betodealmeida merged commit 66cd565 into apache:master Dec 5, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants