Faveod / arel-extensions

Extending Arel
MIT License
142 stars 24 forks source link

Strange behaviour with ActiveRecord #10

Closed cpinto closed 5 years ago

cpinto commented 5 years ago

After enabling arel_extensions some of my tests started failing, specifically ones verifying the results meet a certain order. If I remove the gem, the tests pass. The strange behaviour I'm seeing is:

> Job::Task.ordered.pluck(:position)
=> [1, 2, 3, 0]

> Job::Task.ordered.to_sql 
=> "SELECT \"job_tasks\".* FROM \"job_tasks\" ORDER BY \"job_tasks\".\"position\" ASC"

> Job::Task.ordered.first.position 
=> 0

> Job::Task.ordered.last.position 
=> 3

> Job::Task.ordered.map {|t| t.position } 
=> [1, 2, 3, 0]

> ActiveRecord::Base.connection.exec_query(Job::Task.ordered.select(:id,:position).to_sql).to_a
=> [{"id"=>118, "position"=>0}, {"id"=>115, "position"=>1}, {"id"=>116, "position"=>2}, {"id"=>117, "position"=>3}]

> Job::Task.ordered.select(:id,:position).to_a 
=> [#<Job::Task id: 115, position: 1>, #<Job::Task id: 116, position: 2>, #<Job::Task id: 117, position: 3>, #<Job::Task id: 118, position: 0>]

Is this a known issue? Any hints on how I can dig deeper to understand what may be causing this?

TIA

cpinto commented 5 years ago

Upon closer inspection, this may be related to the postgresql visitor as it's the only arel-extensions code being executed, as per the trace: https://gist.github.com/cpinto/cd84e67bdbb48e706a05fa0ce94b6f6f

cpinto commented 5 years ago

I'm just learning Ruby/Rails so this may be off: what I found is that this change causes the problem, by reseting the order array: https://github.com/Faveod/arel-extensions/commit/b3f0e7112b22edabd67c67136290d2bd1275d03b.

I can't seem to find a bug report attached to the patch.

When the method visit_Arel_Nodes_SelectStatement is called, the collector.value is ["",[]]. I'm not sure what's causing it. However if I change the if statement from this: if !collector.value.blank? && o.limit.blank? && o.offset.blank? to this: if !collector.value.reject{|e| e.blank?}.blank? && o.limit.blank? && o.offset.blank?

I get the intended result.

Could this be related to any recent changes in AR?

cpinto commented 5 years ago

Quick update to share that sometimes the collector.value is a string so the patch had to accommodate for both an array of empty elements as well as a string.

jdelporte commented 5 years ago

Hi Celso,

Thank you for bringing this issue to us. It appears it has been some changes on the use of the collector in ActiveRecord/Arel. As you pointed it out, the collector.value is now an array that contains the sql query string and the different bindings. I just pushed a new commit on git, that should support this new behavior.

Regards,

cpinto commented 5 years ago

@jdelporte thank you very much for looking into this.