diff --git a/.circleci/config.yml b/.circleci/config.yml
index 36ba30e462e2fef5c4558b6be64cb62517255636..76122817b5443d2bc8045fa56a8b1c796a1a658d 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -126,7 +126,7 @@ executors:
working_directory: /home/circleci/metabase/metabase/
docker:
- image: metabase/ci:circleci-java-11-clj-1.10.3.929-07-27-2021-node-browsers
- - image: circleci/mongo:4.0
+ - image: circleci/mongo:5.0
presto-186:
working_directory: /home/circleci/metabase/metabase/
@@ -196,11 +196,11 @@ executors:
resource_class: large
- fe-mongo-4:
+ fe-mongo-5:
working_directory: /home/circleci/metabase/metabase/
docker:
- image: metabase/ci:circleci-java-11-clj-1.10.3.929-07-27-2021-node-browsers
- - image: metabase/qa-databases:mongo-sample-4.0
+ - image: metabase/qa-databases:mongo-sample-5.0
fe-postgres-12:
working_directory: /home/circleci/metabase/metabase/
@@ -1266,10 +1266,10 @@ workflows:
source-folder: << matrix.folder >>
- fe-tests-cypress:
- name: e2e-tests-mongo-4-<< matrix.edition >>
+ name: e2e-tests-mongo-5-<< matrix.edition >>
requires:
- build-uberjar-<< matrix.edition >>
- e: fe-mongo-4
+ e: fe-mongo-5
cypress-group: "mongo"
source-folder: mongo
qa-db: true
diff --git a/frontend/test/metabase-db/mongo/query.cy.spec.js b/frontend/test/metabase-db/mongo/query.cy.spec.js
index d0fe9073e8401c79d4db59902e551ff5cc0df566..10082c04edf09fe723d01875f98bd9fc2a0e5f00 100644
--- a/frontend/test/metabase-db/mongo/query.cy.spec.js
+++ b/frontend/test/metabase-db/mongo/query.cy.spec.js
@@ -104,5 +104,5 @@ function writeNativeMongoQuery() {
parseSpecialCharSequences: false,
});
cy.get(".NativeQueryEditor .Icon-play").click();
- cy.findByText("18,760");
+ cy.findByText("42");
}
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo.clj b/modules/drivers/mongo/src/metabase/driver/mongo.clj
index 78e4833164adac457bca3b4a289ce749d1b541d6..a8bdd5f2b948ae5539a92a7bb70fe67d6264358e 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo.clj
@@ -18,6 +18,7 @@
[monger.collection :as mc]
[monger.command :as cmd]
[monger.conversion :as m.conversion]
+ [monger.core :as mg]
[monger.db :as mdb]
monger.json
[schema.core :as s]
@@ -174,8 +175,9 @@
(defmethod driver/describe-database :mongo
[_ database]
(with-mongo-connection [^com.mongodb.DB conn database]
- {:tables (set (for [collection (disj (mdb/get-collection-names conn) "system.indexes")]
- {:schema nil, :name collection}))}))
+ {:tables (set (for [collection (disj (mdb/get-collection-names conn) "system.indexes")]
+ {:schema nil, :name collection}))
+ :version (get (mg/command conn {:buildInfo 1}) "version")}))
(defn- table-sample-column-info
"Sample the rows (i.e., documents) in `table` and return a map of information about the column keys we found in that
@@ -215,6 +217,13 @@
:native-parameters]]
(defmethod driver/supports? [:mongo feature] [_ _] true))
+(defmethod driver/database-supports? [:mongo :expressions] [_ _ db]
+ (let [version (some-> (get-in db [:details :version])
+ (str/split #"\.")
+ first
+ Integer/parseInt)]
+ (and (some? version) (<= 5 version))))
+
(defmethod driver/mbql->native :mongo
[_ query]
(qp/mbql->native query))
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
index 3b99478c65f73f19e9e30c541c7d21d83b989985..40716c2c15ec543ae320ed45660af4949b98edfb 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
@@ -105,10 +105,18 @@
[field]
(field->name field \.))
+(defmethod ->lvalue :expression
+ [[_ expression-name]]
+ expression-name)
+
(defmethod ->rvalue :default
[x]
x)
+(defmethod ->rvalue :expression
+ [[_ expression-name]]
+ (->rvalue (mbql.u/expression-with-name (:query *query*) expression-name)))
+
(defmethod ->rvalue (class Field)
[{coercion :coercion_strategy, :as field}]
(let [field-name (str \$ (field->name field "."))]
@@ -284,6 +292,77 @@
(u.date/add unit amount)
(u.date/bucket unit)))))))
+;;; ---------------------------------------------------- functions ---------------------------------------------------
+
+;; It doesn't make 100% sense to have lvalues for all these but it's a formal requirement
+
+(defmethod ->lvalue :avg [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :stddev [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :sum [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :min [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :max [[_ inp]] (->lvalue inp))
+
+(defmethod ->lvalue :floor [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :ceil [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :round [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :abs [[_ inp]] (->lvalue inp))
+
+(defmethod ->lvalue :log [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :exp [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :sqrt [[_ inp]] (->lvalue inp))
+
+(defmethod ->lvalue :trim [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :ltrim [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :rtrim [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :upper [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :lower [[_ inp]] (->lvalue inp))
+(defmethod ->lvalue :length [[_ inp]] (->lvalue inp))
+
+(defmethod ->lvalue :power [[_ & args]] (->lvalue (first args)))
+(defmethod ->lvalue :replace [[_ & args]] (->lvalue (first args)))
+(defmethod ->lvalue :concat [[_ & args]] (->lvalue (first args)))
+(defmethod ->lvalue :substring [[_ & args]] (->lvalue (first args)))
+
+(defmethod ->lvalue :+ [[_ & args]] (->lvalue (first args)))
+(defmethod ->lvalue :- [[_ & args]] (->lvalue (first args)))
+(defmethod ->lvalue :* [[_ & args]] (->lvalue (first args)))
+(defmethod ->lvalue :/ [[_ & args]] (->lvalue (first args)))
+
+(defmethod ->rvalue :coalesce [[_ & args]] (->lvalue (first args)))
+
+(defmethod ->rvalue :avg [[_ inp]] {"$avg" (->rvalue inp)})
+(defmethod ->rvalue :stddev [[_ inp]] {"$stdDevPop" (->rvalue inp)})
+(defmethod ->rvalue :sum [[_ inp]] {"$sum" (->rvalue inp)})
+(defmethod ->rvalue :min [[_ inp]] {"$min" (->rvalue inp)})
+(defmethod ->rvalue :max [[_ inp]] {"$max" (->rvalue inp)})
+
+(defmethod ->rvalue :floor [[_ inp]] {"$floor" (->rvalue inp)})
+(defmethod ->rvalue :ceil [[_ inp]] {"$ceil" (->rvalue inp)})
+(defmethod ->rvalue :round [[_ inp]] {"$round" (->rvalue inp)})
+(defmethod ->rvalue :abs [[_ inp]] {"$abs" (->rvalue inp)})
+
+(defmethod ->rvalue :log [[_ inp]] {"$log10" (->rvalue inp)})
+(defmethod ->rvalue :exp [[_ inp]] {"$exp" (->rvalue inp)})
+(defmethod ->rvalue :sqrt [[_ inp]] {"$sqrt" (->rvalue inp)})
+
+(defmethod ->rvalue :trim [[_ inp]] {"$trim" (->rvalue inp)})
+(defmethod ->rvalue :ltrim [[_ inp]] {"$ltrim" (->rvalue inp)})
+(defmethod ->rvalue :rtrim [[_ inp]] {"$rtrim" (->rvalue inp)})
+(defmethod ->rvalue :upper [[_ inp]] {"$toUpper" (->rvalue inp)})
+(defmethod ->rvalue :lower [[_ inp]] {"$toLower" (->rvalue inp)})
+(defmethod ->rvalue :length [[_ inp]] {"$strLenCP" (->rvalue inp)})
+
+(defmethod ->rvalue :power [[_ & args]] {"$pow" (mapv ->rvalue args)})
+(defmethod ->rvalue :replace [[_ & args]] {"$replaceAll" (mapv ->rvalue args)})
+(defmethod ->rvalue :concat [[_ & args]] {"$concat" (mapv ->rvalue args)})
+(defmethod ->rvalue :substring [[_ & args]] {"$substrCP" (mapv ->rvalue args)})
+
+(defmethod ->rvalue :+ [[_ & args]] {"$add" (mapv ->rvalue args)})
+(defmethod ->rvalue :- [[_ & args]] {"$subtract" (mapv ->rvalue args)})
+(defmethod ->rvalue :* [[_ & args]] {"$multiply" (mapv ->rvalue args)})
+(defmethod ->rvalue :/ [[_ & args]] {"$divide" (mapv ->rvalue args)})
+
+(defmethod ->rvalue :coalesce [[_ & args]] {"$ifNull" (mapv ->rvalue args)})
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | CLAUSE APPLICATION |
@@ -645,7 +724,6 @@
{$skip offset}))
{$limit items-per-page}]))))
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Process & Run |
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj b/modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj
index 1e1d91c094f6a50e265dfac6065d7fae9cbb3dde..f88d3bafd2162884249af14024f68d26acb44f42 100644
--- a/modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj
+++ b/modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj
@@ -153,6 +153,70 @@
[:distinct $price]]
:limit 5})))))))
+(defn- extract-projections [projections q]
+ (select-keys (get-in q [:query 0 "$project"]) projections))
+
+(deftest expressions-test
+ (mt/test-driver :mongo
+ (testing "Should be able to deal with expressions (#9382 is for BQ but we're doing it for mongo too)"
+ (is (= {"bob" "$latitude", "cobb" "$name"}
+ (extract-projections
+ ["bob" "cobb"]
+ (qp/query->native
+ (mt/mbql-query venues
+ {:fields [[:expression "bob"] [:expression "cobb"]]
+ :expressions {:bob [:field $latitude nil]
+ :cobb [:field $name nil]}
+ :limit 5}))))))
+ (testing "Should be able to deal with 1-arity functions"
+ (is (= {"cobb" {"$toUpper" "$name"},
+ "bob" {"$abs" "$latitude"} }
+ (extract-projections
+ ["bob" "cobb"]
+ (qp/query->native
+ (mt/mbql-query venues
+ {:filters [[:expression "bob"] [:expression "cobb"]]
+ :expressions {:bob [:abs $latitude]
+ :cobb [:upper $name]}
+ :limit 5}))))))
+ (testing "Should be able to deal with 2-arity functions"
+ (is (= {"bob" {"$add" ["$price" 300]}}
+ (extract-projections
+ ["bob"]
+ (qp/query->native
+ (mt/mbql-query venues
+ {:filters [[:expression "bob"] [:expression "cobb"]]
+ :expressions {:bob [:+ $price 300]}
+ :limit 5}))))))
+ (testing "Should be able to deal with a little indirection"
+ (is (= {"bob" {"$abs" {"$subtract" ["$price" 300]}}}
+ (extract-projections
+ ["bob"]
+ (qp/query->native
+ (mt/mbql-query venues
+ {:filters [[:expression "bob"] [:expression "cobb"]]
+ :expressions {:bob [:abs [:- $price 300]]}
+ :limit 5}))))))
+ (testing "Should be able to deal with a little indirection, with an expression in"
+ (is (= {"bob" {"$abs" "$latitude"},
+ "cobb" {"$ceil" {"$abs" "$latitude"}}}
+ (extract-projections
+ ["bob" "cobb"]
+ (qp/query->native
+ (mt/mbql-query venues
+ {:filters [[:expression "bob"] [:expression "cobb"]]
+ :expressions {:bob [:abs $latitude]
+ :cobb [:ceil [:expression "bob"]]}
+ :limit 5}))))))
+ (testing "Should be able to deal with coalescing"
+ (is (= {"bob" {"$ifNull" ["$latitude" "$price"]}}
+ (extract-projections
+ ["bob"]
+ (qp/query->native
+ (mt/mbql-query venues
+ {:expressions {:bob [:coalesce [:field $latitude nil] [:field $price nil]]}
+ :limit 5}))))))))
+
(deftest compile-time-interval-test
(mt/test-driver :mongo
(testing "Make sure time-intervals work the way they're supposed to."
diff --git a/modules/drivers/mongo/test/metabase/driver/mongo_test.clj b/modules/drivers/mongo/test/metabase/driver/mongo_test.clj
index 7300f05cd2a6030ac8e5f690a176fbe28720f846..f44ae69ae5f3ff4e5887c39adb74579e3e32c863 100644
--- a/modules/drivers/mongo/test/metabase/driver/mongo_test.clj
+++ b/modules/drivers/mongo/test/metabase/driver/mongo_test.clj
@@ -61,10 +61,33 @@
:expected true}
{:details {:conn-uri "mongodb://localhost:3000/bad-db-name?connectTimeoutMS=50"}
:expected false}]]
- (testing (str "connect with " details)
- (is (= expected
- (driver.u/can-connect-with-details? :mongo details))
- (str message))))))
+ (testing (str "connect with " details)
+ (is (= expected
+ (driver.u/can-connect-with-details? :mongo details))
+ (str message))))))
+
+(deftest database-supports?-test
+(mt/test-driver
+ :mongo
+ (doseq [{:keys [details expected]} [{:details {:host "localhost"
+ :port 3000
+ :dbname "bad-db-name"
+ :version "5.0.0"}
+ :expected true}
+ {:details {}
+ :expected false}
+ {:details {:version nil}
+ :expected false}
+ {:details {:host "localhost"
+ :port 27017
+ :dbname "metabase-test"
+ :version "2.2134234.lol"}
+ :expected false}]]
+ (testing (str "connect with " details)
+ (is (= expected
+ (let [db (db/insert! Database {:name "dummy", :engine "mongo", :details details})]
+ (driver/database-supports? :mongo :expressions db))))))))
+
(def ^:private native-query
"[{\"$project\": {\"_id\": \"$_id\"}},
@@ -96,11 +119,11 @@
(deftest describe-database-test
(mt/test-driver :mongo
- (is (= {:tables #{{:schema nil, :name "checkins"}
- {:schema nil, :name "categories"}
- {:schema nil, :name "users"}
- {:schema nil, :name "venues"}}}
- (driver/describe-database :mongo (mt/db))))))
+ (is (= #{{:schema nil, :name "checkins"}
+ {:schema nil, :name "categories"}
+ {:schema nil, :name "users"}
+ {:schema nil, :name "venues"}}
+ (:tables (driver/describe-database :mongo (mt/db)))))))
(deftest describe-table-test
(mt/test-driver :mongo
diff --git a/src/metabase/sync/interface.clj b/src/metabase/sync/interface.clj
index 33747a9247f40dd4323558bde94707270338b0fa..bfab0baeb2faf5d98d93c5b6f1f9917519330cc5 100644
--- a/src/metabase/sync/interface.clj
+++ b/src/metabase/sync/interface.clj
@@ -17,7 +17,8 @@
(def DatabaseMetadata
"Schema for the expected output of `describe-database`."
- {:tables #{DatabaseMetadataTable}})
+ {:tables #{DatabaseMetadataTable}
+ (s/optional-key :version) (s/maybe su/NonBlankString)})
(def TableMetadataField
"Schema for a given Field as provided in `describe-table`."
diff --git a/src/metabase/sync/sync_metadata.clj b/src/metabase/sync/sync_metadata.clj
index fe7f29bc58f3aa99eaa9493b8087e1097a51a891..f612cd49cc5283325f99c2fbaf5e71ffea17eb40 100644
--- a/src/metabase/sync/sync_metadata.clj
+++ b/src/metabase/sync/sync_metadata.clj
@@ -34,7 +34,7 @@
(def ^:private sync-steps
[(sync-util/create-sync-step "sync-timezone" sync-tz/sync-timezone! sync-timezone-summary)
;; Make sure the relevant table models are up-to-date
- (sync-util/create-sync-step "sync-tables" sync-tables/sync-tables! sync-tables-summary)
+ (sync-util/create-sync-step "sync-tables" sync-tables/sync-tables-and-database! sync-tables-summary)
;; Now for each table, sync the fields
(sync-util/create-sync-step "sync-fields" sync-fields/sync-fields! sync-fields-summary)
;; Now for each table, sync the FKS. This has to be done after syncing all the fields to make sure target fields exist
diff --git a/src/metabase/sync/sync_metadata/tables.clj b/src/metabase/sync/sync_metadata/tables.clj
index 92f2392e0e21852004f1f465ba8e5df1a3f0e0ff..5f3d4045bfd50fc81f44352dd9c5ea5d4745e946 100644
--- a/src/metabase/sync/sync_metadata/tables.clj
+++ b/src/metabase/sync/sync_metadata/tables.clj
@@ -3,6 +3,7 @@
(:require [clojure.data :as data]
[clojure.string :as str]
[clojure.tools.logging :as log]
+ [metabase.models.database :as db-model :refer [Database]]
[metabase.models.humanization :as humanization]
[metabase.models.table :as table :refer [Table]]
[metabase.sync.fetch-metadata :as fetch-metadata]
@@ -80,6 +81,14 @@
;;; ---------------------------------------------------- Syncing -----------------------------------------------------
+(s/defn ^:private update-database-metadata!
+ "If there is a version in the db-metadata update the DB to have that in the DB model"
+ [database :- i/DatabaseInstance db-metadata :- i/DatabaseMetadata]
+ (log/info (trs "Found new version for DB: {0}" (:version db-metadata)))
+ (db/update! Database (u/the-id database)
+ :details
+ (assoc (:details database) :version (:version db-metadata))))
+
;; TODO - should we make this logic case-insensitive like it is for fields?
(s/defn ^:private create-or-reactivate-tables!
@@ -137,10 +146,11 @@
:description description))))
-(s/defn ^:private db-metadata :- #{i/DatabaseMetadataTable}
- "Return information about `database` by calling its driver's implementation of `describe-database`."
- [database :- i/DatabaseInstance]
- (set (for [table (:tables (fetch-metadata/db-metadata database))
+(s/defn ^:private table-set :- #{i/DatabaseMetadataTable}
+ "So there exist tables for the user and metabase metadata tables for internal usage by metabase.
+ Get set of user tables only, excluding metabase metadata tables."
+ [db-metadata :- i/DatabaseMetadata]
+ (set (for [table (:tables db-metadata)
:when (not (metabase-metadata/is-metabase-metadata-table? table))]
table)))
@@ -152,19 +162,26 @@
:db_id (u/the-id database)
:active true))))
-(s/defn sync-tables!
+(s/defn sync-tables-and-database!
"Sync the Tables recorded in the Metabase application database with the ones obtained by calling `database`'s driver's
- implementation of `describe-database`."
+ implementation of `describe-database`.
+ Also syncs the database metadata taken from describe-database if there is any"
[database :- i/DatabaseInstance]
;; determine what's changed between what info we have and what's in the DB
- (let [db-metadata (db-metadata database)
+ (let [db-metadata (fetch-metadata/db-metadata database)
+ db-tables (table-set db-metadata)
our-metadata (our-metadata database)
strip-desc (fn [metadata]
(set (map #(dissoc % :description) metadata)))
[new-tables old-tables] (data/diff
- (strip-desc db-metadata)
+ (strip-desc db-tables)
(strip-desc our-metadata))
- [changed-tables] (data/diff db-metadata our-metadata)]
+ [changed-tables] (data/diff db-tables our-metadata)]
+ ;; update database metadata from database
+ (when (some? (:version db-metadata))
+ (sync-util/with-error-handling (format "Error creating/reactivating tables for %s"
+ (sync-util/name-for-logging database))
+ (update-database-metadata! database db-metadata)))
;; create new tables as needed or mark them as active again
(when (seq new-tables)
(sync-util/with-error-handling (format "Error creating/reactivating tables for %s"
diff --git a/test/metabase/sync/sync_metadata/comments_test.clj b/test/metabase/sync/sync_metadata/comments_test.clj
index 28dda64b529fe98154956fee2eccc9c73d8cc29c..0a3b3d75a484593d4cd3fbe72a72023d32efdf5f 100644
--- a/test/metabase/sync/sync_metadata/comments_test.clj
+++ b/test/metabase/sync/sync_metadata/comments_test.clj
@@ -103,7 +103,7 @@
;; change the description in metabase while the source table comment remains the same
(db/update-where! Table {:id (mt/id "table_with_updated_desc")}, :description "updated table description")
;; now sync the DB again, this should NOT overwrite the manually updated description
- (sync-tables/sync-tables! (mt/db))
+ (sync-tables/sync-tables-and-database! (mt/db))
(is (= #{{:name (mt/format-name "table_with_updated_desc"), :description "updated table description"}}
(db->tables (mt/db)))))))))
@@ -114,6 +114,6 @@
;; modify the source DB to add the comment and resync
(driver/notify-database-updated driver/*driver* (mt/db))
(tx/create-db! driver/*driver* (basic-table "table_with_comment_after_sync" "added comment"))
- (sync-tables/sync-tables! (mt/db))
+ (sync-tables/sync-tables-and-database! (mt/db))
(is (= #{{:name (mt/format-name "table_with_comment_after_sync"), :description "added comment"}}
(db->tables (mt/db))))))))