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

fix: accept null params for validation #17788

Merged
merged 1 commit into from
Dec 17, 2021
Merged

fix: accept null params for validation #17788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

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:

Suggested change
if template_params is not None and len(template_params) > 0:
if template_params:

Copy link
Member Author

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.

# TODO: factor the Database object out of template rendering
# or provide it as mydb so we can render template params
# without having to also persist a Query ORM object.
Expand Down
8 changes: 7 additions & 1 deletion tests/integration_tests/base_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ def validate_sql(
user_name=None,
raise_on_error=False,
database_name="examples",
template_params=None,
):
if user_name:
self.logout()
Expand All @@ -466,7 +467,12 @@ def validate_sql(
resp = self.get_json_resp(
"/superset/validate_sql_json/",
raise_on_error=False,
data=dict(database_id=dbid, sql=sql, client_id=client_id),
data=dict(
database_id=dbid,
sql=sql,
client_id=client_id,
templateParams=template_params,
),
)
if raise_on_error and "error" in resp:
raise Exception("validate_sql failed")
Expand Down
31 changes: 31 additions & 0 deletions tests/integration_tests/sql_validator_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,37 @@ def test_validate_sql_endpoint_mocked(self, get_validator_by_name):
self.assertEqual(1, len(resp))
self.assertIn("expected,", resp[0]["message"])

@patch("superset.views.core.get_validator_by_name")
@patch.dict(
"superset.config.SQL_VALIDATORS_BY_ENGINE",
PRESTO_SQL_VALIDATORS_BY_ENGINE,
clear=True,
)
def test_validate_sql_endpoint_mocked_params(self, get_validator_by_name):
"""Assert that, with a mocked validator, annotations make it back out
from the validate_sql_json endpoint as a list of json dictionaries"""
if get_example_database().backend == "hive":
pytest.skip("Hive validator is not implemented")
self.login("admin")

validator = MagicMock()
get_validator_by_name.return_value = validator
validator.validate.return_value = [
SQLValidationAnnotation(
message="This worked", line_number=4, start_column=12, end_column=42,
)
]

resp = self.validate_sql(
"SELECT * FROM somewhere_over_the_rainbow",
client_id="1",
raise_on_error=False,
template_params="null",
)

self.assertEqual(1, len(resp))
self.assertNotIn("error,", resp[0]["message"])

@patch("superset.views.core.get_validator_by_name")
@patch.dict(
"superset.config.SQL_VALIDATORS_BY_ENGINE",
Expand Down