okay let cursor refactor tests

Author: mann-db

kernel/src/transaction/mod.rs

@@ -314,7 +314,8 @@ impl Transaction {
     /// the same domain in a single transaction. If a duplicate domain is included, the `commit` will
     /// fail (that is, we don't eagerly check domain validity here).
     pub fn with_domain_metadata_removed(mut self, domain: String) -> Self {
-        self.domain_metadatas.push(DomainMetadata::new(domain.clone(), String::new(), true));
+        self.domain_metadatas
+            .push(DomainMetadata::new(domain.clone(), String::new(), true));
         self
     }
 }

kernel/tests/write.rs

@@ -1190,7 +1190,7 @@ async fn test_set_domain_metadata_basic() -> Result<(), Box<dyn std::error::Erro
         setup_test_tables(schema.clone(), &[], None, "test_table").await?
     {
         let snapshot = Arc::new(Snapshot::builder(table_url.clone()).build(&engine)?);
-        let mut txn = snapshot.transaction()?;
+        let txn = snapshot.transaction()?;
 
         // write context does not conflict with domain metadata in any way
         let _write_context = txn.get_write_context();
@@ -1201,9 +1201,9 @@ async fn test_set_domain_metadata_basic() -> Result<(), Box<dyn std::error::Erro
         let domain2 = "spark.settings";
         let config2 = r#"{"cores": 4}"#;
 
-        txn.set_domain_metadata(domain1.to_string(), config1.to_string())?;
-        txn.set_domain_metadata(domain2.to_string(), config2.to_string())?;
-        txn.commit(&engine)?;
+        txn.with_domain_metadata(domain1.to_string(), config1.to_string())
+            .with_domain_metadata(domain2.to_string(), config2.to_string())
+            .commit(&engine)?;
 
         let commit_data = store
             .get(&Path::from(format!(
@@ -1258,20 +1258,23 @@ async fn test_set_domain_metadata_errors() -> Result<(), Box<dyn std::error::Err
         setup_test_tables(schema.clone(), &[], None, "test_table").await?
     {
         let snapshot = Arc::new(Snapshot::builder(table_url.clone()).build(&engine)?);
-        let mut txn = snapshot.transaction()?;
 
-        // System domain rejection
+        // Scenario 1: System domain rejection
+        let txn = snapshot.clone().transaction()?;
         let err = txn
-            .set_domain_metadata("delta.system".to_string(), "config".to_string())
+            .with_domain_metadata("delta.system".to_string(), "config".to_string())
+            .commit(&engine)
             .unwrap_err();
         assert!(err
             .to_string()
-            .contains("system-controlled 'delta.*' domain"));
-
-        // Duplicate domain rejection
-        txn.set_domain_metadata("app.config".to_string(), "v1".to_string())?;
-        let err = txn
-            .set_domain_metadata("app.config".to_string(), "v2".to_string())
+            .contains("Users cannot modify system controlled metadata domains"));
+
+        // Scenario 2: Duplicate domain rejection
+        let txn2 = snapshot.clone().transaction()?;
+        let err = txn2
+            .with_domain_metadata("app.config".to_string(), "v1".to_string())
+            .with_domain_metadata("app.config".to_string(), "v2".to_string())
+            .commit(&engine)
             .unwrap_err();
         assert!(err
             .to_string()
@@ -1284,15 +1287,20 @@ async fn test_set_domain_metadata_errors() -> Result<(), Box<dyn std::error::Err
 async fn test_remove_domain_metadata_basic() -> Result<(), Box<dyn std::error::Error>> {
     let _ = tracing_subscriber::fmt::try_init();
 
-    let schema = Arc::new(StructType::new(vec![StructField::nullable("number", DataType::INTEGER)]));
+    let schema = Arc::new(StructType::new(vec![StructField::nullable(
+        "number",
+        DataType::INTEGER,
+    )]));
 
-    for (table_url, engine, store, table_name) in setup_test_tables(schema.clone(), &[], None, "test_table").await? {
+    for (table_url, engine, store, table_name) in
+        setup_test_tables(schema.clone(), &[], None, "test_table").await?
+    {
         let snapshot = Arc::new(Snapshot::builder(table_url.clone()).build(&engine)?);
-        let mut txn = snapshot.transaction()?;
+        let txn = snapshot.transaction()?;
 
         // Remove domain metadata (creates tombstone even if it doesn't exist)
-        txn.remove_domain_metadata("app.deprecated".to_string())?;
-        txn.commit(&engine)?;
+        txn.with_domain_metadata_removed("app.deprecated".to_string())
+            .commit(&engine)?;
 
         let commit_data = store
             .get(&Path::from(format!(
@@ -1305,7 +1313,10 @@ async fn test_remove_domain_metadata_basic() -> Result<(), Box<dyn std::error::E
             .into_iter()
             .try_collect()?;
 
-        let domain_action = actions.iter().find(|v| v.get("domainMetadata").is_some()).unwrap();
+        let domain_action = actions
+            .iter()
+            .find(|v| v.get("domainMetadata").is_some())
+            .unwrap();
         assert_eq!(domain_action["domainMetadata"]["domain"], "app.deprecated");
         assert_eq!(domain_action["domainMetadata"]["configuration"], "");
         assert_eq!(domain_action["domainMetadata"]["removed"], true);
@@ -1317,52 +1328,86 @@ async fn test_remove_domain_metadata_basic() -> Result<(), Box<dyn std::error::E
 async fn test_domain_metadata_set_remove_conflicts() -> Result<(), Box<dyn std::error::Error>> {
     let _ = tracing_subscriber::fmt::try_init();
 
-    let schema = Arc::new(StructType::new(vec![StructField::nullable("number", DataType::INTEGER)]));
+    let schema = Arc::new(StructType::new(vec![StructField::nullable(
+        "number",
+        DataType::INTEGER,
+    )]));
 
-    for (table_url, engine, _, _) in setup_test_tables(schema.clone(), &[], None, "test_table").await? {
+    for (table_url, engine, _, _) in
+        setup_test_tables(schema.clone(), &[], None, "test_table").await?
+    {
         let snapshot = Arc::new(Snapshot::builder(table_url.clone()).build(&engine)?);
-        let mut txn = snapshot.transaction()?;
-
-        // Test 1: Set then remove in same transaction
-        txn.set_domain_metadata("app.config".to_string(), "v1".to_string())?;
-        let err = txn.remove_domain_metadata("app.config".to_string()).unwrap_err();
-        assert!(err.to_string().contains("already specified in this transaction"));
-
-        // Test 2: Remove then set in same transaction
-        txn.remove_domain_metadata("test.domain".to_string())?;
-        let err = txn.set_domain_metadata("test.domain".to_string(), "v1".to_string()).unwrap_err();
-        assert!(err.to_string().contains("already specified in this transaction"));
-
-        // Test 3: Remove then remove same domain
-        txn.remove_domain_metadata("another.domain".to_string())?;
-        let err = txn.remove_domain_metadata("another.domain".to_string()).unwrap_err();
-        assert!(err.to_string().contains("already specified in this transaction"));
-
-        // Test 4: System domain removal
-        let err = txn.remove_domain_metadata("delta.system".to_string()).unwrap_err();
-        assert!(err.to_string().contains("system-controlled 'delta.*' domain"));
+
+        // Scenario 1: Set then remove same domain
+        let txn = snapshot.clone().transaction()?;
+        let err = txn
+            .with_domain_metadata("app.config".to_string(), "v1".to_string())
+            .with_domain_metadata_removed("app.config".to_string())
+            .commit(&engine)
+            .unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("already specified in this transaction"));
+
+        // Scenario 2: Remove then set same domain
+        let txn2 = snapshot.clone().transaction()?;
+        let err = txn2
+            .with_domain_metadata_removed("test.domain".to_string())
+            .with_domain_metadata("test.domain".to_string(), "v1".to_string())
+            .commit(&engine)
+            .unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("already specified in this transaction"));
+
+        // Scenario 3: Remove then remove same domain
+        let txn3 = snapshot.clone().transaction()?;
+        let err = txn3
+            .with_domain_metadata_removed("another.domain".to_string())
+            .with_domain_metadata_removed("another.domain".to_string())
+            .commit(&engine)
+            .unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("already specified in this transaction"));
+
+        // Scenario 4: System domain removal
+        let txn4 = snapshot.clone().transaction()?;
+        let err = txn4
+            .with_domain_metadata_removed("delta.system".to_string())
+            .commit(&engine)
+            .unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("Users cannot modify system controlled metadata domains"));
     }
     Ok(())
 }
 
 #[tokio::test]
-async fn test_domain_metadata_set_then_remove_across_transactions() -> Result<(), Box<dyn std::error::Error>> {
+async fn test_domain_metadata_set_then_remove_across_transactions(
+) -> Result<(), Box<dyn std::error::Error>> {
     let _ = tracing_subscriber::fmt::try_init();
 
-    let schema = Arc::new(StructType::new(vec![StructField::nullable("number", DataType::INTEGER)]));
+    let schema = Arc::new(StructType::new(vec![StructField::nullable(
+        "number",
+        DataType::INTEGER,
+    )]));
 
-    for (table_url, engine, store, table_name) in setup_test_tables(schema.clone(), &[], None, "test_table").await? {
+    for (table_url, engine, store, table_name) in
+        setup_test_tables(schema.clone(), &[], None, "test_table").await?
+    {
         // Transaction 1: Set domain metadata
         let snapshot = Arc::new(Snapshot::builder(table_url.clone()).build(&engine)?);
-        let mut txn = snapshot.transaction()?;
-        txn.set_domain_metadata("app.config".to_string(), r#"{"version": 1}"#.to_string())?;
-        txn.commit(&engine)?;
+        let txn = snapshot.transaction()?;
+        txn.with_domain_metadata("app.config".to_string(), r#"{"version": 1}"#.to_string())
+            .commit(&engine)?;
 
         // Transaction 2: Remove the same domain metadata
         let snapshot = Arc::new(Snapshot::builder(table_url.clone()).build(&engine)?);
-        let mut txn = snapshot.transaction()?;
-        txn.remove_domain_metadata("app.config".to_string())?;
-        txn.commit(&engine)?;
+        let txn = snapshot.transaction()?;
+        txn.with_domain_metadata_removed("app.config".to_string())
+            .commit(&engine)?;
 
         // Verify removal commit
         let commit_data = store
@@ -1376,7 +1421,10 @@ async fn test_domain_metadata_set_then_remove_across_transactions() -> Result<()
             .into_iter()
             .try_collect()?;
 
-        let domain_action = actions.iter().find(|v| v.get("domainMetadata").is_some()).unwrap();
+        let domain_action = actions
+            .iter()
+            .find(|v| v.get("domainMetadata").is_some())
+            .unwrap();
         assert_eq!(domain_action["domainMetadata"]["domain"], "app.config");
         assert_eq!(domain_action["domainMetadata"]["configuration"], "");
         assert_eq!(domain_action["domainMetadata"]["removed"], true);