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

introduce `conn.data_source_exists?` and `conn.data_sources`. #21715

Merged
merged 1 commit into from Sep 22, 2015

Conversation

@senny
Copy link
Member

senny commented Sep 22, 2015

These new methods are used from the Active Record model layer to
determine which relations are viable to back a model. These new methods
allow us to change conn.tables in the future (#21601) to only return tables and
no views. Same for conn.table_exists?.

The goal is to provide the following introspection methods on the
connection:

  • tables
  • table_exists?
  • views
  • view_exists?
  • data_sources (views + tables)
  • data_source_exists? (views + tables)

ToDo

  • check wether optimized #data_sources and #data_source_exists? implementation is possible for specific adapters. Instead of querying views and tables separately.
@senny
Copy link
Member Author

senny commented Sep 22, 2015

/cc @yui-knk @matthewd

@rafaelfranca @sgrif what do you guys think?

@senny senny added the activerecord label Sep 22, 2015
@senny senny force-pushed the introduce_data_sources branch 2 times, most recently from 1afa301 to 390da81 Sep 22, 2015
@senny senny self-assigned this Sep 22, 2015
@senny senny force-pushed the introduce_data_sources branch from 390da81 to 5a5ffaf Sep 22, 2015
@matthewd
Copy link
Member

matthewd commented Sep 22, 2015

👍

Here, we distinguish table-qua-table from "thing we can run queries against" -- sometimes we need to know whether a thing is a table or a view (or something else), but most of the time, we want to treat them interchangeably. Thus, better to give that concept a name, and address it unambiguously... at least in the closer-to-the-metal layers: models can still use "table" when they actually mean data_source, because they never really mean table.

Technically the right word for this would be relation, but I don't think its precision warrants the confusion due to the overlap with AR::Relation.

@rafaelfranca
Copy link
Member

rafaelfranca commented Sep 22, 2015

👍

@sgrif
Copy link
Contributor

sgrif commented Sep 22, 2015

Looks good to me.

@@ -23,6 +23,20 @@ def table_alias_for(table_name)
table_name[0...table_alias_length].tr('.', '_')
end

# Returns the relations names usuable to back Active Record models.
# Defaults to all #tables and #views.

This comment has been minimized.

Copy link
@yui-knk

yui-knk Sep 22, 2015

Contributor

I feel it is not "defaults".
For example is it better: ?

data sources mean all tables and views

This comment has been minimized.

Copy link
@yui-knk

yui-knk Sep 22, 2015

Contributor

plus in Changelog

These methods determine what relations can be used to back Active Record models (usually tables and views).

Do we intend that user can override data_sources when they would not use views, they would not use tables, or they would use some lists of table names as Active Record back?
If so, senny's comment is reasonable and how about to add these comments

You can override this method if you would not use views (or tables) as Active Record back.
def data_sources
tables
end

This comment has been minimized.

Copy link
@senny

senny Sep 22, 2015

Author Member

@yui-knk it's not so much about wether the user can overwrite these methods but the adapters can. We can't say for sure that every adapter will have working views and tables as data source.

@@ -23,6 +23,20 @@ def table_alias_for(table_name)
table_name[0...table_alias_length].tr('.', '_')
end

# Returns the relations names usuable to back Active Record models.

This comment has been minimized.

Copy link
@kaspth

kaspth Sep 22, 2015

Member

useable 😁

assert_equal 'id', @cache.primary_keys('posts')
end

def test_table_methods_deprecation
assert_deprecated { assert @cache.table_exists?('posts') }

This comment has been minimized.

Copy link
@yui-knk

yui-knk Sep 22, 2015

Contributor

Is this

assert_deprecated { @cache.table_exists?('posts') } ?

This comment has been minimized.

Copy link
@senny

senny Sep 22, 2015

Author Member

why? I put the assert there to make sure that it actually still returns true in the case of an existing table.

assert_equal 'id', @cache.primary_keys('posts')
end

def test_table_methods_deprecation
assert_deprecated { assert @cache.table_exists?('posts') }
assert_deprecated { assert @cache.tables('posts') }

This comment has been minimized.

Copy link
@yui-knk

yui-knk Sep 22, 2015

Contributor

dito

@@ -47,6 +47,11 @@ def test_table_exists
assert @connection.table_exists?(view_name), "'#{view_name}' table should exist"
end

def test_views_ara_valid_data_sources
view_name = Ebook.table_name
assert @connection.view_exists?(view_name), "'#{view_name}' should be a data source"

This comment has been minimized.

Copy link
@yui-knk

yui-knk Sep 22, 2015

Contributor

assert @connection.data_source_exists?(view_name), "'#{view_name}' should be a data source" ??

@@ -36,6 +36,21 @@ def test_table_exists?
assert !@connection.table_exists?(nil)
end

def test_data_sources
tables = @connection.data_sources

This comment has been minimized.

Copy link
@yui-knk

yui-knk Sep 22, 2015

Contributor

I feel data_sources = @connection.data_sources is better because name of this test is "test_data_sources"

@yui-knk
Copy link
Contributor

yui-knk commented Sep 22, 2015

conn.data_source_exists? and conn.data_sources are good! Thanks 💛

These new methods are used from the Active Record model layer to
determine which relations are viable to back a model. These new methods
allow us to change `conn.tables` in the future to only return tables and
no views. Same for `conn.table_exists?`.

The goal is to provide the following introspection methods on the
connection:

* `tables`
* `table_exists?`
* `views`
* `view_exists?`
* `data_sources` (views + tables)
* `data_source_exists?` (views + tables)
@senny senny force-pushed the introduce_data_sources branch from 5a5ffaf to 152b85f Sep 22, 2015
senny added a commit that referenced this pull request Sep 22, 2015
introduce `conn.data_source_exists?` and `conn.data_sources`.
@senny senny merged commit 7805fa2 into master Sep 22, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@senny senny deleted the introduce_data_sources branch Sep 22, 2015
senny added a commit that referenced this pull request Nov 5, 2015
…ata_sources"

introduce `conn.data_source_exists?` and `conn.data_sources`.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
	activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
	activerecord/lib/active_record/connection_adapters/schema_cache.rb
	activerecord/lib/active_record/schema_dumper.rb
	activerecord/lib/active_record/type_caster/connection.rb
	activerecord/test/active_record/connection_adapters/fake_adapter.rb
	activerecord/test/cases/adapters/postgresql/schema_test.rb
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Feb 22, 2017
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.