diff options
| author | Sean Griffin <sean@thoughtbot.com> | 2015-01-25 14:54:18 -0700 |
|---|---|---|
| committer | Sean Griffin <sean@thoughtbot.com> | 2015-01-25 16:23:01 -0700 |
| commit | 2c46d6db4feaf4284415f2fb6ceceb1bb535f278 (patch) | |
| tree | da82ab59d7f5a6c82ef69a87f29284e3930e37a2 /activerecord/lib | |
| parent | 79f71d35e957772df7454212b3f235423064832a (diff) | |
| download | rails-2c46d6db4feaf4284415f2fb6ceceb1bb535f278.tar.gz rails-2c46d6db4feaf4284415f2fb6ceceb1bb535f278.tar.bz2 rails-2c46d6db4feaf4284415f2fb6ceceb1bb535f278.zip | |
Introduce `Relation::WhereClause`
The way that bind values are currently stored on Relation is a mess.
They can come from `having`, `where`, or `join`. I'm almost certain that
`having` is actually broken, and calling `where` followed by `having`
followed by `where` will completely scramble the binds.
Joins don't actually add the bind parameters to the relation itself, but
instead add it onto an accessor on the arel AST which is undocumented,
and unused in Arel itself. This means that the bind values must always
be accessed as `relation.arel.bind_values + relation.bind_values`.
Anything that doesn't is likely broken (and tons of bugs have come up
for exactly that reason)
The result is that everything dealing with `Relation` instances has to
know far too much about the internals. The binds are split, combined,
and re-stored in non-obvious ways that makes it difficult to change
anything about the internal representation of `bind_values`, and is
extremely prone to bugs.
So the goal is to move a lot of logic off of `Relation`, and into
separate objects. This is not the same as what is currently done with
`JoinDependency`, as `Relation` knows far too much about its internals,
and vice versa. Instead these objects need to be black boxes that can
have their implementations swapped easily.
The end result will be two classes, `WhereClause` and `JoinClause`
(`having` will just re-use `WhereClause`), and there will be a single
method to access the bind values of a `Relation` which will be
implemented as
```
join_clause.binds + where_clause.binds + having_clause.binds
```
This is the first step towards that refactoring, with the internal
representation of where changed, and an intermediate representation of
`where_values` and `bind_values` to let the refactoring take small
steps. These will be removed shortly.
Diffstat (limited to 'activerecord/lib')
| -rw-r--r-- | activerecord/lib/active_record/relation.rb | 7 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/query_methods.rb | 34 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/where_clause.rb | 29 |
3 files changed, 67 insertions, 3 deletions
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 9c4db8a05e..54cb6861b8 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -1,18 +1,19 @@ # -*- coding: utf-8 -*- -require 'arel/collectors/bind' +require "arel/collectors/bind" module ActiveRecord # = Active Record Relation class Relation MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group, - :order, :joins, :where, :having, :bind, :references, + :order, :joins, :having, :references, :extending, :unscope] SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering, :reverse_order, :distinct, :create_with, :uniq] + CLAUSE_METHODS = [:where] INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having] - VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches, Explain, Delegation diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index e1c74ce90a..9a636bc527 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1,6 +1,7 @@ require 'active_support/core_ext/array/wrap' require 'active_support/core_ext/string/filters' require 'active_model/forbidden_attributes_protection' +require "active_record/relation/where_clause" module ActiveRecord module QueryMethods @@ -90,6 +91,35 @@ module ActiveRecord CODE end + Relation::CLAUSE_METHODS.each do |name| + class_eval <<-CODE, __FILE__, __LINE__ + 1 + def #{name}_clause # def where_clause + @values[:#{name}] || new_#{name}_clause # @values[:where] || new_where_clause + end # end + # + def #{name}_clause=(value) # def where_clause=(value) + assert_mutability! # assert_mutability! + @values[:#{name}] = value # @values[:where] = value + end # end + CODE + end + + def where_values + where_clause.parts + end + + def where_values=(values) + self.where_clause = Relation::WhereClause.new(values || [], where_clause.binds) + end + + def bind_values + where_clause.binds + end + + def bind_values=(values) + self.where_clause = Relation::WhereClause.new(where_clause.parts, values || []) + end + def create_with_value # :nodoc: @values[:create_with] || {} end @@ -1117,5 +1147,9 @@ module ActiveRecord raise ArgumentError, "The method .#{method_name}() must contain arguments." end end + + def new_where_clause + Relation::WhereClause.empty + end end end diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb new file mode 100644 index 0000000000..b39fc1fed1 --- /dev/null +++ b/activerecord/lib/active_record/relation/where_clause.rb @@ -0,0 +1,29 @@ +module ActiveRecord + class Relation + class WhereClause + attr_reader :parts, :binds + + def initialize(parts, binds) + @parts = parts + @binds = binds + end + + def +(other) + WhereClause.new( + parts + other.parts, + binds + other.binds, + ) + end + + def ==(other) + other.is_a?(WhereClause) && + parts == other.parts && + binds == other.binds + end + + def self.empty + new([], []) + end + end + end +end |