-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix: Unhandled exception Str Column Type #22147
Conversation
@villebro can you please check this? |
Codecov Report
@@ Coverage Diff @@
## master #22147 +/- ##
===========================================
+ Coverage 53.22% 67.00% +13.77%
===========================================
Files 1833 1833
Lines 69935 69932 -3
Branches 7571 7571
===========================================
+ Hits 37221 46855 +9634
+ Misses 30755 21118 -9637
Partials 1959 1959
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
superset/connectors/base/models.py
Outdated
( | ||
(hasattr(metric, "get") and metric.get("column")) or {} | ||
).get("column_name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 314 we're already calling a type guard making sure that the type is in fact AdhocMetric
. So it appears there may be something wrong either in the declaration of the AdhocMetric
type or the type guard. Can you post the full stack trace of the error you're seeing and see if we need to update either one of those instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@villebro sorry for the late reply. As you mentioned correctly, I missed the check for AdhocMetric
. The issue was not with metric
param, rather with column
. Added a check around that.
@villebro please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor improvement proposal to further simplify the code
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@villebro applied the requested changes. Please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix!
SUMMARY
The metric is a list of
Any
, so that may or may not have the attributeget
. This pull request adds a check for that and avoids the APIs using the same throwing internal server error.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
NA
TESTING INSTRUCTIONS
NA
ADDITIONAL INFORMATION