Use left joins instead of inner join when searching

[samnang]

When searching an association with belongs_to :foobar, optional: true, the current implementation uses INNER JOIN which will filter out resources where the association does not exist.

By default when searching associations, it should LEFT OUTER JOIN the association.

[sedubois]

The tests throw NoMethodError: undefined method 'left_joins' for #<ActiveRecord::Relation []>.

[samnang]

Oh, I didn't notice the left_joins is only supported since the Rails 5, so we need to have a conditional check for the Rails version. If it's still on Rails 4, we need to find a way to dynamic generating the left joins query string from those associations. I tried to access attribute_types[:association_name], but I couldn't find a way to get primary_key and foreign_key options, is there any way to access those options?

[sedubois]

@pablobm should Rails < 5 be dropped from the appraisal as it's EOL?

[nickcharlton]

I'm sort of averse to dropping Rails <5 support just yet, and it wouldn't be something I'd want to do in this PR. I've opened #1588 for that bit.

[pablobm]

Duplicate of #1386

[pablobm]

@samnang - A way to retrieve association keys would be using the reflection API. For example:

> Order.reflect_on_association(:line_items).foreign_key
=> "order_id"

This also works on scopes:

> Order.where(address_state: 'NY').reflect_on_association(:line_items).foreign_key
=> "order_id"

Having said that, we may end up dropping support for Rails <5 in the next few months (see #1588), so this solution may not live long... or will it?

[pablobm]

Also reported at #1637, which includes a detailed description of the issue.

[samnang]

@samnang - A way to retrieve association keys would be using the reflection API. For example:

> Order.reflect_on_association(:line_items).foreign_key
=> "order_id"

This also works on scopes:

> Order.where(address_state: 'NY').reflect_on_association(:line_items).foreign_key
=> "order_id"

Having said that, we may end up dropping support for Rails <5 in the next few months (see #1588), so this solution may not live long... or will it?

Thanks for the response. I think we better to wait instead of introducing complexity by using reflection to construct left join for all tables to join.

[livedo]

@nickcharlton, instead of just waiting, would you be open to merging a version with essentially .send(Rails::VERSION::MAJOR >= 5 ? "left_joins" : "joins", tables_to_join) for the time being?

[pablobm]

@livedo, I would be ok with that, with small changes.

Instead of detecting the Rails version, I'd prefer respond_to?(:left_join). Also I'd file this away into a dedicated, self-describing function. Something like this:

    def search_results(resources)
      inner_or_left_joins(resources, tables_to_join).
        where(query_template, *query_values)
    end

    def inner_or_left_joins(left, right)
      # ActiveRecord::QueryMethods#left_join is not supported with Rails 4 and below
      if left.respond_to?(:left_join)
        left.left_joins(right)
      else
        left.joins(right)
      end
    end

[nickcharlton]

I second @pablobm's suggestion. I'll also say that now the 0.14.0 release is out, removing support for Rails 4 is something we'll be looking at soon and we can likely merge this branch broadly as is.

So if it's something we can avoid working around, I'd recommend patching your current install instead of us merging that.

[pablobm]

As @nickcharlton says, my suggestion is not relevant any more, since we have dropped support for Rails 4.

@samnang - Would you be able to correct the lint warnings (as raised by Hound)?

@nickcharlton - Would you be ok to merge this, given the above?

[nickcharlton]

Thanks for your work on this!