Fix getting native query for mongo returns a broken string (#31542)

src/metabase/db/query.clj

@@ -37,23 +37,18 @@
 (defn- format-sql*
   "Return a nicely-formatted version of a generic `sql` string.
   Note that it will not play well with Metabase parameters."
-  (^String [sql]
-   (format-sql* sql (mdb.connection/db-type)))
-
-  (^String [^String sql db-type]
-   (when sql
-     (if (isa? driver.impl/hierarchy db-type :sql)
-       (let [formatter (SqlFormatter/of (case db-type
-                                          :mysql Dialect/MySql
-                                          :postgres Dialect/PostgreSql
-                                          :redshift Dialect/Redshift
-                                          :sparksql Dialect/SparkSql
-                                          :sqlserver Dialect/TSql
-                                          :oracle Dialect/PlSql
-                                          :bigquery-cloud-sdk Dialect/MySql
-                                          Dialect/StandardSql))]
-         (.format formatter sql))
-       sql))))
+  [^String sql db-type]
+  (when sql
+    (let [formatter (SqlFormatter/of (case db-type
+                                       :mysql Dialect/MySql
+                                       :postgres Dialect/PostgreSql
+                                       :redshift Dialect/Redshift
+                                       :sparksql Dialect/SparkSql
+                                       :sqlserver Dialect/TSql
+                                       :oracle Dialect/PlSql
+                                       :bigquery-cloud-sdk Dialect/MySql
+                                       Dialect/StandardSql))]
+      (.format formatter sql))))
 
 (defn- fix-sql-params
   "format-sql* will expand parameterized values (e.g. {{#123}} -> { { # 123 } }).
@@ -63,9 +58,15 @@
     (let [rgx #"\{\s*\{\s*[^\}]+\s*\}\s*\}"]
       (str/replace sql rgx (fn [match] (str/replace match #"\s*" ""))))))
 
-(def ^{:arglists '([sql] [sql db-type])} format-sql
-  "Return a nicely-formatted version of a `sql` string."
-  (comp fix-sql-params format-sql*))
+(defn format-sql
+  "Return a nicely-formatted version of a `query` string.
+  For mongo queries, return as is since it's already in a nice json-like format."
+ ([sql]
+  (format-sql sql (mdb.connection/db-type)))
+ ([sql db-type]
+  (if (isa? driver.impl/hierarchy db-type :sql)
+    (fix-sql-params (format-sql* sql db-type))
+    sql)))
 
 (defmulti compile
   "Compile a `query` (e.g. a Honey SQL map) to `[sql & args]`."

test/metabase/db/query_test.clj

 (ns metabase.db.query-test
   (:require
    [cheshire.core :as json]
-   [clojure.java.io :as io]
    [clojure.string :as str]
    [clojure.test :refer :all]
    [metabase.db.query :as mdb.query]
@@ -76,23 +75,34 @@
                :database (mt/id)}]
         (verify-same-query q)))))
 
-(deftest nonsql-dialects-return-original-string-test
-  (testing "Passing a mongodb query through format-sql returns the original string"
-    (with-open [r (io/reader (io/resource "metabase/db/mongodbquery.json"))]
-      (let [query                 (slurp r)
-            ;; Formatting a non-sql string returns nothing
-            formatted-query       (mdb.query/format-sql query :mongo)
-            ;; This is a mongodb query, but if you pass in the wrong driver it will attempt the format
+(deftest nonsql-dialects-return-original-query-test
+  (testing "Passing a mongodb query through format-sql returns the original query (#31122)"
+    (let [query [{"$group"  {"_id" {"created_at" {"$let" {"vars" {"parts" {"$dateToParts" {"timezone" "UTC"
+                                                                                           "date"     "$created_at"}}}
+                                                          "in"   {"$dateFromParts" {"timezone" "UTC"
+                                                                                    "year"     "$$parts.year"
+                                                                                    "month"    "$$parts.month"
+                                                                                    "day"      "$$parts.day"}}}}}
+                             "sum" {"$sum" "$tax"}}}
+                 {"$sort"    {"_id" 1}}
+                 {"$project" {"_id" false
+                              "created_at" "$_id.created_at"
+                              "sum" true}}]
+          formatted-query (mdb.query/format-sql query :mongo)]
+
+      (testing "Formatting a non-sql query returns the same query"
+        (is (= query formatted-query)))
+
+      ;; TODO(qnkhuat): do we really need to handle case where wrong driver is passed?
+      (let [;; This is a mongodb query, but if you pass in the wrong driver it will attempt the format
             ;; This is a corner case since the system should always be using the right driver
-            weird-formatted-query (mdb.query/format-sql query :postgres)]
-        (testing "Formatting a non-sql query returns the same string"
-          (is (= query formatted-query)))
+            weird-formatted-query (mdb.query/format-sql (json/generate-string query) :postgres)]
         (testing "The wrong formatter will change the format..."
           (is (not= query weird-formatted-query)))
         (testing "...but the resulting data is still the same"
           ;; Bottom line - Use the right driver, but if you use the wrong
           ;; one it should be harmless but annoying
-          (is (= (json/parse-string query)
+          (is (= query
                  (json/parse-string weird-formatted-query))))))))
 
 (deftest ^:parallel format-sql-with-params-test

test_resources/metabase/db/mongodbquery.json

-[
-  {
-    "$lookup": {
-      "from": "people",
-      "let": {
-        "let_user_id_146740": "$user_id"
-      },
-      "pipeline": [
-        {
-          "$match": {
-            "$expr": {
-              "$eq": [
-                "$$let_user_id_146740",
-                "$_id"
-              ]
-            }
-          }
-        }
-      ],
-      "as": "join_alias_People"
-    }
-  },
-  {
-    "$unwind": {
-      "path": "$join_alias_People",
-      "preserveNullAndEmptyArrays": true
-    }
-  },
-  {
-    "$lookup": {
-      "from": "products",
-      "let": {
-        "let_product_id_146741": "$product_id"
-      },
-      "pipeline": [
-        {
-          "$match": {
-            "$expr": {
-              "$eq": [
-                "$$let_product_id_146741",
-                "$_id"
-              ]
-            }
-          }
-        }
-      ],
-      "as": "join_alias_Products"
-    }
-  },
-  {
-    "$unwind": {
-      "path": "$join_alias_Products",
-      "preserveNullAndEmptyArrays": true
-    }
-  },
-  {
-    "$project": {
-      "_id": "$_id",
-      "user_id": "$user_id",
-      "product_id": "$product_id",
-      "created_at": "$created_at",
-      "People__name": "$join_alias_People.name",
-      "People__state": "$join_alias_People.state",
-      "People__source": "$join_alias_People.source",
-      "Products__category": "$join_alias_Products.category",
-      "Products__price": "$join_alias_Products.price",
-      "Products__rating": "$join_alias_Products.rating"
-    }
-  },
-  {
-    "$limit": 1048575
-  }
-]