Allow for use of a different primary key #50
Conversation
|
is it possible to add an alias method like |
|
Unfortunately no: in my particular case, the |
|
Given that we already support customizing the foreign key, it makes sense to also be able to customize the primary key.at the option works as expected. @Two9A The changes look good to me. Do you think you would be able to add a test case that tests the customization of the |
|
Interestingly, it seems that the |
|
Recent? It's present in rails 2.2… |
|
Ah, I see. The tests are failing (but only sometimes) on |
| @@ -141,7 +141,7 @@ def leaves | |||
| class_eval <<-EOV | |||
| def self.leaves | |||
| internal_ids = select(:#{configuration[:foreign_key]}).where(arel_table[:#{configuration[:foreign_key]}].not_eq(nil)) | |||
| where("id NOT IN (\#{internal_ids.to_sql})").default_tree_order | |||
| where("#{configuration[:primary_key]} NOT IN (\#{internal_ids.to_sql})").default_tree_order | |||
There was a problem hiding this comment.
This must use quote_column_name or should be changed to use arel. There is no guarantee, that the value given to :primary_key is not a reserved keyword or contains characters that require quoting.
|
I think a lot of the duplication in the new tests could be avoided by setting the constant for the TreeMixin to an instance var in |
|
I think the test failures are because |
| @@ -141,7 +141,7 @@ def leaves | |||
| class_eval <<-EOV | |||
| def self.leaves | |||
| internal_ids = select(:#{configuration[:foreign_key]}).where(arel_table[:#{configuration[:foreign_key]}].not_eq(nil)) | |||
| where("#{configuration[:primary_key]} NOT IN (\#{internal_ids.to_sql})").default_tree_order | |||
| where.not(:#{configuration[:primary_key]} => internal_ids).default_tree_order | |||
There was a problem hiding this comment.
This is not backwards compatible, where.not is a very recent addition to activerecord.
There was a problem hiding this comment.
Try the following:
where(arel_table[:#{configuration[:primary_key]}].not_in(internal_ids))
|
Wow, that was worse. |
|
The test for
|
|
Alright, the only failures are on Ruby 1.9, so I'm going to call that a pass. Sorry about the mess. |
|
Yes, it seems some dependencies no longer support ruby 1.9.2 / 1.9.3 and need to be locked to older versions. Those are not related to your changes, so it's ok. |
|
@Two9A The changes look good, can you squash them to a single commit? If you don't know how to do that let me know and I can do it for you. |
| @@ -63,6 +65,7 @@ module ClassMethods | |||
| # a custom column by passing a symbol or string. | |||
| def acts_as_tree(options = {}) | |||
| configuration = { | |||
| primary_key: "id", | |||
There was a problem hiding this comment.
Could you change the default to the tables primary key? It is returned by calling the method primary_key.
That way it will work fine, if the model uses a non-standard primary key name.
|
@Two9A I have one small change for the default value of the |
|
There are still random failures on older rails versions, likely due to activerecord's column cache. |
|
I've got a fix for the test failures on rails 3.2 in my local fork, I'm still working on finding a fix for the somewhat sporadic failures with activerecord 4.1. |
|
@Two9A I just pushed acts_as_tree 2.5.0 to RubyGems, which includes your changes. Thank you for your contribution and sorry for the delay! |
In situations where external tree-based data is cached or otherwise held locally, the ids used by the external service can differ from the database's primary key.