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

Does not respect association ON clause (acts_as_paranoid) #570

Open
mrbrdo opened this issue Apr 18, 2022 · 8 comments
Open

Does not respect association ON clause (acts_as_paranoid) #570

mrbrdo opened this issue Apr 18, 2022 · 8 comments

Comments

@mrbrdo
Copy link
Contributor

mrbrdo commented Apr 18, 2022

Let's say the Translation model includes acts_as_paranoid.
Typical Rails Base.all.joins(:translations) will correctly have INNER JOIN ... ON ... ."deleted_at" IS NULL
However, the scopes/queries generated by Mobility will not include this ON condition.

I'm not sure how to go about fixing this... It seems the ActiveRecord interface to joins is mostly private and I didn't find a way to get a complete join (e.g. Arel join with all conditions) from ActiveRecord. I didn't find a way in ActiveRecord to perform a join with a table alias (for an association with taking into account the ON conditions).
Any suggestion? Is there at least a single place / setting in Mobility where it would be possible to add the additional conditions for the joins Mobility generates internally?

@mrbrdo mrbrdo changed the title Does not respect association conditions (acts_as_paranoid) Does not respect association ON clause (acts_as_paranoid) Apr 18, 2022
@shioyama
Copy link
Owner

shioyama commented May 3, 2022

Can you create either a test or a demo app which shows the issue? Also, I've tagged this with table_backend since you're referring specifically to something that impacts that backend only.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 3, 2022

Yes I think it only affects table_backend.

See failing spec: https://github.com/mrbrdo/mobility/blob/mrbrdo/spec/mobility/plugins/active_record/query_spec.rb#L114

@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 26, 2022

@shioyama can you comment on this failing spec?

@shioyama
Copy link
Owner

shioyama commented Jun 18, 2022

Thanks for raising the issue.

Let's say the Translation model includes acts_as_paranoid.

The Translation model is really an implementation detail, it's not built to handle things like this. I don't know how to fix this, it may be possible but it doesn't look easy.

The Table backend uses aliases to distinguish between joins in different locales, so instead of article_translations it's aliasing that to article_translations_en (say for English). That allows to do very powerful things in multiple locales without conflicts between locales, but of course in this case it makes things difficult. Even if we add the default scope into the query, which is possible, it doesn't work because it would be WHERE "article_translations"."title" != 'DELETED'" which doesn't match the join, so it fails.

If there was a simple way to add a default_scoped somewhere in either the AR table backend or in the query plugin, I'd be interested in this, but otherwise I think it's really stretching what Mobility is able to do.

Sorry about that! I suspect at least there's a fix that would work in a non-generic way, but I don't see it right now.

@shioyama
Copy link
Owner

I'd like to close this since I don't see it as an issue with Mobility, but I'll leave it open in case we can come to some generic solution.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Jun 22, 2022

Possibly the last question from my OP? Is there at least a single place / setting in Mobility where it would be possible to add the additional conditions for the joins Mobility generates internally?
It would at least allow to manually update the join scope, even if it's not automatic, it would be helpful.

You can see how I patched it for now in my use case (AR backend): https://github.com/mrbrdo/spree_mobility/blob/main/lib/spree_mobility/core_ext/mobility/backends/active_record/table/mobility_acts_as_paranoid_decorator.rb

@shioyama
Copy link
Owner

shioyama commented Jun 22, 2022

Is there at least a single place / setting in Mobility where it would be possible to add the additional conditions for the joins Mobility generates internally?

It could be an option for the table backend, used in apply_scope.

Actually, it's already possible if you subclass Mobility::Backends::ActiveRecord::Table and patch the private method join_translations. So I'd suggest maybe trying that first. You can pass the new backend class to backend: rather than :table and it should work.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Jun 22, 2022

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

No branches or pull requests

2 participants