Merge pull request #158 from appsignal/log_file_path-improvements

Don't fallback on tmp if it's not writable

lib/appsignal/config.rb

@@ -5,6 +5,7 @@
 
 module Appsignal
   class Config
+    SYSTEM_TMP_DIR = '/tmp'
     DEFAULT_CONFIG = {
       :debug                          => false,
       :ignore_errors                  => [],
@@ -91,12 +92,19 @@ def []=(key, value)
     def log_file_path
       path = config_hash[:log_path] || root_path
       if path && File.writable?(path)
-        File.join(File.realpath(path), 'appsignal.log')
+        return File.join(File.realpath(path), 'appsignal.log')
+      end
+
+      if File.writable? SYSTEM_TMP_DIR
+        $stdout.puts "appsignal: Unable to log to '#{path}'. Logging to "\
+          "'#{SYSTEM_TMP_DIR}' instead. Please check the "\
+          "permissions for the application's log directory."
+        File.join(SYSTEM_TMP_DIR, 'appsignal.log')
       else
-        '/tmp/appsignal.log'
+        $stdout.puts "appsignal: Unable to log to '#{path}' or the "\
+          "'#{SYSTEM_TMP_DIR}' fallback. Please check the permissions "\
+          "for the application's (log) directory."
       end
-    rescue Errno::ENOENT
-      '/tmp/appsignal.log'
     end
 
     def valid?

spec/lib/appsignal/config_spec.rb

@@ -40,31 +40,145 @@
       })
     end
 
-    context "if a log file path is set" do
-      let(:config) { project_fixture_config('production', :log_path => '/tmp') }
+    describe "#log_file_path" do
+      let(:stdout) { StringIO.new }
+      let(:config) { project_fixture_config('production', :log_path => log_path) }
+      subject { config.log_file_path }
+      around do |example|
+        original_stdout = $stdout
+        $stdout = stdout
+        example.run
+        $stdout = original_stdout
+      end
+
+      context "when path is writable" do
+        let(:log_path) { File.join(tmp_dir, 'writable-path') }
+        before { FileUtils.mkdir_p(log_path, :mode => 0755) }
+        after { FileUtils.rm_rf(log_path) }
+
+        it "returns log file path" do
+          expect(subject).to eq File.join(log_path, 'appsignal.log')
+        end
+
+        it "prints no warning" do
+          subject
+          expect(stdout.string).to be_empty
+        end
+      end
 
-      its(:log_file_path) { should end_with('/tmp/appsignal.log') }
+      shared_examples '#log_file_path: tmp path' do
+        let(:system_tmp_dir) { Appsignal::Config::SYSTEM_TMP_DIR }
+        before { FileUtils.mkdir_p(system_tmp_dir) }
+        after { FileUtils.rm_rf(system_tmp_dir) }
 
-      context "if it is not writable" do
-        before { FileUtils.mkdir_p('/tmp/not-writable', :mode => 0555) }
+        context "when the /tmp fallback path is writable" do
+          before { FileUtils.chmod(0777, system_tmp_dir) }
 
-        let(:config) { project_fixture_config('production', :log_path => '/tmp/not-writable') }
+          it "returns returns the tmp location" do
+            expect(subject).to eq(File.join(system_tmp_dir, 'appsignal.log'))
+          end
 
-        its(:log_file_path) { should eq '/tmp/appsignal.log' }
+          it "prints a warning" do
+            subject
+            expect(stdout.string).to include "appsignal: Unable to log to '#{log_path}'. "\
+              "Logging to '#{system_tmp_dir}' instead."
+          end
+        end
+
+        context "when the /tmp fallback path is not writable" do
+          before { FileUtils.chmod(0555, system_tmp_dir) }
+
+          it "returns nil" do
+            expect(subject).to be_nil
+          end
+
+          it "prints a warning" do
+            subject
+            expect(stdout.string).to include "appsignal: Unable to log to '#{log_path}' "\
+              "or the '#{system_tmp_dir}' fallback."
+          end
+        end
       end
 
-      context "if it does not exist" do
-        let(:config) { project_fixture_config('production', :log_path => '/non-existing') }
+      context "when path is nil" do
+        let(:log_path) { nil }
+
+        context "when root_path is nil" do
+          before { allow(config).to receive(:root_path).and_return(nil) }
 
-        its(:log_file_path) { should eq '/tmp/appsignal.log' }
+          include_examples '#log_file_path: tmp path'
+        end
+
+        context "when root_path is set" do
+          it "returns returns the project log location" do
+            expect(subject).to eq File.join(config.root_path, 'appsignal.log')
+          end
+
+          it "prints no warning" do
+            subject
+            expect(stdout.string).to be_empty
+          end
+        end
+      end
+
+      context "when path does not exist" do
+        let(:log_path) { '/non-existing' }
+
+        include_examples '#log_file_path: tmp path'
+      end
+
+      context "when path is not writable" do
+        let(:log_path) { File.join(tmp_dir, 'not-writable-path') }
+        before { FileUtils.mkdir_p(log_path, :mode => 0555) }
+        after { FileUtils.rm_rf(log_path) }
+
+        include_examples '#log_file_path: tmp path'
       end
 
-      context "if it is nil" do
-        let(:config) { project_fixture_config('production', :log_path => nil) }
+      context "when path is a symlink" do
+        context "when linked path does not exist" do
+          let(:real_path) { File.join(tmp_dir, 'real-path') }
+          let(:log_path) { File.join(tmp_dir, 'symlink-path') }
+          before { File.symlink(real_path, log_path) }
+          after { FileUtils.rm(log_path) }
 
-        before { config.stub(:root_path => nil) }
+          include_examples '#log_file_path: tmp path'
+        end
 
-        its(:log_file_path) { should eq '/tmp/appsignal.log' }
+        context "when linked path exists" do
+          context "when linked path is not writable" do
+            let(:real_path) { File.join(tmp_dir, 'real-path') }
+            let(:log_path) { File.join(tmp_dir, 'symlink-path') }
+            before do
+              FileUtils.mkdir_p(real_path)
+              FileUtils.chmod(0444, real_path)
+              File.symlink(real_path, log_path)
+            end
+            after do
+              FileUtils.rm_rf(real_path)
+              FileUtils.rm(log_path)
+            end
+
+            include_examples '#log_file_path: tmp path'
+          end
+
+          context "when linked path is writable" do
+            let(:real_path) { File.join(tmp_dir, 'real-path') }
+            let(:log_path) { File.join(tmp_dir, 'symlink-path') }
+            before do
+              FileUtils.mkdir_p(real_path)
+              File.symlink(real_path, log_path)
+            end
+            after do
+              FileUtils.rm_rf(real_path)
+              FileUtils.rm(log_path)
+            end
+
+            it "returns real path of log path" do
+              expect(subject).to eq(File.join(real_path, 'appsignal.log'))
+            end
+          end
+        end
       end
     end
 

spec/spec_helper.rb

@@ -133,10 +133,11 @@ def clear_subscribers
   config.before :all do
     FileUtils.rm_rf(tmp_dir)
     FileUtils.mkdir_p(tmp_dir)
-  end
 
-  config.after do
-    Thread.current[:appsignal_transaction] = nil
+    # Use modifiable SYSTEM_TMP_DIR
+    Appsignal::Config.send :remove_const, :SYSTEM_TMP_DIR
+    Appsignal::Config.send :const_set, :SYSTEM_TMP_DIR,
+      File.join(tmp_dir, 'system-tmp')
   end
 
   config.before do
@@ -149,6 +150,10 @@ def clear_subscribers
     end
   end
 
+  config.after do
+    Thread.current[:appsignal_transaction] = nil
+  end
+
   config.after :all do
     ActiveSupport::Notifications.notifier.clear_subscribers
     FileUtils.rm_f(File.join(project_fixture_path, 'log/appsignal.log'))