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

Fix counter_cache create/concat with overlapping counter_cache_column #41321

Conversation

intrip
Copy link
Contributor

@intrip intrip commented Feb 3, 2021

Summary

Fix when multiple belongs_to maps to the same counter_cache column.
In such situation inverse_which_updates_counter_cache may find the wrong relation which leads into an invalid increment of the counter_cache.

This is done by improving the inverse relation detection in 2 ways:

  • by comparing inverse.klass with the current active_record (doesn't work for polymorphic)
  • by relying on inverse_of

The above adjustments allows to correctly map the inverse_which_updates_counter_cache for both non-polymorphic/polymorphic relations.

Fixes #41250

@intrip intrip force-pushed the 41250-fix-update-counter-cache-same-column-name branch 17 times, most recently from 27acec4 to 6283ea3 Compare February 8, 2021 20:09
@intrip intrip changed the title WIP: Fix counter cache update with came counter_cache_column WIP: Fix counter_cache create/concat with overlapping counter_cache_column Feb 8, 2021
@intrip intrip force-pushed the 41250-fix-update-counter-cache-same-column-name branch 4 times, most recently from f87328b to a944e97 Compare February 9, 2021 10:08
@intrip intrip changed the title WIP: Fix counter_cache create/concat with overlapping counter_cache_column Fix counter_cache create/concat with overlapping counter_cache_column Feb 9, 2021
@intrip intrip force-pushed the 41250-fix-update-counter-cache-same-column-name branch from a944e97 to 961ebf9 Compare February 13, 2021 11:14
@intrip
Copy link
Contributor Author

intrip commented Mar 11, 2021

Would be great if somebody from the could take a look at this one :)

@intrip
Copy link
Contributor Author

intrip commented May 10, 2021

bump 🏓

@rails-bot
Copy link

rails-bot bot commented Aug 8, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Aug 8, 2021
Fix when multiple `belongs_to` maps to the same counter_cache column.
In such situation `inverse_which_updates_counter_cache` may find the
wrong relation which leads into an invalid increment of the
counter_cache.

This is done by improving the inverse relation detection in 2 ways:
- by comparing `inverse.klass` with the current
  `active_record` (doesn't work for polymorphic)
- by relying on `inverse_of`

The above adjustments allows to correctly map the
`inverse_which_updates_counter_cache` for both non-polymorphic/polymorphic
relations.

Fixes rails#41250
@intrip intrip force-pushed the 41250-fix-update-counter-cache-same-column-name branch from 961ebf9 to b25f3e5 Compare August 9, 2021 20:58
@rails-bot rails-bot bot removed the stale label Aug 9, 2021
@rails-bot
Copy link

rails-bot bot commented Nov 7, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Nov 7, 2021
@intrip
Copy link
Contributor Author

intrip commented Nov 8, 2021

still relevant

@rails-bot rails-bot bot removed the stale label Nov 8, 2021
@rails-bot
Copy link

rails-bot bot commented Feb 6, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Feb 6, 2022
@@ -298,6 +300,11 @@ def actual_source_reflection # FIXME: this is a horrible name
self
end

def klass_suppress_errors
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel great... can you elaborate on why it's required?

(Do we have a more general "candidate associations that might be my inverse" lookup elsewhere that we can lean on here?)

@rails-bot rails-bot bot removed the stale label Feb 7, 2022
@rails-bot
Copy link

rails-bot bot commented May 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label May 8, 2022
@intrip
Copy link
Contributor Author

intrip commented May 11, 2022

will check as soon as I can, sorry

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.

ActiveRecord updates counters when it shouldn't
3 participants