Update readme to include fixes for rails 3.2.x. #7

Merged
merged 2 commits into from Feb 7, 2014

Conversation

Projects
None yet
4 participants
@tomlea
Contributor

tomlea commented Jan 6, 2014

We hit this issue while trying to use the buildpack with a rails 3.2 app. No matter what we did, in the heroku environment prepared statements were still being generated, and our staging server became a mountain of error messages.

Rails ignores the database.yml when DATABASE_URL is defined, but rails 3.2.x does not correctly parse prepared_statements=false.

prepared_statements=false is evaluated to {prepared_statements: "false"}, rather than {prepared_statements: false}. The string "false" evaluates to true, so prepared statements are not disabled correctly.

The monkey patch filters the config options hash, and correctly maps the "false" to false.

Update readme to include fixes for rails 3.2.x.
Rails ignores the database.yml when DATABASE_URL is defined, but rails 3.2.x does not correctly parse `prepared_statements=false`.

prepared_statements=false is evaluated to `{prepared_statements: "false"}`, rather than `{prepared_statements: false}`. The string `"false"` evaluates to true, so prepared statements are not disabled correctly.

The monkey patch filters the config options hash, and correctly maps the `"false"` to `false`.
@jeffday

This comment has been minimized.

Show comment Hide comment
@jeffday

jeffday Jan 14, 2014

👍

jeffday commented Jan 14, 2014

👍

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Jan 14, 2014

Contributor

Can you modify config in Rails 3.2 as mentioned here https://devcenter.heroku.com/articles/concurrency-and-database-connections ?

#config/initializers/database_connection.rb
Rails.application.config.after_initialize do
  ActiveRecord::Base.connection_pool.disconnect!

  ActiveSupport.on_load(:active_record) do
    config = Rails.application.config.database_configuration[Rails.env]
    config['prepared_statements'] = false
    ActiveRecord::Base.establish_connection(config)
  end
end
Contributor

schneems commented Jan 14, 2014

Can you modify config in Rails 3.2 as mentioned here https://devcenter.heroku.com/articles/concurrency-and-database-connections ?

#config/initializers/database_connection.rb
Rails.application.config.after_initialize do
  ActiveRecord::Base.connection_pool.disconnect!

  ActiveSupport.on_load(:active_record) do
    config = Rails.application.config.database_configuration[Rails.env]
    config['prepared_statements'] = false
    ActiveRecord::Base.establish_connection(config)
  end
end
@tomlea

This comment has been minimized.

Show comment Hide comment
@tomlea

tomlea Jan 14, 2014

Contributor

@schneems I'd prefer it if we could toggle it based on the DATABASE_URL. Having an an initializer that hard codes config feels like setting up trip wires for future me. Putting it in an environment variable gets me closer to happy, but if it's not in the DATABASE_URL it's still going to trip me.

Contributor

tomlea commented Jan 14, 2014

@schneems I'd prefer it if we could toggle it based on the DATABASE_URL. Having an an initializer that hard codes config feels like setting up trip wires for future me. Putting it in an environment variable gets me closer to happy, but if it's not in the DATABASE_URL it's still going to trip me.

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Jan 14, 2014

Contributor

You can still grab that value from DATABASE_URL:

uri                           = URI.parse(ENV["DATABASE_URL"])
params                        = CGI.parse(uri.query || "")
prepared_statements           = params['prepared_statements'] == 'true'
config['prepared_statements'] = prepared_statements
ActiveRecord::Base.establish_connection(config)
Contributor

schneems commented Jan 14, 2014

You can still grab that value from DATABASE_URL:

uri                           = URI.parse(ENV["DATABASE_URL"])
params                        = CGI.parse(uri.query || "")
prepared_statements           = params['prepared_statements'] == 'true'
config['prepared_statements'] = prepared_statements
ActiveRecord::Base.establish_connection(config)
@tomlea

This comment has been minimized.

Show comment Hide comment
@tomlea

tomlea Jan 14, 2014

Contributor

Yep, also a valid solution. To my taste (refined or otherwise), I prefer the original. It just feels better to be enhancing behaviour than to be duplicating behaviour.

It being a matter of taste though, I don't care too much which solution is documented, as long as it's documented. I just don't want anyone else to have to figure this one out the hard way (or the even harder way - in production).

Contributor

tomlea commented Jan 14, 2014

Yep, also a valid solution. To my taste (refined or otherwise), I prefer the original. It just feels better to be enhancing behaviour than to be duplicating behaviour.

It being a matter of taste though, I don't care too much which solution is documented, as long as it's documented. I just don't want anyone else to have to figure this one out the hard way (or the even harder way - in production).

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Jan 14, 2014

Contributor

Rails.application.database_configuration is a documented and supported interface as well as ActiveRecord::Base.establish_connection, as long as the above code runs, it should be valid in any Rails version, where with monkey patching we might be patching an interface that no longer gets used, or worse, causing unintended side effects.

I'm not sure if the ActiveSupport on_load hooks have remained consistent, but I think they should.

FWIW I fixed this in Rails 4.1. We're going to stop writing over the database.yml file and instead merge in config values from DATABASE_URL so if you upgrade to 4.1 when it releases you could do this in your database.yml

production:
  url: <%= ENV['DATABASE_URL'] %>
  prepared_statements: false

I'm still working on the code in the buildpack to allow this, but it should be a better experience then this. Thanks for reporting the problems you've been having and submitting this PR! I never even knew 3.2 had this problem 😄 ❤️. Maybe a middle ground solution is we could make a rails3 hacks file and in the README point there. We can document multiple solutions. WDYT @gregburek

Contributor

schneems commented Jan 14, 2014

Rails.application.database_configuration is a documented and supported interface as well as ActiveRecord::Base.establish_connection, as long as the above code runs, it should be valid in any Rails version, where with monkey patching we might be patching an interface that no longer gets used, or worse, causing unintended side effects.

I'm not sure if the ActiveSupport on_load hooks have remained consistent, but I think they should.

FWIW I fixed this in Rails 4.1. We're going to stop writing over the database.yml file and instead merge in config values from DATABASE_URL so if you upgrade to 4.1 when it releases you could do this in your database.yml

production:
  url: <%= ENV['DATABASE_URL'] %>
  prepared_statements: false

I'm still working on the code in the buildpack to allow this, but it should be a better experience then this. Thanks for reporting the problems you've been having and submitting this PR! I never even knew 3.2 had this problem 😄 ❤️. Maybe a middle ground solution is we could make a rails3 hacks file and in the README point there. We can document multiple solutions. WDYT @gregburek

@tomlea

This comment has been minimized.

Show comment Hide comment
@tomlea

tomlea Jan 14, 2014

Contributor

Sounds reasonable. Although I'd argue that it working in any Rails version is undesirable at this point, as it should be removed once an app is upgraded to 4.1 (perhaps consider suggesting a time bomb [1] in the final copy).

Merging in the config sounds like a great way to go, separating out the perserver/eployment from the per app config will make me very happy!

[1]

if Gem::Version.new(Rails.version) >= Gem::Version.new("4.1")
  raise "#{__FILE__} is no longer needed now you've updated Rails"
end 
Contributor

tomlea commented Jan 14, 2014

Sounds reasonable. Although I'd argue that it working in any Rails version is undesirable at this point, as it should be removed once an app is upgraded to 4.1 (perhaps consider suggesting a time bomb [1] in the final copy).

Merging in the config sounds like a great way to go, separating out the perserver/eployment from the per app config will make me very happy!

[1]

if Gem::Version.new(Rails.version) >= Gem::Version.new("4.1")
  raise "#{__FILE__} is no longer needed now you've updated Rails"
end 
README.md
+With Rails 4.1, you can disable prepared statements by appending `?prepared_statements=false` to the database's URI.
+Set the `PGBOUNCER_PREPARED_STATEMENTS` config var to `false` for the buildpack to do that for you.
+
+For Rails 3.2 - 4.0 you will first need to apply a simplified backport of [this commit](https://github.com/rails/rails/commit/e54acf1308e2e4df047bf90798208e03e1370098). Just create a new ruby file in `config/initializers` with the following content:

This comment has been minimized.

Show comment Hide comment
@gregburek

gregburek Feb 6, 2014

Contributor

I would prefer re-wording this line as:

Rails 3.2 - 4.0 also requires an initializer to properly cast the prepared_statements configuration string as a boolean. This initializer is adapted from this commit. In file config/initializers/database_connection.rb insert the following:

@gregburek

gregburek Feb 6, 2014

Contributor

I would prefer re-wording this line as:

Rails 3.2 - 4.0 also requires an initializer to properly cast the prepared_statements configuration string as a boolean. This initializer is adapted from this commit. In file config/initializers/database_connection.rb insert the following:

@gregburek

This comment has been minimized.

Show comment Hide comment
@gregburek

gregburek Feb 6, 2014

Contributor

It is unfortunate that this boolean casting is needed for some, but not all versions of Rails, as it makes it difficult to give clear, much less framework agnostic instructions.

In researching this, I also found some documentation for JDBC and PHP/PDO here Once this PR is merged, I will add that link along with a warning that everything is terrible and each framework will require its own thing to properly disable prepared statements.

Contributor

gregburek commented Feb 6, 2014

It is unfortunate that this boolean casting is needed for some, but not all versions of Rails, as it makes it difficult to give clear, much less framework agnostic instructions.

In researching this, I also found some documentation for JDBC and PHP/PDO here Once this PR is merged, I will add that link along with a warning that everything is terrible and each framework will require its own thing to properly disable prepared statements.

@tomlea

This comment has been minimized.

Show comment Hide comment
@tomlea

tomlea Feb 7, 2014

Contributor

Updated copy.

Contributor

tomlea commented Feb 7, 2014

Updated copy.

@gregburek

This comment has been minimized.

Show comment Hide comment
@gregburek

gregburek Feb 7, 2014

Contributor

@cwninja Thanks!

Contributor

gregburek commented Feb 7, 2014

@cwninja Thanks!

gregburek added a commit that referenced this pull request Feb 7, 2014

Merge pull request #7 from ms-digital-labs/readme-update-for-rails-3-2
Update readme to include fixes for rails 3.2.x.

@gregburek gregburek merged commit 7268307 into heroku:master Feb 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment