-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Query builder #1780
Add Query builder #1780
Conversation
* Make query_builder.rs in sqlx-core * Add QueryBuilder::new() * Add QueryBuilder::push() * Define questions for documentation * Get new, push, push_bind working with types * Handle postgres' numbered bind varaibles * Add a test for QueryBuilder#build * Move arguments into Option * Refactor query builder * Finish testing QueryBuilder#build * Remove design doc * Add a test for pushing strings with push_bind * Integration test green * Adjust some tests * Make query builder generic about placeholder segmenent ('$N' or '?')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a decent starting point but I have some nits.
sqlx-core/src/arguments.rs
Outdated
@@ -16,6 +16,10 @@ pub trait Arguments<'q>: Send + Sized + Default { | |||
fn add<T>(&mut self, value: T) | |||
where | |||
T: 'q + Send + Encode<'q, Self::Database> + Type<Self::Database>; | |||
|
|||
fn place_holder(&self, _argument_count: u16) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: argument_count
shouldn't be necessary as the Arguments
impl should already know how many arguments have been pushed. Personally, I also think "placeholder" should be one word, not two.
The intermediate String
is also a wasted allocation. An alternative signature might look like this:
use std::fmt::{Self, Write};
fn format_placeholder<W: Write>(&self, mut writer: W) -> fmt::Result {
writer.write_str("?")
}
and the Postgres implementation would look like this:
fn format_placeholder<W: Write>(&self, mut writer: W) -> fmt::Result {
write!(writer, "${}", self.count)
}
QueryBuilder
would then simply invoke it like:
// This is highly unlikely to return an error so panicking is fine.
arguments.format_placeholder(&mut self.query).expect("error in format_placeholder")
sqlx-core/src/query_builder.rs
Outdated
} | ||
|
||
pub fn push(&mut self, sql: impl Display) -> &mut Self { | ||
self.query.push_str(&format!(" {}", sql)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.query.push_str(&format!(" {}", sql)); | |
// An error here is also highly unlikely but ignoring it is a code smell. | |
write!(self.query, " {}", sql).expect("error formatting `sql`"); |
It is a bit weird to insert a space automatically, though. What if someone is trying to concatenate an identifier using this method, or is building a string literal where formatting matters? This violates the principle of least surprise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better! I think this is a decent place to start.
Something that is missing though is some documentation. I can add that myself after merging unless you want to take a shot at it. |
If you feel motivated to do it, go ahead. I probably won't get to it until at least Monday otherwise. |
elaborates on the API introduced in #1780
This doesn't seem to get passed through the compile time checks, is that correct? |
@OskarPersson the latest commit is on a different feature branch for PR #1790, when this PR was merged it was passing CI. |
@abonander Does that PR add compile time checks for queries created using this builder? (I'm not able to test that myself at the moment) |
No, that's more in the purview of #1491 which may end up as a separate crate. |
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
elaborates on the API introduced in #1780
#291
I'm not sure how close this is to what was wanted.
Would love some feedback!
:)