Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Composite keys Support #451

Closed
wants to merge 4 commits into from

2 participants

@TiagoCardoso1983

I've just pushed all instances to a feature branch, so it doesn't get too general

Tiago Cardoso added some commits
Tiago Cardoso payload object's object is no prepared to respond to the possibility …
…of a composite key implementation while still working for the standard of one primary key only (this was a fix to make djobs work with the gem composite_primary_keys)
73a2b96
Tiago Cardoso corrected another implementation which does not get along with compos…
…ite keys (see pull request 450)
949c3f6
@albus522
Owner

At minimum you will need to add tests for this to be accepted. However, I am not sure that composite primary keys is something we want to support or that this would be the best approach to support it. This approach makes every job that contains an ActiveRecord model more expensive to load, while very few people would ever use this feature.

@albus522 albus522 closed this
@TiagoCardoso1983

About the tests, the purpose was to change the primary key fetching from the attributes, therefore not to break the existing specs. But it's ok, I'll add a few specs to specifically test this. About being it more expensive to load, I don't see the huge performance gap. The only difference is that instead of using the Hash#[] to fetch the primary key value, I now use Hash#values_at. The rest will work exactly as it has been working, no performance gap in there. I'd even say it is more of a different approach in getting the primary key instead of a specific patch for the composite gem.

@albus522
Owner

The tests are to make sure we don't inadvertently break an intended feature with a future update.

The performance gap comes from values_at generating and filling a new array, generating more objects that will later have to be garbage collected. Also under the hood several other methods are called adding to the stack depth and iterating over the hash more than is necessary. The performance gap may not be huge, but we have a lot more people running a lot of jobs than we have using composite primary keys.

@TiagoCardoso1983

That is indeed a valid concern, I'll have a look into ruby documentation again to see if one can come up with a better solution. Nevertheless, I think the current implementation presents a problem for any Ruby ORM which implements composite primary keys, namely Datamapper, which delayed job already officially supports (I've seen an adapter, correct me if I'm wrong), and which supports it natively. It might be true that most Rails projects don't use Composite Keys, but I'd say that mostly is because the default ORM is AR, which doesn't support it, and forces new users to its "convention", which for them means "the majority". I'm not fond of that argument in the AR, and I wouldn't be in the DJ case as well, since I'll have to trade data integrity for external convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 15, 2012
  1. payload object's object is no prepared to respond to the possibility …

    Tiago Cardoso authored
    …of a composite key implementation while still working for the standard of one primary key only (this was a fix to make djobs work with the gem composite_primary_keys)
  2. corrected another implementation which does not get along with compos…

    Tiago Cardoso authored
    …ite keys (see pull request 450)
  3. corrected an unfortunate leftover

    Tiago Cardoso authored
This page is out of date. Refresh to see the latest.
View
4 lib/delayed/psych_ext.rb
@@ -97,9 +97,9 @@ def visit_Psych_Nodes_Mapping_with_class(object)
when /^!ruby\/ActiveRecord:(.+)$/
klass = resolve_class($1)
payload = Hash[*object.children.map { |c| accept c }]
- id = payload["attributes"][klass.primary_key]
+ id = payload["attributes"].values_at(*klass.primary_key)
begin
- klass.unscoped.find(id)
+ klass.unscoped.find(*id)
rescue ActiveRecord::RecordNotFound
raise Delayed::DeserializationError
end
View
4 lib/delayed/serialization/active_record.rb
@@ -3,9 +3,9 @@ class ActiveRecord::Base
yaml_as "tag:ruby.yaml.org,2002:ActiveRecord"
def self.yaml_new(klass, tag, val)
- klass.unscoped.find(val['attributes'][klass.primary_key])
+ klass.unscoped.find(*val['attributes'].values_at(*klass.primary_key))
rescue ActiveRecord::RecordNotFound
- raise Delayed::DeserializationError, "ActiveRecord::RecordNotFound, class: #{klass} , primary key: #{val['attributes'][klass.primary_key]} "
+ raise Delayed::DeserializationError, "ActiveRecord::RecordNotFound, class: #{klass} , primary key: #{val['attributes'].values_at(*klass.primary_key).join(",")} "
end
def to_yaml_properties
Something went wrong with that request. Please try again.