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 endpoint to validate databases using separate parameters #14420

Merged
merged 3 commits into from May 13, 2021

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Apr 30, 2021

SUMMARY

This PR adds a new endpoint at api/v1/database/validate_parameters for validating separate parameters when adding/editing a database. With this flow, users can add databases by passing DB-specific parameters (username, password, host, port, etc.) instead of the full SQLAlchemy URI.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Added unit tests for the new command ValidateDatabaseParametersCommand, as well as the new API.

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@betodealmeida betodealmeida changed the title WIP feat: API endpoint to validate databases using separate parameters Apr 30, 2021
@betodealmeida betodealmeida marked this pull request as ready for review April 30, 2021 00:54
"username": url.username,
"database": url.database,
}
errors = database.db_engine_spec.extract_errors(ex, context)
Copy link
Member

Choose a reason for hiding this comment

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

add logger.warning here

Copy link
Member Author

Choose a reason for hiding this comment

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

No need, raising exceptions in the API get logged by the error handler:

# SIP-40 compatible error responses; make sure APIs raise
# SupersetErrorException or SupersetErrorsException
@superset_app.errorhandler(SupersetErrorException)
def show_superset_error(ex: SupersetErrorException) -> FlaskResponse:
logger.warning(ex)
return json_errors_response(errors=[ex.error], status=ex.status)
@superset_app.errorhandler(SupersetErrorsException)
def show_superset_errors(ex: SupersetErrorsException) -> FlaskResponse:
logger.warning(ex)
return json_errors_response(errors=ex.errors, status=ex.status)

@hughhhh hughhhh self-requested a review May 11, 2021 02:00
Copy link
Member

@hughhhh hughhhh left a comment

Choose a reason for hiding this comment

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

1 comment and 🚢

@betodealmeida betodealmeida force-pushed the ch6759 branch 2 times, most recently from a5ffd8a to 6b9828f Compare May 11, 2021 16:37
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #14420 (b579f44) into master (09050ae) will decrease coverage by 0.01%.
The diff coverage is 89.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14420      +/-   ##
==========================================
- Coverage   77.40%   77.38%   -0.02%     
==========================================
  Files         958      959       +1     
  Lines       48326    48450     +124     
  Branches     5677     5680       +3     
==========================================
+ Hits        37405    37493      +88     
- Misses      10721    10757      +36     
  Partials      200      200              
Flag Coverage Δ
hive 80.96% <89.55%> (+0.06%) ⬆️
javascript 72.51% <ø> (+0.04%) ⬆️
mysql 81.21% <89.55%> (+0.06%) ⬆️
postgres 81.25% <89.55%> (+0.06%) ⬆️
presto ?
python 81.62% <89.55%> (-0.10%) ⬇️
sqlite 80.85% <89.55%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...rset-frontend/src/components/ErrorMessage/types.ts 100.00% <ø> (ø)
superset/constants.py 100.00% <ø> (ø)
superset/db_engine_specs/postgres.py 96.93% <ø> (ø)
superset/databases/commands/validate.py 76.00% <76.00%> (ø)
superset/databases/api.py 92.41% <93.75%> (+0.08%) ⬆️
superset/db_engine_specs/base.py 88.42% <96.42%> (+0.33%) ⬆️
superset/config.py 91.09% <100.00%> (ø)
superset/databases/commands/exceptions.py 94.00% <100.00%> (+0.81%) ⬆️
superset/databases/schemas.py 99.54% <100.00%> (+0.01%) ⬆️
superset/db_engine_specs/bigquery.py 95.87% <100.00%> (ø)
... and 20 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 09050ae...b579f44. Read the comment docs.

@betodealmeida betodealmeida force-pushed the ch6759 branch 7 times, most recently from c5a9205 to db8afac Compare May 12, 2021 04:03
@betodealmeida betodealmeida merged commit 31f406a into apache:master May 13, 2021
@@ -630,7 +630,7 @@ def _try_json_readsha( # pylint: disable=unused-argument

# Default row limit for SQL Lab queries. Is overridden by setting a new limit in
# the SQL Lab UI
DEFAULT_SQLLAB_LIMIT = 1000
DEFAULT_SQLLAB_LIMIT = 10000
Copy link
Member

Choose a reason for hiding this comment

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

@betodealmeida any reason for the change from 1,000 to 10,000? We have had a number of employees mention that 10,000 overloads their local storage use and breaks Chrome and thus I'm unsure if the default of 10,000 makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, let me change the default back. Sorry for that!

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
…pache#14420)

* feat: new endpoint for validating database parameters

* Rebase

* Remove broken tests
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…pache#14420)

* feat: new endpoint for validating database parameters

* Rebase

* Remove broken tests
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
…pache#14420)

* feat: new endpoint for validating database parameters

* Rebase

* Remove broken tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.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/XL 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants