Skip to content

Commit

Permalink
Remove BuilderTx indirection
Browse files Browse the repository at this point in the history 
This removes the `SqliteReportBuilderTx` as it was effectively unused and an unnecessary indirection.

All users were going through the main `SqliteReportBuilder`, which was using a transaction for each method anyway.

Depending on the plans for error handling, a whole report (builder) should be constructed without errors for it to be internally consistent anyway, so the ability to rollback transactions with an arbitrary granularity does not make sense for now.
  • Loading branch information
@Swatinem
Swatinem committed Jul 24, 2024
1 parent c26f8f3 commit 00df4ae
Showing 1 changed file with 76 additions and 205 deletions.
281 changes: 76 additions & 205 deletions src/report/sqlite/report_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,7 @@ use crate::{
report::{models, ReportBuilder},
};

/// Returned by [`SqliteReportBuilder::transaction`]. Contains the actual
/// implementation for most of the `ReportBuilder` trait except for `build()`
/// which is implemented on [`SqliteReportBuilder`]. All
/// `SqliteReportBuilderTx`s created by a `SqliteReportBuilder` must
/// go out of scope before `SqliteReportBuilder::build()` can be called because
/// their `conn` member mutably borrows the SQLite database and prevents
/// `build()` from moving it into a `SqliteReport`.
pub struct SqliteReportBuilderTx<'a> {
rng: &'a mut StdRng,
coverage_sample_id_iterator: &'a mut RangeFrom<i64>,
branches_data_id_iterator: &'a mut RangeFrom<i64>,
method_data_id_iterator: &'a mut RangeFrom<i64>,
span_data_id_iterator: &'a mut RangeFrom<i64>,

pub filename: &'a Path,
pub conn: Transaction<'a>,
}

impl<'a> SqliteReportBuilderTx<'a> {
pub fn rollback(self) -> Result<()> {
Ok(self.conn.rollback()?)
}
}

/// Implementation of the [`ReportBuilder`] trait to build [`SqliteReport`]s.
/// The [`SqliteReportBuilder::transaction`] method returns a
/// [`SqliteReportBuilderTx`], an auxiliary `ReportBuilder` implementation which
/// will run its operations in a transaction that gets committed when the
/// `SqliteReportBuilderTx` goes out of scope. A non-transaction
/// `SqliteReportBuilder`'s `ReportBuilder` functions (except for `build()`)
/// call `self.transaction()?` for each call and delegate to the
/// `SqliteReportBuilderTx` implementation.
pub struct SqliteReportBuilder {
pub filename: PathBuf,
pub conn: Connection,
Expand Down Expand Up @@ -76,169 +45,9 @@ impl SqliteReportBuilder {
pub fn new(filename: PathBuf) -> Result<SqliteReportBuilder> {
Self::new_with_rng(filename, StdRng::from_entropy())
}

/// Create a [`SqliteReportBuilderTx`] with a [`rusqlite::Transaction`] that
/// will automatically commit itself when it goes out of scope.
///
/// Each `Transaction` holds a mutable reference to `self.conn` and prevents
/// `self.build()` from being called.
pub fn transaction(&mut self) -> Result<SqliteReportBuilderTx<'_>> {
let mut builder_tx = SqliteReportBuilderTx {
filename: &self.filename,
conn: self.conn.transaction()?,
rng: &mut self.rng,
coverage_sample_id_iterator: &mut self.coverage_sample_id_iterator,
branches_data_id_iterator: &mut self.branches_data_id_iterator,
method_data_id_iterator: &mut self.method_data_id_iterator,
span_data_id_iterator: &mut self.span_data_id_iterator,
};
builder_tx
.conn
.set_drop_behavior(rusqlite::DropBehavior::Commit);
Ok(builder_tx)
}
}

impl ReportBuilder<SqliteReport> for SqliteReportBuilder {
fn insert_file(&mut self, path: String) -> Result<models::SourceFile> {
self.transaction()?.insert_file(path)
}

fn insert_context(
&mut self,
context_type: models::ContextType,
name: &str,
) -> Result<models::Context> {
self.transaction()?.insert_context(context_type, name)
}

fn insert_coverage_sample(
&mut self,
sample: models::CoverageSample,
) -> Result<models::CoverageSample> {
self.transaction()?.insert_coverage_sample(sample)
}

fn multi_insert_coverage_sample(
&mut self,
samples: Vec<&mut models::CoverageSample>,
) -> Result<()> {
self.transaction()?.multi_insert_coverage_sample(samples)
}

fn insert_branches_data(
&mut self,
branch: models::BranchesData,
) -> Result<models::BranchesData> {
self.transaction()?.insert_branches_data(branch)
}

fn multi_insert_branches_data(
&mut self,
branches: Vec<&mut models::BranchesData>,
) -> Result<()> {
self.transaction()?.multi_insert_branches_data(branches)
}

fn insert_method_data(&mut self, method: models::MethodData) -> Result<models::MethodData> {
self.transaction()?.insert_method_data(method)
}

fn multi_insert_method_data(&mut self, methods: Vec<&mut models::MethodData>) -> Result<()> {
self.transaction()?.multi_insert_method_data(methods)
}

fn insert_span_data(&mut self, span: models::SpanData) -> Result<models::SpanData> {
self.transaction()?.insert_span_data(span)
}

fn multi_insert_span_data(&mut self, spans: Vec<&mut models::SpanData>) -> Result<()> {
self.transaction()?.multi_insert_span_data(spans)
}

fn associate_context(&mut self, assoc: models::ContextAssoc) -> Result<models::ContextAssoc> {
self.transaction()?.associate_context(assoc)
}

fn multi_associate_context(&mut self, assocs: Vec<&mut models::ContextAssoc>) -> Result<()> {
self.transaction()?.multi_associate_context(assocs)
}

fn insert_raw_upload(&mut self, raw_upload: models::RawUpload) -> Result<models::RawUpload> {
self.transaction()?.insert_raw_upload(raw_upload)
}

/// Consumes this builder and returns a [`SqliteReport`].
///
/// If any
/// [`SqliteReportBuilderTx`]s are still in scope, they hold a mutable
/// reference to `self.conn` and prevent this function from being
/// called:
/// ```compile_fail,E0505
/// # use codecov_rs::report::sqlite::*;
/// # use codecov_rs::report::ReportBuilder;
/// # use tempfile::tempdir;
/// # let temp_dir = tempdir().unwrap();
/// # let db_file = temp_dir.path().join("test.db");
///
/// let mut report_builder = SqliteReportBuilder::new(db_file).unwrap();
///
/// let mut tx = report_builder.transaction().unwrap();
/// let _ = tx.insert_file("foo.rs".to_string());
/// let _ = tx.insert_file("bar.rs".to_string());
///
/// // ERROR: cannot move out of `report_builder` because it is borrowed
/// let report = report_builder.build().unwrap();
/// ```
///
/// Making sure they go out of scope will unblock calling `build()`:
/// ```
/// # use codecov_rs::report::sqlite::*;
/// # use codecov_rs::report::ReportBuilder;
/// # use tempfile::tempdir;
/// # let temp_dir = tempdir().unwrap();
/// # let db_file = temp_dir.path().join("test.db");
///
/// let mut report_builder = SqliteReportBuilder::new(db_file).unwrap();
///
/// // `tx` will go out of scope at the end of this block
/// {
/// let mut tx = report_builder.transaction().unwrap();
/// let _ = tx.insert_file("foo.rs".to_string());
/// let _ = tx.insert_file("bar.rs".to_string());
/// }
///
/// // Works fine now
/// let report = report_builder.build().unwrap();
/// ```
///
/// Rolling the transaction back also works:
/// ```
/// # use codecov_rs::report::sqlite::*;
/// # use codecov_rs::report::ReportBuilder;
/// # use tempfile::tempdir;
/// # let temp_dir = tempdir().unwrap();
/// # let db_file = temp_dir.path().join("test.db");
///
/// let mut report_builder = SqliteReportBuilder::new(db_file).unwrap();
///
/// let mut tx = report_builder.transaction().unwrap();
/// let _ = tx.insert_file("foo.rs".to_string());
/// let _ = tx.insert_file("bar.rs".to_string());
/// let _ = tx.rollback();
///
/// // Works fine now
/// let report = report_builder.build().unwrap();
/// ```
fn build(self) -> Result<SqliteReport> {
Ok(SqliteReport {
filename: self.filename,
conn: self.conn,
})
}
}

impl<'a> ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'a> {
fn insert_file(&mut self, path: String) -> Result<models::SourceFile> {
let model = models::SourceFile {
id: seahash::hash(path.as_bytes()) as i64,
Expand Down Expand Up @@ -274,9 +83,9 @@ impl<'a> ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'a> {

fn multi_insert_coverage_sample(
&mut self,
mut samples: Vec<&mut models::CoverageSample>,
samples: Vec<&mut models::CoverageSample>,
) -> Result<()> {
for sample in &mut samples {
for sample in &samples {
sample.local_sample_id = self.coverage_sample_id_iterator.next().unwrap();
}
<models::CoverageSample as Insertable<8>>::multi_insert(
Expand All @@ -298,9 +107,9 @@ impl<'a> ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'a> {

fn multi_insert_branches_data(
&mut self,
mut branches: Vec<&mut models::BranchesData>,
branches: Vec<&mut models::BranchesData>,
) -> Result<()> {
for branch in &mut branches {
for branch in &branches {
branch.local_branch_id = self.branches_data_id_iterator.next().unwrap();
}
<models::BranchesData as Insertable<7>>::multi_insert(
Expand All @@ -317,11 +126,8 @@ impl<'a> ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'a> {
Ok(method)
}

fn multi_insert_method_data(
&mut self,
mut methods: Vec<&mut models::MethodData>,
) -> Result<()> {
for method in &mut methods {
fn multi_insert_method_data(&mut self, methods: Vec<&mut models::MethodData>) -> Result<()> {
for method in &methods {
method.local_method_id = self.method_data_id_iterator.next().unwrap();
}
<models::MethodData as Insertable<9>>::multi_insert(
Expand All @@ -338,8 +144,8 @@ impl<'a> ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'a> {
Ok(span)
}

fn multi_insert_span_data(&mut self, mut spans: Vec<&mut models::SpanData>) -> Result<()> {
for span in &mut spans {
fn multi_insert_span_data(&mut self, spans: Vec<&mut models::SpanData>) -> Result<()> {
for span in &spans {
span.local_span_id = self.span_data_id_iterator.next().unwrap();
}
<models::SpanData as Insertable<9>>::multi_insert(spans.iter().map(|v| &**v), &self.conn)?;
Expand Down Expand Up @@ -385,10 +191,75 @@ impl<'a> ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'a> {
Ok(raw_upload)
}

/// Consumes this builder and returns a [`SqliteReport`].
///
/// If any
/// [`SqliteReportBuilderTx`]s are still in scope, they hold a mutable
/// reference to `self.conn` and prevent this function from being
/// called:
/// ```compile_fail,E0505
/// # use codecov_rs::report::sqlite::*;
/// # use codecov_rs::report::ReportBuilder;
/// # use tempfile::tempdir;
/// # let temp_dir = tempdir().unwrap();
/// # let db_file = temp_dir.path().join("test.db");
///
/// let mut report_builder = SqliteReportBuilder::new(db_file).unwrap();
///
/// let mut tx = report_builder.transaction().unwrap();
/// let _ = tx.insert_file("foo.rs".to_string());
/// let _ = tx.insert_file("bar.rs".to_string());
///
/// // ERROR: cannot move out of `report_builder` because it is borrowed
/// let report = report_builder.build().unwrap();
/// ```
///
/// Making sure they go out of scope will unblock calling `build()`:
/// ```
/// # use codecov_rs::report::sqlite::*;
/// # use codecov_rs::report::ReportBuilder;
/// # use tempfile::tempdir;
/// # let temp_dir = tempdir().unwrap();
/// # let db_file = temp_dir.path().join("test.db");
///
/// let mut report_builder = SqliteReportBuilder::new(db_file).unwrap();
///
/// // `tx` will go out of scope at the end of this block
/// {
/// let mut tx = report_builder.transaction().unwrap();
/// let _ = tx.insert_file("foo.rs".to_string());
/// let _ = tx.insert_file("bar.rs".to_string());
/// }
///
/// // Works fine now
/// let report = report_builder.build().unwrap();
/// ```
///
/// Rolling the transaction back also works:
/// ```
/// # use codecov_rs::report::sqlite::*;
/// # use codecov_rs::report::ReportBuilder;
/// # use tempfile::tempdir;
/// # let temp_dir = tempdir().unwrap();
/// # let db_file = temp_dir.path().join("test.db");
///
/// let mut report_builder = SqliteReportBuilder::new(db_file).unwrap();
///
/// let mut tx = report_builder.transaction().unwrap();
/// let _ = tx.insert_file("foo.rs".to_string());
/// let _ = tx.insert_file("bar.rs".to_string());
/// let _ = tx.rollback();
///
/// // Works fine now
/// let report = report_builder.build().unwrap();
/// ```
fn build(self) -> Result<SqliteReport> {
Err(CodecovError::ReportBuilderError(
"called `build()` on a transaction".to_string(),
))
self.conn.flush_prepared_statement_cache();
self.conn.cache_flush()?;
Ok(SqliteReport {
filename: self.filename,
conn: self.conn,
})
}
}

Expand Down

0 comments on commit 00df4ae

Please sign in to comment.