Fix include_optional_linkage_data with joins

[pjungwir]

When the user adds a join in records, it confuses get_join_arel_node because it finds the same number of joins before & after adding a join for include_optional_linkage_data (or equivalently for always_include_*_linkage_data).

This commit falls back to searching the existing arel nodes for a compatible join and uses that if found.

Fixes #1449

All Submissions:

• I've checked to ensure there aren't other open Pull Requests for the same update/change.
• I've submitted a ticket for my issue if one did not already exist.
• My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
• I've used Github auto-closing keywords in the commit message or the description.
• I've added/updated tests for this change.

Bug fixes and Changes to Core Features:

• I've included an explanation of what the changes do and why I'd like you to include them.
• I've provided test(s) that fails without the change.

Test Plan:

Reviewer Checklist:

• Maintains compliance with JSON:API
• Adequate test coverage exists to prevent regressions

[averydev]

This has had a hugely positive impact on performance for us. Thank you!

[lgebhardt]

This looks good to me. Thanks @pjungwir

[Jasonlgrd]

Hey, thanks for this fix. But I have another problem, more tricky 😬

I have a user and patient_enrolled_journal_event resources

In user resource, I have this relation:

has_one :patient_enrolled_journal_event

In patient_enrolled_journal_event resource, I have this relation:

has_one :patient_enrolled_journal_event,
              exclude_links: :default,
              foreign_key_on: :related,
              relation_name: :patient_enrolled_journal_event

In PatientEnrolledJournalEvent model, I have this scope:

scope :active, lambda {
    where(archived_at: nil).left_outer_joins(:user).where.not(
      user: {
        id: nil
      }
    )
  }

And in PatientEnrolledJournalEventResource, I rewrite records method to apply this scope like this:

def self.records(options = {})
    super.active
  end

And when I get http://localhost:3001/api/v1/users?include=patient-enrolled-journal-event I have this SQL error :

SQLite3::SQLException: ambiguous column name: id:\nSELECT patient_enrolled_journal_events.*, \"id\" AS jr_source_id FROM \"patient_enrolled_journal_events\" INNER JOIN \"users\" \"user\" ON \"user\".\"id\" = \"patient_enrolled_journal_events\".\"user_id\" WHERE \"patient_enrolled_journal_events\".\"archived_at\" IS NULL AND \"user\".\"id\" IS NOT NULL AND \"patient_enrolled_journal_events\".\"id\" = ? ORDER BY patient_enrolled_journal_events.id asc\n^",

I have investigated, my breakpoint is in get_join_arel_node method:

last_join = join_sources.find { |j|
            valid_join_types.any? { |t| j.is_a?(t) } && j.left.name == table_name
          }

j.left.name = user
table_name = users
So, the condition is false :(

If I replace my left_outer_joins in scope with this:

scope :active, lambda {
    where(archived_at: nil).joins("LEFT OUTER JOIN users ON users.id = #{table_name}.user_id").where.not(
      user: {
        id: nil
      }
    )
  }

it fix the problem, but I think isn't a good solution.

What do you think about this error ? Thanks 🙏

[Jasonlgrd]

I can create an issue but before I would like your opinion on this problem :D

[pjungwir]

Thanks for your note! It looks like you found a bug with this PR. We are comparing an attribute name (singular) to a table name (plural). It worked for me because I was joining through a has_many, so the attribute name was also plural. But joining in the other direction, as you're doing, has a mismatch. So the fix is to use the table name of j.left. Diving into Arel for 10 minutes hasn't shown me how to do that yet, but if you find a way then I think it would make a good PR.

[pjungwir]

Just guessing, but probably j.left.relation.name (assuming j.left is going to have a Arel::Attributes::Attribute)?

[Jasonlgrd]

table_name = relationship.resource_klass._table_name

last_join = join_sources.find { |j|
    valid_join_types.any? { |t| j.is_a?(t) } && j.left.name == table_name
}

You compare a relation name with a table name above.
What do you think about to replace this:

table_name = relationship.resource_klass._table_name

with

table_name = relationship.name

(It's a idea for the moment, I don't launched tests)

[pjungwir]

Does it make more sense to match on the table name or the Rails association name? I lean toward the table name (see my comments just above), since associations could have different names but refer to the same table---but maybe the association name is okay instead. If that's what you prefer, then you should rename the local variable from table_name to something like association_name.

[Jasonlgrd]

The join_sources[0].left is a Arel::Nodes::TableAlias class so we have a table_name getter. I think we can use this getter instead of name ?

My suggestion

 last_join = join_sources.find { |j|
            valid_join_types.any? { |t| j.is_a?(t) } && j.left.table_name == table_name
          }

And now we compare the table names. What do you think ?

[pjungwir]

That looks right to me!

[Jasonlgrd]

Thanks, I will create a PR to propose a fix !