Support nested children in Arel::Nodes::And

[ciihla]

Fixes #925.
Fixes #1240.

[scarroll32]

This looks great but do we have test coverage?

[deivid-rodriguez]

Nope, also I tried to reproduce the issue and couldn't.

[scarroll32]

@ciihla thank you for this contribution, is it possible to add a test (that would fail first without your PR) ?

[ciihla]

@ciihla thank you for this contribution, is it possible to add a test (that would fail first without your PR) ?

The test is very specific, maybe it has to do something with combination of multitenancy(acts_as_tenant) + postgres_ext-serializers + our specific usage (hierarchy sql condition). but basically our test fails within this:

let!(:column) { create(:column, space: space) }
    let!(:column2) { create(:column, space: space) }
    let!(:column_value) { create(:column_value, space: space, feature: feature_1, column: column) }
    let!(:column_value2) { create(:column_value, space: space, feature: feature_2, column: column2) }

    let(:params) { { q: { column_values_column_id_not_eq: column.id, id_in: [feature_1.id, feature_2.id] } } }

    it 'returns all data' do
      expect(space.features
                      .select('features.*, case when count(childs.id) > 0 then true else false end as has_children')
                      .joins('left join features childs on childs.parent_id = features.id')
                      .group('features.id').ransack(params[:q]).result.to_a.count).to eq(1)
    end

error:

  NoMethodError:
       undefined method `children' for nil:NilClass
     # /Users/jan.uhlar/.rvm/gems/ruby-3.0.3@pb-backend/gems/ransack-2.5.0/lib/ransack/adapters/active_record/context.rb:146:in `block in remove_association'
     # /Users/jan.uhlar/.rvm/gems/ruby-3.0.3@pb-backend/gems/ransack-2.5.0/lib/ransack/adapters/active_record/context.rb:145:in `delete_if'
     # /Users/jan.uhlar/.rvm/gems/ruby-3.0.3@pb-backend/gems/ransack-2.5.0/lib/ransack/adapters/active_record/context.rb:145:in `remove_association'
     # /Users/jan.uhlar/.rvm/gems/ruby-3.0.3@pb-backend/gems/ransack-2.5.0/lib/ransack/adapters/active_record/ransack/nodes/condition.rb:10:in `block in arel_predicate'
   

Sorry I dont have much time to elaborate more :(

[deivid-rodriguez]

Thanks @ciihla, it definitely makes sense to me that a third gem is involved here. If you find time to isolate this to a sample app, it'd be great. Even temporarily removing either of these gems from your application to see if the issue still reproduces and further isolate the culprit would be great.

[shannondoah]

We encountered another use case that this patch seems to fix: querying a scoped association where the association model also has a default scope (multiple conditions). Here is a standalone test that exposes the error, and a copy with the patch applied that executes without error.

Here is another example reproducing the failure with a different set of conditions.

[tappleby]

@shannondoah I just ran into this same issue, have you been running this patch in production?

[shannondoah]

@tappleby No, we haven't. For the use case we encountered initially, we swapped out ransack's not_eq with a custom Arel predicate as a safer workaround. This has since come up in other places in our application where the same workaround may not be feasible, however, so we are hoping a more robust solution makes its way into an official release 🤞

[gregnavis]

We also run into this problem while using acts_as_tenant:

# This results in NoMethodError
ActsAsTenant.with_tenant(Tenant.first) { Job.ransack({"entity_tags_tag_id_not_in"=>["1"]}).result.to_a }


# This works fine
ActsAsTenant.without_tenant { Job.ransack({"entity_tags_tag_id_not_in"=>["1"]}).result.to_a }

It'd be great to see this PR merged. For now, we'll use an override similar to what @shannondoah described.

[eqdrs]

I'm running into this issue when filtering by an associated attribute (many-to-many) using negative matchers like not_in or not_null.

After applying the suggested changes, I hit a different problem: the extract_correlated_key method is now returning an array with two Arel::Nodes::Equality objects.

This causes the following error:

undefined method 'eq' for an instance of Array

Any idea how we could handle this case?

[bopm]

@eqdrs can you try this fork #1561? I am way behind on providing test cases and other needed things to get it merged, but I am using this code for quite some time in production.

[bopm]

@ciihla I've included your changes in my PR, and planning to offer it for review. Tell me if you have any objections.

[phikes]

Just wanted to chime in here and say: This PR has fixed the same error as in #925 for me, however I also have a fairly complex case:

I am using acts_as_taggable_on and want to search for records which do not have a specific tag at all among their tags. I am doing this by using a field of record_tags_name and a predicate of not_eq_all with a value of the tag name that shouldn't be among the tag names of the record.

If necessary I can try to write a test case, let me know!

[bopm]

@phikes it may help to fix it :) So if you going to do it, keep me posted, I am happy to look into that too.

[bopm]

And to be clear, @phikes did you try my PR?

[phikes]

Thanks for coming back to me, @bopm and thanks for working on this :-). I did not. I monkey patched the changes from this PR. I can try to wok on a test case tomorrow. Without looking into the code: Do you happen to know if the codebase already depends on the acts_as_taggable gem for development? That would probably make it very easy to construct the very same test case.

[bopm]

@phikes two thoughts here:

1. Probably, you do not need that dependency by default; it's more than enough to just conform to the table structure expected.
2. I dealt with acts_as_taggable in the codebase I patched it all together for. In this form:

ransacker :tagged_ids do |parent|
    tagging_table = ActsAsTaggableOn::Tagging.arel_table
    tag_table = ActsAsTaggableOn::Tag.arel_table
    Arel::Nodes::Grouping.new(
      tagging_table
        .join(tag_table)
        .on(tag_table[:id].eq(tagging_table[:tag_id]))
        .where(
          tagging_table[:taggable_type].eq(base_class.name)
          .and(tagging_table[:taggable_id].eq(parent.table[:id]))
          .and(tag_table[:identifies_id].eq(parent.table[:org_id]))
          .and(tag_table[:identifies_type].eq(Org.to_s)),
        )
        .project(
          Arel::Nodes::NamedFunction.new(
            'GROUP_CONCAT',
            [
              Arel::Nodes::NamedFunction.new(
                'DISTINCT',
                [tag_table[:id]],
              ),
            ],
          ),
        ),
    )
  end

[scarroll32]

@ciihla can this be closed now #1561 is merged?

[ciihla]

@ciihla can this be closed now #1561 is merged?

probably so 👍