Skip to content
Permalink
Browse files

Change `having_values` to use the `WhereClause` class

This fixed an issue where `having` can only be called after the last
call to `where`, because it messes with the same `bind_values` array.
With this change, the two can be called as many times as needed, in any
order, and the final query will be correct. However, once something
assigns `bind_values`, that stops. This is because we have to move all
of the bind values from the having clause over to the where clause since
we can't differentiate the two, and assignment was likely in the form
of:

`relation.bind_values += other.bind_values`

This will go away once we remove all places that are assigning
`bind_values`, which is next on the list.

While this fixes a bug that was present in at least 4.2 (more likely
present going back as far as 3.0, becoming more likely in 4.1 and later
as we switched to prepared statements in more cases), I don't think this
can be easily backported. The internal changes to `Relation` are
non-trivial, anything that involves modifying the `bind_values` array
would need to change, and I'm not confident that we have sufficient test
coverage of all of those locations (when `having` was called with a hash
that could generate bind values).

[Sean Griffin & anthonynavarre]
  • Loading branch information...
sgrif committed Jan 26, 2015
1 parent 1152219 commit 39f2c3b3ea6fac371e79c284494e3d4cfdc1e929
@@ -5,12 +5,12 @@ module ActiveRecord
# = Active Record Relation
class Relation
MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group,
:order, :joins, :having, :references,
:order, :joins, :references,
:extending, :unscope]

SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering,
:reverse_order, :distinct, :create_with, :uniq]
CLAUSE_METHODS = [:where]
CLAUSE_METHODS = [:where, :having]
INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having]

VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
@@ -467,8 +467,10 @@ def delete_all(conditions = nil)
invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select { |method|
if MULTI_VALUE_METHODS.include?(method)
send("#{method}_values").any?
else
elsif SINGLE_VALUE_METHODS.include?(method)
send("#{method}_value")
elsif CLAUSE_METHODS.include?(method)
send("#{method}_clause").any?
end
}
if invalid_methods.any?
@@ -290,7 +290,7 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
operation,
distinct).as(aggregate_alias)
]
select_values += select_values unless having_values.empty?
select_values += select_values unless having_clause.empty?

select_values.concat group_fields.zip(group_aliases).map { |field,aliaz|
if field.respond_to?(:as)
@@ -93,10 +93,11 @@ def #{name}_clause=(value) # def where_clause=(value)
end

def bind_values
where_clause.binds
where_clause.binds + having_clause.binds
end

def bind_values=(values)
self.having_clause = Relation::WhereClause.new(having_clause.predicates, [])
self.where_clause = Relation::WhereClause.new(where_clause.predicates, values || [])
end

@@ -605,7 +606,7 @@ def having(opts, *rest)
def having!(opts, *rest) # :nodoc:
references!(PredicateBuilder.references(opts)) if Hash === opts

self.having_values += build_where(opts, rest)
self.having_clause += having_clause_factory.build(opts, rest)
self
end

@@ -869,7 +870,7 @@ def build_arel

collapse_wheres(arel, (where_clause.predicates - [''])) #TODO: Add uniq with real value comparison / ignore uniqs that have binds

arel.having(*having_values.uniq.reject(&:blank?)) unless having_values.empty?
arel.having(*having_clause.predicates.uniq.reject(&:blank?)) if having_clause.any?

arel.take(connection.sanitize_limit(limit_value)) if limit_value
arel.skip(offset_value.to_i) if offset_value
@@ -1108,9 +1109,11 @@ def check_if_method_has_arguments!(method_name, args)
def new_where_clause
Relation::WhereClause.empty
end
alias new_having_clause new_where_clause

def where_clause_factory
@where_clause_factory ||= Relation::WhereClauseFactory.new(klass, predicate_builder)
end
alias having_clause_factory where_clause_factory
end
end
@@ -3,7 +3,7 @@ class Relation
class WhereClause # :nodoc:
attr_reader :predicates, :binds

delegate :empty?, to: :predicates
delegate :any?, :empty?, to: :predicates

def initialize(predicates, binds)
@predicates = predicates
@@ -1468,10 +1468,26 @@ def test_distinct

def test_doesnt_add_having_values_if_options_are_blank
scope = Post.having('')
assert_equal [], scope.having_values
assert scope.having_clause.empty?

scope = Post.having([])
assert_equal [], scope.having_values
assert scope.having_clause.empty?
end

def test_having_with_binds_for_both_where_and_having
post = Post.first
having_then_where = Post.having(id: post.id).where(title: post.title).group(:title)
where_then_having = Post.where(title: post.title).having(id: post.id).group(:title)

assert_equal [post], having_then_where
assert_equal [post], where_then_having
end

def test_multiple_where_and_having_clauses
post = Post.first
having_then_where = Post.having(id: post.id).where(title: post.title).having(id: post.id).where(title: post.title).group(:title)

assert_equal [post], having_then_where
end

def test_references_triggers_eager_loading

0 comments on commit 39f2c3b

Please sign in to comment.
You can’t perform that action at this time.