refactor(aggregator): do not override existing epoch settings on save

Author: turmelclem

mithril-aggregator/src/database/query/epoch_settings/insert_or_ignore_epoch_settings.rs

@@ -0,0 +1,116 @@
+use sqlite::Value;
+
+use mithril_persistence::sqlite::{Query, SourceAlias, SqLiteEntity, WhereCondition};
+
+use crate::database::record::EpochSettingsRecord;
+
+/// Query to update [EpochSettingsRecord] in the sqlite database
+pub struct InsertOrIgnoreEpochSettingsQuery {
+    condition: WhereCondition,
+}
+
+impl InsertOrIgnoreEpochSettingsQuery {
+    pub fn one(epoch_settings: EpochSettingsRecord) -> Self {
+        Self {
+            condition: WhereCondition::new(
+                "(epoch_setting_id, protocol_parameters, cardano_transactions_signing_config) values (?1, ?2, ?3)",
+                vec![
+                    Value::Integer(*epoch_settings.epoch_settings_id as i64),
+                    Value::String(
+                        serde_json::to_string(&epoch_settings.protocol_parameters).unwrap(),
+                    ),
+                    Value::String(
+                        serde_json::to_string(&epoch_settings.cardano_transactions_signing_config)
+                            .unwrap(),
+                    ),
+                ],
+            ),
+        }
+    }
+}
+
+impl Query for InsertOrIgnoreEpochSettingsQuery {
+    type Entity = EpochSettingsRecord;
+
+    fn filters(&self) -> WhereCondition {
+        self.condition.clone()
+    }
+
+    fn get_definition(&self, condition: &str) -> String {
+        // it is important to alias the fields with the same name as the table
+        // since the table cannot be aliased in a RETURNING statement in SQLite.
+        let projection = Self::Entity::get_projection()
+            .expand(SourceAlias::new(&[("{:epoch_setting:}", "epoch_setting")]));
+
+        format!("insert or ignore into epoch_setting {condition} returning {projection}")
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use mithril_common::entities::{BlockNumber, CardanoTransactionsSigningConfig, Epoch};
+    use mithril_common::test::double::fake_data;
+    use mithril_persistence::sqlite::ConnectionExtensions;
+
+    use crate::database::query::GetEpochSettingsQuery;
+    use crate::database::test_helper::main_db_connection;
+
+    use super::*;
+
+    #[test]
+    fn test_insert_epoch_setting_in_empty_db() {
+        let connection = main_db_connection().unwrap();
+
+        let expected_epoch_settings = EpochSettingsRecord {
+            epoch_settings_id: Epoch(3),
+            protocol_parameters: fake_data::protocol_parameters(),
+            cardano_transactions_signing_config: CardanoTransactionsSigningConfig {
+                security_parameter: BlockNumber(24),
+                step: BlockNumber(62),
+            },
+        };
+        let record = connection
+            .fetch_first(InsertOrIgnoreEpochSettingsQuery::one(
+                expected_epoch_settings.clone(),
+            ))
+            .unwrap();
+
+        assert_eq!(Some(expected_epoch_settings), record);
+    }
+
+    #[test]
+    fn test_cant_replace_existing_value() {
+        let connection = main_db_connection().unwrap();
+
+        let expected_epoch_settings = EpochSettingsRecord {
+            epoch_settings_id: Epoch(3),
+            protocol_parameters: fake_data::protocol_parameters(),
+            cardano_transactions_signing_config: CardanoTransactionsSigningConfig {
+                security_parameter: BlockNumber(24),
+                step: BlockNumber(62),
+            },
+        };
+        let record = connection
+            .fetch_first(InsertOrIgnoreEpochSettingsQuery::one(
+                expected_epoch_settings.clone(),
+            ))
+            .unwrap();
+        assert!(record.is_some());
+
+        let record = connection
+            .fetch_first(InsertOrIgnoreEpochSettingsQuery::one(EpochSettingsRecord {
+                cardano_transactions_signing_config: CardanoTransactionsSigningConfig {
+                    security_parameter: BlockNumber(134),
+                    step: BlockNumber(872),
+                },
+                ..expected_epoch_settings.clone()
+            }))
+            .unwrap();
+        assert!(record.is_none());
+
+        let record_in_db = connection
+            .fetch_first(GetEpochSettingsQuery::by_epoch(Epoch(3)).unwrap())
+            .unwrap();
+        assert_eq!(Some(expected_epoch_settings), record_in_db);
+    }
+}

mithril-aggregator/src/database/query/epoch_settings/mod.rs

@@ -1,7 +1,7 @@
 mod delete_epoch_settings;
 mod get_epoch_settings;
-mod update_epoch_settings;
+mod insert_or_ignore_epoch_settings;
 
 pub use delete_epoch_settings::*;
 pub use get_epoch_settings::*;
-pub use update_epoch_settings::*;
+pub use insert_or_ignore_epoch_settings::*;

mithril-aggregator/src/database/query/epoch_settings/update_epoch_settings.rs

@@ -4,7 +4,7 @@ use mithril_persistence::sqlite::{HydrationError, Projection, SqLiteEntity};
 use crate::entities::AggregatorEpochSettings;
 
 /// Settings for an epoch, including the protocol parameters.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Clone)]
 pub struct EpochSettingsRecord {
     /// Epoch settings id, i.e. the epoch number.
     pub epoch_settings_id: Epoch,

mithril-aggregator/src/database/record/epoch_settings.rs

@@ -8,8 +8,9 @@ use mithril_common::entities::{Epoch, ProtocolParameters};
 use mithril_persistence::sqlite::{ConnectionExtensions, SqliteConnection};
 
 use crate::database::query::{
-    DeleteEpochSettingsQuery, GetEpochSettingsQuery, UpdateEpochSettingsQuery,
+    DeleteEpochSettingsQuery, GetEpochSettingsQuery, InsertOrIgnoreEpochSettingsQuery,
 };
+use crate::database::record::EpochSettingsRecord;
 use crate::entities::AggregatorEpochSettings;
 use crate::services::EpochPruningTask;
 use crate::{EpochSettingsStorer, ProtocolParametersRetriever};
@@ -50,13 +51,17 @@ impl EpochSettingsStorer for EpochSettingsStore {
         epoch: Epoch,
         epoch_settings: AggregatorEpochSettings,
     ) -> StdResult<Option<AggregatorEpochSettings>> {
+        let record_to_insert = EpochSettingsRecord {
+            epoch_settings_id: epoch,
+            cardano_transactions_signing_config: epoch_settings.cardano_transactions_signing_config,
+            protocol_parameters: epoch_settings.protocol_parameters,
+        };
         let epoch_settings_record = self
             .connection
-            .fetch_first(UpdateEpochSettingsQuery::one(epoch, epoch_settings))
-            .with_context(|| format!("persist epoch settings failure for epoch {epoch:?}"))?
-            .unwrap_or_else(|| panic!("No entity returned by the persister, epoch = {epoch:?}"));
+            .fetch_first(InsertOrIgnoreEpochSettingsQuery::one(record_to_insert))
+            .with_context(|| format!("persist epoch settings failure for epoch {epoch:?}"))?;
 
-        Ok(Some(epoch_settings_record.into()))
+        Ok(epoch_settings_record.map(Into::into))
     }
 
     async fn get_epoch_settings(&self, epoch: Epoch) -> StdResult<Option<AggregatorEpochSettings>> {
@@ -171,4 +176,34 @@ mod tests {
             assert_eq!(None, epoch_settings);
         }
     }
+
+    #[tokio::test]
+    async fn save_epoch_settings_does_not_replace_existing_value_in_database() {
+        let connection = main_db_connection().unwrap();
+
+        let store = EpochSettingsStore::new(Arc::new(connection), None);
+        let expected_epoch_settings = AggregatorEpochSettings {
+            protocol_parameters: ProtocolParameters::new(1, 1, 0.5),
+            ..Dummy::dummy()
+        };
+
+        store
+            .save_epoch_settings(Epoch(2), expected_epoch_settings.clone())
+            .await
+            .expect("saving epoch settings should not fails");
+
+        store
+            .save_epoch_settings(
+                Epoch(2),
+                AggregatorEpochSettings {
+                    protocol_parameters: ProtocolParameters::new(2, 2, 1.5),
+                    ..Dummy::dummy()
+                },
+            )
+            .await
+            .expect("saving epoch settings should not fails");
+
+        let epoch_settings = store.get_epoch_settings(Epoch(2)).await.unwrap().unwrap();
+        assert_eq!(expected_epoch_settings, epoch_settings);
+    }
 }

mithril-aggregator/src/database/repository/epoch_settings_store.rs

@@ -12,16 +12,15 @@ use mithril_persistence::sqlite::{
 };
 
 use crate::database::query::{
-    ImportSignerRecordQuery, InsertCertificateRecordQuery,
+    ImportSignerRecordQuery, InsertCertificateRecordQuery, InsertOrIgnoreEpochSettingsQuery,
     InsertOrReplaceBufferedSingleSignatureRecordQuery,
     InsertOrReplaceSignerRegistrationRecordQuery, InsertOrReplaceStakePoolQuery,
-    InsertSignedEntityRecordQuery, UpdateEpochSettingsQuery, UpdateSingleSignatureRecordQuery,
+    InsertSignedEntityRecordQuery, UpdateSingleSignatureRecordQuery,
 };
 use crate::database::record::{
-    BufferedSingleSignatureRecord, CertificateRecord, SignedEntityRecord, SignerRecord,
-    SignerRegistrationRecord, SingleSignatureRecord,
+    BufferedSingleSignatureRecord, CertificateRecord, EpochSettingsRecord, SignedEntityRecord,
+    SignerRecord, SignerRegistrationRecord, SingleSignatureRecord,
 };
-use crate::entities::AggregatorEpochSettings;
 
 /// In-memory sqlite database without foreign key support with migrations applied
 pub fn main_db_connection() -> StdResult<SqliteConnection> {
@@ -206,16 +205,14 @@ pub fn insert_epoch_settings(
     let query = {
         // leverage the expanded parameter from this query which is unit
         // tested on its own above.
-        let (sql_values, _) = UpdateEpochSettingsQuery::one(
-            Epoch(1),
-            AggregatorEpochSettings {
-                protocol_parameters: ProtocolParameters::new(1, 2, 1.0),
-                cardano_transactions_signing_config: CardanoTransactionsSigningConfig {
-                    security_parameter: BlockNumber(0),
-                    step: BlockNumber(0),
-                },
+        let (sql_values, _) = InsertOrIgnoreEpochSettingsQuery::one(EpochSettingsRecord {
+            epoch_settings_id: Epoch(1),
+            protocol_parameters: ProtocolParameters::new(1, 2, 1.0),
+            cardano_transactions_signing_config: CardanoTransactionsSigningConfig {
+                security_parameter: BlockNumber(0),
+                step: BlockNumber(0),
             },
-        )
+        })
         .filters()
         .expand();