Skip to content
This repository

Inconsistencies when eager loading with fully-qualified table names using Oracle #2481

Closed
wants to merge 2 commits into from

6 participants

Simon Chiang Raimonds Simanovskis Isaac Sanders Steve Klabnik Rafael Mendonça França Jon Leighton
Simon Chiang

This pull request adds a test demonstrating how table names are not correctly detected by ActiveRecord::Relation. I've sketched out an insufficient patch (b039f1b) which allows the new test to pass for Oracle but breaks the test suite in other places, especially for other databases (or at least SQLite3).

In order to get the test to fail, run 'rake test_oracle' after setting up test/config.yml to use an Oracle database with a username like 'ops$user', or any valid variation that has non-word characters in it. It should pass otherwise.

Discussion

Eager loading will revert to the left-outer-join strategy when using fully-qualified table names on Oracle. For example:

  class Dept < ActiveRecord::Base
    set_table_name "dept"
  end

  class Emp < ActiveRecord::Base
    set_table_name "emp"
    belongs_to :dept, :select => "id, name nombre"
  end

  Emp.find(1, :include => :dept).dept.nombre       # ok
  Emp.find(:first, :include => :dept).dept.nombre  # ok

This code works as written because the default one-query-per-association strategy is used; as a result the select is used and nombre gets set on the dept record. However if you qualify the table name then the select is not used in some cases; find using an id breaks but find :first still works.

  class Dept < ActiveRecord::Base
    set_table_name "schema_name.dept"
  end

  class Emp < ActiveRecord::Base
    set_table_name "schema_name.emp"
    belongs_to :dept, :select => "id, name nombre"
  end

  Emp.find(1, :include => :dept).dept.nombre       # NoMethodError
  Emp.find(:first, :include => :dept).dept.nombre  # ok

In the id case eager loading reverts to the left-outer-join strategy and the select is ignored. The difference can be traced to the tables_in_string method which is responsible for extracting tables from the select string. This is how it's written in 2.3.5:

  # [active_record/association.rb]
  def tables_in_string(string)
    return [] if string.blank?
    string.scan(/([\.a-zA-Z_]+).?\./).flatten
  end

Turns out the regexp doesn't handle the fully-qualified names properly. There were some patches that partially fix this in 3.0.0 but the fix isn't complete. This is how the same method is currently:

  # [active_record/relation.rb]
  def tables_in_string(string)
    return [] if string.blank?
    # always convert table names to downcase as in Oracle quoted table names are in uppercase
    # ignore raw_sql_ that is used by Oracle adapter as alias for limit/offset subqueries
    string.scan(/([a-zA-Z_][.\w]+).?\./).flatten.map{ |s| s.downcase }.uniq - ['raw_sql_']
  end

This handles word-based schemas, but it will not handle other valid Oracle schemas like 'schema$name' nor will it handle tables with a db link. The method can be kind-of fixed with a fancier regexp:

  class ActiveRecord::Relation
    NONQUOTED_OBJECT_NAME   = /[A-Za-z][A-z0-9$#]{0,29}/
    NONQUOTED_DATABASE_LINK = /[A-Za-z][A-z0-9$#\.@]{0,127}/
    TABLES_IN_STRING = /((?:#{NONQUOTED_OBJECT_NAME}\.)?#{NONQUOTED_OBJECT_NAME}(?:@#{NONQUOTED_DATABASE_LINK})?)\..?/

    def tables_in_string(string)
      return [] if string.blank?
      string.scan(TABLES_IN_STRING).flatten.map {|str| str.downcase }.uniq - ['raw_sql_']
    end
  end

  class Dept < ActiveRecord::Base
    set_table_name "schema_name.dept"
  end

  class Emp < ActiveRecord::Base
    set_table_name "schema_name.emp"
    belongs_to :dept, :select => "id, name nombre"
  end

  Emp.find(1, :include => :dept).dept.nombre       # ok
  Emp.find(:first, :include => :dept).dept.nombre  # ok

But this is getting kinda wild. Moreover it's not correct for other databases.

A couple links:

And note that this reproduces and addresses issue 5200 from lighthouse, which was not migrated to github.

Simon Chiang add test for eager loading fully-qualified table names with oracle
This test addresses improper detection of valid table names by the
Relation#tables_in_string method on Oracle.  To make the test fail,
setup test/config.yml use an oracle database with a username like
'ops$user', or any valid variation that has non-word characters
in it.  It should pass otherwise.
d1f75d1
Raimonds Simanovskis
rsim commented August 10, 2011

It would be very good if tables_in_string method could be improved to correctly identify when to use eager loading in case of fully qualified table names. Maybe @tenderlove or @jonleighton have some ideas how to do it? Or maybe some ideas how to do it differently without using this complex regex on generated SQL string?

Isaac Sanders

@thinkerbot Is this still an issue?

Simon Chiang

Sure is. As of today the implementation of tables_in_string is the same as it was when this issue was started.

Isaac Sanders

@steveklabnik, as part of the Rails Issues team, may be able to help further.

Steve Klabnik
Collaborator

I'm not able to merge code, just triage issues. If it's still current, then someone else will have to check it out.

Steve Klabnik
Collaborator

... that said, knowing an issue from 9 months ago is still current is useful.

Rafael Mendonça França
Owner

We actually have some issues that are still useful. The problem with this pull request is that it is using a deprecated API (find(:first)). So if we want to merge it we need to change it first.

Simon Chiang

I honestly don't know the best solution to this but from the outside it seems like it would be good if the tables_in_string method were something the adapter specifies vs something baked into ActiveRecord itself.

Steve Klabnik
Collaborator

@jonleighton @tenderlove what do you think about this? Can it be merged?

Jon Leighton

The tables_in_string thing is inherently flawed, so it is going away and being replaced with an explicit references method:

Post.includes(:comments).where('comments.created_at = ?', 2.weeks.ago).references(:comments)

See the AR changelog for more detail.

So I think we can close this.

Jon Leighton jonleighton closed this September 14, 2012
Yasuo Honda yahonda referenced this pull request in rsim/oracle-enhanced November 12, 2012
Closed

Inconsistencies in eager loading with select #20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 2 authors.

Aug 09, 2011
Simon Chiang add test for eager loading fully-qualified table names with oracle
This test addresses improper detection of valid table names by the
Relation#tables_in_string method on Oracle.  To make the test fail,
setup test/config.yml use an oracle database with a username like
'ops$user', or any valid variation that has non-word characters
in it.  It should pass otherwise.
d1f75d1
Apr 28, 2012
Simon Chiang Fix test_with_fully_qualified_table_name to not use deprecated API :f…
…irst
4802d06
This page is out of date. Refresh to see the latest.
9  activerecord/test/cases/associations/eager_test.rb
@@ -22,12 +22,15 @@
22 22
 require 'models/club'
23 23
 require 'models/categorization'
24 24
 require 'models/sponsor'
  25
+require 'models/emp'
  26
+require 'models/dept'
25 27
 
26 28
 class EagerAssociationTest < ActiveRecord::TestCase
27 29
   fixtures :posts, :comments, :authors, :author_addresses, :categories, :categories_posts,
28 30
             :companies, :accounts, :tags, :taggings, :people, :readers, :categorizations,
29 31
             :owners, :pets, :author_favorites, :jobs, :references, :subscribers, :subscriptions, :books,
30  
-            :developers, :projects, :developers_projects, :members, :memberships, :clubs, :sponsors
  32
+            :developers, :projects, :developers_projects, :members, :memberships, :clubs, :sponsors,
  33
+            :emps, :depts
31 34
 
32 35
   def setup
33 36
     # preheat table existence caches
@@ -80,6 +83,10 @@ def test_with_two_tables_in_from_without_getting_double_quoted
80 83
     assert_equal 2, posts.first.comments.size
81 84
   end
82 85
 
  86
+  def test_with_fully_qualified_table_name
  87
+    assert_equal "Research", Emp.find(1, :include => :dept).dept.nombre
  88
+  end
  89
+
83 90
   def test_loading_with_multiple_associations
84 91
     posts = Post.find(:all, :include => [ :comments, :author, :categories ], :order => "posts.id")
85 92
     assert_equal 2, posts.first.comments.size
3  activerecord/test/fixtures/depts.yml
... ...
@@ -0,0 +1,3 @@
  1
+research:
  2
+  id: 1
  3
+  name: Research
4  activerecord/test/fixtures/emps.yml
... ...
@@ -0,0 +1,4 @@
  1
+doej:
  2
+  id: 1
  3
+  name: John Doe
  4
+  dept_id: 1
5  activerecord/test/models/dept.rb
... ...
@@ -0,0 +1,5 @@
  1
+class Dept < ActiveRecord::Base
  2
+  if connection.class.to_s == "ActiveRecord::ConnectionAdapters::OracleAdapter"
  3
+    set_table_name "#{connection.current_user.downcase}.depts"
  4
+  end
  5
+end
6  activerecord/test/models/emp.rb
... ...
@@ -0,0 +1,6 @@
  1
+class Emp < ActiveRecord::Base
  2
+  if connection.class.to_s == "ActiveRecord::ConnectionAdapters::OracleAdapter"
  3
+    set_table_name "#{connection.current_user.downcase}.emps"
  4
+  end
  5
+  belongs_to :dept, :select => "id, name nombre"
  6
+end
8  activerecord/test/schema/schema.rb
@@ -712,6 +712,14 @@ def create_table(*args, &block)
712 712
     t.string 'a$b'
713 713
   end
714 714
 
  715
+  create_table :emps, :force => true do |t|
  716
+    t.string :name
  717
+    t.integer :dept_id
  718
+  end
  719
+  create_table :depts, :force => true do |t|
  720
+    t.string :name
  721
+  end
  722
+
715 723
   except 'SQLite' do
716 724
     # fk_test_has_fk should be before fk_test_has_pk
717 725
     create_table :fk_test_has_fk, :force => true do |t|
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.