Fix deserialization of mongo native query string (#45566) (#45758)

* Fix deserialization of mongo native query string

* Add test

Co-authored-by: lbrdnk <lbrdnk@users.noreply.github.com>

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

@@ -1391,15 +1391,10 @@
   "Parse a serialized native query. Like a normal JSON parse, but handles BSON/MongoDB extended JSON forms."
   [^String s]
   (try
-    ;; TODO: Fixme! In following expression we were previously creating BasicDBObject's. As part of Monger removal
-    ;;       in favor of plain mongo-java-driver we now create Documents. I believe following conversion was and is
-    ;;       responsible for https://github.com/metabase/metabase/issues/38181. When pipeline is deserialized,
-    ;;       we end up with vector of `Document`s into which are appended new query stages, which are clojure
-    ;;       structures. When we render the query in "view native query" in query builder, clojure structures
-    ;;       are transformed to json correctly. But documents are rendered to their string representation (screenshot
-    ;;       in the issue). Possible fix could be to represent native queries in ejson v2, which conforms to json rfc,
-    ;;       hence there would be no need for special bson values handling. That is to be further investigated.
-    (mapv (fn [^org.bson.BsonValue v] (-> v .asDocument org.bson.Document.))
+    ;; Only way to parse _ejson array_ using bson library is through `BsonArray/parse`. That results in sequence
+    ;; of `org.bson.BsonDocument`s. Currently `org.bson.Document` fits our needs better as it (1) implements `Map`
+    ;; and (2) converts `BsonValue`s to java types.
+    (mapv (fn [^org.bson.BsonValue v] (-> v .asDocument .toJson org.bson.Document/parse))
           (org.bson.BsonArray/parse s))
     (catch Throwable e
       (throw (ex-info (tru "Unable to parse query: {0}" (.getMessage e))

modules/drivers/mongo/test/metabase/driver/mongo/execute_test.clj

@@ -6,8 +6,10 @@
    [metabase.driver.mongo.conversion :as mongo.conversion]
    [metabase.driver.mongo.execute :as mongo.execute]
    [metabase.query-processor :as qp]
+   [metabase.query-processor.middleware.cache.impl :as middleware.cache.impl]
    [metabase.query-processor.pipeline :as qp.pipeline]
-   [metabase.test :as mt])
+   [metabase.test :as mt]
+   [toucan2.tools.with-temp :as t2.with-temp])
   (:import
    #_(com.mongodb BasicDBObject)
    (java.util NoSuchElementException)))
@@ -91,3 +93,39 @@
             (is (thrown-with-msg? Throwable
                                   #"Command failed with error 11601.*operation was interrupted"
                                   (qp/process-query query))))))))))
+
+(deftest ^:synchronized question-base-on-native-model-cache-test
+  (testing "Question based on native model is cacheable (#43901)"
+    (mt/test-drivers
+     #{:mongo}
+     (t2.with-temp/with-temp
+       [:model/Card c {:type :model
+                       :dataset_query {:database (mt/id)
+                                       :type     :native
+                                       :native   {:template_tags {}
+                                                  :collection "orders"
+                                                  :query (str "[{\"$addFields\": {}}\n"
+                                                              " {\"$limit\":1}]")}}}]
+       (mt/with-temporary-setting-values [enable-query-caching true]
+         (let [orig-freeze! @#'middleware.cache.impl/freeze!
+               freeze-started (atom false)
+               thrown-data (atom [])]
+           (with-redefs [middleware.cache.impl/freeze! (fn [& args]
+                                                         (reset! freeze-started true)
+                                                         (try
+                                                           (apply orig-freeze! args)
+                                                           (catch Throwable t
+                                                             (swap! thrown-data conj t)
+                                                             (throw t))))]
+             (let [model-based-query (-> (mt/mbql-query orders {:source-table (str "card__" (:id c))})
+                                         (update :cache_strategy assoc
+                                                 ;; Enable caching for current query
+                                                 :avg-execution-time 5000
+                                                 :min_duration_ms 1
+                                                 :multiplier 100000
+                                                 :type :ttl))]
+               (qp/process-query model-based-query)
+               (testing "Sanity: freeze! caching function ran"
+                 (is (true? @freeze-started)))
+               (testing "No exception was thrown during results cache serialization"
+                 (is (zero? (count @thrown-data))))))))))))

modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj

@@ -602,3 +602,9 @@
                            #{}
                            (filter #(contains? % "$lookup") (:query compiled)))]
        (is (= #{1 2 3 4} indices))))))
+
+(deftest ^:parallel parse-query-string-test
+  (testing "`parse-query-string` returns no `Bson...` typed values  (#38181)"
+    ;; ie. parse result does not look as follows: `#object[org.bson.BsonString 0x5f26b3a1 "BsonString{value='1000'}"]`
+    (let [parsed (mongo.qp/parse-query-string "[{\"limit\": \"1000\"}]")]
+      (is (not (instance? org.bson.BsonValue (get-in parsed [0 "limit"])))))))