Support ivars for `all_*` / `*_cloud` subquery blocks

[cupakromer]

Issue

This is the first time we've started using this gem. One of the very first things we tried to do was define tags in a Rails controller (a view has the same issue) where we scoped the tags based on a scoped association using an instance variable.

For example, lets say there is a tagged Widget class which belongs to a Company. In this case we want all of the tags for a widget for a specific company. On the controller we have this code:

@company = Company.find(params[:id])
@tags = Widget.all_tags { where(company: @company) }

The current all_tags implementation is using instance_eval:

acts-as-taggable-array-on/lib/acts-as-taggable-array-on/taggable.rb

Line 24 in daf33f5

subquery_scope = subquery_scope.instance_eval(&block) if block

Since there is no @company on the class the scoped call "fails" returning an empty array [].

SELECT tag FROM (
    SELECT DISTINCT unnest(widgets.tags) as tag
      FROM "widgets"
      WHERE "widgets"."company_id" IS NULL
  ) subquery

Workaround

To work around this we need to create artificial local variables just for the block scope:

@company = Company.find(params[:id])
company = @company
@tags = Widget.all_tags { where(company: company) }

Which produces the correct result:

SELECT tag FROM (
    SELECT DISTINCT unnest(widgets.tags) as tag
      FROM "widgets"
      WHERE "widgets"."company_id" = 1
  ) subquery

Proposal

It would be great if we could avoid having to do this two-step by either having the scope yielded to the block or if the code could support block local variables:

# yield scope
Widget.all_tags { |scope| scope.where(company: company) }

# block local variables
Widget.all_tags(@company) { |company| scope.where(company: company) }

If the yield scope version would be a breaking change, maybe the unused options hash could be leveraged for a block local variable alternative; if there are no other plans for options.

[skatkov]

Hey @cupakromer .

Thanks for your proposal and reporting this issue. I do agree that it looks like this needs an improvement, I'm not really sure what's the best way though (i'm a new maintainer).

But I'd be happy to figure it out with you. (I have a similar functionality I need to build for my own project)

For now, i'll try to ping original creator of this gem -- @tmiyamon . Hopefully he has time to give a bit more feedback.

[skatkov]

I had a try in my project and yeah... first-hand experienced your pain @cupakromer . :)

I would like to figure it out, but if really -- all proposed solutions don't look ActiveRecord'sy (and current block implementation seems hacky).

I'd be really exited to see proper support for AR relations. In your case:

Company.find(params[:id]).widgets.all_tags

It would be nice to have this without breaking existing solution.

I'll try to code something if i'll have time, don't promise it would be fast though.

[syrashid]

Hey not sure if this is still on anyone's radar but I've been using the gem recently quite frequently and ran into similar issues wanting a solution similar to the following

@company.widgets.all_tags

My work around has been creating a scope on the model as following

scope :all_tags, -> { where.not(tags: []).pluck(:tags).flatten.uniq }

Where I've overwritten the default class method, this gives me the freedom to do things like

@company.widgets.all_tags

or

Widget.all_tags

Now this definitely doesn't feel the most rails way of doing this, seeing as we're using a model scope to return an array of tags, but not sure if anyone else had any better solutions yet.

If you'd be willing to let me work on this I'd also be really happy to discuss and collaborate with whoever is currently maintaining. @skatkov ?

[skatkov]

@syrashid I'm not really using this gem anymore - but I do still have rights to publish new releases. If you want to take a stab at it, I don't mind giving it a review and publish a release.

But I would really need for that change to have tests and documentation :)

[syrashid]

@skatkov That would be great! I'll take a proper review of the full repo and then I'll make a pr.

[syrashid]

@skatkov Sorry it's taken so long to get around to this, but I finally managed to submit a pr accomplishing what we discussed. I believe it works as needed and fits within the existing implementation, would you mind taking a look and letting me know if all looks good? I added tests and bits to the documentation as well.

[skatkov]

Taking a look now (while sipping my mochito from vacation)