Use counter instead of `gensym` to make mongo field aliases unique (#37189)

* Add test

* Use atom to track next alias index

* Update testing string

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

* Use volatile instead of atom for index tracking

---------

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

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

@@ -105,6 +105,18 @@
   That is required eg. in `->lvalue :aggregation` call."
   0)
 
+(def ^:dynamic ^:private *next-alias-index*
+  "Tracks index of next alias for join compilation. It is bound in [[mbql->native]] to `volatile!` valued 0. Hence
+   every compilation starts with a fresh 0. Indices are used in [[handle-join]] to make aliases unique. Index values
+   are gathered using [[next-alias-index]], hence first used index is of value 1."
+  nil)
+
+(defn- next-alias-index
+  "Increment [[*next-alias-index*]] counter and return new index. Further context can be found in
+   [[*next-alias-index*]] docstring."
+  []
+  (vswap! *next-alias-index* inc))
+
 (def ^:dynamic ^:private *field-mappings*
   "The mapping from the fields to the projected names created
   by the nested query."
@@ -908,12 +920,11 @@
                      [:field _ (_ :guard #(not= (:join-alias %) alias))])
         ;; Map the own fields to a fresh alias and to its rvalue.
         mapping (map (fn [f] (let [alias (-> (format "let_%s_" (->lvalue f))
-                                            ;; ~ in let aliases provokes a parse error in Mongo. For correct function,
-                                            ;; aliases should also contain no . characters (#32182).
-                                            (str/replace #"~|\." "_")
-                                            gensym
-                                            name)]
-                              {:field f, :rvalue (->rvalue f), :alias alias}))
+                                             ;; ~ in let aliases provokes a parse error in Mongo. For correct function,
+                                             ;; aliases should also contain no . characters (#32182).
+                                             (str/replace #"~|\." "_")
+                                             (str "__" (next-alias-index)))]
+                               {:field f, :rvalue (->rvalue f), :alias alias}))
                      own-fields)]
     ;; Add the mappings from the source query and the let bindings of $lookup to the field mappings.
     ;; In the join pipeline the let bindings have to referenced with the prefix $$, so we add $ to the name.
@@ -1395,7 +1406,8 @@
   "Compile an MBQL query."
   [query]
   (let [query (update query :query preprocess)]
-    (binding [*query* query]
+    (binding [*query* query
+              *next-alias-index* (volatile! 0)]
       (let [source-table-name (if-let [source-table-id (mbql.u/query->source-table-id query)]
                                 (:name (lib.metadata/table (qp.store/metadata-provider) source-table-id))
                                 (query->collection-name query))

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

@@ -15,6 +15,8 @@
    [metabase.util :as u]
    [toucan2.core :as t2]))
 
+(set! *warn-on-reflection* true)
+
 (deftest ^:parallel query->collection-name-test
   (testing "query->collection-name"
     (testing "should be able to extract :collection from :source-query")
@@ -532,3 +534,38 @@
 
         {"$expr" {"$eq" ["$price" {"$add" [{"$subtract" ["$price" 5]} 100]}]}}
         [:= $price [:+ [:- $price 5] 100]]))))
+
+(deftest uniqe-alias-index-test
+  (mt/test-driver
+   :mongo
+   (testing "Field aliases have deterministic unique indices"
+     (let [query (mt/mbql-query
+                  nil
+                  {:joins [{:alias "Products"
+                            :source-table $$products
+                            :condition [:= &Products.products.id $orders.product_id]
+                            :fields :all}
+                           {:alias "People"
+                            :source-table $$people
+                            :condition [:= &People.people.id $orders.user_id]
+                            :fields :all}]
+                   :source-query {:source-table $$orders
+                                  :joins [{:alias "Products"
+                                           :source-table $$products
+                                           :condition [:= &Products.products.id $orders.product_id]
+                                           :fields :all}
+                                          {:alias "People"
+                                           :source-table $$people
+                                           :condition [:= &People.people.id $orders.user_id]
+                                           :fields :all}]}})
+           compiled (qp/compile query)
+           indices (reduce (fn [acc lookup-stage]
+                             (let [let-var-name (-> (get-in lookup-stage ["$lookup" :let]) keys first)
+                                   ;; Following expression ensures index is an integer.
+                                   index (Integer/parseInt (re-find #"\d+$" let-var-name))]
+                               ;; Following expression tests that index is unique.
+                               (is (not (contains? acc index)))
+                               (conj acc index)))
+                           #{}
+                           (filter #(contains? % "$lookup") (:query compiled)))]
+       (is (= #{1 2 3 4} indices))))))