-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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: accept null params for validation #17788
fix: accept null params for validation #17788
Conversation
05d93f4
to
f0ebed9
Compare
🏷️ 2021.49 |
/testenv up |
@eschutho Container image not yet published for this PR. Please try again when build is complete. |
@eschutho Ephemeral environment creation failed. Please check the Actions logs for details. |
Codecov Report
@@ Coverage Diff @@
## master #17788 +/- ##
==========================================
+ Coverage 68.85% 68.99% +0.14%
==========================================
Files 1597 1597
Lines 65251 65357 +106
Branches 6950 6950
==========================================
+ Hits 44927 45093 +166
+ Misses 18439 18379 -60
Partials 1885 1885
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.190.52.174:8080. Credentials are |
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.
I'm not super familiar with this section of the code, but from what I see this looks good to me. Nice fix with good tests to check if it happens again. I tried to test this in the test env but I don't seem to have access to the template params here (see screenshot). This could be something I'm missing in my local setup, so if I can do something to be able to test this I'd be happy to run through it 😁
@@ -2395,7 +2395,7 @@ def validate_sql_json( | |||
schema = request.form.get("schema") or None | |||
template_params = json.loads(request.form.get("templateParams") or "{}") | |||
|
|||
if len(template_params) > 0: | |||
if template_params is not None and len(template_params) > 0: |
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.
Nit, since in Python an empty list/dict is false you can simplify to:
if template_params is not None and len(template_params) > 0: | |
if template_params: |
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.
thanks! Sorry, I just saw this note after I merged. I can clean it up separately.
/testenv up FEATURE_ALERT_REPORTS=true |
@lyndsiWilliams Ephemeral environment creation is currently limited to committers. |
@AAfghahi Ephemeral environment spinning up at http://54.149.252.142:8080. Credentials are |
/testenv up FEATURE_ENABLE_TEMPLATE_PROCESSING=True |
@AAfghahi Ephemeral environment spinning up at http://54.186.195.49:8080. Credentials are |
Thanks for the help with the test env! Everything works as described, I am more confident in my approval 👍 |
SUMMARY
If a null value is passed into the template Params for sql lab, the validation blows up.
json.loads("null") returns None.
TESTING INSTRUCTIONS
Type
null
for a template param and type in the sql lab editor and watch the network requests. Thevalidate_sql_json
requests should not be 500s.ADDITIONAL INFORMATION