Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add breaking test to cover double join errors #512

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ramongtx
Copy link

The specs in this PR adds specs that show specific untested conditions that give raise to a table being joined twice in the same query.

After trying to upgrade to Rails 6, many segments and filters stopped working and we were trying to find the root cause, when we bumped into what seems to be a performance issue in Forest Rails.

To explain what we have found, whenever we try to do an operation that would end up filtering and including an association, i.e. having a field from an associated model in both filter and sort, the query builder ends up double joining the same table, with different aliases, which cost considerably in query performance, doubly so if the table is connected through a has_one through: relationship.

By building a similar query using only ActiveRecord, we wouldn't have this issue (Island.eager_loads(:location).where(location: { id: 1 })). So, looking down the code we have found out that the current use of ArelHelpers library, together with Rails 6.1 table memorization is probably the cause. This sequence of calls that was derived from the filter parser plus resources getter demonstrates the issue:

aliased_resource = Island.reflect_on_association(:location).klass.include(ArelHelpers::Aliases).aliased_as(:location)
joined_resource = ArelHelpers.join_association(Island, :location, Arel::Nodes::OuterJoin, aliases: aliased_resource)
final_resource = Island.eager_load([:location]).joins(joined_resource)

PS: In our specific use-case, the situation is a bit worse than just a performance hit. One of our tables has its name hardcoded (not derived from the class name) and this causes the issue to escalate and give a PG::DuplicateAlias error when the double join of the issue above occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants