22831 fix (group-by time bucketing on replacements for json aliases) (#24545)

* rewrite here...

* no bq

* thinking

* conjunction of alias thing...

* make sure we dont wanna see it

* nit

* multi-arity func

* fiddly bit on test

* add bit to see order by works right

* no default breakout true

src/metabase/driver/postgres.clj

@@ -360,7 +360,7 @@
         qualified        (parent-method query)
         unqualified      (parent-method (update query
                                                 :breakout
-                                                sql.qp/rewrite-fields-to-force-using-column-aliases))]
+                                                #(sql.qp/rewrite-fields-to-force-using-column-aliases % {:is-breakout true})))]
     (if (some field/json-field? stored-fields)
       (merge qualified
              (select-keys unqualified #{:group-by}))

src/metabase/driver/sql/query_processor.clj

@@ -621,19 +621,24 @@
 
 (defn rewrite-fields-to-force-using-column-aliases
   "Rewrite `:field` clauses to force them to use the column alias regardless of where they appear."
-  [form]
-  (mbql.u/replace form
-    [:field id-or-name opts]
-    [:field id-or-name (-> opts
-                           (assoc ::add/source-alias        (::add/desired-alias opts)
-                                  ::add/source-table        ::add/none
-                                  ;; sort of a HACK but this key will tell the SQL QP not to apply casting here either.
-                                  ::nest-query/outer-select true
-                                  ;; used to indicate that this is a forced alias
-                                  ::forced-alias            true)
-                           ;; don't want to do temporal bucketing or binning inside the order by or breakout either.
-                           ;; That happens inside the `SELECT`
-                           (dissoc :temporal-unit :binning))]))
+  ([form]
+   (rewrite-fields-to-force-using-column-aliases form {:is-breakout false}))
+  ([form {is-breakout :is-breakout}]
+   (mbql.u/replace form
+     [:field id-or-name opts]
+     [:field id-or-name (cond-> opts
+                          true
+                          (assoc ::add/source-alias        (::add/desired-alias opts)
+                                 ::add/source-table        ::add/none
+                                 ;; sort of a HACK but this key will tell the SQL QP not to apply casting here either.
+                                 ::nest-query/outer-select true
+                                 ;; used to indicate that this is a forced alias
+                                 ::forced-alias            true)
+                          ;; don't want to do temporal bucketing or binning inside the order by only.
+                          ;; That happens inside the `SELECT`
+                          ;; (#22831) however, we do want it in breakout
+                          (not is-breakout)
+                          (dissoc :temporal-unit :binning))])))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                Clause Handlers                                                 |

test/metabase/driver/postgres_test.clj

@@ -348,17 +348,33 @@
                                                           "injection' OR 1=1--' AND released = 1"
                                                           (keyword "injection' OR 1=1--' AND released = 1")],
                                                :name     "json_alias_test"}]]
-          (let [compile-res (qp/compile
+          (let [field-bucketed [:field (u/the-id field)
+                                {:temporal-unit :month,
+                                 :metabase.query-processor.util.add-alias-info/source-table (u/the-id table),
+                                 :metabase.query-processor.util.add-alias-info/source-alias "dontwannaseethis",
+                                 :metabase.query-processor.util.add-alias-info/desired-alias "dontwannaseethis",
+                                 :metabase.query-processor.util.add-alias-info/position 1}]
+                field-ordinary [:field (u/the-id field) nil]
+                compile-res (qp/compile
                               {:database (u/the-id database)
                                :type     :query
                                :query    {:source-table (u/the-id table)
                                           :aggregation  [[:count]]
-                                          :breakout     [[:field (u/the-id field) nil]]}})]
-            (is (= (str "SELECT (\"json_alias_test\".\"bob\"#>> ?::text[])::VARCHAR  "
+                                          :breakout     [field-bucketed]
+                                          :order-by     [[:asc field-bucketed]]}})
+                only-order  (qp/compile
+                              {:database (u/the-id database)
+                               :type     :query
+                               :query    {:source-table (u/the-id table)
+                                          :order-by     [[:asc field-ordinary]]}})]
+            (is (= (str "SELECT date_trunc('month', CAST((\"json_alias_test\".\"bob\"#>> ?::text[])::VARCHAR  AS timestamp)) "
                         "AS \"json_alias_test\", count(*) AS \"count\" FROM \"json_alias_test\" "
                         "GROUP BY \"json_alias_test\" ORDER BY \"json_alias_test\" ASC")
                    (:query compile-res)))
-            (is (= '("{injection' OR 1=1--' AND released = 1,injection' OR 1=1--' AND released = 1}") (:params compile-res)))))))))
+            (is (= '("{injection' OR 1=1--' AND released = 1,injection' OR 1=1--' AND released = 1}") (:params compile-res)))
+            (is (= (str "SELECT (\"json_alias_test\".\"bob\"#>> ?::text[])::VARCHAR  AS \"json_alias_test\" FROM \"json_alias_test\" "
+                        "ORDER BY \"json_alias_test\" ASC LIMIT 1048575")
+                   (:query only-order)))))))))
 
 (deftest describe-nested-field-columns-test
   (mt/test-driver :postgres