Doesn't work with table_prefixes

[juddblair]

I'm using a table prefix, which is specified in application.rb like so:

config.active_record.table_name_prefix = "d2"

I have a model called "TagDefinition" that I have acts_as_tree on.

The problem is that acts_as_tree appears to look at the table name for TagDefinition, which due to the prefix is actually "d2_tag_definition", and then creates a model "D2TagDefinitionHierarchy" - ActiveRecord adds the table prefix to this, so it thinks the table name for this model is d2_d2_tag_definition_hierarchies. The actual table I added with the migration is d2_tag_definition_hierarchies.

So, the SQL queries in the gem where you manually insert the table name work fine, but if you do something like an ActiveRecord delete on TagDefinition (or pretty much anything where ActiveRecord tries to handle the generated Hierarchy object, rather than with manual SQL), it errors out when ActiveRecord uses the prefix and tries to select from d2_d2_tag_definition_hierarchies, which doesn't exist.

Rails had a similar bug in the table dumper, described here: rails/rails@1bdc098#activerecord/lib/active_record/schema_dumper.rb

I think if you change hierarchy_class_name to strip the prefix from the table name like rails did, you should be fine.

[juddblair]

I fixed it in my fork: https://github.com/juddblair/closure_tree

Rspec is passing, and I built and tested the gem locally with my application, and everything looks good. I didn't write a breaking test for the implementation, but I can submit a pull request for my fix if you'd like.

[mceachen]

Excellent issue request, sir. Thanks for your thoroughness!

I pulled your change onto a new branch, but the weather's too nice to keep typing right now. I think we (and by "we", I don't mean you have to do it) should add a couple breaking tests (to test with a prefix and a suffix), or do something clever with running the whole test suite with Label or Tag with or without a prefix, just to make sure things are working in all environments.

Thanks again for taking the time to write this up!

[juddblair]

Sure thing - I started playing around with the tests since, despite the nice weather, I'm under the weather. I added a prefix and suffix to the spec helper and noticed a few things:

1. The indexes were too long with long prefixes/suffixes. I changed the test table definitions to use explicit index names, which I also had to do in my application. Might be something to fix in the generator if possible.
2. Suffix was problematic - since we singularize the hierarchy table name and use that to name the class, anything that ended with an "s" (actually any suffixes, but especially seemingly plural ones) threw AR for a loop. I fixed this by escaping the prefix/suffix before singularizing it, and then re-adding them, but the resulting line is...interesting (acts_as_tree.rb:393). Might be a cleaner way to do that but it does work.
3. A test on cuisine type failed because the table name assertion didn't take prefix/suffix into account. Fixed this.
4. Several User tests failed because with the addition of prefix/suffix, the supplied table name was no longer correct. Fixed this. As an aside, should we supply the prefix/suffix to custom table names? It seems like we should just accept what people pass in (using a pre-existing table with a different/no prefix/suffix seems to be a good use case, or a shared DB), but I'm not sure how others do this. I'd vote for correct as-is.

Personally, I think the tests should run with all permutations (prefix, suffix, prefix+suffix, neither), but this seems like a good start.

I updated my fork with my changes.

[mceachen]

All permutations: http://travis-ci.org/#!/mceachen/closure_tree/builds/2473467

We'll see how it goes.

[mceachen]

released 3.6.0. Thanks again for your help!

[juddblair]

Sweet! Thanks for the quick turnaround with the release, much appreciated.