fix: status 500 thrown when creating a dataset due to invalid sql

[jzhao62]

SUMMARY

previously when a dataset is being created, supersetGenericDB error can be thrown at the db_engine layer if a sql statement is invalid

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

before.mp4

after

after.mp4

TESTING INSTRUCTIONS

• enable sqllab in database configuration
• open sqllab
• type invalid sql
• click save as dataset

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

[qleroy]

Tested on my dev setup and confirm the Fatal error is turned into a Dataset could not be created. error.

This is also a first step towards resolving #25786.

[john-bodley]

Thanks @jzhao62 for the PR. Would you mind also adding some unit tests to prevent potential future regressions?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.48%. Comparing base (76d897e) to head (3a661ab).
Report is 154 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #27796       +/-   ##
===========================================
+ Coverage   60.48%   83.48%   +22.99%     
===========================================
  Files        1931      521     -1410     
  Lines       76236    37488    -38748     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31297    -14817     
+ Misses      28017     6191    -21826     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	49.09% <14.28%> (-0.08%)	⬇️
javascript	?
mysql	77.17% <100.00%> (?)
postgres	77.28% <100.00%> (?)
presto	53.64% <57.14%> (-0.16%)	⬇️
python	83.48% <100.00%> (+19.99%)	⬆️
sqlite	76.73% <100.00%> (?)
unit	58.93% <14.28%> (+1.31%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[john-bodley]

@jzhao62 for running tests locally try:

> python3.9 -m venv venv
> source venv/bin/activate
> tox -e py39 -- tests/unit_tests/<file>
> tox -e py39-sqlite -- tests/integration_tests/<file>

these instructions should be outlined in CONTRIBUTING.md. Also see here with regards to how to run specific test classes or methods.

[rusackas]

Hey all,

Just wondering if there's any hope of getting this across the finish line, as it would close both an open Discussion and an open Issue.

[jzhao62]

hey @rusackas, i somewhat get stuck in the testing phase, where i cannot pass the celery integration tests from machine, i will address the conflicts and continue work on this

[jzhao62]

@rusackas just wondering can someone kick start the test workflows, i highly suspect that the majority of testcases should work fine, i am seeking some help in the slack regarding at running test at meantime,

[jzhao62]

great, seems no regression is introduced, i will add unit test for coverage on these line and it is a go.

[rusackas]

@jzhao62 fantastic, thanks for following through on this! 🙌

[jzhao62]

@rusackas hi, i have added unit test for coverage and this PR should be ready to go. if anything else let me know

[john-bodley]

Why is this necessary? It seems like we're re-raising the previous error with the same error type.

[john-bodley]

I suspect this line is actually what is relevant, i.e., the changes to superset/connectors.sqla/utils.py are not necessary.