fix: Sync postgres materialized view fields (#50102) (#50114)

* fix: Sync postgres materialized view fields

Fixes: #49912

* Disable test for redshift

* Fix formatting

Co-authored-by: Case Nelson <case@metabase.com>

src/metabase/driver/postgres.clj

@@ -281,51 +281,67 @@
 (defmethod sql-jdbc.sync/describe-fields-sql :postgres
   ;; The implementation is based on `getColumns` in https://github.com/pgjdbc/pgjdbc/blob/fcc13e70e6b6bb64b848df4b4ba6b3566b5e95a3/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java
   [driver & {:keys [schema-names table-names]}]
-  (sql/format {:select [[:c.column_name :name]
-                        [:c.udt_name :database-type]
-                        [[:- :c.ordinal_position [:inline 1]] :database-position]
-                        [:c.table_schema :table-schema]
-                        [:c.table_name :table-name]
-                        [[:not= :pk.column_name nil] :pk?]
-
-                        [[:col_description
-                          [:cast [:cast [:format [:inline "%I.%I"] [:cast :c.table_schema :text] [:cast :c.table_name :text]] :regclass] :oid]
-                          :c.ordinal_position]
-                         :field-comment]
-                        [[:and
-                          [:or [:= :column_default nil] [:= [:lower :column_default] [:inline "null"]]]
-                          [:= :is_nullable [:inline "NO"]]
-                          ;;_ IS_AUTOINCREMENT from: https://github.com/pgjdbc/pgjdbc/blob/fcc13e70e6b6bb64b848df4b4ba6b3566b5e95a3/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java#L1852-L1856
-                          [:not [:or
-                                 [:and [:!= :column_default nil] [:like :column_default [:inline "%nextval(%"]]]
-                                 [:!= :is_identity [:inline "NO"]]]]]
-                         :database-required]
-                        [[:or
-                          [:and [:!= :column_default nil] [:like :column_default [:inline "%nextval(%"]]]
-                          [:!= :is_identity [:inline "NO"]]]
-                         :database-is-auto-increment]]
-               :from [[:information_schema.columns :c]]
-               :left-join [[{:select [:tc.table_schema
-                                      :tc.table_name
-                                      :kc.column_name]
-                             :from [[:information_schema.table_constraints :tc]]
-                             :join [[:information_schema.key_column_usage :kc]
-                                    [:and
-                                     [:= :tc.constraint_name :kc.constraint_name]
-                                     [:= :tc.table_schema :kc.table_schema]
-                                     [:= :tc.table_name :kc.table_name]]]
-                             :where [:= :tc.constraint_type [:inline "PRIMARY KEY"]]}
-                            :pk]
+  (sql/format
+   {:union-all
+    [{:select [[:c.column_name :name]
+               [:c.udt_name :database-type]
+               [[:- :c.ordinal_position [:inline 1]] :database-position]
+               [:c.table_schema :table-schema]
+               [:c.table_name :table-name]
+               [[:not= :pk.column_name nil] :pk?]
+               [[:col_description
+                 [:cast [:cast [:format [:inline "%I.%I"] [:cast :c.table_schema :text] [:cast :c.table_name :text]] :regclass] :oid]
+                 :c.ordinal_position]
+                :field-comment]
+               [[:and
+                 [:or [:= :column_default nil] [:= [:lower :column_default] [:inline "null"]]]
+                 [:= :is_nullable [:inline "NO"]]
+                  ;;_ IS_AUTOINCREMENT from: https://github.com/pgjdbc/pgjdbc/blob/fcc13e70e6b6bb64b848df4b4ba6b3566b5e95a3/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java#L1852-L1856
+                 [:not [:or
+                        [:and [:!= :column_default nil] [:like :column_default [:inline "%nextval(%"]]]
+                        [:!= :is_identity [:inline "NO"]]]]]
+                :database-required]
+               [[:or
+                 [:and [:!= :column_default nil] [:like :column_default [:inline "%nextval(%"]]]
+                 [:!= :is_identity [:inline "NO"]]]
+                :database-is-auto-increment]]
+      :from [[:information_schema.columns :c]]
+      :left-join [[{:select [:tc.table_schema
+                             :tc.table_name
+                             :kc.column_name]
+                    :from [[:information_schema.table_constraints :tc]]
+                    :join [[:information_schema.key_column_usage :kc]
                            [:and
-                            [:= :c.table_schema :pk.table_schema]
-                            [:= :c.table_name :pk.table_name]
-                            [:= :c.column_name :pk.column_name]]]
-               :where [:and
-                       [:raw "c.table_schema !~ '^information_schema|catalog_history|pg_'"]
-                       (when schema-names [:in :c.table_schema schema-names])
-                       (when table-names [:in :c.table_name table-names])]
-               :order-by [:table-schema :table-name :database-position]}
-              :dialect (sql.qp/quote-style driver)))
+                            [:= :tc.constraint_name :kc.constraint_name]
+                            [:= :tc.table_schema :kc.table_schema]
+                            [:= :tc.table_name :kc.table_name]]]
+                    :where [:= :tc.constraint_type [:inline "PRIMARY KEY"]]}
+                   :pk]
+                  [:and
+                   [:= :c.table_schema :pk.table_schema]
+                   [:= :c.table_name :pk.table_name]
+                   [:= :c.column_name :pk.column_name]]]
+      :where [:and
+              [:raw "c.table_schema !~ '^information_schema|catalog_history|pg_'"]
+              (when schema-names [:in :c.table_schema schema-names])
+              (when table-names [:in :c.table_name table-names])]}
+     {:select [[:pa.attname :name]
+               [:pt.typname :database-type]
+               [[:- :pa.attnum [:inline 1]] :database-position]
+               [:pn.nspname :table-schema]
+               [:pc.relname :table-name]
+               [false :pk?]
+               [nil :field-comment]
+               [false :database-required]
+               [false :database-is-auto-increment]]
+      :from [[:pg_catalog.pg_class :pc]]
+      :join [[:pg_catalog.pg_namespace :pn] [:= :pn.oid :pc.relnamespace]
+             [:pg_catalog.pg_attribute :pa] [:= :pa.attrelid :pc.oid]
+             [:pg_catalog.pg_type :pt] [:= :pt.oid :pa.atttypid]]
+      :where [:and [:= :pc.relkind [:inline "m"]]
+              [:>= :pa.attnum 1]]}]
+    :order-by [:table-schema :table-name :database-position]}
+   :dialect (sql.qp/quote-style driver)))
 
 (defmethod sql-jdbc.sync/describe-fks-sql :postgres
   [driver & {:keys [schema-names table-names]}]

test/metabase/driver/postgres_test.clj

@@ -37,6 +37,7 @@
    [metabase.sync.sync-metadata.tables :as sync-tables]
    [metabase.sync.util :as sync-util]
    [metabase.test :as mt]
+   [metabase.test.data.sql :as sql.tx]
    [metabase.util :as u]
    [metabase.util.honey-sql-2 :as h2x]
    [metabase.util.log :as log]
@@ -897,6 +898,75 @@
                       :data
                       (select-keys [:rows :native_form]))))))))))
 
+(deftest enums-test-3
+  (mt/test-driver :postgres
+    (do-with-enums-db!
+     (fn [db]
+       (jdbc/execute! (sql-jdbc.conn/db->pooled-connection-spec db)
+                      [(sql.tx/create-materialized-view-of-table-sql driver/*driver* db "birds_m" "birds")])
+       (sync/sync-database! db)
+
+       (testing "check that describe-table properly describes the database & base types of the enum fields"
+         (is (=? [{:table-schema               "public"
+                   :table-name                 "birds_m"
+                   :name                       "name"
+                   :database-type              "varchar"
+                   :base-type                  :type/Text
+                   :pk?                        false
+                   :database-position          0
+                   :database-required          false
+                   :database-is-auto-increment false
+                   :json-unfolding             false}
+                  {:table-schema               "public"
+                   :table-name                 "birds_m"
+                   :name                       "status"
+                   :database-type              "bird_status"
+                   :base-type                  :type/PostgresEnum
+                   :database-position          1
+                   :database-required          false
+                   :database-is-auto-increment false
+                   :json-unfolding             false}
+                  {:table-schema               "public"
+                   :table-name                 "birds_m"
+                   :name                       "type"
+                   :database-type              "bird type"
+                   :base-type                  :type/PostgresEnum
+                   :database-position          2
+                   :database-required          false
+                   :database-is-auto-increment false
+                   :json-unfolding             false}]
+                 (->> (driver/describe-fields :postgres db {:table-names ["birds_m"]})
+                      (into #{})
+                      (sort-by :database-position)))))
+
+       (testing "check that when syncing the DB the enum types get recorded appropriately"
+         (let [table-id (t2/select-one-pk Table :db_id (u/the-id db), :name "birds_m")]
+           (is (= #{{:name "name", :database_type "varchar", :base_type :type/Text}
+                    {:name "type", :database_type "bird type", :base_type :type/PostgresEnum}
+                    {:name "status", :database_type "bird_status", :base_type :type/PostgresEnum}}
+                  (set (map (partial into {})
+                            (t2/select [Field :name :database_type :base_type] :table_id table-id)))))))
+
+       (testing "End-to-end check: make sure everything works as expected when we run an actual query"
+         (let [table-id           (t2/select-one-pk Table :db_id (u/the-id db), :name "birds_m")
+               bird-type-field-id (t2/select-one-pk Field :table_id table-id, :name "type")]
+           (is (= {:rows        [["Rasta" "good bird" "toucan"]]
+                   :native_form {:query  (str "SELECT \"public\".\"birds_m\".\"name\" AS \"name\","
+                                              " \"public\".\"birds_m\".\"status\" AS \"status\","
+                                              " \"public\".\"birds_m\".\"type\" AS \"type\" "
+                                              "FROM \"public\".\"birds_m\" "
+                                              "WHERE \"public\".\"birds_m\".\"type\" = CAST('toucan' AS \"bird type\") "
+                                              "LIMIT 10")
+                                 :params nil}}
+                  (-> (qp/process-query
+                       {:database (u/the-id db)
+                        :type     :query
+                        :query    {:source-table table-id
+                                   :filter       [:= [:field (u/the-id bird-type-field-id) nil] "toucan"]
+                                   :limit        10}})
+                      :data
+                      (select-keys [:rows :native_form]))))))))))
+
 (deftest enums-actions-test
   (mt/test-driver :postgres
     (testing "actions with enums"

test/metabase/driver/sql_jdbc/sync/describe_table_test.clj

@@ -754,3 +754,53 @@
                         (sql.tx/create-index-sql driver/*driver* "conditional_index" ["column"] {:condition "id > 2"}))
          (is (= #{{:type :normal-column-index :value "id"}}
                 (describe-table-indexes (t2/select-one :model/Table (mt/id :conditional_index))))))))))
+
+(defmethod driver/database-supports? [::driver/driver ::materialized-view-fields]
+  [_driver _feature _database]
+  false)
+
+;; TODO Make all drivers that support materialized-views pass this test
+(doseq [driver [:postgres
+                #_:redshift
+                #_:vertica
+                #_:athena
+                #_:bigquery-cloud-sdk]]
+  (defmethod driver/database-supports? [driver ::describe-materialized-view-fields]
+    [_driver _feature _database]
+    true))
+
+(defmethod driver/database-supports? [:redshift ::describe-materialized-view-fields]
+  [_driver _feature _database]
+  false)
+
+(deftest describe-materialized-view-fields
+  (mt/test-drivers (set/intersection (mt/normal-drivers-with-feature ::describe-materialized-view-fields)
+                                     (mt/sql-jdbc-drivers))
+    (do-with-temporary-dataset
+     (mt/dataset-definition "describe-materialized-views-test"
+                            ["orders"
+                             [{:field-name "amount" :base-type :type/Integer}]
+                             [[1 2]]])
+     (fn []
+       (try
+         ;; Move creating and dropping view to a sql-jdbc defmethod of a metabase.test.data defmulti
+         (jdbc/execute! (sql-jdbc.conn/db->pooled-connection-spec (mt/db))
+                        [(sql.tx/create-materialized-view-of-table-sql driver/*driver* (mt/db) "orders_m" "orders")])
+         (sync/sync-database! (mt/db))
+         (let [orders-id (t2/select-one-pk :model/Table :db_id (mt/id) [:lower :name] "orders")
+               orders-m-id (t2/select-one-pk :model/Table :db_id (mt/id) [:lower :name] "orders_m")]
+           (is (= [["id" :type/Integer 0]
+                   ["amount" :type/Integer 1]]
+                  (t2/select-fn-vec
+                   (juxt (comp u/lower-case-en :name) :base_type :database_position)
+                   :model/Field
+                   :table_id orders-id
+                   {:order-by [:database_position]})
+                  (t2/select-fn-vec
+                   (juxt (comp u/lower-case-en :name) :base_type :database_position)
+                   :model/Field
+                   :table_id orders-m-id
+                   {:order-by [:database_position]}))))
+         (finally
+           (jdbc/execute! (sql-jdbc.conn/db->pooled-connection-spec (mt/db))
+                          [(sql.tx/drop-materialized-view-sql driver/*driver* (mt/db) "orders_m")])))))))

test/metabase/test/data/sql.clj

@@ -343,6 +343,41 @@
 
 (defmethod session-schema :sql/test-extensions [_] nil)
 
+(defmulti create-materialized-view-of-table-sql
+  "Return a `CREATE MATERIALIZED VIEW` statement.
+   The view should be a simple view of the table, like `select * from table`
+   `view-name` is the name of the new view
+   `table-name` is the name of the table."
+  {:arglists '([driver database view-name table-name])}
+  tx/dispatch-on-driver-with-test-extensions
+  :hierarchy #'driver/hierarchy)
+
+(defmethod create-materialized-view-of-table-sql :sql/test-extensions
+  [driver _database view-name table-name]
+  (let [qualified-view-name (sql.u/quote-name driver :table view-name)
+        qualified-table-name (sql.u/quote-name driver :table table-name)
+        schema-name (sql.u/quote-name driver :schema (session-schema driver))]
+    (format "CREATE MATERIALIZED VIEW %s.%s AS select * from %s.%s;"
+            schema-name
+            qualified-view-name
+            schema-name
+            qualified-table-name)))
+
+(defmulti drop-materialized-view-sql
+  "Return a `DROP MATERIALIZED VIEW` statement.
+   `view-name` is the name of the new view."
+  {:arglists '([driver database view-name])}
+  tx/dispatch-on-driver-with-test-extensions
+  :hierarchy #'driver/hierarchy)
+
+(defmethod drop-materialized-view-sql :sql/test-extensions
+  [driver _database view-name]
+  (let [qualified-view-name (sql.u/quote-name driver :table view-name)
+        schema-name (sql.u/quote-name driver :schema (session-schema driver))]
+    (format "DROP MATERIALIZED VIEW %s.%s;"
+            schema-name
+            qualified-view-name)))
+
 (defmethod tx/native-query-with-card-template-tag :sql
   [_driver card-template-tag-name]
   (let [source-table-name (u/lower-case-en (u.random/random-name))]