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

Connect to database if not connected #309

Closed
wants to merge 1 commit into from
Closed

Connect to database if not connected #309

wants to merge 1 commit into from

Conversation

bijalhopkins
Copy link

Fix for this issue #308 (comment)

@mceachen mceachen mentioned this pull request May 10, 2018
@mceachen
Copy link
Collaborator

Thanks for the PR!

This is partially reverting a recent commit--let's make sure we're addressing everyone's use cases before merging (see #305)

@n-rodriguez
Copy link
Contributor

n-rodriguez commented May 25, 2018

Hi! As I was suspecting it breaks when the DB don't exist, for example during tests on a real app :

   ✔ 03 nicolas@localhost 8.502s
      04 ~/.rvm/bin/rvm 2.5.1 do bundle exec rake db:create
      04 Unable to connect to a database. Globalize skipped ignoring columns of translated attributes.
      04 Unable to connect to a database. Globalize skipped ignoring columns of translated attributes.
      04 rake aborted!
      04 PG::ConnectionBad: FATAL:  la base de données « concerto » n'existe pas
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/pg-1.0.0/lib/pg.rb:56:in `initialize'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/pg-1.0.0/lib/pg.rb:56:in `new'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/pg-1.0.0/lib/pg.rb:56:in `connect'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/postgresql_adapter.rb:684:in `connect'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/postgresql_adapter.rb:215:in `initialize'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/postgresql_adapter.rb:40:in `new'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/postgresql_adapter.rb:40:in `postgresql_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:809:in `new_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:853:in `checkout_new_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:832:in `try_to_checkout_new_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:793:in `acquire_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:521:in `checkout'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:380:in `connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:1008:in `retrieve_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_handling.rb:118:in `retrieve_connection'
      04 /home/nicolas/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/connection_handling.rb:90:in `connection'
      04 /home/nicolas/PROJECTS/CONCERTO/concerto/vendor/gems/closure_tree/lib/closure_tree/support_flags.rb:22:in `order_is_numeric?'
      04 /home/nicolas/PROJECTS/CONCERTO/concerto/vendor/gems/closure_tree/lib/closure_tree/support.rb:28:in `initialize'
      04 /home/nicolas/PROJECTS/CONCERTO/concerto/vendor/gems/closure_tree/lib/closure_tree/has_closure_tree.rb:16:in `new'
      04 /home/nicolas/PROJECTS/CONCERTO/concerto/vendor/gems/closure_tree/lib/closure_tree/has_closure_tree.rb:16:in `has_closure_tree'
      04 /home/nicolas/PROJECTS/CONCERTO/concerto/app/models/page.rb:45:in `<class:Page>'
      04 /home/nicolas/PROJECTS/CONCERTO/concerto/app/models/page.rb:24:in `<main>'

As you can notice, Globalize gem has the same chicken and egg issue :

      04 Unable to connect to a database. Globalize skipped ignoring columns of translated attributes.
      04 Unable to connect to a database. Globalize skipped ignoring columns of translated attributes.

It's here : https://github.com/globalize/globalize/blob/master/lib/globalize/active_record/act_macro.rb#L44

@mceachen
Copy link
Collaborator

mceachen commented May 25, 2018

@n-rodriguez thanks a bunch for taking the time to research that. Can you think of a way to write a breaking test so this can be automated?

@n-rodriguez
Copy link
Contributor

n-rodriguez commented May 25, 2018

Well... this one is a bit complicated : Ruby module inclusion depends on the return value of order_is_numeric? :

include ClosureTree::NumericDeterministicOrdering if _ct.order_is_numeric?

@n-rodriguez
Copy link
Contributor

The method is evaluated only once (I think) : at boot time.

@n-rodriguez
Copy link
Contributor

So the patch I've made is wrong because it will always return false and thus the module won't be included.

@n-rodriguez
Copy link
Contributor

In fact I don't why this part should be dynamic...

@n-rodriguez
Copy link
Contributor

n-rodriguez commented May 25, 2018

We make a DB query to check if the DB field is an integer... Well... (us) the developer should know in advance the field type and pass it as an option to has_closure_tree method.
It's just a suggestion as I don't know the implications of such a change ^^

@n-rodriguez
Copy link
Contributor

By reading the doc (https://github.com/ClosureTree/closure_tree#deterministic-ordering) it seems legit to get the info from the DB... but sometimes a good duplication is better than a bad abstraction ^^

@n-rodriguez
Copy link
Contributor

This one should fix it for good :) #311

@n-rodriguez
Copy link
Contributor

Hello! I think this issue can be closed as it's fixed by #311

@bijalhopkins
Copy link
Author

👍

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

Successfully merging this pull request may close these issues.

3 participants