Make Pgx_value.t opaque

It's not safe to use the string contents directly, so this makes
Pgx_value.t opaque.

The biggest issue here is that the string and binary representations
can be different for the same data, but there are weird issues
with using the raw representation for floats and other conversions
as well.

pgx/src/pgx_value.ml

@@ -6,7 +6,8 @@ let compare_int = Int.compare
 let compare_option = Stdlib.Option.compare
 let compare_list = Base.compare_list
 
-type t = string option [@@deriving compare, sexp_of]
+type v = string [@@deriving compare, sexp_of]
+type t = v option [@@deriving compare, sexp_of]
 
 exception Conversion_failure of string [@@deriving sexp_of]
 

pgx/src/pgx_value_intf.ml

@@ -1,6 +1,12 @@
 (** A wrapper for holding Postgres types *)
 module type S = sig
-  type t = string option [@@deriving compare, sexp_of]
+  (** [v] is opaque because the exact contents depend on Postgres types, so you could have two [v]'s with the
+      same value but different internal data representation, for example if you did a [SELECT 'a'::bytea] vs
+      [SELECT 'a'::varchar], the internal representation will be different, but the actual data if you use
+      [to_binary] or [to_string] will be the same. *)
+  type v [@@deriving compare, sexp_of]
+
+  type t = v option [@@deriving compare, sexp_of]
 
   exception Conversion_failure of string [@@deriving sexp_of]
 

pgx/test/test_pgx_value.ml

@@ -9,7 +9,7 @@ let equal_value (x : t) (y : t) = x = y
 let pp_hstore ppf x = Sexp.pp_hum ppf (sexp_of_hstore x)
 let equal_hstore x y = Sexp.equal (sexp_of_hstore x) (sexp_of_hstore y)
 let printer sexp value = sexp value |> Sexp.to_string_hum
-let sort_hstore = List.sort (fun (k, _) (k', _) -> compare k k')
+let sort_hstore = List.sort (fun (k, _) (k', _) -> String.compare k k')
 let to_hstore_sorted v = to_hstore v |> Option.map sort_hstore
 let to_hstore_sorted_exn v = to_hstore_exn v |> sort_hstore
 let pp_inet ppf (addr, port) = Format.fprintf ppf "%a:%d" Ipaddr.pp addr port

pgx_test/src/pgx_test.ml

@@ -16,10 +16,17 @@ end
 
 module Alcotest_ext = struct
   let uuid = Alcotest.testable Uuidm.pp Uuidm.equal
+
+  let pgx_value =
+    Alcotest.testable
+      (fun fmt t ->
+        Pgx_value.sexp_of_t t |> Sexplib0.Sexp.to_string_hum |> Format.pp_print_string fmt)
+      (fun a b -> Pgx_value.compare a b = 0)
+  ;;
 end
 
-let check_result = Alcotest.(check (list (list (option string))))
-let check_results = Alcotest.(check (list (list (list (option string)))))
+let check_result = Alcotest.(check (list (list Alcotest_ext.pgx_value)))
+let check_results = Alcotest.(check (list (list (list Alcotest_ext.pgx_value))))
 
 module Make_tests
     (Pgx_impl : Pgx.S)
@@ -117,19 +124,24 @@ struct
       ; Alcotest_io.test_case "query - 1 query" `Quick (fun () ->
             with_conn (fun dbh ->
                 simple_query dbh "select 1"
-                >>| check_results "select 1" [ [ [ Some "1" ] ] ]))
+                >>| check_results "select 1" [ [ [ Pgx.Value.of_string "1" ] ] ]))
       ; Alcotest_io.test_case "query - multiple" `Quick (fun () ->
             with_conn (fun dbh ->
                 simple_query dbh "select 1; select 2; select 3"
                 >>| check_results
                       "select three"
-                      [ [ [ Some "1" ] ]; [ [ Some "2" ] ]; [ [ Some "3" ] ] ]))
+                      Pgx.Value.
+                        [ [ [ of_string "1" ] ]
+                        ; [ [ of_string "2" ] ]
+                        ; [ [ of_string "3" ] ]
+                        ]))
       ; Alcotest_io.test_case "query - multiple single query" `Quick (fun () ->
             with_conn (fun dbh ->
                 simple_query dbh "select 1 union all select 2 union all select 3"
                 >>| check_results
                       "select unit all"
-                      [ [ [ Some "1" ]; [ Some "2" ]; [ Some "3" ] ] ]))
+                      Pgx.Value.
+                        [ [ [ of_string "1" ]; [ of_string "2" ]; [ of_string "3" ] ] ]))
       ; Alcotest_io.test_case "query - empty" `Quick (fun () ->
             with_conn (fun dbh -> simple_query dbh "" >>| check_results "empty query" []))
       ; Alcotest_io.test_case
@@ -157,14 +169,16 @@ struct
                   execute_fold s ~params:[] ~init:[] ~f:(fun acc a -> return (a :: acc))))
             >>| check_result
                   "fold values"
-                  [ [ Some "3"; Some "4" ]; [ Some "1"; Some "2" ] ])
+                  Pgx.Value.
+                    [ [ of_string "3"; of_string "4" ]; [ of_string "1"; of_string "2" ] ])
       ; Alcotest_io.test_case "test execute_prepared" `Quick (fun () ->
             with_conn
             @@ fun dbh ->
             Prepared.(prepare dbh ~query:"values (1,2),(3,4)" >>= execute ~params:[])
             >>| check_result
                   "prepare & execute"
-                  [ [ Some "1"; Some "2" ]; [ Some "3"; Some "4" ] ])
+                  Pgx.Value.
+                    [ [ of_string "1"; of_string "2" ]; [ of_string "3"; of_string "4" ] ])
       ; Alcotest_io.test_case "test execute_iter" `Quick (fun () ->
             let n = ref 0 in
             let rows = Array.make 2 [] in
@@ -178,26 +192,27 @@ struct
             Array.to_list rows
             |> check_result
                  "execute_iter"
-                 [ [ Some "1"; Some "2" ]; [ Some "3"; Some "4" ] ])
+                 Pgx.Value.
+                   [ [ of_string "1"; of_string "2" ]; [ of_string "3"; of_string "4" ] ])
       ; Alcotest_io.test_case "with_prepare" `Quick (fun () ->
             with_conn
             @@ fun dbh ->
             let name = "with_prepare" in
             Prepared.(
               with_prepare dbh ~name ~query:"values ($1)" ~f:(fun s ->
-                  execute s ~params:[ Some "test" ]))
-            >>| check_result name [ [ Some "test" ] ])
+                  execute s ~params:Pgx.Value.[ of_string "test" ]))
+            >>| check_result name Pgx.Value.[ [ of_string "test" ] ])
       ; Alcotest_io.test_case "interleave unnamed prepares" `Quick (fun () ->
             with_conn
             @@ fun dbh ->
             let open Prepared in
             with_prepare dbh ~query:"values ($1)" ~f:(fun s1 ->
                 with_prepare dbh ~query:"values (1)" ~f:(fun s2 ->
-                    execute s1 ~params:[ Some "test" ]
+                    execute s1 ~params:Pgx.Value.[ of_string "test" ]
                     >>= fun r1 -> execute s2 ~params:[] >>| fun r2 -> r1, r2))
             >>| fun (r1, r2) ->
-            check_result "outer prepare" [ [ Some "test" ] ] r1;
-            check_result "inner prepare" [ [ Some "1" ] ] r2)
+            check_result "outer prepare" Pgx.Value.[ [ of_string "test" ] ] r1;
+            check_result "inner prepare" Pgx.Value.[ [ of_string "1" ] ] r2)
       ; Alcotest_io.test_case "in_transaction invariant" `Quick (fun () ->
             with_conn
             @@ fun dbh ->
@@ -224,12 +239,18 @@ struct
             | Error (Pgx.PostgreSQL_Error _) -> ()
             | Error exn -> reraise exn)
       ; Alcotest_io.test_case "execute_many function" `Quick (fun () ->
-            let params = [ [ Some "1" ]; [ Some "2" ]; [ Some "3" ] ] in
+            let params =
+              Pgx.Value.[ [ of_string "1" ]; [ of_string "2" ]; [ of_string "3" ] ]
+            in
             with_conn (fun dbh ->
                 execute_many dbh ~query:"select $1::int" ~params
                 >>| check_results
                       "execute_many result"
-                      [ [ [ Some "1" ] ]; [ [ Some "2" ] ]; [ [ Some "3" ] ] ]))
+                      Pgx.Value.
+                        [ [ [ of_string "1" ] ]
+                        ; [ [ of_string "2" ] ]
+                        ; [ [ of_string "3" ] ]
+                        ]))
       ; Alcotest_io.test_case "query with SET" `Quick (fun () ->
             with_conn (fun dbh ->
                 simple_query dbh "SET LOCAL TIME ZONE 'Europe/Rome'; SELECT 'x'"
@@ -261,10 +282,13 @@ struct
                    (DELIMITER '|')"
                 >>| check_results
                       "copy out result"
-                      [ []
-                      ; []
-                      ; [ [ Some "Roger Federer|19\n" ]; [ Some "Rafael Nadal|15\n" ] ]
-                      ]))
+                      Pgx.Value.
+                        [ []
+                        ; []
+                        ; [ [ of_string "Roger Federer|19\n" ]
+                          ; [ of_string "Rafael Nadal|15\n" ]
+                          ]
+                        ]))
       ; Alcotest_io.test_case "copy out extended query" `Quick (fun () ->
             with_temp_db (fun dbh ~db_name:_ ->
                 execute
@@ -279,7 +303,10 @@ struct
                 >>= fun _ -> execute dbh "COPY tennis_greats TO STDOUT (DELIMITER '|')")
             >>| check_result
                   "copy out extended result"
-                  [ [ Some "Roger Federer|19\n" ]; [ Some "Rafael Nadal|15\n" ] ])
+                  Pgx.Value.
+                    [ [ of_string "Roger Federer|19\n" ]
+                    ; [ of_string "Rafael Nadal|15\n" ]
+                    ])
       ; Alcotest_io.test_case "execute_prepared_iter and transact test" `Quick (fun () ->
             with_temp_db (fun dbh ~db_name:_ ->
                 with_transaction dbh (fun dbh ->
@@ -307,7 +334,7 @@ struct
                         >>= fun () -> return !acc))
                 >>| check_result
                       "prepare & transact result"
-                      [ [ Some "Roger Federer"; Some "19" ] ]))
+                      Pgx.Value.[ [ of_string "Roger Federer"; of_string "19" ] ]))
       ; Alcotest_io.test_case "commit while not in transaction" `Quick (fun () ->
             try_with (fun () ->
                 with_conn
@@ -354,12 +381,12 @@ struct
                     let acc = ref [] in
                     execute_iter
                       s
-                      ~params:[ Some "Andy Murray"; Some "3" ]
+                      ~params:Pgx.Value.[ of_string "Andy Murray"; of_string "3" ]
                       ~f:(fun fields -> return (acc := fields :: !acc))
                     >>= fun () -> return !acc)
                 >>| check_result
                       "isolation query result"
-                      [ [ Some "Andy Murray"; Some "3" ] ]))
+                      Pgx.Value.[ [ of_string "Andy Murray"; of_string "3" ] ]))
       ; Alcotest_io.test_case "multi typed table" `Quick (fun () ->
             with_temp_db (fun dbh ~db_name:_ ->
                 simple_query

pgx_value_core/src/pgx_value_core.ml

@@ -18,7 +18,7 @@ let of_time t =
     2016-06-07 15:37:46Z (utc timezone) -> 2016-06-07 11:37:46-04
     2016-06-07 15:37:46-04 (local timezone) -> 2016-06-07 15:37:46-04
   *)
-  Some (Time.to_string_abs ~zone:Time.Zone.utc t)
+  Time.to_string_abs ~zone:Time.Zone.utc t |> Pgx.Value.of_string
 ;;
 
 let to_time' =
@@ -55,9 +55,9 @@ let to_time' =
     | _ -> convert_failure "time" s
 ;;
 
-let to_time_exn = required to_time'
-let to_time = Option.map ~f:to_time'
-let of_date d = Some (Date.to_string d)
+let to_time_exn v = Pgx.Value.to_string_exn v |> to_time'
+let to_time v = Pgx.Value.to_string v |> Option.map ~f:to_time'
+let of_date d = Date.to_string d |> Pgx.Value.of_string
 let to_date' = Date.of_string
-let to_date_exn = required to_date'
-let to_date = Option.map ~f:to_date'
+let to_date_exn v = Pgx.Value.to_string_exn v |> to_date'
+let to_date v = Pgx.Value.to_string v |> Option.map ~f:to_date'

pgx_value_core/src/pgx_value_core.mli

@@ -1,7 +1,10 @@
 (** Pgx_value types using Core_kernel's Date and Time modules *)
 open Core_kernel
 
-include Pgx_value_intf.S
+type v = Pgx_value.v [@@deriving compare, sexp_of]
+type t = Pgx_value.t [@@deriving compare, sexp_of]
+
+include Pgx_value_intf.S with type v := v and type t := t
 
 val of_date : Date.t -> t
 val to_date_exn : t -> Date.t

pgx_value_core/test/test_pgx_value_core.ml

@@ -1,7 +1,7 @@
 open Core_kernel
 module Value = Pgx_value_core
 
-let time_roundtrip str = Value.to_time_exn (Some str)
+let time_roundtrip str = Value.of_string str |> Value.to_time_exn
 let printer = Time.to_string_abs ~zone:Time.Zone.utc
 
 let time_testable =