Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(api): database schemas migration to new API #10436

Merged
merged 9 commits into from
Jul 29, 2020

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jul 27, 2020

SUMMARY

This PR deprecates the old API endpoint /superset/schemas/<db_id> and migrates it to a new endpoint on /api/v1/database/<id>/schemas/

Also fixes:

  • An N+1 query on the database get list endpoint
  • Applies database filtering, so a user that has no access to the database get HTTP 404

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

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 27, 2020
@@ -252,6 +252,24 @@ def test_schemas(self):
response = json.loads(rv.data.decode("utf-8"))
self.assertEqual(0, len(response["result"]))

def test_database_schemas(self):
Copy link
Member

@bkyryliuk bkyryliuk Jul 27, 2020

Choose a reason for hiding this comment

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

suggestion: there are some complexity due to the api pagination, it may be useful to test it as well
e.g. @nytai helped me to figure out that dropbox/incubator-superset-internal@a7aec30 setting max_page_size = -1 and page_size = -1 breaks some assumptions about pagination apis & it's not covered by unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This endpoint has no pagination, since if I'm not mistaken the dropdown does not support it yet

Copy link
Member

Choose a reason for hiding this comment

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

We can convert the dropdown to use pagination if we think it would be useful. Currently, it only makes one api call to retrieve the list of options.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be super useful on the SQLLab database dropdown

@codecov-commenter
Copy link

Codecov Report

Merging #10436 into master will decrease coverage by 6.22%.
The diff coverage is 90.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10436      +/-   ##
==========================================
- Coverage   70.25%   64.03%   -6.23%     
==========================================
  Files         605      544      -61     
  Lines       32377    30612    -1765     
  Branches     3271     2885     -386     
==========================================
- Hits        22745    19601    -3144     
- Misses       9522    10830    +1308     
- Partials      110      181      +71     
Flag Coverage Δ
#cypress 54.48% <100.00%> (-0.10%) ⬇️
#javascript ?
#python 69.78% <89.28%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
superset/views/core.py 74.27% <0.00%> (-0.31%) ⬇️
superset/databases/api.py 87.50% <91.66%> (+0.32%) ⬆️
superset-frontend/src/components/TableSelector.jsx 52.94% <100.00%> (-31.93%) ⬇️
superset/databases/schemas.py 100.00% <100.00%> (ø)
...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%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...uperset-frontend/src/views/CRUD/dataset/Button.tsx 6.66% <0.00%> (-93.34%) ⬇️
...uperset-frontend/src/utils/getClientErrorObject.ts 0.00% <0.00%> (-88.89%) ⬇️
.../src/dashboard/components/FilterIndicatorGroup.jsx 11.76% <0.00%> (-88.24%) ⬇️
... and 230 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 6f56cd5...5a8ca23. Read the comment docs.

@dpgaspar dpgaspar marked this pull request as ready for review July 28, 2020 14:04
@dpgaspar dpgaspar requested review from villebro and nytai July 28, 2020 14:05
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.

LGTM. I tried hard to come up with a single nit, but came up short. 🙂 Good work! 🎉

@dpgaspar
Copy link
Member Author

Thank you! @villebro

@dpgaspar dpgaspar merged commit 671461d into apache:master Jul 29, 2020
@dpgaspar dpgaspar deleted the feat/database-schemas-api branch July 29, 2020 08:33
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* fix(log): log crashes if expired or not authenticated

* fix lint and rison

* add tests

* more tests

* perm fix

* fix test not found

* JS lint

* fix Jest test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants