Skip to content

Commit

Permalink
caching existence of a table
Browse files Browse the repository at this point in the history
  • Loading branch information
@tenderlove
tenderlove committed Jul 30, 2010
1 parent 165a312 commit 19c5a95
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions lib/arel/engines/sql/relations/table.rb
View file
Expand Up @@ -13,6 +13,8 @@ def tables= e; @@tables = e; end
end

attr_reader :name, :engine, :table_alias, :options, :christener
attr_reader :table_exists
alias :table_exists? :table_exists

def initialize(name, options = {})
@name = name.to_s
Expand Down Expand Up @@ -45,17 +47,15 @@ def initialize(name, options = {})
end

@@tables ||= engine.connection.tables
@table_exists = @@tables.include?(name) ||
@engine.connection.table_exists?(name)
end
end

def as(table_alias)
Table.new(name, options.merge(:as => table_alias))
end

def table_exists?
@table_exists ||= @@tables.include?(name) || engine.connection.table_exists?(name)
end

def attributes
return @attributes if defined?(@attributes)
if table_exists?
Expand Down

7 comments on commit 19c5a95

@svenfuchs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey aaron, this commit seems to break a bunch of usecases where we need to load a (activerecord) model class before the database connection is available.

I wonder what the reasoning behind the refactoring is. Do you think we can have this reverted by any chance?

Thanks for all your great work, man :)

@tenderlove
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Sven! This refactor is to avoid the "||" test on every call to table_exists?. I thought it would be safe because the constructor seems to assume an active connection. I am probably mistaken though. ;-)

I'm happy to revert this, but would you mind giving me a test case to show the problem? I'm afraid if I don't have a test case, I'll end up making this change again. Thanks!

@svenfuchs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great :) Sure, I'll provide a test

@svenfuchs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Aaron, I've pushed two fixes here: http://github.com/svenfuchs/arel/commits/master

I'm not sure the tests in http://github.com/svenfuchs/arel/commit/4b476404cbbecfedc255039c66c6eececb667d7f#L1R122 will work fine for all adapters. Unfortunately I was only able to run this on Sqlite3 right now.

@svenfuchs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, Aaron, I've seen you've tagged an rc ... could you pull this before the final release?

@tenderlove
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sven done, but you owe me a test. ;-)

@svenfuchs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks :)

But there's tests here: http://github.com/svenfuchs/arel/commit/4b476404cbbecfedc255039c66c6eececb667d7f#L1R120

Or do you mean they aren't passing on any other db engine?

Also, have you seen this one? http://github.com/svenfuchs/arel/commit/3b1b24551106bc116cba404c992b513c5fbd137b

It's a real bug w/ Rails 3.0.0.rc2 imho, because one can't use Table#as at all if the Table has been initialized with an engine as a second argument (which seems to happen in ActiveRecord all the time?)

Please sign in to comment.