[Feature][ActiveRecord] Add support for specifying tables in where clauses in `unscope` method

[Slava Korolev]

I often see a use case when in a query, there are several relations with attributes with the same names(for example, in our codebase, there are several "soft-deletable" entities, and all of them have deleted_at attribute).

Article.where(deleted_at: nil).joins(:comments).where(comments: { deleted_at: nil })

At the moment, it's not possible to unscope deleted_at only for articles or only for comments.

I created a PR that adds support for specifying tables in where clauses in unscope method:

Article.where(deleted_at: nil).joins(:comments).where(comments: { deleted_at: nil }).unscope(where: { articles: :deleted_at })
  # == Article.joins(:comments).where(comments: { deleted_at: nil }) 

PR: https://github.com/rails/rails/pull/38608

Happy to get any feedback, thanks!

[James Coleman]

I think this will likely be confusing in many cases, since it's trivial to come up with example where it won't be able to work as expected.

For example, suppose that someone does Article.where("deleted_at IS NULL") in some method (or even harder to find: in a library), then the result of that has unscope(where: {articles: :deleted_at}) called on it. In that case the unscope call will be a no-op, and the result not only surprising, but likely to cause significant application bugs.

As such, I'm -1 on this change.

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-core/e5117870-9340-4d86-b81c-0ae75840d486%40googlegroups.com.

[Slava Korolev]

Thank you for your feedback!

I think you might misunderstand what I'm proposing, sorry for that.

You still can unscope as you did before, but if you want to specify table you want to affect, you can do it.

In your example, the current implementation also doesn't work:

Article.where("deleted_at IS NULL").unscope(where: :deleted_at) # == SELECT * FROM "articles" WHERE (deleted_at is NULL)

I don't see a way to fix this existing problem.

But if you don't use custom SQL in your queries you can use both old and new approaches:

Article.where(deleted_at: nil).unscope(where: :deleted_at) # == SELECT * FROM "articles"
Article.where(deleted_at: nil).unscope(where: { articles: :deleted_at }) # == SELECT * FROM "articles"

On Saturday, February 29, 2020 at 4:25:08 PM UTC+1, James Coleman wrote:

I think this will likely be confusing in many cases, since it's trivial to come up with example where it won't be able to work as expected.

For example, suppose that someone does Article.where("deleted_at IS NULL") in some method (or even harder to find: in a library), then the result of that has unscope(where: {articles: :deleted_at}) called on it. In that case the unscope call will be a no-op, and the result not only surprising, but likely to cause significant application bugs.

As such, I'm -1 on this change.

On Sat, Feb 29, 2020 at 1:00 AM Slava Korolev <kor...@gmail.com> wrote:

I often see a use case when in a query, there are several relations with attributes with the same names(for example, in our codebase, there are several "soft-deletable" entities, and all of them have deleted_at attribute).

Article.where(deleted_at: nil).joins(:comments).where(comments: { deleted_at: nil })

At the moment, it's not possible to unscope deleted_at only for articles or only for comments.

I created a PR that adds support for specifying tables in where clauses in unscope method:

Article.where(deleted_at: nil).joins(:comments).where(comments: { deleted_at: nil }).unscope(where: { articles: :deleted_at })
  # == Article.joins(:comments).where(comments: { deleted_at: nil }) 

PR: https://github.com/rails/rails/pull/38608

Happy to get any feedback, thanks!

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.

To unsubscribe from this group and stop receiving emails from it, send an email to rubyonra...@googlegroups.com.

[Geoff Harcourt]

Maybe the clearer way to apply it would be to allow unscoping via the merge method?

I think the overall idea is worth pursuing in some fashion. We often have big queries with lots of joins on aggregate dashboards, and ordering on default scopes for joined models adds wasted expense to queries. The workarounds are either no default scopes for ordering or joins via SQL string, which can be inconvenient for a number of reasons. 

To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-core/c044aa68-7fac-4eb6-bb35-93c0cd69e9c6%40googlegroups.com.

[Slava Korolev]

Can you provide an example of how you would like to see it with merge method?

My real-life case is more like that(we use Paranoia gem, but I show a simplified version):

class Article < ApplicationRecord
  has_many :articles_tags
  has_many :tags, through: :articles_tags
  default_scope { where(deleted_at: nil) }
end

class ArticlesTags < ApplicationRecord
  belongs_to :article, -> { unscope(where: :deleted_at) } # It intended to affect only articles; I would like to have it like "unscope(where: { articles: :deleted_at })"
  belongs_to :tag, -> { unscope(where: :deleted_at) } # It intended affect only tags; I would like to have it like "unscope(where: { tags: :deleted_at })"
end

class Tag < ApplicationRecord
  has_many :articles_tags
  has_many :articles, through: :articles_tags
  default_scope { where(deleted_at: nil) }
end

When I call
Article.joins(:tags)
It adds the scope from belongs_to :articles (in this case unscope(where: :deleted_at)) to the where chain and overrides the default scope of tags (where(deleted_at: nil)) even though it was intended to affect different tables

To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-core/c044aa68-7fac-4eb6-bb35-93c0cd69e9c6%40googlegroups.com.

[Geoff Harcourt]

We're using a similar solution, this is what I'd envision:

class Article < ApplicationRecord
  has_many :articles_tags
  has_many :tags, through: :articles_tags
  default_scope { where(deleted_at: nil) }
end

class ArticlesTags < ApplicationRecord
  belongs_to :article

  belongs_to :tag

end

class Tag < ApplicationRecord
  has_many :articles_tags
  has_many :articles, through: :articles_tags
  default_scope { where(deleted_at: nil) }

end

# This currently doesn't work
# There might be some confusion over whether this removes the article ownership
article.joins(:tags).merge(Tag.unscoped)

# This might be less likely to cause confusion or conflicts
article.joins(:tags).merge(Tag.unscope(where: :deleted_at))

Currently joins with default scopes are already a very "use with caution" kind of experience. The two cases for us where they are problematic at the moment are anything with a soft-delete (a where criteria that filters records out that we can't remove from the query) or an ordering for a big rollup query where the ordering of one of the joins in the middle is just wasted effort and won't be used at all in the final result.

The place where this becomes really inconvenient is when the default scope falls on a joined relationship that has further joins on it, as once you go to a join via a string condition you lose the ability (I think?) to continue on with AR relationships.

To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-core/c311ad8c-9d0a-4b4a-bc6b-03713f7c5b5e%40googlegroups.com.

[Slava Korolev]

I see a potential problem in Article.joins(:tags).merge(Tag.unscope(where: :deleted_at)):
For example — what happens if you add something in between Tag and unscope? Like Article.joins(:tags).merge(Tag.joins(:whatever).unscope(where: :deleted_at))

I don't see a way to implement it without explicitly defining a relation to affect in the unscope method. 

Why do you think Article.joins(:tags).unscope(where: {tags: :deleted_at}) is confusing?
It repeats the structure of a where clause: Article.joins(:tags).where(tags: {deleted_at: nil}) and don't break the current behaviour.

We can think about something similar for ordering too:
Article.joins(:tags).unscope(order: :tags)
Article.joins(:tags).unscope(order: {tags: :deleted_at})

And yes, I think once you go to a join(or query) via a string condition, you lose the ability to unscope anyway.

To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-core/c311ad8c-9d0a-4b4a-bc6b-03713f7c5b5e%40googlegroups.com.