search

liveh2o / spectacles

ActiveRecord Views for PostgreSQL
MIT License
90 stars 11 forks source link

Get the user from the connection config #24

Closed liveh2o closed 8 years ago

liveh2o commented 8 years ago

The new database_user method added to the PostgreSQL adapter in #23 was expecting to pull the database user from where Rails' stores the configuration it loads from the database.yml. This won't work for a couple reasons:

  1. Spectacles is not Rails-dependent and thus won't always have a config to reference.
  2. Even when it does, ActiveRecord::Base.establish_connection can be called with a different configuration (this is actually how the specs in this gem work, which why the PostgreSQL specs started failing after this was merged).

Additionally, the method was expecting a YAML file to have a config nested under "defaults", which is not standard and cannot be guaranteed to exist.

A safer way to solve this would be to get the database user from the config that was used to establish the current connection. Unfortunately, this means accessing an instance variable directly (i.e. the connection class does not provide a config method or attribute), but I don't see any other way to guarantee the correct database user for the current connection is returned.

// @mmmries @jamis

liveh2o commented 8 years ago

Also, I should really get this setup in Travis so these things surface before the PR is merged...

nailed_it

mmmries commented 8 years ago

So sorry for the failing specs. No idea why I didn't run them locally after my change was made :sob:

I'll pay my penance by opening a PR to setup Travis

mmmries commented 8 years ago

It's unfortunate that we can't find a more reliable way of getting at the current config than using that instance variable. If anyone knows a better way to get to the config I would love to hear it, but if this is the best we have then I'm :+1: for rolling with it.

mmmries commented 8 years ago

bump, I know using @config is not ideal, but I think we should get this moving even so.