From 2c46d6db4feaf4284415f2fb6ceceb1bb535f278 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sun, 25 Jan 2015 14:54:18 -0700 Subject: 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. --- activerecord/lib/active_record/relation.rb | 7 +++-- .../lib/active_record/relation/query_methods.rb | 34 ++++++++++++++++++++++ .../lib/active_record/relation/where_clause.rb | 29 ++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 activerecord/lib/active_record/relation/where_clause.rb (limited to 'activerecord/lib') 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 -- cgit v1.2.3