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

All AR::ConnectionAdapters#tables return only tables(exclude views) #21601

Merged
merged 1 commit into from Nov 12, 2015

Conversation

@yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Sep 12, 2015

As reported on #21509,
tables method of adapters for mysql and sqlite include view
but for postgresql does not include. This commit fix all of them
to return only tables(exclude views). Close #21509.

This commit includes these fixes:

Implement tables_and_views for mysql and sqlite

tables method of adapters for mysql and sqlite has two
functions.

  1. Return all tables names
  2. Return all tables names which matches to database name or table name
    (specified by argument)

Move no. 2 function to tables_and_views method.

Change arguments of tables method

Now the responsibility of tables is to return all tables names,
so change this method to be argument-less method.

Change some tests

  • test_tables_quoting in activerecord/test/cases/adapters/mysql2/schema_test.rb
    This test asserts database name is quoted internally. tables_and_views accepts
    database name. So change from @connection.tables to @connection.tables_and_views.
  • test_tables_logs_name in activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
    This test asserts what SQL is executed. SQL created by tables is changed, so change sql
    used to assertion.
@rails-bot
Copy link

@rails-bot rails-bot commented Sep 12, 2015

r? @arthurnn

(@rails-bot has picked a reviewer for you, use r? to override)

end
end

def tables_and_views(name = nil, database = nil, like = nil) #:nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2015
Member

I don't think name is being used anywhere so lets remove it.

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2015
Member

Also can we make this method private? It is not being used anywhere besides test.

This comment has been minimized.

@yui-knk

yui-knk Sep 13, 2015
Author Contributor

it is my mistakes, I will fix them :)

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Sep 12, 2015

I'm not sure. This is a huge change on behavior. tables is public API and I bet someone is depending in the fact that it returns also view. I vote to just document that it returns views.

@sgrif @senny @matthewd thoughts?

@sgrif
Copy link
Contributor

@sgrif sgrif commented Sep 12, 2015

This will break apps I have worked on in the past for sure.

@sgrif
Copy link
Contributor

@sgrif sgrif commented Sep 12, 2015

The inconsistency between PG and the rest bothers me as well, though.

@senny
Copy link
Member

@senny senny commented Sep 13, 2015

We should deal with that inconsistency somehow. It's quite the difference in behavior and client code for all adapters can never be sure what they'll get. I think there's a handful of issues on GitHub about that behavior.

One approach that comes to mind would be deprecating tables() for Rails 5 in favor of tables(views: true) and tables(views: false). This would force callers to be specific about what they want and we can maintain the current inconsistent behavior of tables(). At a later stage (Rails 5.1), we can then move tables() to the new behavior. Either returning both views and tables or just tables. Whichever is preferable.

@yui-knk
Copy link
Contributor Author

@yui-knk yui-knk commented Sep 13, 2015

@rafaelfranca

bet someone is depending in the fact that it returns also view

Sure. But at the same time, I think someone is depending in the fact that it does not return view (postgresql users). I am not sure changing all adapters to returns table + view is better than only return table.

@senny
Now #21609 PR is posted. So user can views + talbes to get both of them. I think tables should return only table in the future. Is it better add views option to tables (tables(views: true)) and waning for Rails 5.0?

@senny
Copy link
Member

@senny senny commented Sep 13, 2015

I need to have a better look where tables is used but my first reaction is that tables should return both tables and views. My thinking is that at it's core a view is a virtual table. Many features that work with a table can also work with a view. That's just a guess though, we need to look through our current usage to determine what's better.

I like the idea of passing in view: true and view: false because this let's us make the change in a completely backwards compatible manner. In the first step we can deprecate calling tables without any options and describing the future behavior change. After that it's save to change tables without any arguments.

@senny senny assigned senny and unassigned arthurnn Sep 13, 2015
@senny
Copy link
Member

@senny senny commented Sep 13, 2015

/cc @matthewd

@senny
Copy link
Member

@senny senny commented Sep 22, 2015

@yui-knk #21609 was merged, is there anything that you'd like to update?

@senny
Copy link
Member

@senny senny commented Sep 22, 2015

@yui-knk please hold changes to this PR, I'm currently working on a patch that should make the situation for tables more clear. I'll ping you on the PR when it's ready.

@senny
Copy link
Member

@senny senny commented Sep 22, 2015

@yui-knk I merged my patch. We can now put in place the necessary deprecations to change the behavior of #tables. I suggest the following:

  • Deprecate #tables on all the adapters, where views and tables are returned in favor of data_sources but keep the current implementation.
  • Keep #tables as is on adapters that only return tables.

Then after 5.0 lands no-one should be relying on #tables behavior with views any longer and we can safely change the implementations and remove the deprecations.

This means that in the transition phase. Basically 5.0 some adapters won't have a method to only get tables. But that shouldn't be too much of a problem as that is the same situation that we have right now.

@yui-knk yui-knk force-pushed the yui-knk:fix/ar_tables_without_view2 branch 2 times, most recently to 308406e Sep 23, 2015
@yui-knk
Copy link
Contributor Author

@yui-knk yui-knk commented Sep 23, 2015

@senny I updated to only warning!

@@ -70,6 +72,10 @@ def drop_database(name) #:nodoc:

# Returns the list of all tables in the schema search path.
def tables(name = nil)
ActiveSupport::Deprecation.warn(<<-MSG.squish) if name
And passing arguments to #tables is deprecated.

This comment has been minimized.

@kamipo

kamipo Sep 23, 2015
Member

Looks like no need And.

This comment has been minimized.

@yui-knk

yui-knk Sep 23, 2015
Author Contributor

Thank, I fixed it. Copy & paste is evil 👿

@yui-knk yui-knk force-pushed the yui-knk:fix/ar_tables_without_view2 branch from 308406e to ddb3eae Sep 23, 2015
@@ -626,9 +627,19 @@ def collation
end

def tables(name = nil) # :nodoc:
ActiveSupport::Deprecation.warn(<<-MSG.squish)
#tables of mysql adapter returns tables and views,

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

What about the following wording:

    #tables currently returns both tables and views.
    This behavior is deprecated and will be changed with Rails 5.1 to only return tables.
    Use #data_sources instead.
#tables of mysql adapter returns tables and views,
in Rails 5.1 this method will be changed to return only tables.
If you need tables and views, please use #data_sources.
#{"And passing arguments to #tables is deprecated." if name }

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

Can we split this off into a separate deprecation?

        if name
          ActiveSupport::Deprecation.warn(<<-MSG.squish)
            Passing arguments to #tables is deprecated without replacement.
          MSG
        end

Reported on #21509, how views is treated by `#tables` are differ
by each adapters. To fix this different behavior, after Rails 5.0
is released, deprecate `#tables`.

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

Maybe:?

*   Deprecate `connection.tables` on the SQLite3 and MySQL adapters.
    Also deprecate passing arguments to `#tables`.

    The `#tables` method of some adapters (mysql, mysql2, sqlite3) would return
    both tables and views while others (postgresql) just return tables. To make
    their behavior consistent `#tables` will return only tables in the future.

    *Yuichiro Kaneko*
@@ -38,6 +38,7 @@ def data_source_exists?(name)
end

# Returns an array of table names defined in the database.
# Returning tables only or including views depends on each adapters.

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

I don't think it's worth to document this. It's been like this for a long time. I'd keep the docs for this method as is.

@@ -70,6 +72,10 @@ def drop_database(name) #:nodoc:

# Returns the list of all tables in the schema search path.
def tables(name = nil)
ActiveSupport::Deprecation.warn(<<-MSG.squish) if name
Passing arguments to #tables is deprecated.

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

Passing arguments to #tables is deprecated without replacement.

if current_adapter?(:MysqlAdapter, :Mysql2Adapter, :SQLite3Adapter)
assert_deprecated { @connection.tables }
elsif current_adapter?(:PostgreSQLAdapter)
assert_deprecated { @connection.tables(:books) }

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

Let's split this into a separate test-case. Passing arguments is not just deprecated for the PostgreSQL adapter:

    if current_adapter?(:MysqlAdapter, :Mysql2Adapter, :SQLite3Adapter)
      def test_tables_returning_both_tables_and_views_is_deprecated
        assert_deprecated { @connection.tables }
      end
    end

    def test_passing_arguments_to_tables_is_deprecated
      assert_deprecated { @connection.tables(:books) }
    end
@@ -308,6 +309,13 @@ def exec_rollback_db_transaction #:nodoc:
# SCHEMA STATEMENTS ========================================

def tables(name = nil, table_name = nil) #:nodoc:
ActiveSupport::Deprecation.warn(<<-MSG.squish)
#tables of sqlite3 adapter returns tables and views,

This comment has been minimized.

@senny

senny Sep 30, 2015
Member

same as above.

@senny
Copy link
Member

@senny senny commented Sep 30, 2015

@yui-knk running the update produces tons of warnings. We should change our internals to no longer depend on any of the deprecated behavior. This means to also touch on the behavior of table_eixsts? and look into deprecations on that end as well.

@@ -21,7 +21,7 @@ def index_name
end

def table_exists?
connection.table_exists?(table_name)
connection.data_source_exists?(table_name)

This comment has been minimized.

@kamipo

kamipo Nov 9, 2015
Member

SchemaMigration#table_exists? is used for SchemaMigration#create_table and SchemaMigration#drop_table. It seems that should use table_exists? rather than data_source_exists?.

This comment has been minimized.

@yui-knk

yui-knk Nov 9, 2015
Author Contributor

Ditto :)

@@ -87,7 +87,7 @@ def create_view(name, query)
end

def drop_view(name)
@connection.execute "DROP VIEW #{name}" if @connection.table_exists? name
@connection.execute "DROP VIEW #{name}" if @connection.data_source_exists? name

This comment has been minimized.

@kamipo

kamipo Nov 9, 2015
Member

view_exists??

This comment has been minimized.

@yui-knk

yui-knk Nov 9, 2015
Author Contributor

Yes 🙇

Reported on #21509, how views is treated by `#tables` are differ
by each adapters. To fix this different behavior, after Rails 5.0
is released, deprecate `#tables`.

And `#table_exists?` would check both tables and views.
To make their behavior consistent with `#tables`, after Rails 5.0
is released, deprecate `#table_exists?`.
@yui-knk yui-knk force-pushed the yui-knk:fix/ar_tables_without_view2 branch to 7429633 Nov 9, 2015
senny added a commit that referenced this pull request Nov 12, 2015
All `AR::ConnectionAdapters#tables` return only tables(exclude views)
@senny senny merged commit 19398ab into rails:master Nov 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@senny
Copy link
Member

@senny senny commented Nov 12, 2015

🎉 @yui-knk thank you for working on this! 💛

@yui-knk
Copy link
Contributor Author

@yui-knk yui-knk commented Nov 12, 2015

@senny 🎉 thanks for your review !!!! 😍

@yui-knk yui-knk deleted the yui-knk:fix/ar_tables_without_view2 branch Nov 12, 2015
kamipo added a commit to kamipo/rails that referenced this pull request Nov 19, 2015
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request May 20, 2016
also deprecate `#table_exists?`, `#tables` and passing arguments to `#tables`

Refer rails/rails#21601
rails/rails#21509
kamipo added a commit to kamipo/rails that referenced this pull request Jan 3, 2017
Passing `name` to `tables` is already deprecated at rails#21601.
Passing `name` to `indexes` is also unused.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants