feat(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions #21838

Antonio-RiveroMartnez · 2022-10-17T21:07:02Z

SUMMARY

BigQuery is now returning the project name twice in the error message when connecting, which breaks the Regex in the custom_errors dictionary, so we need to adjust it in order to stop displaying the general connection failed message. Also, we are updating the message displayed to users and the tests.

Additionally, in order to actually being able to catch the right error, we are raising the exceptions with their original message instead of the default one, otherwise the exception handler will ignore it and display the default message.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
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 · 2022-10-17T21:13:45Z

Codecov Report

Merging #21838 (5a11829) into master (fa67315) will decrease coverage by 11.35%.
The diff coverage is 35.71%.

@@             Coverage Diff             @@
##           master   #21838       +/-   ##
===========================================
- Coverage   66.95%   55.59%   -11.36%     
===========================================
  Files        1807     1807               
  Lines       69182    69212       +30     
  Branches     7402     7402               
===========================================
- Hits        46319    38479     -7840     
- Misses      20952    28822     +7870     
  Partials     1911     1911

Flag	Coverage Δ
hive	`52.91% <28.57%> (-0.02%)`	⬇️
mysql	`?`
postgres	`?`
presto	`52.81% <28.57%> (-0.02%)`	⬇️
python	`57.96% <35.71%> (-23.54%)`	⬇️
sqlite	`?`
unit	`51.09% <35.71%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
superset/db_engine_specs/bigquery.py	`54.35% <ø> (-27.70%)`	⬇️
superset/databases/commands/create.py	`67.92% <25.00%> (-23.92%)`	⬇️
superset/databases/commands/test_connection.py	`64.00% <33.33%> (-34.64%)`	⬇️
superset/databases/api.py	`53.29% <50.00%> (-42.24%)`	⬇️
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/tags/core.py	`4.54% <0.00%> (-95.46%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-90.91%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-87.88%)`	⬇️
superset/key_value/commands/delete_expired.py	`0.00% <0.00%> (-84.00%)`	⬇️
superset/dashboards/commands/importers/v0.py	`15.62% <0.00%> (-76.25%)`	⬇️
... and 287 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

hughhhh · 2022-10-19T21:00:48Z

superset/databases/commands/create.py

+            # so we try to return the message from the errors instead of using the
+            # default message
+            error_list: List[SupersetError] = ex.errors
+            for error in error_list:


instead of doing a loop can we just do set check?

if SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR in error_list

Hmm error_list contains a list of SupersetError, while SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR is just a enum, that comparison won't work. Reducing error_list would also require iterating over the list, so, let me see if there's a more "Pythonic" way of checking without looping. But right now, I don't see how to get to the error_type for each item in the list without somehow looping over the list.

hughhhh · 2022-10-20T16:43:59Z

superset/databases/commands/create.py

+                    error.error_type
+                    == SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR
+                ):
+                    raise DatabaseConnectionFailedError(


can we just raise the original DatabaseTestConnectionFailedError

Updated. I am catching the new exception type in the API now along with sending back to the user a dict with the error messages instead of just raw strings.

can you show me the diff between the payload coming back from the API when you send DatabaseConnectionFailedError vs the original exception?

Before I was raising DatabaseConnectionFailedError which only has one error in it, so the API response would look like:
{message: '<message_here>'} while now I'm just throwing the original exception (DatabaseTestConnectionFailed) which has a list of errors, so the API would respond with a dict like: {[error_type]: [error_message], [error_type_2]: [error_message_2], ...} which is already a known pattern to the DatabaseModal for error displaying.

So basically after the change based on the input you gave me, I'm doing the latter.

For example:

Before when throwing a DatabaseConnectionFailedError the API would respond:

422 { message: 'Unable to connect. Verify that the following roles are set on the service account: "BigQuery Data Viewer", "BigQuery Metadata Viewer", "BigQuery Job User" and the following permissions are set "bigquery.readsessions.create", "bigquery.readsessions.getData"' }

And now throwing the original DatabaseTestConnectionFailed it looks like:

422 { CONNECTION_DATABASE_PERMISSIONS_ERROR: 'Unable to connect. Verify that the following roles are set on the service account: "BigQuery Data Viewer", "BigQuery Metadata Viewer", "BigQuery Job User" and the following permissions are set "bigquery.readsessions.create", "bigquery.readsessions.getData"', ... <other_errors_here> }

jinghua-qa

Able to see the custom msg for error msg. LGTM

- Start sending the message from test connection exceptions instead of a generic exception with no message in it - Update the regex CONNECTION_DATABASE_PERMISSIONS_REGEX because google is sending the project name twice now - Update tests

- Add the original message ONLY for CONNECTION_DATABASE_PERMISSIONS_ERROR error types so we can keep using the default message for other cases used in other places like tests

- Raise the original DatabaseTestConnectionFailedError exception instead of a new one. - Adding normalized_messages method in SupersetErrorsException since the to_dict is not useful for getting the right message to users

- Raise the original DatabaseConnectionFailedError from the test command with the right message when necessary - Remove the unnecessary code changes to handle multiple errors or DatabaseTestConnectionFailedError exceptions

Antonio-RiveroMartnez · 2022-10-25T19:18:20Z

superset/databases/commands/create.py

+                engine=self._properties.get("sqlalchemy_uri", "").split(":")[0],
+            )
+            # So we can show the original message
+            raise ex


Original message -> Custom message after extracting the error.
Otherwise the DatabaseConnectionFailedError would have the default Connection failed ... message caught from below.

- Send a SupersetErrorsException instead of Test one since in the future we will be raising just superset exceptions - Update testsq

eschutho · 2023-04-29T00:14:40Z

superset/databases/commands/test_connection.py

@@ -145,7 +152,7 @@ def ping(engine: Engine) -> bool:
            )
            # check for custom errors (wrong username, wrong password, etc)
            errors = database.db_engine_spec.extract_errors(ex, context)
-            raise DatabaseTestConnectionFailedError(errors) from ex
+            raise SupersetErrorsException(errors) from ex


@Antonio-RiveroMartnez I'm looking into some 500 errors for test connections, and I saw this change. What would happen if I were to change this back to raise the DatabaseTestConnectionFailedError which is a 422? Would it break anything from this PR? I checked through the implementation and it looks like it should still catch the SupersetErrorsExceptions because it is the base class, but I wanted to see if I might be missing anything here.

Hey @eschutho, the problem of doing that is that we would lost the original message and instead the message from DatabaseTestConnectionFailedError would be used (Connection failed, please check your connection settings) and not the custom message with roles after parsing the original message.

pull-request-size bot added the size/M label Oct 17, 2022

Antonio-RiveroMartnez changed the title ~~chore(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions~~ feat(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions Oct 17, 2022

AAfghahi approved these changes Oct 19, 2022

View reviewed changes

hughhhh reviewed Oct 19, 2022

View reviewed changes

hughhhh reviewed Oct 20, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the feature-59707 branch 2 times, most recently from 4dfe6d0 to 99f166b Compare October 24, 2022 17:03

jinghua-qa approved these changes Oct 24, 2022

View reviewed changes

Antonio-RiveroMartnez added 4 commits October 25, 2022 13:51

BigQuery:

f842416

- Start sending the message from test connection exceptions instead of a generic exception with no message in it - Update the regex CONNECTION_DATABASE_PERMISSIONS_REGEX because google is sending the project name twice now - Update tests

BigQuery:

8c04fa6

- Add the original message ONLY for CONNECTION_DATABASE_PERMISSIONS_ERROR error types so we can keep using the default message for other cases used in other places like tests

BigQuery:

82d13e5

- Raise the original DatabaseTestConnectionFailedError exception instead of a new one. - Adding normalized_messages method in SupersetErrorsException since the to_dict is not useful for getting the right message to users

BigQuery:

3909223

- Raise the original DatabaseConnectionFailedError from the test command with the right message when necessary - Remove the unnecessary code changes to handle multiple errors or DatabaseTestConnectionFailedError exceptions

Antonio-RiveroMartnez force-pushed the feature-59707 branch from 99f166b to 3909223 Compare October 25, 2022 19:15

Antonio-RiveroMartnez commented Oct 25, 2022

View reviewed changes

hughhhh self-requested a review October 26, 2022 17:31

hughhhh approved these changes Oct 26, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the feature-59707 branch 2 times, most recently from c932d96 to 38a9003 Compare October 26, 2022 19:10

BigQuery:

5a11829

- Send a SupersetErrorsException instead of Test one since in the future we will be raising just superset exceptions - Update testsq

Antonio-RiveroMartnez force-pushed the feature-59707 branch from 38a9003 to 5a11829 Compare October 26, 2022 19:58

pull-request-size bot added size/L and removed size/M labels Oct 26, 2022

hughhhh merged commit 203b289 into apache:master Oct 26, 2022

eschutho reviewed Apr 29, 2023

View reviewed changes

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions #21838

feat(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions #21838

Antonio-RiveroMartnez commented Oct 17, 2022

codecov bot commented Oct 17, 2022 •

edited

hughhhh Oct 19, 2022

Antonio-RiveroMartnez Oct 19, 2022 •

edited

hughhhh Oct 20, 2022

Antonio-RiveroMartnez Oct 20, 2022

hughhhh Oct 20, 2022

Antonio-RiveroMartnez Oct 21, 2022 •

edited

jinghua-qa left a comment

Antonio-RiveroMartnez Oct 25, 2022

eschutho Apr 29, 2023

Antonio-RiveroMartnez May 2, 2023

feat(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions #21838

feat(bigquery): Custom message when Service Account doesnt have the correct Roles and Permissions #21838

Conversation

Antonio-RiveroMartnez commented Oct 17, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 17, 2022 • edited

Codecov Report

hughhhh Oct 19, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Oct 19, 2022 • edited

Choose a reason for hiding this comment

hughhhh Oct 20, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Oct 20, 2022

Choose a reason for hiding this comment

hughhhh Oct 20, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Oct 21, 2022 • edited

Choose a reason for hiding this comment

jinghua-qa left a comment

Choose a reason for hiding this comment

Antonio-RiveroMartnez Oct 25, 2022

Choose a reason for hiding this comment

eschutho Apr 29, 2023

Choose a reason for hiding this comment

Antonio-RiveroMartnez May 2, 2023

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2022 •

edited

Antonio-RiveroMartnez Oct 19, 2022 •

edited

Antonio-RiveroMartnez Oct 21, 2022 •

edited