Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support for ActiveRecord 4.0.x #149

Closed
wants to merge 19 commits into from

3 participants

Sammy Larbi Charlie Savage wogg
Sammy Larbi

:fireworks: :fireworks: :fireworks: :fireworks: :fireworks: :fireworks: :fireworks: :fireworks:

The tests are :green_heart: for mysql, postgresql, and sqlite3. Someone else will need to check the other adapters.

Since I'm just a :baby: in CPK land, I do encourage you to review my work! In particular, see commit: codeodor@4c731ef where I changed the test. Because of changes in Rails which broke some unstated assumptions in the code, I think it was OK to do that. However, I'd like you to review that one in detail to be sure!

I now consider this pull request ready to be merged into the main repo, though if you wish to maintain backward compatibility, you may consider not merging it directly into master, since I made no effort to keep it working with Rails 3.2.x.

The original PR message follows:

:warning: This is not a complete pull request, and it should not be merged into the master branch! :warning:

I chose master as the base branch because there is not an "ar_4.0.x" branch in the CPK repository like there is for Rails 3.1 and 3.2.

I'm starting an incomplete pull request because I would not be shocked to learn that a lot of people are already working on compatibility with Rails 4.0, and we are working in isolation instead of having a central body of knowledge.

So far, I've added a few of the obvious cases which I needed to get all of my own app's tests passing. There is a little I've left out, because for the moment they are horrible kludges, but I will fix them and include them too!

After that, I'll begin focusing on the MySQL tests, then Postgres, then whatever other DBs are supported.

Someone else feel free to join in. I guess fork my ar_4.0.x branch and make pull requests there? Or get with me and I'll assign you collaborator access to commit directly.

Once we get far enough, we'll update this Pull Request for review by the CPK team.

:+1: ?

codeodor added some commits
Sammy Larbi codeodor change AR dependency 0976c66
Sammy Larbi codeodor tell CPK to use Rails 4.0 beta1 28431b2
Sammy Larbi codeodor change referenced of #scoped to #scope 8d582e5
Sammy Larbi codeodor new namespace for ConnectionSpecification::Resolver cf14706
Sammy Larbi codeodor new initialize method for Relation 2d4152f
Sammy Larbi codeodor change Dirty implementation to match Rails 4 855e00a
Sammy Larbi codeodor use the loaded Rails' implementation of the method unless it applies …
…to CPK
600c5e6
Sammy Larbi codeodor use Rails' version unless it applies to CPK 430d6a2
Sammy Larbi codeodor updates to look like Rails AssociationScope 90bf124
Sammy Larbi codeodor fix for default ordering by primary key 1bdf251
Sammy Larbi codeodor fix CPK update, switch to including CPK persistence as a module to ga…
…in access to `super` in Rails
bd9dd0c
Sammy Larbi codeodor fix for removal of `in?` method 4fe1b55
Sammy Larbi codeodor fix nil scope 74010dd
Sammy Larbi codeodor fix for default ordering by PK when column names are ambiguous due to…
… being present in multiple tables in the query
fb05888
Sammy Larbi codeodor fix for destroy 14f7899
Sammy Larbi codeodor change back to following Rails to fix #changes on composite_belongs_to 2b504ce
Sammy Larbi codeodor Fix test for bracket assignment (see description)
Rails 4 has a commit (see rails/rails@522c0fd) that ensures the PK is always present in the @attributes hash, which was causing this test to break when attempting to assign values to that key in the hash (since it just loops over them).

Given there was no expectation the key existed in the prior versions, a valid fix seemed to me to be to do not test assignment in this version.

However, I'm not sure if that's a valid assumption or not, so please review / consider that carefully.
4c731ef
Sammy Larbi

MySQL tests are :green_heart: as of this comment!

Sammy Larbi

No commits needed for postgresql, it's a :green_heart: too.

Sammy Larbi

sqlite3 gets the :green_heart:

That's all I can do. Someone else will need to test ibmdb, oracle, and oracle_enhanced.

Charlie Savage
Owner

HI codeodor - thanks for the patch and AR 4.0 support. We aren't planning on switching soon to Rails 4 though, so I'm happy to go with what you have done for now.

We'll want to up the major version number of CPK I think and create a new branch for AR 4.0. Then make a pre-release gem. So I can setup a branch and then i'll see if I can merge this pull request into it.

Sammy Larbi

Sure, that makes sense. Once you create the branch, I can resubmit the pull request to the correct branch (or maybe modify it using the API, if possible) if you'd like.

Let me know how else I can be of help!

wogg

We over at RescueTime are very interested in getting this branch created as well.
Can assist with real-world testing.

Charlie Savage
Owner

Applied - thanks!

Definitely submit patches as you find stuff.

Charlie Savage cfis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 20, 2013
  1. Sammy Larbi

    change AR dependency

    codeodor authored
  2. Sammy Larbi
  3. Sammy Larbi
  4. Sammy Larbi
  5. Sammy Larbi
  6. Sammy Larbi
  7. Sammy Larbi
  8. Sammy Larbi
Commits on Mar 22, 2013
  1. Sammy Larbi
Commits on Mar 23, 2013
  1. Sammy Larbi
  2. Sammy Larbi

    fix CPK update, switch to including CPK persistence as a module to ga…

    codeodor authored
    …in access to `super` in Rails
  3. Sammy Larbi
  4. Sammy Larbi

    fix nil scope

    codeodor authored
  5. Sammy Larbi

    fix for default ordering by PK when column names are ambiguous due to…

    codeodor authored
    … being present in multiple tables in the query
  6. Sammy Larbi

    fix for destroy

    codeodor authored
  7. Sammy Larbi
  8. Sammy Larbi

    Fix test for bracket assignment (see description)

    codeodor authored
    Rails 4 has a commit (see rails/rails@522c0fd) that ensures the PK is always present in the @attributes hash, which was causing this test to break when attempting to assign values to that key in the hash (since it just loops over them).
    
    Given there was no expectation the key existed in the prior versions, a valid fix seemed to me to be to do not test assignment in this version.
    
    However, I'm not sure if that's a valid assumption or not, so please review / consider that carefully.
  9. Sammy Larbi

    :green_heart: for mysql: move initialize_dupe into new core file so `super` does …

    codeodor authored
    …not end up calling the Rails version of the method
  10. Sammy Larbi

    add note in History

    codeodor authored
This page is out of date. Refresh to see the latest.
3  History.rdoc
View
@@ -1,3 +1,6 @@
+== 6.0.0 (unreleased)
+* ActiveRecord 4.0 support (Sammy Larbi)
+
== 5.0.12 2013-01-21
* Fix HasManyAssociation foreign_key_present? method to work with non CPK assocations (Charlie Savage)
2  composite_primary_keys.gemspec
View
@@ -26,5 +26,5 @@ Gem::Specification.new do |s|
# Dependencies
s.required_ruby_version = '>= 1.8.7'
- s.add_dependency('activerecord', '>= 3.2.9', '~> 3.2.0')
+ s.add_dependency('activerecord', '>= 4.0.0.beta1')
end
5 lib/composite_primary_keys.rb
View
@@ -26,7 +26,7 @@
unless defined?(ActiveRecord)
require 'rubygems'
- gem 'activerecord', '>= 3.2.9', '~> 3.2.0'
+ gem 'activerecord', '4.0.0.beta1'
require 'active_record'
end
@@ -65,12 +65,13 @@
# CPK files
+require 'composite_primary_keys/persistence'
require 'composite_primary_keys/base'
+require 'composite_primary_keys/core'
require 'composite_primary_keys/composite_arrays'
require 'composite_primary_keys/composite_predicates'
require 'composite_primary_keys/counter_cache'
require 'composite_primary_keys/fixtures'
-require 'composite_primary_keys/persistence'
require 'composite_primary_keys/relation'
require 'composite_primary_keys/sanitization'
require 'composite_primary_keys/version'
2  lib/composite_primary_keys/associations/association.rb
View
@@ -4,7 +4,7 @@ class Association
def creation_attributes
attributes = {}
- if reflection.macro.in?([:has_one, :has_many]) && !options[:through]
+ if (reflection.macro == :has_one || reflection.macro == :has_many) && !options[:through]
# CPK
# attributes[reflection.foreign_key] = owner[reflection.active_record_primary_key]
Array(reflection.foreign_key).zip(Array(reflection.active_record_primary_key)).each do |key1, key2|
25 lib/composite_primary_keys/associations/association_scope.rb
View
@@ -25,7 +25,7 @@ def add_constraints(scope)
if reflection.source_macro == :belongs_to
if reflection.options[:polymorphic]
- key = reflection.association_primary_key(klass)
+ key = reflection.association_primary_key(self.klass)
else
key = reflection.association_primary_key
end
@@ -36,8 +36,7 @@ def add_constraints(scope)
foreign_key = reflection.active_record_primary_key
end
- conditions = self.conditions[i]
-
+
if reflection == chain.last
# CPK
# scope = scope.where(table[key].eq(owner[foreign_key]))
@@ -47,14 +46,6 @@ def add_constraints(scope)
if reflection.type
scope = scope.where(table[reflection.type].eq(owner.class.base_class.name))
end
-
- conditions.each do |condition|
- if options[:through] && condition.is_a?(Hash)
- condition = { table.name => condition }
- end
-
- scope = scope.where(interpolate(condition))
- end
else
# CPK
# constraint = table[key].eq(foreign_table[foreign_key])
@@ -66,10 +57,18 @@ def add_constraints(scope)
end
scope = scope.joins(join(foreign_table, constraint))
+ end
+
+ scope_chain[i].each do |scope_chain_item|
+ klass = i == 0 ? self.klass : reflection.klass
+ item = eval_scope(klass, scope_chain_item)
- unless conditions.empty?
- scope = scope.where(sanitize(conditions, table))
+ if scope_chain_item == self.reflection.scope
+ scope.merge! item.except(:where, :includes)
end
+
+ scope.includes! item.includes_values
+ scope.where_values += item.where_values
end
end
4 lib/composite_primary_keys/associations/has_many_association.rb
View
@@ -8,7 +8,7 @@ def delete_records(records, method)
else
# CPK
# keys = records.map { |r| r[reflection.association_primary_key] }
- # scope = scoped.where(reflection.association_primary_key => keys)
+ # scope = scope.where(reflection.association_primary_key => keys)
table = Arel::Table.new(reflection.table_name)
and_conditions = records.map do |record|
eq_conditions = Array(reflection.association_primary_key).map do |name|
@@ -22,7 +22,7 @@ def delete_records(records, method)
condition = condition.or(and_condition)
end
- scope = scoped.where(condition)
+ scope = scope.where(condition)
if method == :delete_all
update_counter(-scope.delete_all)
4 lib/composite_primary_keys/associations/preloader/association.rb
View
@@ -4,9 +4,9 @@ class Preloader
class Association
def records_for(ids)
# CPK
- # scoped.where(association_key.in(ids))
+ # scope.where(association_key.in(ids))
predicate = cpk_in_predicate(table, reflection.foreign_key, ids)
- scoped.where(predicate)
+ scope.where(predicate)
end
def associated_records_by_owner
2  lib/composite_primary_keys/associations/preloader/belongs_to.rb
View
@@ -5,7 +5,7 @@ class BelongsTo
def records_for(ids)
# CPK
predicate = cpk_in_predicate(table, association_key_name, ids)
- scoped.where(predicate)
+ scope.where(predicate)
end
end
end
4 lib/composite_primary_keys/associations/preloader/has_and_belongs_to_many.rb
View
@@ -4,9 +4,9 @@ class Preloader
class HasAndBelongsToMany
def records_for(ids)
# CPK
- #scope = super
+ # scope = super
predicate = cpk_in_predicate(join_table, reflection.foreign_key, ids)
- scope = scoped.where(predicate)
+ scope = build_scope.where(predicate)
klass.connection.select_all(scope.arel.to_sql, 'SQL', scope.bind_values)
end
2  lib/composite_primary_keys/attribute_methods/dirty.rb
View
@@ -16,8 +16,6 @@ def write_attribute(attr, value)
@changed_attributes.delete(attr) unless _field_changed?(attr, old, value)
else
old = clone_attribute_value(:read_attribute, attr)
- # Save Time objects as TimeWithZone if time_zone_aware_attributes == true
- old = old.in_time_zone if clone_with_time_zone_conversion_attribute?(attr, old)
@changed_attributes[attr] = old if _field_changed?(attr, old, value)
end
end
6 lib/composite_primary_keys/attribute_methods/read.rb
View
@@ -25,12 +25,12 @@ def internal_attribute_access_code(attr_name, cast_code)
end
end
- def read_attribute(attr_name)
- # CPK
+ rails_read_attribute = instance_method(:read_attribute)
+ define_method(:read_attribute) do |attr_name|
if attr_name.kind_of?(Array)
attr_name.map {|name| read_attribute(name)}.to_composite_keys
else
- self.class.type_cast_attribute(attr_name, @attributes, @attributes_cache)
+ rails_read_attribute.bind(self).(attr_name)
end
end
end
28 lib/composite_primary_keys/base.rb
View
@@ -3,6 +3,8 @@ class CompositeKeyError < StandardError #:nodoc:
end
class Base
+ include CompositePrimaryKeys::ActiveRecord::Persistence
+
INVALID_FOR_COMPOSITE_KEYS = 'Not appropriate for composite primary keys'
NOT_IMPLEMENTED_YET = 'Not implemented for composite primary keys yet'
@@ -57,32 +59,6 @@ def composite?
self.class.composite?
end
- def initialize_dup(other)
- cloned_attributes = other.clone_attributes(:read_attribute_before_type_cast)
- self.class.initialize_attributes(cloned_attributes, :serialized => false)
- # CPK
- # cloned_attributes.delete(self.class.primary_key)
- Array(self.class.primary_key).each {|key| cloned_attributes.delete(key.to_s)}
-
- @attributes = cloned_attributes
-
- _run_after_initialize_callbacks if respond_to?(:_run_after_initialize_callbacks)
-
- @changed_attributes = {}
- self.class.column_defaults.each do |attr, orig_value|
- @changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @attributes[attr])
- end
-
- @aggregation_cache = {}
- @association_cache = {}
- @attributes_cache = {}
- @new_record = true
-
- ensure_proper_type
- populate_with_current_scope_attributes
- super
- end
-
module CompositeClassMethods
def primary_key
primary_keys
4 lib/composite_primary_keys/connection_adapters/abstract/connection_specification_changes.rb
View
@@ -7,7 +7,7 @@ def self.load_cpk_adapter(adapter)
end
def self.establish_connection(spec = ENV["DATABASE_URL"])
- resolver = ConnectionSpecification::Resolver.new spec, configurations
+ resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new spec, configurations
spec = resolver.spec
# CPK
@@ -18,7 +18,7 @@ def self.establish_connection(spec = ENV["DATABASE_URL"])
end
remove_connection
- connection_handler.establish_connection name, spec
+ connection_handler.establish_connection self, spec
end
class << self
28 lib/composite_primary_keys/core.rb
View
@@ -0,0 +1,28 @@
+module ActiveRecord
+ module Core
+ def initialize_dup(other)
+ cloned_attributes = other.clone_attributes(:read_attribute_before_type_cast)
+ self.class.initialize_attributes(cloned_attributes, :serialized => false)
+ # CPK
+ # cloned_attributes.delete(self.class.primary_key)
+ Array(self.class.primary_key).each {|key| cloned_attributes.delete(key.to_s)}
+ @attributes = cloned_attributes
+
+ run_callbacks(:initialize) unless _initialize_callbacks.empty?
+
+ @changed_attributes = {}
+ self.class.column_defaults.each do |attr, orig_value|
+ @changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @attributes[attr])
+ end
+
+ @aggregation_cache = {}
+ @association_cache = {}
+ @attributes_cache = {}
+
+ @new_record = true
+
+ ensure_proper_type
+ super
+ end
+ end
+end
83 lib/composite_primary_keys/persistence.rb
View
@@ -1,71 +1,53 @@
-module ActiveRecord
- module Persistence
- def destroy
- destroy_associations
-
- if persisted?
- IdentityMap.remove(self) if IdentityMap.enabled?
- # CPK
- #pk = self.class.primary_key
- #column = self.class.columns_hash[pk]
- #substitute = connection.substitute_at(column, 0)
-
+module CompositePrimaryKeys
+ module ActiveRecord
+ module Persistence
+ def relation_for_destroy
+ return super unless composite?
+
where_hash = {}
primary_keys = Array(self.class.primary_key)
- #relation = self.class.unscoped.where(
- # self.class.arel_table[pk].eq(substitute))
-
primary_keys.each do |key|
where_hash[key.to_s] = self[key]
end
- # CPK
- #relation.bind_values = [[column, id]]
-
relation = self.class.unscoped.where(where_hash)
- relation.delete_all
end
+
- @destroyed = true
- freeze
- end
-
- def touch(name = nil)
- attributes = timestamp_attributes_for_update_in_model
- attributes << name if name
+ def touch(name = nil)
+ attributes = timestamp_attributes_for_update_in_model
+ attributes << name if name
- unless attributes.empty?
- current_time = current_time_from_proper_timezone
- changes = {}
+ unless attributes.empty?
+ current_time = current_time_from_proper_timezone
+ changes = {}
- attributes.each do |column|
- column = column.to_s
- changes[column] = write_attribute(column, current_time)
- end
+ attributes.each do |column|
+ column = column.to_s
+ changes[column] = write_attribute(column, current_time)
+ end
- changes[self.class.locking_column] = increment_lock if locking_enabled?
+ changes[self.class.locking_column] = increment_lock if locking_enabled?
- @changed_attributes.except!(*changes.keys)
+ @changed_attributes.except!(*changes.keys)
- relation = self.class.send(:relation)
- arel_table = self.class.arel_table
- primary_key = self.class.primary_key
+ relation = self.class.send(:relation)
+ arel_table = self.class.arel_table
+ primary_key = self.class.primary_key
- primary_key_predicate = relation.cpk_id_predicate(arel_table, Array(primary_key), Array(id))
+ primary_key_predicate = relation.cpk_id_predicate(arel_table, Array(primary_key), Array(id))
- self.class.unscoped.where(primary_key_predicate).update_all(changes) == 1
+ self.class.unscoped.where(primary_key_predicate).update_all(changes) == 1
+ end
end
- end
- def update(attribute_names = @attributes.keys)
- klass = self.class
- if !self.composite?
- attributes_with_values = arel_attributes_values(false, false, attribute_names)
- return 0 if attributes_with_values.empty?
- stmt = klass.unscoped.where(klass.arel_table[klass.primary_key].eq(id)).arel.compile_update(attributes_with_values)
- else
- attributes_with_values = arel_attributes_values(can_change_primary_key?, false, attribute_names)
+ def update_record(attribute_names = @attributes.keys)
+ return super(attribute_names) unless composite?
+
+ klass = self.class
+
+ attributes_with_values = arel_attributes_with_values_for_update(attribute_names)
return 0 if attributes_with_values.empty?
if !can_change_primary_key? and primary_key_changed?
@@ -75,8 +57,9 @@ def update(attribute_names = @attributes.keys)
else
stmt = klass.unscoped.where(ids_hash).arel.compile_update(attributes_with_values)
end
+
+ klass.connection.update stmt.to_sql
end
- klass.connection.update stmt.to_sql
end
end
end
9 lib/composite_primary_keys/relation.rb
View
@@ -6,7 +6,8 @@ class << self
include CompositePrimaryKeys::ActiveRecord::Calculations
include CompositePrimaryKeys::ActiveRecord::FinderMethods
include CompositePrimaryKeys::ActiveRecord::QueryMethods
-
+
+
def delete(id_or_array)
::ActiveRecord::IdentityMap.remove_by_id(self.symbolized_base_class, id_or_array) if ::ActiveRecord::IdentityMap.enabled?
# Without CPK:
@@ -62,9 +63,9 @@ def where_values_hash
end
alias :initialize_without_cpk :initialize
- def initialize(klass, table)
- initialize_without_cpk(klass, table)
- add_cpk_support if klass.composite?
+ def initialize(klass, table, values = {})
+ initialize_without_cpk(klass, table, values)
+ add_cpk_support if klass && klass.composite?
add_cpk_where_values_hash
end
14 lib/composite_primary_keys/relation/query_methods.rb
View
@@ -24,4 +24,18 @@ def reverse_sql_order(order_query)
end.flatten
end
+
+ def order(*args)
+ args.map! do |arg|
+ if arg.is_a?(Arel::Nodes::Ordering) && arg.expr.name.is_a?(Array)
+ arg = arg.expr.name.map do |key|
+ cloned_node = arg.clone
+ cloned_node.expr.name = key
+ cloned_node
+ end
+ end
+ arg
+ end if composite?
+ super(*args)
+ end
end
1  test/test_attributes.rb
View
@@ -36,6 +36,7 @@ def test_brackets_primary_key
def test_brackets_assignment
testing_with do
@first.attributes.each_pair do |attr_name, value|
+ next if attr_name == @first.class.primary_key
@first[attr_name]= !value.nil? ? value * 2 : '1'
assert_equal !value.nil? ? value * 2 : '1', @first[attr_name]
end
5 test/test_dup.rb
View
@@ -22,8 +22,9 @@ def test_dup
testing_with do
clone = @first.dup
- remove_keys = Array(@klass.primary_key).map &:to_s
- assert_equal(@first.attributes.except(*remove_keys), clone.attributes)
+ remove_keys = Array(@klass.primary_key).map(&:to_s)
+ remove_keys << Array(@klass.primary_key) # Rails 4 adds the PK to the attributes, so we want to remove it as well
+ assert_equal(@first.attributes.except(*remove_keys), clone.attributes.except(*remove_keys))
if composite?
@klass.primary_key.each do |key|
Something went wrong with that request. Please try again.