Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: setting specific exceptions common/query_context.py #10942

Merged
merged 2 commits into from
Sep 21, 2020
Merged

fix: setting specific exceptions common/query_context.py #10942

merged 2 commits into from
Sep 21, 2020

Conversation

kkucharc
Copy link
Contributor

SUMMARY

Exceptions which may appear in get_df_payload() method may be more precise:

  • suggested KeyError as specific Exception which can be thrown
  • removed disabled pylint check: broad-except

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@kkucharc kkucharc changed the title fix: setting specific exceptions commands/query_context.py fix: setting specific exceptions common/query_context.py Sep 17, 2020
willbarrett
willbarrett requested changes Sep 17, 2020
View reviewed changes
superset/common/query_context.py Outdated
@@ -265,7 +265,7 @@ def get_df_payload( # pylint: disable=too-many-locals,too-many-statements
except QueryObjectValidationError as ex:
error_message = str(ex)
status = utils.QueryStatus.FAILED
except Exception as ex: # pylint: disable=broad-except
except KeyError as ex:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot that can go wrong here, as the try wraps the get_query_result method - I'd recommend leaving this one at the broad exception. Errors are intentionally swallowed and reported for this chunk of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no problem - I'll bring it back.

superset/common/query_context.py
@@ -228,7 +228,7 @@ def get_df_payload( # pylint: disable=too-many-locals,too-many-statements
status = utils.QueryStatus.SUCCESS
is_loaded = True
stats_logger.incr("loaded_from_cache")
except Exception as ex: # pylint: disable=broad-except
except KeyError as ex:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@mistercrunch
Copy link
Member

I think broad-except is totally ok sometimes. A clear example is with external database calls when we support multiple engines. There's almost no way to know ahead of time what any given driver will raise. DBAPI isn't prescriptive enough to enable that, and even if it were implementations may diverge from the the prescribed way.

Personally I'd recommend adding broad-except to the global allow-list, at list for now.

If really we wanted to push this, we could make sure to have consistent logging on broad exceptions and dig into logs to find what's actually been raised and catch the right thing and provide a better error message. But I think there's much bigger fish to catch at this time!

@kkucharc
Copy link
Contributor Author

@mistercrunch I totally agree with having broad-except, especially when we have as you said various integrations. My suggestions in the PR comes from my intuition - I can imagine there can be some RuntimeExceptions or even something more complicated memory issues, which can be neglected by broad-exception.

If I may share my opinion about adding broad-except to global disabled rules, I would disagree, because of two reasons:

  1. probably if project will achieve big amount of broad exceptions it will be difficult and painful to come back,
  2. I'm afraid of over-use broad exception, because it's much easier to react to any bad thing that may happen than think deeper how to address each error one by one. I guess also it may extend reviewing process.
    But of course you know wider context of the subject and requirements, so I am totally open to adding global broad-except and/or for further discussion 👍

Also if it comes to the current PR I am open to closing this PR if it may cause more harm than good. I fully agree that it might be worth to take care about all broad exceptions to get the most of them.

@kkucharc kkucharc force-pushed the fix/pylint-commands-exceptions branch from 807a222 to c406953 Compare September 21, 2020 08:38
@willbarrett willbarrett merged commit f01c488 into apache:master Sep 21, 2020
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 22, 2020
Merge remote-tracking branch 'upstream/master' into feature/dash_leve…
1fa663e 
…l_access/dashboard_by_id_endpoints

* upstream/master: (29 commits)
  fix(presto): default unknown types to string type (apache#10753)
  feat(row-level-security): add base filter type and filter grouping (apache#10946)
  docs: add gallery screenshot & link in README (apache#10988)
  docs: add a "Gallery" page (apache#10968)
  build: add PR lint action (apache#10990)
  adding filters back that caused issues (apache#10989)
  chore: selectors refactor in SQLLab test suite (Cypress) (apache#10944)
  ESLint: Remove ts-ignore comments (apache#10933)
  style: fix checkbox color (apache#10970)
  fix: changed disabled rules in datasets module (apache#10979)
  fix: update the time filter for 'Last Year' option in explore (apache#10829)
  fix: use nullpool even for user lookup in the celery (apache#10938)
  Allow empty observations in alerting (apache#10939)
  fix: re-enabling several globally disabled lint rules (apache#10957)
  fix: setting specific exceptions common/query_context.py (apache#10942)
  Pylint disabled rule `pointless-string-statement` is not raising warining anymore - removing (apache#10975)
  fix: pylint disabled rules in dashboard/api.py (apache#10976)
  fix: removed disabled lint rule `too-many-locals` in connectors/base/models.py (apache#10958)
  ESLint: Re-enable rule no-access-state-in-setstate (apache#10870)
  fix: simply is_adhoc_metric (apache#10964)
  ...
@kkucharc kkucharc deleted the fix/pylint-commands-exceptions branch September 25, 2020 07:30
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@kkucharc @auxten
fix: setting specific exceptions common/query_context.py (apache#10942)
ddcfa20 
* Specified an exceptions in reading cache in `get_df_payload()` method

* Reverted change after review:
- added broad exception in `get_df_payload` method
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willbarrett willbarrett willbarrett approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.38.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kkucharc @mistercrunch @willbarrett