Fix NoMethodError preparable for Arel::Visitors::PostgreSQL

[kelhusseiny]

After upgrading to 5.0.0.beta1 I got an error called

  `select_all': undefined method `preparable' for #<Arel::Visitors::PostgreSQL:0x007ff9416cf718> (NoMethodError)

So I did debug the problem and I found that module DetermineIfPreparableVisitor is extended only in case database configs for prepared_statements is true.

  @visitor = Arel::Visitors::PostgreSQL.new self
  if self.class.type_cast_config_to_boolean(config.fetch(:prepared_statements) { true })
    @prepared_statements = true
    @visitor.extend(DetermineIfPreparableVisitor)
  else
    @prepared_statements = false
  end

for that, I did that to fix the problem, and it works fine now 👍

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[matthewd]

Thanks! Can you add a test?

[kelhusseiny]

@matthewd After searching I found the only related file that tests DatabaseStatements here which doesn't include any tests for prepared_statements config. Do you think that I should add new tests for that case here?

[adityashedge]

Just wanted to say that this is a genuine issue with Postgresql which you can reproduce by setting prepared_statements: false in your database.yml.

Tested with gem 'rails', '>= 5.0.0.beta1', '< 5.1'

[kelhusseiny]

@adityashedge Yeah That's right. But our system has much complex queries which don't work with prepared_statements: true. For that we can't upgrade to rails5.
@matthewd Just a gentle reminder. What are we going to do with this please to get it merged?

[sgrif]

@Azzurrio Nothing has changed on this since his original comment. This needs a test.

[matthewd]

@Azzurrio I imagine pretty much any test that does a query will work to exercise this. Personally, I'd look for other tests that temporarily vary the connection configuration.

[kelhusseiny]

@matthewd & @sgrif Ok, I've added a test. Does it able to merge now?

[kelhusseiny]

@sgrif Just a reminder, Is this missing something else?

[sferik]

👍

[matthewd]

This pollutes the config for subsequent tests; we need a teardown that puts it back how it was

[kelhusseiny]

@matthewd done! 👍

[kelhusseiny]

@matthewd Are we done with this?

[matthewd]

Sorry... is it really true before this test runs? I would've thought it would be unset.

[kelhusseiny]

@matthewd yeah it was unset which defaults to true. Do you suggest to use Developer.connection_config.delete(:prepared_statements) instead?

[matthewd]

We should probably just store the existing value in a variable then put it back afterwards, so this test doesn't have any knowledge of what the default is. (Technically we'd be changing it from not-present to explicitly-nil, but I think it's safe to pretend those are the same.)

[kelhusseiny]

@matthewd Okay done 👍

[matthewd]

Thanks ❤️