Fix bug in deserializing JSON from the postgres adapter

The next.jdbc-suggested changes to make JSON decoding automatic apply to
the whole JVM. If they're applied, then decoding metadata off of
features will fail because the adapter has been assuming it's being
handed a raw postgres object and calling `.getValue.`

This patch makes the decoding more defensive, and lets it fail gently,
since right now the only metadata examined at all is the `:expires-at`
key. If we miss decoding this, then we just don't get expiry warnings.

Fixes #3.

Author: rgm

modules/fstore-postgres/src/hyak2/postgres_store.clj

@@ -9,12 +9,13 @@
    highly cacheable, there is optional caching provided by clojure.core.memoize
    to reduce the load on the database."
   (:require
-   [clojure.core.memoize :as memo]
-   [clojure.data.json    :as json]
+   [clojure.core.memoize  :as memo]
+   [clojure.data.json     :as json]
+   [clojure.tools.logging :refer [warn]]
    [hugsql.adapter.next-jdbc]
-   [hugsql.core          :as hugsql]
-   [hyak2.adapter        :as ha]
-   [hyak2.platform       :as hp]
+   [hugsql.core           :as hugsql]
+   [hyak2.adapter         :as ha]
+   [hyak2.platform        :as hp]
    [next.jdbc])
   (:import
    (java.time Instant LocalDateTime ZoneOffset)))
@@ -31,20 +32,43 @@
     (let [inst (Instant/parse s)]
       (LocalDateTime/ofInstant inst ZoneOffset/UTC))))
 
+(defn- decode-feature-meta
+  "Decode metadata off a feature-row, return as a map. The adapter may or may
+   not have set up auto-deserialization of the JSON metadata column.
+
+   See https://cljdoc.org/d/com.github.seancorfield/next.jdbc/1.3.1002/doc/getting-started/tips-tricks#working-with-json-and-jsonb"
+  [feature-row]
+  (let [raw-meta (:metadata feature-row)]
+    (try
+      (cond
+        (map? raw-meta) ;; has already been decoded by something earlier on
+        raw-meta
+
+        (instance? org.postgresql.util.PGobject raw-meta) ;; is still a PGobject
+        (json/read-str (.getValue raw-meta) :key-fn keyword)
+
+        (string? raw-meta)
+        (json/read-str raw-meta :key-fn keyword)
+
+        :else
+        (throw (ex-info "metadata decoding failure" {:feature-row feature-row})))
+      (catch Exception ex
+        (warn (ex-message ex))
+        {}))))
+
 (defn- feature-row->feature
   "Flatten an {:fkey :metadata} row into a flat map with expires-at parsed.
 
    Just decodes JSON manually (instead of setting up auto-convert via
    next.jdbc) so we don't mess with library consumers. "
-  [feature-row]
-  (let [raw-meta (.getValue (:metadata feature-row))
-        metadata (json/read-str raw-meta :key-fn keyword)
-        ?expires-at (:expires-at metadata)]
-    (if ?expires-at
-      (let [expires-at (string->ldt ?expires-at)]
-        (merge metadata {:fkey (:fkey feature-row)
-                         :expires-at expires-at}))
-      (merge metadata {:fkey (:fkey feature-row)}))))
+  ([feature-row]
+   (let [metadata (decode-feature-meta feature-row)
+         ?expires-at (:expires-at metadata)]
+     (if ?expires-at
+       (let [expires-at (string->ldt ?expires-at)]
+         (merge metadata {:fkey (:fkey feature-row)
+                          :expires-at expires-at}))
+       (merge metadata {:fkey (:fkey feature-row)})))))
 
 (let [opts {:adapter (hugsql.adapter.next-jdbc/hugsql-adapter-next-jdbc)
             :quoting :ansi}]

test/hyak2/core_test.cljc

@@ -9,7 +9,8 @@
       [hyak2.memory-store            :as memory-store]
       [hyak2.postgres-store          :as postgres-store]
       [hyak2.redis-store             :as redis-store]
-      [hyak2.platform                :as hp])
+      [hyak2.platform                :as hp]
+      [hyak2.test-helpers.psql       :as test.psql])
      :cljs
      (:require
       [clojure.core.async            :as a]
@@ -38,28 +39,52 @@
 
 (defn run-with-memory-store [test-fn]
   (binding [*fstore* (memory-store/create-fstore!)]
-    (test-fn)))
+    (testing "[memory-store]:"
+      (test-fn))))
 
-#?(:clj (defn run-with-redis-store [test-fn]
-          (let [fstore (redis-store/create-fstore!
-                        "hyak-core2-test"
-                        {:spec {:uri (System/getenv "REDIS_URL")}}
-                        {:clean? true})]
-            (binding [*fstore* fstore]
-              (test-fn))
-            (redis-store/destroy-fstore! fstore)))
+#?(:clj  (defn run-with-redis-store [test-fn]
+           (let [fstore (redis-store/create-fstore!
+                         "hyak-core2-test"
+                         {:spec {:uri (System/getenv "REDIS_URL")}}
+                         {:clean? true})]
+             (binding [*fstore* fstore]
+               (testing "[redis store]:"
+                 (test-fn)))
+             (redis-store/destroy-fstore! fstore)))
    :cljs (defn run-with-redis-store [_]))
 
-#? (:clj (defn run-with-postgres-store
+#?(:clj  (defn run-with-bare-postgres-store
+           ;; this datasource isn't set up for JSON serialization/deserialization
+           ;; so the hyak adapter does its own internal serialization
            ([test-fn]
-            (run-with-postgres-store {:recreate-tables? true} test-fn))
+            (run-with-bare-postgres-store {:recreate-tables? true}
+                                          test-fn))
            ([fstore-opts test-fn]
             (let [jdbc-url (System/getenv "JDBC_DATABASE_URL")
-                  prefix   "hyak_core2_test_"
+                  prefix   "hyak_core2_bare_test_"
                   fstore   (postgres-store/create-fstore! prefix jdbc-url fstore-opts)]
-              (binding [*fstore* fstore]
-                (test-fn)))))
-    :cljs (defn run-with-postgres-store ([_]) ([_ _])))
+              (binding [*fstore* fstore
+                        test.psql/*use-psql-auto-serialization?* false]
+                (testing "[psql bare store]:"
+                  (test-fn))))))
+   :cljs (defn run-with-bare-postgres-store ([_]) ([_ _])))
+
+#?(:clj  (defn run-with-jsonifying-postgres-store
+           ;; this datasource has been rigged to serialize/deserialize JSON
+           ;; so the hyak adapter can presume it's handled by the datasource
+           ([test-fn]
+            (run-with-jsonifying-postgres-store {:recreate-tables? true
+                                                 :disable-serialization? true}
+                                                test-fn))
+           ([fstore-opts test-fn]
+            (let [jdbc-url (System/getenv "JDBC_DATABASE_URL")
+                  prefix   "hyak_core2_json_test_"
+                  fstore   (postgres-store/create-fstore! prefix jdbc-url fstore-opts)]
+              (binding [*fstore* fstore
+                        test.psql/*use-psql-auto-serialization?* true]
+                (testing "[psql json store]:"
+                  (test-fn))))))
+   :cljs (defn run-with-jsonifying-postgres-store ([_]) ([_ _])))
 
 ;; }}}
 
@@ -81,7 +106,8 @@
   (doto add-remove-test
     run-with-memory-store
     run-with-redis-store
-    run-with-postgres-store))
+    run-with-bare-postgres-store
+    run-with-jsonifying-postgres-store))
 
 (defn boolean-gate-test []
   (testing "boolean gates work & enable/disable are idempotent"
@@ -99,10 +125,11 @@
   (doto boolean-gate-test
     run-with-memory-store
     run-with-redis-store
-    run-with-postgres-store))
+    run-with-bare-postgres-store
+    run-with-jsonifying-postgres-store))
 
 (deftest postgres-caching-test
-  (run-with-postgres-store
+  (run-with-bare-postgres-store
    {:recreate-tables? true :ttl/threshold 50}
    (fn []
      (testing "postgres can cache feature tests"
@@ -142,7 +169,8 @@
   (doto actor-gate-test
     run-with-memory-store
     run-with-redis-store
-    run-with-postgres-store))
+    run-with-bare-postgres-store
+    run-with-jsonifying-postgres-store))
 
 (defn group-gate-test []
   (testing "group gates work & enable/disable/reg/dereg are idempotent"
@@ -170,7 +198,8 @@
   (doto group-gate-test
     run-with-memory-store
     run-with-redis-store
-    run-with-postgres-store))
+    run-with-bare-postgres-store
+    run-with-jsonifying-postgres-store))
 
 (defn epsilon= [eps a b] (< (abs (- a b)) eps))
 
@@ -194,7 +223,8 @@
   (doto percent-of-time-test
     run-with-memory-store
     run-with-redis-store
-    (partial run-with-postgres-store {:ttl/threshold 1500})))
+    (partial run-with-bare-postgres-store {:ttl/threshold 1500})
+    (partial run-with-jsonifying-postgres-store {:ttl/threshold 1500})))
 
 (defn percent-of-actors-test []
   (testing "pct-of-actors gate works, is idempotent"
@@ -217,7 +247,8 @@
   (doto percent-of-actors-test
     run-with-memory-store
     run-with-redis-store
-    (partial run-with-postgres-store {:ttl/threshold 1500})))
+    (partial run-with-bare-postgres-store {:ttl/threshold 1500})
+    (partial run-with-jsonifying-postgres-store {:ttl/threshold 1500})))
 
 (deftest expired-test
   (doto
@@ -232,7 +263,8 @@
         (is (not (sut/expired? *fstore* fkey now)))))
     run-with-memory-store
     run-with-redis-store
-    run-with-postgres-store))
+    run-with-bare-postgres-store
+    run-with-jsonifying-postgres-store))
 
 (defn stale-fstore-test []
   (testing "an `expires-at` in the past means fstore is `stale?` and noisy"
@@ -264,6 +296,7 @@
   (doto stale-fstore-test
     run-with-memory-store
     run-with-redis-store
-    run-with-postgres-store))
+    run-with-bare-postgres-store
+    run-with-jsonifying-postgres-store))
 
 ;; vi:fdm=marker

test/hyak2/test_helpers/psql.clj

@@ -0,0 +1,87 @@
+(ns hyak2.test-helpers.psql
+  "Deal with auto-serialization tips and tricks from next.jdbc.
+
+   See https://cljdoc.org/d/com.github.seancorfield/next.jdbc/1.3.1002/doc/getting-started/tips-tricks#working-with-json-and-jsonb
+
+   Problem: this advice proposes extending IPersistentMap, IPersistentVector
+   and org.postgresql.util.PGobject. This happens at the whole-JVM level until
+   restart, but I want test cases that exercise both the possibility that the
+   extension has and hasn't happened.
+
+   Solution: set a dynamic var `*use-psql-auto-serialization?*` that we can
+   flip per-test with `bind` and check it in `x->json` and `json->x` to no-op
+   when I need to, or handle json (de-)serialization when I don't."
+  (:require
+   [jsonista.core        :as json]
+   [next.jdbc.prepare    :as prepare]
+   [next.jdbc.result-set :as rs])
+  (:import
+   (java.sql            PreparedStatement)
+   (org.postgresql.util PGobject)))
+
+(def ^:dynamic *use-psql-auto-serialization?* false)
+
+(def x->json (fn [x]
+               (if *use-psql-auto-serialization?*
+                 (json/write-value-as-string x)
+                 x)))
+
+(def json->x (let [mapper (json/object-mapper {:decode-key-fn keyword})]
+               (fn [json-str-or-x]
+                 (if *use-psql-auto-serialization?*
+                   (json/read-value json-str-or-x mapper)
+                   json-str-or-x))))
+
+(defn clj->pgobject
+  "Transforms Clojure data to a PGobject that contains the data as JSON.
+   PGObject type defaults to `jsonb` but can be changed via metadata key
+   `:pgtype`"
+  [x]
+  (let [pgtype (or (:pgtype (meta x)) "jsonb")]
+    (doto (PGobject.)
+      (.setType pgtype)
+      (.setValue (x->json x)))))
+
+(defn pgobject->clj
+  "Transform PGobject containing `json` or `jsonb` value to Clojure data."
+  [^org.postgresql.util.PGobject v]
+  (let [pg-type         (.getType v)
+        raw-value       (.getValue v)
+        value           (if (#{"jsonb" "json"} pg-type)
+                          (json->x raw-value)
+                          raw-value)
+        value-with-meta (if (instance? clojure.lang.IObj value)
+                          (with-meta value {:pgtype pg-type})
+                          value)]
+    value-with-meta))
+
+(defn init-json-write-helper []
+  ;; if a SQL parameter is a Clojure hash map or vector, it'll be transformed
+  ;; to a PGobject for JSON/JSONB:
+  (extend-protocol next.jdbc.prepare/SettableParameter
+    clojure.lang.IPersistentMap
+    (set-parameter [m ^PreparedStatement s i]
+      (.setObject s i (clj->pgobject m)))
+
+    clojure.lang.IPersistentVector
+    (set-parameter [v ^PreparedStatement s i]
+      (.setObject s i (clj->pgobject v)))))
+
+(defn init-json-read-helper []
+  ;; if a row contains a PGobject then we'll convert them to Clojure data
+  ;; while reading (if column is either "json" or "jsonb" type):
+  (extend-protocol next.jdbc.result-set/ReadableColumn
+    org.postgresql.util.PGobject
+    (read-column-by-label [^org.postgresql.util.PGobject v _]
+      (pgobject->clj v))
+    (read-column-by-index [^org.postgresql.util.PGobject v _2 _3]
+      (pgobject->clj v))))
+
+(defn extend-psql-for-auto-serialization! []
+  (init-json-read-helper)
+  (init-json-write-helper))
+
+;; extend the relevant next.jdbc protocols unconditionally in test, and use the
+;; *use-psql-auto-serialization?* dynamic var to opt in and out with `(bind [,,,])`
+
+(extend-psql-for-auto-serialization!)