fix: roll back patches in check_for_downloadable_update (#270)

* fix: roll back patches in check_for_downloadable_update

* Update docs

library/src/updater.rs

@@ -268,6 +268,10 @@ pub fn check_for_downloadable_update(channel: Option<&str>) -> anyhow::Result<bo
     let response = request_fn(&url, request)?;
     shorebird_debug!("Patch check response: {:?}", response);
 
+    if let Some(rolled_back_patches) = response.rolled_back_patch_numbers {
+        roll_back_patches_if_needed(rolled_back_patches)?;
+    }
+
     if let Some(patch) = response.patch {
         match should_install_patch(patch.number)? {
             ShouldInstallPatchCheckResult::PatchOkToInstall => Ok(true),
@@ -387,17 +391,9 @@ fn update_internal(_: &UpdaterLockState, channel: Option<&str>) -> anyhow::Resul
     let response = patch_check_request_fn(&patches_check_url(&config.base_url), request)?;
     shorebird_info!("Patch check response: {:?}", response);
 
-    with_mut_state(|state| {
-        if let Some(rolled_back_patches) = response.rolled_back_patch_numbers {
-            if !rolled_back_patches.is_empty() {
-                for patch_number in rolled_back_patches {
-                    state.uninstall_patch(patch_number)?;
-                }
-            }
-        }
-
-        Ok(())
-    })?;
+    if let Some(rolled_back_patches) = response.rolled_back_patch_numbers {
+        roll_back_patches_if_needed(rolled_back_patches)?;
+    }
 
     if !response.patch_available {
         return Ok(UpdateStatus::NoUpdate);
@@ -459,6 +455,15 @@ fn update_internal(_: &UpdaterLockState, channel: Option<&str>) -> anyhow::Resul
     })
 }
 
+fn roll_back_patches_if_needed(patch_numbers: Vec<usize>) -> anyhow::Result<()> {
+    with_mut_state(|state| {
+        for patch_number in patch_numbers {
+            state.uninstall_patch(patch_number)?;
+        }
+        Ok(())
+    })
+}
+
 fn should_install_patch(patch_number: usize) -> Result<ShouldInstallPatchCheckResult> {
     // Don't install a patch if it has previously failed to boot.
     let is_known_bad_patch = with_state(|state| Ok(state.is_known_bad_patch(patch_number)))?;
@@ -1499,28 +1504,34 @@ mod rollback_tests {
 
 #[cfg(test)]
 mod check_for_downloadable_update_tests {
+    use std::vec;
+
     use anyhow::Result;
     use serial_test::serial;
     use tempdir::TempDir;
 
     use crate::{
         network::PatchCheckResponse, report_launch_failure, report_launch_start,
-        test_utils::install_fake_patch, updater::tests::init_for_testing,
+        report_launch_success, test_utils::install_fake_patch, updater::tests::init_for_testing,
+        with_mut_state,
     };
 
     use super::Patch;
 
-    fn mock_server(available_patch_number: usize) -> mockito::ServerGuard {
+    fn mock_server(
+        available_patch_number: Option<usize>,
+        rolled_back_patch_numbers: Option<Vec<usize>>,
+    ) -> mockito::ServerGuard {
         let mut server = mockito::Server::new();
         let check_response = PatchCheckResponse {
-            patch_available: true,
-            patch: Some(Patch {
-                number: available_patch_number,
+            patch_available: available_patch_number.is_some(),
+            patch: available_patch_number.map(|number| Patch {
+                number,
                 hash: "#".to_string(),
                 download_url: "download_url".to_string(),
                 hash_signature: None,
             }),
-            rolled_back_patch_numbers: None,
+            rolled_back_patch_numbers,
         };
         let check_response_body = serde_json::to_string(&check_response).unwrap();
         let _ = server
@@ -1531,11 +1542,24 @@ mod check_for_downloadable_update_tests {
         server
     }
 
+    #[serial]
+    #[test]
+    fn returns_false_if_no_patch_is_available() -> Result<()> {
+        let server = mock_server(None, None);
+        let tmp_dir = TempDir::new("example").unwrap();
+        init_for_testing(&tmp_dir, Some(&server.url()));
+
+        let is_update_available = crate::check_for_downloadable_update(None)?;
+        assert!(!is_update_available);
+
+        Ok(())
+    }
+
     #[serial]
     #[test]
     fn returns_false_if_patch_is_already_installed() -> Result<()> {
         let patch_number = 1;
-        let server = mock_server(patch_number);
+        let server = mock_server(Some(patch_number), None);
         let tmp_dir = TempDir::new("example").unwrap();
         init_for_testing(&tmp_dir, Some(&server.url()));
 
@@ -1551,7 +1575,7 @@ mod check_for_downloadable_update_tests {
     #[test]
     fn returns_false_if_patch_is_known_bad() -> Result<()> {
         let patch_number = 1;
-        let server = mock_server(patch_number);
+        let server = mock_server(Some(patch_number), None);
         let tmp_dir = TempDir::new("example").unwrap();
         init_for_testing(&tmp_dir, Some(&server.url()));
 
@@ -1569,7 +1593,7 @@ mod check_for_downloadable_update_tests {
     #[test]
     fn returns_true_if_patch_has_no_issues() -> Result<()> {
         let patch_number = 1;
-        let server = mock_server(patch_number);
+        let server = mock_server(Some(patch_number), None);
         let tmp_dir = TempDir::new("example").unwrap();
         init_for_testing(&tmp_dir, Some(&server.url()));
 
@@ -1578,4 +1602,35 @@ mod check_for_downloadable_update_tests {
 
         Ok(())
     }
+
+    #[serial]
+    #[test]
+    fn rolls_back_patches_if_needed() -> Result<()> {
+        let patch_number = 1;
+        let server = mock_server(None, Some(vec![patch_number]));
+        let tmp_dir = TempDir::new("example").unwrap();
+        init_for_testing(&tmp_dir, Some(&server.url()));
+
+        install_fake_patch(patch_number)?;
+        report_launch_start()?;
+        report_launch_success()?;
+
+        with_mut_state(|state| {
+            assert_eq!(
+                state.next_boot_patch().map(|p| p.number),
+                Some(patch_number)
+            );
+            Ok(())
+        })?;
+
+        let is_update_available = crate::check_for_downloadable_update(None)?;
+        assert!(!is_update_available);
+
+        with_mut_state(|state| {
+            assert!(state.next_boot_patch().map(|p| p.number).is_none());
+            Ok(())
+        })?;
+
+        Ok(())
+    }
 }

shorebird_code_push/example/pubspec.yaml

@@ -17,7 +17,7 @@ dependencies:
 dev_dependencies:
   flutter_test:
     sdk: flutter
-  very_good_analysis: ^5.0.0
+  very_good_analysis: ^7.0.0
 
 flutter:
   assets:

shorebird_code_push/lib/src/shorebird_updater.dart

@@ -130,6 +130,10 @@ abstract class ShorebirdUpdater {
   /// Checks for available updates and returns the [UpdateStatus].
   /// This method should be used to determine the update status before calling
   /// [update].
+  ///
+  /// If this detects that the current patch has been rolled back, the current
+  /// patch will be uninstalled.
+  /// A separate call to `update()` is required to install new patches.
   Future<UpdateStatus> checkForUpdate({UpdateTrack? track});
 
   /// Updates the app to the latest patch (if available).