fix(database): make to display validation error msg when all cases #20095

prosdev0107 · 2022-05-17T10:44:22Z

SUMMARY

The dynamic form to connect to PostgreSQL is not returning connection errors to the user.
No errors display when the error_type is GENERIC_DB_ENGINE_ERROR and so error alerts are displayed at this case.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

db-error-display.mov

TESTING INSTRUCTIONS

How to reproduce the bug

Please follow the steps with this sample data.

Hover over the + icon > Data > Connect database.
Select PostgreSQL.
Input everything correctly with the exception of the DB name.
Click on CONNECT. No error is shown.
Fix the DB name, and now enter an incorrect username.
Click on CONNECT. No error is shown.
Replace the password to an incorrect value.
Click on CONNECT. No error is shown.

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-05-17T11:32:50Z

Codecov Report

Merging #20095 (d7428c6) into master (6650076) will increase coverage by 0.00%.
The diff coverage is 45.45%.

@@           Coverage Diff           @@
##           master   #20095   +/-   ##
=======================================
  Coverage   66.35%   66.35%           
=======================================
  Files        1767     1767           
  Lines       67356    67360    +4     
  Branches     7147     7149    +2     
=======================================
+ Hits        44694    44698    +4     
  Misses      20834    20834           
  Partials     1828     1828

Flag	Coverage Δ
javascript	`52.12% <45.45%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...onents/ErrorMessage/ErrorMessageWithStackTrace.tsx	`60.00% <ø> (ø)`
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`31.69% <14.28%> (+0.16%)`	⬆️
...rontend/src/components/ErrorMessage/ErrorAlert.tsx	`90.24% <100.00%> (+0.77%)`	⬆️

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

stephenLYZ

Thanks for the fix! Left a comment.

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

stephenLYZ · 2022-05-18T01:48:25Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -1088,7 +1088,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
    let alertErrors: string[] = [];
    if (isEmpty(dbErrors) === false) {


Can we also modify this condition to keep the code readable to be consistent? Thanks!

yousoph · 2022-05-20T18:55:37Z

/testenv up

github-actions · 2022-05-20T18:57:40Z

@yousoph Ephemeral environment spinning up at http://54.214.197.133:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

…uide.

rusackas · 2022-06-02T20:20:35Z

/testenv up

github-actions · 2022-06-02T20:25:13Z

@rusackas Ephemeral environment spinning up at http://54.149.117.75:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

stephenLYZ · 2022-06-03T13:26:39Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -1086,22 +1090,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
  // eslint-disable-next-line consistent-return
  const errorAlert = () => {
    let alertErrors: string[] = [];
-    if (isEmpty(dbErrors) === false) {
+    if (isEmpty(dbErrors)) {


Suggested change

if (isEmpty(dbErrors)) {

if (!isEmpty(dbErrors)) {

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

yousoph · 2022-06-06T08:01:56Z

@prosdev0107
this is looking good! Two small changes:

In the copy, let's put "See more" in quotes. The second sentence should start:

Click "See more" for database-provided... etc etc

Can we move "See more" to the bottom of the error message? See the design guidelines for an example

prosdev0107 · 2022-06-06T18:00:12Z

@prosdev0107 this is looking good! Two small changes:

In the copy, let's put "See more" in quotes. The second sentence should start:

Click "See more" for database-provided... etc etc

Can we move "See more" to the bottom of the error message? See the design guidelines for an example

@yousoph
Thank you for your feedback and Resolved them now!

yousoph · 2022-06-07T05:37:44Z

/testenv up

github-actions · 2022-06-07T05:42:01Z

@yousoph Ephemeral environment spinning up at http://34.219.229.127:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2022-06-07T05:54:45Z

Thanks for the changes! Looking good to me

rusackas · 2022-06-10T17:58:01Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -1086,22 +1092,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
  // eslint-disable-next-line consistent-return
  const errorAlert = () => {
    let alertErrors: string[] = [];
-    if (isEmpty(dbErrors) === false) {
+    if (!isEmpty(dbErrors)) {


Previously, this was if (isEmpty(dbErrors) === false)
This new line of if (!isEmpty(dbErrors)) is logically the same, which is great...

However, now I'm a little worried. Our QA round didn't catch anything wrong when this logic was indeed wrong. So... what does this exactly DO, and how do we test it to make sure it's working as expected?

Unit tests to cover this area of code and prove it works would be ideal. Spinning up an ephemeral for manual testing now that CI has passed. Please let us know what manual testing steps would help here.

@rusackas
I tried to add the unit test for this case but couldn't do it because of that getValidation function is a call in a hooks and so it is invalid hook call in jest file. And so I add manual testing steps into the Test instructions

rusackas · 2022-06-10T19:33:16Z

/testenv up

github-actions · 2022-06-10T19:40:39Z

@rusackas Ephemeral environment spinning up at http://35.85.218.215:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

michael-s-molina · 2022-06-27T16:41:21Z

Hi @prosdev0107. Thanks for the PR. I have two suggestions:

Can we treat invalid usernames and passwords with a generic message? Right now the message confirms that my username is valid and only my password is invalid which raises security concerns. Maybe something like "Invalid credentials" for both the username and password?

The second suggestion is to automatically scroll to the bottom when there's an error because it's not very clear right now.

yousoph · 2022-06-27T17:16:46Z

@michael-s-molina I believe the message is actually generic right now - even if your username is wrong, it gives you the same message saying that the password provided doesn't work for the username entered

michael-s-molina · 2022-06-27T17:27:00Z

@michael-s-molina I believe the message is actually generic right now - even if your username is wrong, it gives you the same message saying that the password provided doesn't work for the username entered

I'm not sure if this is another error but when I try with an unknown user I get another message:

yousoph · 2022-06-27T17:43:26Z

Oops, you're right! It does look like it's a diff error when the username is wrong

zhaoyongjie

This error message is from the database driver, considering security risks(you couldn't know what message will output from drivers), I suggest doing some exception catch in Superset.

others, LGTM

rusackas · 2022-07-07T04:23:28Z

@yousoph is this OK to merge if the errors displayed are at the whim of the database? We can catch/throw an exception as Yongjie suggests - but maybe we can do that in a separate PR? ¯\_(ツ)_/¯

Otherwise, @prosdev0107 I think this just needs a rebase or conflict resolution

rusackas · 2022-08-09T16:40:55Z

Conflicts resolved... let's merge if/when CI passes

github-actions · 2022-08-24T18:27:48Z

Ephemeral environment shutdown and build artifacts deleted.

…cases (apache#20095)" This reverts commit d568999.

… msg when all … (#21277)

… error msg when all … (apache#21277) (cherry picked from commit 4b22137)

…pache#20095) * fix(database): make to display validation error msg when all cases * fix(db): make to update the alert error condition * fix(db): make to add error detail display * fix(db): make to update error alert display by superset error style guide. * fix(db): make to style modal header title with h4 * fix(db): make to place see more on bottom instead of top * fix(db): make to fix shortly * fix(db): make to fix lint issue Co-authored-by: Evan Rusackas <evan@preset.io>

fix(database): make to display validation error msg when all cases

42bcc0a

superset-github-bot bot added the Preset-Patch label May 17, 2022

pull-request-size bot added the size/XS label May 17, 2022

stephenLYZ reviewed May 17, 2022

View reviewed changes

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Outdated Show resolved Hide resolved

fix(db): make to update the alert error condition

5c6bd51

stephenLYZ reviewed May 18, 2022

View reviewed changes

fix(db): make to add error detail display

5644890

pull-request-size bot added size/M and removed size/XS labels May 20, 2022

diegomedina248 approved these changes May 23, 2022

View reviewed changes

prosdev0107 added 4 commits May 23, 2022 11:12

fix(db): make to update error alert display by superset error style g…

39f3243

…uide.

fix(db): make to rebase master

9dbedc6

fix(db): make to style modal header title with h4

19e8868

fix(db): make to merge master

da05138

stephenLYZ reviewed Jun 3, 2022

View reviewed changes

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Outdated Show resolved Hide resolved

fix(db): make to place see more on bottom instead of top

4527683

prosdev0107 added 3 commits June 9, 2022 19:32

fix(db): make to fix shortly

0da5e33

fix(db): make to fix lint issue

289ef3e

fix(db): make to merge master

36cc44e

rusackas reviewed Jun 10, 2022

View reviewed changes

zhaoyongjie self-requested a review June 29, 2022 08:02

zhaoyongjie approved these changes Jun 29, 2022

View reviewed changes

rusackas added 2 commits July 11, 2022 13:56

Merge branch 'master' into fix/47160-database-validation

fcf40eb

Merge branch 'master' into fix/47160-database-validation

d7428c6

rusackas merged commit d568999 into apache:master Aug 24, 2022

sadpandajoe added a commit to preset-io/superset that referenced this pull request Aug 31, 2022

Revert "fix(database): make to display validation error msg when all …

2d5bc29

…cases (apache#20095)" This reverts commit d568999.

sadpandajoe added a commit to preset-io/superset that referenced this pull request Aug 31, 2022

Revert "fix(database): make to display validation error msg when all …

4cecc4a

…cases (apache#20095)" This reverts commit d568999.

sadpandajoe mentioned this pull request Aug 31, 2022

fix: Revert "#20095 - fix(database): make to display validation error msg when all … #21277

Merged

9 tasks

rusackas pushed a commit that referenced this pull request Aug 31, 2022

fix: Revert "#20095 - fix(database): make to display validation error…

4b22137

… msg when all … (#21277)

sadpandajoe added a commit to preset-io/superset that referenced this pull request Sep 1, 2022

fix: Revert "apache#20095 - fix(database): make to display validation…

916fe22

… error msg when all … (apache#21277) (cherry picked from commit 4b22137)

zhaoyongjie mentioned this pull request Sep 6, 2022

fix: database modal crashed when use SQLAlchemy URI string #21342

Merged

9 tasks

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

fix(database): make to display validation error msg when all cases #20095

fix(database): make to display validation error msg when all cases #20095

prosdev0107 commented May 17, 2022 •

edited by eschutho

Loading

codecov bot commented May 17, 2022 •

edited

Loading

stephenLYZ left a comment

stephenLYZ May 18, 2022

yousoph commented May 20, 2022

github-actions bot commented May 20, 2022

rusackas commented Jun 2, 2022

github-actions bot commented Jun 2, 2022

stephenLYZ Jun 3, 2022

yousoph commented Jun 6, 2022

prosdev0107 commented Jun 6, 2022

yousoph commented Jun 7, 2022

github-actions bot commented Jun 7, 2022

yousoph commented Jun 7, 2022

rusackas Jun 10, 2022

rusackas Jun 10, 2022

prosdev0107 Jun 13, 2022 •

edited

Loading

rusackas commented Jun 10, 2022

github-actions bot commented Jun 10, 2022

michael-s-molina commented Jun 27, 2022

yousoph commented Jun 27, 2022

michael-s-molina commented Jun 27, 2022

yousoph commented Jun 27, 2022

zhaoyongjie left a comment

rusackas commented Jul 7, 2022 •

edited

Loading

rusackas commented Aug 9, 2022

github-actions bot commented Aug 24, 2022

		@@ -1088,7 +1088,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
		let alertErrors: string[] = [];
		if (isEmpty(dbErrors) === false) {

fix(database): make to display validation error msg when all cases #20095

fix(database): make to display validation error msg when all cases #20095

Conversation

prosdev0107 commented May 17, 2022 • edited by eschutho Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 17, 2022 • edited Loading

Codecov Report

stephenLYZ left a comment

Choose a reason for hiding this comment

stephenLYZ May 18, 2022

Choose a reason for hiding this comment

yousoph commented May 20, 2022

github-actions bot commented May 20, 2022

rusackas commented Jun 2, 2022

github-actions bot commented Jun 2, 2022

stephenLYZ Jun 3, 2022

Choose a reason for hiding this comment

yousoph commented Jun 6, 2022

prosdev0107 commented Jun 6, 2022

yousoph commented Jun 7, 2022

github-actions bot commented Jun 7, 2022

yousoph commented Jun 7, 2022

rusackas Jun 10, 2022

Choose a reason for hiding this comment

rusackas Jun 10, 2022

Choose a reason for hiding this comment

prosdev0107 Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

rusackas commented Jun 10, 2022

github-actions bot commented Jun 10, 2022

michael-s-molina commented Jun 27, 2022

yousoph commented Jun 27, 2022

michael-s-molina commented Jun 27, 2022

yousoph commented Jun 27, 2022

zhaoyongjie left a comment

Choose a reason for hiding this comment

rusackas commented Jul 7, 2022 • edited Loading

rusackas commented Aug 9, 2022

github-actions bot commented Aug 24, 2022

prosdev0107 commented May 17, 2022 •

edited by eschutho

Loading

codecov bot commented May 17, 2022 •

edited

Loading

prosdev0107 Jun 13, 2022 •

edited

Loading

rusackas commented Jul 7, 2022 •

edited

Loading