chore(sql_parse): Provide more meaningful SQLGlot errors

[john-bodley]

SUMMARY

#26767 replaced the non-validating sqlparse parser with the pseudo-validating SQLGlot parser to aid with SQL parsing. The main use case for SQL parsing—from a security perspective—is to identify/extract the underlying tables/views referenced in the statement and checking whether the user is authorized to access them.

Previously, given the non-validating nature of sqlparse, virtually all SQL statements could be parsed—irrespective of whether they were valid—which meant that any syntax errors were reported by the underlying engine in a pseudo-meaningful manner. Now however, given that SQLGlot is more opinionated, syntax errors are detected by SQLGlot during the table/view extraction phase meaning that the query is never run and thus the engine's parser is never run resulting in a non-actionable error, i.e., it merely stated that the SQL could not be parsed. This is especially problematic in SQL Lab where freeform SQL is the norm and can be rather lengthy.

The TL;DR is now SQL statements which contain a syntax error are likely going to be captured by SQLGlot upstream of being executed by the engine.

This PR proposes leveraging the messaging from SQLGlot's ParseError or TokenError (example) to provide a more meaningful (and thus actionable) error message to the user.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

PREVIOUS

Prior to #26767 sqlparse was able to "parse" the SQL statement—missing an AND after the key = 'foobar' clause—which was then executed by the Trino engine (given that the user had access to the datainfra.one_row table) resulting in the following Trino error,

which detected the error was around the 1 token.

CURRENT

After #26767 SQLGlot was (rightfully) unable to parse the SQL statement and thus could not extract the underlying tables which halted the execution flow. The resulting error message isn't overly helpful as it doesn't inform the user where the actual syntax error is.

PROPOSED

Leveraging SQLGlot's ParseError et al. exceptions to provide a more meaningful error message,

which correctly detected the error was around the 1 token. The messaging is loosely based on how MySQL presents errors.

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[john-bodley]

I included the work may as SQLGlot may result in false positives whilst parsing. See here as an example.

[john-bodley]

Simply doing str(ex) doesn't work as SQLGlot includes ANSI codes which are helpful when printing in the terminal, but simply render as [4m and [0m in HTML.

[mistercrunch]

It feels to me that the SQL IDE should really just pass the SQL to the underlying connection and return the results or error message in most cases, and not get in the way of things. Now I'm wondering if _extract_tables_from_sql needs to run at all times or only if there are schema/tables restrictions in place (?)

[john-bodley]

@mistercrunch that was the other option I considered and had discussed internally, i.e., the error being somewhat context aware. Rather than SQLGlot's error being wrapped in a SupersetSecurityException it would simply bubble up and be handled in a context aware case-by-case basis.

Thus in the context of SQL Lab—which doesn't leverage a cache and where institutions may have a third party security manager, e.g. Apache Ranger—then allowing the SqlglotError error to pass through to the engine (irrespective of if the user has access to the underlying tables/views) is likely rather safe—especially given that the engine's parser will most likely detect the error.

The challenge arrises when:

1. There is no underlying authorization checks at the engine level.
2. SQLGlot returns a false positive (example) which can be exploited by an actor if (1) is true.

Furthermore the current implement of raise_for_access() isn't context aware. Given the enumerated challenges it might be best for individual institutions to augment their security manager as they see fit. For example at Airbnb if we feel that either,

1. SQLGlot has a non-acceptable false positive rate, or
2. SQLGlot's error messaging is too terse (when compared with the underlying engine)

then we may augment our security manager to simply not raise when the raise_for_access() method results in a SqlglotError when invoked from SQL Lab (leveraging either the referrer or call stack) as we have the peace of mind that Apache Ranger will prevent unauthorized access.

[betodealmeida]

Now I'm wondering if _extract_tables_from_sql needs to run at all times or only if there are schema/tables restrictions in place.

That's a good idea! Part of the problem today is that _extract_tables_from_sql is called by simply accessing the .tables property in the parsed query, we could make it more explicit so it's only called whenever really needed and not by mistake.

[betodealmeida]

❤️

[michael-s-molina]

Wow! That's what I call a well-defined PR description 👏🏼

Thanks for the improvement and explanations.