More testing for SQL trickery (#109)

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.

deps.edn

@@ -20,9 +20,10 @@
  :aliases
  {:dev
   {:extra-deps
-   {io.github.metabase/hawk     {:mvn/version "1.0.0"}
-    virgil/virgil               {:mvn/version "0.3.0"}
-    org.clojure/tools.namespace {:mvn/version "1.4.5"}}
+   {hashp/hashp                 {:mvn/version "0.2.2"}
+    io.github.metabase/hawk     {:mvn/version "1.0.0"}
+    org.clojure/tools.namespace {:mvn/version "1.4.5"}
+    virgil/virgil               {:mvn/version "0.3.0"}}
 
    :extra-paths
    ["test" "test/resources"]

java/com/metabase/macaw/AstWalker.java

@@ -5,8 +5,6 @@
 import clojure.lang.Cons;
 import clojure.lang.IFn;
 
-import java.util.ArrayDeque;
-import java.util.Deque;
 import java.util.EnumMap;
 import java.util.List;
 import java.util.Map;
@@ -249,8 +247,6 @@ public String getValue() {
         }
     }
 
-    private static final String NOT_SUPPORTED_YET = "Not supported yet.";
-
     private Acc acc;
     private final EnumMap<CallbackKey, IFn> callbacks;
     private ISeq contextStack = PersistentList.EMPTY;
@@ -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);
     }
 
     @Override
     public void visit(Execute execute) {
-        throw new UnsupportedOperationException(NOT_SUPPORTED_YET);
+        throw new AnalysisError(AnalysisErrorType.INVALID_QUERY);
     }
 
     @Override
     public void visit(SetStatement set) {
-        throw new UnsupportedOperationException(NOT_SUPPORTED_YET);
+        throw new AnalysisError(AnalysisErrorType.INVALID_QUERY);
     }
 
     @Override
     public void visit(ResetStatement reset) {
-        throw new UnsupportedOperationException(NOT_SUPPORTED_YET);
+        throw new AnalysisError(AnalysisErrorType.INVALID_QUERY);
     }
 
     @Override
     public void visit(ShowColumnsStatement set) {
-        throw new UnsupportedOperationException(NOT_SUPPORTED_YET);
+        throw new AnalysisError(AnalysisErrorType.INVALID_QUERY);
     }
 
     @Override
     public void visit(ShowIndexStatement showIndex) {
-        throw new UnsupportedOperationException(NOT_SUPPORTED_YET);
+        throw new AnalysisError(AnalysisErrorType.INVALID_QUERY);
     }
 
     @Override

src/macaw/core.clj

@@ -52,10 +52,14 @@
   ;; For Metabase, we are always dealing with single queries, so there's no point ever having this behavior.
   ;; TODO When JSQLParser 4.10 is released, move to the more robust [[CCJSqlParserUtil.sanitizeSingleSql]] helper.
   ;; See https://github.com/JSQLParser/JSqlParser/issues/1988
-  (-> query
-      (str/replace #"\n{2,}" "\n")
-      (escape-keywords (:non-reserved-words opts))
-      (CCJSqlParserUtil/parse (->parser-fn opts))))
+  (try
+    (-> query
+        (str/replace #"\n{2,}" "\n")
+        (escape-keywords (:non-reserved-words opts))
+        (CCJSqlParserUtil/parse (->parser-fn opts)))
+    (catch JSQLParserException e
+      {:error   :macaw.error/unable-to-parse
+       :context {:cause e}})))
 
 (defn scope-id
   "A unique identifier for the given scope."
@@ -77,21 +81,20 @@
 
   (Specifically, it returns their fully-qualified names as strings, where 'fully-qualified' means 'as referred to in
   the query'; this function doesn't do additional inference work to find out a table's schema.)"
-  [statement & {:as opts}]
+  [parsed & {:as opts}]
   ;; By default, we will preserve identifiers verbatim, to be agnostic of case and quote behavior.
   ;; This may result in duplicate components, which are left to the caller to deduplicate.
   ;; In Metabase's case, this is done during the stage where the database metadata is queried.
   (try
-    (->> (collect/query->components statement (merge {:preserve-identifiers? true} opts))
-         (walk/postwalk (fn [x]
-                          (if (string? x)
-                            (unescape-keywords x (:non-reserved-words opts))
-                            x))))
+    (if (map? parsed)
+      parsed
+      (->> (collect/query->components parsed (merge {:preserve-identifiers? true} opts))
+           (walk/postwalk (fn [x]
+                            (if (string? x)
+                              (unescape-keywords x (:non-reserved-words opts))
+                              x)))))
     (catch AnalysisError e
-      (->macaw-error e))
-    (catch JSQLParserException e
-      {:error   :macaw.error/unable-to-parse
-       :context {:cause e}})))
+      (->macaw-error e))))
 
 (defn- raw-components [xs]
   (into (empty xs) (keep :component) xs))
@@ -111,16 +114,15 @@
   "Given a parsed query (i.e., a [subclass of] `Statement`) return a set of all the table identifiers found within it."
   [sql & {:keys [mode] :as opts}]
   (try
-    (let [query (parsed-query sql opts)]
-      (case mode
-        :ast-walker-1    (-> (query->components query opts) :tables raw-components (->> (hash-map :tables)))
-        :basic-select    (-> (BasicTableExtractor/getTables query) tables->identifiers)
-        :compound-select (-> (CompoundTableExtractor/getTables query) tables->identifiers)))
+    (let [parsed (parsed-query sql opts)]
+      (if (map? parsed)
+        parsed
+        (case mode
+          :ast-walker-1 (-> (query->components parsed opts) :tables raw-components (->> (hash-map :tables)))
+          :basic-select (-> (BasicTableExtractor/getTables parsed) tables->identifiers)
+          :compound-select (-> (CompoundTableExtractor/getTables parsed) tables->identifiers))))
     (catch AnalysisError e
-      (->macaw-error e))
-    (catch JSQLParserException e
-      {:error   :macaw.error/unable-to-parse
-       :context {:cause e}})))
+      (->macaw-error e))))
 
 (defn replace-names
   "Given an SQL query, apply the given table, column, and schema renames.

test/macaw/acceptance_test.clj

@@ -11,15 +11,6 @@
 
 (set! *warn-on-reflection* true)
 
-(def broken-queries
-  "The DANGER ZONE
-  This map gives a pattern in the exception message we expect to receive when trying to analyze the given fixture."
-  {:broken/between       #"Encountered unexpected token: \"BETWEEN\""
-   :broken/filter-where  #"Encountered unexpected token: \"\(\""
-   :sqlserver/execute    #"Not supported yet"
-   :sqlserver/executesql #"Not supported yet"
-   :oracle/open-for      #"Encountered unexpected token: \"OPEN\""})
-
 (defn- fixture-analysis [fixture]
   (some-> fixture (ct/fixture->filename "acceptance" ".analysis.edn") io/resource slurp read-string))
 
@@ -60,16 +51,25 @@
 
 (def ^:private merged-fixtures-file "test/resources/acceptance/queries.sql")
 
-(defn- merged-fixtures
-  "The fixtures in merged fixtures file, mapped by their identifiers."
-  []
-  (->> (str/split (slurp merged-fixtures-file) #"-- FIXTURE: ")
+;; TODO generically detect queries.<namespace>.sql files
+(def ^:private dynamic-fixtures-file "test/resources/acceptance/queries.dynamic.sql")
+
+(defn- read-merged [file]
+  (->> (str/split (slurp file) #"-- FIXTURE: ")
        (keep (fn [named-query]
                (when-not (str/blank? named-query)
                  (let [[nm qry] (.split ^String named-query "\n" 2)]
                    [(keyword nm) (str/trim qry)]))))
        (into {})))
 
+(defn- merged-fixtures
+  "The fixtures in merged fixtures file, mapped by their identifiers."
+  []
+  (merge (read-merged merged-fixtures-file)
+         (update-keys
+          (read-merged dynamic-fixtures-file)
+          #(keyword "dynamic" (name %)))))
+
 (defn- validate-analysis [correct override actual]
   (let [expected (or override correct)]
     (when override
@@ -88,11 +88,11 @@
     x))
 
 (defn- get-override* [expected-cs mode fixture ck]
-  (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)))
 
 (defn- get-override [expected-cs mode fixture ck]
   (or
@@ -119,19 +119,18 @@
             :let [opts (opts-mode m)]]
       (if (= m :ast-walker-1)
         ;; Legacy testing path for `components`, which only supports the original walker, and throws exceptions.
-        (if-let [expected-msg (broken-queries fixture)]
-          (testing (str prefix " analysis cannot be parsed")
-            (is (thrown-with-msg? Exception expected-msg (ct/components sql opts))))
-          (let [cs (testing (str prefix " analysis does not throw")
-                     (is (ct/components sql opts)))]
-            (doseq [[ck cv] (dissoc expected-cs :overrides :error :skip)]
-              (testing (str prefix " analysis is correct: " (name ck))
-                (let [actual-cv (get-component cs ck)
-                      override  (get-override expected-cs m fixture ck)]
-                  (validate-analysis cv override actual-cv))))))
+        (let [cs (testing (str prefix " analysis does not throw")
+                   (is (ct/components sql opts)))]
+          (doseq [[ck cv] (dissoc expected-cs :overrides :error :skip)]
+            (testing (str prefix " analysis is correct: " (name ck))
+              (let [actual-cv (:error cs (get-component cs ck))
+                    override  (get-override expected-cs m fixture ck)]
+                (validate-analysis cv override actual-cv)))))
         ;; Testing path for newer modes.
         (let [correct  (:error expected-cs (:tables expected-cs))
-              override (get-override expected-cs m fixture :tables)
+              override (if (str/includes? sql "-- BROKEN")
+                         :macaw.error/unable-to-parse
+                         (get-override expected-cs m fixture :tables))
               ;; For now, we only support (and test) :tables
               tables   (testing (str prefix " table analysis does not throw for mode " m)
                          (is (ct/tables sql opts)))]

test/resources/acceptance/dynamic__select_function.analysis.edn

@@ -0,0 +1,4 @@
+{:error :macaw.error/illegal-expression
+ :overrides
+ {:select-only
+  {:tables #{}}}}

test/resources/acceptance/misc__implicit_semicolons.analysis.edn

@@ -0,0 +1,2 @@
+{:source-columns []
+ :tables         [{:table "t"}]}

test/resources/acceptance/misc__implicit_semicolons.renames.edn

@@ -0,0 +1,2 @@
+{:tables {{:table "t"} "s"}
+ :columns {}}

test/resources/acceptance/misc__implicit_semicolons.rewritten.sql

@@ -0,0 +1,5 @@
+SELECT 1
+-- NOTE: don't remove the number of blank lines here, they're significant
+
+
+FROM s

test/resources/acceptance/misc__implicit_semicolons.sql

@@ -0,0 +1,5 @@
+SELECT 1
+-- NOTE: don't remove the number of blank lines here, they're significant
+
+
+FROM t

test/resources/acceptance/queries.dynamic.sql

@@ -0,0 +1,23 @@
+-- FIXTURE: generate-series
+SELECT t.day::date AS date
+FROM generate_series(timestamp '2021-01-01', now(), interval '1 day') AS t(day)
+
+-- FIXTURE: format
+SELECT * FROM format('%I', table_name_variable);
+
+-- FIXTURE: prepared-stmt
+EXECUTE stmt('table_name');
+
+-- FIXTURE: variable
+-- BROKEN
+EXECUTE 'SELECT * FROM ' || table_name;
+
+-- FIXTURE: call-function
+CALL user_function('table_name');
+
+-- FIXTURE: select-function
+SELECT user_function('table_name');
+
+-- FIXTURE: cursor
+-- BROKEN
+FETCH ALL FROM my_cursor;

test/resources/acceptance/queries.sql

@@ -236,10 +236,6 @@ SELECT
     c.x
 FROM b, c;
 
--- FIXTURE: dynamic/generate-series
-SELECT t.day::date AS date
-FROM generate_series(timestamp '2021-01-01', now(), interval '1 day') AS t(day)
-
 -- FIXTURE: literal/with-table
 SELECT FALSE, 'str', 1, x FROM t
 

test/resources/acceptance/sqlserver__execute.analysis.edn

@@ -1,2 +1 @@
-{:error     :macaw.error/invalid-query
- :overrides {:ast-walker-1 :macaw.error/unable-to-parse}}
+{:error     :macaw.error/invalid-query}

test/resources/acceptance/sqlserver__executesql.analysis.edn

@@ -1,2 +1 @@
-{:error     :macaw.error/invalid-query
- :overrides {:ast-walker-1 :macaw.error/unable-to-parse}}
+{:error     :macaw.error/invalid-query}