More testing for SQL trickery #109
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @crisptrutski and the rest of your teammates on |
| @@ -11,15 +11,6 @@ | |||
|
|
|||
| (set! *warn-on-reflection* true) | |||
|
|
|||
| (def broken-queries | |||
There was a problem hiding this comment.
Since errors are values now, we don't need this.
| "The fixtures in merged fixtures file, mapped by their identifiers." | ||
| [] | ||
| (->> (str/split (slurp merged-fixtures-file) #"-- FIXTURE: ") | ||
| ;; TODO generically detect queries.<namespace>.sql files |
There was a problem hiding this comment.
Waiting for a 3rd file before bothering.
| @@ -0,0 +1,5 @@ | |||
| SELECT 1 | |||
There was a problem hiding this comment.
The "implicit semicolons" tests are unrelated.
Smuggling them in before the 5.0 upgrade, which will affect how this is handled.
| EXECUTE stmt('table_name'); | ||
|
|
||
| -- FIXTURE: variable | ||
| -- BROKEN |
There was a problem hiding this comment.
From the permissions perspective, broken is still fine.
| CALL user_function('table_name'); | ||
|
|
||
| -- FIXTURE: select-function | ||
| SELECT user_function('table_name'); |
There was a problem hiding this comment.
This is the case we don't handle, and is a potential blocker.
On the other hand, we do block the ability to create such a malicious (or just naive) function via Metabase.
Since we can't anticipate all user functions, we may need an allow list on expressions.
There was a problem hiding this comment.
You mean you can only escape Metabase permissions if somebody with better access had prepared some function for you? That makes sense, but also maybe we really want to be able to configure a whitelist for allowed functions?
| (or (get global-overrides mode) | ||
| (get-in ns-overrides [mode (namespace fixture)]) | ||
| (get-in expected-cs [:overrides mode :error]) | ||
| (or (get-in expected-cs [:overrides mode :error]) | ||
| (get-in expected-cs [:overrides mode ck]) | ||
| (when-keyword (get-in expected-cs [:overrides mode])))) | ||
| (when-keyword (get-in expected-cs [:overrides mode])) | ||
| (get-in ns-overrides [mode (namespace fixture)]) | ||
| (get global-overrides mode))) |
There was a problem hiding this comment.
I had a bug in my overrides, and at first thought it was precedence. I think with this change its more intuitive anyway though.
| (catch AnalysisError e | ||
| (->macaw-error e)) | ||
| (catch JSQLParserException e |
There was a problem hiding this comment.
This moved into parsed-query
| (catch AnalysisError e | ||
| (->macaw-error e)) | ||
| (catch JSQLParserException e |
There was a problem hiding this comment.
This was not working, as the exception is thrown while building the argument for this function.
| @@ -1030,32 +1026,32 @@ public void visit(Alter alter) { | |||
|
|
|||
| @Override | |||
| public void visit(Statements stmts) { | |||
| throw new UnsupportedOperationException(NOT_SUPPORTED_YET); | |||
| throw new AnalysisError(AnalysisErrorType.INVALID_QUERY); | |||
There was a problem hiding this comment.
We don't have any plans to support statements AFAIK, can update this if that changes.
crisptrutski
changed the title
Merge activity
|
This PR improves the ergonomics for adding queries which all have the same error, and uses that to add a bunch more "skullduggery detected" fixtures.
Unfortunately I realize that a case has slipped through, and the fix is non-trivial. Needs further discussion.
Also fixed an issue where the "all the components" function was not actually catching JSQLParser exceptions.