More acceptance tests relevant to table permissions (#101)

* More acceptance tests relevant to table permissions

* lil tweakz

* backfill expectations for currently broken stuff

* More data driven

* support raw errors

* Better comments

* Fixups

* Kondo

src/macaw/core.clj

@@ -85,9 +85,12 @@
 
 (defn query->tables
   "Given a parsed query (i.e., a [subclass of] `Statement`) return a set of all the table identifiers found within it."
-  [statement & {:keys [mode] :as opts}]
+  [sql & {:keys [mode] :as opts}]
   (case mode
-    :ast-walker-1 (raw-components (:tables (query->components statement opts)))
+    :ast-walker-1 (-> (parsed-query sql)
+                      (query->components opts)
+                      :tables
+                      raw-components)
     :basic-select :macaw.error/not-implemented))
 
 (defn replace-names

test/macaw/acceptance_test.clj

@@ -13,8 +13,11 @@
 (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: \"\(\""})
+  {: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))
@@ -35,23 +38,34 @@
   #{:ast-walker-1
     :basic-select})
 
-(def ^:private not-implemented?
-  #{:basic-select})
+(def global-overrides
+  {:basic-select :macaw.error/not-implemented})
 
 (defn- validate-analysis [correct override actual]
   (let [expected (or override correct)]
     (when override
-      (if (vector? correct)
-        (is (not= correct (ct/sorted actual)) "Override is still needed")
-        (is (not= correct actual) "Override is still needed")))
+      (testing "Override is still needed"
+        (if (and (vector? correct) (not (keyword actual)))
+          (is (not= correct (ct/sorted actual)))
+          (is (not= correct actual)))))
 
-    (if (vector? expected)
+    (if (and (vector? expected) (not (keyword actual)))
       (is (= expected (ct/sorted actual)))
-      (is (= expected actual)))))
+      (when (not= expected actual)
+        (is (= expected actual))))))
+
+(defn- when-keyword [x]
+  (when (keyword? x)
+    x))
 
 (defn- get-override [expected-cs mode ck]
-  (or (get-in expected-cs [:overrides mode ck])
-      (get-in expected-cs [:overrides ck])))
+  (or (get global-overrides mode)
+      (get-in expected-cs [:overrides mode :error])
+      (get-in expected-cs [:overrides :error])
+      (get-in expected-cs [:overrides mode ck])
+      (get-in expected-cs [:overrides ck])
+      (when-keyword (get-in expected-cs [:overrides mode]))
+      (when-keyword (get expected-cs :overrides))))
 
 (defn- test-fixture
   "Test that we can parse a given fixture, and compare against expected analysis and rewrites, where they are defined."
@@ -63,33 +77,29 @@
         expected-rw (fixture-rewritten fixture)
         base-opts   {:non-reserved-words [:final]}
         opts-mode   (fn [mode] (assoc base-opts :mode mode))]
-
-    (if-let [expected-msg (broken-queries fixture)]
-      (testing (str prefix " analysis cannot be parsed")
-        (is (thrown-with-msg? Exception expected-msg (ct/components sql base-opts)))
-        (doseq [m test-modes]
-          (is (thrown-with-msg? Exception expected-msg (ct/tables sql (opts-mode m))))))
-      (do
-        (let [m    :ast-walker-1
-              opts (opts-mode m)]
-          (when-let [cs (testing (str prefix " analysis does not throw")
-                          (is (ct/components sql opts)))]
-            (doseq [[ck cv] (dissoc expected-cs :overrides)]
+    (assert sql "Fixture exists")
+    (doseq [m test-modes
+            :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)]
               (testing (str prefix " analysis is correct: " (name ck))
                 (let [actual-cv (get-component cs ck)
                       override  (get-override expected-cs m ck)]
                   (validate-analysis cv override actual-cv))))))
-
-        (doseq [m test-modes]
-          (when-let [ts (testing (str prefix " table analysis does not throw for mode " m)
-                          (is (ct/tables sql (opts-mode m))))]
-            (if (not-implemented? m)
-              (testing (str m " is not implemented yet")
-                (is (= :macaw.error/not-implemented ts)))
-              (when-let [correct (get expected-cs :tables)]
-                (testing (str prefix " table analysis is correct for mode " m)
-                  (let [override (get-override expected-cs m :tables)]
-                    (validate-analysis correct override ts)))))))))
+        ;; Testing path for newer modes.
+        (let [correct  (:error expected-cs (:tables expected-cs))
+              override (get-override expected-cs m :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)))]
+          (testing (str prefix " table analysis is correct for mode " m)
+            (validate-analysis correct override tables)))))
 
     (when renames
       (let [broken?   (:broken? renames)
@@ -128,14 +138,14 @@
 (create-fixture-tests!)
 
 (comment
- ;; Unload all the tests, useful for flushing stale fixture tests
- (doseq [[sym ns-var] (ns-interns *ns*)]
-   (when (:test (meta ns-var))
-     (ns-unmap *ns* sym)))
+  ;; Unload all the tests, useful for flushing stale fixture tests
+  (doseq [[sym ns-var] (ns-interns *ns*)]
+    (when (:test (meta ns-var))
+      (ns-unmap *ns* sym)))
 
- (test-fixture :compound/cte)
- (test-fixture :compound/cte-nonambiguous)
- (test-fixture :literal/with-table)
- (test-fixture :literal/without-table)
+  (test-fixture :compound/cte)
+  (test-fixture :compound/cte-nonambiguous)
+  (test-fixture :literal/with-table)
+  (test-fixture :literal/without-table)
 
- (test-fixture :broken/filter-where))
+  (test-fixture :broken/filter-where))

test/macaw/core_test.clj

@@ -24,7 +24,7 @@
 
 (defn tables [sql & {:as opts}]
   (let [opts (update opts :mode #(or % :ast-walker-1))]
-    (m/query->tables (m/parsed-query sql opts) opts)))
+    (m/query->tables sql opts)))
 
 (def raw-components #(let [xs (empty %)] (into xs (keep :component) %)))
 (def columns        (comp raw-components :columns components))

test/resources/acceptance/bigquery__table_wildcard.analysis.edn

@@ -0,0 +1,12 @@
+{:source-columns []
+ :tables         ::not-sure-what-we-should-do-if-we-continue-supporting-this
+
+ :overrides
+ {:basic-select
+  ;; do not allow wildcard selects
+  {:error :macaw.error/illegal-expression}
+
+  ;; Just plain old wacky
+  :source-columns [{:table "`project_id.dataset_id.table_*`", :column "_TABLE_SUFFIX"}]
+  ;; Kinda makes sense, but very raw, and Metabase won't handle it.
+  :tables [{:table "`project_id.dataset_id.table_*`"}]}}

test/resources/acceptance/bigquery__table_wildcard.sql

@@ -0,0 +1,7 @@
+SELECT
+  *
+FROM
+  `project_id.dataset_id.table_*`
+WHERE
+  _TABLE_SUFFIX BETWEEN '20230101' AND '20230131'
+LIMIT 1000;

test/resources/acceptance/broken__between.analysis.edn

@@ -0,0 +1,5 @@
+{:source-columns [{:table "usage-stats" :column "instance_started"}
+                  {:table "usage-stats" :column "month_started"}
+                  {:table "usage-stats" :column "instance_finished"}]
+ :tables         [{:table "usage_stats"}]
+ :overrides      :macaw.error/unable-to-parse}

test/resources/acceptance/broken__filter_where.analysis.edn

@@ -0,0 +1,5 @@
+{:source-columns [{:table "execution" :column "error"}
+                  {:table "execution" :column "instance_id"}
+                  {:table "execution" :column "running_time"}]
+ :tables         [{:table "execution"}]
+ :overrides      :macaw.error/unable-to-parse}

test/resources/acceptance/compound__cte_masking.analysis.edn

@@ -4,6 +4,7 @@
 
  ;; See https://github.com/metabase/metabase/issues/42586
  :overrides
- ;; TODO currently each table gets hidden by the other CTE
- {:tables          []
-  :source-columns  []}}
+ {:ast-walker-1
+  ;; TODO currently each table gets hidden by the other CTE
+  {:tables         []
+   :source-columns []}}}

test/resources/acceptance/oracle__open_for.sql

@@ -0,0 +1,5 @@
+ OPEN ccur FOR
+    'select c.category
+     from ' || TABLE_NAME || ' c
+     where c.deptid=' || PI_N_Dept ||
+     ' and c.category not in ('|| sExcludeCategories ||')';

test/resources/acceptance/reserved__final.analysis.edn

@@ -0,0 +1,4 @@
+{:source-columns [{:table "invoice" :column "amount_paid_cents"}
+                  {:table "invoice" :column "id"}
+                  {:table "invoice" :column "is_deleted"}]
+ :tables [{:table "invoice"}]}

test/resources/acceptance/shadow__subselect.analysis.edn

@@ -12,7 +12,4 @@
  {:source-columns #{{:table "departments" :column "id"}
                     {:table "departments" :column "name"}
                     {:column "first_name"}
-                    {:column "last_name"}}
-
-  :basic-select
-  {:tables       ::not-implemented}}}
+                    {:column "last_name"}}}}

test/resources/acceptance/simple__select_into.analysis.edn

@@ -0,0 +1,5 @@
+{:source-columns [{:table "user" :column "id"}
+                  {:table "user" :column "name"}]
+ :tables         [{:table "user"}]
+ :overrides
+ {:basic-select :macaw.error/illegal-expression}}

test/resources/acceptance/simple__select_into.sql

@@ -0,0 +1,3 @@
+SELECT id, name
+INTO new_user_summary
+FROM user;

test/resources/acceptance/simple__select_star.edn

@@ -0,0 +1,3 @@
+{:has-wildcard?  true
+ :source-columns []
+ :tables         [{:table "t"}]}

test/resources/acceptance/simple__select_star.sql

@@ -0,0 +1 @@
+SELECT * FROM t;

test/resources/acceptance/sqlserver__execute.analysis.edn

@@ -0,0 +1,2 @@
+{:error     :macaw.error/illegal-expression
+ :overrides :macaw.error/unable-to-parse}

test/resources/acceptance/sqlserver__execute.sql

@@ -0,0 +1 @@
+EXECUTE stmt;

test/resources/acceptance/sqlserver__executesql.analysis.edn

@@ -0,0 +1,2 @@
+{:error     :macaw.error/illegal-expression
+ :overrides :macaw.error/unable-to-parse}

test/resources/acceptance/sqlserver__executesql.sql

@@ -0,0 +1 @@
+EXEC sp_executesql @SQL