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

Clear @cache_keys cache after update_all, delete_all, destroy_all #41789

Merged
merged 1 commit into from Apr 7, 2021

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Mar 29, 2021

The calculated cache key is cached on a relation, but there is no way to
re-calculation even if mutation to collection on a relation is happened.

The @cache_keys cache should be cleared at least after update_all,
delete_all, destroy_all.

Related to #41784.

…all`

The calculated cache key is cached on a relation, but there is no way to
re-calculation even if mutation to collection on a relation is happened.

The `@cache_keys` cache should be cleared at least after `update_all`,
`delete_all`, `destroy_all`.

Related to rails#41784.
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find tap very confusing, but up to you if you are going to use it

@kamipo
Copy link
Member Author

kamipo commented Mar 30, 2021

There are two main reasons why I've used tap here.

The first one is destroy_all already uses that, and the ensure block-like cleaning up in tap is a little common pattern in the codebase.

def destroy_all
records.each(&:destroy).tap { reset }
end

% git grep -n '\.tap { reset' lib
lib/active_record/associations/collection_proxy.rb:473:        @association.delete_all(dependent).tap { reset_scope }
lib/active_record/associations/collection_proxy.rb:500:        @association.destroy_all.tap { reset_scope }
lib/active_record/associations/collection_proxy.rb:619:        @association.delete(*records).tap { reset_scope }
lib/active_record/associations/collection_proxy.rb:691:        @association.destroy(*records).tap { reset_scope }
lib/active_record/relation.rb:491:      klass.connection.update(stmt, "#{klass} Update All").tap { reset }
lib/active_record/relation.rb:578:      records.each(&:destroy).tap { reset }
lib/active_record/relation.rb:619:      klass.connection.delete(stmt, "#{klass} Destroy").tap { reset }

The second one is that I'd not like to name what the return value of klass.connection.update is.

The return value of klass.connection.delete is named as affected, but the return value of klass.connection.update is actually found/matched rows count (#25370), not affected rows.

If there is no strong objection to those points, I would like to continue to use it here.

@rafaelfranca
Copy link
Member

Like I said it is up to you. But tap and map are too close to me that sometimes I read that code and I think it is returning an array of items with the return of reset.

@kamipo
Copy link
Member Author

kamipo commented Apr 7, 2021

I see your point.

Just as the tap { break expr } idiom was added as yield_self { expr } / then { expr } in Ruby, the ensure block-like tap idiom may be invented in a better way in the future (e.g. as Golang like defer: https://github.com/tagomoris/deferral).

In the meantime, leave this idiom as it is for now.

@kamipo kamipo merged commit 7fd8079 into rails:main Apr 7, 2021
@kamipo kamipo deleted the update_cache_key_after_mutation branch April 7, 2021 07:47
kamipo added a commit that referenced this pull request Apr 7, 2021
Clear `@cache_keys` cache after `update_all`, `delete_all`, `destroy_all`
kamipo added a commit that referenced this pull request Apr 10, 2021
kamipo added a commit that referenced this pull request Apr 10, 2021
quadule added a commit to quadule/rails that referenced this pull request Sep 5, 2022
Call `reset` after bulk inserts and upserts to discard stale data. This
matches the behavior of `update_all` added in rails#41789.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants