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

Rails 6 multiple database tasks throws exception when environment config is used #35468

Closed
morgoth opened this issue Mar 4, 2019 · 10 comments · Fixed by #35497
Closed

Rails 6 multiple database tasks throws exception when environment config is used #35468

morgoth opened this issue Mar 4, 2019 · 10 comments · Fixed by #35497
Assignees
Milestone

Comments

@morgoth
Copy link
Member

morgoth commented Mar 4, 2019

In Rails 6 with multiple database support, rake tasks are now build dynamically based on db config:

ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name|
desc "Create #{spec_name} database for current environment"
task spec_name => :load_config do
db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: spec_name)
ActiveRecord::Tasks::DatabaseTasks.create(db_config.config)
end
end
end

This causes problems when evaluating config/database.yml that uses some configuration set on app level:

# config/environments/development.rb
Rails.application.configure do
  config.some_config = "I'm set"
end
default: &default
  some_config: <%= Rails.configuration.some_config %>

as in time of building rake task, app environment is not loaded yet, thus some_config method is not defined.

@eileencodes I'm wondering if those task names could be built without loading env, but I guess to get them, database.yml has to be evaluated. Other option would be to load the env in time of building them, but this looks like a too big overhead on rake -T in example.

@eileencodes
Copy link
Member

Can you make a basic app on Rails 6 that reproduces this and I will take a look.

Unfortunately we cannot avoid loading ENV at the time we do - I tried for weeks to work around that, it's just not possible.

@eileencodes eileencodes added this to the 6.0.0 milestone Mar 4, 2019
@morgoth
Copy link
Member Author

morgoth commented Mar 4, 2019

Here you go https://github.com/morgoth/ar-bug#readme :-)

@eileencodes eileencodes self-assigned this Mar 4, 2019
@eileencodes
Copy link
Member

Is this the error you're seeing or a different one?

rake aborted!
NoMethodError: Cannot load database configuration:
undefined method `max_threads' for #<Rails::Application::Configuration:0x00007fc39d0b96c8>
(erb):9:in `<main>'
/Users/eileencodes/open_source/rails_apps/ar-bug/Rakefile:6:in `<top (required)>'

@morgoth
Copy link
Member Author

morgoth commented Mar 4, 2019

yes, it's that error

@eileencodes
Copy link
Member

So there's a deeper problem we identified with setting any environment specific config that we really should have never allowed this behavior to work in the first place.

While it used to work without blowing up it presents some problems. If you set max_threads in development to 10 and in test to 5 and then load the configurations the configuration settings will change based on the environment you load that configuration in.

So if you load the configuration in test it will set max_threads in dev and test to 5 because Rails can't read a different environment when you're loading another environment.

I'm not sure we can support this going forward because it was a broken system to begin with. I will try to figure out how to not raise an unhelpful error.

@eileencodes
Copy link
Member

Actually, I'll first experiment a bit more with not blowing up but having been down this road before I'm not sure it's possible or recommended to support this going forward.

@morgoth
Copy link
Member Author

morgoth commented Mar 4, 2019

So if you load the configuration in test it will set max_threads in dev and test to 5 because Rails can't read a different environment when you're loading another environment.

Yes, I know about it. it works the same way as config_for

@eileencodes
Copy link
Member

eileencodes commented Mar 5, 2019

We have a chicken and egg problem here and I don't know how to solve this. Possible solutions are:

  1. Just load the YAML to get the namespaces without the ERB. This won't work though if you have multi-line erb, yaml can't parse it and it will blow up with a Psych error.
  2. Load the environment when we call for_each. Sure this is possible and it works but it means we're loading the env for bin/rake -T which may be a performance hit we're not willing to take.
  3. Have the app pre-provide the namespaces. If we had a config option like config.database_namespaces = [:primary, :animals] we could then just loop through those and create the tasks, but this is extra work app engineers need to do.

Clearly it's not acceptable to just break this but I also don't want to release Rails 6 without the rake tasks for multiple databases 😢

@eileencodes
Copy link
Member

@tenderlove @rafaelfranca and/or @matthewd do you have any ideas how I can create the rake tasks for multiple database like db:create:animals without loading the env or without parsing the erb in the yaml file?

We can load the yaml and ignore the ERB but if an app uses multi-line erb like we do at GitHub it will blow up with Psych::SyntaxError: (<unknown>): found character that cannot start any token while scanning for the next token at line 11 column 3.

If we try to load the rake tasks assuming there's no Rails configuration we get this issue. Because the env isn't loaded max_threads isn't defined.

@tenderlove
Copy link
Member

We could parse the ERB, but either not evaluate the Ruby code, or just put in dummy values. Are the namespaces going to be dynamic? Nobody can configure the namespaces via Rails.application.configure { }, right?

@eileencodes eileencodes changed the title Rails 6 multiple database tasks causing problems with loading app Rails 6 multiple database tasks throws exception when environment config is used Mar 6, 2019
eileencodes added a commit to eileencodes/rails that referenced this issue Mar 6, 2019
This change adds a new method that loads the YAML for the database
config without parsing the ERB. This may seem odd but bear with me:

When we added the ability to have rake tasks for multiple databases we
started looping through the configurations to collect the namespaces so
we could do `rake db:create:my_second_db`. See rails#32274.

This caused a problem where if you had `Rails.config.max_threads` set in
your database.yml it will blow up because the environment that defines
`max_threads` isn't loaded during `rake -T`. See rails#35468.

We tried to fix this by adding the ability to just load the YAML and
ignore ERB all together but that caused a bug in GitHub's YAML loading
where if you used multi-line ERB the YAML was invalid. That led us to
reverting some changes in rails#33748.

After trying to resolve this a bunch of ways `@tenderlove` came up with
replacing the ERB values so that we don't need to load the environment
but we also can load the YAML.

This change adds a DummyCompiler for ERB that will replace all the
values so we can load the database yaml and create the rake tasks.
Nothing else uses this method so it's "safe".

DO NOT use this method in your application.

Fixes rails#35468
eikes added a commit to eikes/rails that referenced this issue Sep 27, 2022
Commit 37d1429 introduced the DummyERB to avoid loading the environment when
running `rake -T`.

The DummyCompiler simply replaced all output from `<%=` with a fixed string and
removed everything else. This worked okay when it was used for YAML values.
When using `<%=` within a YAML key, it caused an error in the YAML parser,
making it impossible to use ERB as you would expect. For example a
`database.yml` file containing the following should be possible:

  development:
    <% 5.times do |i| %>
    shard_<%= i %>:
      database: db/development_shard_<%= i %>.sqlite3
      adapter: sqlite3
    <% end %>

Instead of using a broken ERB compiler we can temporarily use a
`Rails.application.config` that does not raise an error when configurations are
accessed which have not been set as described in rails#35468.

This change removes the `DummyCompiler` and uses the standard `ERB::Compiler`.
It introduces the `DummyConfig` which delegates all known configurations to the
real `Rails::Application::Configuration` instance and returns a dummy string for
everything else. This restores the full ERB capabilities without compromising on
speed when generating the rake tasks for multiple databases.

Deprecates `config.active_record.suppress_multiple_database_warning`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants