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

fix: database connection validation when creation #11653

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Nov 11, 2020

SUMMARY

Better database connection validation when creating a database. Discovered this bug while developing #11509 , the symptom of the failed test on the user side is you can add databases with wrong passwords as long as TABLE_NAMES_CACHE_CONFIG is enabled. The cause is that Database.get_all_schema_names is a cached function with self.id as cache key, but a newly created uncommitted Database instance always has self.id == None.

Also renames a couple of test databases to make it easier to debug in case some of them slip through failed tests and got incorrectly created.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

To reproduce the bug with unit tests:

  1. Add TABLE_NAMES_CACHE_CONFIG in test config. E.g.
    TABLE_NAMES_CACHE_CONFIG = {
      **CACHE_CONFIG,
      "CACHE_KEY_PREFIX": "superset_data_cache",
    }
    
  2. Run test case
    pytest tests/databases/api_tests.py::TestDatabaseApi::test_create_database_conn_fail
    
    multiple times
  3. You test will fail with AssertionError: 201 != 422

To reproduce the bug in user interface:

  1. Enable TABLE_NAMES_CACHE_CONFIG
  2. Try adding a database with wrong password multiple times.

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

cc @dpgaspar @john-bodley

@ktmud ktmud requested a review from dpgaspar November 11, 2020 02:27
@codecov-io
Copy link

codecov-io commented Nov 11, 2020

Codecov Report

Merging #11653 (be2ce74) into master (a9f9c4b) will decrease coverage by 3.88%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11653      +/-   ##
==========================================
- Coverage   65.76%   61.87%   -3.89%     
==========================================
  Files         873      873              
  Lines       42316    42310       -6     
  Branches     3972     3972              
==========================================
- Hits        27827    26180    -1647     
- Misses      14374    15950    +1576     
- Partials      115      180      +65     
Flag Coverage Δ
cypress ?
javascript 62.84% <ø> (ø)
python 61.28% <100.00%> (-0.64%) ⬇️

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

Impacted Files Coverage Δ
superset/databases/commands/create.py 91.83% <100.00%> (+0.34%) ⬆️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
... and 164 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 a9f9c4b...be2ce74. Read the comment docs.

@ktmud ktmud force-pushed the fix-database-tests branch from 4045167 to 82dfa7f Compare November 11, 2020 02:59
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 11, 2020
@ktmud ktmud force-pushed the fix-database-tests branch from 55dadd4 to 6491ca7 Compare November 11, 2020 17:13
@john-bodley
Copy link
Member

@dpgaspar would you mind taking a look at this as it changes some of the logic related to your API refactor?

@ktmud ktmud force-pushed the fix-database-tests branch 2 times, most recently from bd1ef12 to 459be21 Compare November 11, 2020 20:50
@ktmud ktmud force-pushed the fix-database-tests branch from 459be21 to be2ce74 Compare November 11, 2020 20:51
@dpgaspar dpgaspar merged commit 302c960 into apache:master Nov 12, 2020
@ktmud ktmud deleted the fix-database-tests branch November 12, 2020 08:13
ktmud added a commit to ktmud/superset that referenced this pull request Nov 12, 2020
ktmud added a commit that referenced this pull request Nov 12, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 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
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/M 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants