Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dotenv::Railtie.overload violates policy about env files priority #424

Closed
domcermak opened this issue Jan 30, 2021 · 3 comments
Closed

Dotenv::Railtie.overload violates policy about env files priority #424

domcermak opened this issue Jan 30, 2021 · 3 comments

Comments

@domcermak
Copy link

domcermak commented Jan 30, 2021

Expected behavior

.env.development should have higher priority then .env while using Dotenv::Railtie.overload

Actual behavior

#overload method violates policy about env files priority.

Let's say RAILS_ENV=development and there are files .env.development and .env in the rails root directory.

Case of #load method

Dotenv::Railtie.load

calls

def self.load
  instance.load
end

which calls

def load
   Dotenv.load(*dotenv_files)
end

which calls

def load(*filenames)
  with(*filenames) do |f|
    ignoring_nonexistent_files do
      env = Environment.new(f, true)
      instrument("dotenv.load", env: env) { env.apply }
    end
  end
end

which calls

def apply
  each { |k, v| ENV[k] ||= v }
end

which uses the ||= operator, so when it is used with files in order given by

def dotenv_files
  [
    root.join(".env.#{Rails.env}.local"),
    (root.join(".env.local") unless Rails.env.test?),
    root.join(".env.#{Rails.env}"),
    root.join(".env")
  ].compact
end

then the #load method fulfills the priority policy mentioned in readme. So variables in .env.development have higher priority then .env. This is correct behaviour. Unlike the #overloadmethod, which I am going to describe now.

Case of #overload method

Dotenv::Railtie.overload

calls

def overload
  Dotenv.overload(*dotenv_files)
end

which calls

def overload(*filenames)
  with(*filenames) do |f|
    ignoring_nonexistent_files do
      env = Environment.new(f, false)
      instrument("dotenv.overload", env: env) { env.apply! }
    end
  end
end

which calls

def apply!
  each { |k, v| ENV[k] = v }
end

which uses the = operator, so the ENV values are really overriden. But with the given dotenv_files from

def dotenv_files
  [
    root.join(".env.#{Rails.env}.local"),
    (root.join(".env.local") unless Rails.env.test?),
    root.join(".env.#{Rails.env}"),
    root.join(".env")
  ].compact
end

this means, that variables from .env file have the highest priority, because they are processed after all other env files.
This violates the policy about env files priority. It also violates the comment by the #overload method definition "Same as load, but will override existing values in ENV".

There is a test for the #overload method in rails_spec.rb on line 116.

it "overloads .env.test with .env" do
   expect(ENV["DOTENV"]).to eql("true")
end

which confirms the priority policy is violated, because "overloads .env.test with .env". Which leaves me confused, because documentation says there is a priority policy, and then the test tests that the policy is being violated intentionally.

Expected solution

Change #overload method in Dotenv::Railtie from

def overload
  Dotenv.overload(*dotenv_files)
end

to

def overload
  Dotenv.overload(*dotenv_files.reverse)
end

or a change of documentation.

System configuration

dotenv version: 2.7.6

Rails version: 5.0.7.2

Ruby version: 2.6.6

@feihokpai
Copy link

feihokpai commented Jun 21, 2022

Guys, I read the source code and identified the same problem. This problem is without solution since January, 2021. Is there any preview about you will change that? I didn't see any Opened Pull Request about that.

@domcermak
Copy link
Author

domcermak commented Jun 25, 2022

@feihokpai you can always go with a monkey patch just like we did in our project but I created a PR with a fix I would prefer so we can hopefully get it fixed.

@bkeepers
Copy link
Owner

Fixed in #460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants