Fix load to respect existing environment variables over env file when…

… doing variable interpolation
cbjoseph · Feb 26, 2018 · 7f82f73 · 7f82f73
1 parent a47020f
commit 7f82f73
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 26 deletions.
diff --git a/lib/dotenv.rb b/lib/dotenv.rb
@@ -12,7 +12,7 @@ class << self
   def load(*filenames)
     with(*filenames) do |f|
       ignoring_nonexistent_files do
-        env = Environment.new(f)
+        env = Environment.new(f, true)
         instrument("dotenv.load", env: env) { env.apply }
       end
     end
@@ -21,7 +21,7 @@ def load(*filenames)
   # same as `load`, but raises Errno::ENOENT if any files don't exist
   def load!(*filenames)
     with(*filenames) do |f|
-      env = Environment.new(f)
+      env = Environment.new(f, true)
       instrument("dotenv.load", env: env) { env.apply }
     end
   end
@@ -30,7 +30,7 @@ def load!(*filenames)
   def overload(*filenames)
     with(*filenames) do |f|
       ignoring_nonexistent_files do
-        env = Environment.new(f)
+        env = Environment.new(f, false)
         instrument("dotenv.overload", env: env) { env.apply! }
       end
     end

diff --git a/lib/dotenv/environment.rb b/lib/dotenv/environment.rb
@@ -4,13 +4,13 @@ module Dotenv
   class Environment < Hash
     attr_reader :filename
 
-    def initialize(filename)
+    def initialize(filename, is_load)
       @filename = filename
-      load
+      load(is_load)
     end
 
-    def load
-      update Parser.call(read)
+    def load(is_load)
+      update Parser.call(read, is_load)
     end
 
     def read

diff --git a/lib/dotenv/parser.rb b/lib/dotenv/parser.rb
@@ -32,14 +32,15 @@ class Parser
     class << self
       attr_reader :substitutions
 
-      def call(string)
-        new(string).call
+      def call(string, is_load)
+        new(string, is_load).call
       end
     end
 
-    def initialize(string)
+    def initialize(string, is_load)
       @string = string
       @hash = {}
+      @is_load = is_load
     end
 
     def call
@@ -74,7 +75,7 @@ def parse_value(value)
 
       if Regexp.last_match(1) != "'"
         self.class.substitutions.each do |proc|
-          value = proc.call(value, @hash)
+          value = proc.call(value, @hash, @is_load)
         end
       end
       value

diff --git a/lib/dotenv/substitutions/command.rb b/lib/dotenv/substitutions/command.rb
@@ -20,7 +20,7 @@ class << self
           )
         /x
 
-        def call(value, _env)
+        def call(value, _env, _is_load)
           # Process interpolated shell commands
           value.gsub(INTERPOLATED_SHELL_COMMAND) do |*|
             # Eliminate opening and closing parentheses

diff --git a/lib/dotenv/substitutions/variable.rb b/lib/dotenv/substitutions/variable.rb
@@ -18,17 +18,27 @@ class << self
           \}?           # closing brace
         /xi
 
-        def call(value, env)
+        def call(value, env, is_load)
+          combined_env = if is_load
+                           env.merge(ENV)
+                         else
+                           ENV.to_h.merge(env)
+                         end
           value.gsub(VARIABLE) do |variable|
             match = $LAST_MATCH_INFO
+            substitute(match, variable, combined_env)
+          end
+        end
+
+        private
 
-            if match[1] == '\\'
-              variable[1..-1]
-            elsif match[3]
-              env.fetch(match[3]) { ENV[match[3]] }
-            else
-              variable
-            end
+        def substitute(match, variable, env)
+          if match[1] == '\\'
+            variable[1..-1]
+          elsif match[3]
+            env.fetch(match[3], "")
+          else
+            variable
           end
         end
       end

diff --git a/spec/dotenv/environment_spec.rb b/spec/dotenv/environment_spec.rb
@@ -11,7 +11,7 @@
 
     it "fails if file does not exist" do
       expect do
-        Dotenv::Environment.new(".does_not_exists")
+        Dotenv::Environment.new(".does_not_exists", true)
       end.to raise_error(Errno::ENOENT)
     end
   end
@@ -47,7 +47,7 @@ def env(text)
     file = Tempfile.new("dotenv")
     file.write text
     file.close
-    env = Dotenv::Environment.new(file.path)
+    env = Dotenv::Environment.new(file.path, true)
     file.unlink
     env
   end

diff --git a/spec/dotenv/parser_spec.rb b/spec/dotenv/parser_spec.rb
@@ -2,7 +2,7 @@
 
 describe Dotenv::Parser do
   def env(string)
-    Dotenv::Parser.call(string)
+    Dotenv::Parser.call(string, true)
   end
 
   it "parses unquoted values" do
@@ -55,11 +55,23 @@ def env(string)
       .to eql("FOO" => "test", "BAR" => "testbar")
   end
 
-  it "reads variables from ENV when expanding if not found in local env" do
+  it "expands variables from ENV if not found in .env" do
     ENV["FOO"] = "test"
     expect(env("BAR=$FOO")).to eql("BAR" => "test")
   end
 
+  it "expands variables from ENV if found in .env during load" do
+    ENV["FOO"] = "test"
+    expect(env("FOO=development\nBAR=${FOO}")["BAR"])
+      .to eql("test")
+  end
+
+  it "doesn't expand variables from ENV if in local env in overload" do
+    ENV["FOO"] = "test"
+    expect(env("FOO=development\nBAR=${FOO}")["BAR"])
+      .to eql("test")
+  end
+
   it "expands undefined variables to an empty string" do
     expect(env("BAR=$FOO")).to eql("BAR" => "")
   end

diff --git a/spec/dotenv_spec.rb b/spec/dotenv_spec.rb
@@ -10,7 +10,8 @@
       let(:env_files) { [] }
 
       it "defaults to .env" do
-        expect(Dotenv::Environment).to receive(:new).with(expand(".env"))
+        expect(Dotenv::Environment).to receive(:new).with(expand(".env"),
+                                                          anything)
           .and_return(double(apply: {}, apply!: {}))
         subject
       end
@@ -22,7 +23,7 @@
       it "expands the path" do
         expected = expand("~/.env")
         allow(File).to receive(:exist?) { |arg| arg == expected }
-        expect(Dotenv::Environment).to receive(:new).with(expected)
+        expect(Dotenv::Environment).to receive(:new).with(expected, anything)
           .and_return(double(apply: {}, apply!: {}))
         subject
       end
@@ -55,10 +56,17 @@
   end
 
   describe "load" do
+    let(:env_files) { [] }
     subject { Dotenv.load(*env_files) }
 
     it_behaves_like "load"
 
+    it "initializes the Environment with a truthy is_load" do
+      expect(Dotenv::Environment).to receive(:new).with(anything, true)
+        .and_return(double(apply: {}, apply!: {}))
+      subject
+    end
+
     context "when the file does not exist" do
       let(:env_files) { [".env_does_not_exist"] }
 
@@ -70,10 +78,17 @@
   end
 
   describe "load!" do
+    let(:env_files) { [] }
     subject { Dotenv.load!(*env_files) }
 
     it_behaves_like "load"
 
+    it "initializes Environment with truthy is_load" do
+      expect(Dotenv::Environment).to receive(:new).with(anything, true)
+        .and_return(double(apply: {}, apply!: {}))
+      subject
+    end
+
     context "when one file exists and one does not" do
       let(:env_files) { [".env", ".env_does_not_exist"] }
 
@@ -88,6 +103,12 @@
     subject { Dotenv.overload(*env_files) }
     it_behaves_like "load"
 
+    it "initializes the Environment with a falsey is_load" do
+      expect(Dotenv::Environment).to receive(:new).with(anything, false)
+        .and_return(double(apply: {}, apply!: {}))
+      subject
+    end
+
     context "when loading a file containing already set variables" do
       let(:env_files) { [fixture_path("plain.env")] }