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.

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

[payload] Set status code on error rather than query status #4567

Merged

Conversation

john-bodley
Copy link
Member

@mistercrunch previously you used to raise an exception if there was no-data, which would then get caught here and thus the non-200 status code would ensure that the error message was reported.

When I removed the exception, and instead set the error message (to ensure consistency with how other query errors were handled) the exception wasn't raised, and the response status code was defined purely on whether the query failed or not rather than also checking the presence of an error.

Note in the future I'll try to take a pass at unifying some of this logic; standardizing payloads, determining when exceptions should/shouldn't be thrown, etc.

@john-bodley john-bodley force-pushed the john-bodley-payload-error-status-code branch from d4168e0 to 636ec05 Compare March 7, 2018 19:52
@michellethomas
Copy link
Contributor

lgtm

@john-bodley john-bodley force-pushed the john-bodley-payload-error-status-code branch from 636ec05 to 1e8cd0e Compare March 7, 2018 21:26
@codecov-io
Copy link

codecov-io commented Mar 7, 2018

Codecov Report

Merging #4567 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4567   +/-   ##
======================================
  Coverage    71.2%   71.2%           
======================================
  Files         187     187           
  Lines       14785   14785           
  Branches     1083    1083           
======================================
  Hits        10527   10527           
  Misses       4255    4255           
  Partials        3       3
Impacted Files Coverage Δ
superset/views/core.py 71.15% <50%> (ø) ⬆️

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 369f652...1e8cd0e. Read the comment docs.

@john-bodley john-bodley merged commit 826d063 into apache:master Mar 7, 2018
@john-bodley john-bodley deleted the john-bodley-payload-error-status-code branch March 7, 2018 21:58
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…rror-status-code

[payload] Set status code on error rather than query status
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants