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

AR causes stack level too deep when joining tables #25653

Closed
krzysiek1507 opened this issue Jul 2, 2016 · 9 comments
Closed

AR causes stack level too deep when joining tables #25653

krzysiek1507 opened this issue Jul 2, 2016 · 9 comments

Comments

@krzysiek1507
Copy link
Contributor

Steps to reproduce

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'activerecord', '5.0.0'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  def self.test_it
    current_scope.to_sql
  end
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_through_joins
    Post.joins(comments: :post).test_it
  end
end

Expected behavior

Should work same as in AR4.2.6 (I hope).

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'activerecord', '4.2.6'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  def self.test_it
    current_scope.to_sql
  end
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_through_joins
    Post.joins(comments: :post).test_it
  end
end

Actual behavior

Raises

SystemStackError: stack level too deep

System configuration

Rails version: 5.0.0

Ruby version: 2.2.4

@krzysiek1507
Copy link
Contributor Author

Related to #18109

@mctaylorpants
Copy link
Contributor

It looks like the regression was introduced by 12e9e38.

@maclover7
Copy link
Contributor

cc @sgrif

@kamipo
Copy link
Member

kamipo commented Jul 5, 2016

@k0kubun Can you take a look at this?

@k0kubun
Copy link
Contributor

k0kubun commented Jul 5, 2016

I guess this can be resolved in the same way as #25187 and only that direction can solve both problems. But as you can see from the discussion in the PR, scoping(current_scope) itself is going to be removed and I guess the fix won't be done.

By the way, just curious, why do you do circular joins and refer to current_scope? I couldn't imagine the use case from the code. (I understand stack level too deep is definitely a bug and it should be handled properly.)

@krzysiek1507
Copy link
Contributor Author

@k0kubun don't ask why. :D I found this use case in some gem.

@k0kubun
Copy link
Contributor

k0kubun commented Jul 5, 2016

don't ask why. :D I found this use case in some gem.

Ah, I see. It may be difficult to fix since it's caused in a gem. Sorry for the trouble and thank you for reporting it.

@k0kubun
Copy link
Contributor

k0kubun commented Jul 5, 2016

Sent a patch to fix this issue. #25702
Please tell me if the branch does not solve your problem.

@krzysiek1507
Copy link
Contributor Author

@k0kubun branch solves the problem.

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

No branches or pull requests

5 participants