Skip to content

Fix ambiguous ORDER BY clauses by fully qualifying column names #452

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eliezersouzareis
Copy link

@eliezersouzareis eliezersouzareis commented Jun 10, 2025

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

Eliezer Reis added 3 commits June 10, 2025 13:58
…with_order and has_many_order_with_option methods to use fully_qualified_order_by
@seuros
Copy link
Member

seuros commented Jul 21, 2025

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!

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

Successfully merging this pull request may close these issues.

2 participants