Fix ambiguous ORDER BY clauses by fully qualifying column names

[eliezersouzareis]

This pull request addresses potential SQL ambiguity issues in ORDER BY clauses by introducing and using a new method: fully_qualified_order_by.

Changes made:

• Introduced fully_qualified_order_by, which combines the quoted table and column name with sort direction.

• Replaced ambiguous order_by references with fully_qualified_order_by in:

  • reorder_with_parent_id (in numeric_order_support.rb)
  • with_order_option, scope_with_order, and has_many_order_with_option (in support.rb)

Why?

tldr: If you try to use joins or subqueries in a relationship and you have a positional column with the same name the closure tree uses you are going to have, see and feel pain inflicted upon you.
This PR aims to fix this.

Suppose you have two closure trees for different things and they share a relationship;

class InternalCategory < ApplicationRecord
  has_closure_tree order: "position"
end

class ExternalCategory < ApplicationRecord
  has_closure_tree order: "position"
  has_many :internal_category_classifications
  has_many :internal_categories, through: :internal_category_classifications
  
  has_many :descendants_internal_category, through: :self_and_descendants, source: :internal_categories
  
  has_many :bar_external_categories
end

class Bar < ApplicationRecord
  has_many :bar_external_categories,
  has_many :external_categories
end

class Foo < ApplicationRecord
  belongs_to :bar
  has_many :external_categories, through: :bar
end

class Biz < ApplicationRecord
  belongs_to :foo
  belongs_to :internal_category # always a root category
  
  has_many :external_categories,
                     lambda { |biz|
                       joins(:internal_categories)
                         .joins("inner join internal_category_hierarchies on internal_categories.id = internal_category_hierarchies.descendant_id")
                         .where("internal_category_hierarchies.ancestor_id = #{biz.internal_category_id}")
                     },
                     through: :foo,
                     source:    :external_categories
end

Biz#external_categories will trip up a "PG::AmbiguousColumn: ERROR: column reference "position" is ambiguous" on postgresql

The real world use case is a lot more convoluted than this, but I think I simplified it enough to be understandable to why the order expression should be fully qualified with table name and column name

[seuros]

Hi @eliezersouzareis!

Thanks for this PR addressing the ambiguous ORDER BY clauses. Good news - I just released v9.0.0 of closure_tree, which includes a major refactoring where I've converted many hardcoded SQL queries to use Arel (PR #453). This conversion should resolve the ambiguity issues you're experiencing, as Arel automatically handles proper table aliasing and column qualification.

Could you please test your use case with v9.0.0 and let me know if the issue persists? If it does, I'd be happy to work on additional fixes. The Arel conversion specifically addresses:

• Proper table aliasing in complex joins
• Automatic column qualification to prevent ambiguity
• Database-agnostic query generation

You can update to v9.0.0 by running:

bundle update closure_tree

Thanks again for identifying this issue and providing such a detailed explanation!

[eliezersouzareis]

Hi. Since you bumped both the active_record dependency and the Ruby version requirement, I can’t test your changes because I’m stuck on Ruby on Rails 5.1.7.

I could, of course, fork the repo and downgrade the version requirements, but that would likely also mean adjusting the code. At that point, it would be moot, since I wouldn’t really be testing your work. That feels like more work than I can reasonably take on just to test this.

Thanks for maintaining the gem and keeping it moving forward.

[seuros]

Thank you.

I could not reproduce the issue with the details you provided, so i seem that the ordering was an ActiveRecord issue or our usage of hardcoded SQL.

I believe the new version since we use Arel, should not cause this issue anymore.