cornucopia-rs
Ergonomic params exploration
I am exploring using a lightweight solution using a trait :
pub trait StringSql: std::fmt::Debug + ToSql + Sync {}
impl StringSql for String {}
impl StringSql for &str {}
impl StringSql for Cow<'_, str> {}
impl StringSql for Box<str> {}
This way we do not use any wrapper or dynamic dispatch, we support all types supported by postgres and we stay strongly typed.
I work well with Option and Vec but can often be less user friendly is some cases:
insert_book()
.bind(client, &None::<&str>, &"Necronomicon")
.unwrap()
Will fix #140
TODO:
- Generate generic params structure for composite types ? (require keeping fields info into
CornucopiaType)
This solution does not work for params struct because of https://github.com/rust-lang/rust/issues/23246 🥲 It makes me want to remove params struct support 😒
I will have more time to work on this issue starting next Monday. In the meantime, thanks a lot for laying this groundwork, I appreciate the effort!
It's impossible to support ergonomic params for composite type params struct without a deep refactoring of our type logic because they can be nested inside other params struct
On reflection, not supporting composite type params with this feature should not be a blocking problem, they are still usable, but less ergonomic. We can support them later.
Having to use a PhantomData for parameter structures is a big no-no for me. The only reason I wanted them is because Rust does not support named arguments. We could replace these structures with a macro that reorders the arguments to call regular functions, which means less code generated for people who don't want this feature and named arguments for those who do. At the same time, this could improve the api by replacing:
Stmt.bind(client, a, b, c) -> Query
Stmt.params(client, params) -> Query
Stmt.bind(client, a, b, c) -> T
Stmt.params(client, params) -> T
with
Stmt.query(client, a, b, c) -> Query
stmt_query!(stmt, client, c: .., a: .., b: ..) -> Query;
Stmt.execute(client, a, b, c) -> T
stmt_execute!(stmt, client, c: .., a: .., b: ..) -> T;
A macro could work fine, but I don't think reordering is possible using only declarative macros. I looked at a tentative approach documented here. It's quite hacky and uses the typed_builder crate.
I might be wrong though! I wouldn't block on reordering either TBH, as long as it enforces exactly the right param names and types.
Are we still keeping the type annotation syntax for parameters? The "type annotation" nomenclature wouldn't really make sense (since there's no actual rust type for the params anymore), but it could still be useful to reduce duplication in the annotations.
Oh! I was able to remove the generic bound from ArraySql by using an associated item :
pub trait ArraySql: std::fmt::Debug + ToSql + Sync {
type Item;
fn slice(&self) -> &[Self::Item];
}
This makes it so that we don't need phantomdata at all, from what I can tell. With just a couple modifications (and removing the phantomdata), I get the test suite to run!
This is the commit I added https://github.com/LouisGariepy/cornucopia/commit/01f018edf0ef49fe15d15634f0e60644d291a656
Is there more work needed to bring this PR to a reviewable state? No hurry, just asking out of curiosity :smile: .
Apart from two remaining limitations, the code is ready for review:
- Fixing SQL and domain escape from iterators: could be solved by using a generic iterator type as we do for Item but as iterators uses a lifetime, I was unable to do this.
- Generate a generic parameter structure for composite types? (needs to keep field information in
CornucopiaType): this is not a blocker but is necessary for this PR to be complete.
@Virgiel
Apart from two remaining limitations, the code is ready for review:
* Fixing SQL and domain escape from iterators: could be solved by using a generic iterator type as we do for Item but as iterators uses a lifetime, I was unable to do this. * Generate a generic parameter structure for composite types? (needs to keep field information in `CornucopiaType`): this is not a blocker but is necessary for this PR to be complete.
After playing with the code and pondering some more, I think I might be over engineering the params struct for not much gain.
Your first point (iterators and lifetimes) would, I think, require GATs, which are still a couple months away at best.
The second point (composite types limitations) only applies to params structs, not to the bind function, is that right?
If so, then your macro solution might be the best solution, as I'd be keen to sidestep all this complexity. I'd like to hear your thoughts on this.
Thanks!
Your first point (iterators and lifetimes) would, I think, require GATs, which are still a couple months away at best.
Yes and this is only problematic when using an iterator params for an array of domains, in that case in will panic at runtime. This behaviour is ok for me, iterator params is an advanced feature.
The second point (composite types limitations) only applies to params structs, not to the bind function, is that right?
When we generate params structure for composite type, we do not yet use ergonomic traits because it require to know the type fields recursively which is not stored in CornucopiaType. This is also ok for me, we could improve that later.
If so, then your macro solution might be the best solution, as I'd be keen to sidestep all this complexity. I'd like to hear your thoughts on this.
I looked into it and writing a macro for this is not really possible for the moment. I think we should document that params struct are a temporary solution for Rust laking named args, and that they are unstable and will be remove one day. It should be considered an advanced feature.
With this PR, the misuse of the bind function creates much less ergonomic errors at compile time, but is more flexible and even supports the use of iterator parameters for arrays, which I really like
Sorry for the delay! I'm still playing with the code on a regular basis. I've made some progress here and there with a different, iterator-based approach, but I cannot say I'm super confident about solving all the limitations. I'm currently stuck in trait bound hell.
As you can probably tell, I'm a bit hesitant to merge this PR. It introduces caveats, reduces error clarity, has potential runtime panics, and has other limitations which is unclear if they can be resolved with the current architecture. On one hand I'd prefer to keep exploring the problem space before committing to a partial solution, but on the other hand I recognize the importance for ergonomics, and a partial solution might be better than nothing.
With that said, I'm willing to go forward and merge if you think this is the right move.
Should we make a json feature for the JsonSql ergonomic trait?
Hmm. I think this can be pretty confusing for users, but at the same time I don't want to force dependencies upon them either. I guess that if we feature-gate the JSON types themselves, might as well feature-gate the JsonSql trait too :shrug: .
Thanks for your work on this PR!
As you can probably tell, I'm a bit hesitant to merge this PR. It introduces caveats, reduces error clarity, has potential runtime panics, and has other limitations which is unclear if they can be resolved with the current architecture. On one hand I'd prefer to keep exploring the problem space before committing to a partial solution, but on the other hand I recognize the importance for ergonomics, and a partial solution might be better than nothing.
I just fixed the potential runtime panic! I agree that the caveats are not easily balanced against the benefits. I feel like we're hitting hard on the limitations of Rust where we're either explicit, verbose and strict, or we have to work around the type system.
Nice :smile: If I'm reading correctly, the only remaining outstanding issue is ergonomic composite types as params? I'm OK with leaving that for another PR!
With JSON feature-gating, this should be good to merge.
I think we should also add test cases to assert that the new domain escaping in params works as intended.
I'll also run benchmarks against main to see if the performance is impacted by these changes. I wouldn't think so, since it's all static dispatch from what I can see, but better be safe than sorry.
I've been using this branch for a while now, and so far, it works perfectly. It has saved me countless repeated lines so far, and I'm sure it will save me countless more in the future. 🚀🎉
With that said, I'd love for this to move forward and become part of the package, as it would, I suspect, benefit many.
@skreborn Thank you for the feeback! This is super valuable :smile:.
I think the consensus is that the benefits outweigh the difficulties brought by the PR. Even if we were to change the implementation, the feature itself seems like a "must-have" for ergonomics, so its a good starting point.
All in all, I am now disposed to merge!
I will do a final read-through, since the PR is pretty hefty and its been a while since I looked at it in detail, but I'll try to do it in a timely manner (today).
Huge thanks to @skreborn for bringing up the issue, and to @Virgiel for making a solid implementation, as always.
Cheers!
There are a couple code nitpicks and organizational changes I'd like to see, but I think we can work on that as we move towards 0.9. The changes can be made in parallel and incrementally without blocking this feature wholly.
For reference, the changes I'm thinking of are roughly as follows:
- Moving client traits out of
lib.rs - Checking trait impls bounds (Do we need
Sendfor ArraySql?) - Finding better abstractions for (and generally refactor) the type registrar and codegen systems. This is to keep everything maintainable, accounting for our present and future feature additions.
This is such a nice solution that improves ergonomics a lot, thank you for this @Virgiel 👍
From my somewhat limited testing I haven't noticed any issues. @LouisGariepy when do you think we'll have a new release with this feature included?
@jacobsvante I don't think there's anything major blocking a new release. I usually try to bundle at least a couple PRs to reduce version churn, but we could probably make an exception here since this is a pretty significant improvement that many users could benefit from.
On the implementation side, the consensus is that we need to rework some parts to make the complexity more manageable, but I don't necessarily view that as a blocker.
@Virgiel thoughts on releasing 0.9 with this standalone PR?
Great to hear @LouisGariepy 👍
Btw I would love to see an equivalent of StringSql implemented for number types as well (i32, i64 etc). This would be very useful when passing in number based newtypes such as struct Id(i32) to the *Params struct (if the newtype implements ToSql itself, of course).
@LouisGariepy I would prefer to merge #160 and #161, and to make a decision about #154 before the next release.