From 5e122ab590a3ac1b117026b24bd64c0d985e702e Mon Sep 17 00:00:00 2001
From: Schneems <richard.schneeman+foo@gmail.com>
Date: Wed, 11 Dec 2024 12:13:11 -0600
Subject: [PATCH] Fix shared layer logic

There was a bug reported where deploying a Ruby application after modifying the Gemfile.lock would result in an error because we were skipping running `bundle install`. Debugging this lead me to find that we were returning the incorrect value from our shared cache logic (accidentally returning the currnt/now value rather than the old value).

The idea behind `cached_layer_write_metadata` is to either return the old cached data or return a message stating why it couldn't be returned. It was accidentally returning the new/current value instead.

I added a unit test to assert this behavior. While developing I used an integration test to debug the issue (given below).

This commit fixes the integration test given below:

## Gemfile.lock cache invalidation integration reproduction


```
$ cargo test test_gemfile_lock_invalidates_cache -- --exact
```

```
#[test]
fn test_gemfile_lock_invalidates_cache() {
    let app_dir = tempfile::tempdir().unwrap();
    fs_err::write(
        app_dir.path().join("Gemfile"),
        r#"
        source "https://rubygems.org"

        gem "rake"
    "#,
    )
    .unwrap();

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.0)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();
    let mut config = amd_arm_builder_config("heroku/builder:24", &app_dir.path().to_string_lossy());
    TestRunner::default().build(
        config.clone()
        .buildpacks([
            BuildpackReference::CurrentCrate,
        ]),
        |context| {
            let stdout = context.pack_stdout.clone();
            println!("{}", stdout);
            assert_contains!(stdout, "# Heroku Ruby Buildpack");
            assert_contains!(
                stdout,
                r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
            );
            assert_contains!(stdout, "Installing rake");

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.1)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();

            context.rebuild(
                config.buildpacks([BuildpackReference::CurrentCrate]),
                |rebuild_context| {
                    println!("{}", rebuild_context.pack_stdout);

                assert_contains!(
                    rebuild_context.pack_stdout,
                    r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
                );
                assert_contains!(rebuild_context.pack_stdout, "Installing rake");
                },
            );
        });
}
```
---
 buildpacks/ruby/src/layers/shared.rs | 33 ++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/buildpacks/ruby/src/layers/shared.rs b/buildpacks/ruby/src/layers/shared.rs
index 58854211..f7dcc3dc 100644
--- a/buildpacks/ruby/src/layers/shared.rs
+++ b/buildpacks/ruby/src/layers/shared.rs
@@ -38,7 +38,7 @@ pub(crate) trait MetadataDiff {
 
 /// Standardizes formatting for layer cache clearing behavior
 ///
-/// If the diff is empty, there are no changes and the layer is kept
+/// If the diff is empty, there are no changes and the layer is kept and the old data is returned
 /// If the diff is not empty, the layer is deleted and the changes are listed
 pub(crate) fn restored_layer_action<M>(old: &M, now: &M) -> (RestoredLayerAction, Meta<M>)
 where
@@ -46,7 +46,7 @@ where
 {
     let diff = now.diff(old);
     if diff.is_empty() {
-        (RestoredLayerAction::KeepLayer, Meta::Data(now.clone()))
+        (RestoredLayerAction::KeepLayer, Meta::Data(old.clone()))
     } else {
         (
             RestoredLayerAction::DeleteLayer,
@@ -222,6 +222,35 @@ mod tests {
     }
     migrate_toml_chain! {TestMetadata}
 
+    #[test]
+    fn test_restored_layer_action_returns_old_data() {
+        #[derive(Debug, Clone)]
+        struct AlwaysNoDiff {
+            value: String,
+        }
+
+        impl MetadataDiff for AlwaysNoDiff {
+            fn diff(&self, _: &Self) -> Vec<String> {
+                vec![]
+            }
+        }
+
+        let old = AlwaysNoDiff {
+            value: "old".to_string(),
+        };
+        let now = AlwaysNoDiff {
+            value: "now".to_string(),
+        };
+
+        let result = restored_layer_action(&old, &now);
+        match result {
+            (RestoredLayerAction::KeepLayer, Meta::Data(data)) => {
+                assert_eq!(data.value, "old");
+            }
+            _ => panic!("Expected to keep layer"),
+        }
+    }
+
     #[test]
     fn test_cached_layer_write_metadata_restored_layer_action() {
         let temp = tempfile::tempdir().unwrap();