input-output-hk / mithril

Stake-based threshold multi-signatures protocol
https://mithril.network
Apache License 2.0
115 stars 36 forks source link

Remove connection from database provider #1715

Closed Alenar closed 1 month ago

Alenar commented 1 month ago

Content

This PR includes a refactor of the database API used to query entities from the database.

As of today we used the Provider trait to encapsulate a entity query, this trait come with a major issue: it needs to borrow a connection to the database, making it difficult to return a iterator to a query result. This PR replace this trait with a new one: Query. Basically a query is what was a provider but instead of owning a reference to a database connection it own a WhereCondition that will be applied when this query is run.

This means that a query implementation can't run itself like the provider could, instead you use an extension method directly on the sql connection: fetch. This method take a query as a parameter and nothing more since it own the where condition to apply

When we doesn't need the iterator returned by fetch there's two new other extension method on the db connection that are available:

Query design philosophy

All implementation have been made using the following model

pub struct GetStakePoolQuery {
    condition: WhereCondition,
}

impl Query for GetStakePoolQuery {
    type Entity = StakePool;

    fn filters(&self) -> WhereCondition {
        self.condition.clone()
    }

    fn get_definition(&self, condition: &str) -> String { [...] }
}

This is at creation, and by the query itself, that the condition is defined:

impl GetStakePoolQuery {
    pub fn by_epoch(epoch: Epoch) -> StdResult<Self> {
        let condition = WhereCondition::new("epoch = ?*", vec![Value::Integer(epoch.try_into()?)]);

        Ok(Self { condition })
    }
}

Caveat: This system make it more difficult to chain conditions outside of the query (like in the repository). This is only visible in the changes to the OpenMessageRepository and its associated 'Get' queries but it may be cumbersome if we want to multiply such uses.

Usage

Before:

let provider = GetCardanoTransactionProvider::new(&self.connection);
let filters = provider.get_transaction_between_blocks_condition(range);
let transactions = provider.find(filters)?;

After:

let transactions = self
        .connection
        .fetch(GetCardanoTransactionQuery::between_blocks(range))?;

Pre-submit checklist

Comments

Caveat:

Issue(s)

Closes #1711

github-actions[bot] commented 1 month ago

Test Results

    3 files  ±0     43 suites  ±0   8m 35s :stopwatch: +10s   992 tests +1    992 :white_check_mark: +1  0 :zzz: ±0  0 :x: ±0  1 090 runs  +1  1 090 :white_check_mark: +1  0 :zzz: ±0  0 :x: ±0 

Results for commit c3a60e67. ± Comparison against base commit 8b66e454.

This pull request removes 28 and adds 29 tests. Note that renamed tests count towards both. ``` mithril-aggregator ‑ database::provider::certificate::get_certificate::tests::test_get_certificate_records mithril-aggregator ‑ database::provider::certificate::insert_certificate::tests::test_insert_certificate_record mithril-aggregator ‑ database::provider::certificate::insert_certificate::tests::test_insert_many_certificates_records mithril-aggregator ‑ database::provider::epoch_setting::delete_epoch_setting::tests::test_delete mithril-aggregator ‑ database::provider::epoch_setting::delete_epoch_setting::tests::test_prune mithril-aggregator ‑ database::provider::epoch_setting::get_epoch_setting::tests::test_get_epoch_settings mithril-aggregator ‑ database::provider::epoch_setting::update_epoch_setting::tests::test_update_epoch_setting mithril-aggregator ‑ database::provider::signed_entity::get_signed_entity::tests::test_get_signed_entity_records mithril-aggregator ‑ database::provider::signed_entity::insert_signed_entity::tests::test_insert_signed_entity_record mithril-aggregator ‑ database::provider::signer::get_signer::tests::test_get_signer_records … ``` ``` mithril-aggregator ‑ database::query::certificate::get_certificate::tests::test_get_all_certificate_records mithril-aggregator ‑ database::query::certificate::get_certificate::tests::test_get_certificate_records_by_epoch mithril-aggregator ‑ database::query::certificate::insert_certificate::tests::test_insert_certificate_record mithril-aggregator ‑ database::query::certificate::insert_certificate::tests::test_insert_many_certificates_records mithril-aggregator ‑ database::query::epoch_setting::delete_epoch_setting::tests::test_delete_below_threshold mithril-aggregator ‑ database::query::epoch_setting::delete_epoch_setting::tests::test_delete_by_epoch mithril-aggregator ‑ database::query::epoch_setting::get_epoch_setting::tests::test_get_epoch_settings mithril-aggregator ‑ database::query::epoch_setting::update_epoch_setting::tests::test_update_epoch_setting mithril-aggregator ‑ database::query::signed_entity::get_signed_entity::tests::test_get_signed_entity_records mithril-aggregator ‑ database::query::signed_entity::insert_signed_entity::tests::test_insert_signed_entity_record … ```

:recycle: This comment has been updated with latest results.