Don't unnecessarily load all models in stack check (#3941)

Don't unnecessarily load all models in stack check

* loading all models can cause issues in cases where the table for a
  model does not yet exist

Co-authored-by: Alex Rocha <alex.rocha@broadcom.com>
Co-authored-by: Sam Gunaratne <385176+Samze@users.noreply.github.com>

lib/tasks/stack_check.rake

@@ -4,13 +4,12 @@ namespace :stacks do
     logger = Steno.logger('cc.stack')
     db = VCAP::CloudController::DB.connect(RakeConfig.config.get(:db), logger)
     next unless db.table_exists?(:stacks)
+    next unless db.table_exists?(:buildpack_lifecycle_data)
 
-    VCAP::CloudController::DB.load_models_without_migrations_check(RakeConfig.config.get(:db), logger)
     RakeConfig.config.load_db_encryption_key
     require 'models/runtime/buildpack_lifecycle_data_model'
     require 'models/runtime/stack'
     require 'cloud_controller/check_stacks'
-
     VCAP::CloudController::CheckStacks.new(RakeConfig.config).validate_stacks
   end
 end

spec/migrations/ensure_migrations_are_current_spec.rb

@@ -7,7 +7,6 @@
   end
 
   it 'runs the rake task successfully' do
-    Application.load_tasks
     expect { Rake::Task['db:ensure_migrations_are_current'].invoke }.not_to raise_error
   end
 end

spec/spec_helper.rb

@@ -131,7 +131,9 @@
 
     rspec_config.include SpaceRestrictedResponseGenerators
 
-    rspec_config.before(:all) { WebMock.disable_net_connect!(allow: %w[codeclimate.com fake.bbs]) }
+    rspec_config.before(:all) do
+      WebMock.disable_net_connect!(allow: %w[codeclimate.com fake.bbs])
+    end
     rspec_config.before(:all, type: :integration) do
       WebMock.allow_net_connect!
       @uaa_server = FakeUAAServer.new(6789)
@@ -156,6 +158,10 @@
 
     rspec_config.before :suite do
       VCAP::CloudController::SpecBootstrap.seed
+      # We only want to load rake tasks once:
+      # calling this more than once will load tasks again and 'invoke' or 'execute' calls
+      # will call rake tasks multiple times
+      Application.load_tasks
     end
 
     rspec_config.before do

spec/tasks/db_spec.rb

@@ -9,8 +9,6 @@
       allow(RakeConfig).to receive(:config).and_return(TestConfig.config_instance)
       allow(DBMigrator).to receive(:from_config).and_return(db_migrator)
       allow(db_migrator).to receive(:apply_migrations)
-
-      Application.load_tasks
     end
 
     it 'logs to configured sinks + STDOUT' do

spec/tasks/stack_check_spec.rb

@@ -0,0 +1,89 @@
+require 'spec_helper'
+require 'tasks/rake_config'
+require 'cloud_controller/check_stacks'
+
+RSpec.describe 'stack_check' do
+  let(:stack_file_contents) do
+    {
+      'default' => 'cflinuxfs4',
+      'stacks' => [
+        cflinuxfs4
+      ],
+      'deprecated_stacks' => ['cflinuxfs3']
+    }
+  end
+
+  let(:cflinuxfs4) { { 'name' => 'cflinuxfs4', 'description' => 'fs4' } }
+
+  let(:db_double) do
+    dbl = double('db')
+    allow(dbl).to receive(:table_exists?).and_return(true)
+    dbl
+  end
+
+  before do
+    file = Tempfile.new
+    file.write(stack_file_contents.to_yaml)
+    file.close
+    TestConfig.override(stacks_file: file.path)
+    allow(RakeConfig).to receive(:config).and_return(TestConfig.config_instance)
+  end
+
+  it 'does not load all models' do
+    expect(VCAP::CloudController::DB).not_to receive(:load_models_without_migrations_check)
+    expect(VCAP::CloudController::DB).not_to receive(:load_models)
+    Rake::Task['stacks:stack_check'].execute
+  end
+
+  context 'stacks' do
+    context 'when stacks table doesnt exist' do
+      before do
+        allow(db_double).to receive(:table_exists?).with(:stacks).and_return false
+        allow(VCAP::CloudController::DB).to receive(:connect).and_return(db_double)
+      end
+
+      it 'does nothing' do
+        expect_any_instance_of(VCAP::CloudController::CheckStacks).not_to receive(:validate_stacks)
+        Rake::Task['stacks:stack_check'].execute
+      end
+    end
+
+    context 'when stacks table does exist' do
+      before do
+        allow(db_double).to receive(:table_exists?).with(:stacks).and_return true
+        allow(VCAP::CloudController::DB).to receive(:connect).and_return(db_double)
+      end
+
+      it 'validates stacks' do
+        expect_any_instance_of(VCAP::CloudController::CheckStacks).to receive(:validate_stacks).and_call_original
+        Rake::Task['stacks:stack_check'].execute
+      end
+    end
+  end
+
+  context 'buildpack_lifecycle_data' do
+    context 'when buildpack_lifecycle_data table doesnt exist' do
+      before do
+        allow(db_double).to receive(:table_exists?).with(:buildpack_lifecycle_data).and_return false
+        allow(VCAP::CloudController::DB).to receive(:connect).and_return(db_double)
+      end
+
+      it 'does nothing' do
+        expect_any_instance_of(VCAP::CloudController::CheckStacks).not_to receive(:validate_stacks)
+        Rake::Task['stacks:stack_check'].execute
+      end
+    end
+
+    context 'when buildpack_lifecycle_data table does exist' do
+      before do
+        allow(double).to receive(:table_exists?).with(:buildpack_lifecycle_data).and_return true
+        allow(VCAP::CloudController::DB).to receive(:connect).and_return(db_double)
+      end
+
+      it 'validates stacks' do
+        expect_any_instance_of(VCAP::CloudController::CheckStacks).to receive(:validate_stacks).and_call_original
+        Rake::Task['stacks:stack_check'].execute
+      end
+    end
+  end
+end

spec/unit/lib/cloud_controller/check_stacks_spec.rb

@@ -11,7 +11,7 @@ module VCAP::CloudController
         'stacks' => [
           cflinuxfs4
         ],
-        'deprecated_stacks' => 'cflinuxfs3'
+        'deprecated_stacks' => ['cflinuxfs3']
       }
     end