feat: Surface parameters fields for database model by hughhhh · Pull Request #14646 · apache/superset

hughhhh · 2021-05-14T17:08:43Z

SUMMARY

Update the api to surface parameters in GET database. This will be leveraged for whenever a user wants to edit a database assuming configuration type is dynamic.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

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

AAfghahi · 2021-05-14T17:22:34Z

+  host: string;
+  password?: string;
+  port: number;
+  query: Object;


I think it is better to use the query props here, which I think you can import.

Confusingly, this is a different query: https://docs.sqlalchemy.org/en/14/core/engines.html#add-parameters-to-the-url-query-string

oh interesting, so then would Record<string, string> be more explicit here given that it has string keys and values?

That sounds like a good plan.

I also added in some props in #14583. For bigquery most of those props are going be optional (maybe some other dbs too). Are we going to use db parameters for Gsheets for example?

@AAfghahi right, Record<string, string>

But we need to keep in mind that these are specific for Postgres and similar databases. For other DBs some attributes might be optional, or there might be a different set of attributes, so maybe we should just define the type as Object.

that's fair, I hadn't considered that.

codecov · 2021-05-14T18:23:41Z

Codecov Report

Merging #14646 (5f30cee) into master (74473e2) will increase coverage by 0.39%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #14646      +/-   ##
==========================================
+ Coverage   77.20%   77.59%   +0.39%     
==========================================
  Files         958      958              
  Lines       48492    49127     +635     
  Branches     5691     5767      +76     
==========================================
+ Hits        37437    38119     +682     
+ Misses      10855    10787      -68     
- Partials      200      221      +21

Flag	Coverage Δ
hive	`81.26% <80.00%> (?)`
javascript	`72.37% <ø> (-0.16%)`	⬇️
mysql	`81.20% <80.00%> (-0.02%)`	⬇️
postgres	`81.23% <100.00%> (-0.01%)`	⬇️
presto	`80.92% <80.00%> (?)`
python	`82.08% <100.00%> (+0.80%)`	⬆️
sqlite	`81.18% <80.00%> (?)`

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

Impacted Files	Coverage Δ
superset/databases/api.py	`92.41% <ø> (ø)`
superset/models/core.py	`89.33% <100.00%> (-0.07%)`	⬇️
...src/dashboard/components/dnd/handleScroll/index.ts	`67.85% <0.00%> (-5.48%)`	⬇️
superset/exceptions.py	`99.13% <0.00%> (-0.87%)`	⬇️
superset/reports/logs/api.py	`94.59% <0.00%> (-0.86%)`	⬇️
superset/app.py	`81.23% <0.00%> (-0.15%)`	⬇️
superset-frontend/src/dashboard/actions/hydrate.js	`1.74% <0.00%> (-0.02%)`	⬇️
superset/utils/urls.py	`100.00% <0.00%> (ø)`
...rset-frontend/src/dashboard/util/findPermission.ts	`100.00% <0.00%> (ø)`
... and 17 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 74473e2...5f30cee. Read the comment docs.

eschutho · 2021-05-14T19:44:10Z

+    @property
+    def parameters(self) -> Optional[Dict[str, Any]]:
+        # Build parameters if db_engine_spec is a subclass of BasicParametersMixin
+        parameters = {"engine": self.backend}


so for sqlalchemy forms, we're only going to return engine? Maybe that should be the only required type then, it looks like.

Yea good catch i'll make sure to update the type on the types file

betodealmeida · 2021-05-14T21:07:25Z

+        # Build parameters if db_engine_spec is a subclass of BasicParametersMixin
+        parameters = {"engine": self.backend}
+
+        if issubclass(self.db_engine_spec, BasicParametersMixin):


After we add BQ we need to change this, right?

Yea that PR will have the updated logic here to allow us to pass Big Query credentials

hughhhh · 2021-05-16T00:42:50Z

closing in favor of #14653

save types and update api

182eb3c

pull-request-size Bot added the size/M label May 14, 2021

betodealmeida self-requested a review May 14, 2021 17:13

AAfghahi reviewed May 14, 2021

View reviewed changes

eschutho reviewed May 14, 2021

View reviewed changes

betodealmeida reviewed May 14, 2021

View reviewed changes

remove parameters for now

5f30cee

hughhhh closed this May 16, 2021

mistercrunch deleted the hugh/add-parameters-api branch March 25, 2024 21:21

Conversation

hughhhh commented May 14, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughhhh commented May 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented May 14, 2021 •

edited

Loading