fix: include through associations referenced in filters in ResourcesGetter

[Pacyfik]

Hi 👋

Background

This PR is a follow up to my previous PR: #732
forest_liana 9.12.0 introduced a db query optimisation which includes specifying fields which are to be fetched from the database.
I've found another situation where this causes an issue. Specifically, when there is a filter or multiple filters applied on the forest admin frontend on a through association which is not a displayed column.
I hope the specs I included explain the exact situation. You can reproduce the error if you comment out line 351 of app/services/forest_liana/resources_getter.rb (select << "#{@resource.table_name}.#{association.foreign_key}") and run the new specs.

I'd appreciate your opinions on the approach taken and would be happy to help validate any alternative ones.

--

We've also experienced a few similar issues related to polymorphic associations which this PR does not fix.
I've had a hard time investigating those. However, I've found out that uncommenting this particular line in lib/forest_liana/active_record_override.rb seems to fix most of them.
Could you consider uncommenting this line in the next release? The issues I've mentioned block us from updating the gem at the moment.

Affected Versions

Forest agent (forest_liana) >= 9.12.0

Definition of Done

General

• Write an explicit title for the Pull Request, following Conventional Commits specification
• Test manually the implemented changes
• Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

• Consider the security impact of the changes made

[matthv]

Hi 👋

Thanks a lot for your contribution!

I tried reproducing the issue on my side with a basic setup like this:

class Owner < ApplicationRecord
  has_one :dog
  has_one :cabin
  has_one :solar_panel, through: :cabin
end

If I hide the solar_panel column in Forest and apply one or multiple filters on the collection, everything seems to work as expected; no error is raised.

Is there any particular configuration in your project that might help us better understand the conditions to reproduce the bug? Or maybe I misunderstood the expected behavior in this case?

Happy to dig further if you can share more insights!

[Pacyfik]

Hi @matthv 👋
Thanks for taking a look.

In the situation you're describing, I think you'd need to hide cabin and solar_panel (since solar_panel is a through: :cabin association) but show dog and also filter using solar_panel. This is because the compute_select_fields logic gets applied only when there is an association included/requested (see here).

You can try using my branch, commenting out this line and running the specs I've added. They reflect our use case quite well.

I've been thinking about some of the other issues we've encountered as well which would not be fixed by this PR. Unfortunately, I just don't have the capacity to investigate them further. Like I said before, uncommenting this line in lib/forest_liana/active_record_override.rb makes everything work again. At the cost of some performance degradation obviously.
Would you consider adding a configuration flag to the gem which would allow us to apply that line? It could be an opt-in config, as I imagine most users don't need this and might like the performance benefit. In our case, we'd prefer things to work as before and still be able to update the gem to the newest version.

[matthv]

I spent some time on this today and noticed that the extract_associations_from_filter method wasn't behaving correctly. I took the opportunity to patch it in this draft branch, which also incorporates some of the logic you proposed in your patch.

Could you let me know if this fixes the issue you’ve been encountering?

I'm also going to test your branch and will get back to you shortly.
Finally, I'm looking into the other issue you mentioned regarding polymorphic associations as well.

Thanks again for all the detailed feedback! 🙏

[Pacyfik]

Hi @matthv Sorry for the delay in responding. I've looked at your fix and it does indeed cover the case which I attempted to fix with my PR 🎉
However, the other issues we've experienced, specifically with polymorphic associations, still persist.
Since we lack the resources to focus on resolving them one by one, ideally we'd love to be able to conditionally include the line from ForestLiana::ActiveRecordOverride::Associations::JoinDependency that I mentioned before, based on config or even an env var.

[matthv]

Hi @Pacyfik
I’ve just merged the first fix v9.15.1. I’ll take care of the other issue as soon as I have some time.

In the meantime, I’ve created another branch with the line uncommented to unblock you for now.