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

Transform options keys to symbols. #151

Conversation

joshuapinter
Copy link

@joshuapinter joshuapinter commented Jan 23, 2024

Before this, any options with a String key (e.g. "host") instead of a Symbol key (e.g. :host) would get silently ignored.

This is surprising in general but in particular, when using Rails and reading the database.yml config, they are all String keys so have no affect unless they are transformed before passing to Trilogy.new.

This reduces the surprises and ensures the keys are all transformed to Symbols before accessing them.

Fixes #149.

Before this, any options with a String key (e.g. "host") instead of a Symbol key (e.g. :host) would get silently ignored.

This is surprising in general but in particular, when using Rails and reading the `database.yml` config, they are all String keys so have no affect unless they are transformed before passing to `Trilogy.new`.

This reduces the surprises and ensures the keys are all transformed to Symbols before accessing them.
@brianmario
Copy link
Contributor

I would have expected Rails to be using HashWithIndifferentAccess here, strange...

@brianmario
Copy link
Contributor

I may be off track here, but I think this is where ActiveRecord is symbolizing the keys of the config hash before then being passed to and used by the underlying driver.

In any case, I think this is worth adding to the codebase 👍

@jhawthorn
Copy link
Member

I'm not sure about this. We shouldn't expect to be able to pass the hash Rails uses for configuration into Trilogy.new, they happen to be almost identical but aren't compatible. For example the :ssl_mode key in Rails is different https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb#L32-L33.

For other Rails adapters this is even more true and the keys are not the same https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L310-L320

I'm on the fence because this is probably fine but it might be better not to accept string keys and leave the option open for us to warn or error on invalid configuration in the future.

@joshuapinter
Copy link
Author

I think either transforming the keys to symbols or showing a warning would do the trick.

The silently ignoring them was problematic for us because this "worked" in Development:

config = Rails.configuration.database_configuration[ Rails.env ]
connection = Trilogy.new( config )
#=> <Trilogy:0x000000013a9de1e0

But in reality, the keys were just completely ignored and it wasn't until we got to Staging and Production that we saw an error like this:

config = Rails.configuration.database_configuration[ Rails.env ]
connection = Trilogy.new( config )
#=> .../trilogy-2.6.1/lib/trilogy.rb:18:in `_connect': No such file or directory - trilogy_connect - unable to connect to /tmp/mysql.sock (Trilogy::SyscallError::ENOENT)

We know about it now but doing one of these options would help somebody in the future getting caught off guard.

@eileencodes
Copy link
Member

I'm curious @joshuapinter, is there a reason are you connecting to the db with the config hash from Railties instead of Active Record? AR does a bunch of parsing to make the config from Railties valid, and with the introduction of db config objects, accessing the hash is discouraged.

I think the connection will work fine if you use ActiveRecord::Base.configurations.configs_for(env_name: Rails.env).first instead of Rails.configuration.database_configuration[Rails.env]?

@joshuapinter
Copy link
Author

Hi @eileencodes, we're connecting to a secondary, temporary database during a migration/conversion to pull data from the secondary database over to our primary database/app. It's in the same MySQL server with the same credentials but just a different database name.

So we pull the config for our primary database and then just change the database name and pass that config to Trilogy to return a connection we can write queries against.

This was the best way I found to do this but not sure if there's better ways.

@eileencodes
Copy link
Member

So we pull the config for our primary database and then just change the database name and pass that config to Trilogy to return a connection we can write queries against.

I would recommend adding another entry to the datatabase yaml that inherits the values from the primary and changes just the db name. But I'm still not sure why you would need the Railties version, you can access the config hash on the object with ActiveRecord::Base.configurations.configs_for(env_name: Rails.env).first.configuration_hash.

@joshuapinter
Copy link
Author

Thanks for the recommendation but database name is generated dynamically as part of the import/migration process.

That being said, I didn't know ActiveRecord::Base.configurations.configs_for(env_name: Rails.env).first.configuration_hash was a thing and it worked great.

The only thing I needed to do was .dup before passing it to Trilogy because it's frozen and I get a can't modify frozen Hash error otherwise.

Do you want me to close this PR?

@jhawthorn
Copy link
Member

Closing this. In the future we may warn or error, but it should be easy enough for callers to use symbols and Rails will also do this for us.

@jhawthorn jhawthorn closed this Sep 10, 2024
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.

Trilogy.connect not working in Staging and Production environments.
4 participants