feat: Create BigQuery Parameters for DatabaseModal by hughhhh · Pull Request #14721 · apache/superset

hughhhh · 2021-05-19T19:31:48Z

SUMMARY

Backend piece for creating BigQuery Database connection form. In this PR, we setup a BigQueryParametersSchema with credential_json as a encrypted_extra field. This allows the frontend to know that this field should merged into encrypted_extra JSON which is needed for validate and add endpoints.

curl "http://localhost:9001/api/v1/database/available/"

{
  "databases": [
   // ...
    {
      "engine": "bigquery",
      "name": "Google BigQuery",
      "parameters": {
        "properties": {
          "credentials_info": {
            "description": "credentials.json file for BigQuery",
            "type": "string",
            "x-encrypted": true
          }
        },
        "type": "object"
      }
  ]
}

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

codecov · 2021-05-19T20:18:35Z

Codecov Report

Merging #14721 (42e7247) into master (d71b8b3) will decrease coverage by 0.07%.
The diff coverage is 82.14%.

@@            Coverage Diff             @@
##           master   #14721      +/-   ##
==========================================
- Coverage   77.62%   77.54%   -0.08%     
==========================================
  Files         962      962              
  Lines       49017    49082      +65     
  Branches     6155     6155              
==========================================
+ Hits        38050    38062      +12     
- Misses      10763    10816      +53     
  Partials      204      204

Flag	Coverage Δ
hive	`81.37% <82.14%> (-0.01%)`	⬇️
javascript	`72.41% <ø> (ø)`
mysql	`81.64% <82.14%> (-0.01%)`	⬇️
postgres	`81.66% <82.14%> (-0.01%)`	⬇️
presto	`?`
python	`82.03% <82.14%> (-0.16%)`	⬇️
sqlite	`81.27% <78.57%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
superset/models/core.py	`89.70% <ø> (-0.27%)`	⬇️
superset/databases/commands/validate.py	`75.00% <66.66%> (-1.00%)`	⬇️
superset/db_engine_specs/bigquery.py	`90.83% <76.47%> (-5.04%)`	⬇️
superset/databases/api.py	`92.39% <100.00%> (-0.03%)`	⬇️
superset/databases/schemas.py	`99.57% <100.00%> (+0.02%)`	⬆️
superset/db_engine_specs/base.py	`88.30% <100.00%> (ø)`
superset/db_engine_specs/gsheets.py	`78.94% <0.00%> (-21.06%)`	⬇️
superset/db_engine_specs/presto.py	`84.42% <0.00%> (-5.48%)`	⬇️
superset/connectors/sqla/models.py	`88.53% <0.00%> (-1.44%)`	⬇️

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 d71b8b3...42e7247. Read the comment docs.

hughhhh · 2021-05-21T02:02:12Z


-            if issubclass(engine_spec, BasicParametersMixin):
-                payload["parameters"] = engine_spec.parameters_json_schema()
+            if hasattr(engine_spec, "parameters_json_schema") or hasattr(


@betodealmeida let me know if this is good enough of a guard statement moving forward to make sure we don't call parameters_json_schema for engines we have implemented yet

This looks great!

Though I think ideally we'd have all these methods in BaseEngineSpec and we'd check if parameters_schema is not None.

betodealmeida · 2021-05-21T19:15:15Z

        sqlalchemy_uri = engine_spec.build_sqlalchemy_uri(
-            self._properties["parameters"]  # type: ignore
+            self._properties["parameters"],  # type: ignore
+            self._properties.get("encrypted_extra", "{}"),


We need to deserialize this:

serialized_encrypted_extra = self._properties.get("encrypted_extra", "{}") try: encrypted_extra = json.loads(serialized_encrypted_extra) except json.decoder.JSONDecodeError: encrypted_extra = {}

Then:

sqlalchemy_uri = engine_spec.build_sqlalchemy_uri( self._properties["parameters"], encrypted_extra, )

You can then pass serialized_encrypted_extra on line 95 below.

betodealmeida · 2021-05-21T20:28:33Z


-            if issubclass(engine_spec, BasicParametersMixin):
-                payload["parameters"] = engine_spec.parameters_json_schema()
+            if hasattr(engine_spec, "parameters_json_schema") or hasattr(


This looks great!

betodealmeida · 2021-05-21T20:28:52Z

        the constructed SQLAlchemy URI to be passed.
        """
        parameters = data.pop("parameters", None)
+        encrypted_extra = data.get("encrypted_extra", None)


Same here, we need to try to deserialize from JSON like above.

betodealmeida · 2021-05-21T20:31:36Z

+def encrypted_field_properties(self, field: Any, **_) -> Dict[str, Any]:  # type: ignore
+    ret = {}
+    if isinstance(field, EncryptedField):
+        if self.openapi_version.major > 2:
+            ret["x-encrypted-extra"] = True
+    return ret


Let's move this and EncryptedField outside, since other specs will also use them. We can have them in superset/databases/schemas.py.

betodealmeida · 2021-05-21T20:36:23Z

+    @classmethod
+    def get_parameters_from_uri(cls, _: str) -> Any:
+        # BigQuery doesn't have parameters
+        return None


Ideally:

get_parameters_from_uri(build_sqlalchemy_uri(parameters)) == parameters

Ie, this should be the opposite of build_sqlalchemy_uri.

We need to return credentials_info here so that the edit form can show it to the user. This means that we need to modify get_parameters_from_uri to also receive encrypted_extra.

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

betodealmeida

Just a comment on superset/databases/schemas.py that needs to be addresses.

betodealmeida · 2021-05-22T20:43:20Z

+        if hasattr(engine_spec, "build_sqlalchemy_uri"):
+            data["sqlalchemy_uri"] = engine_spec.build_sqlalchemy_uri(  # type: ignore
+                parameters, encrypted_extra
+            )


This needs to replace lines 273-278, and inside the if block.

betodealmeida

Looks great, excited to see this working with the new FE! I left just a few nits

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* master: (163 commits) fix(native-filters): Manage default value of filters by superset (apache#14785) fix: Additional ResultSet tests (apache#14741) chore: added BasicParametersMixin to Redshift (apache#14752) fix: make dataset list sort case insensitive (apache#14528) fix: use encodeURIComponent when getting table metadata (apache#14790) fix: ensure engine is outside parameters (apache#14787) database modal should close on connect with tab layout (apache#14771) feat(native-filters): add search all filter options (apache#14710) fix: extra query in Dashboard when native filter enabled (apache#14770) chore: Improves the native filters UI/UX - iteration 2 (apache#14753) fix(native filters): Fix explore state (apache#14779) fix(explore): DndColumnSelect not handling controls with "multi: false" (apache#14737) feat: Create BigQuery Parameters for DatabaseModal (apache#14721) feat: enable user impersonation in GSheets (apache#14767) fix: add DB should not say it's Postgres (apache#14766) Revert "fix(dashboard): multiple query trigger when native filter enabled (apache#14734)" (apache#14762) feat: save database with new dynamic form (apache#14583) fix: save non-parameter DBs (apache#14759) chore: Removes ColorSchemeControl.less (apache#14199) fix(explore): Icons width (apache#14717) ...

save bg form

139cd49

pull-request-size Bot added the size/L label May 19, 2021

remove prints

f252d72

add fe bigquery form

b77a3e5

betodealmeida reviewed May 20, 2021

View reviewed changes

Comment thread superset/db_engine_specs/bigquery.py Outdated

hughhhh added 6 commits May 20, 2021 15:54

update api

bb24664

overriding credentials info json

4a1683e

add custom field to big query schema

1fc08d4

tis works

b8640a1

update

8ca9b80

remove fe code

8adfe4c

hughhhh changed the title ~~save bg form~~ feat: Create BigQuery Parameters for DatabaseModal May 21, 2021

hughhhh commented May 21, 2021

View reviewed changes

hughhhh marked this pull request as ready for review May 21, 2021 02:05

hughhhh requested a review from betodealmeida May 21, 2021 02:06

hughhhh added 4 commits May 21, 2021 10:09

update encrypted_extra reference

7d2b7d9

fix linting

9bddea7

fix linting

cdc6580

fix linting againa

0506752

betodealmeida requested changes May 21, 2021

View reviewed changes

hughhhh and others added 5 commits May 21, 2021 17:32

Update superset/db_engine_specs/bigquery.py

401873d

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

refactor

77ce531

rewrite get parameters from uri

5a604b4

remove unused import

bb66ffb

Merge branch 'master' into hugh-bg-form-3

dffb040

betodealmeida requested changes May 22, 2021

View reviewed changes

hughhhh added 2 commits May 22, 2021 17:10

yerp

b4e9a02

linting

d9d7379

betodealmeida approved these changes May 23, 2021

View reviewed changes

Comment thread superset/databases/api.py Outdated

Comment thread superset/db_engine_specs/bigquery.py Outdated

Comment thread superset/db_engine_specs/bigquery.py Outdated

Comment thread superset/db_engine_specs/bigquery.py Outdated

hughhhh and others added 5 commits May 23, 2021 09:36

Update superset/db_engine_specs/bigquery.py

eaecbb6

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/db_engine_specs/bigquery.py

a5cd2b7

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/databases/api.py

361e755

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

fix test

65822ed

Update superset/db_engine_specs/bigquery.py

42e7247

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

hughhhh merged commit 6d33432 into master May 23, 2021

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021

feat: Create BigQuery Parameters for DatabaseModal (apache#14721)

e9ba6ae

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

feat: Create BigQuery Parameters for DatabaseModal (apache#14721)

6ef7891

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

feat: Create BigQuery Parameters for DatabaseModal (apache#14721)

e2c884b

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 2024

mistercrunch deleted the hugh-bg-form-3 branch March 26, 2024 16:13

qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026

feat: Create BigQuery Parameters for DatabaseModal (apache#14721)

a973f54

Conversation

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

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betodealmeida left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betodealmeida left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hughhhh commented May 19, 2021 •

edited

Loading

codecov Bot commented May 19, 2021 •

edited

Loading