Permalink
Browse files

Refactor Active Record to let Arel manage bind params

A common source of bugs and code bloat within Active Record has been the
need for us to maintain the list of bind values separately from the AST
they're associated with. This makes any sort of AST manipulation
incredibly difficult, as any time we want to potentially insert or
remove an AST node, we need to traverse the entire tree to find where
the associated bind parameters are.

With this change, the bind parameters now live on the AST directly.
Active Record does not need to know or care about them until the final
AST traversal for SQL construction. Rather than returning just the SQL,
the Arel collector will now return both the SQL and the bind parameters.
At this point the connection adapter will have all the values that it
had before.

A bit of this code is janky and something I'd like to refactor later. In
particular, I don't like how we're handling associations in the
predicate builder, the special casing of `StatementCache::Substitute` in
`QueryAttribute`, or generally how we're handling bind value replacement
in the statement cache when prepared statements are disabled.

This also mostly reverts #26378, as it moved all the code into a
location that I wanted to delete.

/cc @metaskills @yahonda, this change will affect the adapters

Fixes #29766.
Fixes #29804.
Fixes #26541.
Close #28539.
Close #24769.
Close #26468.
Close #26202.

There are probably other issues/PRs that can be closed because of this
commit, but that's all I could find on the first few pages.
  • Loading branch information...
sgrif committed Jul 24, 2017
1 parent 0449d8b commit 213796fb4936dce1da2f0c097a054e1af5c25c2c
Showing with 283 additions and 459 deletions.
  1. +1 −1 Gemfile.lock
  2. +1 −1 activerecord/lib/active_record/associations/has_many_through_association.rb
  3. +2 −4 activerecord/lib/active_record/associations/join_dependency/join_association.rb
  4. +1 −1 activerecord/lib/active_record/collection_cache_key.rb
  5. +30 −20 activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
  6. +2 −2 activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
  7. +4 −0 activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
  8. +13 −36 activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
  9. +2 −1 activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
  10. +1 −1 activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb
  11. +2 −1 activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb
  12. +2 −1 activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
  13. +11 −17 activerecord/lib/active_record/relation.rb
  14. +3 −3 activerecord/lib/active_record/relation/calculations.rb
  15. +3 −3 activerecord/lib/active_record/relation/finder_methods.rb
  16. +0 −8 activerecord/lib/active_record/relation/from_clause.rb
  17. +13 −71 activerecord/lib/active_record/relation/predicate_builder.rb
  18. +5 −3 activerecord/lib/active_record/relation/predicate_builder/array_handler.rb
  19. +0 −2 activerecord/lib/active_record/relation/predicate_builder/base_handler.rb
  20. +10 −1 activerecord/lib/active_record/relation/predicate_builder/basic_object_handler.rb
  21. +20 −6 activerecord/lib/active_record/relation/predicate_builder/range_handler.rb
  22. +5 −0 activerecord/lib/active_record/relation/query_attribute.rb
  23. +16 −28 activerecord/lib/active_record/relation/query_methods.rb
  24. +32 −60 activerecord/lib/active_record/relation/where_clause.rb
  25. +2 −46 activerecord/lib/active_record/relation/where_clause_factory.rb
  26. +14 −12 activerecord/lib/active_record/statement_cache.rb
  27. +31 −1 activerecord/lib/active_record/validations/uniqueness.rb
  28. +1 −3 activerecord/test/cases/adapter_test.rb
  29. +0 −17 activerecord/test/cases/associations/association_scope_test.rb
  30. +1 −1 activerecord/test/cases/bind_parameter_test.rb
  31. +1 −1 activerecord/test/cases/inheritance_test.rb
  32. +2 −4 activerecord/test/cases/relation/merging_test.rb
  33. +1 −1 activerecord/test/cases/relation/where_chain_test.rb
  34. +48 −54 activerecord/test/cases/relation/where_clause_test.rb
  35. +1 −1 activerecord/test/cases/relation/where_test.rb
  36. +1 −1 activerecord/test/cases/relation_test.rb
  37. +1 −46 activerecord/test/cases/relations_test.rb
@@ -31,7 +31,7 @@ GIT
GIT
remote: https://github.com/rails/arel.git
revision: 67a51c62f4e19390cd8eb408596ca48bb0806362
revision: 7a29220c689feb0581e21d5324b85fc2f201ac5e
specs:
arel (8.0.0)
@@ -154,7 +154,7 @@ def delete_records(records, method)
stmt.from scope.klass.arel_table
stmt.wheres = arel.constraints
count = scope.klass.connection.delete(stmt, "SQL", scope.bound_attributes)
count = scope.klass.connection.delete(stmt, "SQL")
end
when :nullify
count = scope.update_all(source_reflection.foreign_key => nil)
@@ -23,11 +23,10 @@ def match?(other)
super && reflection == other.reflection
end
JoinInformation = Struct.new :joins, :binds
JoinInformation = Struct.new :joins
def join_constraints(foreign_table, foreign_klass, join_type, tables, chain)
joins = []
binds = []
tables = tables.reverse
# The chain starts with the target table, but we want to end with it here (makes
@@ -43,7 +42,6 @@ def join_constraints(foreign_table, foreign_klass, join_type, tables, chain)
join_scope = reflection.join_scope(table, foreign_klass)
if join_scope.arel.constraints.any?
binds.concat join_scope.bound_attributes
joins.concat join_scope.arel.join_sources
right = joins.last.right
right.expr = right.expr.and(join_scope.arel.constraints)
@@ -53,7 +51,7 @@ def join_constraints(foreign_table, foreign_klass, join_type, tables, chain)
foreign_table, foreign_klass = table, klass
end
JoinInformation.new joins, binds
JoinInformation.new joins
end
def table
@@ -29,7 +29,7 @@ def collection_cache_key(collection = all, timestamp_column = :updated_at) # :no
arel = query.arel
end
result = connection.select_one(arel, nil, query.bound_attributes)
result = connection.select_one(arel, nil)
if result.blank?
size = 0
@@ -9,30 +9,36 @@ def initialize
end
# Converts an arel AST to SQL
def to_sql(arel, binds = [])
if arel.respond_to?(:ast)
collected = visitor.accept(arel.ast, collector)
collected.compile(binds, self).freeze
def to_sql(arel_or_sql_string, binds = [])
if arel_or_sql_string.respond_to?(:ast)
unless binds.empty?
raise "Passing bind parameters with an arel AST is forbidden. " \
"The values must be stored on the AST directly"
end
sql, binds = visitor.accept(arel_or_sql_string.ast, collector).value
[sql.freeze, binds || []]
else
arel.dup.freeze
[arel_or_sql_string.dup.freeze, binds]
end
end
# This is used in the StatementCache object. It returns an object that
# can be used to query the database repeatedly.
def cacheable_query(klass, arel) # :nodoc:
collected = visitor.accept(arel.ast, collector)
if prepared_statements
klass.query(collected.value)
sql, binds = visitor.accept(arel.ast, collector).value
query = klass.query(sql)
else
klass.partial_query(collected.value)
query = klass.partial_query(arel.ast)
binds = []
end
[query, binds]
end
# Returns an ActiveRecord::Result instance.
def select_all(arel, name = nil, binds = [], preparable: nil)
arel, binds = binds_from_relation arel, binds
sql = to_sql(arel, binds)
arel = arel_from_relation(arel)
sql, binds = to_sql(arel, binds)
if !prepared_statements || (arel.is_a?(String) && preparable.nil?)
preparable = false
else
@@ -131,20 +137,23 @@ def exec_update(sql, name = nil, binds = [])
#
# If the next id was calculated in advance (as in Oracle), it should be
# passed in as +id_value+.
def insert(arel, name = nil, pk = nil, id_value = nil, sequence_name = nil, binds = [])
value = exec_insert(to_sql(arel, binds), name, binds, pk, sequence_name)
def insert(arel, name = nil, pk = nil, id_value = nil, sequence_name = nil)
sql, binds = to_sql(arel)
value = exec_insert(sql, name, binds, pk, sequence_name)
id_value || last_inserted_id(value)
end
alias create insert
# Executes the update statement and returns the number of rows affected.
def update(arel, name = nil, binds = [])
exec_update(to_sql(arel, binds), name, binds)
def update(arel, name = nil)
sql, binds = to_sql(arel)
exec_update(sql, name, binds)
end
# Executes the delete statement and returns the number of rows affected.
def delete(arel, name = nil, binds = [])
exec_delete(to_sql(arel, binds), name, binds)
def delete(arel, name = nil)
sql, binds = to_sql(arel)
exec_delete(sql, name, binds)
end
# Returns +true+ when the connection adapter supports prepared statement
@@ -430,11 +439,12 @@ def single_value_from_rows(rows)
row && row.first
end
def binds_from_relation(relation, binds)
if relation.is_a?(Relation) && binds.empty?
relation, binds = relation.arel, relation.bound_attributes
def arel_from_relation(relation)
if relation.is_a?(Relation)
relation.arel
else
relation
end
[relation, binds]
end
# Fixture value is quoted by Arel, however scalar values
@@ -92,8 +92,8 @@ def clear_query_cache
def select_all(arel, name = nil, binds = [], preparable: nil)
if @query_cache_enabled && !locked?(arel)
arel, binds = binds_from_relation arel, binds
sql = to_sql(arel, binds)
arel = arel_from_relation(arel)
sql, binds = to_sql(arel, binds)
cache_sql(sql, name, binds) { super(sql, name, binds, preparable: preparable) }
else
super
@@ -24,6 +24,10 @@ def quote(value)
return value.quoted_id
end
if value.respond_to?(:value_for_database)
value = value.value_for_database
end
_quote(value)
end
@@ -7,7 +7,9 @@
require_relative "abstract/schema_dumper"
require_relative "abstract/schema_creation"
require "arel/collectors/bind"
require "arel/collectors/composite"
require "arel/collectors/sql_string"
require "arel/collectors/substitute_binds"
module ActiveRecord
module ConnectionAdapters # :nodoc:
@@ -129,19 +131,6 @@ def <=>(version_string)
end
end
class BindCollector < Arel::Collectors::Bind
def compile(bvs, conn)
casted_binds = bvs.map(&:value_for_database)
super(casted_binds.map { |value| conn.quote(value) })
end
end
class SQLString < Arel::Collectors::SQLString
def compile(bvs, conn)
super(bvs)
end
end
def valid_type?(type) # :nodoc:
!native_database_types[type].nil?
end
@@ -432,14 +421,14 @@ def raw_connection
end
def case_sensitive_comparison(table, attribute, column, value) # :nodoc:
table[attribute].eq(value)
table[attribute].eq(Arel::Nodes::BindParam.new(value))
end
def case_insensitive_comparison(table, attribute, column, value) # :nodoc:
if can_perform_case_insensitive_comparison_for?(column)
table[attribute].lower.eq(table.lower(value))
table[attribute].lower.eq(table.lower(Arel::Nodes::BindParam.new(value)))
else
table[attribute].eq(value)
table[attribute].eq(Arel::Nodes::BindParam.new(value))
end
end
@@ -457,24 +446,6 @@ def column_name_for_operation(operation, node) # :nodoc:
visitor.accept(node, collector).value
end
def combine_bind_parameters(
from_clause: [],
join_clause: [],
where_clause: [],
having_clause: [],
limit: nil,
offset: nil
) # :nodoc:
result = from_clause + join_clause + where_clause + having_clause
if limit
result << limit
end
if offset
result << offset
end
result
end
def default_index_type?(index) # :nodoc:
index.using.nil?
end
@@ -609,9 +580,15 @@ def column_for(table_name, column_name)
def collector
if prepared_statements
SQLString.new
Arel::Collectors::Composite.new(
Arel::Collectors::SQLString.new,
Arel::Collectors::Bind.new,
)
else
BindCollector.new
Arel::Collectors::SubstituteBinds.new(
self,
Arel::Collectors::SQLString.new,
)
end
end
@@ -177,7 +177,8 @@ def clear_cache!
#++
def explain(arel, binds = [])
sql = "EXPLAIN #{to_sql(arel, binds)}"
sql, binds = to_sql(arel, binds)
sql = "EXPLAIN #{sql}"
start = Time.now
result = exec_query(sql, "EXPLAIN", binds)
elapsed = Time.now - start
@@ -5,7 +5,7 @@ module ConnectionAdapters
module MySQL
module DatabaseStatements
# Returns an ActiveRecord::Result instance.
def select_all(arel, name = nil, binds = [], preparable: nil) # :nodoc:
def select_all(*) # :nodoc:
result = if ExplainRegistry.collect? && prepared_statements
unprepared_statement { super }
else
@@ -5,7 +5,8 @@ module ConnectionAdapters
module PostgreSQL
module DatabaseStatements
def explain(arel, binds = [])
sql = "EXPLAIN #{to_sql(arel, binds)}"
sql, binds = to_sql(arel, binds)
sql = "EXPLAIN #{sql}"
PostgreSQL::ExplainPrettyPrinter.new.pp(exec_query(sql, "EXPLAIN", binds))
end
@@ -203,7 +203,8 @@ def disable_referential_integrity # :nodoc:
#++
def explain(arel, binds = [])
sql = "EXPLAIN QUERY PLAN #{to_sql(arel, binds)}"
sql, binds = to_sql(arel, binds)
sql = "EXPLAIN QUERY PLAN #{sql}"
SQLite3::ExplainPrettyPrinter.new.pp(exec_query(sql, "EXPLAIN", []))
end
@@ -53,7 +53,7 @@ def insert(values) # :nodoc:
im = arel.create_insert
im.into @table
substitutes, binds = substitute_values values
substitutes = substitute_values values
if values.empty? # empty insert
im.values = Arel.sql(connection.empty_insert_statement_value)
@@ -67,11 +67,11 @@ def insert(values) # :nodoc:
primary_key || false,
primary_key_value,
nil,
binds)
)
end
def _update_record(values, id, id_was) # :nodoc:
substitutes, binds = substitute_values values
substitutes = substitute_values values
scope = @klass.unscoped
@@ -80,28 +80,21 @@ def _update_record(values, id, id_was) # :nodoc:
end
relation = scope.where(@klass.primary_key => (id_was || id))
bvs = binds + relation.bound_attributes
um = relation
.arel
.compile_update(substitutes, @klass.primary_key)
@klass.connection.update(
um,
"SQL",
bvs,
)
end
def substitute_values(values) # :nodoc:
binds = []
substitutes = []
values.each do |arel_attr, value|
binds.push QueryAttribute.new(arel_attr.name, value, klass.type_for_attribute(arel_attr.name))
substitutes.push [arel_attr, Arel::Nodes::BindParam.new]
values.map do |arel_attr, value|
bind = QueryAttribute.new(arel_attr.name, value, klass.type_for_attribute(arel_attr.name))
[arel_attr, Arel::Nodes::BindParam.new(bind)]
end
[substitutes, binds]
end
def arel_attribute(name) # :nodoc:
@@ -380,7 +373,7 @@ def update_all(updates)
stmt.wheres = arel.constraints
end
@klass.connection.update stmt, "SQL", bound_attributes
@klass.connection.update stmt, "SQL"
end
# Updates an object (or multiple objects) and saves it to the database, if validations pass.
@@ -510,7 +503,7 @@ def delete_all
stmt.wheres = arel.constraints
end
affected = @klass.connection.delete(stmt, "SQL", bound_attributes)
affected = @klass.connection.delete(stmt, "SQL")
reset
affected
@@ -578,7 +571,8 @@ def to_sql
conn = klass.connection
conn.unprepared_statement {
conn.to_sql(relation.arel, relation.bound_attributes)
sql, _ = conn.to_sql(relation.arel)
sql
}
end
end
@@ -663,7 +657,7 @@ def has_join_values?
def exec_queries(&block)
skip_query_cache_if_necessary do
@records = eager_loading? ? find_with_associations.freeze : @klass.find_by_sql(arel, bound_attributes, &block).freeze
@records = eager_loading? ? find_with_associations.freeze : @klass.find_by_sql(arel, &block).freeze
preload = preload_values
preload += includes_values unless eager_loading?
Oops, something went wrong.

0 comments on commit 213796f

Please sign in to comment.