JSON query in postgres now walks identifier tree if one encountered (fixes #22967) (#23278)

Pursuant to #22967.

Previously the notion of the identifier in json-query in postgres assumed a singular identifier at the top-level of the Identifier record. This is not always the case in complicated binning scenarios - the identifier could also just comprise an entire tree of Identifier record maps with the actual Identifier we're looking to turn into the JSON query in some leaf somewhere.

Therefore, the json-query's determination of what the identifier is now walks that identifier record tree and replaces the identifiers that have JSON field semantics with the reification of ToSql from json-query.

Previous attempt at solution hacked together something at the json-query level, changing the reification, but that only worked for one level down, instead of just walking the Identifier tree and therefore getting stuff at arbitrary tree depth. And we do get nontrivial tree depth in the bucketing in #22967.

src/metabase/driver/postgres.clj

@@ -5,6 +5,7 @@
             [clojure.set :as set]
             [clojure.string :as str]
             [clojure.tools.logging :as log]
+            [clojure.walk :as walk]
             [honeysql.core :as hsql]
             [honeysql.format :as hformat]
             [java-time :as t]
@@ -32,7 +33,8 @@
             [pretty.core :refer [PrettyPrintable]])
   (:import [java.sql ResultSet ResultSetMetaData Time Types]
            [java.time LocalDateTime OffsetDateTime OffsetTime]
-           [java.util Date UUID]))
+           [java.util Date UUID]
+           metabase.util.honeysql_extensions.Identifier))
 
 (comment
   ddl.postgres/keep-me)
@@ -305,11 +307,10 @@
       (format "%s::%s" (pr-str expr) (name psql-type)))))
 
 (defmethod sql.qp/json-query :postgres
-  [_ identifier nfc-field]
+  [_ unwrapped-identifier nfc-field]
   (letfn [(handle-name [x] (if (number? x) (str x) (name x)))]
     (let [field-type           (:database_type nfc-field)
           nfc-path             (:nfc_path nfc-field)
-          unwrapped-identifier (:form identifier)
           parent-identifier    (field/nfc-field->parent-identifier unwrapped-identifier nfc-field)
           names                (format "{%s}" (str/join "," (map handle-name (rest nfc-path))))]
       (reify
@@ -320,11 +321,10 @@
 
 (defmethod sql.qp/->honeysql [:postgres :field]
   [driver [_ id-or-name opts :as clause]]
-  (let [stored-field (when (integer? id-or-name)
-                       (qp.store/field id-or-name))
+  (let [stored-field  (when (integer? id-or-name)
+                        (qp.store/field id-or-name))
         parent-method (get-method sql.qp/->honeysql [:sql :field])
-        identifier    (parent-method driver clause)
-        _nfc-path     (:nfc_path stored-field)]
+        identifier    (parent-method driver clause)]
     (cond
       (= (:database_type stored-field) "money")
       (pg-conversion identifier :numeric)
@@ -332,7 +332,10 @@
       (field/json-field? stored-field)
       (if (::sql.qp/forced-alias opts)
         (keyword (::add/source-alias opts))
-        (sql.qp/json-query :postgres identifier stored-field))
+        (walk/postwalk #(if (instance? Identifier %)
+                          (sql.qp/json-query :postgres % stored-field)
+                          %)
+                       identifier))
 
       :else
       identifier)))

test/metabase/driver/postgres_test.clj

@@ -18,6 +18,7 @@
             [metabase.models.secret :as secret]
             [metabase.models.table :refer [Table]]
             [metabase.query-processor :as qp]
+            [metabase.query-processor.store :as qp.store]
             [metabase.sync :as sync]
             [metabase.sync.sync-metadata :as sync-metadata]
             [metabase.test :as mt]
@@ -286,7 +287,7 @@
       (is (= false (driver/database-supports? :postgres :nested-field-columns no-json-unfold-db))))))
 
 (deftest ^:parallel json-query-test
-  (let [boop-identifier (hx/with-type-info (hx/identifier :field "boop" "bleh -> meh") {})]
+  (let [boop-identifier (:form (hx/with-type-info (hx/identifier :field "boop" "bleh -> meh") {}))]
     (testing "Transforming MBQL query with JSON in it to postgres query works"
       (let [boop-field {:nfc_path [:bleh :meh] :database_type "integer"}]
         (is (= ["(boop.bleh#>> ?::text[])::integer " "{meh}"]
@@ -304,6 +305,30 @@
         (is (= ["(boop.bleh#>> ?::text[])::bigint " "{boop,foobar,1234}"]
                (hsql/format (#'sql.qp/json-query :postgres boop-identifier boolean-boop-field))))))))
 
+(deftest json-field-test
+  (mt/test-driver :postgres
+    (testing "Deal with complicated identifier (#22967)"
+      (let [details   (mt/dbdef->connection-details :postgres :db {:database-name "complicated_identifiers"
+                                                                   :json-unfolding true})]
+        (mt/with-temp* [Database [database  {:engine :postgres, :details details}]
+                        Table    [table     {:db_id (u/the-id database)
+                                             :name "complicated_identifiers"}]
+                        Field    [val-field {:table_id      (u/the-id table)
+                                             :nfc_path      [:jsons "values" "qty"]
+                                             :database_type "integer"}]]
+        (qp.store/with-store
+          (qp.store/fetch-and-store-database! (u/the-id database))
+          (qp.store/fetch-and-store-tables! [(u/the-id table)])
+          (qp.store/fetch-and-store-fields! [(u/the-id val-field)])
+          (let [field-clause [:field (u/the-id val-field) {:binning
+                                                           {:strategy :num-bins,
+                                                            :num-bins 100,
+                                                            :min-value 0.75,
+                                                            :max-value 54.0,
+                                                            :bin-width 0.75}}]]
+            (is (= ["((floor((((complicated_identifiers.jsons#>> ?::text[])::integer  - 0.75) / 0.75)) * 0.75) + 0.75)" "{values,qty}"]
+                   (hsql/format (sql.qp/->honeysql :postgres field-clause)))))))))))
+
 (deftest json-alias-test
   (mt/test-driver :postgres
     (testing "json breakouts and order bys have alias coercion"