feat: Add validate_sql_json endpoint for checking that a given sql query is valid for the chosen database

[bearcage]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

This is the first step toward supporting live as-you-type
SQL validation against databases that support doing it (for
starters just presto).

I'm trying to stick as much to the existing query machinery
as possible to avoid introducing too much new state for handling
this, so I plumbed it through as a db spec parameter and an
option you can set on an individual query.

As of this commit:

• Plumb validation db_spec options
• Add validation_only option and transform to the sql_json view
• Tests for the above on the backend
• Debounced requestValidationQuery in onSqlChange
• Check incoming results -- full query vs validation
• Tests for the above on the frontend
• Highlight error lines
• Print error message in alert-danger box above toolbar controls

TEST PLAN

Tests incoming in another commit on this PR

ADDITIONAL INFORMATION

• Has associated issue: Realtime syntax validation of queries in SQL lab #6707
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

REVIEWERS

@xtinec @betodealmeida @khtruong

[xtinec]

@bearcage looks like there is a lint error we want to fix https://travis-ci.org/apache/incubator-superset/jobs/526725518

[DiggidyDave]

A couple of high-level points and suggestions for consideration:

1. I'm not a big database guy, but the term 'validation query' seems to have an established meaning and I fear this will be unnecessarily overloading a term and possibly causing confusion. Someone implementing a new BaseEngineSpec impl might ask "does my database support validation queries?" and find this comment on SO and do the wrong thing. Consider:

   • renaming it to something less ambiguous and more self-documenting
   • adding more comments to the base class documenting the member property and method

2. Tying this to a database's built in implementation is perhaps limiting. It seem unlikely that many (any?) new databases will supply this functionality, but if there was a configurable "per-dialect" or "per database-type" endpoint exposing an abstract API that we define, than lots of options could be plugged in there (including forwarding through to the provider). Consider:

   • removing the validation support from BaseEngineSpec and having a configurable sql validation handler map, which by default has one impl for Presto that forwards to the server. That way we reserve the right to implement additional handlers to make this feature available to other db types going forward. We might say But we can always add that later!, and that is true, but if we stub out the interfaces and make the pattern clear and self-documenting now, then the community might pick up the work to add verification handler types for other database types. The pattern we pick now is the pattern we are going to live with.

[john-bodley]

A couple of general comments:

1. Would you mind adding some unit tests?
2. Superset supports (and encourages) the use of typing (see CONTRIBUTING.md). Would you mind adding type hints to the new functions you added? There’s also a note about how to document exceptions (if still relevant).

[bearcage]

Initially I had this in a separate endpoint, but it seemed like a lot of duplication of effort to pull over all the other conditional modifications we do on queries before executing them (limit, ctas, and templating). Templating in particular was what initially convinced me to move this in to the query execution flow.

That said, I'm not at all used to OSS projects, so I hadn't considered the rhetorical angle. I'll split it back out.

Point well taken on the name. How about /validate_sql_json for the endpoint and "SQL Validation" as the name of the feature?

---

Happy to add tests and typing — I have some tests which I'd planned to stack on this commit. I'll squash them instead. Since this portion of the code isn't particularly liberal with types, do y'all prefer py3-only annotations, or comment annotations?

[john-bodley]

Please use Python 3 only type annotations as Superset only supports Python 3.6+.

[DiggidyDave]

it seemed like a lot of duplication of effort to pull over all the other conditional modifications we do on queries before executing them

I don't think it is much more effort... you can still use all of that. I think it would literally just become:

    @classmethod
    def make_validation_query(cls, sql):
        '''
        throws: Exception if no validator type is configured for engine
        '''
        return SQLValidator.get(cls.engine).validate(sql)

And that type would just do the last bit of prep that the method currently does, for that impl. The main thing here is this would move it to a configuration option and provide abstractions for impls, rather than make it an intrinsic property of database types.

Why am I pushing on this? LOL... I would love to see this be a feature that works well for all databases eventually, not just a patchwork or one-off if the customer has the right thing. A consistent, delightful experience that isn't tied to the db in use, or at least leaving that possibility open.

[bearcage]

Tagged [WIP] since I need to update unit tests to go with the review changes.

[bearcage]

@DiggidyDave @mistercrunch since you've both mentioned it at one point or another, I pulled validate out to a separate endpoint.

I think this is probably the right call in the long run since the more I think about it the less it makes sense to fuse validation in to the query path. For presto it's quite similar, but for any other validation scheme I can come up with it won't necessarily be. This also avoids conflating validation requests with query requests if you e.g. track 5xx response rate by endpoint.

---

I've also fixed up the unit tests and picked that commit in to this PR for review together.

---

One remaining question on the backend side of this change — what's the right way to expose, from backend-to-frontend, whether or not the frontend should bother trying to call validate_sql_json at all for a given selected db? In other words, how do I expose the db engine config map (or something derived from it) to the frontend so we don't spam the endpoint with calls that will always error out?

[codecov-io]

Codecov Report

Merging #7422 into lyft-release-sp8 will increase coverage by 0.13%.
The diff coverage is 88.09%.

@@                 Coverage Diff                 @@
##           lyft-release-sp8   #7422      +/-   ##
===================================================
+ Coverage             64.96%   65.1%   +0.13%     
===================================================
  Files                   430     433       +3     
  Lines                 21164   21290     +126     
  Branches               2342    2342              
===================================================
+ Hits                  13750   13861     +111     
- Misses                 7290    7305      +15     
  Partials                124     124

Impacted Files	Coverage Δ
superset/config.py	93.86% <100%> (+0.07%)	⬆️
superset/sql_validators/base.py	100% <100%> (ø)
superset/sql_validators/__init__.py	83.33% <83.33%> (ø)
superset/sql_validators/presto_db.py	84.5% <84.5%> (ø)
superset/views/core.py	72.95% <90.9%> (+0.41%)	⬆️

---

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 90eef51...b0d76a2. Read the comment docs.

[bearcage]

@DiggidyDave @mistercrunch @xtinec @john-bodley I think all the review comments are addressed here — does anybody have any other issues to raise?

[DiggidyDave]

👍