feat: add endpoint to fetch available DBs #14208
Conversation
betodealmeida
force-pushed
the
ch6945
branch
7 times, most recently
from
c2c4fe1 to
5cf45dc
Compare
Codecov Report
@@ Coverage Diff @@
## master #14208 +/- ##
==========================================
+ Coverage 76.13% 76.16% +0.02%
==========================================
Files 945 945
Lines 47946 48030 +84
Branches 5950 5950
==========================================
+ Hits 36503 36580 +77
- Misses 11237 11244 +7
Partials 206 206
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -839,3 +841,67 @@ def function_names(self, pk: int) -> Response: | |||
| if not database: | |||
| return self.response_404() | |||
| return self.response(200, function_names=database.function_names,) | |||
|
|
|||
| @expose("/available/", methods=["GET"]) | |||
There was a problem hiding this comment.
My initial thought is that this endpoint refers to a status of databases and not a new resource. Should we make this endpoint more restful and if so, how about something like GET /databases?status=available. On the other hand, I realize that we already have a lot of other existing routes that aren't particularly restful either, so then this route would be an outlier.
There was a problem hiding this comment.
I don't have a strong opinion about this... @dpgaspar any thoughts here?
There was a problem hiding this comment.
With the current pattern, i'd rather it be separate endpoint vs. trying to hotwire it into the database endpoint. It is cleaner for us to have endpoint -> command this will also help with error handling in the future.
Would love to hear what @dpgaspar has to say
There was a problem hiding this comment.
looks like we can leave this as is for consistency.
| @@ -839,3 +841,67 @@ def function_names(self, pk: int) -> Response: | |||
| if not database: | |||
| return self.response_404() | |||
| return self.response(200, function_names=database.function_names,) | |||
|
|
|||
| @expose("/available/", methods=["GET"]) | |||
There was a problem hiding this comment.
With the current pattern, i'd rather it be separate endpoint vs. trying to hotwire it into the database endpoint. It is cleaner for us to have endpoint -> command this will also help with error handling in the future.
Would love to hear what @dpgaspar has to say
* feat: add endpoint to fetch available DBs * Fix lint
mistercrunch
added
🏷️ bot
SUMMARY
We're working on redesigning the "Add Database" dialog so that users don't have to type a SQLAlchemy URI or figure out which driver should be used for each database. Instead, in the new flow:
These parameters are passed to the backend, which builds the SQLAlchemy URI depending on the selected database. Since the logic of building the SQLAlchemy URI is DB-dependent we opted against doing it in the frontend, and store the logic in each DB engine spec.
To make this work, this PR introduces:
BaseParametersMixinmixin for DB engine specs, which can be progressively added to DB engine specs in order to allow them to be configured via parameters instead of the SQLAlchemy URI. The mixin defines a schema for the parameters (so the frontend knows what do prompt), and methods to convert between the parameters and the SQLAlchemy URI.DatabaseParametersSchemaMixin. The schema mixin handles the job of building the SQLAlchemy URI from the parameters before validating a payload representing a database. This way, APIs don't need to be updated to support the new way of configuring databases./api/v1/database/available/that list the available databases. Users can specify insuperset/config.pya list of preferred databases, which will then be displayed prominently to the user (with logos, eg).With this work we hope to simplify the task of adding a new database, removing a lot of the guesswork that is needed today. Users will immediately know which databases are available and what information is needed to configure them. And they will always have the option of falling back to a SQLAlchemy URI if they want or need.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Added unit tests and tested manually that
test_connectionworks with the new payload.ADDITIONAL INFORMATION