search

take-five / activerecord-hierarchical_query

Simple DSL for creating recursive queries with ActiveRecord
MIT License
119 stars 24 forks source link

Error: undefined method `bound_attributes` for ActiveRecord::Relation #20

Closed VitaliyAdamkov closed 5 years ago

VitaliyAdamkov commented 7 years ago

ruby 2.1.2, rails 3.2.14, gem version 1.0.0 When traversing descendants, i've got error from .rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/cte/non_recursive_term.rb:18:in `bind_values'

def bind_values
  scope.bound_attributes
end

I've looked into source here, at github, and have found that in master branch you have next lines in that file:

def bind_values
  scope.bind_values
end

When i replaced the line in this local file, i've got similar error for file .rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/cte/recursive_term.rb That error had been bypassed the same way and traversing started to work as expected. Help me, please, to use your very handful gem without that local files editings.

take-five commented 7 years ago

Could you please provide full error output: exception message + backtrace?

VitaliyAdamkov commented 7 years ago

ProductGroup.join_recursive { start_with(product_group_id: nil).connect_by(id: :product_group_id).order_siblings(:name) }

> NoMethodError:   ProductGroup Load (140.2ms)  SELECT "product_groups"."id", "product_groups"."product_group_id", ARRAY[ROW_NUMBER() OVER (ORDER BY "product_groups"."name" ASC)] AS __order_column FROM "product_groups" WHERE "product_groups"."product_group_id" IS NULL                                                                                                                                            
> undefined method `bound_attributes' for #<ActiveRecord::Relation:0x0055e7e7721da0>                                                                                                                         
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-3.2.14/lib/active_record/relation/delegation.rb:45:in `method_missing'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/cte/non_recursive_term.rb:18:in `bind_values'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/cte/union_term.rb:15:in `bind_values'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/cte/query_builder.rb:28:in `bind_values'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/join_builder.rb:74:in `bind_values'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/join_builder.rb:27:in `build'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query/query.rb:312:in `join_to'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query.rb:42:in `join_recursive'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-hierarchical_query-1.0.0/lib/active_record/hierarchical_query.rb:49:in `join_recursive'
>         from (irb):2
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/railties-3.2.14/lib/rails/commands/console.rb:47:in `start'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/railties-3.2.14/lib/rails/commands/console.rb:8:in `start'
>         from /home/mobil/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/railties-3.2.14/lib/rails/commands.rb:41:in `<top (required)>'
>         from bin/rails:4:in `require'
>         from bin/rails:4:in `<main>'
xtagon commented 6 years ago

I am getting this same error on Ruby 2.4.2, Rails 4.2.10 gem version 1.0.1

zachaysan commented 6 years ago

Is this still an issue in Rails 5.1? I'd like to tackle fixing it if so.

zachaysan commented 6 years ago

I'm able to reproduce. Working on it now.

zachaysan commented 5 years ago

Ok. I've got a fix and test ready I just need to clean up some things here and there. I'm modernizing all the gems as well, since I find keeping things current easier.

Since I'm taking over this project I'm going to be dropping support for Rails 4 and below. The Rails core team is doing some changes to how the Arel classes work and it won't be possible for me to structure things agnostically without a level of effort that I don't think it is worth.

But since this is my first real change to this repo I might not have my head fully around how everything fits together. So if @take-five agrees, I'm going to keep it on the rails-5 branch for now, though I'm bumping the version to 1.1.0 because the change.

I have to go offline until tomorrow, but should have the fix pushed in the next 24 hours.

zachaysan commented 5 years ago

Ok @take-five I've got a new branch called rails-5-upcoming that you can take a look at. My primary worry is that I misunderstood something fundamental, but all the tests pass in both this repo and my extensively tested / complicated Rails project. But out of an abundance of caution I decided to keep it off the rails-5 branch for now.

If it looks fine you can cut the gem for 1.1.0 which gives us Rails 5.2 support and fixes this bug.

Edit:

It looks like travis is failing due to the Rails 5.0 Gemfile. I'll take a look at why tonight, I suspect its gem conflicts and it shouldn't be hard to fix.

take-five commented 5 years ago

Hey @zachaysan, I took a look at your changes and they seem to be OK, however, I'd also add a failing unit test to be sure that your changes actually fix the problem. Sorry, I didn't work with Arel for last 3-4 years, so can't help you much with its internals anymore.

take-five commented 5 years ago

I'll release 1.1.0 tomorrow after Travis CI is fixed

zachaysan commented 5 years ago

@take-five отлично. (Я немного говорю по-русски.)

I figured out what was the issue with Travis. I'd assumed that the Rails core team was going to deprecate the changes they were making, but it seems like the Arel library is a "move fast and break things" part of Rails that core assumes people can keep up with. I narrowed support for this version to 5.2 and it looks like Travis is ok now.

How do you think I should go about testing the failing unit test? I did it manually of course, but short of undefining the Arel::As method I'm not sure how to go about it cleanly.

Thanks for being patient with me, I know how annoying it is to have someone new to a codebase.

xtagon commented 5 years ago

Personally I'm very grateful that you're taking on the task of keeping this project moving forward. If you could keep a Rails 4 branch separate for a while, that would be really nice since I'm using this on a Rails 4 application that I can't feasible upgrade to Rails 5 right away. So far any issue we've ran into has workarounds so it's not a big deal, just a little scary to drop support completely.

Thanks again for the contributions <3 and I look forward to testing out on Rails 5 as well.

take-five commented 5 years ago

How do you think I should go about testing the failing unit test?

You said earlier that you were able to reproduce the issue. How did you reproduce it? Maybe these steps could be transformed into a unit test?

take-five commented 5 years ago

I tried to add a test with a code snippet from https://github.com/take-five/activerecord-hierarchical_query/issues/20#issuecomment-298257855, but it doesn't fail

      it 'works' do
        expect(
          klass.join_recursive do
            start_with(parent_id: nil).connect_by(id: :parent_id).order_siblings(:name)
          end
        ).to include root, child_1, child_2, child_3, child_4, child_5
      end
xtagon commented 5 years ago

What happens if you call to_a on the relation before checking with .include? I think include behaves differently when you run it on a relation.

take-five commented 5 years ago

@xtagon, calling .to_a still doesn't reproduce the issue

zachaysan commented 5 years ago

Hey guys sorry, I had other things scheduled today. I'll go through these comments over the weekend and figure out how to reproduce.

As for Rails 4 support, if you like @xtagon I can explain to you what I've learned and you can handle the Rails 4 branch, but given that we're already talking about Rails 6 I really strongly recommend updating very soon. Most libraries don't follow more than one major release behind.

xtagon commented 5 years ago

Okay thanks. I plan to upgrade as soon as I can and if it keeps getting delayed then I can help with the Rails 4 branch.

I appreciate all the support from both of you, thanks 👍

zachaysan commented 5 years ago

You said earlier that you were able to reproduce the issue. How did you reproduce it? Maybe these steps could be transformed into a unit test?

Ok I finally understand my confusion here. Sorry, I'm still new to this.

I don't know what caused this bug in the past because I haven't looked at Rails 5.1 or less. I take the philosophy that it's better to spend 8 hours of a day fixing 10 things on the latest release than it is to fix 2 things and have them work for past releases.

Error: undefined method `bound_attributes`

Before my patch, I found a way to reproduce that ^ in my own private code base after upgrading to Rails 5.2. I was never able to with @VitaliyAdamkov's code. I figured his was out of date, but converted my code into the similarly structured code below:

    Category.left_outer_joins(:articles)
      .join_recursive do |query|
      query
        .connect_by(id: :parent_id)
    end

After some investigation I found that scopes now used hashes that map values to their attributes, not arrays. So I updated the methods to call values and I made them merge, instead of concat.

I also found that I had to update Arel::As because in 5.2 names were no longer always available. From what I could tell from the Rails core comments, there is an architectural change going on that I expect will continue, which is why I coded so defensively there. Rails 6 is going to have support for multiple databases so I suspect it may be related to this and this is why they've decided to start doing some of the changes at 5.2.

So the sum of my changes, as of 1.1.0, mean that we now no longer call bound_attributes anywhere in this Gem so this issue is now permanently solved. Because I was unable to re-create this issue prior to 5.2, 1.0.1 should continue to work for Rails 5.0 and Rails 5.1. If if is important to have 1.1.0 work for all minor releases of Rails 5 I can work that into the codebase but it will clutter it up a bit and I don't really see the point.

zachaysan commented 5 years ago

As mentioned in this comment chain, I'm going to make it so that 1.1.0 works for >5.0.

Will have something pushed early next week.

take-five commented 5 years ago

@zachaysan FYI, I granted you owner permissions on rubygems, so feel free to release new versions yourself

zachaysan commented 5 years ago

Great, thanks Alexey.

zachaysan commented 5 years ago

I finished the changes to support all minor versions of Rails and Travis CI build passed.

I'm going to push to Ruby Gems shortly then merge the upcoming-rails-5 branch to the rails-5 branch. Then I'll figure out what I need to do (update README, etc) to merge the rails-5 branch into master.

Pushing 1.1.0 to ruby gems shortly. If that goes smoothly I'm going to close this issue. Please open a new one if any new issues are encountered.

zachaysan commented 5 years ago

Just a FYI, I moved Rails 5 to the master branch and deleted the other Rails 5 branches. I created a new branch for Rails 4 and I upgraded some of the dependancies there as well. It seems like there is an issue with Ruby 2.4 and up with Rails 4.1 and down, so I prioritized supporting Rails 4.2 and modern Ruby, since that's the longterm supported version anyway.

Because I'm dropping support for Rails 3, I cut a new gem for 0.2.0 though other than dependancy updates, little has changed. I also tagged 1.1.0 and 0.2.0. This repo is now in a state that I'm happy with. If there are issues let me know.