Clearer query API

[LouisGariepy]

Non-returning statements have this peculiar property that they are executed right after binding their parameters. Meanwhile, statements that return something are only executed after using one of the finalizers.

I'd like to explore the idea of adding an execute finalizer for non-returning statements.

Originally suggested here https://github.com/cornucopia-rs/cornucopia/discussions/88#discussioncomment-3656850.

[jacobsvante]

This will be a great addition for clarity IMO 👍

[Virgiel]

It is hard to create ergonomic API with the strictness of Rust. Ultimately we would have this :

insert_book().execute(&client, &"The Great Gatsby").await?;
insert_book().execute(&client, name: &"The Great Gatsby").await?;

But because Rust does not support named arguments neither function overloading we need to have two variant :

insert_book().args(&client, &"The Great Gatsby").await?;
insert_book().struct(&client, Params { name: &"The Great Gatsby" }).await?;

We could use generic functions allowing either struct or tuple, but we would lose some tooling ergonomics:

insert_book().execute(&client, (&"The Great Gatsby")).await?;
insert_book().execute(&client, Params { name: &"The Great Gatsby" }).await?;

[jacobsvante]

I don't know what makes it hard to do in Rust because of its strictness as I don't really know the internals of cornucopia, but I'm sure you're right.

Looking at your examples I think execute() would be closer to the current API, if you're not changing anything else. Perhaps it should even be called bind_execute() to make it completely clear. This would go well with the struct equivalent to bind() – params() – which would then be called params_execute().

[LouisGariepy]

Originally, I thought the suggestion was something like

// this (proposed)
insert_book().bind(&client, &"The Great Gatsby").execute().await?;
// instead of that (current)
insert_book().execute(&client, &"The Great Gatsby").await?;

This has the benefit to make all queries have a consistent api query_name().bind(...).finalizer(), where finalizer is a method that extracts a number of rows (e.g. vec, one, etc.).

I might have misunderstood the suggestion completely though (sorry if this is the case!). I'll amend the issue title if this the case, just let me know.

[Virgiel]

I see how this make the code closer to the SQL semantics, but I think we can resolve this without making the syntax more verbose or generating new types with a simple naming change.

What do you think of this :

insert_book().execute(&client, &"The Great Gatsby").await?;
insert_book().execute_struct(&client, Params { name: &"The Great Gatsby" }).await?;

select_books().query(&client, &"The Great Gatsby").vec().await?;
select_books().query_struct(&client, Params { name: &"The Great Gatsby" }).vec().await?;

Compared to our current syntax:

insert_book().bind(&client, &"The Great Gatsby").await?;
insert_book().params(&client, Params { name: &"The Great Gatsby" }).await?;

select_books().bind(&client, &"The Great Gatsby").vec().await?;
select_books().params(&client, Params { name: &"The Great Gatsby" }).vec().await?;

[Virgiel]

execute_struct and query_struct are a little verbose and misleading, so I welcome any naming proposition. We could do something similar to rfind for 'reverse find' in std with pexecute and pquery for 'params execute' and 'params query'.

[LouisGariepy]

As discussed, we should take a stance on this before releasing the next version. Adding to the milestone.

[jacobsvante]

Personally I prefer a "release often" mentality as releases can get stuck for a long time while waiting for a few PRs (e.g clap v3). I would recommend deferring until 0.10.

[LouisGariepy]

@jacobsvante I just realized that GATs are coming in two days from now, so I think we should postpone any query API changes until then, since that might shake things up a lot.

In general though, I agree we should settle on a more regular and structured release schedule. For now, I moved some items to 0.10 to make 0.9 a bit lighter. I'd also like to add a changelog to the project.

Sorry for the off-topic comments, I'll get back to your proposal soon @Virgiel.

[jacobsvante]

I was also thinking about GATs, so excited! 😀

[LouisGariepy]

Queries which have a return value

I prefer our current bind compared to query. One reason is that query suggest that we are declaring a query (a lot of DB crates use this terminology), but in cornucopia, the query is already defined. What it is really doing is binding the parameters to the predefined query, as the current name suggest. This is also how SQLx uses bind, so it should be familiar to new users.

Queries which do not have a return value

I think execute instead of our current bind is slightly better, but not by much in terms of clarity (should we have a bind method that also executes, or an execute method that also binds).

Proposition

I think the clearest API would be something like

insert_book()
    .bind(&client, &"The Great Gatsby")
    .execute()
    .await?;
    
insert_book()
    .bind_struct(&client, Params { name: &"The Great Gatsby" })
    .execute()
    .await?;

select_books()
    .bind(&client, &"The Great Gatsby")
    .all()
    .await?;
    
select_books()
    .bind_struct(&client, Params { name: &"The Great Gatsby" })
    .all()
    .await?;

This makes the API predictable and consistent.

I'd like to hear your thoughts on this.

[jacobsvante]

I agree with your reasoning, I think we should go that route! Great work!

Personally I would prefer .all() over .vec() because I think it's more conventional (at least from query APIs I've been using before).

(Also I think .opt() and .one() should be kept as is, but I don't think that's been up for debate..!)

[LouisGariepy]

Oh! I agree about vec too, I think we should keep all. (I copy pasted without looking hard enough :x). I'll edit my comment above.