paurkedal / ocaml-caqti

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

Add a printf-like `Caqti_query.qprintf` for dynamic queries #103

Closed bclement-ocp closed 1 year ago

bclement-ocp commented 1 year ago

Hi, and thanks for building and maintaining this project :)

As I was playing around I realized that there is some tension when building dynamic queries between the recommended way of using Caqti_query.t directly and the "convenient" way of building a query string with Format. I ended up writing a qprintf function that is like asprintf, but generates Caqti_query.t instead of strings. A more detailed rationale is in the commit message and replicated below; I find it fairly useful and thought others might too.

Sometimes, you need to construct queries dynamically rather than writing them out in code. Building an ORM would be an obvious reason for doing that, but it is quite often that one has to sprinkle a little bit of dynamism here and there.

In the case that one has to generate queries dynamically, the most obvious way coming from standard Caqti usage is to build up a query string fragment using Format or Printf, then use it as if you had written it manually. Simple. But then what happens is that Caqti will take the string that you have built dynamically, and parse it back into a Caqti_query.t before using the fragments to build the actual query string.

That doesn't sound very efficient, and relies on you (as the dynamic-query-generator) not messing up the generation of things like quoted strings if you don't want to expose security holes in your software. In fact, Caqti's documentation quietly recommends using the Caqti_query.t type directly when building dynamic queries, touting its easy composability.

While true (Caqti_query.t is easily composable), it loses out on convenience compared to the Format + parse approach. For instance, if you want to generate, say, the MAX of two dynamically generated queries, you have to write S [ L "MAX("; q1; L ", "; q2; L ")" ]. Maybe if you are keen on combinators, you can figure out how to write a somewhat more readable version such as l "MAX(" %% q1 % ", " %% q2 % ")". But you never get something as nice as the "MAX(%a, %a)" that you get with Format.

Or at least, that's what I thought before realizing that format6 is actually very polymorphic. It has to, so that it can accomodate the whole family of *printf functions. And it turns out that it is polymorphic enough to write a printf-style function to build a Caqti_query.t.

So here is qprintf. Like sprintf, but builds a Caqti_query.t instead of a string. You can delimit quotes and environment variables using tags (@{<Q>the quote@} and @{<E>env.var@}, respectively), and include parameters with param.

This gives a convenient, readable and secure (w.r.t. quoting) way of building queries dynamically, which I thought was worth sharing.

paurkedal commented 1 year ago

Looks like a great addition. I'll take a closer look when I'm back from a conference. There is a typo "tagsm" in the documentation, and I'd also like to ask you to make the indentation of the doc strings consistent (4 spaces). And we should extend test_query.ml, maybe just a few check_expect to cover the various features. I can also do the latter when I'm back.

bclement-ocp commented 1 year ago

Weird, I thought I already fixed that for the docstring indentation — I must have messed something up. All these should be fixed now.

bclement-ocp commented 1 year ago

The test failure is in caqti-lwt/test-unix/test_pool_lwt.ml and seems unrelated.

paurkedal commented 1 year ago

Yes, ignore the pool error. I hoped I had resolved it, but apparently not.

So this looks good now. Thanks! Can I credit you by full name (changelog and authors list), or do you have other preference?

paurkedal commented 1 year ago

I pushed my proposed update to the change log and authors; we can force update if you want it changed.

bclement-ocp commented 1 year ago

Yes, full name is fine.

paurkedal commented 1 year ago

Thanks again!