paurkedal / ocaml-caqti

Cooperative-threaded access to relational data
https://paurkedal.github.io/ocaml-caqti/index.html
GNU Lesser General Public License v3.0
307 stars 36 forks source link

Failure to connect with Cockroach serverless uri #95

Closed dangdennis closed 2 years ago

dangdennis commented 2 years ago

Issue

Caqti cannot connect to cockroachdb serverless's uri. I suspect it might be that a particular query param is filtered out during uri parsing.

Example

Attempting to connect to cockroach's url at postgresql://user:password@free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&options=--cluster=some-cluster-name-4321 yields

18.09.22 21:27:43.110       dream.log  WARN Called from Caqti_driver_postgresql.Connect_functor.connect.(fun) in file "caqti-driver-postgresql/lib/caqti_driver_postg1
18.09.22 21:27:43.110      dream.http ERROR REQ 4 Failed to connect to <postgresql://dennis:_@free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb?sslmode=v"

18.09.22 21:27:43.110      dream.http ERROR REQ 4 Raised at Stdlib__Map.Make.find in file "map.ml", line 137, characters 10-25
18.09.22 21:27:43.110      dream.http ERROR REQ 4 Called from Logs.Tag.find in file "src/logs.ml", line 154, characters 14-32

Notice that <postgresql://dennis:_@free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb?sslmode=v" misses the expected &options=--cluster=some-cluster-name-4321.

Anyone can create a new cluster for free at https://cockroachlabs.cloud/

If I resolve the issue, I'll create a PR

dangdennis commented 2 years ago

Running the cmd in utop confirms it's some failure in parsing of the options key. If we replace the extra = with its unicode %3D, I still get the same error. Hmm

module Db = (val Caqti_lwt.connect (Uri.of_string "postgresql://dennis:YKXpVzPorCaKY2NaaJH1xw@free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&options=--cluster=saucy-gundi-4894") >>= Caqti_lwt.or_fail |> Lwt_main.run);;
Failed to connect to <postgresql://dennis:_@free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&options=--cluster=saucy-gundi-4894>: Connection failure: extra key/value separator "=" in URI query parameter: "options"

But parsing the uri itself yields the KV params correctly. The = in options is already decoded.

utop # Uri.query b
;;
- : (string * string list) list =
[("sslmode", ["verify-full"]); ("options", ["--cluster=saucy-gundi-4894"])]
paurkedal commented 2 years ago

Are you sure the option and argument shall be separated by a = and not a space (%20)? As for as I can tell, the error comes from either libpq or the remote side, since Caqti passes the URI to ocaml-postgresql which passes it to libpq.

paurkedal commented 2 years ago

Forget my previous post, the problem is rather than the URI-encoding is lost when going though Uri.of_string and Uri.to_string, but it can probably be customized with the Uri.pct_encoder.

paurkedal commented 2 years ago

Can you check if 867049eeb56d1d63d3c06823f781907f9167cf60 (on master) fixes the issue?

dangdennis commented 2 years ago

I'll check this out today. Thank you for the commit @paurkedal.

Aside, can you explain how you locally iterate on the ocaml library? I couldn't figure out the workflow.

My best attempt was to use opam to pin caqti and caqti-driver-postgresql to the local clone of this repo such that my app would supposedly use the locally pinned repo with my changes. That didn't work seem to work. Edits to the local files didn't get picked up, but I suspect it's some minor issue.

On the other hand, the real bugger was that I couldn't get the caqti-driver-postgresql tests to yield any real test results with dune runtest caqti-driver-postgresql -f beyond

Testing `test_postgresql'.
This run has ID `W3JIEGJ5'.

Full test results in `~/Desktop/ocaml-caqti/_build/default/caqti-driver-postgresql/test/_build/_tests/test_postgresql'.
Test Successful in 0.000s. 0 test run.
paurkedal commented 2 years ago

To your first question, opam pin add caqti-driver-postgresql --dev-repo should work, since it's still compatible with the released packages. In general, you might have to pin all Caqti packages you use.

To the second question, the testsuite will only test against URIs listed in the file testsuite/uris.conf, which defaults to sqlite3-only. Put one URI per line.

dangdennis commented 2 years ago

It works! I connected to my serverless crdb cluster and successfully queried some data.